-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
timers: use only a single TimerWrap instance #20555
Conversation
27495f3
to
b415d75
Compare
There is one difference in behaviour that I would like some input on: with multiple TimerWraps we run nextTick after executing each list. With a single TimerWrap we do it only after all the lists due to run execute. This shouldn't but can have implications on badly written tests/code. For example, (We can of course fix that by calling |
b415d75
to
94590b4
Compare
To avoid this being |
94590b4
to
71dd3d7
Compare
I'll try to get to this in the next couple of days and give it a full review. |
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.
Nice work, LGTM.
I'm kind of curious how this will hold up in the real world. Specifically, I hypothesize that most timers are canceled before they expire.
Currently, that's an O(1) operation but with this pull request, it's O(n).
lib/timers.js
Outdated
|
||
let timerListId = Number.MIN_SAFE_INTEGER; | ||
let refCount = 0; | ||
let refStatus = true; |
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.
Is refStatus
necessary? It can be derived from refCount
, can't it?
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's necessary with maybeChangeRefStatus
which is generic (so we don't know if the number went up or down). I could instead have incRef
and decRef
— then we wouldn't need it. That's probably better in the end, I'll refactor.
test/parallel/test-priority-queue.js
Outdated
assert(queue.remove(5)); | ||
assert(queue.remove(10)); | ||
assert(queue.remove(15)); | ||
const removed = [5, 10, 15]; |
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.
Maybe this?
const removed = [5, 10, 15];
for (const i of removed)
assert(queue.remove(i));
lib/internal/priority_queue.js
Outdated
|
||
peek() { | ||
const value = this[kHeap][1]; | ||
return value !== undefined ? value : null; |
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'm just curious, is there a reason to remap undefined
to null
?
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.
Probably not, honestly. I'll change it.
@bnoordhuis It should still be mostly O(1), only O(log n) when it's the last timer in a list and the n here is the number of timer lists so pretty low in most usage. The architrcture overall is actually almost exactly the same as the current implementation except we use a binary heap implemented in JS over an array rather the timerwrap / libuv one. Also FWIW the decrease in our clearTimeout benchmark is only 16% but that comes from setting the symbol prop for kRefed status (not the other stuff). But that's also a benchmark where I recently got 1600% bump (the PR landed in 9.x at some point). |
@bnoordhuis also, thanks for reviewing! 😊 |
71dd3d7
to
51d5988
Compare
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 should still be mostly O(1), only O(log n) when it's the last timer in a list and the n here is the number of timer lists so pretty low in most usage.
Is that still true with a pattern like this?
var q = []
while (q.length < 1e3) q.push(setTimeout(() => 0, q.length))
while (q.length > 0) clearTimeout(q.pop())
My reading of the code is that this snippet has n^2 running time.
test/parallel/test-priority-queue.js
Outdated
const PriorityQueue = require('internal/priority_queue'); | ||
|
||
{ | ||
// Checks that the queue is fundumentally correct |
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.
Typo: fundamentally
benchmark/util/priority-queue.js
Outdated
|
||
function main({ n, type }) { | ||
const PriorityQueue = require('internal/priority_queue'); | ||
const heap = new PriorityQueue(); |
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.
Minor consistency issue: it's called queue
everywhere else.
@bnoordhuis That's actually exactly what That snippet should be something like In the current architecture that snippet would have to create a handle for each timeout list and that would then have to sort it into the libuv binary heap. Then when clearing, it would delete the list and close the libuv timer handle (which would have to remove it from the binary heap). |
To take this a bit further: the current architecture (which I believe @Fishrock123 came up with) is based on the assumption that I investigated using timer wheels and other similar approaches and while they improve the performance of creating & cancelling many different length timeouts, they make the management of |
Actually, I see what you're referencing @bnoordhuis. I think our benchmarks might've misled me here. |
How so? Socket timers have millisecond resolution and they're updated with every read and write. I don't think it's uncommon to end up in a situation where there is a list per socket. Could be mitigated by rounding down to seconds, that makes it 1000x more likely for them to end up in the same list. edit: missed your last comment. Guess we're on the same page now. :-) |
Right but lists are based on the durations not expiries. And the priority queue just tracks expiries for each list.
Not quite :) I'm just seeing that |
d2f567d
to
9216376
Compare
@bnoordhuis I've refactored the PriorityQueue implementation to allow storing a reference to the node's index using a custom setter function. That gets rid of the possible regression in (After extensive testing and trying a few other options, this was the most efficient solution that had negligible impact on performance in the best case scenario while removing the need for I would appreciate another review if possible! |
0e481fb
to
db83b3c
Compare
An efficient JS implementation of a binary heap on top of an array with worst-case O(log n) runtime for all operations, including arbitrary item removal (unlike O(n) for most binary heap array implementations). PR-URL: #20555 Fixes: #16105 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Hang all timer lists off a single TimerWrap and use the PriorityQueue to manage expiration priorities. This makes the Timers code clearer, consumes significantly less resources and improves performance. PR-URL: #20555 Fixes: #16105 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This PR moves almost the full entirety of timers into JS land and hangs them all off one TimerWrap. This simplifies a lot of code related to timer refing/unrefing and significantly improves performance.
Fixes: #16105
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes