Replace animation frame queue sort with binary heap#319734
Open
Xio-Shark wants to merge 2 commits into
Open
Conversation
The animation frame runner previously sorted the entire queue (Array.sort + Array.shift) on every frame, which is O(n log n). Replace this with a max-heap (AnimationFrameQueue) that provides O(log n) push and pop while preserving FIFO order within the same priority level via an insertion-order counter.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves the determinism and performance of animation-frame scheduling by replacing per-iteration array sorting with a stable priority queue, and adds browser tests to validate queue ordering/cancellation behavior.
Changes:
- Replace array-based animation frame queues (sort+shift) with a stable heap-based queue (push+pop).
- Preserve insertion order for callbacks with the same priority via an explicit order field.
- Add browser tests covering priority ordering, stability, cancellation, and re-entrancy (callbacks enqueueing more work during a frame).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vs/base/test/browser/dom.test.ts | Adds tests to validate animation-frame queue ordering, stability, and cancellation. |
| src/vs/base/browser/dom.ts | Introduces a stable heap-backed animation frame queue and updates scheduling logic to use it. |
Comment on lines
+563
to
+566
| assert.deepStrictEqual( | ||
| calls, | ||
| items.toSorted((a, b) => b.priority - a.priority || a.index - b.index).map(item => item.index) | ||
| ); |
Comment on lines
+426
to
+428
| setOrder(order: number): void { | ||
| this._order = order; | ||
| } |
- Replace Array.prototype.toSorted with [...items].sort() for broader JS runtime compatibility in test environments. - Guard setOrder against re-assignment to prevent silent heap corruption; order is now effectively one-time via a -1 sentinel.
Author
|
@microsoft-github-policy-service agree |
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.
Summary
Array.sort()+Array.shift()pattern in the animation frame runner with a max-heap (AnimationFrameQueue), reducing queue operations from O(n log n) to O(log n) per push/pop._order) toAnimationFrameQueueItemso callbacks with the same priority execute in FIFO order — the previous sort was not stable._runnerand_priorityreadonlysince they never change after construction.Motivation
Every animation frame, the runner called
currentQueue.sort()on the entire queue and thenshift()the first element. For workloads that schedule many animation frame callbacks (e.g. editor rendering, cursor blinking, overlays), this is wasteful: a heap provides O(log n) insertion and extraction while naturally maintaining priority order.Changes
src/vs/base/browser/dom.ts: NewAnimationFrameQueueclass backed by a binary max-heap withrunsBefore()comparison (priority desc, then insertion order asc).NEXT_QUEUEandCURRENT_QUEUEnow holdAnimationFrameQueueinstances instead of plain arrays.src/vs/base/test/browser/dom.test.ts: Four new tests covering priority ordering, FIFO stability with 128 items, cancellation, and mid-frame scheduling.Verification
tsgo --project ./src/tsconfig.json --noEmit --skipLibCheck— passednode build/eslint.ts— passedscripts/test.sh --grep "AnimationFrameQueue"— 4/4 passingdomtest suite — 41/41 passing, no regressionsTest plan