FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: expose FixedQueue internally and fix nextTick bug by apapirovski · Pull Request #20468 · nodejs/node · GitHub

/ node Public

lib: expose FixedQueue internally and fix nextTick bug - #20468

Closed
apapirovski wants to merge 2 commits into
nodejs:masterfrom
apapirovski:patch-fixed-queue
ConversationCommits (2)ChecksFiles changed
Closed

lib: expose FixedQueue internally and fix nextTick bug#20468
apapirovski wants to merge 2 commits into
nodejs:masterfrom
apapirovski:patch-fixed-queue

Conversation

apapirovski commented May 2, 2018

Copy link
Copy Markdown
Contributor

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 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 commented May 2, 2018

Copy link
Copy Markdown
Contributor Author

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

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

mscdex commented May 2, 2018

Copy link
Copy Markdown
Contributor

Any change in benchmark results after these changes?

apapirovski commented May 2, 2018

Copy link
Copy Markdown
Contributor Author

@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.

This was referenced May 2, 2018
Closed
Closed
lib: expose FixedQueue internally and fix nextTick bug
69dd197 
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 force-pushed the patch-fixed-queue branch from 84a02b4 to 69dd197 Compare May 3, 2018 05:16

apapirovski commented May 3, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 commented May 3, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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%

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
Comment thread lib/internal/process/next_tick.js

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

benjamingr May 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nice find!

benjamingr approved these changes May 3, 2018
View reviewed changes

benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

apapirovski commented May 4, 2018

Copy link
Copy Markdown
Contributor Author

+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?

apapirovski added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 4, 2018
mcollina approved these changes May 5, 2018
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

apapirovski commented May 6, 2018

Copy link
Copy Markdown
Contributor Author

Landed in 9a3ae2f

apapirovski closed this May 6, 2018
apapirovski deleted the patch-fixed-queue branch May 6, 2018 05:22
apapirovski added a commit that referenced this pull request May 6, 2018
lib: expose FixedQueue internally and fix nextTick bug
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>
MylesBorins pushed a commit that referenced this pull request May 8, 2018
lib: expose FixedQueue internally and fix nextTick bug
f508574 
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>
MylesBorins mentioned this pull request May 8, 2018
Merged
MylesBorins pushed a commit that referenced this pull request May 8, 2018
lib: expose FixedQueue internally and fix nextTick bug
d0cbb4c 
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>
MylesBorins pushed a commit that referenced this pull request May 9, 2018
lib: expose FixedQueue internally and fix nextTick bug
7b8e9ca 
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
jasnell jasnell approved these changes
addaleax addaleax approved these changes
benjamingr benjamingr approved these changes
BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. performance Issues and PRs related to the performance of Node.js. process Issues and PRs related to the process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL