GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — src: improve MakeCallback() performance by addaleax · Pull Request #41331 · nodejs/node

Conversation

addaleax commented Dec 26, 2021

Copy link

Copy Markdown

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

addaleax added 3 commits

December 26, 2021 20:28


          src: guard slightly costly check in MakeCallback more strongly

bb0bd36


          src: store native async execution resources as v8::Local

f97d04e

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

853853f

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

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 added the needs-ci PRs that need a full CI run. label

Dec 26, 2021

addaleax mentioned this pull request

Dec 26, 2021

src: skip costly pushing/popping if we pass zeroes as async context #41279

Closed

addaleax commented

Dec 26, 2021

View reviewed changes

Comment thread

src/env.cc

		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();

addaleax Dec 26, 2021

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

addaleax added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed needs-ci PRs that need a full CI run. labels

Dec 26, 2021

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Dec 26, 2021

This comment has been minimized.

Flarna approved these changes

Dec 26, 2021

View reviewed changes

Comment thread

src/env-inl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/api/callback.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

addaleax added 2 commits

December 26, 2021 23:34


          fixup! src: guard slightly costly check in MakeCallback more strongly

b98e4bd


          fixup! src: store native async execution resources as v8::Local

069373e

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Dec 26, 2021

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Dec 26, 2021

This comment has been minimized.

RaisinTen reviewed

Dec 28, 2021

View reviewed changes

Comment thread

src/env.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/env-inl.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/env.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

tniessen approved these changes

Dec 28, 2021

View reviewed changes

tniessen commented Dec 28, 2021

Copy link

Copy Markdown

Member

Local manual benchmarking

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          fixup! src: store native async execution resources as v8::Local

a7787bb

addaleax added commit-queue-rebase

PRs the Commit Queue should land as multiple self-contained commits.

request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels

Dec 28, 2021

addaleax commented Dec 28, 2021

Copy link

Copy Markdown

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

tniessen reacted with thumbs up emoji

tniessen reacted with laugh emoji

All reactions

👍 1 reaction
😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Dec 28, 2021

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label

Jan 1, 2022

nodejs-github-bot commented Jan 1, 2022

Copy link

Copy Markdown

Collaborator

Landed in 5f07d00...a4795ad

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot closed this

Jan 1, 2022

nodejs-github-bot pushed a commit that referenced this pull request

Jan 1, 2022


          src: guard slightly costly check in MakeCallback more strongly

6932fb8

PR-URL: #41331
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

nodejs-github-bot pushed a commit that referenced this pull request

Jan 1, 2022


          src: store native async execution resources as v8::Local

69ea677

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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

nodejs-github-bot pushed a commit that referenced this pull request

Jan 1, 2022


          src: split out async stack corruption detection from inline fn

a4795ad

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

PR-URL: #41331
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

This was referenced Jan 2, 2022

CI Reliability 2022-01-02 nodejs/reliability#163

Open

CI Reliability 2022-01-03 nodejs/reliability#164

Open

CI Reliability 2022-01-04 nodejs/reliability#165

Open

CI Reliability 2022-01-05 nodejs/reliability#166

Open

CI Reliability 2022-01-06 nodejs/reliability#167

Open

CI Reliability 2022-01-07 nodejs/reliability#168

Open

CI Reliability 2022-01-08 nodejs/reliability#169

Open

addaleax deleted the 41279-response branch

January 9, 2022 19:26

targos pushed a commit that referenced this pull request

Jan 14, 2022


          src: guard slightly costly check in MakeCallback more strongly

6187e81

PR-URL: #41331
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

targos pushed a commit that referenced this pull request

Jan 14, 2022


          src: store native async execution resources as v8::Local

250e197

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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

targos pushed a commit that referenced this pull request

Jan 14, 2022


          src: split out async stack corruption detection from inline fn

337ebfc

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

PR-URL: #41331
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

targos mentioned this pull request

Jan 16, 2022

v17.4.0 release proposal #41557

Merged

danielleadams pushed a commit that referenced this pull request

Jan 31, 2022


          src: guard slightly costly check in MakeCallback more strongly

f809625

PR-URL: #41331
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

danielleadams pushed a commit that referenced this pull request

Jan 31, 2022


          src: store native async execution resources as v8::Local

40db8e5

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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

danielleadams pushed a commit that referenced this pull request

Jan 31, 2022


          src: split out async stack corruption detection from inline fn

cd40686

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

PR-URL: #41331
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

danielleadams pushed a commit that referenced this pull request

Feb 1, 2022


          src: guard slightly costly check in MakeCallback more strongly

59625f7

PR-URL: #41331
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

danielleadams pushed a commit that referenced this pull request

Feb 1, 2022


          src: store native async execution resources as v8::Local

72921f4

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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

danielleadams pushed a commit that referenced this pull request

Feb 1, 2022


          src: split out async stack corruption detection from inline fn

fe4158a

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

PR-URL: #41331
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>

danielleadams mentioned this pull request

Feb 1, 2022

v16.14.0 proposal #41804

Merged

e3dio commented Feb 1, 2022

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

abenhamdine mentioned this pull request

Dec 1, 2022

MakeCallback is very slow nodejs/performance#24

Open

Reviewers

tniessen

tniessen approved these changes

Flarna

Flarna approved these changes

RaisinTen

RaisinTen approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

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

PRs the Commit Queue should land as multiple self-contained commits.

performance

Issues and PRs related to the performance of Node.js.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.