lib: expose FixedQueue internally and fix nextTick bug #20468

apapirovski · 2018-05-02T13:47:28Z

A bug was introduced together with the FixedQueue implementation for process.nextTick which meant that the queue wouldn't necessarily fully clear on each run through. Fix it and abstract the data structure into an internal module that can later be re-used elsewhere.

I'm currently working on tests for the FixedQueue itself. Those should come in the next 24 hours.

The abstraction is in preparation for another PR that uses the FixedQueue in another module. (That said, the fact that we'll be able to properly test it outside of nextTick is also a nice side-effect.)

Checklist

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

apapirovski · 2018-05-02T13:47:58Z

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

mscdex · 2018-05-02T14:19:14Z

Any change in benchmark results after these changes?

apapirovski · 2018-05-02T15:30:47Z

@mscdex I don't have time to run them locally until tomorrow but quick runs indicated +-2% change. I'm waiting on my two earlier PRs that fix nextTick benchmarks to land and then will run it on the CI.

A bug was introduced together with the FixedQueue implementation for process.nextTick which meant that the queue wouldn't necessarily fully clear on each run through. Fix it and abstract the data structure into an internal module that can later be used elsewhere.

apapirovski · 2018-05-03T07:14:54Z

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

apapirovski · 2018-05-03T10:12:39Z

@nodejs/process @nodejs/performance I would appreciate some reviews on this. This fixes a bug in nextTick and with the recent changes also significantly improves its performance in most usage:

                                             confidence improvement accuracy (*)   (**)  (***)
 process/next-tick-breadth-args.js n=4000000        ***     13.19 %       ±1.91% ±2.52% ±3.23%
 process/next-tick-breadth.js n=4000000             ***     36.53 %       ±3.42% ±4.52% ±5.82%
 process/next-tick-depth-args.js n=12000000         ***      3.95 %       ±1.44% ±1.90% ±2.43%
 process/next-tick-depth.js n=12000000                       0.44 %       ±1.71% ±2.25% ±2.89%
 process/next-tick-exec-args.js n=5000000           ***     15.48 %       ±2.26% ±2.98% ±3.83%
 process/next-tick-exec.js n=5000000                ***     22.22 %       ±2.49% ±3.28% ±4.22%

                                                       confidence improvement accuracy (*)   (**)  (***)
 streams/creation.js kind='duplex' n=50000000                          0.47 %       ±1.12% ±1.48% ±1.90%
 streams/creation.js kind='readable' n=50000000                        1.17 %       ±1.51% ±2.00% ±2.57%
 streams/creation.js kind='transform' n=50000000                      -0.33 %       ±1.05% ±1.39% ±1.79%
 streams/creation.js kind='writable' n=50000000                        0.49 %       ±1.49% ±1.97% ±2.53%
 streams/pipe.js n=5000000                                     **      4.26 %       ±2.68% ±3.53% ±4.53%
 streams/pipe-object-mode.js n=5000000                                 1.56 %       ±2.35% ±3.10% ±3.99%
 streams/readable-bigread.js n=1000                                    0.14 %       ±1.65% ±2.17% ±2.79%
 streams/readable-bigunevenread.js n=1000                              0.54 %       ±1.34% ±1.77% ±2.27%
 streams/readable-boundaryread.js type='buffer' n=2000        ***     -2.50 %       ±0.87% ±1.15% ±1.47%
 streams/readable-boundaryread.js type='string' n=2000                -0.24 %       ±0.89% ±1.18% ±1.51%
 streams/readable-readall.js n=5000                                    0.28 %       ±1.31% ±1.73% ±2.22%
 streams/readable-unevenread.js n=1000                                -0.58 %       ±0.68% ±0.89% ±1.15%
 streams/writable-manywrites.js n=2000000                     ***      6.63 %       ±1.68% ±2.21% ±2.84%

benjamingr · 2018-05-03T21:33:37Z

lib/internal/process/next_tick.js

@@ -222,6 +112,8 @@ function setupNextTick() {
          args[i - 1] = arguments[i];
    }

-    push(new TickObject(callback, args, getDefaultTriggerAsyncId()));
+    if (queue.isEmpty())
+      tickInfo[kHasScheduled] = 1;


benjamingr

+1 on the fix and also extracting FixedQueue though I'm not sure why it's in the same PR. Changes LGTM.

apapirovski · 2018-05-04T04:18:51Z

+1 on the fix and also extracting FixedQueue though I'm not sure why it's in the same PR. Changes LGTM.

I started by extracting it then found the bug. Since this adjusts the FixedQueue API a decent amount, it didn't feel like it was necessary to do them in two separate PRs. It also allows us to provide more robust tests for FixedQueue itself, not just for nextTick.

@addaleax I know you wrote the original documentation for this. Were the changes I made to account for the new behaviour clear enough?

mcollina

LGTM

apapirovski · 2018-05-06T05:22:26Z

Landed in 9a3ae2f

A bug was introduced together with the FixedQueue implementation for process.nextTick which meant that the queue wouldn't necessarily fully clear on each run through. Fix it and abstract the data structure into an internal module that can later be used elsewhere. PR-URL: #20468 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

apapirovski added process Issues and PRs related to the process subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. labels May 2, 2018

nodejs-github-bot added build Issues and PRs related to build files or the CI. process Issues and PRs related to the process subsystem. labels May 2, 2018

apapirovski removed the build Issues and PRs related to build files or the CI. label May 2, 2018

This was referenced May 2, 2018

benchmark: track exec time in next-tick-exec #20462

Closed

benchmark: fix next-tick-depth #20461

Closed

apapirovski force-pushed the patch-fixed-queue branch from 84a02b4 to 69dd197 Compare May 3, 2018 05:16

fixup: improve perf slightly and add tests

a6a26a1

apapirovski added the performance Issues and PRs related to the performance of Node.js. label May 3, 2018

benjamingr reviewed May 3, 2018

View reviewed changes

benjamingr approved these changes May 3, 2018

View reviewed changes

jasnell approved these changes May 3, 2018

View reviewed changes

addaleax approved these changes May 3, 2018

View reviewed changes

apapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 4, 2018

BridgeAR approved these changes May 5, 2018

View reviewed changes

mcollina approved these changes May 5, 2018

View reviewed changes

apapirovski closed this May 6, 2018

apapirovski deleted the patch-fixed-queue branch May 6, 2018 05:22

MylesBorins mentioned this pull request May 8, 2018

v10.1.0 proposal #20606

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: expose FixedQueue internally and fix nextTick bug #20468

lib: expose FixedQueue internally and fix nextTick bug #20468

apapirovski commented May 2, 2018

apapirovski commented May 2, 2018

mscdex commented May 2, 2018

apapirovski commented May 2, 2018

apapirovski commented May 3, 2018 •

edited

Loading

apapirovski commented May 3, 2018 •

edited

Loading

benjamingr May 3, 2018

benjamingr left a comment

apapirovski commented May 4, 2018

mcollina left a comment

apapirovski commented May 6, 2018

lib: expose FixedQueue internally and fix nextTick bug #20468

lib: expose FixedQueue internally and fix nextTick bug #20468

Conversation

apapirovski commented May 2, 2018

Checklist

apapirovski commented May 2, 2018

mscdex commented May 2, 2018

apapirovski commented May 2, 2018

apapirovski commented May 3, 2018 • edited Loading

apapirovski commented May 3, 2018 • edited Loading

benjamingr May 3, 2018

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

apapirovski commented May 4, 2018

mcollina left a comment

Choose a reason for hiding this comment

apapirovski commented May 6, 2018

apapirovski commented May 3, 2018 •

edited

Loading

apapirovski commented May 3, 2018 •

edited

Loading