GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — async_hooks: fast-path for PromiseHooks in JS by Qard · Pull Request #32891 · nodejs/node

Conversation

@Qard

@Qard Qard commented Apr 16, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
himself65, puzpuzpuz, and BridgeAR reacted with thumbs up emoji mmarchini, bricss, legendecas, puzpuzpuz, BridgeAR, and wtchangdm reacted with heart emoji
All reactions
  • 👍 3 reactions
  • ❤️ 6 reactions
@Qard Qard added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. async_hooks Issues and PRs related to the async hooks subsystem. labels Apr 16, 2020
@Qard Qard self-assigned this Apr 16, 2020
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 16, 2020
@Qard Qard mentioned this pull request Apr 16, 2020
async_hooks performance/stability improvement plans nodejs/diagnostics#376
Closed
@Qard
Qard force-pushed the promise-hooks-in-js branch 4 times, most recently from 6e714dd to c92c5da Compare April 16, 2020 23:54
puzpuzpuz
puzpuzpuz reviewed Apr 17, 2020
View reviewed changes
Comment thread lib/internal/async_hooks.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
puzpuzpuz
puzpuzpuz reviewed Apr 17, 2020
View reviewed changes
Comment thread src/async_wrap.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
puzpuzpuz
puzpuzpuz reviewed Apr 17, 2020
View reviewed changes
Comment thread src/env.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@Qard
Qard force-pushed the promise-hooks-in-js branch from c92c5da to 9b75102 Compare April 17, 2020 19:28
@jasnell

jasnell commented Apr 18, 2020

Copy link
Copy Markdown
Member

/cc @mcollina @addaleax
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
puzpuzpuz
puzpuzpuz reviewed Apr 18, 2020
View reviewed changes
Comment thread lib/internal/async_hooks.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@puzpuzpuz

puzpuzpuz commented Apr 18, 2020

Copy link
Copy Markdown
Member

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@puzpuzpuz

puzpuzpuz commented Apr 18, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@puzpuzpuz

puzpuzpuz commented Apr 19, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Qard

Qard commented Apr 20, 2020

Copy link
Copy Markdown
Member Author

@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.
puzpuzpuz reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Apr 21, 2020

Copy link
Copy Markdown
Member

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).
jasnell, puzpuzpuz, and Qard reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Qard

Qard commented Apr 21, 2020

Copy link
Copy Markdown
Member Author

Sounds like a plan. Thanks for the input! 😄
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Qard Qard added the diag-agenda Issues and PRs to discuss during Diagnostics Working Group meetings. label Apr 21, 2020
@Qard Qard mentioned this pull request Apr 21, 2020
Node.js Diagnostics WorkGroup Meeting 2020-04-22 nodejs/diagnostics#381
Closed
@Qard
Qard force-pushed the promise-hooks-in-js branch from 11fbcf7 to 282653b Compare April 21, 2020 21:52
puzpuzpuz
puzpuzpuz reviewed Apr 22, 2020
View reviewed changes
Comment thread src/async_wrap.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Flarna
Flarna reviewed Apr 23, 2020
View reviewed changes
Comment thread lib/internal/async_hooks.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Flarna
Flarna reviewed Apr 23, 2020
View reviewed changes
Comment thread src/env.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Flarna
Flarna reviewed Apr 23, 2020
View reviewed changes
Comment thread lib/internal/async_hooks.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Flarna
Flarna reviewed Apr 23, 2020
View reviewed changes
Comment thread lib/internal/async_hooks.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Flarna
Flarna reviewed Apr 23, 2020
View reviewed changes
Comment thread lib/internal/async_hooks.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@mcollina mcollina mentioned this pull request May 5, 2020
Node.js Technical Steering Committee (TSC) Meeting 2020-05-07 nodejs/TSC#860
Closed
legendecas
legendecas approved these changes May 6, 2020
View reviewed changes
@addaleax

addaleax commented May 9, 2020

Copy link
Copy Markdown
Member

Landed in 13c5a16
puzpuzpuz and Qard reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax pushed a commit that referenced this pull request May 9, 2020
@Qard @addaleax
async_hooks: move PromiseHook handler to JS
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 <[email protected]>

PR-URL: #32891
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@addaleax addaleax closed this May 9, 2020
addaleax added a commit to addaleax/node that referenced this pull request May 9, 2020
@addaleax
src: fix compiler warning in async_wrap.cc
4cfbd58 
    ../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
@addaleax addaleax mentioned this pull request May 9, 2020
src: fix compiler warning in async_wrap.cc #33322
Closed
2 tasks done
addaleax added a commit that referenced this pull request May 9, 2020
@addaleax
src: fix compiler warning in async_wrap.cc
ae34e47 
    ../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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@addaleax addaleax added dont-land-on-v10.x and removed dont-land-on-v14.x labels May 10, 2020
@Qard
Qard deleted the promise-hooks-in-js branch May 11, 2020 00:16
codebytere pushed a commit that referenced this pull request May 11, 2020
@Qard @codebytere
async_hooks: move PromiseHook handler to JS
b061655 
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 <[email protected]>

PR-URL: #32891
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
codebytere pushed a commit that referenced this pull request May 11, 2020
@addaleax @codebytere
src: fix compiler warning in async_wrap.cc
fd9c7c2 
    ../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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
codebytere added a commit that referenced this pull request May 18, 2020
@codebytere
2020-05-19, Version 14.3.0 (Current)
2324d42 
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
@codebytere codebytere mentioned this pull request May 18, 2020
v14.3.0 proposal #33452
Merged
codebytere added a commit that referenced this pull request May 19, 2020
@codebytere
2020-05-19, Version 14.3.0 (Current)
76a52dd 
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
codebytere added a commit that referenced this pull request May 19, 2020
@codebytere
2020-05-19, Version 14.3.0 (Current)
a33e87c 
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
codebytere added a commit that referenced this pull request May 19, 2020
@codebytere
2020-05-19, Version 14.3.0 (Current)
5d81e4d 
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
codebytere added a commit that referenced this pull request May 19, 2020
@codebytere
2020-05-19, Version 14.3.0 (Current)
c841f51 
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
@johanneswuerbach 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 Flarna mentioned this pull request Mar 8, 2022
Async hooks cause programs with frozen promises to crash #42229
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@addaleax addaleax addaleax approved these changes
@legendecas legendecas legendecas approved these changes
@Flarna Flarna Flarna approved these changes
+1 more reviewer
@puzpuzpuz puzpuzpuz puzpuzpuz approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. diag-agenda Issues and PRs to discuss during Diagnostics Working Group meetings. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@Qard @jasnell @puzpuzpuz @mcollina @Flarna @nodejs-github-bot @addaleax @legendecas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.