Skip to content

Write immediate spring updates synchronously - #6

Merged
Guiw5 merged 1 commit into
mainfrom
perf/open-path
Sep 9, 2026
Merged

Guiw5 merged 1 commit into
mainfrom
perf/open-path

Conversation

@Guiw5

@Guiw5 Guiw5 commented Sep 9, 2026

Copy link
Copy Markdown
Owner

Follow-up to #5, which found that both the drag and the open path are script-bound rather than layout-bound.

Change

Every spring update that was unconditionally immediate: true went through api.start() and was then awaited. start() queues the update and resolves through the animation loop, so each of those cost a frame even though nothing was animating. SpringRef.set() writes the values synchronously and stops the running animation, which is exactly what those call sites wanted.

Five awaited round-trips are gone from the open and close sequences, and the per-pointermove write during a drag no longer allocates a promise it never reads.

Conditional cases are untouched: snapSmoothly and resizeSmoothly still go through asyncSet, because their immediate depends on prefers-reduced-motion and on the resize source.

Measured

Production Vite build, React 19, 4x CPU throttle, timed from the library's own onSpringStart / onSpringEnd callbacks so the instrumentation costs nothing.

Opening the sheet, first open after load:

reaches open onSpringEnd({type:'OPEN'})
before no, still opening after 4 s never fired
after 1355 ms fired at 1352 ms

The early state transitions also get through faster, 5 of them in 15 ms against 2 in 19 ms, which is the awaited frames disappearing.

Treat the absolute numbers as environment-specific rather than as a benchmark. Getting a trustworthy figure here was hard: polling loops and MutationObservers both perturbed the result badly enough to invert it, and cold-cache reloads swamped it. The callback timings above are the only instrumentation that did not distort what it measured.

The bigger lead this uncovered

Even after this change, roughly a second of the open sequence at 4x throttle sits in a single gap between two substates, and it lands on activate — the step that turns on the scroll lock, the focus trap and the aria hider before the sheet is visible. That is now the largest single cost on the open path and the obvious next thing to look at. It is out of scope here.

Also worth knowing: on main, the open sequence could fail to complete at all in the harness above, leaving the machine in opening and never firing onSpringEnd({type:'OPEN'}). Consumers that gate work on that callback would wait forever. This PR made it complete in every run I did, but I have not proven the underlying cause, so treat it as improved rather than fixed.

Test plan

  • npm test: lint, 25 unit tests, library build, docs build
  • Docs fixtures in Chrome: simple opens, closes with Escape, reopens and closes with the Dismiss button; scrollable drags between snap points with the height animating (506 → 623 → 663 → 760 px) and back down
  • Packed and installed into a clean Vite consumer on React 19: open, drag with the sheet tracking the pointer, spring back, unmount, scroll lock released

Unconditional immediate updates went through api.start() and were awaited, costing a frame each even though nothing animated. api.set() writes them synchronously. Removes five awaited round-trips from open and close, and a promise per pointermove during a drag.
@Guiw5
Guiw5 merged commit a717079 into main Sep 9, 2026
2 checks passed
@Guiw5
Guiw5 deleted the perf/open-path branch September 9, 2026 05:40
@Guiw5

Guiw5 commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Correcting the record on this PR's description.

The claim that main could leave the open sequence stuck in opening and never fire onSpringEnd({type:'OPEN'}) was wrong. It was an artifact of the harness, not a defect in the library.

The automated Chrome I measured in runs requestAnimationFrame at 1 frame per second, while still reporting document.visibilityState === 'visible' and document.hasFocus() === true. Measured directly: eleven consecutive frame intervals of 1000-1017ms.

react-spring caps a frame's progress at 64ms, so a 115ms tween needs two frames. At 1fps that is two seconds rather than 33ms, and the five awaited start({ immediate: true }) round-trips this PR removed each cost a frame on top. That is why the old code appeared to hang and the new code appeared to complete: at 1fps the difference between five awaited frames and zero is five seconds.

What still holds:

  • The change itself is correct and is a real reduction in awaited work. It removes about five frames from the open and close sequences, which is roughly 80ms at 60fps, not seconds.
  • The measured behaviour at a normal frame rate is a ~105ms openSmoothly against a 115ms configured duration, which is right.

What does not hold: the "may never complete" paragraph and the before/after table in the description. Those numbers are contaminated and should be ignored.

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.

1 participant