async_hooks: fast-path for PromiseHooks in JS by Qard · Pull Request #32891 · nodejs/node · GitHub

This is an attempt to eliminate the GC-tracking of promises needed to emit destroy events to async_hooks if there is nothing listening to those events. I've replicated the PromiseHook handler and PromiseWrap behaviour in JS to allow a fast-path for handling promise-related events when there is no destroy hook present and therefore no need for expensive gc-tracking. This should significantly improve the performance of async_hooks in promise-heavy applications for the typical/suggested case of context management with AsyncLocalStorage.

With this comes a few important changes to consider:

Promises will no longer emit trace events unless there is a destroy hook
The resource for a promise event will no longer have the isChainedPromise property (Though there are ideas of how this could be restored, if there are objections)

/cc @mcollina @addaleax

I like the latest commit. I also had an idea that we could use Promise objects as resources without allocating extra wrap objects. I guess, with the current state of this PR it should be ready for initial benchmarking. WDYT?

Started a micro-benchmarks build: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/561/

Got result for async_hooks/promises.js benchmark (https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/561/):

19:10:28                                                          confidence improvement accuracy (*)   (**)  (***)
19:10:28  async_hooks/promises.js asyncHooks='disabled' n=1000000                -0.44 %       ±3.42% ±4.54% ±5.91%
19:10:28  async_hooks/promises.js asyncHooks='enabled' n=1000000         ***     36.47 %       ±2.94% ±3.93% ±5.15%

Update. I've added a variation of async_hooks/promises.js benchmark with destory hook enabled locally just to compare the old (native) implementation of PromiseHook with the JS one proposed in this PR. Here is the result obtained on my machine (--runs 30):

                                                                       confidence improvement accuracy (*)   (**)  (***)
 async_hooks/promises.js asyncHooks='enabledWithDestroyHook' n=1000000        ***    -13.32 %       ±0.96% ±1.29% ±1.69%

I also got results for async_hooks/async-resource-vs-destroy.js on my machine with --runs 30 setting and autocannon as the benchmarker:

$ cat out-http.csv | Rscript benchmark/compare.R
                                                                                                                                                                    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'            ***      6.67 %       ±1.77% ±2.40% ±3.20%
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='async-resource' benchmarker='autocannon'                 ***      6.96 %       ±3.61% ±4.87% ±6.48%
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='destroy' benchmarker='autocannon'                          *     -3.59 %       ±3.08% ±4.16% ±5.55%
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='async-local-storage' benchmarker='autocannon'                -0.64 %       ±1.68% ±2.27% ±3.04%
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='async-resource' benchmarker='autocannon'                     -1.69 %       ±3.59% ±4.85% ±6.45%
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='destroy' benchmarker='autocannon'                            -0.16 %       ±2.97% ±4.04% ±5.44%

I also tried running it with wrk on my local machine and on CI, but had to cancel those runs as it took 14+h. It seems it gets stuck eventually when wrk is used as the benchmarker.

Update. The above benchmarks were run for the previous version of this PoC. They don't highlight current performance.

@jasnell In regard to the missing trace events we discussed in nodejs/diagnostics#376, do you think it would be reasonable to apply two different wraps depending on if there is a destroy hook? If a destroy hook is present it would apply the full wrap with gc-tracking, and if it's absent it would only apply the simplified wrap. I'm thinking of exposing the wrap class to JS and switching the logic based on the destroy hook presence. Another option would be to have two separate PromiseHook handler functions and switch between them based on destroy handler presence. 🤔

My hope was to be able to separate PromiseHook somewhat, as I'd ultimately like to make it a separate API eventually. Might be difficult though.

In regard to the missing trace events we discussed in nodejs/diagnostics#376, do you think it would be reasonable to apply two different wraps depending on if there is a destroy hook? If a destroy hook is present it would apply the full wrap with gc-tracking, and if it's absent it would only apply the simplified wrap.

I'm not @jasnell but I can answer this as well. Go ahead with two different wraps depending if there is a destroy hook. It's unlikely we will be ever able to remove the destroy hook, but if we can provide a fast path it would be very handy. I would make sure we emit some debug info when one or the other is turned on (NODE_DEBUG).

Sounds like a plan. Thanks for the input! 😄

Landed in 13c5a16

This avoids the need to wrap every promise in an AsyncWrap and also makes it easier to skip the machinery to track destroy events when there's no destroy listener. Co-authored-by: Andrey Pechkurov <apechkurov@gmail.com> PR-URL: #32891 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

../src/async_wrap.cc: In function ‘uint16_t node::ToAsyncHooksType(v8::PromiseHookType)’: ../src/async_wrap.cc:313:1: error: control reaches end of non-void function [-Werror=return-type] } Refs: nodejs#32891

../src/async_wrap.cc: In function ‘uint16_t node::ToAsyncHooksType(v8::PromiseHookType)’: ../src/async_wrap.cc:313:1: error: control reaches end of non-void function [-Werror=return-type] } Refs: #32891 PR-URL: #33322 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

This avoids the need to wrap every promise in an AsyncWrap and also makes it easier to skip the machinery to track destroy events when there's no destroy listener. Co-authored-by: Andrey Pechkurov <apechkurov@gmail.com> PR-URL: #32891 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

../src/async_wrap.cc: In function ‘uint16_t node::ToAsyncHooksType(v8::PromiseHookType)’: ../src/async_wrap.cc:313:1: error: control reaches end of non-void function [-Werror=return-type] } Refs: #32891 PR-URL: #33322 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Notable changes: async_hooks: * (SEMVER-MINOR) move PromiseHook handler to JS (Stephen Belanger) #32891 cli: * (SEMVER-MINOR) add `--trace-atomics-wait` flag (Anna Henningsen) #33292 fs: * (SEMVER-MINOR) add .ref() and .unref() methods to watcher classes (rickyes) #33134 http: * (SEMVER-MINOR) expose http.validate-header-name/value (osher) #33119 repl: * (SEMVER-MINOR) deprecate repl._builtinLibs (Ruben Bridgewater) #33294 * (SEMVER-MINOR) remove obsolete completer variable (Ruben Bridgewater) #33294 * (SEMVER-MINOR) deprecate repl.inputStream and repl.outputStream (Ruben Bridgewater) #33294 * (SEMVER-MINOR) improve repl autocompletion for require calls (Ruben Bridgewater) #33282 * (SEMVER-MINOR) replace hard coded core module list with actual list (Ruben Bridgewater) #33282 * (SEMVER-MINOR) show reference errors during preview (Ruben Bridgewater) #33282 * (SEMVER-MINOR) improve repl preview (Ruben Bridgewater) #33282 src: * add support for TLA (Gus Caplan) #30370 test: * (SEMVER-MINOR) refactor test/parallel/test-bootstrap-modules.js (Ruben Bridgewater) #33282 PR-URL: TODO

Notable changes: async_hooks: * (SEMVER-MINOR) move PromiseHook handler to JS (Stephen Belanger) #32891 cli: * (SEMVER-MINOR) add `--trace-atomics-wait` flag (Anna Henningsen) #33292 fs: * (SEMVER-MINOR) add .ref() and .unref() methods to watcher classes (rickyes) #33134 http: * (SEMVER-MINOR) expose http.validate-header-name/value (osher) #33119 repl: * (SEMVER-MINOR) deprecate repl._builtinLibs (Ruben Bridgewater) #33294 * (SEMVER-MINOR) deprecate repl.inputStream and repl.outputStream (Ruben Bridgewater) #33294 * (SEMVER-MINOR) show reference errors during preview (Ruben Bridgewater) #33282 * (SEMVER-MINOR) improve repl preview (Ruben Bridgewater) #33282 src: * add support for TLA (Gus Caplan) #30370 PR-URL: TODO

Notable changes: async_hooks: * (SEMVER-MINOR) move PromiseHook handler to JS (Stephen Belanger) #32891 cli: * (SEMVER-MINOR) add `--trace-atomics-wait` flag (Anna Henningsen) #33292 fs: * (SEMVER-MINOR) add .ref() and .unref() methods to watcher classes (rickyes) #33134 http: * (SEMVER-MINOR) expose http.validate-header-name/value (osher) #33119 repl: * (SEMVER-MINOR) deprecate repl._builtinLibs (Ruben Bridgewater) #33294 * (SEMVER-MINOR) deprecate repl.inputStream and repl.outputStream (Ruben Bridgewater) #33294 * (SEMVER-MINOR) show reference errors during preview (Ruben Bridgewater) #33282 * (SEMVER-MINOR) improve repl preview (Ruben Bridgewater) #33282 src: * add support for TLA (Gus Caplan) #30370 PR-URL: #33452

Qard added lib / src Issues and PRs related to general changes in the lib or src directory. async_hooks Issues and PRs related to the async hooks subsystem. labels Apr 16, 2020

Qard self-assigned this Apr 16, 2020

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 16, 2020

Qard mentioned this pull request Apr 16, 2020

async_hooks performance/stability improvement plans nodejs/diagnostics#376

Closed

Qard force-pushed the promise-hooks-in-js branch 4 times, most recently from 6e714dd to c92c5da Compare April 16, 2020 23:54

puzpuzpuz reviewed Apr 17, 2020

View reviewed changes

Comment thread lib/internal/async_hooks.js Outdated

puzpuzpuz reviewed Apr 17, 2020

View reviewed changes

Comment thread src/async_wrap.cc Outdated

puzpuzpuz reviewed Apr 17, 2020

View reviewed changes

Comment thread src/env.cc Outdated

Qard force-pushed the promise-hooks-in-js branch from c92c5da to 9b75102 Compare April 17, 2020 19:28

puzpuzpuz reviewed Apr 18, 2020

View reviewed changes

Comment thread lib/internal/async_hooks.js Outdated

Qard added the diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. label Apr 21, 2020

Qard mentioned this pull request Apr 21, 2020

Node.js Diagnostics WorkGroup Meeting 2020-04-22 nodejs/diagnostics#381

Closed

Qard force-pushed the promise-hooks-in-js branch from 11fbcf7 to 282653b Compare April 21, 2020 21:52

puzpuzpuz reviewed Apr 22, 2020

View reviewed changes

Comment thread src/async_wrap.cc Outdated

Flarna reviewed Apr 23, 2020

View reviewed changes

Comment thread lib/internal/async_hooks.js Outdated

Flarna reviewed Apr 23, 2020

View reviewed changes

Comment thread src/env.cc Outdated

Flarna reviewed Apr 23, 2020

View reviewed changes

Comment thread lib/internal/async_hooks.js Outdated

Flarna reviewed Apr 23, 2020

View reviewed changes

Comment thread lib/internal/async_hooks.js Outdated

Flarna reviewed Apr 23, 2020

View reviewed changes

Comment thread lib/internal/async_hooks.js Outdated

mcollina mentioned this pull request May 5, 2020

Node.js Technical Steering Committee (TSC) Meeting 2020-05-07 nodejs/TSC#860

Closed

legendecas approved these changes May 6, 2020

View reviewed changes

addaleax closed this May 9, 2020

addaleax mentioned this pull request May 9, 2020

src: fix compiler warning in async_wrap.cc #33322

Closed

2 tasks

addaleax added dont-land-on-v10.x and removed dont-land-on-v14.x labels May 10, 2020

Qard deleted the promise-hooks-in-js branch May 11, 2020 00:16

codebytere mentioned this pull request May 18, 2020

v14.3.0 proposal #33452

Merged

johanneswuerbach mentioned this pull request Jun 17, 2020

AsyncLocalStorage based ContextManager open-telemetry/opentelemetry-js#1210

Merged

This was referenced Jul 25, 2020

feat: add async HTTP context adonisjs/http-server#18

Merged

[Experiment] async_hooks: optimize fast-path promise hook for ALS #34512

Closed

Flarna mentioned this pull request Mar 8, 2022

Async hooks cause programs with frozen promises to crash #42229

Closed

async_hooks: fast-path for PromiseHooks in JS by Qard · Pull Request #32891 · nodejs/node · GitHub

Uh oh!

Uh oh!

async_hooks: fast-path for PromiseHooks in JS - #32891

async_hooks: fast-path for PromiseHooks in JS#32891Qard wants to merge 16 commits intonodejs:masternodejs/node:masterfrom Qard:promise-hooks-in-jsQard/node:promise-hooks-in-jsCopy head branch name to clipboard

Conversation

Qard commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jasnell commented Apr 18, 2020

Uh oh!

Uh oh!

puzpuzpuz commented Apr 18, 2020

Uh oh!

puzpuzpuz commented Apr 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

puzpuzpuz commented Apr 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Qard commented Apr 20, 2020

Uh oh!

mcollina commented Apr 21, 2020

Uh oh!

Qard commented Apr 21, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

addaleax commented May 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

async_hooks: fast-path for PromiseHooks in JS#32891
Qard wants to merge 16 commits into
nodejs:masterfrom
Qard:promise-hooks-in-js

Qard commented Apr 16, 2020 •
edited
Loading

puzpuzpuz commented Apr 18, 2020 •
edited
Loading

puzpuzpuz commented Apr 19, 2020 •
edited
Loading