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.

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

src: improve MakeCallback() performance #41331

Closed
wants to merge 6 commits into from

Conversation

addaleax
Copy link
Member

Opening this as a better alternative to #41279. I could have sworn that there was a MakeCallback() benchmark, but I am unable to find it. Local manual benchmarking gives a 16% boost for MakeCallback() itself:

$ ./node-prev -e 'const { makeCallback } = require("./test/addons/make-callback/build/Release/binding"); console.time("bm"); for (let i = 0; i < 100_000_000; i++) makeCallback(this, () => {}); console.timeEnd("bm")'
bm: 23.567s
$ ./node -e 'const { makeCallback } = require("./test/addons/make-callback/build/Release/binding"); console.time("bm"); for (let i = 0; i < 100_000_000; i++) makeCallback(this, () => {}); console.timeEnd("bm")'
bm: 19.795s

MessagePort benchmark:

$ node benchmark/compare.js --old ./node-prev --new ./node --filter messageport worker | Rscript benchmark/compare.R
[00:07:49|% 100| 1/1 files | 60/60 runs | 4/4 configs]: Done
                                                                       confidence improvement accuracy (*)   (**)  (***)
worker/messageport.js n=1000000 style='eventemitter' payload='object'        ***      3.68 %       ±1.34% ±1.79% ±2.32%
worker/messageport.js n=1000000 style='eventemitter' payload='string'        ***      4.46 %       ±1.06% ±1.41% ±1.83%
worker/messageport.js n=1000000 style='eventtarget' payload='object'         ***      2.62 %       ±1.31% ±1.74% ±2.27%
worker/messageport.js n=1000000 style='eventtarget' payload='string'         ***      2.31 %       ±1.04% ±1.38% ±1.81%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 4 comparisons, you can thus
expect the following amount of false-positive results:
  0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.04 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

src: guard slightly costly check in MakeCallback more strongly
src: store native async execution resources as v8::Local

This is possible because the async stack is always expected
to match the native call stack, and saves performance overhead
that comes from the usage of v8::Global.

src: split out async stack corruption detection from inline fn

This is fairly expensive code that unnecessarily bloats the
contents of the inline function.

This is possible because the async stack is always expected
to match the native call stack, and saves performance overhead
that comes from the usage of `v8::Global`.
This is fairly expensive code that unnecessarily bloats the
contents of the inline function.
@addaleax addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. performance Issues and PRs related to the performance of Node.js. addons Issues and PRs related to native addons. async_hooks Issues and PRs related to the async hooks subsystem. labels Dec 26, 2021
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Dec 26, 2021
for (size_t i = 0; i < info_->native_execution_async_resources.size(); ++i) {
Local<Object> obj = context->GetDataFromSnapshotOnce<Object>(
info_->native_execution_async_resources[i])
.ToLocalChecked();
native_execution_async_resources_[i].Reset(context->GetIsolate(), obj);
js_execution_async_resources->Set(context, i, obj).Check();
Copy link
Member Author

Choose a reason for hiding this comment

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

@joyeecheung This code is effectively doing the same thing as before, but I did want to ask – what exactly are the semantics of the async stack after a snapshot? Is it expected that you start out in a state where there is an active async context, even though the underlying async operation doesn’t even exist in the deserialized instance? (Or, another way: Should the AsyncHooks state be serialized/deserialized at all?)

@addaleax addaleax added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Dec 26, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 26, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

src/env-inl.h Show resolved Hide resolved
src/api/callback.cc Outdated Show resolved Hide resolved
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 26, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 26, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

src/env.h Show resolved Hide resolved
src/env-inl.h Show resolved Hide resolved
src/env.cc Outdated Show resolved Hide resolved
@tniessen
Copy link
Member

Local manual benchmarking

Would it make sense to add the benchmark (or a similar one) to the repo?

@addaleax addaleax added commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 28, 2021
@addaleax
Copy link
Member Author

Local manual benchmarking

Would it make sense to add the benchmark (or a similar one) to the repo?

Absolutely – again, I could have sworn there already was one 🙂 So, yes, it does make sense, no, I’m not particularly eager to do something about it myself. 🙂

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 28, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 1, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 5f07d00...a4795ad

nodejs-github-bot pushed a commit that referenced this pull request Jan 1, 2022
PR-URL: #41331
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Jan 1, 2022
This is possible because the async stack is always expected
to match the native call stack, and saves performance overhead
that comes from the usage of `v8::Global`.

PR-URL: #41331
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Jan 1, 2022
This is fairly expensive code that unnecessarily bloats the
contents of the inline function.

PR-URL: #41331
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@addaleax addaleax deleted the 41279-response branch January 9, 2022 19:26
targos pushed a commit that referenced this pull request Jan 14, 2022
PR-URL: #41331
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2022
This is possible because the async stack is always expected
to match the native call stack, and saves performance overhead
that comes from the usage of `v8::Global`.

PR-URL: #41331
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2022
This is fairly expensive code that unnecessarily bloats the
contents of the inline function.

PR-URL: #41331
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
PR-URL: #41331
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
This is possible because the async stack is always expected
to match the native call stack, and saves performance overhead
that comes from the usage of `v8::Global`.

PR-URL: #41331
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
This is fairly expensive code that unnecessarily bloats the
contents of the inline function.

PR-URL: #41331
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
PR-URL: nodejs#41331
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
This is possible because the async stack is always expected
to match the native call stack, and saves performance overhead
that comes from the usage of `v8::Global`.

PR-URL: nodejs#41331
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
This is fairly expensive code that unnecessarily bloats the
contents of the inline function.

PR-URL: nodejs#41331
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #41331
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
This is possible because the async stack is always expected
to match the native call stack, and saves performance overhead
that comes from the usage of `v8::Global`.

PR-URL: #41331
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
This is fairly expensive code that unnecessarily bloats the
contents of the inline function.

PR-URL: #41331
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
@e3dio
Copy link

e3dio commented Feb 1, 2022

Following up here with addon benchmark as result of this PR, seeing improvement with latest Node 17.4

Slide-1-copy-1 (1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. async_hooks Issues and PRs related to the async hooks subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants