Skip to content
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

Closed
wants to merge 5 commits into from

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented May 6, 2018

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 apapirovski added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. lib / src Issues and PRs related to general changes in the lib or src directory. performance Issues and PRs related to the performance of Node.js. labels May 6, 2018
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 6, 2018
@apapirovski apapirovski changed the title Patch priority queue timers: use only a single TimerWrap instance May 6, 2018
@apapirovski
Copy link
Member Author

apapirovski commented May 6, 2018

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
Copy link
Member Author

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
Copy link
Contributor

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

Copy link
Member

@bnoordhuis bnoordhuis left a 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;
Copy link
Member

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?

Copy link
Member Author

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.

assert(queue.remove(5));
assert(queue.remove(10));
assert(queue.remove(15));
const removed = [5, 10, 15];
Copy link
Member

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


peek() {
const value = this[kHeap][1];
return value !== undefined ? value : null;
Copy link
Member

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?

Copy link
Member Author

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.

@apapirovski
Copy link
Member Author

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
Copy link
Member Author

@bnoordhuis also, thanks for reviewing! 😊

@apapirovski
Copy link
Member Author

@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!

Copy link
Member

@bnoordhuis bnoordhuis left a 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.

const PriorityQueue = require('internal/priority_queue');

{
// Checks that the queue is fundumentally correct
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: fundamentally


function main({ n, type }) {
const PriorityQueue = require('internal/priority_queue');
const heap = new PriorityQueue();
Copy link
Member

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.

@apapirovski
Copy link
Member Author

apapirovski commented May 10, 2018

@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
Copy link
Member Author

apapirovski commented May 10, 2018

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
Copy link
Member Author

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

@bnoordhuis
Copy link
Member

bnoordhuis commented May 10, 2018

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
Copy link
Member Author

apapirovski commented May 10, 2018

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 apapirovski force-pushed the patch-priority-queue branch 2 times, most recently from d2f567d to 9216376 Compare May 11, 2018 09:18
@apapirovski
Copy link
Member Author

apapirovski commented May 11, 2018

@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!

@apapirovski apapirovski force-pushed the patch-priority-queue branch 2 times, most recently from 0e481fb to db83b3c Compare May 12, 2018 07:40
@apapirovski apapirovski deleted the patch-priority-queue branch May 22, 2018 19:28
apapirovski added a commit that referenced this pull request May 22, 2018
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>
apapirovski added a commit that referenced this pull request May 22, 2018
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>
@Trott
Copy link
Member

Trott commented May 24, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. performance Issues and PRs related to the performance of Node.js. semver-major PRs that contain breaking changes and should be released in the next major version. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimizing 'unreferenced' timers