Repository navigation
[ui] Clean up retainers of traces - #176
Conversation
The map from workspace to timeline element exists so that a resize applies to the timeline it was made on. But a workspace outlives the page showing it, and is shown by a new page each time it is opened, so holding the element strongly kept every page a workspace had ever been shown by: the detached DOM, the mithril event dictionary left on it, and the track tree view that `onCanvasRedraw` closes over. The element is now held weakly. Nothing reads it without first checking that it is still connected, so a page that has gone was always handled; it just could not be collected.
cdamus
left a comment
cdamus
left a comment
There was a problem hiding this comment.
Very nice! Seems to work:
I really only have nits to offer in-line in the comments. Nothing to block a squerge.
Although I would like to know whether the tickmark_panel change actually fixes a real problem (see comment in-line): the WeakMap half of the explanation does not hold.
Process nit: the first commit bundles three unrelated, separately revertable fixes under an empty message body, while the second commit has a thorough one. A commit message in that same style would be nice. The new comments in track_tree_view.ts and TimelineSync also wrap at ~100 columns against 80 elsewhere in those files (prettier doesn't reflow comments, so it passes --check).
| ctx.trash.defer(() => { | ||
| this.disableTimelineSync(this._sessionId); | ||
| this._ctx = undefined; | ||
| // These outlive the trace otherwise. The timer is the worst of them: it is a root in its own |
There was a problem hiding this comment.
The fix is right, but I'd give this._chan?.close() at least equal billing. An open BroadcastChannel with an onmessage handler is itself a GC root held by the browser, and that handler is this.onmessage.bind(this) → the plugin instance → _ctx. Dropping the reference without closing would not have released it, so it isn't just tidiness, yet the comment doesn't mention the channel at all while calling the timer "the worst of them".
There was a problem hiding this comment.
Added tougher guards here, not just against what I'd happened to see leaking.
|
|
||
| onremove() { | ||
| this.interactions?.[Symbol.dispose](); | ||
| // Releases the perf stats container registered in `oncreate`. `PerfManager.containers` lives on |
| */ | ||
| export function applyTrackShellWidth(trace: Trace, timeline: HTMLElement) { | ||
| timelines.set(trace.currentWorkspace, timeline); | ||
| timelines.set(trace.currentWorkspace, new WeakRef(timeline)); |
There was a problem hiding this comment.
Optional: this runs on every render and now allocates a fresh WeakRef each time. Guarding it would mirror the "setting the same value again is a no-op" comment three lines below:
if (timelines.get(trace.currentWorkspace)?.deref() !== timeline) {
timelines.set(trace.currentWorkspace, new WeakRef(timeline));
}Pair each of the plugin's roots with its undo at the point it is made, rather than in a list at the end that a later addition can miss. Clear the channel's message handler as well as closing it: an open channel with a listener is held by the browser, so dropping our reference to it releases nothing. Stop re-wrapping the timeline element in a fresh WeakRef on every render.
Neither half of this earned its place. `WeakMap` has ephemeron semantics, so a value referencing its own key does not pin the entry: the cached track was already collectable, and traces are still released with the `delete` gone. The disposal was a drive-by fix for an `AsyncDisposable` that upstream never disposes, and it cannot complete. `AsyncDisposableStack` awaits its disposers one at a time, and the first await never resolves, because disposing the engine closes the socket without rejecting pending queries. So only the first of the three tables is dropped, and the suspended unwind leaves `engine.pendingQueries` holding a continuation that reaches back to the trace: a path from engine to trace that did not exist before. The tables die with the shell anyway.
cdamus
left a comment
cdamus
left a comment
There was a problem hiding this comment.
Thanks for the changes! 🚀
2 participants
Welcome to Perfetto!
Make sure your PR has a bug/issue attached or has at least
a clear description of the problem you are trying to fix.
This fixes a few cases where views / utilities would retain reference to a trace with respect to which they were created, even after that trace was disposed.
Together with https://github.com/android-graphics/sokatoa/pull/6144, it should ensure that
_TraceImpl's are GC'ed in the following sequence:Note
The last step is required because Monaco's context implementation attaches a listener to the most-recently focused node. If you don't focus anything else, the Omnibox input is retained, and it in turn holds listeners that refer to the
_TraceImpl. I decided not to do anything about that on the Sokatoa side, because (1) several attempts failed, and (2) it is transient and would be cleared by normal app use - after closing a capture, you'd probably eventually do something that focused something else.For more details please see
https://perfetto.dev/docs/contributing/getting-started