perf(studio): define timeline viewport budgets and fixtures - #2696
perf(studio): define timeline viewport budgets and fixtures#2696miguel-heygen wants to merge 1 commit into
Conversation
fa190d8 to
d49c381
Compare
4762c45 to
bc1c49e
Compare
d49c381 to
2540026
Compare
bc1c49e to
2cbd849
Compare
2540026 to
a26435d
Compare
2cbd849 to
2469850
Compare
a26435d to
5d7f12d
Compare
2469850 to
cbd22db
Compare
5d7f12d to
b8882aa
Compare
cbd22db to
ea7b955
Compare
b8882aa to
f411080
Compare
0241316 to
4033f5f
Compare
4033f5f to
e9360d1
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Clean foundation PR — pure additions of budgets + diagnostics + fixtures, no behavior changes to the timeline. The deterministic-generator contract holds up (pure indexed math, no Date.now/Math.random/async), the fixture stability test at useStudioTestHooks.test.tsx:37-64 proves round-trip equality on 50k identities, and the "128-roots-per-row envelope" math in fixtureTrack (timelinePerformanceFixture.ts:53-60) is documented and enforced by the expect(Math.max(...perTrack.values())).toBeLessThanOrEqual(128) assertion. Ready as the contract D2..D8 will be measured against.
Concerns
-
Boundary asymmetry in
getTimelineResourceBudgetStatus—timelinePerformanceDiagnostics.ts:101-103boundsclipRootsandclipRootsPerRowinclusively (<=) butdescendantsstrictly (<). The test title at :61 flags this as intentional ("including the strict descendant boundary"), and I don't think it's wrong — but the source has no comment explaining why the descendant ceiling is strict while the clip ceilings are inclusive. Someone touching these three lines later will see the mixed operators and normalize them "for consistency," silently loosening the descendant guard. A one-line source comment on line 103 documenting the intent would prevent that. -
Dev-only-branch imports may not tree-shake cleanly —
useStudioTestHooks.ts:3-13importscreateTimelinePerformanceFixture,readTimelinePerformanceDiagnostics, andTIMELINE_VIEWPORT_BUDGETSat top level; the only consumption is inside theif (!isDev || …) return;branch wrapped in try/catch (useStudioTestHooks.ts:54-61). Vite replacesimport.meta.env.DEVwithfalseat build time so the branch dies, but with the try/catch shell and theObject.freezeside-effect calls in the budget/fixture modules, esbuild/rollup DCE is not guaranteed to elide the imports from the prod chunk. Grep confirms no other prod consumers, so a static-import graph analysis in the next PR (vite build --reportor similar) would verify. Not a blocker — worst case is ~2KB of dead code in prod — but worth confirming before the file-size gate absorbs it as new baseline.
Nits
readNonNegativeNumberattimelinePerformanceDiagnostics.ts:24-27silently returns0on unparseable input (e.g. a writer emittingdata-timeline-cache-bytes="NaN"shows up as 0 bytes cached, which looks healthy to a budget check). For a diagnostics module whose purpose is exposing violations, a dev-only warn on unparseable input would help catch writer bugs before they land as false-clean budget signals.countPostersat :29-42 similarly drops unknowndata-timeline-poster-statevalues silently (if (state && state in counts) …). Same class of silent-failure as above — a typo'd poster state disappears from the counts with no signal.timeOverscanViewportRatio: 0.5attimelineViewportBudgets.ts:57is not in the<=1validator list at :115-119 (unlike the three other ratio fields). If that's intentional because overscan >100% is conceivable, a source comment would prevent someone from adding it "for consistency" later.dense-shortatelementCount: 1_000infixtureTracknever reaches the dense branch — all 1000 indices satisfyindex < TRACK_COUNT, sotrack = index(one clip per track). The profile is only actually dense at 50k. A// dense-short only diverges from non-dense profiles at 50kcomment attimelinePerformanceFixture.ts:54would clarify.
Questions
-
Budget calibration derivation — the numeric values (
maxMountedClipRoots: 512,interactionP95Ms: 50,frameIntervalP95Ms: 33.3,memoryReturnToleranceRatio: 0.15, etc.) look like a post-virtualization target contract, not the current-main baseline. The stack screenshot shows 3018 → 20 mounted clips post-Family-D, so the 512 ceiling is comfortably above the post-D state but well below the current-main baseline. Is the design doc explicit about the numeric derivation, or should reviewers treat these as reasonable-failure ceilings that assume Family D lands intact? (I couldn't fetch the heygenverse doc — 403 without SSO from here.) -
CI wiring intent — no
.github/workflowschanges here, so these budgets are enforced by whatever test framework consumes them locally / in Fallow. Are D2..D8 planning to wire budget-hit checks into required CI (via thewarmupRuns: 3, measuredRuns: 5, requiredPassingRuns: 4quorum), or is this staying as a local-run contract?
What I didn't verify
- Tree-shaking of the dev-only fixture module in prod builds (would need a
vite build --reportrun). - The heygenverse design doc for numeric-budget derivation (auth-gated, 403).
- Downstream stack behavior — D2..D8 haven't landed here, so budget calibration is a future check.
- The reported perf numbers (−99.3% / −93.3% / −35.3%) come from the stack, not this PR — this PR only encodes the contract they'll be measured against.
— Review by Rames D Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — grade A — rubric CORRECT
Role in stack: D1 base — budgets + diagnostics + fixtures; no behavior change; contract for D2-D8.
Root-cause fit (contract fit): The budget module owns every ceiling (DOM, media, latency, memory, quorum) as a frozen singleton with resolveTimelineViewportBudgets(overrides) for per-test hard-forks — no global mutation, so downstream PRs cannot silently loosen the contract. The 512 mounted-clip / 5,000-descendant / 4-of-5 quorum ceilings sit ~45% below the -99.3% target claim, giving virtualization work headroom without letting a regression slip through.
Budget calibration check:
- mounted rows: none (implicit via overscan=4 + 128/row) — OK
- mounted clips: 512 total, 128/row — tight vs -99.3% target
- descendants: 5,000 (strict
<) — snug given 512×~10 = 5,120 - interaction latency: 50/75ms constrained — matches INP guidance
- frame cadence: 33.3/50ms — 30fps/20fps floor
- long tasks: 50ms — PerformanceObserver default
- memory return: 0.15 ratio — reasonable GC-noise tolerance
- run quorum: 3 warmup + 5 measured + 4 required — validated by
requiredPassingRuns > measuredRunsguard
Claims verified (file:line at HEAD e9360d10):
- 691/-0, 7 files: 125+36+85+115+157+48+125 = 691 (files-list).
- Deterministic fixtures: no
Math.random/Date.now/cryptointimelinePerformanceFixture.ts:1-157. - Dense-short coverage:
fixtureStartuses(index % 128) * 0.5on 1.5s clips over 120s duration — genuinely dense (timelinePerformanceFixture.ts:63). - Dev-gated hook:
import.meta.env.DEV === trueguard + cleanupdelete window.__studioTest(useStudioTestHooks.ts:52-92). - Tests exercise budgets:
timelineViewportBudgets.test.ts:38-46asserts override rejection for negative, NaN, quorum-inversion, ratio>1. - 50k determinism:
useStudioTestHooks.test.tsx:36-59compares two independent generations byte-equal.
Adversarial findings:
- nit —
getTimelineResourceBudgetStatususes<for descendants but<=for clipRoots/perRow (timelinePerformanceDiagnostics.ts:88-92); documented as "strict boundary" in test, but asymmetry is easy to trip on downstream — worth a code comment. - nit —
maxClipsInOneRowgroups clips without a[data-timeline-row]ancestor under a sharednullbucket (timelinePerformanceDiagnostics.ts:44-50), over-counting orphans; benign today, but D2-D8 must preserve the row-ancestor invariant. - nit — silent
try/catch { isDev = false }aroundimport.meta.envaccess (useStudioTestHooks.ts:56-60); defensible for non-Vite envs, but no telemetry if the DEV flag disappears in a future toolchain move. - Fixture cover gap — only dense-short is byte-stability-asserted at 50k (
useStudioTestHooks.test.tsx:37); long-overlap / composition-heavy / remote-unsupported / keyframe-heavy-expanded get only 1k coverage.
CI state: All 27 required green. No red required checks.
Suggested next step: Land D1 as the frozen contract. Recommend a follow-up in D2 to (a) add a code comment on the intentional < for descendants, (b) extend the byte-identity 50k assertion to the other four profiles, (c) confirm D2-D8 never leave clips without a data-timeline-row ancestor so maxClipsInOneRow stays truthful.
— Review by Via
miguel-heygen
left a comment
There was a problem hiding this comment.
Single-PR review of #2696 only at e9360d1048fb7d366c90d5a5021a687069e23354. I audited all seven changed files end-to-end against main; I did not review or rely on D2–D8.
The shape is strong where it is connected: timelineViewportBudgets.ts:49-124 gives the contract one immutable owner, timelinePerformanceDiagnostics.ts:61-93 recomputes from the DOM instead of retaining cleanup-sensitive counters, and timelinePerformanceFixture.ts:31-156 uses deterministic indexed generation without clocks/randomness. Rames/Via already covered the strict-descendant asymmetry, dev-only tree-shaking, unknown diagnostic values, and breadth of 50k profile coverage; the findings below are additive.
Blockers
-
Live row diagnostics are disconnected from the real Timeline DOM.
timelinePerformanceDiagnostics.ts:44-49,76depends on[data-timeline-row], but the actual row wrapper atpackages/studio/src/player/components/TimelineLanes.tsx:149-157emits no such attribute. Repository-wide, the marker exists only in this reader and its synthetic unit test. A real Timeline therefore reportsmountedRows: 0, and every clip is grouped into the sharednullbucket, makingmaxMountedClipRootsInOneRowequal the total clip count. The PR also claims a mounted-row budget, buttimelineViewportBudgets.ts:1-43has no mounted-row ceiling andgetTimelineResourceBudgetStatus()attimelinePerformanceDiagnostics.ts:96-104has no row result. Add the production marker (or derive from real structure), an enforceable row budget/status, and an integration test that mounts the real Timeline rather than hand-authoring the marker. -
An absent Timeline is reported as fully within budget.
readTimelinePerformanceDiagnostics()intentionally returns zeroes when no root is mounted (timelinePerformanceDiagnostics.ts:66-93), whilegetTimelineResourceBudgetStatus()ignorestimelineRoots(:96-104). At this head,{timelineRoots: 0, ...zeroes}returns all three status flagstrue. A read taken before mount or after a render failure can therefore become a false-green strict run. Make root validity part of the status (normally exactly one Timeline), or expose a strict reader that rejects an absent/ambiguous root; pin the missing-root failure path.
Important
-
Fixture loads are not isolated from prior project state.
useStudioTestHooks.ts:71-86shallow-merges the fixture fields but preservesisPlaying, work-area points, clip manifest/parent map, pending requests, beat/edit state, and other project state that the store's own reset clears atplayerStore.ts:517-546. The same fixture can therefore render/measure differently depending on what Studio had loaded first (at minimum, an already-playing project keeps advancing after the load). Install a reset-equivalent fixture state in the same singlesetStatetransaction and test from deliberately dirty state. -
Run-quorum validation permits vacuous/non-executable contracts.
timelineViewportBudgets.ts:99-123accepts{ measuredRuns: 0, requiredPassingRuns: 0 }and fractional run counts because it checks only finite/non-negative andrequired <= measured. Require integer counts,measuredRuns >= 1,requiredPassingRuns >= 1, and a non-negative integer warmup count; add the boundaries totimelineViewportBudgets.test.ts:40-47. -
Runtime string validation trusts inherited object keys.
timelinePerformanceFixture.ts:42-49indexes a normal object and checks truthiness, so the browser-facing test hook acceptsprofile: "__proto__"/"constructor"and installs elements withduration: undefinedandstart: NaN. The sibling poster parser usesstate in countsattimelinePerformanceDiagnostics.ts:37-40, which also accepts inherited keys. UseObject.hasOwn(...)at both contract boundaries and add prototype-key regressions. -
The 50k “determinism” test does not cover the 50k output.
useStudioTestHooks.test.tsx:47-63compares the summary, elements 0–2, and the last element—49,996 middle elements and all profile-owned maps/sets are unchecked. A stable hash/full structural comparison would pin the reproducible-fixture claim without retaining another copy indefinitely.
CI
The exact head is not green: Preview parity hit its 20-minute job limit while installing ffmpeg, before Chrome or the parity check ran; preview-regression then failed because the dependency job was cancelled. This appears infrastructure/non-attributable—the PR does not touch that workflow—but the required gate still needs a successful rerun. Build, lint, format, typecheck, Studio/full tests, Windows, Fallow, file-size, CodeQL, and player-perf are green.
I am posting COMMENT rather than a formal changes-requested state because this PR is authored by the same shared miguel-heygen GitHub identity. Treat the two contract gaps above as merge blockers.
Verdict: COMMENT (With fixes)
Reasoning: The intended measurement foundation can currently report fabricated row metrics and pass an unmounted Timeline, so it is not yet a trustworthy gate for the later virtualization work.
— Magi
e9360d1 to
09b469a
Compare
|
Addressed at
Targeted contract/store tests and the complete Studio suite/typecheck pass. |
|
Magi review resolution update: every item labeled as a follow-up is integrated into D1 itself at
The two blockers are also fixed in D1: real lane rows emit |
a186ca4 to
f158e6c
Compare
f158e6c to
c458578
Compare

What
Define the measurable timeline viewport contract before changing rendering behavior.
How
TimelineLanesrow shell and require exactly one mounted Timeline, so a pre-mount or crashed run cannot pass on all-zero diagnostics.This is D1 of Family D. The permanent two-state CI workflow lands later in the same stack in #2884.
Test plan
Review fixes are included at head
09b469a31.