src: improve MakeCallback() performance #41331

addaleax · 2021-12-26T19:57:56Z

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 · 2021-12-26T20:02:52Z

src/env.cc

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


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

src/env-inl.h

src/api/callback.cc

src/env.h

src/env-inl.h

src/env.cc

tniessen · 2021-12-28T17:12:54Z

Local manual benchmarking

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

addaleax · 2021-12-28T17:51:40Z

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

nodejs-github-bot · 2022-01-01T18:36:53Z

Landed in 5f07d00...a4795ad

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

e3dio · 2022-02-01T15:37:11Z

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

addaleax added 3 commits Dec 26, 2021

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++ performance addons async_hooks labels Dec 26, 2021

nodejs-github-bot added the needs-ci 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 reviewed Dec 26, 2021

View changes

addaleax added request-ci and removed needs-ci labels Dec 26, 2021

github-actions bot removed the request-ci label Dec 26, 2021

This comment has been hidden.

Sign in to view

Flarna approved these changes Dec 26, 2021

View changes

src/env-inl.h Show resolved Hide resolved

src/api/callback.cc Outdated Show resolved Hide resolved

addaleax added 2 commits Dec 26, 2021

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 label Dec 26, 2021

github-actions bot removed the request-ci label Dec 26, 2021

This comment has been hidden.

Sign in to view

RaisinTen reviewed Dec 28, 2021

View changes

src/env.h Show resolved Hide resolved

src/env-inl.h Show resolved Hide resolved

src/env.cc Outdated Show resolved Hide resolved

tniessen approved these changes Dec 28, 2021

View changes

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

a7787bb

addaleax added commit-queue-rebase request-ci labels Dec 28, 2021

github-actions bot removed the request-ci label Dec 28, 2021

addaleax added the commit-queue label Jan 1, 2022

nodejs-github-bot removed the commit-queue label Jan 1, 2022

nodejs-github-bot closed this Jan 1, 2022

github-actions bot mentioned this pull request Jan 2, 2022

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

Open

23 tasks

github-actions bot mentioned this pull request Jan 3, 2022

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

Open

23 tasks

github-actions bot mentioned this pull request Jan 4, 2022

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

Open

25 tasks

github-actions bot mentioned this pull request Jan 5, 2022

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

Open

26 tasks

github-actions bot mentioned this pull request Jan 6, 2022

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

Open

19 tasks

github-actions bot mentioned this pull request Jan 7, 2022

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

Open

20 tasks

github-actions bot mentioned this pull request Jan 8, 2022

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

Open

20 tasks

addaleax deleted the 41279-response branch Jan 9, 2022

targos mentioned this pull request Jan 16, 2022

v17.4.0 release proposal #41557

Merged

danielleadams mentioned this pull request Feb 1, 2022

v16.14.0 proposal #41804

Merged

nodejs / node Public

src: improve MakeCallback() performance #41331

src: improve MakeCallback() performance #41331

addaleax commented Dec 26, 2021

addaleax Dec 26, 2021

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

tniessen commented Dec 28, 2021

addaleax commented Dec 28, 2021

nodejs-github-bot commented Jan 1, 2022

e3dio commented Feb 1, 2022

nodejs / node Public

src: improve MakeCallback() performance #41331

src: improve MakeCallback() performance #41331

Conversation

addaleax commented Dec 26, 2021

src: guard slightly costly check in MakeCallback more strongly

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

src: split out async stack corruption detection from inline fn

addaleax Dec 26, 2021

Choose a reason for hiding this comment

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

tniessen commented Dec 28, 2021

addaleax commented Dec 28, 2021

nodejs-github-bot commented Jan 1, 2022

e3dio commented Feb 1, 2022

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