timers: use only a single TimerWrap instance

[apapirovski]

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.

                                            confidence improvement accuracy (*)    (**)   (***)
timers/timers-breadth.js n=5000000                   *      3.24 %       ±2.84%  ±3.78%  ±4.92%
timers/timers-cancel-pooled.js n=5000000           ***    -16.38 %       ±2.06%  ±2.74%  ±3.57%
timers/timers-cancel-unpooled.js n=1000000         ***     47.81 %       ±1.33%  ±1.79%  ±2.37%
timers/timers-depth.js n=1000                              -0.24 %       ±0.89%  ±1.19%  ±1.56%
timers/timers-insert-pooled.js n=5000000           ***     16.08 %       ±7.09%  ±9.49% ±12.47%
timers/timers-insert-unpooled.js n=1000000         ***    162.84 %       ±4.82%  ±6.49%  ±8.59%
timers/timers-timeout-pooled.js n=10000000         ***     15.90 %       ±1.59%  ±2.11%  ±2.76%
timers/timers-timeout-unpooled.js n=1000000        ***    948.32 %      ±21.44% ±28.89% ±38.36%

Fixes: #16105

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[apapirovski]

CI: https://ci.nodejs.org/job/node-test-pull-request/14729/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/182/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1400/

[apapirovski]

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, test/parallel/test-cluster-setup-master-multiple.js doesn't account for the possibility of all timeouts executing on the same tick.

(We can of course fix that by calling process._tickCallback() ourselves much like we do in modules but the question is whether we should or we should just fix our tests.)

[apapirovski]

To avoid this being semver-major, if at all possible, I've re-added the nextTick behaviour back in (along with a corresponding test). We could perhaps discuss that changing in a semver-major PR although it's not strictly necessary.

[Fishrock123]

I'll try to get to this in the next couple of days and give it a full review.

[bnoordhuis]

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).

[bnoordhuis]

Is refStatus necessary? It can be derived from refCount, can't it?

[apapirovski]

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.

[bnoordhuis]

Maybe this?

const removed = [5, 10, 15];
for (const i of removed)
  assert(queue.remove(i));

[bnoordhuis]

I'm just curious, is there a reason to remap undefined to null?

[apapirovski]

Probably not, honestly. I'll change it.

[apapirovski]

Currently, that's an O(1) operation but with this pull request, it's O(n).

@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).

[apapirovski]

@bnoordhuis also, thanks for reviewing! 😊

[apapirovski]

@nodejs/timers @jasnell @addaleax This is a fairly substantial change to the internals of timers. Would love to get a few more eyes on this, if possible. Thanks in advance!

[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.

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.

[bnoordhuis]

Typo: fundamentally

[bnoordhuis]

Minor consistency issue: it's called queue everywhere else.

[apapirovski]

@bnoordhuis That's actually exactly what timers/timers-cancel-unpooled.js and timers/timers-insert-unpooled.js test. Both saw a substantial improvement.

That snippet should be something like O(n log n) for both insert and remove (not accounting for whatever the cost of managing the Object map is in V8) but that would be the same as now, except there's less overhead because it doesn't have to call into C++ to do it and create a handle.

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).

[apapirovski]

To take this a bit further: the current architecture (which I believe @Fishrock123 came up with) is based on the assumption that net (and related) are the most significant consumers of timeouts, which means that most apps should only ever have a few timer lists at most. In that regard, being able to add yet another http timeout (likely for 120s or 5s since those are our defaults) in O(1) is more important than being able to create more lists.

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 net style timeouts much slower.

[apapirovski]

Actually, I see what you're referencing @bnoordhuis. I think our benchmarks might've misled me here.

[bnoordhuis]

based on the assumption that net (and related) are the most significant consumers of timeouts, which means that most apps should only ever have a few timer lists at most

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. :-)

[apapirovski]

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.

Right but lists are based on the durations not expiries. And the priority queue just tracks expiries for each list.

edit: missed your last comment. Guess we're on the same page now. :-)

Not quite :) I'm just seeing that clearTimeout can indeed be slower depending on where in priority queue we're removing from but that only applies to large n and only when having to actually delete from the priority queue. Our benchmarks iterate in a favourable way but it's possible to also iterate in an unfavourable one.

[apapirovski]

@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 clearTimeout when a large number of timeouts with unique durations are scheduled (eg. 10000+ timeouts, all with different durations). I've also adjusted our benchmarks to actually properly test that scenario.

(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 O(n) search in remove — or rather, removed the need to use remove (replaced by removeAt) in Timers.)

I would appreciate another review if possible!

[Trott]

This appears to be the cause of #20907. PR to revert: #20919