process: refactor nextTick for clarity #17738

apapirovski · 2017-12-18T14:43:01Z

A few changes to nextTick to further simplify the code as much as possible:

Do not share unnecessary information about nextTick state between JS & C++, instead only track whether a nextTick is scheduled or not. The current implementation was left over from when this information was actually necessary to share between JS & C++ but that is no longer necessary.
Turn nextTickQueue into an Object instead of a class since multiple instances are never created. This was something that tripped me up the first time I started reading this code. I'll readily admit it's not a major change but it makes it obvious that there's only ever one queue.
Other assorted refinements and refactoring.

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

Checklist

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

Affected core subsystem(s)

process, src

addaleax · 2017-12-18T15:50:41Z

lib/internal/process/next_tick.js

@@ -179,6 +140,8 @@ function setupNextTick() {
                 triggerAsyncId,
                 this);
      }
+
+      nextTickQueue.push(this);


Is there a reason to move this into the constructor? I find moving side-effects into constructors to be surprising about 99 % of the time…

Probably not. Was trying to consolidate between the two nextTick functions but you're right, this likely doesn't belong here.

Fishrock123 · 2017-12-18T17:05:06Z

lib/internal/process/next_tick.js

-// large and cause the process to run out of memory. When this value
-// is reached the nextTimeQueue array will be shortened (see tickDone
-// for details).
-const kMaxCallbacksPerLoop = 1e4;


Doesn't this expose infinite nextTick loop problems? and / or, isn't this a breaking change?

This became irrelevant when @mscdex turned this into a linked list instead of an Array. I don't really see that it does anything right now other than make sure the indexes don't overflow and since I got rid of length & index, it's no longer necessary.

Also, FWIW, our benchmarks are an indirect test for this and they still pass. (If one were to remove this without removing the index & length values then the integers overflow.)

I thought this was to prevent infinite nextTick recursion... I'm not sure how a linkedlist helps that? Maybe I didn't look deep enough? Not sure.

No, it doesn't prevent infinite recursion. Nothing does, our documentation even calls it out as an issue.

When an Array used to be used for nextTick, we wouldn't clear it until 1e4 runs so it could grow to near Infinite size without this condition to remove stale tick objects.

Edit: This was confusingly worded. What I mean is: before this 1e4 limit was introduced, we never cleared the Array until the whole outer loop completed but this meant that _tickCallback could run long enough until the nextTickQueue array was so big that it hogged all available memory. The 1e4 limit was then introduced to occasionally empty the nextTickQueue so the process wouldn't run out of memory.

With a linked list this is no longer necessary as on each tock it adjusts the linked list and the GC can do its job whenever it wants/needs to. The only reason that code then remained was for the cases where an integer overflow could occur due to reaching too large of an index & length.

Note: The next tick queue is completely drained on each pass of the event loop before additional I/O is processed. As a result, recursively setting nextTick callbacks will block any I/O from happening, just like a while(true); loop.

https://nodejs.org/dist/latest-v9.x/docs/api/process.html#process_process_nexttick_callback_args

Just try const set = () => process.nextTick(set); set(); in repl and be prepared to force quit it. 😆

@Fishrock123 here's the commit that introduced this code apapirovski@5757642

@apapirovski That was a very helpful explanation… on some level it’s sad to understand code only once it’s going away, but I always wondered about this. :)

apapirovski · 2017-12-18T17:18:02Z

FWIW, here's the benchmark CI results:

 process/next-tick-breadth-args.js millions=4      0.88 %            5.225539e-01
 process/next-tick-breadth.js millions=4          -0.49 %            4.959895e-01
 process/next-tick-depth-args.js millions=12       5.25 %        *** 4.221255e-18
 process/next-tick-depth.js millions=12            5.55 %        *** 1.263990e-10
 process/next-tick-exec-args.js millions=5         0.97 %            4.162916e-01
 process/next-tick-exec.js millions=5              1.76 %        *** 2.312287e-04

apapirovski · 2017-12-19T18:58:00Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12207/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1160/

AndreasMadsen · 2017-12-20T08:50:32Z

lib/internal/process/next_tick.js

-  var kLength = 1;
+  const kScheduled = 0;
+
+  const nextTickQueue = {


I'm curious, why make this an object literal instead of the class that existed before? I would think V8 still optimizes classes (using hidden classes) better than object literals.

AFAIK, It does the same thing with both in terms of hidden classes. (That's part of the reason Object.create(null) is used in a lot of our code because it prevents creating a hidden class and just skips straight to dictionary object mode.)

The reasoning for the change is mostly that it's straight-up confusing the first time one looks at this code, NextTickQueue is declared outside of setup and it appears that multiple queues might be supported until one starts reading into the source.

Also, the tickInfo[kScheduled] flag is flipped inside push and shift now, so declaring it outside of setup is impossible.

I see. Properly we could merge the code for the timers linked list and the nextTick linked list, but this is fine for now.

PS: new class NextTickQueue {} is a valid syntax, although I find it equally odd.

Ideally, we wouldn't. nextTick has a special version that's optimized for its own use because it only needs a singly linked list, whereas timers definitely need doubly linked.

We have very different concepts of what is ideal :)

Using doubly linked list, despite it being completely unnecessary for nextTick, would come with a significant performance regression and memory consumption increase...

Do not share unnecessary information about nextTick state between JS & C++, instead only track whether a nextTick is scheduled or not. Turn nextTickQueue into an Object instead of a class since multiple instances are never created. Other assorted refinements and refactoring.

apapirovski · 2017-12-24T13:10:11Z

Post-rebase CI: https://ci.nodejs.org/job/node-test-pull-request/12285/

apapirovski · 2017-12-24T14:14:12Z

Landed in 4444b6b

Do not share unnecessary information about nextTick state between JS & C++, instead only track whether a nextTick is scheduled or not. Turn nextTickQueue into an Object instead of a class since multiple instances are never created. Other assorted refinements and refactoring. PR-URL: #17738 Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins · 2018-01-09T23:37:49Z

This does not land cleanly on v9.x, could you please backport

apapirovski · 2018-01-10T17:26:06Z

This will land cleanly once #18079 lands.

Do not share unnecessary information about nextTick state between JS & C++, instead only track whether a nextTick is scheduled or not. Turn nextTickQueue into an Object instead of a class since multiple instances are never created. Other assorted refinements and refactoring. PR-URL: nodejs#17738 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Do not share unnecessary information about nextTick state between JS & C++, instead only track whether a nextTick is scheduled or not. Turn nextTickQueue into an Object instead of a class since multiple instances are never created. Other assorted refinements and refactoring. Backport-PR-URL: #19006 PR-URL: #17738 Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins · 2018-07-31T18:32:07Z

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

apapirovski added lib / src Issues and PRs related to general changes in the lib or src directory. process Issues and PRs related to the process subsystem. labels Dec 18, 2017

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. labels Dec 18, 2017

apapirovski force-pushed the patch-next-tick-refactor branch 2 times, most recently from 54e624e to 5ababbd Compare December 18, 2017 14:52

addaleax approved these changes Dec 18, 2017

View reviewed changes

Fishrock123 reviewed Dec 18, 2017

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 19, 2017

apapirovski force-pushed the patch-next-tick-refactor branch 3 times, most recently from 079dfed to 55b2a46 Compare December 19, 2017 18:57

AndreasMadsen reviewed Dec 20, 2017

View reviewed changes

apapirovski added 2 commits December 24, 2017 08:06

fixup: remove side-effects from constructor

1d84f1d

apapirovski force-pushed the patch-next-tick-refactor branch from 55b2a46 to 1d84f1d Compare December 24, 2017 13:07

apapirovski closed this Dec 24, 2017

apapirovski deleted the patch-next-tick-refactor branch December 24, 2017 14:14

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017

MylesBorins added the backport-requested-v9.x label Jan 9, 2018

addaleax added backported-to-v9.x and removed backport-requested-v9.x labels Feb 26, 2018

addaleax mentioned this pull request Feb 27, 2018

v9.7.0 proposal #19040

Closed

MylesBorins added the backport-requested-v8.x label Jul 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

process: refactor nextTick for clarity #17738

process: refactor nextTick for clarity #17738

apapirovski commented Dec 18, 2017 •

edited

Loading

addaleax Dec 18, 2017

apapirovski Dec 18, 2017

Fishrock123 Dec 18, 2017

apapirovski Dec 18, 2017

apapirovski Dec 18, 2017

Fishrock123 Dec 18, 2017

apapirovski Dec 18, 2017 •

edited

Loading

apapirovski Dec 18, 2017 •

edited

Loading

apapirovski Dec 18, 2017 •

edited

Loading

addaleax Dec 18, 2017

apapirovski commented Dec 18, 2017

apapirovski commented Dec 19, 2017 •

edited

Loading

AndreasMadsen Dec 20, 2017

apapirovski Dec 20, 2017

apapirovski Dec 20, 2017

AndreasMadsen Dec 20, 2017 •

edited

Loading

apapirovski Dec 20, 2017

AndreasMadsen Dec 20, 2017

apapirovski Dec 20, 2017

apapirovski commented Dec 24, 2017

apapirovski commented Dec 24, 2017

MylesBorins commented Jan 9, 2018

apapirovski commented Jan 10, 2018 •

edited

Loading

MylesBorins commented Jul 31, 2018

process: refactor nextTick for clarity #17738

process: refactor nextTick for clarity #17738

Conversation

apapirovski commented Dec 18, 2017 • edited Loading

Checklist

Affected core subsystem(s)

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 Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

apapirovski Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

apapirovski Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Dec 18, 2017

apapirovski commented Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen Dec 20, 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

apapirovski commented Dec 24, 2017

apapirovski commented Dec 24, 2017

MylesBorins commented Jan 9, 2018

apapirovski commented Jan 10, 2018 • edited Loading

MylesBorins commented Jul 31, 2018

apapirovski commented Dec 18, 2017 •

edited

Loading

apapirovski Dec 18, 2017 •

edited

Loading

apapirovski Dec 18, 2017 •

edited

Loading

apapirovski Dec 18, 2017 •

edited

Loading

apapirovski commented Dec 19, 2017 •

edited

Loading

AndreasMadsen Dec 20, 2017 •

edited

Loading

apapirovski commented Jan 10, 2018 •

edited

Loading