async_hooks: improve resource stack performance by addaleax · Pull Request #33575 · nodejs/node · GitHub

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

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%

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

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

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.

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

Why not just use a symbol property? 🤔

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

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

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.

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

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

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.

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

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?

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

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

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

I’ll close this in favor of #33801.

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++. In particular, this is useful when using the async_hooks callback trampoline. This particularly improves performance when async hooks are not being used. (This is continuation of nodejs#33575.) Refs: nodejs#33575

Removes some of the performance overhead that came with `executionAsyncResource()` by using the JS resource array only as a cache for the values provided by C++. The fact that we now use an entry trampoline is used to pass the resource without requiring extra C++/JS boundary crossings, and the direct accesses to the JS resource array from C++ are removed in all fast paths. This particularly improves performance when async hooks are not being used. This is a continuation of nodejs#33575 and shares some of its code with it. ./node benchmark/compare.js --new ./node --old ./node-master --runs 30 --filter messageport worker | Rscript benchmark/compare.R [00:06:14|% 100| 1/1 files | 60/60 runs | 2/2 configs]: Done confidence improvement accuracy (*) (**) (***) worker/messageport.js n=1000000 payload='object' ** 12.64 % ±7.30% ±9.72% ±12.65% worker/messageport.js n=1000000 payload='string' * 11.08 % ±9.00% ±11.98% ±15.59% ./node benchmark/compare.js --new ./node --old ./node-master --runs 20 --filter async-resource-vs-destroy async_hooks | Rscript benchmark/compare.R [00:22:35|% 100| 1/1 files | 40/40 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' 1.60 % ±7.35% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='async-resource' benchmarker='autocannon' 6.05 % ±6.57% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='destroy' benchmarker='autocannon' * 8.27 % ±7.50% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='async-local-storage' benchmarker='autocannon' 7.42 % ±8.22% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='async-resource' benchmarker='autocannon' 4.33 % ±7.84% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='destroy' benchmarker='autocannon' 5.96 % ±7.15% (**) (***) async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='async-local-storage' benchmarker='autocannon' ±9.84% ±12.94% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='async-resource' benchmarker='autocannon' ±8.81% ±11.60% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='destroy' benchmarker='autocannon' ±10.07% ±13.28% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='async-local-storage' benchmarker='autocannon' ±11.01% ±14.48% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='async-resource' benchmarker='autocannon' ±10.50% ±13.81% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='destroy' benchmarker='autocannon' ±9.58% ±12.62% Refs: nodejs#33575

Removes some of the performance overhead that came with `executionAsyncResource()` by using the JS resource array only as a cache for the values provided by C++. The fact that we now use an entry trampoline is used to pass the resource without requiring extra C++/JS boundary crossings, and the direct accesses to the JS resource array from C++ are removed in all fast paths. This particularly improves performance when async hooks are not being used. This is a continuation of #33575 and shares some of its code with it. ./node benchmark/compare.js --new ./node --old ./node-master --runs 30 --filter messageport worker | Rscript benchmark/compare.R [00:06:14|% 100| 1/1 files | 60/60 runs | 2/2 configs]: Done confidence improvement accuracy (*) (**) (***) worker/messageport.js n=1000000 payload='object' ** 12.64 % ±7.30% ±9.72% ±12.65% worker/messageport.js n=1000000 payload='string' * 11.08 % ±9.00% ±11.98% ±15.59% ./node benchmark/compare.js --new ./node --old ./node-master --runs 20 --filter async-resource-vs-destroy async_hooks | Rscript benchmark/compare.R [00:22:35|% 100| 1/1 files | 40/40 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' 1.60 % ±7.35% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='async-resource' benchmarker='autocannon' 6.05 % ±6.57% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='destroy' benchmarker='autocannon' * 8.27 % ±7.50% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='async-local-storage' benchmarker='autocannon' 7.42 % ±8.22% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='async-resource' benchmarker='autocannon' 4.33 % ±7.84% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='destroy' benchmarker='autocannon' 5.96 % ±7.15% (**) (***) async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='async-local-storage' benchmarker='autocannon' ±9.84% ±12.94% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='async-resource' benchmarker='autocannon' ±8.81% ±11.60% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='destroy' benchmarker='autocannon' ±10.07% ±13.28% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='async-local-storage' benchmarker='autocannon' ±11.01% ±14.48% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='async-resource' benchmarker='autocannon' ±10.50% ±13.81% async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='destroy' benchmarker='autocannon' ±9.58% ±12.62% Refs: #33575 PR-URL: #34319 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels May 27, 2020

addaleax added async_hooks Issues and PRs related to the async hooks subsystem. performance Issues and PRs related to the performance of Node.js. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels May 27, 2020

legendecas reviewed May 27, 2020

View reviewed changes

addaleax mentioned this pull request May 27, 2020

async-resource-vs-destroy benchmark is broken locally #33591

Closed

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18

Qard mentioned this pull request Jun 9, 2020

async_hooks: callback trampoline for MakeCallback #33801

Closed

3 tasks

addaleax closed this Jun 10, 2020

addaleax deleted the async-resource-improvements branch July 11, 2020 23:49

addaleax mentioned this pull request Jul 12, 2020

async_hooks: improve resource stack performance #34319

Closed

2 tasks

snyk-bot mentioned this pull request Sep 27, 2022

[Snyk] Security upgrade gatsby from 2.18.18 to 4.0.0 aliceUnhinged613/node#67

Open

wedataintelligence mentioned this pull request Sep 27, 2022

[Snyk] Security upgrade gatsby from 2.18.18 to 4.0.0 wedataintelligence/node#49

Open

This was referenced Sep 27, 2022

[Snyk] Security upgrade gatsby from 2.13.73 to 4.0.0 kissofire/node#76

Open

[Snyk] Security upgrade gatsby from 2.18.18 to 4.0.0 molllyn1/node#56

Open

137717unity mentioned this pull request Sep 28, 2022

[Snyk] Security upgrade gatsby from 2.18.18 to 4.0.0 137717unity/node#74

Open

snyk-bot mentioned this pull request Sep 28, 2022

[Snyk] Security upgrade gatsby from 2.24.27 to 4.0.0 baby636/node#65

Open

ron-a20 mentioned this pull request Sep 28, 2022

[Snyk] Security upgrade gatsby from 2.18.18 to 4.0.0 ron-a20/node#74

Open

This was referenced Sep 28, 2022

[Snyk] Security upgrade gatsby from 2.18.18 to 4.0.0 Rednickle/node-1#59

Open

[Snyk] Security upgrade gatsby from 2.23.20 to 4.0.0 turkdevops/node#397

Open

137717unity mentioned this pull request Jun 23, 2023

[Snyk] Security upgrade gatsby from 2.18.18 to 4.0.0 137717unity/node#99

Open

molllyn1 mentioned this pull request Jun 23, 2023

[Snyk] Security upgrade gatsby from 2.18.18 to 4.0.0 molllyn1/node#82

Open

aliceUnhinged613 mentioned this pull request Jun 24, 2023

[Snyk] Security upgrade gatsby from 2.18.18 to 4.0.0 aliceUnhinged613/node#95

Open

ron-a20 mentioned this pull request Jun 24, 2023

[Snyk] Security upgrade gatsby from 2.18.18 to 4.0.0 ron-a20/node#98

Open

wedataintelligence mentioned this pull request Jun 24, 2023

[Snyk] Security upgrade gatsby from 2.18.18 to 4.0.0 wedataintelligence/node#81

Open

aliceUnhinged613 mentioned this pull request Nov 25, 2023

[Snyk] Fix for 28 vulnerabilities aliceUnhinged613/node#98

Open

wedataintelligence mentioned this pull request Nov 28, 2023

[Snyk] Fix for 28 vulnerabilities wedataintelligence/node#87

Open

baby636 mentioned this pull request Dec 7, 2023

[Snyk] Security upgrade gatsby from 2.24.27 to 4.0.0 baby636/node#121

Open

This was referenced Dec 15, 2023

[Snyk] Security upgrade gatsby from 2.24.27 to 4.0.0 baby636/node#122

Open

[Snyk] Fix for 43 vulnerabilities baby636/node#126

Open

baby636 mentioned this pull request Jan 6, 2024

[Snyk] Security upgrade gatsby from 2.24.27 to 4.0.0 baby636/node#128

Open

+              const jsResourceStack = new SafeMap();
+              // Contains either a single key (null) or nothing. If the key is present,
+              // this points to the current async resource.
+              const cachedResourceHolder = new SafeMap();

async_hooks: improve resource stack performance by addaleax · Pull Request #33575 · nodejs/node · GitHub

Uh oh!

Uh oh!

async_hooks: improve resource stack performance - #33575

async_hooks: improve resource stack performance#33575addaleax wants to merge 1 commit intonodejs:masternodejs/node:masterfrom addaleax:async-resource-improvementsaddaleax/node:async-resource-improvementsCopy head branch name to clipboard

Conversation

addaleax commented May 27, 2020

Checklist

Uh oh!

addaleax commented May 27, 2020

Uh oh!

legendecas May 27, 2020

Choose a reason for hiding this comment

Uh oh!

addaleax May 27, 2020

Choose a reason for hiding this comment

Uh oh!

addaleax May 27, 2020

Choose a reason for hiding this comment

Uh oh!

Qard May 27, 2020

Choose a reason for hiding this comment

Uh oh!

addaleax May 27, 2020

Choose a reason for hiding this comment

Uh oh!

addaleax May 27, 2020

Choose a reason for hiding this comment

Uh oh!

Qard commented May 27, 2020

Uh oh!

ronag commented May 27, 2020

Uh oh!

addaleax commented May 27, 2020

Uh oh!

ronag commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rochdev commented May 27, 2020

Uh oh!

addaleax commented May 27, 2020

Uh oh!

Qard commented Jun 2, 2020

Uh oh!

addaleax commented Jun 5, 2020

Uh oh!

Qard commented Jun 5, 2020

Uh oh!

addaleax commented Jun 5, 2020

Uh oh!

addaleax commented Jun 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

async_hooks: improve resource stack performance#33575
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:async-resource-improvements

ronag commented May 27, 2020 •
edited
Loading