Repository navigation
fix(core): a paused preview lets go of the scenes it has unmounted - #7195
Merged
Merged
Conversation
Contributor
Contributor
There was a problem hiding this comment.
Approve at 81efde98. No blockers.
- Fails without the fix: The line is present. It names both cases of
lets go of each scene the preview mounts and unmounts…with their failure counts: 15 of 18 playing runs fail and the paused scrub fails every run wheninit.tsis reverted. queryCache: Dropping the per-entry revision and clearing the cache ininvalidateCompositionTimingCachesgives the same results as before. Every path that bumpscompositionTimingRevisiongoes through that function (the observer callback,takeRecordsinqueryForTransport, the media-event handlers,withoutOwn*Recordsand disconnect). A cleared map only ever holds results computed since the last invalidation. When the observer is off or a render capture has started, the query still bypasses the cache as before.neverStyledStarts→WeakMap: It's only ever read withget/setand replaced whole when the revision changes. Nothing iterates it, so the switch is safe.clipsNeverStyledVisible: The prune inretireScenecovers the paused case, which the playing-tick prune (clock.isPlaying()guard) never reached. TheisConnectedcheck on add closes the stale-list path. The prune is O(set size) per unmount, which is fine.- Test: Collecting garbage until the count stops falling doesn't weaken the check, because a scene that is still held survives every round. The added paused case is the one that reproduced on every run.
Nit (doesn't block): kept() returns next when next >= count, so a count that rises between rounds is reported at the higher value. That's conservative, just noting it.
CI at this head when I reviewed: PR captures and Comments had passed. CI, regression, Windows render verification and the rest were still pending or running, after earlier runs on this SHA were cancelled and requeued.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A preview that plays or scrubs past several scenes kept the unmounted ones in memory: after playing, until playback ran again; after a paused scrub, for good. The scene window swaps each scene that leaves it for an empty placeholder, but three runtime caches still pointed at the old scene elements, so every element of those scenes stayed alive.
This is also the merge-queue failure in
coreRuntimeBrowser.test.ts›lets go of each scene the preview mounts and unmounts…(expected 4695 to be less than 800): about 800 nodes is one scene, so that run held five.Why
Memory in a long preview grows with every scene passed and is only given back when playback runs again, or never after a paused scrub. The test that guards this fails about half the time under load, which ejects unrelated PRs from the merge queue.
Related work
None.
How
The first two are rebuilt only on their next read, and a stopped transport reads nothing:
queryForTransport's cache (selector → elements) is now emptied ininvalidateCompositionTimingCaches, which every timing change already goes through, unmounting a scene included. The revision stamp per entry is gone, since a cleared cache only ever holds current results.WeakMap, so it holds no element.The test gains a paused-scrub case (same five scenes, seeked without playing), and its node count now collects garbage until the count stops falling (at most six rounds) before it reads. One collection sometimes leaves a freed scene unswept, about one run in fifteen with the fix; a scene that is still held survives every round.
Test plan
nodesarray or the start cache's map. The count stayed flat across eight collections over 2 s, so nothing was waiting to be freed.coreRuntimeBrowser.test.ts: 82 of 82 pass. Coresrc/runtimetests: 2757 pass.oxfmtandoxlintare clean on both files.Fails without the fix:
lets go of each scene the preview mounts and unmounts, its captured audio includedfails withexpected 2811 to be less than 800(also 1207, 2009, 4560) wheninit.tsis reverted and the test is kept, 15 of 18 loaded cases;lets go of each scene … (its timeline built as the script runs, scrubbed while paused)fails withexpected 4586 to be less than 800every run.Size
A lone runtime fix: one memory leak through three element caches, and the queue flake it causes, small enough to revert by itself, with nothing open on the same code to carry it.