Perf: Rely on more efficient Map for Priority Queue internal state #47156
Conversation
This file contains 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
|
Size Change: -68 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Jan 18, 2023
| isRunning = false; | ||
| return; | ||
| for ( const [ nextElement, callback ] of waitingList ) { | ||
| waitingList.delete( nextElement ); |
There was a problem hiding this comment.
Is it ok to delete an element from a map while we continue iterating. We won't be skipping elements randomly or something? I guess not since the tests look ok, but that's the only weird thing here.
There was a problem hiding this comment.
yeah I verified this before adding it; also, if we delete and element and then add it back it will appear at the end of the iteration since it was added last, even though it existed in the map when iteration began.
youknowriad
approved these changes
Jan 18, 2023
dmsnell
force-pushed
the
perf/priority-queue-use-map
branch
2 times, most recently
from
307cef9 to
1c25ee4
Compare
dmsnell
force-pushed
the
perf/priority-queue-use-map
branch
from
1c25ee4 to
1d29e0a
Compare
…47156) The priority queue has been maintaining an array of enqueued contexts and a `WeakMap` of enqueued callbacks for dispatching idle updates. Priority ordering of context is maintained through the inherent order of the array, but this comes at the cost of calling `.shift()` on every dispatch, which rewrites the ordering of all the remaining keys or indices of the array. When the priority queue gets large (such as in store updates) this can incur a measurable cost in time. Additionally, the use of the array for ordering relies on `WeakMap` semantics to loosely hold references to the contexts, which might be removed. In this patch we're relying solely on a single `Map`. By spec, `Map` iteration follows the order of first insertion of its keys, which exactly matches the needs of the priority queue iteration. We're able to avoid using the `Array` altogether, and while not totally free, `Map` deletion is measurably faster than `Array.prototype.shift()` and will involve at least one fewer references to the context memory than having it in the array _and_ the `WeakMap` did. The goal of this patch is small and likely hard to measure. It aims to lower memory allocation and pressure on the garbage collector during the critical hot-patch of code which runs on every keypress or change in the editor, so being a little better here will hopefully add up to macro-level improvements in editor responsiveness.
dmsnell
force-pushed
the
perf/priority-queue-use-map
branch
from
1d29e0a to
14e8390
Compare
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.
What?
Small performance refactor in priority queue.
Why?
This is targeting code in the critical hot path following keypresses and editor changes. It's aim is to guard the editor responsiveness.
How?
The priority queue has been maintaining an array of enqueued contexts and a
WeakMapof enqueued callbacks for dispatching idle updates.Priority ordering of context is maintained through the inherent order of the array, but this comes at the cost of calling
.shift()on every dispatch, which rewrites the ordering of all the remaining keys or indices of the array. When the priority queue gets large (such as in store updates) this can incur a measurable cost in time. Additionally, the use of the array for ordering relies onWeakMapsemantics to loosely hold references to the contexts, which might be removed.In this patch we're relying solely on a single
Map. By spec,Mapiteration follows the order of first insertion of its keys, which exactly matches the needs of the priority queue iteration. We're able to avoid using theArrayaltogether, and while not totally free,Mapdeletion is measurably faster thanArray.prototype.shift()and will involve at least one fewer references to the context memory than having it in the array and theWeakMapdid.The goal of this patch is small and likely hard to measure. It aims to lower memory allocation and pressure on the garbage collector during the critical hot-patch of code which runs on every keypress or change in the editor, so being a little better here will hopefully add up to macro-level improvements in editor responsiveness.
Testing Instructions
Assuming this is covered by the existing test cases, please audit the code and look for discrepancies in the refactor which might change the behavior of the component. It's heavily used through the editor and so should be vetted with extra scrutiny.