Repository navigation
fix(sdk): release the parsed film when a session is disposed - #6289
Merged
Merged
Conversation
A disposed session kept its whole parsed document and element caches. Anything that still referenced the session, such as a stale closure or React fiber, therefore kept the replaced film in memory, so memory grew with every film swap. dispose() now drops the document and caches.
A disposed session answers reads from an empty document. Outside production it now logs once, with a stack, so a caller still holding a stale session shows up instead of silently reading nothing.
Contributor
Edit accuracy: accurate 271 (base branch 271), smooth 138 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
Contributor
There was a problem hiding this comment.
Verdict: APPROVE · 0 blocker · 0 important · 0 nits · b0bf28bd
Reviewed at b0bf28bd, which includes the follow-up commit that adds the disposed-read warning. 1e7cf53e alone failed the comment ratchet. That is green here.
Scope
- Both files read end-to-end. Every document read goes through the
parsedgetter, so all post-dispose reads and dispatches hit the empty document and warn once outside production.dispose()is idempotent. - Studio paths that can touch a stale session (
useGsapPropertyDebounceunmount flush,sdkEditTransactioncandidates,useSdkSessionswap): edits on an empty document decline or fail. The persist queue is already guarded bydisposed, so nothing writes an emptied film to disk. - CI is green on this head.
session.dispose-release.test.tsran inSDK: unit + contract + smoke(2 tests).
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
Composition.dispose()now drops the parsed document and the element snapshot caches.Why
A disposed session kept its whole parsed (linkedom) document. Anything that still referenced the session, such as a stale closure or a React fiber's old props, therefore kept the replaced film alive, so Studio's memory grew with every film swap.
Measured
Headless Chrome, a generated fixture project (150 clips, 2 videos, 25 off-canvas elements), swapping between two films 25 times with no other interaction, heap read after a forced GC:
Retained copies of the parsed film HTML in a heap snapshot at swap 25: 29 before, 4 after (the live session plus interned strings). Live documents, DOM nodes and listeners were flat in both runs.
Not fixed here
Growth is halved, not gone: about 0.26 MB per swap remains. Stale render scopes of several memoized callbacks still chain to one another and keep a lower-case copy of each film's source and other per-swap data alive. That needs changes in several components, so it is a separate PR.
Test
session.dispose-release.test.tsopens ten sessions, keeps every disposed session referenced, forces a GC and requires every parsed document to be collected. It fails on main (10 of 10 documents retained) and passes with this change, three runs in a row.A disposed session now answers reads from an empty document instead of its old one, and outside production logs one warning with a stack the first time it is read, so a caller still holding a stale session can be found.