async_hooks: callback trampoline for MakeCallback by Qard · Pull Request #33801 · nodejs/node

Qard · 2020-06-09T03:47:32Z

This is the start of an effort to eliminate the additional two native-to-JavaScript barrier crosses for the before and after events when async_hooks is active. It also moves the domain callback to be triggered within the trampoline rather than on the native side, when there are before/after events to trigger.

I plan to work on making the domain callback trigger fully JavaScript-side tomorrow which should eliminate the need to even have native-side code for domains. Currently the only native code we have for domains is storing the callback in the environment so it can be triggered from this single location in callback.cc. With that no longer being triggered on the native side there's actually no reason to even store it on the native side so that could be easily eliminated to allow domains to live entirely on the JS side.

I'm also hoping to move the execution async resource stack to be managed purely on the JS side too, through this trampoline. This should eliminate the need for the extra state synchronization code needed in #33575 for the performance gains it aims to make.

Side note: There's no try/catch in the trampoline because it currently relies on InternalCallbackScope to catch errors and handle them appropriately. It's also relying on InternalCallbackScope for other things like timer and microtask management, so some deeper modifications will need to be made if we want to move any of that to the JS side too. Personally, I think it's worth considering, and I plan to continue investigating that idea.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

addaleax

Looks very nice so far! 🚀

Qard · 2020-06-09T22:10:06Z

I added a commit which removes the last vestiges of native domain code, moving the domain hook callback to be registered directly with async_hooks internals. Eventually I hope to generalize that as a feature for other hook mechanisms like fs hooks, audit hooks, etc. to be able to hook more deeply into the callback execution flow. For now, this domain-specific function storing should be enough.

Qard · 2020-06-10T23:08:30Z

Here's a full run of all the async_hooks benchmarks.

❯ node benchmark/compare.js --old ./node-master --new ./node-pr-33801 async_hooks | Rscript benchmark/compare.R
[01:57:57|% 100| 4/4 files | 60/60 runs | 3/3 configs]: Done
                                                                                                                                                                           confidence improvement
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='async-local-storage' benchmarker='test-double-http'                     0.84 %
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='async-resource' benchmarker='test-double-http'                          1.89 %
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='destroy' benchmarker='test-double-http'                                 1.16 %
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='async-local-storage' benchmarker='test-double-http'                 1.87 %
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='async-resource' benchmarker='test-double-http'                      1.15 %
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='destroy' benchmarker='test-double-http'                             1.66 %
 async_hooks/gc-tracking.js method='trackingDisabled' n=1000000                                                                                                                           1.01 %
 async_hooks/gc-tracking.js method='trackingEnabled' n=1000000                                                                                                                            2.82 %
 async_hooks/gc-tracking.js method='trackingEnabledWithDestroyHook' n=1000000                                                                                                             2.62 %
 async_hooks/http-server.js duration=5 connections=50 asyncHooks='after' benchmarker='test-double-http'                                                                                   4.02 %
 async_hooks/http-server.js duration=5 connections=50 asyncHooks='all' benchmarker='test-double-http'                                                                                     7.54 %
 async_hooks/http-server.js duration=5 connections=50 asyncHooks='before' benchmarker='test-double-http'                                                                                 -5.42 %
 async_hooks/http-server.js duration=5 connections=50 asyncHooks='disabled' benchmarker='test-double-http'                                                                               -1.76 %
 async_hooks/http-server.js duration=5 connections=50 asyncHooks='init' benchmarker='test-double-http'                                                                                  -13.10 %
 async_hooks/http-server.js duration=5 connections=50 asyncHooks='none' benchmarker='test-double-http'                                                                                   -1.30 %
 async_hooks/http-server.js duration=5 connections=500 asyncHooks='after' benchmarker='test-double-http'                                                                                  3.11 %
 async_hooks/http-server.js duration=5 connections=500 asyncHooks='all' benchmarker='test-double-http'                                                                                   -6.92 %
 async_hooks/http-server.js duration=5 connections=500 asyncHooks='before' benchmarker='test-double-http'                                                                                 0.16 %
 async_hooks/http-server.js duration=5 connections=500 asyncHooks='disabled' benchmarker='test-double-http'                                                                               9.65 %
 async_hooks/http-server.js duration=5 connections=500 asyncHooks='init' benchmarker='test-double-http'                                                                                  -4.49 %
 async_hooks/http-server.js duration=5 connections=500 asyncHooks='none' benchmarker='test-double-http'                                                                                   3.37 %
 async_hooks/promises.js asyncHooks='disabled' n=1000000                                                                                                                                  3.53 %
 async_hooks/promises.js asyncHooks='enabled' n=1000000                                                                                                                                  -4.48 %
 async_hooks/promises.js asyncHooks='enabledWithDestroy' n=1000000                                                                                                                        2.11 %
                                                                                                                                                                          accuracy (*)    (**)   (***)
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='async-local-storage' benchmarker='test-double-http'           ±3.52%  ±4.69%  ±6.10%
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='async-resource' benchmarker='test-double-http'                ±3.90%  ±5.19%  ±6.75%
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='async' type='destroy' benchmarker='test-double-http'                       ±3.41%  ±4.54%  ±5.91%
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='async-local-storage' benchmarker='test-double-http'       ±3.50%  ±4.66%  ±6.06%
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='async-resource' benchmarker='test-double-http'            ±3.74%  ±4.98%  ±6.48%
 async_hooks/async-resource-vs-destroy.js n=1000000 duration=5 connections=500 path='/' asyncMethod='callbacks' type='destroy' benchmarker='test-double-http'                   ±3.56%  ±4.74%  ±6.17%
 async_hooks/gc-tracking.js method='trackingDisabled' n=1000000                                                                                                                 ±7.10%  ±9.45% ±12.31%
 async_hooks/gc-tracking.js method='trackingEnabled' n=1000000                                                                                                                  ±6.32%  ±8.42% ±10.96%
 async_hooks/gc-tracking.js method='trackingEnabledWithDestroyHook' n=1000000                                                                                                   ±5.68%  ±7.56%  ±9.84%
 async_hooks/http-server.js duration=5 connections=50 asyncHooks='after' benchmarker='test-double-http'                                                                        ±20.25% ±26.97% ±35.14%
 async_hooks/http-server.js duration=5 connections=50 asyncHooks='all' benchmarker='test-double-http'                                                                          ±21.92% ±29.19% ±38.02%
 async_hooks/http-server.js duration=5 connections=50 asyncHooks='before' benchmarker='test-double-http'                                                                       ±17.45% ±23.22% ±30.23%
 async_hooks/http-server.js duration=5 connections=50 asyncHooks='disabled' benchmarker='test-double-http'                                                                     ±18.06% ±24.03% ±31.28%
 async_hooks/http-server.js duration=5 connections=50 asyncHooks='init' benchmarker='test-double-http'                                                                         ±32.32% ±43.30% ±56.95%
 async_hooks/http-server.js duration=5 connections=50 asyncHooks='none' benchmarker='test-double-http'                                                                         ±15.37% ±20.46% ±26.64%
 async_hooks/http-server.js duration=5 connections=500 asyncHooks='after' benchmarker='test-double-http'                                                                       ±19.12% ±25.45% ±33.15%
 async_hooks/http-server.js duration=5 connections=500 asyncHooks='all' benchmarker='test-double-http'                                                                         ±17.86% ±23.76% ±30.93%
 async_hooks/http-server.js duration=5 connections=500 asyncHooks='before' benchmarker='test-double-http'                                                                      ±18.15% ±24.18% ±31.53%
 async_hooks/http-server.js duration=5 connections=500 asyncHooks='disabled' benchmarker='test-double-http'                                                                    ±14.30% ±19.06% ±24.85%
 async_hooks/http-server.js duration=5 connections=500 asyncHooks='init' benchmarker='test-double-http'                                                                        ±17.41% ±23.17% ±30.16%
 async_hooks/http-server.js duration=5 connections=500 asyncHooks='none' benchmarker='test-double-http'                                                                        ±17.19% ±22.88% ±29.77%
 async_hooks/promises.js asyncHooks='disabled' n=1000000                                                                                                                        ±8.05% ±10.73% ±14.01%
 async_hooks/promises.js asyncHooks='enabled' n=1000000                                                                                                                         ±8.50% ±11.32% ±14.74%
 async_hooks/promises.js asyncHooks='enabledWithDestroy' n=1000000                                                                                                             ±10.50% ±13.98% ±18.20%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 24 comparisons, you can thus
expect the following amount of false-positive results:
  1.20 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.24 false positives, when considering a   1% risk acceptance (**, ***),
  0.02 false positives, when considering a 0.1% risk acceptance (***)

With the async_hooks callback trampoline, domains no longer need any native code. With this, domains can exist in pure JavaScript.

addaleax · 2020-06-11T15:20:15Z

@Qard Can you also run the MessagePort benchmark as I did in #33575? It’s somewhat ironic but I find that it tends to give much clearer numbers.

puzpuzpuz · 2020-06-11T17:20:49Z

I've run worker/messageport.js benchmark on CI and here is the result:

20:10:10                                                   confidence improvement accuracy (*)   (**)  (***)
20:10:10  worker/messageport.js n=1000000 payload='object'        ***      3.95 %       ±2.06% ±2.74% ±3.56%
20:10:10  worker/messageport.js n=1000000 payload='string'        ***      6.48 %       ±2.54% ±3.39% ±4.41%
20:10:10 
20:10:10 Be aware that when doing many comparisons the risk of a false-positive
20:10:10 result increases. In this case there are 2 comparisons, you can thus
20:10:10 expect the following amount of false-positive results:
20:10:10   0.10 false positives, when considering a   5% risk acceptance (*, **, ***),
20:10:10   0.02 false positives, when considering a   1% risk acceptance (**, ***),
20:10:10   0.00 false positives, when considering a 0.1% risk acceptance (***)

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/604/

Qard · 2020-06-11T17:41:14Z

Switched to ready to review. It's functional as it is and seems to provide a bit of improvement already. I still intend to follow this up with some more changes I'm working on now to move the resource stack management to JS, but I'm less certain about that part due to the current need to always track the resources, even when async_hooks is disabled.

nodejs-github-bot · 2020-06-19T03:50:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31936/

Qard 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. async_hooks Issues and PRs related to the async hooks subsystem. labels Jun 9, 2020

Qard requested review from addaleax, puzpuzpuz and vdeturckheim June 9, 2020 03:47

vdeturckheim reviewed Jun 9, 2020

View reviewed changes

Comment thread lib/internal/async_hooks.js Outdated

addaleax reviewed Jun 9, 2020

View reviewed changes

Comment thread lib/internal/async_hooks.js Outdated

Comment thread src/api/callback.cc Outdated

Comment thread src/api/callback.cc Outdated

Qard force-pushed the async-hooks-callback-trampoline branch from 2096bab to 8942366 Compare June 9, 2020 17:25

Flarna reviewed Jun 9, 2020

View reviewed changes

Comment thread src/api/callback.cc Outdated

addaleax mentioned this pull request Jun 10, 2020

async_hooks: improve resource stack performance #33575

Closed

2 tasks

devsnek reviewed Jun 10, 2020

View reviewed changes

Comment thread src/env.h Outdated

Qard force-pushed the async-hooks-callback-trampoline branch from a0fd282 to 410acd0 Compare June 10, 2020 23:24

Qard added 2 commits June 10, 2020 16:26

async_hooks: callback trampoline for MakeCallback

34d91e5

domain: remove native domain code

27276c3

With the async_hooks callback trampoline, domains no longer need any native code. With this, domains can exist in pure JavaScript.

Qard force-pushed the async-hooks-callback-trampoline branch from 410acd0 to 27276c3 Compare June 10, 2020 23:26

Qard marked this pull request as ready for review June 11, 2020 17:35

addaleax approved these changes Jun 11, 2020

View reviewed changes

vdeturckheim approved these changes Jun 11, 2020

View reviewed changes

devsnek approved these changes Jun 11, 2020

View reviewed changes

puzpuzpuz approved these changes Jun 11, 2020

View reviewed changes

Flarna reviewed Jun 11, 2020

View reviewed changes

Comment thread src/api/callback.cc

Flarna approved these changes Jun 11, 2020

View reviewed changes

Qard added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 15, 2020

noahbrenner mentioned this pull request Jul 17, 2020

Deprecation warning since Node 14.5.0 gulpjs/gulp#2460

Closed

4 tasks

puzpuzpuz mentioned this pull request Jul 25, 2020

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

Merged

rochdev mentioned this pull request Feb 1, 2021

Scope manager and exporter integration tests DataDog/dd-trace-js#1224

Closed

137717unity mentioned this pull request May 22, 2023

[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 3.15.0 137717unity/node#95

Open

snyk-bot mentioned this pull request May 23, 2023

[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 3.15.0 ron-a20/node#95

Open

wedataintelligence mentioned this pull request May 24, 2023

[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 3.15.0 wedataintelligence/node#77

Open

baby636 mentioned this pull request May 24, 2023

[Snyk] Security upgrade gatsby-plugin-sharp from 2.6.24 to 3.15.0 baby636/node#92

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 Nov 28, 2023

[Snyk] Fix for 37 vulnerabilities baby636/node#110

Open

ron-a20 mentioned this pull request Dec 19, 2023

[Snyk] Fix for 30 vulnerabilities ron-a20/node#112

Open

137717unity mentioned this pull request Dec 19, 2023

[Snyk] Fix for 30 vulnerabilities 137717unity/node#113

Open

wedataintelligence mentioned this pull request Dec 19, 2023

[Snyk] Fix for 30 vulnerabilities wedataintelligence/node#96

Open

aliceUnhinged613 mentioned this pull request Dec 19, 2023

[Snyk] Fix for 30 vulnerabilities aliceUnhinged613/node#114

Open

baby636 mentioned this pull request Dec 20, 2023

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

Open

137717unity mentioned this pull request Apr 14, 2024

[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 3.15.0 137717unity/node#125

Open

aliceUnhinged613 mentioned this pull request Apr 14, 2024

[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 3.15.0 aliceUnhinged613/node#125

Open

ron-a20 mentioned this pull request Apr 14, 2024

[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 3.15.0 ron-a20/node#123

Open

molllyn1 mentioned this pull request Apr 14, 2024

[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 3.15.0 molllyn1/node#109

Open

kissofire mentioned this pull request Apr 14, 2024

[Snyk] Security upgrade gatsby-plugin-sharp from 2.2.13 to 3.15.0 kissofire/node#126

Open

wedataintelligence mentioned this pull request Apr 14, 2024

[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 3.15.0 wedataintelligence/node#108

Open

baby636 mentioned this pull request Apr 15, 2024

[Snyk] Security upgrade gatsby-plugin-sharp from 2.6.24 to 3.15.0 baby636/node#146

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

async_hooks: callback trampoline for MakeCallback#33801

async_hooks: callback trampoline for MakeCallback#33801
Qard wants to merge 2 commits into
nodejs:masterfrom
Qard:async-hooks-callback-trampoline

Qard commented Jun 9, 2020

Uh oh!

Uh oh!

addaleax left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Qard commented Jun 9, 2020

Uh oh!

Uh oh!

Qard commented Jun 10, 2020

Uh oh!

Uh oh!

addaleax commented Jun 11, 2020

Uh oh!

puzpuzpuz commented Jun 11, 2020

Uh oh!

Qard commented Jun 11, 2020

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

Qard commented Jun 9, 2020

Checklist

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Qard commented Jun 9, 2020

Uh oh!

Uh oh!

Qard commented Jun 10, 2020

Uh oh!

Uh oh!

addaleax commented Jun 11, 2020

Uh oh!

puzpuzpuz commented Jun 11, 2020

Uh oh!

Qard commented Jun 11, 2020

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants