Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

process: improve queueMicrotask performance #28093

Closed
wants to merge 3 commits into from
Closed

process: improve queueMicrotask performance #28093

wants to merge 3 commits into from

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Jun 6, 2019 •
edited
Loading

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

@apapirovski
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.
@nodejs-github-bot

This comment has been minimized.

Sign in to view
targos
targos reviewed Jun 6, 2019
View reviewed changes
lib/async_hooks.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

targos
targos reviewed Jun 6, 2019
View reviewed changes
lib/internal/process/task_queues.js Outdated
@@ -172,7 +171,7 @@ function queueMicrotask(callback) {
const asyncResource = createMicrotaskResource();
asyncResource.callback = callback;

enqueueMicrotask(FunctionPrototype.bind(runMicrotask, asyncResource));
enqueueMicrotask(FunctionPrototypeBind(runMicrotask, asyncResource));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@joyeecheung joyeecheung Jun 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

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

Fishrock123
Fishrock123 reviewed Jun 7, 2019
View reviewed changes
lib/async_hooks.js
@@ -151,15 +152,14 @@ class AsyncResource {
this[async_id_symbol] = asyncId;
this[trigger_async_id_symbol] = triggerAsyncId;

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

Choose a reason for hiding this comment

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

Is moving this safe?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@mcollina
Copy link
Member

mcollina commented Jun 8, 2019

@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
mcollina approved these changes Jun 8, 2019
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@apapirovski
Copy link
Member 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
Copy link
Member

mcollina commented Jun 8, 2019

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

@apapirovski
Copy link
Member Author

Landed in cde3928

@apapirovski 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
@apapirovski
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
@apapirovski @BridgeAR
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 BridgeAR mentioned this pull request Jun 17, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fishrock123 Fishrock123 Fishrock123 left review comments

@targos targos targos left review comments

@joyeecheung joyeecheung joyeecheung left review comments

@mcollina mcollina mcollina approved these changes

@devsnek devsnek devsnek approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

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
@apapirovski @nodejs-github-bot @joyeecheung @mcollina @Fishrock123 @targos @devsnek @BridgeAR