timers: cross JS/C++ border less frequently for setImmediate() #17064

addaleax · 2017-11-16T00:12:43Z

This removes the process._needImmediateCallback property
and its semantics of having a 1/0 switch that tells C++ whether
immediates are currently scheduled.

Instead, a counter keeping track of all immediates is created,
that can be increased on setImmediate() or decreased when an
immediate is run or cleared.

This is faster, because rather than writing/reading a C++ getter,
this operation can be performed as a direct memory read/write via
a typed array. The only C++ call that is left to make is
activating the native handles upon creation of the first
Immediate after the queue is empty.

One other (good!) side-effect is that immediate._destroyed now
reliably tells whether an immediate is still scheduled to run or not.

Also, as a nice extra, this should make it easier to implement
an internal variant of setImmediate for C++ that piggybacks
off the same mechanism, which should be useful at least for
async hooks and HTTP/2.

(/cc @jasnell so you’re aware of this last thing 🙂)

Benchmark results:

$ ./node benchmark/compare.js --new ./node --old ./node-master-1b093cb93df0 --runs 10 --filter immediate timers | Rscript benchmark/compare.R
[00:08:53|% 100| 4/4 files | 20/20 runs | 1/1 configs]: Done
                                                     improvement confidence      p.value
 timers/immediate.js type="breadth" thousands=2000      25.61 %         ** 1.432301e-03
 timers/immediate.js type="breadth1" thousands=2000      7.66 %            1.320233e-01
 timers/immediate.js type="breadth4" thousands=2000      4.61 %            5.669053e-01
 timers/immediate.js type="clear" thousands=2000       311.40 %        *** 3.896291e-07
 timers/immediate.js type="depth" thousands=2000        17.54 %         ** 9.755389e-03
 timers/immediate.js type="depth1" thousands=2000       17.09 %        *** 7.176229e-04
 timers/set-immediate-breadth-args.js millions=5        10.63 %          * 4.250034e-02
 timers/set-immediate-breadth.js millions=10            20.62 %        *** 9.150439e-07
 timers/set-immediate-depth-args.js millions=10         17.97 %        *** 6.819135e-10

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

timers

addaleax · 2017-11-16T00:15:01Z

🤞

CI: https://ci.nodejs.org/job/node-test-commit/14061/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1076/

Fishrock123 · 2017-11-16T00:18:55Z

Hmmm, I'm pretty @mscdex has touched the immediates code more than I have. It lives in the same file and uses some the same general structure but I haven't really touched the details much at all.

refack · 2017-11-16T00:32:35Z

lib/timers.js

+
+    if (async_hook_fields[kDestroy] > 0) {
+      emitDestroy(immediate[async_id_symbol]);
+    }
  }

  immediate._onImmediate = null;

  immediateQueue.remove(immediate);


Does it make sense to move this into the if (or even flip the if to if (immediate._destroyed) return;)

You can try it but all of this code is so fragile I’d rather not touch more than necessary

refack · 2017-11-16T00:40:06Z

src/node.cc

+  uv_check_start(env->immediate_check_handle(), CheckImmediate);
+  // Idle handle is needed only to stop the event loop from blocking in poll.
+  uv_idle_start(env->immediate_idle_handle(),
+                [](uv_idle_t*){ /* do nothing, just keep the loop running */ });


I see that the comment came from the previous impl, but it seems redundant with the comment above the line...

Maybe. One comment is describing the what, one comment is describing the why, so I’m okay with having two.

Edit: On second thought, yes, I guess this can be removed if you like

refack · 2017-11-16T00:42:10Z

src/node.cc

-    Local<Value> value,
-    const PropertyCallbackInfo<void>& info) {
-  Environment* env = Environment::GetCurrent(info);
+  if (MaybeStopImmediate(env))


What's the case here? A callback from uv after the Immediate was cleared on the JS side?
If so maybe add a comment?

Yes, that is one way to trigger this, but the check here isn’t semantically tied to that – the general idea is that if there’s nothing to run, don’t bother calling into JS, no matter why there is nothing to run.

(In particular, it’s not quite worked out yet how this is going to work with other native code also using this mechanism, as hinted at in the commit message, and it’s quite possible that that may not be the only way to trigger it.)

refack

👍

jasnell

Woo... Nice

bnoordhuis · 2017-11-16T15:43:16Z

src/env-inl.h

@@ -512,6 +513,11 @@ inline void Environment::set_fs_stats_field_array(double* fields) {
  fs_stats_field_array_ = fields;
 }

+inline AliasedBuffer<uint32_t, v8::Uint32Array>&
+Environment::scheduled_immediate_count() {


I'm not really a fan of returning a mutable reference. I realize other AliasedBuffer things do that too but IMO they shouldn't, it obscures whether call sites deal with a copy or a reference.

(I'll allow that it looks like a little nicer coupled with array syntax.)

I know. :) I don’t like it when one can’t tell the difference either, but in these cases it should be pretty clear.

Trott · 2017-11-16T19:34:50Z

All the code being removed/moved in lib/timers.js is covered by tests (although some of it just barely). If it's not too onerous, it would be nice to run a coverage report on this PR to make sure that all the new/changed/moved code in lib/timers.js is still covered.

This removes the `process._needImmediateCallback` property and its semantics of having a 1/0 switch that tells C++ whether immediates are currently scheduled. Instead, a counter keeping track of all immediates is created, that can be increased on `setImmediate()` or decreased when an immediate is run or cleared. This is faster, because rather than reading/writing a C++ getter, this operation can be performed as a direct memory read/write via a typed array. The only C++ call that is left to make is activating the native handles upon creation of the first `Immediate` after the queue is empty. One other (good!) side-effect is that `immediate._destroyed` now reliably tells whether an `immediate` is still scheduled to run or not. Also, as a nice extra, this should make it easier to implement an internal variant of `setImmediate` for C++ that piggybacks off the same mechanism, which should be useful at least for async hooks and HTTP/2. Benchmark results: $ ./node benchmark/compare.js --new ./node --old ./node-master-1b093cb93df0 --runs 10 --filter immediate timers | Rscript benchmark/compare.R [00:08:53|% 100| 4/4 files | 20/20 runs | 1/1 configs]: Done improvement confidence p.value timers/immediate.js type="breadth" thousands=2000 25.61 % ** 1.432301e-03 timers/immediate.js type="breadth1" thousands=2000 7.66 % 1.320233e-01 timers/immediate.js type="breadth4" thousands=2000 4.61 % 5.669053e-01 timers/immediate.js type="clear" thousands=2000 311.40 % *** 3.896291e-07 timers/immediate.js type="depth" thousands=2000 17.54 % ** 9.755389e-03 timers/immediate.js type="depth1" thousands=2000 17.09 % *** 7.176229e-04 timers/set-immediate-breadth-args.js millions=5 10.63 % * 4.250034e-02 timers/set-immediate-breadth.js millions=10 20.62 % *** 9.150439e-07 timers/set-immediate-depth-args.js millions=10 17.97 % *** 6.819135e-10

addaleax · 2017-11-16T20:09:25Z

@Trott Thanks for the suggestion! The coverage for existing code remains the same with this patch, so that’s nice.

That being said – 100 % coverage for timers would be really nice…

addaleax · 2017-11-18T00:51:24Z

Landed in ae558af

This removes the `process._needImmediateCallback` property and its semantics of having a 1/0 switch that tells C++ whether immediates are currently scheduled. Instead, a counter keeping track of all immediates is created, that can be increased on `setImmediate()` or decreased when an immediate is run or cleared. This is faster, because rather than reading/writing a C++ getter, this operation can be performed as a direct memory read/write via a typed array. The only C++ call that is left to make is activating the native handles upon creation of the first `Immediate` after the queue is empty. One other (good!) side-effect is that `immediate._destroyed` now reliably tells whether an `immediate` is still scheduled to run or not. Also, as a nice extra, this should make it easier to implement an internal variant of `setImmediate` for C++ that piggybacks off the same mechanism, which should be useful at least for async hooks and HTTP/2. Benchmark results: $ ./node benchmark/compare.js --new ./node --old ./node-master-1b093cb93df0 --runs 10 --filter immediate timers | Rscript benchmark/compare.R [00:08:53|% 100| 4/4 files | 20/20 runs | 1/1 configs]: Done improvement confidence p.value timers/immediate.js type="breadth" thousands=2000 25.61 % ** 1.432301e-03 timers/immediate.js type="breadth1" thousands=2000 7.66 % 1.320233e-01 timers/immediate.js type="breadth4" thousands=2000 4.61 % 5.669053e-01 timers/immediate.js type="clear" thousands=2000 311.40 % *** 3.896291e-07 timers/immediate.js type="depth" thousands=2000 17.54 % ** 9.755389e-03 timers/immediate.js type="depth1" thousands=2000 17.09 % *** 7.176229e-04 timers/set-immediate-breadth-args.js millions=5 10.63 % * 4.250034e-02 timers/set-immediate-breadth.js millions=10 20.62 % *** 9.150439e-07 timers/set-immediate-depth-args.js millions=10 17.97 % *** 6.819135e-10 PR-URL: #17064 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins · 2017-12-11T22:11:16Z


edit: nvm I got it too land after other Prs were backported!

This removes the `process._needImmediateCallback` property and its semantics of having a 1/0 switch that tells C++ whether immediates are currently scheduled. Instead, a counter keeping track of all immediates is created, that can be increased on `setImmediate()` or decreased when an immediate is run or cleared. This is faster, because rather than reading/writing a C++ getter, this operation can be performed as a direct memory read/write via a typed array. The only C++ call that is left to make is activating the native handles upon creation of the first `Immediate` after the queue is empty. One other (good!) side-effect is that `immediate._destroyed` now reliably tells whether an `immediate` is still scheduled to run or not. Also, as a nice extra, this should make it easier to implement an internal variant of `setImmediate` for C++ that piggybacks off the same mechanism, which should be useful at least for async hooks and HTTP/2. Benchmark results: $ ./node benchmark/compare.js --new ./node --old ./node-master-1b093cb93df0 --runs 10 --filter immediate timers | Rscript benchmark/compare.R [00:08:53|% 100| 4/4 files | 20/20 runs | 1/1 configs]: Done improvement confidence p.value timers/immediate.js type="breadth" thousands=2000 25.61 % ** 1.432301e-03 timers/immediate.js type="breadth1" thousands=2000 7.66 % 1.320233e-01 timers/immediate.js type="breadth4" thousands=2000 4.61 % 5.669053e-01 timers/immediate.js type="clear" thousands=2000 311.40 % *** 3.896291e-07 timers/immediate.js type="depth" thousands=2000 17.54 % ** 9.755389e-03 timers/immediate.js type="depth1" thousands=2000 17.09 % *** 7.176229e-04 timers/set-immediate-breadth-args.js millions=5 10.63 % * 4.250034e-02 timers/set-immediate-breadth.js millions=10 20.62 % *** 9.150439e-07 timers/set-immediate-depth-args.js millions=10 17.97 % *** 6.819135e-10 PR-URL: #17064 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

gibfahn · 2017-12-19T22:56:45Z

Should this land on 6.x and 8.x? I'm leaning towards leaving it to bake for a while first, thoughts?

This removes the `process._needImmediateCallback` property and its semantics of having a 1/0 switch that tells C++ whether immediates are currently scheduled. Instead, a counter keeping track of all immediates is created, that can be increased on `setImmediate()` or decreased when an immediate is run or cleared. This is faster, because rather than reading/writing a C++ getter, this operation can be performed as a direct memory read/write via a typed array. The only C++ call that is left to make is activating the native handles upon creation of the first `Immediate` after the queue is empty. One other (good!) side-effect is that `immediate._destroyed` now reliably tells whether an `immediate` is still scheduled to run or not. Also, as a nice extra, this should make it easier to implement an internal variant of `setImmediate` for C++ that piggybacks off the same mechanism, which should be useful at least for async hooks and HTTP/2. Benchmark results: $ ./node benchmark/compare.js --new ./node --old ./node-master-1b093cb93df0 --runs 10 --filter immediate timers | Rscript benchmark/compare.R [00:08:53|% 100| 4/4 files | 20/20 runs | 1/1 configs]: Done improvement confidence p.value timers/immediate.js type="breadth" thousands=2000 25.61 % ** 1.432301e-03 timers/immediate.js type="breadth1" thousands=2000 7.66 % 1.320233e-01 timers/immediate.js type="breadth4" thousands=2000 4.61 % 5.669053e-01 timers/immediate.js type="clear" thousands=2000 311.40 % *** 3.896291e-07 timers/immediate.js type="depth" thousands=2000 17.54 % ** 9.755389e-03 timers/immediate.js type="depth1" thousands=2000 17.09 % *** 7.176229e-04 timers/set-immediate-breadth-args.js millions=5 10.63 % * 4.250034e-02 timers/set-immediate-breadth.js millions=10 20.62 % *** 9.150439e-07 timers/set-immediate-depth-args.js millions=10 17.97 % *** 6.819135e-10 PR-URL: nodejs#17064

This removes the `process._needImmediateCallback` property and its semantics of having a 1/0 switch that tells C++ whether immediates are currently scheduled. Instead, a counter keeping track of all immediates is created, that can be increased on `setImmediate()` or decreased when an immediate is run or cleared. This is faster, because rather than reading/writing a C++ getter, this operation can be performed as a direct memory read/write via a typed array. The only C++ call that is left to make is activating the native handles upon creation of the first `Immediate` after the queue is empty. One other (good!) side-effect is that `immediate._destroyed` now reliably tells whether an `immediate` is still scheduled to run or not. Also, as a nice extra, this should make it easier to implement an internal variant of `setImmediate` for C++ that piggybacks off the same mechanism, which should be useful at least for async hooks and HTTP/2. Benchmark results: $ ./node benchmark/compare.js --new ./node --old ./node-master-1b093cb93df0 --runs 10 --filter immediate timers | Rscript benchmark/compare.R [00:08:53|% 100| 4/4 files | 20/20 runs | 1/1 configs]: Done improvement confidence p.value timers/immediate.js type="breadth" thousands=2000 25.61 % ** 1.432301e-03 timers/immediate.js type="breadth1" thousands=2000 7.66 % 1.320233e-01 timers/immediate.js type="breadth4" thousands=2000 4.61 % 5.669053e-01 timers/immediate.js type="clear" thousands=2000 311.40 % *** 3.896291e-07 timers/immediate.js type="depth" thousands=2000 17.54 % ** 9.755389e-03 timers/immediate.js type="depth1" thousands=2000 17.09 % *** 7.176229e-04 timers/set-immediate-breadth-args.js millions=5 10.63 % * 4.250034e-02 timers/set-immediate-breadth.js millions=10 20.62 % *** 9.150439e-07 timers/set-immediate-depth-args.js millions=10 17.97 % *** 6.819135e-10 PR-URL: nodejs#17064 Backport-PR-URL: nodejs#18179 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This removes the `process._needImmediateCallback` property and its semantics of having a 1/0 switch that tells C++ whether immediates are currently scheduled. Instead, a counter keeping track of all immediates is created, that can be increased on `setImmediate()` or decreased when an immediate is run or cleared. This is faster, because rather than reading/writing a C++ getter, this operation can be performed as a direct memory read/write via a typed array. The only C++ call that is left to make is activating the native handles upon creation of the first `Immediate` after the queue is empty. One other (good!) side-effect is that `immediate._destroyed` now reliably tells whether an `immediate` is still scheduled to run or not. Also, as a nice extra, this should make it easier to implement an internal variant of `setImmediate` for C++ that piggybacks off the same mechanism, which should be useful at least for async hooks and HTTP/2. Benchmark results: $ ./node benchmark/compare.js --new ./node --old ./node-master-1b093cb93df0 --runs 10 --filter immediate timers | Rscript benchmark/compare.R [00:08:53|% 100| 4/4 files | 20/20 runs | 1/1 configs]: Done improvement confidence p.value timers/immediate.js type="breadth" thousands=2000 25.61 % ** 1.432301e-03 timers/immediate.js type="breadth1" thousands=2000 7.66 % 1.320233e-01 timers/immediate.js type="breadth4" thousands=2000 4.61 % 5.669053e-01 timers/immediate.js type="clear" thousands=2000 311.40 % *** 3.896291e-07 timers/immediate.js type="depth" thousands=2000 17.54 % ** 9.755389e-03 timers/immediate.js type="depth1" thousands=2000 17.09 % *** 7.176229e-04 timers/set-immediate-breadth-args.js millions=5 10.63 % * 4.250034e-02 timers/set-immediate-breadth.js millions=10 20.62 % *** 9.150439e-07 timers/set-immediate-depth-args.js millions=10 17.97 % *** 6.819135e-10 Backport-PR-URL: #18179 PR-URL: #17064 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. performance Issues and PRs related to the performance of Node.js. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Nov 16, 2017

addaleax requested a review from Fishrock123 November 16, 2017 00:12

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Nov 16, 2017

refack reviewed Nov 16, 2017

View reviewed changes

refack approved these changes Nov 16, 2017

View reviewed changes

JungMinu approved these changes Nov 16, 2017

View reviewed changes

jasnell approved these changes Nov 16, 2017

View reviewed changes

bnoordhuis approved these changes Nov 16, 2017

View reviewed changes

Fishrock123 removed their request for review November 16, 2017 18:49

addaleax force-pushed the timers-immediate-improve branch from 6e26656 to 8310986 Compare November 16, 2017 19:37

addaleax closed this Nov 18, 2017

addaleax deleted the timers-immediate-improve branch November 18, 2017 00:51

MylesBorins added the backport-requested-v9.x label Dec 11, 2017

apapirovski mentioned this pull request Dec 12, 2017

timers: clean up for readability #17279

Closed

3 tasks

MylesBorins removed the backport-requested-v9.x label Dec 12, 2017

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

gibfahn added the lts-watch-v6.x label Dec 19, 2017

gibfahn added the lts-watch-v8.x label Dec 19, 2017

gibfahn added the baking-for-lts PRs that need to wait before landing in a LTS release. label Dec 19, 2017

addaleax mentioned this pull request Jan 10, 2018

[v8.x] backport timers perf improvement & C++ SetImmediate() #18086

Closed

addaleax added the backported-to-v8.x label Jan 10, 2018

AndreasMadsen mentioned this pull request Jan 16, 2018

Backport async hooks to v8.x #18179

Closed

4 tasks

MylesBorins removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: cross JS/C++ border less frequently for setImmediate() #17064

timers: cross JS/C++ border less frequently for setImmediate() #17064

addaleax commented Nov 16, 2017

addaleax commented Nov 16, 2017

Fishrock123 commented Nov 16, 2017 •

edited

Loading

refack Nov 16, 2017

addaleax Nov 16, 2017

refack Nov 16, 2017

addaleax Nov 16, 2017 •

edited

Loading

refack Nov 16, 2017 •

edited

Loading

addaleax Nov 16, 2017 •

edited

Loading

refack left a comment

jasnell left a comment

bnoordhuis Nov 16, 2017

addaleax Nov 16, 2017

Trott commented Nov 16, 2017 •

edited

Loading

addaleax commented Nov 16, 2017

addaleax commented Nov 18, 2017

MylesBorins commented Dec 11, 2017 •

edited

Loading

gibfahn commented Dec 19, 2017

timers: cross JS/C++ border less frequently for setImmediate() #17064

timers: cross JS/C++ border less frequently for setImmediate() #17064

Conversation

addaleax commented Nov 16, 2017

Checklist

Affected core subsystem(s)

addaleax commented Nov 16, 2017

Fishrock123 commented Nov 16, 2017 • edited Loading

refack Nov 16, 2017

Choose a reason for hiding this comment

addaleax Nov 16, 2017

Choose a reason for hiding this comment

refack Nov 16, 2017

Choose a reason for hiding this comment

addaleax Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

refack Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

addaleax Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

bnoordhuis Nov 16, 2017

Choose a reason for hiding this comment

addaleax Nov 16, 2017

Choose a reason for hiding this comment

Trott commented Nov 16, 2017 • edited Loading

addaleax commented Nov 16, 2017

addaleax commented Nov 18, 2017

MylesBorins commented Dec 11, 2017 • edited Loading

gibfahn commented Dec 19, 2017

Fishrock123 commented Nov 16, 2017 •

edited

Loading

addaleax Nov 16, 2017 •

edited

Loading

refack Nov 16, 2017 •

edited

Loading

addaleax Nov 16, 2017 •

edited

Loading

Trott commented Nov 16, 2017 •

edited

Loading

MylesBorins commented Dec 11, 2017 •

edited

Loading