process: improve nextTick() performance#13446

mscdex · 2017-06-04T03:39:00Z

I recently noticed that with the introduction of Async Hooks also came the (perhaps unintentional) reverting of 804d57d. I actually tried a lot of different solutions, but almost all of them resulted in regressions in other nextTick() benchmarks (e.g. a perf boost in no args case, but regression in args case) which I still don't quite understand. One thing I did notice is that the ideal solution of just using a plain object and embedding the symbol properties as computed properties causes TurboFan to kick in and that causes a significant slowdown for nextTick().

I was able to improve the existing non-ES6-based constructor solution, but it turns out using an ES6 class improves performance even more (even surpassing the plain object-based solutions that performed better in whichever benchmarks), so that is what I decided on.

Results:

 improvement confidence p.value process/next-tick-breadth-args.js millions=2 27.75 % *** 1.271176e-20 process/next-tick-breadth.js millions=2 7.71 % *** 4.155765e-13 process/next-tick-depth-args.js millions=12 47.78 % *** 4.150674e-52 process/next-tick-depth.js millions=12 47.32 % *** 7.742778e-31

CI: https://ci.nodejs.org/job/node-test-pull-request/8464/

Checklist

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

Affected core subsystem(s)

process

refack

LGTM (just some nits)

refack · 2017-06-04T03:55:14Z

lib/internal/process/next_tick.js

Just wondering... It this a style choice or a DEOPT?

Style choice, I think we tend to use braces for multi-line conditionals from what I've generally seen, this is just one "offender" I ran across while working in this file.

refack · 2017-06-04T04:06:16Z

lib/internal/process/next_tick.js

I'm not 100% with so much side effects in a constructor...
If it's not a performance hit I'd rather see these four lines in a static method or factory
If it is a performance hit, I think it should be documented:
// These side effects were folded into the c'tor for performance
(or something to that effect)

You can't move these other statements back into nextTick() without causing a performance regression from what I saw. I believe it's because of Crankshaft's inlining limits, but I did not want to introduce regressions at this moment (we will have to deal with it once TurboFan is everywhere).
Unfortunately internalNextTick() exceeds these limits, so there's not much I can do there.

Yeah so probably even a comment will "irritate" Crankshaft
What can you do 🤷‍♂️

Hopefully once V8 (or perhaps they already do in later versions) is able to better optimize computed properties in TurboFan, we can use plain objects again without losing performance.

Fishrock123

lgtm w comments

Fishrock123 · 2017-06-05T12:51:01Z

lib/internal/process/next_tick.js

Can we clarify the comment rather than removing it?

Removing it matches internalNextTick() and it doesn't seem like a particularly helpful comment as process._exiting is pretty self-explanatory to me.

Yes, the code says what but I'm wondering if it may be helpful to say why.

FWIW Crankshaft can decide not to inline a function because it has a comment in it 😞 so if we come up with a better comment it probably should go in line 231.

Eh, it makes little difference as the comment was already there and that will soon be an obsolete optimization as I understand.

I don't understand what you mean by a 'why comment.'
AIUI it's a comment explaining why we're doing this (as opposed to explaining what this code does).

@gibfahn I understand that part. What I meant was I don't understand what this is in reference to. Is it the process._exiting line? The changes I'm making? Something else? What is the suggested 'why comment' ?

What is the suggested 'why comment' ?
(AIUI) This is the comment, it's a "why comment", it's helpful to preserve it.
// on the way out, don't bother. it won't get fired anyway.
FWIW I'm sure putting it above the function is fine.

@Fishrock123 @gibfahn I've added something above both functions now, let me know if that is what you had in mind or not.

@Fishrock123 @gibfahn I've added something above both functions now, let me know if that is what you had in mind or not.
Clearer than the original, thanks.

Fishrock123 · 2017-06-05T12:52:41Z

lib/internal/process/next_tick.js

Is there a reason this is var over const?

I think it was triggering a deopt.

refack · 2017-06-05T13:22:31Z

lib/internal/process/next_tick.js

// we bail fast `if (process._exiting)` since the loop has effectively stopped// and there will never be a `nextTick`

@Fishrock123 PTAL

AndreasMadsen · 2017-06-05T14:09:45Z

benchmark/process/next-tick-depth-args.js

Why this increase? You are getting ridiculous statistical confidence. By some quick calculation, this should only improve the confidence by a factor of 2.5. Compensating for that and the previous version should also give you confidence well beyond ***.

I always choose something large enough to allow for any re-opts to occur and for results to stabilize. With 2, I was getting results that varied quite a bit in comparison because it was finishing so quickly (at least on my machine).

mscdex · 2017-06-07T18:34:47Z

FWIW with V8 5.9 in master I now get these results with this PR as-is:

 improvement confidence p.value process/next-tick-breadth-args.js millions=2 6.17 % *** 2.725997e-04 process/next-tick-breadth.js millions=2 6.68 % ** 1.568356e-03 process/next-tick-depth-args.js millions=12 46.71 % *** 2.458707e-81 process/next-tick-depth.js millions=12 43.66 % *** 3.338435e-66

I'm not sure how breadth-args was so much higher previously, I would've expected results more like I am seeing now in 5.9, where the args and non-args are much closer together.

mscdex · 2017-06-08T19:21:06Z

Ok, I've made a couple more adjustments:

Moved the non-assignment code out of the TickObject class constructor as the difference in code location no longer makes any difference with TurboFan in V8 5.9
Switched from an array to a linked list for storing TickObject instances, this increases performance more significantly than the original changes alone. I think it may be possible to make some further optimizations to avoid use of the tickInfo counters shared with C++ land (at least the kIndex counter), however I've decided to leave it alone for now.

Here are the results as of these latest changes:

 improvement confidence p.value process/next-tick-breadth-args.js millions=4 135.31 % *** 3.011128e-73 process/next-tick-breadth.js millions=4 361.40 % *** 5.097232e-76 process/next-tick-depth-args.js millions=12 44.08 % *** 1.276617e-66 process/next-tick-depth.js millions=12 43.85 % *** 1.333610e-49

CI: https://ci.nodejs.org/job/node-test-pull-request/8561/

mscdex · 2017-06-08T23:14:58Z

@refack @Fishrock123 @jasnell @evanlucas Please review these recent changes.

TimothyGu · 2017-06-09T00:02:21Z

lib/internal/process/next_tick.js

This is faster than spread parameters even with V8 5.9?

I'm not sure how spread could be used here. Do you mean rest parameters? If so, I tried that and while it's a little faster in one benchmark, it's slower to varying degrees in all others.

Yeah, that's what I meant. Sorry, could never get all the terminologies right.

refack · 2017-06-09T02:40:03Z

So I'm just writing down what I saw (writing down mostly to organize my thoughts):

the linked list queue
inlined setupInit X 2
switch for args X 2
the const microTasksTickObject

Anything else of significance?

mscdex · 2017-06-09T04:03:25Z

@refack That's mostly it, unless you want to also count the switch to ES6 class for TickObject.

refack · 2017-06-09T04:18:32Z

Thanks for the confirmation ✔️

mscdex · 2017-06-16T07:27:41Z

Rebased. If there are no objections in the next 24 hours I will assume all who approved the original changes also approve of these latest changes that I previously outlined.

PR-URL: nodejs#13446 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

PR-URL: #13446 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

mscdex added performance Issues and PRs related to the performance of Node.js. process Issues and PRs related to the process subsystem. labels Jun 4, 2017

refack approved these changes Jun 4, 2017
View reviewed changes

jasnell approved these changes Jun 4, 2017
View reviewed changes

Fishrock123 approved these changes Jun 5, 2017
View reviewed changes

refack reviewed Jun 5, 2017
View reviewed changes

AndreasMadsen reviewed Jun 5, 2017
View reviewed changes

mscdex force-pushed the process-nexttick-perf branch 2 times, most recently from ddc2508 to 447162bCompare June 5, 2017 20:26

mscdex mentioned this pull request Jun 7, 2017
http: always cork outgoing writes #13522
Merged
3 tasks

evanlucas approved these changes Jun 7, 2017
View reviewed changes

mscdex added the wip Issues and PRs that are still a work in progress. label Jun 8, 2017

mscdex force-pushed the process-nexttick-perf branch from 447162b to d68f8baCompare June 8, 2017 19:16

mscdex removed the wip Issues and PRs that are still a work in progress. label Jun 8, 2017

TimothyGu reviewed Jun 9, 2017
View reviewed changes

refack approved these changes Jun 9, 2017
View reviewed changes

mscdex force-pushed the process-nexttick-perf branch from d68f8ba to 2126cadCompare June 16, 2017 07:24

refack approved these changes Jun 16, 2017
View reviewed changes

process: improve nextTick() performance
460ee75
PR-URL: nodejs#13446 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

mscdex force-pushed the process-nexttick-perf branch from 2126cad to 460ee75Compare June 17, 2017 14:39

mscdex merged commit 460ee75 into nodejs:masterJun 17, 2017

mscdex deleted the process-nexttick-perf branch June 17, 2017 14:41

mscdex added the dont-land-on-v4.x label Jun 17, 2017

mscdex added dont-land-on-v6.x labels Jun 17, 2017

addaleax mentioned this pull request Jun 21, 2017
v8.2.0 proposal #13744
Merged

Uh oh!

process: improve nextTick() performance#13446

process: improve nextTick() performance #13446

Uh oh!

Conversation

mscdex commented Jun 4, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refackJun 4, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdexJun 4, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdexJun 5, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahnJun 5, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdexJun 5, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refackJun 5, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdexJun 5, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Jun 7, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Jun 8, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Jun 4, 2017•
edited
Loading

refackJun 4, 2017•
edited
Loading

mscdexJun 4, 2017•
edited
Loading

mscdexJun 5, 2017•
edited
Loading

gibfahnJun 5, 2017•
edited
Loading

mscdexJun 5, 2017•
edited
Loading

refackJun 5, 2017•
edited
Loading

mscdexJun 5, 2017•
edited
Loading

mscdex commented Jun 7, 2017•
edited
Loading

mscdex commented Jun 8, 2017•
edited
Loading

mscdexJun 9, 2017•
edited
Loading

mscdex commented Jun 16, 2017•
edited
Loading