src: improve MakeCallback() performance

[addaleax]

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.

[addaleax]

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

[tniessen]

Local manual benchmarking

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

[addaleax]

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]

Landed in 5f07d00...a4795ad

[e3dio]

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