timers: clean up for readability #17279

apapirovski · 2017-11-23T19:06:31Z

Just doing a little bit of clean up in timers to — hopefully — make it a bit easier to grok.

Remove micro-optimizations that no longer yield any benefits
Simplify Timeout & Immediate to just run all their setup code in the constructor instead of needing extra helper functions
Simplify Timeout to just have a single argument that determines whether it's repeating or not, instead of having two separate helper functions that differ in only one line
Adjust timers benchmarks to run long enough to offer meaningful data

I ran a benchmark locally and the performance seemed to remain equivalent but will start a Benchmark CI immediately to confirm.

Checklist

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

Affected core subsystem(s)

benchmark, timers

apapirovski · 2017-11-23T19:17:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11680/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/59/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1106/ (why not)

mscdex · 2017-11-23T20:06:08Z

lib/timers.js

+
+    this[async_id_symbol] = ++async_id_fields[kAsyncIdCounter];
+    this[trigger_async_id_symbol] = initTriggerId();
+    if (async_hook_fields[kInit] > 0)


Please use braces for multi-line bodies.

This is just relocated code. Happy to adjust though.

mscdex · 2017-11-23T20:07:31Z

lib/timers.js

+    }
+    return this;
+  }
+}

 function setTimeout(callback, after, arg1, arg2, arg3) {


Did you benchmark using rest args here?

Yeah, still a no go.

mscdex · 2017-11-23T20:08:43Z

lib/timers.js

+      break;
+    default:
+      args = [arg1, arg2, arg3];
+      for (i = 5; i < arguments.length; i++)


I know this wasn't the case previously, but can you also add braces here please?

mscdex · 2017-11-23T20:10:08Z

lib/timers.js

@@ -47,6 +47,8 @@ const { kInit, kDestroy, kAsyncIdCounter } = async_wrap.constants;
 const async_id_symbol = Symbol('asyncId');
 const trigger_async_id_symbol = Symbol('triggerAsyncId');

+const functionApply = Function.prototype.apply;


I'm not sure we can safely do this. This was kind of explicitly avoided in 98609fc. If we do this, it means we may not pick up a monkey-patched version of apply() during runtime.

Yeah, so the question here is whether we want to pick up a monkey patched version of apply or not? I mean, my instinct says 'no' (and part of the reason for this adjustment) because realistically the internals could change again in the future to where apply is not even used anymore.

/cc @nodejs/tsc

I think the answer here might actually be different and that is, use Reflect.apply — assuming performance holds up.

Switched to Reflect.apply which seems correct here. Quick test revealed no differences in performance. Open to other thoughts on this though.

mscdex · 2017-11-23T20:12:44Z

lib/timers.js

-      this[async_id_symbol], 'Immediate', this[trigger_async_id_symbol], this
-    );
+class Immediate {
+  constructor(args) {


What if we allow the _onImmediate value to be passed to the constructor to save a step?

See the comment right below... that's what I wanted to do but it runs 3x slower on our benchmarks. I have not figured out what exactly causes it to plummet.

It's also why it was like this before too.

Ah ok, if I had to guess I would say it might be the different types (Promise vs function). *shrug*

But then it's fine for Timeout (and it was in its constructor originally)... so I have no clue. Maybe @bmeurer can shed some light on this for us?

My guess here is that this trigger function tracking on the hidden class. Have you tried passing it to constructor, but setting the field _onImmediate to null before and only then store the value?

Will try, thanks for the tip!

That did it!

Hey 👋
I know that it's a pretty old PR, but I was studying this code, and these lines caught my attention:

// this must be set to null first to avoid function tracking // on the hidden class, revisit in V8 versions after 6.2 this._onTimeout = null; this._onTimeout = callback;

Why in these cases the hidden class optimization is spoiling the performance instead of improving?
Is it still happen?

Fishrock123

Seems reasonable, certainly makes the code nicer.

One comment to be addressed.

Some history here. I was kinda not for this back in #12745, although I suppose this is not quite as aggressive.

My comment from then still stands, this may be rather difficult to backport and backport fixes from - would be able to check how much work that might be?

Also, @TimothyGu's comment from last time may still be relevant and may make this semver-major.

Is changing to ES6 classes backwards compatible? The prototype methods will no longer be enumerable after such a change.

Fishrock123 · 2017-11-23T20:37:33Z

lib/timers.js

@@ -424,80 +426,148 @@ exports.enroll = function(item, msecs) {
 * DOM-style timers
 */

+class Timeout {
+  constructor(callback, after, args, isRepeat) {


Can args and isRepeat go into an options object? It is foreseeable that: a) we may expose the constructor, and b) that we may want to also have an unrefed option.

They could but then we're creating an extra object just to pass them to the constructor (which just reassigns them anyway). I would maybe leave that change to when we actually do decide to expose as it will have a small performance implication? Just thinking out loud here.

Fishrock123 · 2017-11-23T20:39:54Z

lib/timers.js

-    }
-  }
+  else
+    Reflect.apply(timer._onTimeout, timer, args);


This performs well? It is certainly much nicer. have any idea how this might be on older versions of Node?

Yeah, it's now equivalent to what was there before.

Forgot to address the second part, I don't think we can switch over on older Node.js versions hence the labels on this PR. That said, I'm planning to run the benchmarks on v8.x to confirm.

Fishrock123 · 2017-11-23T20:44:49Z

lib/timers.js

-Timeout.prototype.close = function() {
-  this._onTimeout = null;
-  if (this._handle) {
-    // Fewer checks may be possible, but these cover everything.


Did you check this in detail? If not, can you please preserve the comment?

I don't think we can do less checks there (I followed the breadcrumb trail as much as I could) but happy to put the comment back.

(Also, I did remove an unnecessary this check.)

Ok sounds good then.

Fishrock123 · 2017-11-23T20:45:34Z

lib/timers.js

+    this._idleNext = null;
+    this._idlePrev = null;
+    // this must be set to null first to avoid function tracking
+    // on the hidden class, revisit in V8 versions after 6.2


This is a thing? ... Interesting.

apapirovski · 2017-11-23T21:23:59Z

Some history here. I was kinda not for this back in #12745, although I suppose this is not quite as aggressive. My comment from then still stands, this may be rather difficult to backport and backport fixes from - would be able to check how much work that might be?

Yeah, I'm open to hearing other people's thoughts on this so we can try to come to a consensus. Also, it would be fairly easy to convert the Timeout class back to how it was and still preserve all of the other relevant changes in this PR.

Since we're no longer using helper functions to create the Timeout & Immediate instances (meaning those classes are instantiated in a few places), I thought having them as ES6 classes made it slightly more readable and clear than having them as functions that are actually classes. But yeah, it's not too painful to change back and preserve the other changes in this PR.

apapirovski · 2017-11-24T01:42:21Z

Ran a Benchmark on the CI just to make sure and here are the results:

 timers/immediate.js type="breadth1" thousands=5000     -2.63 %        *** 5.634607e-05
 timers/immediate.js type="breadth4" thousands=5000      0.49 %            7.652080e-01
 timers/immediate.js type="breadth" thousands=5000       2.51 %        *** 9.290645e-05
 timers/immediate.js type="clear" thousands=5000         0.93 %            3.366699e-01
 timers/immediate.js type="depth1" thousands=5000       -1.27 %          * 1.191555e-02
 timers/immediate.js type="depth" thousands=5000        -4.23 %        *** 7.135043e-16
 timers/set-immediate-breadth-args.js millions=5         3.29 %            6.900801e-02
 timers/set-immediate-breadth.js millions=10             2.18 %        *** 6.599080e-06
 timers/set-immediate-depth-args.js millions=5          -0.48 %            3.170515e-01
 timers/timers-breadth.js thousands=5000                 1.25 %         ** 2.818523e-03
 timers/timers-cancel-pooled.js millions=5               0.62 %            5.948548e-01
 timers/timers-cancel-unpooled.js millions=1            97.49 %        *** 2.412041e-45
 timers/timers-depth.js thousands=1                     -0.01 %            9.215241e-01
 timers/timers-insert-pooled.js millions=5               2.49 %        *** 1.611240e-05
 timers/timers-insert-unpooled.js millions=1             1.53 %            2.240269e-01
 timers/timers-timeout-pooled.js millions=10           -22.55 %        *** 4.849235e-27

No clue what's going on with the timers-timeout-pooled.js, while I'm pretty sure timers-cancel-unpooled.js is hitting the "function tracking" issue discussed above and the new version fixes that. I would like to fix timers-timeout-pooled.js before landing this although I'm not convinced that benchmark is 100% representative.

Fishrock123 · 2017-11-24T17:22:38Z

Also, it would be fairly easy to convert the Timeout class back to how it was and still preserve all of the other relevant changes in this PR.

Do you think you could at least split it into two commits, other improvements first? That would make things easier.

apapirovski · 2017-11-24T19:02:09Z

@Fishrock123 Yeah, I should be able to split this up over the weekend.

apapirovski · 2017-11-25T13:53:02Z

I've changed my mind, I don't think this really needs ES6 classes. All the 'real' changes from this PR can exist without them. PTAL @mscdex & @Fishrock123.

apapirovski · 2017-11-25T13:54:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11729/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/61/

apapirovski · 2017-11-25T18:08:10Z

New benchmark results, was able to resolve all regressions and we've got a major bump on one benchmark:

 timers/immediate.js type="breadth1" thousands=5000     -2.58 %        *** 6.584465e-07
 timers/immediate.js type="breadth4" thousands=5000     -0.37 %            8.223856e-01
 timers/immediate.js type="breadth" thousands=5000       2.12 %         ** 1.340324e-03
 timers/immediate.js type="clear" thousands=5000         2.02 %          * 3.461550e-02
 timers/immediate.js type="depth1" thousands=5000       -0.03 %            9.631820e-01
 timers/immediate.js type="depth" thousands=5000        -3.02 %        *** 1.611862e-07
 timers/set-immediate-breadth-args.js millions=5         2.42 %            2.093568e-01
 timers/set-immediate-breadth.js millions=10             1.83 %        *** 4.767013e-06
 timers/set-immediate-depth-args.js millions=5          -0.46 %            2.570641e-01
 timers/timers-breadth.js thousands=5000                 1.69 %        *** 3.210408e-05
 timers/timers-cancel-pooled.js millions=5               1.45 %            2.326928e-01
 timers/timers-cancel-unpooled.js millions=1            95.99 %        *** 8.813862e-41
 timers/timers-depth.js thousands=1                     -0.05 %            5.132490e-01
 timers/timers-insert-pooled.js millions=5               2.13 %        *** 1.252006e-05
 timers/timers-insert-unpooled.js millions=1             4.07 %         ** 4.065701e-03
 timers/timers-timeout-pooled.js millions=10             4.13 %            3.526779e-01

(And just to be clear, I'm not trying to get performance boosts here. Just making sure we're not regressing while improving readability of this code.)

Remove micro-optimizations that no longer yield any benefits, restructure timers & immediates to be a bit more straightforward. Adjust timers benchmarks to run long enough to offer meaningful data.

apapirovski · 2017-11-28T13:52:09Z

@Fishrock123 @mscdex Any objections to this landing? Not using ES6 classes any longer so this is strictly readability refactoring.

refack · 2017-11-28T17:27:29Z

benchmark/timers/timers-timeout-pooled.js

+  function cb2() {}
+
+  process.on('exit', function() {
+    bench.end(iterations / 1e6);


Not loving this... It's async, so it could add noise, also having the callbacks have side effects improves the chances they are not optimized away.
I'd rather have duplicate cb() code, or have cb just count++ and cb2 test for exit.

Addressed with duplicate code.

refack · 2017-11-28T17:33:58Z

benchmark/timers/timers-timeout-pooled.js


  for (var i = 0; i < iterations; i++) {
-    setTimeout(cb, 1);
+    setTimeout(i % 2 ? cb : cb2, 1);


Suggestion: add a comment that this sets up n*1E6 timeout objects which will all get called together on next uv tick.
Which BTW mean this mesures:

setup n*1E6 timeouts

end tick - exit JS

poll in uv for 1ms

reenter JS

call n*1E6 callbacks

I wish we could exclude steps 2-4 from the measurement 🤔

Added a comment.

refack · 2017-11-28T17:52:02Z

Regarding backporting. https://github.com/nodejs/node/commits/v6.x/lib/timers.js shows 8 commits in 2017. So if used to estimate churn, indicates this is a very stable module.

TimothyGu · 2017-11-28T19:08:38Z

lib/timers.js

-    }
-  }
+  else
+    Reflect.apply(timer._onTimeout, timer, args);


How well does this compare with timer._onTimeout(...args)?

It's much faster, I don't have the numbers anymore but I tested that version first. In general, rest params in arguments perform reasonably well in most situations but spread syntax is still pretty slow.

apapirovski · 2017-11-28T21:33:05Z

Landed in d8bc4f2

Remove micro-optimizations that no longer yield any benefits, restructure timers & immediates to be a bit more straightforward. Adjust timers benchmarks to run long enough to offer meaningful data. PR-URL: #17279 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

MylesBorins · 2017-12-11T22:35:41Z


edit: it landed after other PRs were backported

apapirovski · 2017-12-12T00:15:15Z

I think this should land cleanly once #17064 gets back-ported.

Remove micro-optimizations that no longer yield any benefits, restructure timers & immediates to be a bit more straightforward. Adjust timers benchmarks to run long enough to offer meaningful data. PR-URL: #17279 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

apapirovski added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. dont-land-on-v4.x labels Nov 23, 2017

apapirovski force-pushed the patch-timers-perf branch 2 times, most recently from 764ec52 to faa30f3 Compare November 23, 2017 19:15

mscdex reviewed Nov 23, 2017

View reviewed changes

apapirovski force-pushed the patch-timers-perf branch from 3903245 to 118e205 Compare November 23, 2017 20:22

Fishrock123 reviewed Nov 23, 2017

View reviewed changes

apapirovski force-pushed the patch-timers-perf branch from 744284e to 40adcde Compare November 23, 2017 22:00

apapirovski force-pushed the patch-timers-perf branch 3 times, most recently from 8709eb3 to e094ee2 Compare November 25, 2017 13:43

jasnell approved these changes Nov 25, 2017

View reviewed changes

apapirovski added 2 commits November 26, 2017 10:24

timers: clean up for readability

a77330e

Remove micro-optimizations that no longer yield any benefits, restructure timers & immediates to be a bit more straightforward. Adjust timers benchmarks to run long enough to offer meaningful data.

fixup: do not set domain in constructor

eff64e1

apapirovski force-pushed the patch-timers-perf branch from e094ee2 to eff64e1 Compare November 26, 2017 15:25

refack approved these changes Nov 28, 2017

View reviewed changes

fixup: address feedback on benchmark changes

4805558

refack approved these changes Nov 28, 2017

View reviewed changes

TimothyGu approved these changes Nov 28, 2017

View reviewed changes

apapirovski closed this Nov 28, 2017

apapirovski deleted the patch-timers-perf branch November 28, 2017 21:33

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

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

timers: clean up for readability #17279

timers: clean up for readability #17279

Conversation

apapirovski commented Nov 23, 2017 • edited Loading

Checklist

Affected core subsystem(s)

apapirovski commented Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Nov 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Nov 23, 2017

apapirovski commented Nov 24, 2017

Fishrock123 commented Nov 24, 2017

apapirovski commented Nov 24, 2017

apapirovski commented Nov 25, 2017

apapirovski commented Nov 25, 2017 • edited Loading

apapirovski commented Nov 25, 2017 • edited Loading

apapirovski commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Nov 28, 2017

MylesBorins commented Dec 11, 2017 • edited Loading

apapirovski commented Dec 12, 2017

apapirovski commented Nov 23, 2017 •

edited

Loading

apapirovski commented Nov 23, 2017 •

edited

Loading

mscdex Nov 23, 2017 •

edited

Loading

apapirovski Nov 23, 2017 •

edited

Loading

apapirovski Nov 25, 2017 •

edited

Loading

apapirovski commented Nov 25, 2017 •

edited

Loading

apapirovski commented Nov 25, 2017 •

edited

Loading

MylesBorins commented Dec 11, 2017 •

edited

Loading