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

process: improve queueMicrotask performance by apapirovski · Pull Request #28093 · nodejs/node · GitHub

/ node Public

process: improve queueMicrotask performance - #28093

Closed
apapirovski wants to merge 3 commits into
nodejs:masterfrom
apapirovski:faster-queue-microtask
ConversationCommits (3)ChecksFiles changed
Closed

process: improve queueMicrotask performance#28093
apapirovski wants to merge 3 commits into
nodejs:masterfrom
apapirovski:faster-queue-microtask

Conversation

apapirovski commented Jun 6, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Optimize the hot code paths of queueMicrotask by not creating unnecessary objects, not looking up properties on frozen primordials, etc.

Benchmark: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/389/

                                            confidence improvement accuracy (*)   (**)  (***)
process/queue-microtask-breadth.js n=400000        ***      3.91 %       ±1.28% ±1.69% ±2.17%
process/queue-microtask-depth.js n=1200000         ***     16.19 %       ±0.84% ±1.11% ±1.42%

That said, this is what I get locally so I figure the system makes a difference:

                                             confidence improvement accuracy (*)   (**)  (***)
 process/queue-microtask-breadth.js n=400000        ***     49.12 %       ±2.95% ±3.93% ±5.12%
 process/queue-microtask-depth.js n=1200000         ***     18.32 %       ±1.47% ±1.96% ±2.55%
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

process: improve queueMicrotask performance
25af820 
Optimize the hot code paths of queueMicrotask by
not creating unnecessary objects, not looking up
properties on frozen primordials, etc.

This comment has been minimized.

Sign in to view
targos reviewed Jun 6, 2019
View reviewed changes
Comment thread lib/async_hooks.js Outdated

nodejs-github-bot commented Jun 6, 2019

Copy link
Copy Markdown
Collaborator

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

targos reviewed Jun 6, 2019
View reviewed changes
Comment thread lib/internal/process/task_queues.js Outdated
asyncResource.callback = callback;

enqueueMicrotask(FunctionPrototype.bind(runMicrotask, asyncResource));
enqueueMicrotask(FunctionPrototypeBind(runMicrotask, asyncResource));

targos Jun 6, 2019

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

Same question here.
Are we actually doing something wrong with the primordials so that V8 doesn't know that the properties never change?

apapirovski Jun 6, 2019

Copy link
Copy Markdown
Contributor Author

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

Not sure. @mcollina flagged this for me and I've noticed it's definitely slower. He might have more insights — not sure if he talked to the V8 team about it at all.

joyeecheung Jun 6, 2019 •
edited
Loading

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

There is an ongoing effort to optimize frozen object performance, see https://bugs.chromium.org/p/v8/issues/detail?id=6831 and https://bugs.chromium.org/p/v8/issues/detail?id=8538 we may revisit this when the patches upstream land here.

mcollina Jun 8, 2019

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

Are we actually doing something wrong with the primordials so that V8 doesn't know that the properties never change?

We are. Essentially accessing them is slower than accessing normal objects. We should look on not reading properties in hot code path, but maybe only once when the module is loaded.

joyeecheung commented Jun 6, 2019

Copy link
Copy Markdown
Member

Maybe @bmeurer would be interested in taking a look at the use cases and perf impact here.

Fishrock123 reviewed Jun 7, 2019
View reviewed changes
Comment thread lib/async_hooks.js

// This prop name (destroyed) has to be synchronized with C++
const destroyed = { destroyed: false };
this[destroyedSymbol] = destroyed;

Fishrock123 Jun 7, 2019

Copy link
Copy Markdown
Contributor

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

Is moving this safe?

apapirovski Jun 8, 2019

Copy link
Copy Markdown
Contributor Author

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

Yeah. This is only needed for the case where requireManualDestroy = false. We pass it on directly to C++ in registerDestroyHook.

mcollina commented Jun 8, 2019

Copy link
Copy Markdown
Member

@apapirovski Can you please reintroduce the fix for the frozen primordials? I don't understand the reasoning behind removing it.

You might want to re-run the benchmarks before landing.

mcollina approved these changes Jun 8, 2019
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 Jun 8, 2019

Copy link
Copy Markdown
Contributor Author

@mcollina I removed it because I don't have strong feelings about it. Happy to re-introduce if @targos doesn't feel strongly about it.

I ran the benchmark after my changes and results were still very similar. The main speedup here was that we create two fewer objects per microtask.

mcollina commented Jun 8, 2019

Copy link
Copy Markdown
Member

Then ok. If it does not affect benchmark I’m good!

apapirovski commented Jun 10, 2019

Copy link
Copy Markdown
Contributor Author

Landed in cde3928

apapirovski deleted the faster-queue-microtask branch June 10, 2019 06:18
pull Bot pushed a commit to Pandinosaurus/node that referenced this pull request Jun 10, 2019
process: improve queueMicrotask performance
cde3928 
Optimize the hot code paths of queueMicrotask by
not creating unnecessary objects, not looking up
properties on frozen primordials, etc.

PR-URL: nodejs#28093
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
process: improve queueMicrotask performance
b491eab 
Optimize the hot code paths of queueMicrotask by
not creating unnecessary objects, not looking up
properties on frozen primordials, etc.

PR-URL: #28093
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BridgeAR mentioned this pull request Jun 17, 2019
Merged
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

joyeecheung joyeecheung left review comments
mcollina mcollina approved these changes
BridgeAR BridgeAR approved these changes
+3 more reviewers
Fishrock123 Fishrock123 left review comments
targos targos left review comments
devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL