Repository navigation
perf(studio): scan the preview document once per timeline derivation - #3007
Closed
miguel-heygen wants to merge 3 commits into
Closed
miguel-heygen wants to merge 3 commits into
miguel-heygen wants to merge 3 commits into
Conversation
… uses buildMissingCompositionElements looked its DOM hosts up with `doc.getElementById(existing.id)`, falling back to a whole-document `[data-composition-id]` query. But `existing.id` is the manifest's stableClipId — the author `id` when there is one, otherwise `data-hf-id` — and getElementById cannot see the second, which is how most generated clips are identified. The fast path therefore missed for nearly every clip and the fallback query ran per clip: 9,276 whole-document queries on a single open of a 3,000-clip project, essentially all of them finding nothing. Key one index on both halves of stableClipId (plus data-composition-id, which the fallback covered), built from the only elements that can change the outcome: those carrying data-composition-src or data-composition-file. Clips identified by data-hf-id now resolve, which they never did before. The tests pin both halves. Identity parity: over a generated project, every returned element is byte-identical to its input apart from compositionSrc. Size independence: twenty times the clips resolve through the same number of whole-document queries, where before the fix that count grew per clip (363 queries at 200 clips, against 21 now).
The runtime publishes element identity twice — clip.id in __clipManifest and node.id in __clipTree — and studio joins the two id spaces to build the timeline's parent map. Nothing pinned what either path emits, so a change to either fallback silently changed element identity, which studio reads as element replacement. Characterizes both paths for a clip with a DOM id, a data-hf-id only, a composition id only, and no identity at all, and records why the manifest cannot simply mirror the tree's __clip-N ordinal: the two traversals enumerate different element sets, so the Nth id-less clip of one is not the Nth id-less clip of the other.
Deriving timeline elements from a clip manifest called findTimelineDomNodeForClip once per clip, and each of those calls rebuilt the document's whole candidate set: a `[data-start]` query plus a four-selector `closest()` on every node it returned. On a 273-clip project that quadratic pass ran on every manifest message (~1/s) and was the largest task in the app — 99ms of `getTimelineDomNodes` alone across a ten-click window, with single frames stretching past 100ms. The scan is now a TimelineDomPass built once per derivation and dropped with it. It stays pass-scoped on purpose: the preview document mutates between manifests, so a cached scan that outlived one would resolve a clip to a detached node — selecting the wrong element, far worse than a slow scan. Claiming the resolved node moved into the resolver so a shared pass can never hand two clips the same element. Two per-selection costs in the property panel go with it, since every selection re-renders it: detectAvailableWeights ran nine synchronous `document.fonts.check()` calls per click and is now cached per family and cleared on `loadingdone`, and the font dropdown's ~1,500-entry catalogue — keyed on the selected family, so no memo could hold across clicks — is only built while the dropdown is actually open. Measured on a 273-clip / 40-track fixture, production React, headless Chrome, ten clip selections: main-thread script 388ms -> 261ms (-33%) idle, no clicks 648ms -> 297ms (-54%, 4x CPU throttle) longest frame 50ms -> 25ms 4x throttle p95 41.6ms -> 33.3ms, max 133ms -> 92ms
Collaborator
Author
|
Closing this stale performance branch. The repeated fallback candidate scan still exists, but the live caller has moved to timelineSyncHydration. The useful fix will be rebuilt narrowly there, alongside correct DOM identity binding, with current query-count and browser evidence. The old idle percentages will not be reused. Audit baseline: |
4 tasks done
Collaborator
Author
|
Verified narrow successor: #3681 |
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.
Deriving timeline elements from a clip manifest called a resolver once per clip, and every call rebuilt the entire candidate set — a
[data-start]query plus a four-selectorclosest()walk on every node it returned. Quadratic in clip count.On a 273-clip project this was the single largest task in the application.
Measured: idle main-thread script time 2,047ms → 882ms per 15s window (−57%), three runs per arm at 4× CPU throttle.
Why this shows up at idle
The derivation runs on a timer at roughly 1.3/s whether or not anyone is interacting. This is not a cost paid on click; it is a cost paid continuously, which is why the number above comes from a window with no input at all.
What this does NOT buy
Click latency is unchanged — 7.4ms median before, 8.3ms after. Inside noise.
That deserves stating plainly because the obvious framing of this change is wrong. An earlier investigation reported selection as janky at "p95 41.8ms, 53% of frames dropped". Both halves were artifacts: the frame metric counted 16.8ms frames as dropped against a 16.7ms threshold (i.e. it counted vsync jitter), and the p95 came from a sample short enough that a single first-click property-panel mount dominated it. Steady-state selection was always ~8ms.
The win here is real and it is the idle path. It is not a responsiveness win.
The scan is deliberately pass-scoped
Built once per derivation and dropped with it. It must never be cached across derivations: the preview document mutates between them, and a cached scan that outlived one would resolve clips to detached nodes — silently binding to the wrong element, which is far worse than a slow scan. There is a test asserting exactly this: a pass built before a mutation still resolves to the detached node; a fresh pass resolves to the live one.
Node-claiming moved into the resolver so a shared pass cannot hand two clips the same element.
One commit here is a guard, not a change
pin clip identity across the manifest and clip-tree pathsadds tests and no behaviour. It pins that the two paths compute clip identity differently and cannot be unified: the clip-tree path has an ordinal fallback the manifest path does not.Unifying them looks like an obvious cleanup and is a data-loss bug. It would change
clip.idfor clips that currently resolve to null, which downstream diffing reads as element replacement — silently dropping selection, expanded state, the keyframe cache, and lint findings. The test exists so the next person to notice the asymmetry finds out why before changing it.Note on a shared file
Touches
timelineElementHelpers.tsaround line 412; another PR in this sequence touches it around line 189. Disjoint, merges cleanly either way.Verification
packages/coregreen (1,437 tests),packages/studiogreen (3,277 tests).