Repository navigation
perf(core): mounting a scene no longer restyles it to look for raster hints - #6300
Conversation
There was a problem hiding this comment.
Verdict: APPROVE · 0 blocker · 0 important · 1 nit · head b228a7e7
The selector can only miss in ways that keep a hint the old code would have dropped, so a miss costs perf and never correctness. Marked elements always match through the runtime's own [data-hf-preview-raster] rule, so unmarking is unchanged. unmarkHolders still visits every added element, and a cross-origin sheet, a selector that does not parse, or a nested rule falls back to *.
- nit
previewRasterHints.ts:86:[style*="will-change"]is case-sensitive, so an inlinestyle="WILL-CHANGE: transform"written by hand is no longer considered.[style*="will-change" i]fixes it. The same kind of miss happens for@scope (.x) { :scope { will-change: transform } }:markLayers'document.querySelectorAll(":scope")matches only<html>. Both only cost the optimization.
Scope
Read the full diff and the whole module. Checked how rule collection handles @media/@supports/@layer/@import/adopted sheets, nesting, and pseudo-element selectors (fits never read pseudo hints, so nothing is lost there). I did not run the tests locally because bun is not available here. CI was still running at review time.
Edit accuracy: accurate 271 (base branch 271), smooth 157 of thoseThe gate passes. Quarantined, measured but not gated (0) |
60128a6 to
7f8eb9c
Compare
There was a problem hiding this comment.
|
|
||
| function forgetRemovedMarks(records: MutationRecord[]): void { | ||
| if (!records.some((record) => record.removedNodes.length)) return; | ||
| for (const element of marked) if (!element.isConnected) marked.delete(element); |
There was a problem hiding this comment.
blocker: a marked layer that is detached and re-attached in a later batch keeps data-hf-preview-raster but is no longer in marked, so nothing restores its hint when it stops qualifying. forgetRemovedMarks drops it from the Set while the attribute stays on the node. On re-attach, fits sees isMarked = false, reads will-change: auto through our own !important rule, and returns false, and writeMark(false) is a no-op because the Set already says unmarked. From then on, followStyle skips the layer, and if this was the only mark, watch() stops observing attributes altogether. Result: a later position: fixed child, or the layer's own z-index going back to auto, paints without the containing block or stacking context the hint provides. Main and b228a7e read the attribute, so they re-check the layer.
Repro (fails on 7f8eb9c, passes on b228a7e):
const words = layer("position: absolute; will-change: transform");
const child = layer("", words);
setPreviewRasterScale(0.5);
words.remove(); await Promise.resolve();
document.body.append(words); await Promise.resolve();
child.style.position = "fixed"; await Promise.resolve();
expect(marked(words)).toBe(false); // stays trueFix: re-adopt the attribute when an element is followed, e.g. at the top of the followTree loop: if (element.hasAttribute(PREVIEW_RASTER_ATTR)) marked.add(element);. watch() then re-arms attributes as well.
There was a problem hiding this comment.
Fixed in 935faf2, as you suggested: followTree adopts any element it visits that already carries data-hf-preview-raster back into marked (adoptCopiedMark), so fits reads it as marked, followStyle follows it and watch() re-arms attributes. Your repro is now a test ("gives the hint back to a marked layer re-attached after removal once a child needs it"). The same gap existed for a cloneNode(true) copy of a marked layer added after the original is removed, so that has a test too. Both fail without the adoption. Core runtime suite 2,431 green, test:browser-raster 3/3.
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · 7f8eb9c → 935faf2
✅ The re-attach blocker is fixed: adoptCopiedMark picks the mark back up from the attribute. My repro and the PR's 22 tests pass locally (24/24).
New blocker: CI fails on this head. In the Build job, verify:packed-manifests reports @hyperframes/producer packed size 776219 bytes is over its budget of 768210 by more than 1% (the limit is 775,892). The producer packs the modular core runtime, so this PR's runtime growth on top of current main pushes it past the 1% tolerance. Main alone (aae32e43e) is green. The Studio edit-accuracy gate failure only follows from Build: its shards were skipped. Fix: raise the producer budget in scripts/package-size-budgets.json and give the reason in the PR body, as the check asks, then re-run CI.
935faf2 to
32984da
Compare
…ster-hint fix adds The producer bundles the runtime. On main (aae32e4) it packs at 773560 bytes (2821768 unpacked) and this branch at 773896 (2822665): packed rises by 336, unpacked by 897.
There was a problem hiding this comment.
Verdict: APPROVE · 0 resolved, 0 open, 0 new · 935faf2 → 87c82d4
✅ CI blocker fixed: the producer budget rises by the 336 packed / 897 unpacked bytes this change adds, the reason is in the PR body, and CI is green on this head. After the rebase, previewRasterHints.ts and its test are byte-identical to 935faf2, which I verified earlier (24/24 locally).
What
Mounting a scene in a small preview no longer forces the browser to restyle the whole scene just to look for
will-changehints. On a page with no hints, the raster-hints pass now reads no computed style at all.Why
Below 1x,
previewRasterHintsdrops purewill-change: transformhints so a small preview does not raster every hinted layer at full size. To decide that, it rangetComputedStyleon every element of every added subtree (followTree). When the added nodes included a<style>or<link>, it ran it on every element of the page (markLayers). Each read on a freshly inserted subtree forces a style recalc of that subtree, in the same task as the insertion.Scene parking (#6189) mounts each upcoming scene's DOM 10 s ahead while playing, so a big scene arrives as one subtree. On a 50 s 3840x2160 film with 7 scenes (1,595 layers in the largest), each mount of a large scene was a single 123 to 131 ms frame, and those frames were the film's last dropped frames. The film declares
will-changenowhere, so every one of those reads came backauto.How
What can carry a hint.
will-changeis not inherited, so only an element whose inline style or a stylesheet rule sets it can carry one.mayHintchecks exactly that without reading computed style: the element is marked, or its ownstyle.getPropertyValue("will-change")is set, or itmatchesthe selectors of the style rules that declarewill-change(including@media/@supports/@layer, imported and adopted sheets; the runtime's own sheet is left out).markLayersandfollowTreecallfitsonly wheremayHintholds.Marks have one writer.
setMarkwrites and clears every mark and keeps the set of marked elements, so "is anything marked" is exact and free. Marks on removed elements are dropped from the set, and an elementfollowTreevisits that arrives carrying the mark (re-attached, or copied withcloneNode) is adopted back into it, so its hint is still given back when a child starts to need it.Attributes are watched only when they can matter. With nothing marked and no rule giving a hint, no attribute or inline style change can make or break a mark, so the observer follows added nodes only. On this film that drops about 1,500 attribute records a second per SVG attribute the scene player animates (
fill,stroke,stroke-width, filterseed/radius/values), each of which used to runfollowTree, and the 8,642-node walk when perf(core): the preview mounts and draws only the scenes near the playhead #6189 stampsdata-durationon a mounted scene. As soon as a mark is written or a hint rule arrives, attributes are watched as before, so unmarking stays exact. A layer hinted inline that only later becomes droppable through an attribute change keeps its hint, which is the browser's own behaviour.Holders are walked only when something is marked.
Rules are read once per page check. The rule selector is re-read where the page is already re-checked: when a
<style>or<link>is added or a linked sheet loads. A rule edited through CSSOM in between is not seen, the same limit the file already has for marks; a hint it misses keeps rastering at full size and paints the same. Cross-origin sheets and selectors that do not parse fall back to every element, as do rules nested in a style rule (relative selectors). The selector is validated against an empty fragment, never against the page.Package size budget. The producer bundles the runtime, so its packed budget rises by the 336 bytes this change adds (unpacked by 897): on
mainat aae32e4 it packs at 773,560 bytes and on this branch at 773,896.mainsits at the edge of that budget, and the previous head failedverify:packed-manifestsby those bytes.Measured
The film
Studio-like playback of that film in
<hyperframes-player>at 1x for the whole 50 s, headless Chrome 152 on an Apple silicon Mac with GPU compositing, GPU rasterization and hardware video decode. Base ismainwith #6189 at its current head merged on top. Three builds in rotating order, three runs each, trace on, each run started with the host's 1-minute load under 10. "Off" is the same base with the hint pass disabled, the floor for a film with no hints.The misses before were at 13.17 to 13.25 s, where #6189 mounts the 1,595-layer scene 10 s before it starts; the single miss after and off was at 20.54/20.58 s (the next mount). The longest frame after stays about 5 ms above off: the remaining per-node work at a mount is the inline-hint check on the 8,642 added elements. Frames over 8 ms spread widely between runs of the same build (47 to 234 after here); over the earlier 15 pairs in both orders they did not follow this change.
The 48-layer fixture from #5268
48
will-change: transformlyric layers animated by GSAP, 528 px player at device scale 2, SwiftShader (the perf runner's browser). Three paired runs,mainagainst this branch:will-changelayers leftWith the swap turned off on this branch, the same probe reads 48 hinted layers, 52 to 53 compositor layers and a 3.64x raster, the numbers #5268 measured before its fix. Frames drawn with missing tiles were not measured: that needs a constrained tile budget the perf runner does not set.
Test plan
Unit tests added/updated
Manual testing performed
Documentation updated (if applicable)
Comments follow CONTRIBUTING.md "Comments": they say why, not what, and a bug fix says what the code must do and how to reproduce the bug
previewRasterHints.test.ts: mounting a 50-layer scene that brings its own<style>while the preview is small reads no computed style and makes no page-wide query (fails onmainwith 53 style reads, and with the selector validated against the page). A layer that only a rule inside@mediahints, added later, is still marked and a plain neighbour is not (fails with grouped rules not collected). Three added-node batches read the stylesheets at most once (fails with a per-batch read), and a hint rule in a<style>that arrives while the preview is small marks a layer added after it (fails without the re-read inmarkLayers). Mounting a hint-free scene walks no holders, and attribute and style writes on a page nothing hints do no work (fails when attributes are always watched); forgetting to clear a mark from the set fails two existing tests, and deciding from rules alone fails four. A marked layer removed and re-attached, and a copy of a marked layer added after the original is removed, both get their hint back when a child turnsposition: fixed(both fail without the adoption infollowTree).test:browser-raster(player perf shard) passes 3 runs in a row. Ignoring inline hints fails 4 existing tests; ignoring rule hints fails 2. The 14 existing tests pass unchanged, including the holder, stylesheet-load and batching cases.The test file passes 3 runs in a row. Full
packages/core/src/runtimesuite: 98 files, 2,431 tests, exit 0. Pre-commit lint, format, typecheck pass;check-comment-citationsandcomment-ratchetpass.