async_hooks: improve resource stack performance

[addaleax]

Removes some of the performance overhead that came with
executionAsyncResource() by storing async resources that
are managed by JS and those managed by C++ separately, and
instead caching the result of executionAsyncResource() with
low overhead to avoid multiple calls into C++.

This particularly improves performance when async hooks are not
being used.

$ ./node benchmark/compare.js --new ./node --old ./node-master --runs 30 --filter messageport worker | Rscript benchmark/compare.R
[00:04:41|% 100| 1/1 files | 60/60 runs | 2/2 configs]: Done
                                                   confidence improvement accuracy (*)    (**)   (***)
 worker/messageport.js n=1000000 payload='object'          *      8.85 %       ±7.40%  ±9.85% ±12.83%
 worker/messageport.js n=1000000 payload='string'        ***     18.56 %       ±8.37% ±11.13% ±14.49%

(I’ll also try to run the async_hooks benchmarks, but they are failing locally for me on master – hence only the ones for MessagePorts as another measure.)

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[addaleax]

28e6626 (#33131) is what breaks the benchmarks for me. /cc @ronag

It looks like the async-resource-vs-destroy benchmark has a slow-down when using executionAsyncResource():

$ ./node benchmark/compare.js --new ./node --old ./node-master --runs 10 --filter async-resource-vs-destroy async_hooks | Rscript benchmark/compare.R
[00:11:09|% 100| 1/1 files | 20/20 runs | 6/6 configs]: Done
                                                                                                                                                                     confidence improvement accuracy (*)
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='async-local-storage' benchmarker='autocannon'            ***    -15.17 %       ±4.56%
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='async-resource' benchmarker='autocannon'                 ***    -11.15 %       ±4.71%
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='destroy' benchmarker='autocannon'                                -1.74 %       ±3.87%
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='async-local-storage' benchmarker='autocannon'        ***    -15.19 %       ±4.46%
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='async-resource' benchmarker='autocannon'             ***     -9.60 %       ±4.65%
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='destroy' benchmarker='autocannon'                            -1.01 %       ±7.32%

So I guess this PR makes a tradeoff, but I think I would generally accept a +18 % speedup without async_hooks at the cost of a -15 % perf drop when they are enabled? /cc @Qard

[legendecas]

Can this single key Map be replaced with a variable slot that can be either null or the resource object?

[addaleax]

The problem is that it needs to be some sort of object that can be easily modified from C++ as well (at least until we have weak references in JS without a flag)… I picked a map because clearing it from C++ never throws exceptions, but it could also be a single-entry Array or object.

[addaleax]

Alternatively, yes, this could be a single variable slot, but that might become an issue with memory retention in some weird edge cases.

[Qard]

Why not just use a symbol property? 🤔

[addaleax]

@Qard Symbol property on what/in place of what? If you’re talking about the map key, that would work just as well, yes. If you’re talking about replacing the map with an object with a single symbol key, that’s harder to clear from C++ than a map, I’d say?

[addaleax]

Btw, we can remove quite a bit of complexity here once we have weak refs without a flag. That would be the actual correct thing to use here. (The internal WeakRef we use elsewhere most likely isn’t fast enough, so we’ll need to wait for V8’s one.)

[Qard]

Not sure how I feel about this. I'm 👍 for the performance improvement aspect, but the way this is done makes an already complicated thing even more difficult to follow. I don't have a specific idea on how to do it better at the moment though, so I don't want to block it. I'll go with +0 for now and think on it a bit.

[ronag]

28e6626 (#33131) is what breaks the benchmarks for me. /cc @ronag

I just ran it on master and it seems fine? I have not run this before myself but the output seems reasonable?

node$ node benchmark/async_hooks/async-resource-vs-destroy.js 
async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path="/" asyncMethod="callbacks" type="async-resource" benchmarker="test-double-http": 405
async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path="/" asyncMethod="async" type="async-resource" benchmarker="test-double-http": 404
async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path="/" asyncMethod="callbacks" type="destroy" benchmarker="test-double-http": 411
async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path="/" asyncMethod="async" type="destroy" benchmarker="test-double-http": 396
async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path="/" asyncMethod="callbacks" type="async-local-storage" benchmarker="test-double-http": 402
async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path="/" asyncMethod="async" type="async-local-storage" benchmarker="test-double-http": 405

[addaleax]

@ronag Here’s what I’m getting:

$ ./node benchmark/async_hooks/async-resource-vs-destroy.js 
Error [ERR_STREAM_DESTROYED]: Cannot call write after a stream was destroyed
    at write_ (_http_outgoing.js:689:11)
    at ServerResponse.end (_http_outgoing.js:783:5)
    at /home/addaleax/src/node/master/benchmark/async_hooks/async-resource-vs-destroy.js:152:13
    at FSReqCallback.readFileAfterClose [as oncomplete] (internal/fs/read_file_context.js:63:3)
Emitted 'error' event on ServerResponse instance at:
    at emitErrorNt (_http_outgoing.js:666:43)
    at processTicksAndRejections (internal/process/task_queues.js:85:21) {
  code: 'ERR_STREAM_DESTROYED'
}

[ronag]

@ronag Here’s what I’m getting:

Looks to me like the benchmark does not wait for all the requests to finish. Hence, once it's dispatched all the requests it closes the server and the response is closed, once the setTimeout fires it will try to end() a closed response and errors.

Is it possible to make close async and have it wait a bit before closing the server? Or just to be quick:

      bench.http({
        path,
        connections,
        duration
      }, () => {
        setTimeout(close, 100); // let requests complete
      });

Sorry to hijack this PR. Should we move this to a separate issue?

[rochdev]

So I guess this PR makes a tradeoff, but I think I would generally accept a +18 % speedup without async_hooks at the cost of a -15 % perf drop when they are enabled?

@addaleax Can you clarify if this means not being used at all, or being enabled either with asyncHooks.createHook().enabled or with AsyncLocalStorage? Mostly asking because I would expect pretty much every serious production apps to use one or the other through an APM tool, so it could be detrimental to everyone depending on which one it is.

[addaleax]

@rochdev The -15 % is referring to asyncResource() usage, including AsyncLocalStorage, but no significant impact with enabled async_hooks in general. That’s what the benchmarks I posted in the comment above state, at least, and it’s consistent with the actual code in this PR.

[Qard]

So there's two separate things happening in this PR: splitting the resource stack, and caching return values from executionAsyncResource. I'm unclear on exactly where the performance improvement is coming from and if we really need both things. Did you try doing either thing on its own to see what the performance impact was? I'd definitely be happy with the caching part, if that's where much of the performance boost is coming from. 🤔

I'm a bit unclear though on why the previous design with the shared array was so slow. Do you have some insight into that which you could share?

[addaleax]

@Qard Caching the executionAsyncResource() value is pointless in the previous code (i.e. current master). Accessing a JS array at a fixed index from JS is basically free.

The problem is that the Get()/Set()/Delete() operations on Arrays from C++ are really expensive, and that’s where the performance improvement comes from. Currently, those calls make up about 20 % of the postMessage() benchmark run time, hence the significant improvements there by getting rid of them.

The problem with this PR, and the reason for the perf impact when async_hooks are enabled, is that Maps aren’t really that fast to access from JS, at least when compared to Arrays, and that the first (non-cached) call to executionAsyncResource() still needs to call back into C++ (hence the caching – it’s there to lower the negative perf impact of that change here in this PR). We can be okay with that but I’m not sure how I feel about it in general.

Ideally, what we’d want is some kind of data structure that can be accessed without significant overhead from both C++ and JS. I’ve thought about that quite a bit, and my personal favourite would be a way to access internal fields of objects from JS (e.g. via V8 extras). However, V8 provides no such built-ins at the time, and there doesn’t appear to be any infrastructure in place to get those. Theoretically, that should be able to come with zero overhead on both sides, though.

[Qard]

So one thing I’ve been looking at in relation to my MakeCallback trampoline is seeing if all the resource stack stuff could be managed entirely on the JS-side within that trampoline. It’s possible we might not need that stuff to live on the native side at all. 🤔

[addaleax]

@Qard Yes, that’s also one alternative I’ve thought about. It seems like quite a bit of work to me, but if you can make that work, awesome :)

[addaleax]

I’ll close this in favor of #33801.