Repository navigation
perf(studio): a paused editor stops redrawing its overlays sixty times a second - #3846
Conversation
90c20ab to
d9081f0
Compare
The composition rect, the selection and hover boxes, the off-canvas indicators and the snap guides each owned an animation-frame loop that re-armed unconditionally. On a paused, untouched editor that is four callbacks per frame reading layout, and the snap-guide loop writes style on every one of them, so the compositor kept committing 55 frames a second with nothing moving. None of the four has a clock of its own; each only changes when something observable happens. They now share one loop that runs at full rate for a moment after a pointer, key, wheel, scroll, resize or visibility change, after a preview message reporting a frame the overlays have not drawn, or after a mutation inside the preview document, and otherwise polls four times a second so a wake source nobody thought of costs a quarter second of staleness rather than a frozen overlay. The frame comparison on preview messages is load-bearing rather than an optimisation: the paused preview posts an unchanged status message every 80 ms so any listener can confirm its position, and treating that as news held the overlays at 60 fps for the life of the tab.
The shared frame loop re-armed after running its subscribers, so a subscriber that threw took down the loop and its idle-poll safety net permanently. The four loops it replaced each re-armed first, which kept that failure to the one overlay that caused it. It now re-arms before running anything and isolates each subscriber, rethrowing out of band so the error still reaches the page's error reporting. The wake also sat outside the recognised-message check, so postMessage traffic from an extension, devtools or any other embed on the page held the overlays awake for 400ms at a time. Only the preview's own messages wake it now, and a repeated paused status post still does not. useMotionPathData had the same unconditional loop and is live whenever a keyframed element is selected; it joins the shared one.
d9081f0 to
1281ca0
Compare
terencecho
left a comment
There was a problem hiding this comment.
APPROVE at 1281ca0cd9efa5b95658960ad000c705f5bda626 — four editor-overlay animation-frame loops (plus a fifth, useMotionPathData) unified onto one shared loop that parks on 400ms of idle silence and wakes only on preview-authored events; the "any window message wakes at 60fps for the tab's life" and "one throw kills all four" edges the independent pre-review pass caught are both structurally fixed. Re-stamp at the retargeted head after Miguel cherry-picked the two PR commits onto main.
Author + scope. miguel-heygen (trust-listed). Head 1281ca0cd. Base main (merge-base f7d2260f9). +408/-23 across 7 files, all under packages/studio/src/components/editor/. Two commits on top of main: e25a40e19 (loop + 4 migrations + tests) and 1281ca0cd (isolation fix + wake-filter + 5th-loop) — cherry-picks of the pre-retarget 35a897ee2 / d9081f03d, byte-identical PR-file content (verified by git diff d9081f03d..1281ca0cd -- packages/studio/src/components/editor/{SnapGuideOverlay.tsx,offCanvasIndicatorRefresh.ts,overlayFrameLoop.test.ts,overlayFrameLoop.ts,useDomEditCompositionRect.ts,useDomEditOverlayRects.ts,useMotionPathData.ts} = empty). All prior verification carries.
Shared loop primitive
overlayFrameLoop.ts:146 — subscribeOverlayFrame(update) returns unsubscribe; last-out tears down. AWAKE_MS = 400 at L28 (documented: post-click CSS transition moves preview ~200ms without further events); IDLE_POLL_MS = 250 at L30 exported so tests compute expected bounds. runFrame re-arm ordering is frameId = null; schedule(); at L81-86 BEFORE subscriber iteration at L87-97 — a subscriber throw cannot brick the loop.
Four overlays migrated (+ 5th)
Old raf = requestAnimationFrame(update) bodies removed; replaced with return subscribeOverlayFrame(update):
SnapGuideOverlay.tsx:116useDomEditCompositionRect.ts:62useDomEditOverlayRects.ts:264(rafPausedRef early-exit preserved inside update)offCanvasIndicatorRefresh.ts:140-142(markDirty at L91 also callsrequestOverlayFrames()so preview-doc mutations wake the loop when top-frame events can't see them)- 5th:
useMotionPathData.ts:157—return subscribeOverlayFrame(tick), old raf variable gone.
Repo-wide requestAnimationFrame( grep under packages/studio/src: every survivor is (a) the shared loop itself, (b) the deferred 6th loop manualEdits.ts:178,181, or (c) unrelated subsystems (timeline/player/captions/one-shot focus/measurement — no overlay-poll shape). No unexplained survivor.
Paused-heartbeat frame-number dedupe (the whole reason v1 did nothing)
onPreviewMessage at overlayFrameLoop.ts:69-76: if (frameNumber === lastPreviewFrame && playing === lastPreviewPlaying) return;. Guarded on data.source === "hf-preview" && data.type === "state" at L67-69. Missing/non-numeric frame becomes null at L72 — shape drift falls to the wake side (fail-safe). isPlaying captured too, so paused→playing flips wake. Payload shape matches runtime contract RuntimeStateMessage.frame: number, isPlaying: boolean at packages/core/src/runtime/types.ts:44-51; poster is init.ts:2580-2595 at bridgeMaxPostIntervalMs.
Test ignores the paused heartbeat, which repeats a frame the overlays already drew (overlayFrameLoop.test.ts:114-129) walks 12 repeated previewState(7) beats at 80ms and asserts runs ≤ ceil(960 / IDLE_POLL_MS) + 1. Reverting the dedupe flips this red.
Wake-source filter
event.data.source === "hf-preview" gate at overlayFrameLoop.ts:61-78 — extension/devtools/embed traffic no longer wakes. Tests does not wake for a message that is not the preview's (:174-185) and wakes for a preview message that is not a state post (:187-200) both present. Note: string-on-payload, not event.source === iframe.contentWindow — an extension could still forge {source:"hf-preview"} and win 400ms of full-rate. Bounded nuisance ceiling, no correctness impact; called out below.
Fault isolation (Via's Q3a)
Try/catch per subscriber at L88-97; re-throw via queueMicrotask(() => { throw error }) at L93 — escapes to the event loop and surfaces to window.onerror. Test keeps every other subscriber running, and the loop alive, when one throws (:143-172) asserts: survivor subscribers ran on every frame (runs.a === 5, runs.c === 5), every failure was surfaced (rethrown.length === 5), loop still requesting frames. Reverting either the try/catch OR the re-arm-before-run order flips this red.
Wake-cost sharing (Via's Q3b)
Dispatch is unconditional at L87-97 — every subscriber runs every tick. Each of the 5 early-returns cheaply via ref-existence checks (!iframe || !overlayEl, rafPausedRef.current, !selector). No demonstrably heavy per-tick predicate one subscriber can force onto others. Non-blocker.
6th-loop extension surface (Via's Q3c)
subscribeOverlayFrame(update: () => void) takes just a poll fn — a subscription with different wake sources (e.g. playing-state changes for the deferred 6th loop) is absorbed by that subscriber's own handlers calling exported requestOverlayFrames() at L114-121, which is exactly what offCanvasIndicatorRefresh.markDirty does at L86-92. Not shape-locked to the current 4.
Park/resume race (Via's Q2)
No dedicated interleaving test, but boundary handling is static-sound: requestOverlayFrames unconditionally cancels idleTimerId and calls schedule() (L114-121); schedule() guards on frameId != null || idleTimerId != null before scheduling (L102) — no double-schedule; runFrame clears frameId first (L81), then reschedules, then iterates — a wake mid-subscriber either finds frameId != null (next tick already booked) or idleTimerId != null (cancelled + reschedule to rAF). Adding one 3-line interleaving test (schedule idle → dispatch wake → assert next tick is rAF not delayed setTimeout) would harden against future edits. Non-blocker.
manualEdits.ts (6th loop) — untouched
startStudioManualEditPlaybackReapply at packages/studio/src/components/editor/manualEdits.ts:168-181 still owns win.requestAnimationFrame(tick). Miguel's deferral honored.
Teardown + listener symmetry
attachListeners() at L123-130: 9 window events (WINDOW_EVENTS, L40-50) + message. detachListeners() at L132-139: same 9 + message. Symmetric. Unsubscribe path (L152-163) fires teardown + cancels rAF + clears idle timer at subscriber-count 0. Per-subscriber MutationObservers (e.g. offCanvasIndicatorRefresh L110-116, 140-145) are owned + disconnected by the subscriber itself — correct separation. Test stops entirely, and stops listening, once the last subscriber leaves (:202-214) pins it.
Explicit wake API
export function requestOverlayFrames() at L114-121 — public wake; used by offCanvasIndicatorRefresh.markDirty at :86-92 so preview-doc MutationObserver ticks wake the loop when top-frame events can't see inside the iframe. Test wakes on an explicit request (:131-141) pins it.
Retarget mechanics
Old head d9081f03d sat on top of #3845's branch perf-park-runtime-transport. #3845 landed on main via squash-merge (d4fba55a4), so a straight rebase would have replayed #3845's already-merged commits and shown DIRTY. Miguel cherry-picked only this PR's two commits onto fresh main → new SHAs e25a40e19 + 1281ca0cd, byte-identical PR-file content. Correct handling for a squash-merge-below-you retarget.
CI (retarget)
Full CI now running on main base — no longer the reduced-matrix stacked-child shape. At review time: Detect changes / Semantic PR title / Format / Lint / Fallow audit / File size check / SDK: unit+contract+smoke / player-perf / regression preflight / preview-regression preflight all SUCCESS; Windows render + Preview parity + Producer:unit / Producer:integration / Typecheck / Test:runtime contract / Studio:load smoke / Studio:timeline viewport gate / CLI smoke / Build IN_PROGRESS. APPROVE is keyed to code (unchanged); GitHub's required-checks gate keeps the merge honest and auto-merge doesn't fire until they land green.
Non-blocking observations (unchanged from prior review)
- Wake-source filter is payload-string, not
event.sourceidentity — extension pages that callwindow.postMessage({source:"hf-preview"})can win 400ms of full-rate. Bounded nuisance, not correctness. Tighter option (event.source === iframe.contentWindow) complicates the API for a trivial cost. - Wake-cost sharing is uniform — no per-subscriber gate parameter. Current subscribers early-return cheaply; only relevant if a future subscriber becomes expensive.
- Park/resume race has no dedicated interleaving test — static reasoning is sound; a 3-line test would harden.
Coordination. Via (peer bot) deferred stamp to tai as primary with three shape-of-question angles pre-retarget — Q1 (2 fallback-timer inputs) applies to #3845 (runtime transport), not this PR; Q2 (park/resume race) covered above (static-sound, no dedicated interleaving test — noted as follow-up); Q3a (fault isolation) verified at L88-97 + test at :143-172; Q3b (wake-cost sharing) verified uniform + cheap; Q3c (6th-loop extension surface) verified requestOverlayFrames() primitive absorbs custom wake sources. Concur with Via's read across the concern surface.
— Review by tai (pr-review)
terencecho
left a comment
There was a problem hiding this comment.
APPROVE at 752aba076f939fb14164737af008c098152d990f — re-stamp after CodeQL js/missing-origin-check fix + 3-unit handler split. Prior stamp at 1281ca0cd (review 5170426363) carries for everything else; this delta is scoped to overlayFrameLoop.ts + its test file, so all five axes from the prior review (shared-loop primitive, 4-overlay migration, paused-heartbeat dedupe, fault isolation, teardown symmetry) still hold. Concurring with Via + Vai's angles.
Origin check — strict equality, canonical strings
if (!isFromPreviewOrigin(event)) return; // overlayFrameLoop.ts:95 (first stmt of onPreviewMessage)
function isFromPreviewOrigin(event: MessageEvent): boolean {
if (typeof window === "undefined") return true;
return event.origin === window.location.origin; // :69
}
===onMessageEvent.origin(browser-set, canonical per spec) vsWindow.location.origin(browser-set). NostartsWith/includes/ regex / trailing-slash normalization — the classiceditor.example.com.attacker.comsuffix-craft has no behavior to distinguish against a canonical-string equality, so that class of bypass is unreachable by construction.- Runs BEFORE
const data: unknown = event.data— satisfies CodeQLjs/missing-origin-checksemantics (origin verified before payload read). - Positive-path tests updated to attach
origin: window.location.origin(.test.ts:97). Negative case at.test.ts:195-215: same well-formed payload fromhttps://not-the-preview.exampleis dropped (no runs, no dispatch). Pre-existing "some-extension source" negative case retained at.test.ts:182-190.
event.source discriminator (Via Q2) — absent, non-blocker
Not present. Origin-equality is sufficient for the CodeQL finding and for the wake-cost regression (an editor-origin popup would still have to send source: "hf-preview" to wake, worst outcome bounded to a 400ms wake window). Belt-and-suspenders event.source === previewFrame.contentWindow is a nice defense-in-depth if the module ever gets a handle to the iframe, but the module today doesn't hold one — reasonable follow-up if you want to close that residual gap post-merge.
Handler split — 3 private units, single-caller (Via Q3)
isFromPreviewOriginat:67— origin equalityisPreviewPayloadat:77— narrowsevent.datato{source: "hf-preview", type: "state"}previewStateOfat:83— extractsframeNumber/playingfallbacks tonull
All three are plain function declarations, NO export. Grep across packages/studio/src at refs/tmp/pr-3846-v3: only two files reference them — overlayFrameLoop.ts itself, and overlayFrameLoop.test.ts (through the observable behavior of onPreviewMessage, not by direct helper calls). onPreviewMessage is the sole caller of all three; onPreviewMessage is the sole window.addEventListener("message", ...) binding in this module (bound at :156, symmetric removeEventListener at :165). No alternate wake path — the origin gate runs on every message-driven wake. CRAP 31.6 satisfied without introducing a testing seam.
Preserved invariants
- Payload-string filter —
data.source === "hf-preview"moved verbatim intoisPreviewPayload:79. Extension/devtools/embed traffic still rejected. - Frame dedupe —
lastPreviewFrame/lastPreviewPlayingmodule state unchanged; comparison shape at:100matches pre-refactor. Paused-heartbeat dedupe test (ignores the paused heartbeat, which repeats a frame the overlays already drew) not touched by this diff. - Handler order — origin check → payload check → dedupe → wake. Reject at any layer is a plain return; no state mutation on rejected messages.
Non-blocking observations
- Defense-in-depth
event.sourcediscriminator — see Via Q2 above. Not blocking. - SSR guard
if (typeof window === "undefined") return true;inisFromPreviewOrigin— dead code at runtime (the handler is bound only inside atypeof window !== "undefined"gate at:151). Harmless; kept for defense against future callers.
CI
All 7 required checks green: Build (2m56s), Render on windows-latest (5m50s), Semantic PR title, Test (7m26s), Test: runtime contract (1m35s), Typecheck (3m3s), regression. Full CI on main base (not the reduced-matrix stacked-child shape).
— Review by tai (pr-review)
Stacked on the runtime half. Review and merge that one first; this branch is targeted at it.
CI does not run on this PR while its base is not
main— the workflow isbranches: [main]. Retarget it tomainonce the base merges, and givemain+ this branch a full suite at that point; the suites quoted below were run locally in the stacked worktree.Four overlays in the editing chrome — the composition rect, the selection and hover boxes, the off-canvas indicators and the snap guides — each owned an animation-frame loop that re-armed unconditionally. On a paused, untouched editor that is four callbacks per frame reading layout, and the snap-guide loop writes style on every one of them, which is why the compositor kept committing 55 frames a second with nothing moving.
None of the four has a clock of its own. Each only changes when something observable happens, so they now share one loop that runs at full rate for a moment after a pointer, key, wheel, scroll, resize or visibility change, after a preview message reporting a frame the overlays have not drawn, or after a mutation inside the preview document. Otherwise it polls four times a second, so a wake source nobody thought of costs a quarter second of staleness rather than a frozen overlay.
The one subtlety, and it is the whole reason the first version of this did nothing. The paused preview posts an unchanged status message every 80 ms so any listener can confirm its position. That message is indistinguishable, at the listener, from the message a moving playhead posts. Waking on every message therefore held the four overlays at 60 fps for the life of the tab: animation-frame callbacks fell five-fold while compositor commits did not move at all. The loop now compares the frame number in the message against the one the overlays last drew and treats a repeat as no news.
The awake window is 400 ms rather than one frame because the consequences of an event outlast the event: a click opens a panel whose CSS transition keeps moving the preview for the next couple of hundred milliseconds, and no further event is dispatched while it does.
Tests
overlayFrameLoop.test.ts, seven tests:runs every subscriber on one frame, not one frame eachdrops to the idle poll rate once nothing has happenedwakes to full rate on a pointer eventwakes to full rate when the preview reports a new frameignores the paused heartbeat, which repeats a frame the overlays already drewwakes on an explicit request, which is how preview-document mutations arrivestops entirely, and stops listening, once the last subscriber leavesEach was proved non-vacuous by breaking its path on purpose (never parking, attaching no listeners, granting no awake window, dropping the frame comparison) and confirming exactly the expected tests fail.
What the independent pre-review pass found
One throw stopped all four overlays, and the safety net with them. The shared loop re-armed after running its subscribers, so a subscriber that threw took down the frame loop and the idle poll permanently — a failure the four original loops kept to themselves, because each re-armed first. It now re-arms before running anything and isolates each subscriber, rethrowing out of band so the error still reaches the page's error reporting. Three of the subscribers read
contentDocumentwith no cross-origin guard, which is pre-existing and unchanged; the isolation is what stops it being four overlays' problem instead of one's. Test:keeps every other subscriber running, and the loop alive, when one throws.Any window message woke the loop. The wake sat outside the recognised-message check, so postMessage traffic from an extension, devtools or any other embed on the page held the overlays awake for 400 ms. Only the preview's own messages wake it now, and a repeated paused status post still does not. Tests:
does not wake for a message that is not the preview's,wakes for a preview message that is not a state post.A fifth loop.
useMotionPathDatahad the same unconditional shape and is live whenever a keyframed element is selected. Converted; it fits inside this PR.Measured
Idle means paused, loaded, focused, no key presses, no pointer. Two projects: a 1689-element / 396-card carousel and a 79-video / 12-audio media project. Both arms were served from the same rig and captured interleaved within each instrument, so a machine-load spike lands on both. This Mac was running other work throughout (1-minute load 44 to 256 across the captures), which inflates absolute milliseconds on both arms equally; the counts do not move with load and are the harder numbers.
FireAnimationFrameper secondCommitper secondsample)Two interleaved passes per fixture per instrument on the final build; the renderer-percentage row is the median over five passes because process sampling is the noisiest instrument here. An earlier three-pass run on the same code minus three behaviour-neutral refactors gave 129.2 → 9.4 ms/s and 283 → 3.8 callbacks per second on the carousel, and 66.8 → 13.0 ms/s and 287 → 3.7 on media: the same result.
Compositor commits are the row worth reading twice. Frame callbacks are a mechanism metric and a change like this can move them without moving any work; commits cannot be gamed, because nothing commits a frame without having done the work. They fall with the callbacks, so the work really is gone.
Two numbers that are not measured
The idle table below was captured before two later changes and has not been re-taken; disk on the measuring machine is under 3 GB and each trace capture is 28-100 MB.
getAnimations()per animated element. It cannot be a regression against the base — the per-frame loop made the same call 60 times a second — but the after column is optimistic by whatever that costs at 12.5 Hz, and on a composition with many CSS-animated elements that is not nothing.A correction to the callback-count figure, and a sixth loop
Re-verifying the final build in a real browser with a per-realm probe (patching
requestAnimationFrameand recording the caller's stack) found 297 of 297 sampled registrations in the preview realm coming from the Studio bundle and none from the runtime, at about 60 a second. Attributed from the served bundle, it isstartStudioManualEditPlaybackReapplyinmanualEdits.ts: it re-arms for as long as__player,__timelineor any entry of the timeline registry reports itself as playing, and on this project a scene timeline added to the paused root is itself unpaused, so it never stops.Two things follow, and both are stated rather than smoothed over:
manualEdits.tsis untouched by either branch and is a candidate for the same treatment in follow-up work. I could not build a control to date the discrepancy: the before-arm build's dependencies have since been removed from the machine.Playback, scrubbing and jumps are unchanged
Same harness, play / scrub / jump phases, interleaved. Five replicate pairs on media, two on the carousel.
Playhead positions and settle times are identical. Dropped frames are not worse on any run; the two arms' worst cases are 503 and 32, and both arms had runs where playback stalled entirely under a load spike (two of five each), so the stall is the machine, not the change. The carousel's playback frame rate rising from ~27 to ~45 is the same main-thread time coming back.
End to end, in a real browser
On the same selected card, at the same playhead advance, the selection overlay moved 255.8 px on both arms — the overlay still tracks an animating element with the loops parked. A drag of a timeline-animated card moves the box on neither arm (the Studio blocks manual movement of an element whose transform the timeline owns), which is the same behaviour before and after.