Repository navigation
perf(g): update animation promises once per frame - #2265
Ethan-Icet wants to merge 2 commits into
Conversation
Every animation started in a task queued its own no-op frame callback (AnimationTimeline.tick, and restart() when seeking), and the timeline wrapped each frame callback in two calls to updateAnimationsPromises, which goes over every animation with promises. Starting N animations in one task (G6 does this on every animated draw) therefore cost about 2·N² updatePromises calls in the next frame: 32 million for N = 4000. Run updateAnimationsPromises once before and once after the frame's callbacks instead. Nothing can change promise state between two callbacks of the same frame, so the extra calls did nothing. Tests: six traces of when ready/finished settle relative to frames and playState, recorded on 6.3.1 and unchanged; three tests that the bookkeeping stays linear when starting, or seeking paused or finished, animations in one task (6.3.1: ~2 million calls for n = 1000). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 50d7d7b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@Ethan-Icet Does this only affect animation performance? Will it improve first-screen rendering performance? |
The helper read `animation.finished` only for its side effect (it puts the animation on the timeline's promise list), which CodeQL reports as an expression with no effect. Assert on it instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Hello @hustcc! It only changes the animation timeline's bookkeeping, so drawing itself is unchanged: a first screen without animations renders exactly as before. But in G6 the first screen is animated by default, so it benefits too. The base theme gives nodes and edges I measured the impact with a new benchmark:
With this PR, the 4,000-node first frame costs the same as with
Side by side, same 2,000 nodes and edges, recorded in headless Chromium. With 6.3.1 the canvas stays blank for about 6 s and then the graph appears all at once: the 1 s fade-in is over before the page gets another frame. With this PR it fades in and is done at about 1.7 s. I also pushed a small follow-up for the CodeQL note (the test reads |
|
@Ethan-Icet Thanks for the PR. Looking at the data, it's a very good and professional PR. We'll use this patch to run regression validation on all cases across G2, G6, and S2 as soon as possible to make sure there are no major issues before merging. If you're interested, you're also welcome to help with the regression testing. Also, we're very much looking forward to further optimizing G6's performance 🤗 |
|
@hustcc Thank you for the quick response, it's much appreciated! I'm working on a side project in my free time (a chess visualization tool built around graphs). I'm not a great UI/UX developer, so it's mostly a vibe-coded toy where I burn my unused weekly Claude quota. It involves benchmarking on quite big graphs, which is how I found this bug. It's not in a shareable state yet. I'm still working on the internals, so the visuals are a bit crude. I'm developing two parallel branches, one with G6 and one with PixiJS, and I try to keep their renders similar. G6 is currently a bit behind in my first tests and I'd like to narrow that gap, so if I run into anything else along the way, I'll share it 🙂 Thanks for the offer, but I'll leave the regression testing in your hands: my time for this is limited, and you know G2, G6 and S2 much better than I do. Thanks again for reviewing this so quickly! |

🤔 This is a ...
🔗 Related issue link
Fixes #2264. Related: #1731.
💡 Background and solution
Starting N animations in one task makes the next frame call
Animation.updatePromises()about 2·N² times. G6 does exactly this on every animateddraw().AnimationTimeline.tickdoes it, and so doesrestart()when finished animations are seeked.updateAnimationsPromises()calls, and each call goes over every animation with promises.At N = 4,000 that's 32 million calls and a 746 ms frame of CPU time; through G6, a 1.77 s frame.
Solution: run
updateAnimationsPromises()once before and once after the frame's callbacks, instead of around each callback.The calls this removes were no-ops:
webAnimationsNextTick) change nothingupdatePromises()reads.updatePromises()with no state change does nothing.The remaining two calls run at the same points as before (after the frame's
tick, beforeapplyPendingEffects), over the list in the same order. Soreadyandfinishedsettle in the same frames as before.Headless Chromium 153; k is the fitted exponent of t ≈ c·N^k over N ≥ 250.
updatePromises()calls, N = 4,000finished, N = 1 / 10 / 100Tests: new
__tests__/unit/dom/animation.promises.spec.ts.requestAnimationFrameoption. Each test records, per frame, whenready/finishedresolve or reject,onfinish/oncancel, andplayState. Scenarios: plain play; 50 animations started together; pause/play/reverse/updatePlaybackRate; cancel/finish()/seeking; animations started from promise callbacks; a destroyed target. The inline snapshots were recorded on the code before this change, and they pass unchanged after it.updatePromises()calls in the frame after 1,000 and 2,000 animations are started, paused and seeked, or finished and seeked, in one task. Before: about 2,000,000 calls at n = 1,000 in each case. After: 5,000 / 11,000, 3,000 / 6,000 and 2,000 / 4,000.pnpm test:unitpasses: 281 tests, the 272 existing plus the 9 new.Not changed: finished animations still stay in
animationsWithPromises(#1731). Dropping them changes when a later read offinishedgets a fresh promise, so it belongs in a separate change.📝 Changelog
☑️ Self Check before Merge
🤖 Generated with Claude Code