-
-
Notifications
You must be signed in to change notification settings - Fork 34.3k
lib: optimize FixedQueue by reusing one segment #60031
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Review requested:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #60031 +/- ##
=======================================
Coverage 88.45% 88.46%
=======================================
Files 703 703
Lines 207805 207821 +16
Branches 40022 40029 +7
=======================================
+ Hits 183822 183848 +26
+ Misses 15980 15961 -19
- Partials 8003 8012 +9
🚀 New features to boost your workflow:
|
mcollina
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! There are diagrams explaining this data structure at the top of the file that would need updating.
The new spare segment isn’t part of the list. It’s just a detached buffer we park for reuse once it's created, so it doesn’t really affect the diagrams. But nonetheless, I added a small paragraph to explain how it works. Let me know if you'd like to see more explanation in the code! |
6fcdacc to
f91c803
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Failed to start CI⚠ No approving reviews found ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/18073218517 |
f91c803 to
0a70daa
Compare
|
@mcollina PTAL |
| @@ -0,0 +1,28 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't include this inside benchmark/util. I believe benchmark/internal would be more appropriate. To be fair, creating a benchmark for a public API that makes use of FixedQueue behind the scenes would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @RafaelGSS, I looked at the other benchmarks and unfortunately I would need to create something super targeted for this. And even then, it might be hard to see the difference indirectly
This is a specific improvement that only gets triggered if we reach segment N + 1, and cycle between N and N + 1
For the benchmark, I was inspired by the priority queue benchmark. If you prefer, I can remove benchmark/util/fixed-queue-oscillate.js
lemire
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a tradeoff: using more memory but also getting more performance.
Yes, that's correct. It hinges on the idea that if we reach N+1 segments, there is a higher chance we will reach there again. But the good thing here is that we keep at max 1 extra spare segment. If 2 segments are empty (we're at N-1), segment N+1 will still be GC'd with its spare |
|
@gurgunday Yes, to be clearer, my comment was tied with my approval of the PR. |
|
I can't see an improvement in the next-tick or event.on benchmarks unfortunately |
Benchmarks:
main:
branch:
When the head segment fills, we reuse a “spare”
FixedCircularBufferif available; when a segment becomes empty, we reset it and stash it as the spare instead of wasting it. This cuts allocations/GC especially when the queue oscillates between N and N+1 segments, which is more than likely to be the case in real life scenarios where Events/Promises/Tasks are emitted/created and consumed in bursts.We only call reset() when reusing a spare (not for a freshly allocated buffer).
And finally, there is no behavior change: we have the same data structure (linked fixed-size circular buffers), same invariants (one-slot-wasted), same FIFO semantics and API. Capacity per segment stays
kSize - 1, so overall capacity is unchanged.