Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async_hooks: callback trampoline for MakeCallback #33801

Closed
wants to merge 2 commits into from

Conversation

Qard
Copy link
Member

@Qard Qard commented Jun 9, 2020

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

@Qard 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
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very nice so far! 🚀

lib/internal/async_hooks.js Outdated Show resolved Hide resolved
src/api/callback.cc Outdated Show resolved Hide resolved
src/api/callback.cc Outdated Show resolved Hide resolved
@Qard Qard force-pushed the async-hooks-callback-trampoline branch from 2096bab to 8942366 Compare June 9, 2020 17:25
@Qard
Copy link
Member Author

Qard commented Jun 9, 2020

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.

src/api/callback.cc Outdated Show resolved Hide resolved
@Qard
Copy link
Member Author

Qard commented Jun 10, 2020

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 (***)

src/env.h Show resolved Hide resolved
@Qard 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
With the async_hooks callback trampoline, domains no longer need any
native code. With this, domains can exist in pure JavaScript.
@Qard Qard force-pushed the async-hooks-callback-trampoline branch from 410acd0 to 27276c3 Compare June 10, 2020 23:26
@addaleax
Copy link
Member

@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
Copy link
Member

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 Qard marked this pull request as ready for review June 11, 2020 17:35
@Qard
Copy link
Member Author

Qard commented Jun 11, 2020

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.

@Qard 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
@nodejs-github-bot
Copy link
Collaborator

codebytere pushed a commit that referenced this pull request Jul 14, 2020
PR-URL: #33801
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
codebytere pushed a commit that referenced this pull request Jul 14, 2020
With the async_hooks callback trampoline, domains no longer need any
native code. With this, domains can exist in pure JavaScript.

PR-URL: #33801
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
codebytere pushed a commit that referenced this pull request Jul 14, 2020
646e5a4 changed the way that the domain hook callback
is called. Previously, the callback was only used in the case that
async_hooks were *not* being used (since domains already integrate
with async hooks the way they should), and the corresponding
deprecation warning also only emitted in that case.

However, that commit didn’t move that condition along when the code
was ported from C++ to JS. As a consequence, the domain hook callback
was used when it wasn’t necessary to use it, and the deprecation
warning emitted accidentally along with it.

Refs: 646e5a4#diff-9f21ce1b9d6d46fdd07b969e8a04e140L192
Refs: 646e5a4#diff-e6db408e12db906ead6ddfac3de15a6fR119
Refs: #33801 (comment)

PR-URL: #34245
Fixes: #34069
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
646e5a4 changed the way that the domain hook callback
is called. Previously, the callback was only used in the case that
async_hooks were *not* being used (since domains already integrate
with async hooks the way they should), and the corresponding
deprecation warning also only emitted in that case.

However, that commit didn’t move that condition along when the code
was ported from C++ to JS. As a consequence, the domain hook callback
was used when it wasn’t necessary to use it, and the deprecation
warning emitted accidentally along with it.

Refs: 646e5a4#diff-9f21ce1b9d6d46fdd07b969e8a04e140L192
Refs: 646e5a4#diff-e6db408e12db906ead6ddfac3de15a6fR119
Refs: #33801 (comment)

PR-URL: #34245
Fixes: #34069
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
codebytere pushed a commit to codebytere/node that referenced this pull request Jul 21, 2020
646e5a4 changed the way that the domain hook callback
is called. Previously, the callback was only used in the case that
async_hooks were *not* being used (since domains already integrate
with async hooks the way they should), and the corresponding
deprecation warning also only emitted in that case.

However, that commit didn’t move that condition along when the code
was ported from C++ to JS. As a consequence, the domain hook callback
was used when it wasn’t necessary to use it, and the deprecation
warning emitted accidentally along with it.

Refs: nodejs@646e5a4#diff-9f21ce1b9d6d46fdd07b969e8a04e140L192
Refs: nodejs@646e5a4#diff-e6db408e12db906ead6ddfac3de15a6fR119
Refs: nodejs#33801 (comment)

PR-URL: nodejs#34245
Fixes: nodejs#34069
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants