Repository navigation
Cut a render pass and keep the divider lines off the paint path - #5
Merged
Merged
Conversation
useReady tracked readiness in a state map that every measurement wrote to, costing a render pass each; it is now derived from the measurements themselves. The header and footer lines animated their alpha inside box-shadow, repainting both on every frame; they are pseudo elements animating opacity now.
3 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two changes, both measured. The interesting part of this PR is what it does not do: profiling killed two planned optimisations.
Changes
Derive the ready flag.
useReadykept a state map that every measurement wrote to twice (register false, then true), each write costing a render pass, and the callbacks were invoked during render throughuseMemo. Readiness is just "the viewport and the content have a size", souseSnapPointsderives it now anduseReadyis gone.Composite the divider lines. The header and footer lines lived in
box-shadowwithcalc(var(--rsbs-content-opacity) * 0.125)as the alpha. Animating a colour inside a shadow repaints both elements every frame. They are pseudo elements now, with a fixed colour and an animatedopacity, which stays on the compositor. Pixel output is unchanged.Measured
React commits from click to
data-rsbs-state="open", React Profiler, 4x CPU throttle:The two remaining cascading commits come from
setMountedinindex.tsx, not from this change.What the profiling said, and what I dropped because of it
I traced a sustained drag and an open on the scrollable fixture at 6x CPU throttle and aggregated the trace by rendering phase.
Drag window, 4575 ms:
Open window, 1331 ms:
Dropped: the
heightMode="transform"idea. The premise was that animatingheightforces layout every frame and that moving totransformwould be the big win. Layout is 1-2% of the time. The change would have cost the sticky footer its position and broken the scroll viewport at small snap points, in exchange for almost nothing.Dropped: removing XState. It is 27% of the bundle, so it looked like the obvious cut. During a drag it is 0.4% of sampled CPU, and the drag is not CPU-bound at all — a third of the trace window is idle, p50 frame time is 16.7 ms at 6x throttle. Removing it is a rewrite of the interruption semantics for a bundle saving on a chunk that consuming apps already load lazily.
What the traces do say is that both paths are script-bound, and that the expensive one is open, not the animation: 1331 ms at 6x throttle with the main thread busy essentially throughout, against a drag that idles. If more work goes into performance, that is where it belongs — the promise-actor chain, focus-trap's tabbable scan, and the aria hider's
body > *walk all run before the sheet is visible.Test plan
npm test: lint, 25 unit tests, library build, docs buildposition: relativechange, which was the risk