Skip to content

perf(studio): a paused editor stops redrawing its overlays sixty times a second - #3846

Merged
miguel-heygen merged 3 commits into
mainfrom
perf-park-studio-overlays
Sep 10, 2026
Merged

miguel-heygen merged 3 commits into
mainfrom
perf-park-studio-overlays

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator

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 is branches: [main]. Retarget it to main once the base merges, and give main + 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:

What it holds Test
Every subscriber runs on one frame, not one frame each runs every subscriber on one frame, not one frame each
The loop drops to the idle poll rate when nothing happens drops to the idle poll rate once nothing has happened
A pointer event brings it back to full rate wakes to full rate on a pointer event
A preview message reporting a new frame brings it back wakes to full rate when the preview reports a new frame
The paused heartbeat does not ignores the paused heartbeat, which repeats a frame the overlays already drew
Preview-document mutations arrive through the explicit request, which is how the off-canvas indicators mark themselves dirty wakes on an explicit request, which is how preview-document mutations arrive
Unsubscribing stops the loop and removes its listeners stops entirely, and stops listening, once the last subscriber leaves

Each 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 contentDocument with 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. useMotionPathData had 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.

Idle, paused, untouched carousel before carousel after media before media after
main-thread self time (ms/s, CDP trace) 145.1 12.4 61.4 11.1
FireAnimationFrame per second 281.0 4.2 284.8 3.9
compositor Commit per second 55.1 4.2 56.4 4.0
CPU-profile busy (ms/s) 94.5 3.2 43.3 3.5
renderer process, % of a core (macOS sample) 21.0 4.2 9.5 1.3
its main thread, % of a core 13.3 1.4 5.7 1.0

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.

  • The parked poll now calls the adapter duration floor once per heartbeat, about 12.5 times a second. For the CSS adapter that is a 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.
  • The manifest post is now retried after a throw rather than dropped, which can hold the loop awake for up to 20 frames longer in that failure case. Not on any normal path.

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 requestAnimationFrame and 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 is startStudioManualEditPlaybackReapply in manualEdits.ts: it re-arms for as long as __player, __timeline or 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:

  • The parks hold. Zero runtime frames in that sample is direct evidence the transport is parked, and the top frame measured 3.8 callbacks a second, so the overlay loop is parked too.
  • The absolute "callbacks per second" row above is lower than what a browser is really doing, because that loop does not appear in it. Both arms were captured with the same instrument, so the before/after ratios and every other row stand; the absolute callback figure should be read as "the loops this change owns", not "every loop on the page". manualEdits.ts is 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.

before after
playhead after a 150-step scrub (carousel) 5 s, every run 5 s, every run
playhead after 30 one-second jumps (carousel) 35 s, every run 35 s, every run
playhead after a 60-step scrub (media) 2 s, every run 2 s, every run
playhead after 30 one-second jumps (media) 32 s, every run 32 s, every run
settle time after the last key 0 – 0.20 s 0 – 0.06 s
playback rate, composition seconds per wall second 0.80 – 0.94 0.83 – 0.94
dropped video frames, worst single play window 503 32
dropped video frames, all windows summed 976 75
playback frame rate, carousel 27.0, 27.3 43.1, 47.5

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.

@miguel-heygen miguel-heygen changed the title A paused editor stops redrawing its overlays sixty times a second perf(studio): a paused editor stops redrawing its overlays sixty times a second Sep 10, 2026
@miguel-heygen
miguel-heygen force-pushed the perf-park-studio-overlays branch 3 times, most recently from 90c20ab to d9081f0 Compare September 10, 2026 14:58
Base automatically changed from perf-park-runtime-transport to main September 10, 2026 15:29
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.
@miguel-heygen
miguel-heygen force-pushed the perf-park-studio-overlays branch from d9081f0 to 1281ca0 Compare September 10, 2026 17:28
@miguel-heygen
miguel-heygen enabled auto-merge (squash) September 10, 2026 17:29

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:116
  • useDomEditCompositionRect.ts:62
  • useDomEditOverlayRects.ts:264 (rafPausedRef early-exit preserved inside update)
  • offCanvasIndicatorRefresh.ts:140-142 (markDirty at L91 also calls requestOverlayFrames() 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.source identity — extension pages that call window.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 terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}
  • === on MessageEvent.origin (browser-set, canonical per spec) vs Window.location.origin (browser-set). No startsWith / includes / regex / trailing-slash normalization — the classic editor.example.com.attacker.com suffix-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 CodeQL js/missing-origin-check semantics (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 from https://not-the-preview.example is 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)

  • isFromPreviewOrigin at :67 — origin equality
  • isPreviewPayload at :77 — narrows event.data to {source: "hf-preview", type: "state"}
  • previewStateOf at :83 — extracts frameNumber / playing fallbacks to null

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 into isPreviewPayload:79. Extension/devtools/embed traffic still rejected.
  • Frame dedupe — lastPreviewFrame / lastPreviewPlaying module state unchanged; comparison shape at :100 matches 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.source discriminator — see Via Q2 above. Not blocking.
  • SSR guard if (typeof window === "undefined") return true; in isFromPreviewOrigin — dead code at runtime (the handler is bound only inside a typeof 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)

@miguel-heygen
miguel-heygen merged commit 9549e03 into main Sep 10, 2026
61 checks passed
@miguel-heygen
miguel-heygen deleted the perf-park-studio-overlays branch September 10, 2026 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants