perf(studio): add timeline clip-window index primitive - #2702
perf(studio): add timeline clip-window index primitive#2702miguel-heygen wants to merge 1 commit into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
9fda27f to
9b91ca5
Compare
f387e18 to
6883a5e
Compare
9b91ca5 to
8a8a531
Compare
6883a5e to
99125c6
Compare
8a8a531 to
0a94dbf
Compare
99125c6 to
994baf5
Compare
0a94dbf to
14895e2
Compare
ef83c66 to
2dd814a
Compare
14895e2 to
e494530
Compare
2dd814a to
b09e9f9
Compare
6359e3f to
bde178d
Compare
b09e9f9 to
c2cae90
Compare
c2cae90 to
6f98580
Compare
Fallow audit reportFound 5 findings. Duplication (4)
Health (1)
Generated by fallow. |
6f98580 to
c588a64
Compare
c588a64 to
143d281
Compare
ee01158 to
108bd87
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — grade A — rubric CORRECT
Role in stack: D7 data-structure — immutable per-row clip-window index (sorted-by-start + prefixMaxEnd + byIdentity) that D8's virtualizer will query for visible time-range slices.
Root-cause fit: Correct shape for the access pattern. Timeline queries are build-once (per tracks snapshot) / query-many-per-frame (scroll, zoom, overscan). Sorted-by-start with a monotonically non-decreasing prefixMaxEnd gives O(log n + k) queries via upperBoundStart(end) - 1 then walk-backward while prefixMaxEnd[cursor] >= range.start — classic Cormen interval-stabbing pruning, beats a segment tree in constant factors, no rebalance cost. Per-row scoping matches how the virtualizer will fetch one row at a time.
Claims verified (file:line at HEAD 143d281b):
- Immutable per-row + prefix-max:
timelineClipIndex.ts:47-67(Object.freeze onbyStart,prefixMaxEnd,byIdentity, row entry, outerrows). - Overlap query returns projection order:
:110-118(final sort byordinal, then map to element). - Identity pins survive filter:
:105-108(byIdentity.get(identity) ?? []). - Half-open
[start, end)semantics consistent: interval-at-range.endexcluded (overlapsL90); zero-duration point atrange.endcorrectly excluded. Tested attimelineClipIndex.test.ts:22-34. - Negative-duration coerced to 0-duration point via
Math.max(0, element.duration)(L34) — no infinite-result pathology; explicitly asserted. - Non-finite guards on both
startandduration—Infinitycorrectly excluded. - 8 files, +399/-22. Matches PR body.
Adversarial findings:
- Nice-to-have — no property/fuzz test comparing query vs O(n) linear-scan reference; hand-crafted cases cover the edges but a 100-random × 100-query fuzz would raise confidence.
- Nice-to-have — in-place mutation of
TimelineElement.startafter index build leavesinterval.start/endfrozen but the returnedelementfresh. Safe under React/ZustandsetElements(new array each time), but worth a one-line JSDoc so future callers don't reach in and mutate. - Nice-to-have —
range.end < range.startsilently returns pins-only; consider an early assertion. - Nice-to-have — Big-O not documented in module JSDoc.
- D7→D8 handoff note: rebuild-only. Correct for build-once/query-many; D8 must memoize on
tracksidentity. Flag for D8 review. (Confirmed D8 does memoize — see #2703 review.)
CI state: All required green.
Suggested next step: APPROVE and land. Ask D8 to (1) memoize on tracks identity (done), (2) add one fuzz test at the D7/D8 boundary comparing queryTimelineClipIndex against a linear-scan oracle over 1k random intervals, (3) add a one-line JSDoc on createTimelineClipIndex noting the "snapshot at build" mutation contract.
— Review by Via
108bd87 to
e454d16
Compare
143d281 to
4f820c4
Compare
|
Addressed at
|
4f820c4 to
e5ce10a
Compare
e454d16 to
32554de
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-delta review at e5ce10ac59 (additive to the existing review).
Important — this PR does not yet window the runtime clip/ruler render path. Repository-wide use at this head finds no production caller of createTimelineClipIndex or queryTimelineClipIndex; Timeline never computes/passes renderTimeRange; and useTimelineTicks.ts:12-14 still calls generateTicks without the new range. The only non-test references to TimelineTimeRange are optional helper/prop definitions. Therefore the exact delta still scans and renders the complete horizontal project, despite the title/body saying visible clip windows are indexed and actor pins are preserved. If adoption intentionally belongs to the next stack PR, scope this PR honestly as index/windowing primitives; otherwise wire and integration-test the behavior here.
Important — the advertised visible-window bound is not guaranteed by the prefix-max scan. timelineClipIndex.ts:111-116 walks backward while the prefix maximum crosses the range start. One early full-duration clip makes that prefix maximum large for every later position, so a query near the end scans all preceding short, already-finished clips: O(total row clips) work to return one overlapping clip. The current “long-lived earlier interval” test has only two entries and the randomized oracle checks correctness, not visited work. Add an adversarial 50k shape and either use a structure that can prune both branches (for example an interval/segment tree) or explicitly narrow the complexity/performance claim.
The overlap boundaries, duplicate identities, pin union, projection order, fractional row keys, and randomized oracle are otherwise well tested.
Verdict: REQUEST CHANGES
Reasoning: The exact PR currently has no runtime performance effect, and its core query can still degrade to the full-row scan the PR claims to remove.
— Magi
32554de to
bd0dccf
Compare
e5ce10a to
06c0f1d
Compare
|
Resolved at Scope is now explicit in both the commit/PR title and body: D7 adds the clip-window index primitive only. It has no production caller and no runtime render effect at this exact PR; #2703 is the immediate child that wires the primitive into The prefix-max backward scan is replaced with balanced max trees:
This prevents one early full-duration clip from forcing a scan through every already-ended clip. The complexity claim is narrowed to the implemented bound: A new adversarial test builds 50,000 clips—one early full-duration clip and 49,999 short ended clips—then queries a late window. The complete clip-index suite, randomized oracle, full Studio suite (291 files / 3,195 tests), and exact-tip 50k Chromium gate pass. |
06c0f1d to
83f737a
Compare
0690067 to
9f3ef4a
Compare
83f737a to
3a8a488
Compare
9f3ef4a to
a451e13
Compare
3a8a488 to
e2d7d99
Compare
a451e13 to
ca7d212
Compare
e2d7d99 to
029971e
Compare
029971e to
65a82ab
Compare
ca7d212 to
5fc5ef6
Compare

What
Add the immutable per-row clip-window index primitive consumed by D8.
Exact-PR scope
This PR deliberately does not call the index from
Timeline, pass a render range, or change runtime rendering by itself. Runtime horizontal clip/ruler windowing and actor-pin wiring land in the immediate child PR, #2703.How
Construction is
O(n log n)from sorting. The overlap tree walk isO((k + 1) log n)forkmatches; identity-pin lookup is proportional to pinned matches, followed by sorting the unique result set back into projection order.Test plan
Review fixes are included at head
06c0f1df5.