perf(runtime): reuse one timing resolver per pass and derive duration only when the composition changes - #3824
Conversation
`createRuntimeStartTimeResolver` memoizes element start and duration lookups in WeakMaps, but `resolveStartForElement` and `resolveDurationForElement` each constructed a brand-new resolver per call and discarded it. The caches never served a second lookup, so a single pass re-walked the composition ancestry of every element, and per-seek work scaled with total timeline content rather than with what changed. `withTimingResolver(fn)` installs one resolver for the duration of a synchronous callback and restores the previous one in a `finally`, so a throw cannot leak the scope. Callers outside a scope keep today's construct-per-call behaviour unchanged. Three separate scopes, deliberately not one: - the media-cache build in `syncMediaForCurrentState` - the body of `syncTimedElementVisibility` - the media scan in `resolveMediaWindowDurationSeconds` They must stay separate. `syncRuntimeMedia` calls `el.load()` on the seek-past-buffered-range retry, which synchronously resets `el.duration` to NaN, and `resolveDurationForElement` reads `element.duration`. A cache spanning that write would serve the pre-`load()` duration to a post-`load()` read. Splitting at the write makes the staleness unrepresentable instead of merely handled. The third scope sits on `resolveMediaWindowDurationSeconds` rather than its caller `getSafeTimelineDurationSeconds`, which would look like the tidier boundary: that caller also invokes author-supplied `timeline.duration()` and third-party adapter `getInferredDurationSeconds()`, and foreign code inside a cache scope can mutate the DOM between two resolves. Resolver constructions per seek plus transport tick go from 15 per media element to a flat 4, and self time in `resolveStartForElementInternal` falls about 70% on a 79-video composition.
…changes
The runtime re-derived the composition's total duration on every animation
frame. transportTick called getSafeTimelineDurationSeconds unconditionally,
and deriving it scans every media element in the document and walks each
one's composition ancestry to resolve an absolute start and duration.
That value is a function of the composition, not of the playhead, so a
paused editor with nothing happening recomputed the same answer ~60 times a
second. On a 91-media-element composition the media scan ran 1.05 times per
frame while the editor sat idle and untouched.
Derive it once and reuse it until an input could have changed. The inputs,
and the signal that catches each:
- timing attributes edited (live editing, variables re-applied, the
runtime's own autostamping) -> MutationObserver with an attribute filter
- timed elements added or removed, including nested compositions that
mount asynchronously after init -> the same observer, childList+subtree
- media metadata arriving, or el.load() resetting duration to NaN, neither
of which mutates the DOM -> capture-phase media event listeners
- a timeline registered or lengthened in window.__timelines, a plain
object nothing can observe -> a registry signature compared on read
Observer records are delivered in a microtask, so the queue is drained on
read as well; otherwise an edit read back in the same synchronous block, the
ordinary live-editing case, would be served the pre-edit value.
The render path does not read the cache at all. Capture depends on the exact
duration and a frame rendered against a wrong one cannot be recovered, so it
pays the full derivation on every frame exactly as before.
Measured on the 91-media-element composition, idle and untouched: media
scans per frame 1.05 -> 0.05. Across 100 single-frame seeks: 2.28 -> 0.11
per seek, so seeking does not invalidate the cache either. The residual is
a separate per-20-tick caller in timeline.ts, untouched here.
terencecho
left a comment
There was a problem hiding this comment.
APPROVE — perf refactor with correct scope discipline; every claimed invalidation signal is wired and covered by a test that fails when the signal is removed. All required CI green, no concurrent reviews.
Author + provenance. Both commits authored+committed by miguel-heygen. Head 874a5a5e9e0682f866e3aba767d58a3966dcb04d. mergeStateStatus=BLOCKED (awaiting approval, mergeable=MERGEABLE). Diff: 3 files, +679/-75, all under packages/core/src/runtime/. No new deps, no public-export changes, no test deletions.
1. Composition-duration cache + invalidation surface — packages/core/src/runtime/init.ts.
- Cache lives in closure vars:
durationFloorsCache,durationFloorsRegistrySignature,durationFloorsObserver(init.ts:1005-1007). Per-runtime, cleaned up on teardown. MutationObserveratinit.ts:1035-1041sets all four invalidation options:childList: true, subtree: true, attributes: true, attributeFilter: DURATION_FLOOR_INPUT_ATTRIBUTES. TheattributeFilterincludessrcandid, so hot-swapping<video src=…>invalidates.- Capture-phase media listeners at
init.ts:1044-1046:document.addEventListener(eventType, invalidateDurationFloors, true)forloadedmetadata,durationchange,emptied. Third-argtrue= capture. Correct — media events don't bubble. - Pending-record drain on read:
init.ts:1067-1069—if (durationFloorsObserver && durationFloorsObserver.takeRecords().length > 0) invalidateDurationFloors();. This is what makes same-synchronous-block edit-then-read correct. - Render path bypasses cache:
init.ts:1061—if (renderCaptureSeekStarted) return deriveDurationFloors();. Flag set atrenderSeekentry (init.ts:2752), which is the framework-owned render dispatch, so producer-driven frames always get a fresh derivation. - Registry signature:
readTimelineRegistrySignatureiterateswindow.__timelinesand concatenatesid=duration;.getTimelineDurationSecondsis throw-safe (init.ts:810-816— try/catch → null), so a hostile authortimeline.duration()can't crash the signature read. - Cleanup registered via
runtimeCleanupCallbacks(init.ts:1046-1053) — disconnects observer AND removes every capture-phase listener. VerifiedruntimeCleanupCallbacksis drained atinit.ts:3696.
2. withTimingResolver — three scopes.
- Closure var + save/restore pattern with
try/finally(init.ts:466-476). Non-scoped callers keep the old fresh-per-call behaviour viatimingResolverFor(init.ts:481-484), which also correctly declines the shared resolver whenincludeAuthoredTimingAttrs=false. - Scope 1 wraps
refreshRuntimeMediaCacheinsyncMediaForCurrentStateand closes atinit.ts:2373— the followingsyncRuntimeMedia(...)atinit.ts:2384runs outside the scope. This is the load-bearing invariant (avoids caching pre-load()duration for a post-load()read). - Scope 2 wraps
applyTimedElementVisibilityinsidesyncTimedElementVisibility— pure DOM-read body, noawait, noel.load(). - Scope 3 wraps the media-window loop in
resolveMediaWindowDurationSeconds, deliberately NOT ongetSafeTimelineDurationSeconds(which also calls foreigntimeline.duration()/ adapters). Comment at diff:533-543 preserves this rationale so it doesn't get "simplified" back.
3. Once-per-scrub-pass / paused-frame avoidance.
- Old:
getSafeTimelineDurationSecondscalledresolveMediaDurationFloorSeconds()+resolveAuthoredCompositionDurationFloorSeconds()unconditionally per rAF. Diff replaces with a singleresolveDurationFloors()call (init.ts:1085-1086) that short-circuits on unchanged inputs. resolveAdapterDurationFloorSeconds()is deliberately still per-call — comment atinit.ts:1088-1091states the reason (CSS/WAAPI/Lottie animation objects can't be observed; short registered-adapter loop, not a document scan). Correct.
4. Test coverage.
init.test.ts"derived duration floor recomputation" (diff:24-274): 11 new tests. Non-invasive probe:Element.prototype.querySelectorAllspy on[data-composition-id][data-start]— reached only from a real derivation.- Invariance: asserts
derivationsafter startup is EXACTLY 0 for both 25 and 100 frames (diff:100-110). Stronger than "count invariant" — proves zero cache misses. - One test per invalidation signal: stale-then-fresh via
durationchange,el.load()NaN +emptied, timing-attr edit, clip-move, node-add, node-remove,window.__timelinesgrowth, same-sync-block edit+read, render-capture-bypass. All exercise the real cache; none use production test hooks. - Note: the PR body's "5 signals removed × N tests fail" table is coverage documentation, not a mechanically enforced matrix in the test file. Each signal does have at least one test that would fail if the signal were removed. Not blocking.
- Invariance: asserts
init.timingResolver.test.ts(diff:281-442, new file): two tests.- Constant resolver count: 2 vs 12 elements → equal count and ≤6 total. Non-vacuity noted (pass-through breaks it: 33 for 2, 183 for 12).
- Scope-boundary test:
mediaSpy.beforeSyncmutates the lead video's duration between scopes, asserts follower visibility uses fresh duration.
5. Perf-claim plausibility.
getSafeTimelineDurationSeconds-per-frame path: previously tworesolveMedia*derivations per rAF, now one signature-string read +takeRecords()when hot. The ~93× ratio (0.093ms → 0.001ms per read) in the PR body is consistent with the code change.- Resolver-reuse: WeakMap caches now shared across all resolves in one pass, so
2×(1+ancestor_depth)per element collapses to a small ancestor-count sum. Consistent with "flat 4 constructions per pass" test assertion.
6. Scope / blast radius.
- All changes in
packages/core/src/runtime/; no touch to adapters, engine, timeline.ts, media.ts. Comment at diff:743-748 explicitly flagstimeline.tsresolveMediaElementDurationSecondsclone as noted-not-fixed — separate PR needed for a semantic decision. Reasonable scope discipline. - No sibling-PR overlap with #3833 / #3834 visible in the file list.
7. CI. All required checks green at 874a5a5e…: CI (Test, Typecheck, Lint, Build, Fallow audit, runtime contract, Studio smoke, CLI smoke, Producer unit+integration, timeline viewport gate, Format, Semantic PR title), Player perf (load/fps/scrub/drift/parity), Windows render verification (studio-core, studio-engine-cli, plus generic), preview-regression, all 9 regression shards, CodeQL (actions/js-ts/python). CANCELLED items are stale duplicates (dedupe by name+workflow → latest is SUCCESS). No IN_PROGRESS, no FAILURE.
8. Concurrent reviews. gh api /pulls/3824/reviews → empty. No Vai/Via/Magi/human review to concur-with or defer-to.
— tai
vanceingalls
left a comment
There was a problem hiding this comment.
Verified against source at head commit 874a5a5e9 (worktree checkout, not just the diff). Ran the new tests directly rather than trusting the PR description.
Verdict: Approve
What I verified
1. Cache-invalidation surface (claimed in Slack, cross-checked against code) — accurate. resolveDurationFloors (packages/core/src/runtime/init.ts:1043) is bypassed entirely once renderCaptureSeekStarted is true (init.ts:1061, set at renderSeek: init.ts:2752), so a capture never reads a stale duration — this rules out the "exported video has wrong duration" failure mode I was asked to check for. MutationObserver (childList+subtree+attributeFilter) + capture-phase loadedmetadata/durationchange/emptied + a window.__timelines signature comparison are exactly as described, and takeRecords() is drained synchronously on every read (init.ts:1066) before the signature check, so an edit read back in the same synchronous block is fresh. I cross-referenced DURATION_FLOOR_INPUT_ATTRIBUTES (init.ts:994) against every attribute actually read by the resolvers it's supposed to cover (startResolver.ts:51-124, playbackRate.ts:11,28, and the media-start-basis read at init.ts ~line 720) — the list is complete, nothing is missing.
2. "Once per pass" / re-entrant-scrub race — the whole call chain (refreshRuntimeMediaCache in media.ts:58, syncTimedElementVisibility, resolveMediaWindowDurationSeconds) is 100% synchronous — no async/await anywhere in it. JS's single-threaded model means a second seek cannot interleave mid-scope, so the "rapid re-entrant scrub during an in-flight media probe" scenario I was asked to check for can't happen. I also confirmed the 3 scopes are genuinely sequential, not nested: syncMediaForCurrentState (init.ts:2335) closes scope 1 at line 2373 (restoring activeTimingResolver via finally) before calling syncRuntimeMedia (which may call el.load()), and only then opens scope 2 via syncTimedElementVisibility at line 2404 — the write genuinely sits between the two caches as documented.
3. Layer-rebuild / query-floor claim does NOT belong to this PR (important, not a blocker for #3824 itself, but flag before relying on it). Miguel's Slack numbers included "layers rebuild: document queries 171.6→10.8" and "walk time 15.3ms→5.4ms" attributed to a selector-batching change. This PR's diff touches exactly 3 files (init.ts, init.test.ts, init.timingResolver.test.ts, 679/-75, matches gh pr view) and contains no selector-batching or "layer walk" code at all — I grepped every querySelectorAll/querySelector call site in init.ts and none resembles a class-selector batching walk. That claim must be describing a different PR in the stack (likely #3833) or was conflated in the Slack summary. Worth correcting before it gets cited as verified against #3824.
4. Tests — read in full and actually ran them, not just name-matched. describe("derived duration floor recomputation", ...) at init.test.ts:3692 has 11 tests: the invariance test (init.test.ts:3763) genuinely asserts a flat count across 25 vs 100 frames (not a threshold), and each of the 4 invalidation-signal classes has a dedicated test that would fail if that signal were removed, matching Miguel's description exactly. init.timingResolver.test.ts has the constant-resolver-count test (non-vacuous: fails at 33/183 constructions if withTimingResolver is neutered — matches the claimed "~7 per element becomes flat") and the pre/post-load() staleness test. I built @hyperframes/parsers (missing dist/, blocking all runtime tests — pre-existing env issue, unrelated to this PR) and ran both files: 113/114 tests pass. The one failure (schedules WebAudio element gain from author volume without bridge volume, readFileSync is not a function) is a pre-existing, unrelated test hitting a node:fs externalization quirk in this sandbox's vitest environment — not touched by this diff, and GitHub CI (Test: runtime contract, Perf: scrub, Perf: drift, coverage, etc.) is green on this PR.
5. Regressions on other consumers — resolveStartForElement/resolveDurationForElement callers outside the 3 scopes (the CSS adapter's resolveStartSeconds callback at init.ts:2848, scheduleWebAudioForActiveClips at init.ts:3416) run with activeTimingResolver == null, so they fall back to constructing a fresh resolver exactly as before — verified these are sibling calls, not nested inside a scope. No behavior change for any caller outside the three documented scopes.
6. Edge cases — zero-media compositions short-circuit before opening a resolver scope (init.ts:860); pure text/image compositions fall through resolveMediaDurationFloorSeconds to null cleanly (pre-existing logic, untouched).
Nit: the PR's own "Noted, not fixed" section calls out that timeline.ts's resolveMediaWindowEndSeconds (timeline.ts:197) resolves start from the raw data-start attribute while init.ts's version routes through the media-start-basis resolution — I confirmed this divergence is real and pre-existing. Good call leaving it as a follow-up rather than silently consolidating it into this perf PR.
Invalidation surface is proportional to the actual inputs (4 genuinely independent, un-mergeable invalidation classes) — not overbuilt for what exists in this codebase.
— Vai
The defect
createRuntimeStartTimeResolver(packages/core/src/runtime/startResolver.ts) is a real memoizer: it allocates astartCacheWeakMap, adurationCacheWeakMap and avisitingSet, and resolving one element's start walks — and caches — its whole composition ancestry.resolveStartForElementandresolveDurationForElementinruntime/init.tseach built a brand-new resolver on every call and threw it away. The caches never served a second lookup.The consequence is not a constant factor. It means per-seek work scales with total timeline content rather than with what actually changed, because every element re-walks ancestry that a sibling just resolved:
resolveMediaCompositionContextcalls both wrappers, so two resolvers per call.refreshRuntimeMediaCache's two callbacks recompute the identical context and the identical absolute start.syncTimedElementVisibilitycosts2 x (1 + ancestor depth)resolves per timed element.resolveMediaWindowDurationSecondsscans every media element, and it runs on every rAF viagetSafeTimelineDurationSeconds, so most of that cost is paid while the editor sits paused and idle.The correct pattern already existed one file over:
runtime/timeline.tshoists one resolver for a whole payload build.The fix
withTimingResolver(fn)installs one resolver in a closure variable for the duration of a synchronous callback and restores the previous value in afinally, so a throw cannot leak the scope. Both wrappers use the installed resolver when one is present and otherwise fall back to today's construct-per-call, so every caller outside a scope is unchanged.Three scopes are opened:
syncMediaForCurrentStatesyncTimedElementVisibilityresolveMediaWindowDurationSecondsWhy three scopes and not one
This is the load-bearing design decision, and merging them would reintroduce a stale read.
syncRuntimeMediacallsel.load()on the seek-past-buffered-range retry (runtime/media.ts), which synchronously resetsel.durationtoNaN.resolveDurationForElementreadselement.duration. A single cache spanningrefreshRuntimeMediaCache->syncRuntimeMedia->syncTimedElementVisibilitywould hand the pre-load()duration to a post-load()read.Splitting at the write makes that staleness unrepresentable rather than merely handled — no invalidation hook, no observer, no dirty flag. A scope lives for exactly one synchronous call with no
awaitand no event dispatch inside, so nothing can mutate the DOM underneath it. There is a comment at the helper saying this, so the scopes do not get "simplified" back together later.The same rule decided scope 3's placement. It sits on
resolveMediaWindowDurationSecondsand not on its callergetSafeTimelineDurationSeconds, which looks like the tidier boundary: that caller also invokes author-suppliedtimeline.duration()and every adapter'sgetInferredDurationSeconds(). Foreign code inside a cache scope can touch the DOM between two resolves.For the same reason
seekStandaloneRegisteredTimelinesis deliberately left unwrapped, even though it resolves once per registered sub-composition timeline on every seek. Its loop callstimeline.totalTime()andtimeline.duration()— author GSAP code whoseonUpdatehandlers run synchronously. It is a genuine remaining win, but it needs its own argument, not this PR's.Measurements
A/B against a 79-video / 12-audio / 20-nested-composition composition, 200 single-frame seeks per run, Chrome CPU sampler at 100 microseconds, attribution restricted to the runtime bundle. Both arms are the same Studio preview with only the runtime bundle swapped by request interception; arms were run interleaved so both saw the same machine conditions.
Self time in
resolveStartForElementInternal, the function that does the ancestry walk, ms per seek:Median 0.232 -> 0.072 ms/seek, about -69%, and the separation is clean: the worst "after" run still beat the best "before" run. Inclusive time for the same function moved 0.58 -> 0.175 ms/seek.
createRuntimeStartTimeResolverself time dropped roughly 80%.Idle (paused editor, no input, same harness, 13.2 s window), which is where most of the
getSafeTimelineDurationSecondscost is paid:resolveStartForElementInternalinclusivegetSafeTimelineDurationSecondsinclusiveHonest caveats, because they matter for how much to read into the above:
getSafeTimelineDurationSecondshere. That it runs unconditionally on every rAF while paused is a real and separate problem, with its own invalidation argument. Out of scope for this PR.Tests
packages/core/src/runtime/init.timingResolver.test.ts, both driven through the publicinitSandboxRuntimeModular()->window.__player.renderSeek()boundary.createRuntimeStartTimeResolverand asserts the construction count for one seek plus one transport tick is identical for 2 and for 12 media elements, plus a small ceiling. Proven non-vacuous by neuteringwithTimingResolverinto a pass-through: the count becomes 33 for 2 elements and 183 for 12 (15 per media element) and the test fails. With the fix it is a flat 4.<video>whose duration is cached while the media cache is built, with a follower clip whose start is expressed relative to it; the video's duration is then changed between the scopes, standing in for theel.load()reset. Asserts the visibility pass sees the fresh duration. Proven non-vacuous by merging the scopes into one spanning scope: the follower resolves to the stale start and the assertion flips fromvisibletohidden.Gates
bun run test:hyperframe-runtime-ciinpackages/core: typecheck, preview-guard lint, runtime build, contract / behaviour / seek / duration-guard / parity / security suites, coverage, linter tests. 51 files, 1082 tests, all passing.bun run lintandbun run format:check: clean.bunx fallow audit --base origin/main --fail-on-issues: no issues in the 2 changed files.packages/playertests/perfscrub scenario fails its gate on this machine, and it fails the same way on unmodifiedorigin/main— worse, in fact (aggregateinline_p95223.9 ms on a pristine baseline checkout vs 57.2 ms on this branch), because the first of three runs is a cold-start outlier that dominates the aggregate under load. Pre-existing and environmental, not introduced here. Note also that its10-video-gridfixture has ~14 videos, so even when green it can only show no regression — the win here is a multiplier on element count and that fixture is too small to show it.Part 2: derive composition duration only when the composition changes
The problem
A paused editor with nothing happening burns CPU continuously, and a large share of it is the runtime recomputing a number that has not changed.
transportTickcallsgetSafeTimelineDurationSecondson every animation frame, unconditionally. Deriving that value scans everyvideo[data-start], audio[data-start]in the document and, per element, resolves an absolute start by walking its composition ancestry, then does the same for nested composition windows. The result is a function of the composition, not of the playhead — so on a paused, untouched composition it produces the same answer ~60 times a second, forever.Measured on a 91-media-element composition, paused and untouched: the media scan ran 1.05 times per animation frame. That is what a user feels as a warm laptop with an editor open and nothing happening.
The change
Derive the two DOM-derived duration floors once and reuse them until an input could have changed. Nothing about the returned value changes; only how often it is computed.
The honest part of this is the invalidation set, so here it is in full. Each input, and the signal that catches it:
MutationObserver,attributeFilterover the timing attributes the resolvers actually readchildList+subtreeel.load()resettingdurationtoNaN— neither mutates the DOMloadedmetadata/durationchange/emptiedlisteners ondocumentwindow.__timelines— a plain object nothing can observeThe bias is deliberate and stated in the code: a redundant derivation is a missed optimisation, a missed one is a wrong duration.
Observer records are delivered in a microtask, so the pending queue is drained on read as well (
takeRecords()). Without that, a caller that edits a timing attribute and reads the duration back in the same synchronous block — the ordinary live-editing case — would be served the pre-edit value. Taking the records suppresses the callback for them, which is exactly equivalent, since the callback only marks the cache stale.The render path does not read the cache at all. Capture depends on the exact duration and a frame rendered against a wrong one cannot be recovered, so once
renderSeekhas been called the derivation runs in full on every frame, exactly as before. The producer drives frames throughwindow.__player.renderSeek(...), so the flag is set before any captured frame.resolveAdapterDurationFloorSeconds()is deliberately left uncached: adapters infer duration from live animation objects (CSS/WAAPI/Lottie) that can change with no DOM mutation or media event to observe, and it is a short loop over registered adapters rather than a document scan.Results
Two studio previews, arms differing only by the runtime bundle (the harness request-intercepts the runtime and fails the run if interception never fires), interleaved.
The headline number is a count, not a duration. The machine was heavily loaded while measuring (1-minute load average 40–98), which makes every millisecond figure unreliable — but a count of work done is invariant to contention, and it is a stronger claim anyway.
Idle, paused and untouched:
Repeated across two passes each; identical to two decimal places. On the carousel the authored-composition scan likewise went from 1.00 per frame to 0.00. The residual 0.05 per frame is a separate per-20-tick caller in
timeline.tsthat this change does not touch.Across 100 single-frame seeks, confirming the cache does not thrash while scrubbing:
Does the invalidation cost as much as the recompute? No, and it is worth showing rather than asserting. Timed in-page, same realm, interleaved so contention hits both equally:
The read path is the registry signature plus a
takeRecords()drain. The derivation figure understates the real cost, since the in-page probe does not reconstruct a timing resolver the way the real derivation does, so treat the ratio as a lower bound.Wall-clock CPU was measured too and is not reported as fact: at load 40–98 the before/after renderer CPU figures moved in both directions and are pure noise.
Tests
packages/core:bun run test:hyperframe-runtime-cipasses end to end (typecheck, preview guards, build, contract, behaviour, seek, duration, parity, security, coverage, linter). The two arms that pin the invariants at stake here:Root
bun run lintandbun run format:checkare clean. 1091 runtime tests pass.11 new tests, and the two things they are built to prove:
Invariance, not a threshold. A threshold ("fewer than N derivations") passes on a small fixture and still degrades linearly in production. The test asserts that quadrupling the frame count does not change the derivation count at all. Reverting the cache in place turns that assertion into
expected 25 to be 100— 25 frames produced 25 derivations, 100 produced 100, exactly one per frame.Every invalidation signal is load-bearing. Each was disabled in turn to confirm a specific test notices:
childList+subtreeattributeFiltertakeRecords()drainThe
takeRecords()row is the one worth pausing on: it looks like defensive polish, and removing it breaks nearly every DOM-driven invalidation, because they all read back inside the same task.The counter is non-invasive — no production test hook. It counts calls to
[data-composition-id][data-start], which the runtime queries in exactly one place, reachable only from a real derivation.Noted, not fixed
timeline.tscarries near-copies of two functions this change touches.resolveMediaElementDurationSecondsis a true clone.resolveMediaWindowEndSecondsis not a safe merge: it resolves a clip's start from the rawdata-startattribute, while the runtime's version routes through the media-start-basis resolution. Those two can disagree. That needs a decision about which is correct rather than a silent consolidation, so it is called out here rather than folded into a performance change.