async_hooks: use typed array stack as fast path #17780

addaleax · 2017-12-20T06:16:25Z

Communicate the current async stack length through a
typed array field rather than a native binding method
Add a new fixed-size async_ids_fast_stack typed array
that contains the async ID stack up to a fixed limit.
This increases performance noticeably, since most of the time
the async ID stack will not be more than a handful of
levels deep.
Make the JS pushAsyncIds() and popAsyncIds() functions
do the same thing as the native ones if the fast path
is applicable.

Benchmarks:

$ ./node benchmark/compare.js --new ./node --old ./node-master --runs 10 --filter next-tick process | Rscript benchmark/compare.R
[00:03:25|% 100| 6/6 files | 20/20 runs | 1/1 configs]: Done
                                               improvement confidence      p.value
 process/next-tick-breadth-args.js millions=4     19.72 %        *** 3.013913e-06
 process/next-tick-breadth.js millions=4          27.33 %        *** 5.847983e-11
 process/next-tick-depth-args.js millions=12      40.08 %        *** 1.237127e-13
 process/next-tick-depth.js millions=12           77.27 %        *** 1.413290e-11
 process/next-tick-exec-args.js millions=5        13.58 %        *** 1.245180e-07
 process/next-tick-exec.js millions=5             16.80 %        *** 2.961386e-07

Checklist

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

Affected core subsystem(s)

async_hooks

- Communicate the current async stack length through a typed array field rather than a native binding method - Add a new fixed-size `async_ids_fast_stack` typed array that contains the async ID stack up to a fixed limit. This increases performance noticeably, since most of the time the async ID stack will not be more than a handful of levels deep. - Make the JS `pushAsyncIds()` and `popAsyncIds()` functions do the same thing as the native ones if the fast path is applicable. Benchmarks: $ ./node benchmark/compare.js --new ./node --old ./node-master --runs 10 --filter next-tick process | Rscript benchmark/compare.R [00:03:25|% 100| 6/6 files | 20/20 runs | 1/1 configs]: Done improvement confidence p.value process/next-tick-breadth-args.js millions=4 19.72 % *** 3.013913e-06 process/next-tick-breadth.js millions=4 27.33 % *** 5.847983e-11 process/next-tick-depth-args.js millions=12 40.08 % *** 1.237127e-13 process/next-tick-depth.js millions=12 77.27 % *** 1.413290e-11 process/next-tick-exec-args.js millions=5 13.58 % *** 1.245180e-07 process/next-tick-exec.js millions=5 16.80 % *** 2.961386e-07

AndreasMadsen · 2017-12-20T07:35:59Z

I think we should just remove the slow path. Having more than 32 nested before() emit seams absolutely ridiculous. If it turns out to be a problem we can consider alternatives.

addaleax · 2017-12-20T07:42:58Z

@AndreasMadsen I know that @trevnorris made the switch from using only a typed array to a std::stack on purpose…

I agree that the slow path is not likely to be taken but I don’t see why we shouldn’t support it just because of that

AndreasMadsen · 2017-12-20T07:52:14Z

I agree that the slow path is not likely to be taken but I don’t see why we shouldn’t support it just because of that

It is code complexity for an edge case that is unlikely to ever happen.

bnoordhuis · 2017-12-20T10:24:06Z

Possibly naive question: can't you grow the typed array when it's full? Seems simpler than a separate data structure for the fallback case.

addaleax · 2017-12-20T10:25:27Z

@bnoordhuis You mean, replace by a larger typed array? Yeah, I guess you could. :)

@AndreasMadsen Would you be okay with that approach?

AndreasMadsen · 2017-12-20T10:29:38Z

@addaleax Yes. I was thinking the same.

addaleax · 2017-12-21T19:45:29Z

@AndreasMadsen @bnoordhuis I’ve switched this to a resizable typed array … it doesn’t seem to affect the perf improvements here. :)

I’m totally fine with this approach, but I have to admit I don’t see a huge benefit over the previous code…

addaleax · 2017-12-21T20:08:08Z

CI: https://ci.nodejs.org/job/node-test-commit/15005/

bnoordhuis

Left some comments that you are welcome to ignore if I'm completely off-mark. :-)

bnoordhuis · 2017-12-21T20:26:36Z

src/env.h

    // Stores the ids of the current execution context stack.
-    std::stack<async_context> async_ids_stack_;


You can remove the #include <stack> now, I think?

bnoordhuis · 2017-12-21T20:26:36Z

lib/internal/async_hooks.js

+  async_hook_fields[kStackLength]++;
+  async_id_fields[kExecutionAsyncId] = asyncId;
+  async_id_fields[kTriggerAsyncId] = triggerAsyncId;
+}


I'd take a slightly different approach here. Growing the stack can be done in JS: allocate a new typed array, copy over the elements, then call into C++ to update the AliasedBuffer's reference. That probably also obviates the need to overload operator=(AliasedBuffer&&).

If you wrote it this way because of how PromiseHook() calls env->async_hooks()->push_async_ids(), perhaps you can work around that by ensuring there are always a few free slots? I think that should work although admittedly I didn't look too closely.

@bnoordhuis It’s not just PromiseHook(), MakeCallback() also does that – I think for your suggestion to work in a generic fashion you’d have to call that growing function from C++, which kind of just moves the problem around

bnoordhuis · 2017-12-21T20:26:36Z

lib/internal/async_hooks.js

+// This is the equivalent of the native push_async_ids() call.
+function pushAsyncIds(asyncId, triggerAsyncId) {
+  const stackLength = async_hook_fields[kStackLength];
+  if (stackLength >= async_hook_fields[kStackCapacity])


Is this needed? You can just check async_ids_stack.length, right?

… right, yes. :) Done!

addaleax · 2017-12-27T15:31:16Z

CI: https://ci.nodejs.org/job/node-test-commit/15087/

- Communicate the current async stack length through a typed array field rather than a native binding method - Add a new fixed-size `async_ids_fast_stack` typed array that contains the async ID stack up to a fixed limit. This increases performance noticeably, since most of the time the async ID stack will not be more than a handful of levels deep. - Make the JS `pushAsyncIds()` and `popAsyncIds()` functions do the same thing as the native ones if the fast path is applicable. Benchmarks: $ ./node benchmark/compare.js --new ./node --old ./node-master --runs 10 --filter next-tick process | Rscript benchmark/compare.R [00:03:25|% 100| 6/6 files | 20/20 runs | 1/1 configs]: Done improvement confidence p.value process/next-tick-breadth-args.js millions=4 19.72 % *** 3.013913e-06 process/next-tick-breadth.js millions=4 27.33 % *** 5.847983e-11 process/next-tick-depth-args.js millions=12 40.08 % *** 1.237127e-13 process/next-tick-depth.js millions=12 77.27 % *** 1.413290e-11 process/next-tick-exec-args.js millions=5 13.58 % *** 1.245180e-07 process/next-tick-exec.js millions=5 16.80 % *** 2.961386e-07 PR-URL: #17780 Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax · 2017-12-27T18:54:48Z

Landed in 83e5215

MylesBorins · 2018-01-09T23:38:34Z

This does not land cleanly on v9.x, could someone please manually backport

addaleax · 2018-01-10T16:01:03Z

This should apply cleanly or at least almost cleanly after #18079 lands

- Communicate the current async stack length through a typed array field rather than a native binding method - Add a new fixed-size `async_ids_fast_stack` typed array that contains the async ID stack up to a fixed limit. This increases performance noticeably, since most of the time the async ID stack will not be more than a handful of levels deep. - Make the JS `pushAsyncIds()` and `popAsyncIds()` functions do the same thing as the native ones if the fast path is applicable. Benchmarks: $ ./node benchmark/compare.js --new ./node --old ./node-master --runs 10 --filter next-tick process | Rscript benchmark/compare.R [00:03:25|% 100| 6/6 files | 20/20 runs | 1/1 configs]: Done improvement confidence p.value process/next-tick-breadth-args.js millions=4 19.72 % *** 3.013913e-06 process/next-tick-breadth.js millions=4 27.33 % *** 5.847983e-11 process/next-tick-depth-args.js millions=12 40.08 % *** 1.237127e-13 process/next-tick-depth.js millions=12 77.27 % *** 1.413290e-11 process/next-tick-exec-args.js millions=5 13.58 % *** 1.245180e-07 process/next-tick-exec.js millions=5 16.80 % *** 2.961386e-07 PR-URL: nodejs#17780 Backport-PR-URL: nodejs#18179 Reviewed-By: James M Snell <jasnell@gmail.com>

- Communicate the current async stack length through a typed array field rather than a native binding method - Add a new fixed-size `async_ids_fast_stack` typed array that contains the async ID stack up to a fixed limit. This increases performance noticeably, since most of the time the async ID stack will not be more than a handful of levels deep. - Make the JS `pushAsyncIds()` and `popAsyncIds()` functions do the same thing as the native ones if the fast path is applicable. Benchmarks: $ ./node benchmark/compare.js --new ./node --old ./node-master --runs 10 --filter next-tick process | Rscript benchmark/compare.R [00:03:25|% 100| 6/6 files | 20/20 runs | 1/1 configs]: Done improvement confidence p.value process/next-tick-breadth-args.js millions=4 19.72 % *** 3.013913e-06 process/next-tick-breadth.js millions=4 27.33 % *** 5.847983e-11 process/next-tick-depth-args.js millions=12 40.08 % *** 1.237127e-13 process/next-tick-depth.js millions=12 77.27 % *** 1.413290e-11 process/next-tick-exec-args.js millions=5 13.58 % *** 1.245180e-07 process/next-tick-exec.js millions=5 16.80 % *** 2.961386e-07 Backport-PR-URL: #18179 PR-URL: #17780 Reviewed-By: James M Snell <jasnell@gmail.com>

evanlucas · 2018-01-22T14:35:25Z

hm, it still isn't cherry-picking cleanly. Anyone have time to backport? Thanks!

- Communicate the current async stack length through a typed array field rather than a native binding method - Add a new fixed-size `async_ids_fast_stack` typed array that contains the async ID stack up to a fixed limit. This increases performance noticeably, since most of the time the async ID stack will not be more than a handful of levels deep. - Make the JS `pushAsyncIds()` and `popAsyncIds()` functions do the same thing as the native ones if the fast path is applicable. Benchmarks: $ ./node benchmark/compare.js --new ./node --old ./node-master --runs 10 --filter next-tick process | Rscript benchmark/compare.R [00:03:25|% 100| 6/6 files | 20/20 runs | 1/1 configs]: Done improvement confidence p.value process/next-tick-breadth-args.js millions=4 19.72 % *** 3.013913e-06 process/next-tick-breadth.js millions=4 27.33 % *** 5.847983e-11 process/next-tick-depth-args.js millions=12 40.08 % *** 1.237127e-13 process/next-tick-depth.js millions=12 77.27 % *** 1.413290e-11 process/next-tick-exec-args.js millions=5 13.58 % *** 1.245180e-07 process/next-tick-exec.js millions=5 16.80 % *** 2.961386e-07 PR-URL: nodejs#17780 Reviewed-By: James M Snell <jasnell@gmail.com>

- Communicate the current async stack length through a typed array field rather than a native binding method - Add a new fixed-size `async_ids_fast_stack` typed array that contains the async ID stack up to a fixed limit. This increases performance noticeably, since most of the time the async ID stack will not be more than a handful of levels deep. - Make the JS `pushAsyncIds()` and `popAsyncIds()` functions do the same thing as the native ones if the fast path is applicable. Benchmarks: $ ./node benchmark/compare.js --new ./node --old ./node-master --runs 10 --filter next-tick process | Rscript benchmark/compare.R [00:03:25|% 100| 6/6 files | 20/20 runs | 1/1 configs]: Done improvement confidence p.value process/next-tick-breadth-args.js millions=4 19.72 % *** 3.013913e-06 process/next-tick-breadth.js millions=4 27.33 % *** 5.847983e-11 process/next-tick-depth-args.js millions=12 40.08 % *** 1.237127e-13 process/next-tick-depth.js millions=12 77.27 % *** 1.413290e-11 process/next-tick-exec-args.js millions=5 13.58 % *** 1.245180e-07 process/next-tick-exec.js millions=5 16.80 % *** 2.961386e-07 Backport-PR-URL: #18290 PR-URL: #17780 Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [#16413](#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [#16413](#16413) * upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260) * re land npm 5.6.0 (Myles Borins) [#18625](#18625) * ICU 60 bump (Steven R. Loomis) [#16876](#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [#16130](#16130) * warn on invalid authentication tag length (Tobias Nießen) [#17566](#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [#18004](#18004) * use typed array stack as fast path (Anna Henningsen) [#17780](#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [#17273](#17273) * separate missing from default context (Andreas Madsen) [#17273](#17273) * rename initTriggerId (Andreas Madsen) [#17273](#17273) * deprecate undocumented API (Andreas Madsen) [#16972](#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [#16998](#16998) * add trace events to async_hooks (Andreas Madsen) [#15538](#15538) * set HTTPParser trigger to socket (Andreas Madsen) [#18003](#18003) * add provider types for net server (Andreas Madsen) [#17157](#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [#16495](#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [#17033](#17033) * module: * add builtinModules (Jon Moss) [#16386](#16386) * replace default paths in require.resolve() (cjihrig) [#17113](#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * add process.ppid (cjihrig) [#16839](#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [#16267](#16267) * add rawPacket in err of `clientError` event (XadillaX) [#17672](#17672) * better support for IPv6 addresses (Mattias Holmlund) [#14772](#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [#17662](#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [#18463](#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [#17478](#17478) * process: * improve unhandled rejection message (Madara Uchiha) [#17158](#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [#12860](#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [#17196](#17196) PR-URL: #18336

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [nodejs#16413](nodejs#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [nodejs#16413](nodejs#16413) * upgrade libuv to 1.19.1 (cjihrig) [nodejs#18260](nodejs#18260) * re land npm 5.6.0 (Myles Borins) [nodejs#18625](nodejs#18625) * ICU 60 bump (Steven R. Loomis) [nodejs#16876](nodejs#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [nodejs#16130](nodejs#16130) * warn on invalid authentication tag length (Tobias Nießen) [nodejs#17566](nodejs#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [nodejs#18004](nodejs#18004) * use typed array stack as fast path (Anna Henningsen) [nodejs#17780](nodejs#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [nodejs#17273](nodejs#17273) * separate missing from default context (Andreas Madsen) [nodejs#17273](nodejs#17273) * rename initTriggerId (Andreas Madsen) [nodejs#17273](nodejs#17273) * deprecate undocumented API (Andreas Madsen) [nodejs#16972](nodejs#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [nodejs#16998](nodejs#16998) * add trace events to async_hooks (Andreas Madsen) [nodejs#15538](nodejs#15538) * set HTTPParser trigger to socket (Andreas Madsen) [nodejs#18003](nodejs#18003) * add provider types for net server (Andreas Madsen) [nodejs#17157](nodejs#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [nodejs#16495](nodejs#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [nodejs#17033](nodejs#17033) * module: * add builtinModules (Jon Moss) [nodejs#16386](nodejs#16386) * replace default paths in require.resolve() (cjihrig) [nodejs#17113](nodejs#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * add process.ppid (cjihrig) [nodejs#16839](nodejs#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [nodejs#16267](nodejs#16267) * add rawPacket in err of `clientError` event (XadillaX) [nodejs#17672](nodejs#17672) * better support for IPv6 addresses (Mattias Holmlund) [nodejs#14772](nodejs#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [nodejs#17662](nodejs#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [nodejs#18463](nodejs#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [nodejs#17478](nodejs#17478) * process: * improve unhandled rejection message (Madara Uchiha) [nodejs#17158](nodejs#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [nodejs#12860](nodejs#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [nodejs#17196](nodejs#17196) PR-URL: nodejs#18336

lib/internal/async_hooks.js

src: use async_context instead of custom struct

d84f16e

nodejs-github-bot added c++ lib / src labels Dec 20, 2017

addaleax added the async_hooks label Dec 20, 2017

addaleax force-pushed the async-no-stack branch from 2ce172b to 6bed72a Compare Dec 20, 2017

addaleax added the performance label Dec 20, 2017

[squash] switch to resizable typed array

12c10d1

jasnell approved these changes Dec 21, 2017

View changes

bnoordhuis reviewed Dec 21, 2017

View changes

[squash] bnoordhuis comments

24c80e7

addaleax added the author ready label Dec 27, 2017

addaleax closed this Dec 27, 2017

addaleax deleted the async-no-stack branch Dec 27, 2017

addaleax removed the author ready label Dec 27, 2017

MylesBorins added the backport-requested-v9.x label Jan 9, 2018

AndreasMadsen added dont-land-on-v4.x labels Jan 10, 2018

AndreasMadsen added dont-land-on-v6.x and removed backport-requested-v6.x labels Jan 10, 2018

addaleax removed the backport-requested-v9.x label Jan 10, 2018

AndreasMadsen added the lts-watch-v8.x label Jan 15, 2018

AndreasMadsen mentioned this pull request Jan 16, 2018

Semver Minor Tracking nodejs/Release#298

Closed

AndreasMadsen mentioned this pull request Jan 16, 2018

Backport async hooks to v8.x #18179

Closed

4 tasks

evanlucas added the backport-requested-v9.x label Jan 22, 2018

addaleax mentioned this pull request Jan 22, 2018

[v9.x] async_hooks: use typed array stack as fast path #18290

Closed

evanlucas mentioned this pull request Jan 30, 2018

v9.5.0 release proposal #18464

Merged

addaleax added backported-to-v9.x and removed backport-requested-v9.x labels Feb 21, 2018

dko-slapdash reviewed Feb 16, 2019

View changes

lib/internal/async_hooks.js Show resolved Hide resolved

BethGriggs removed the lts-watch-v8.x label Sep 19, 2019

nodejs / node Public

async_hooks: use typed array stack as fast path #17780

async_hooks: use typed array stack as fast path #17780

addaleax commented Dec 20, 2017

AndreasMadsen commented Dec 20, 2017 •

edited

addaleax commented Dec 20, 2017

AndreasMadsen commented Dec 20, 2017

bnoordhuis commented Dec 20, 2017

addaleax commented Dec 20, 2017

AndreasMadsen commented Dec 20, 2017

addaleax commented Dec 21, 2017

addaleax commented Dec 21, 2017

bnoordhuis left a comment

bnoordhuis Dec 21, 2017

addaleax Dec 21, 2017

bnoordhuis Dec 21, 2017

addaleax Dec 21, 2017

bnoordhuis Dec 21, 2017

addaleax Dec 21, 2017

addaleax commented Dec 27, 2017

addaleax commented Dec 27, 2017

MylesBorins commented Jan 9, 2018

addaleax commented Jan 10, 2018

evanlucas commented Jan 22, 2018

		// Stores the ids of the current execution context stack.
		std::stack<async_context> async_ids_stack_;

nodejs / node Public

async_hooks: use typed array stack as fast path #17780

async_hooks: use typed array stack as fast path #17780

Conversation

addaleax commented Dec 20, 2017

Checklist

Affected core subsystem(s)

AndreasMadsen commented Dec 20, 2017 • edited

addaleax commented Dec 20, 2017

AndreasMadsen commented Dec 20, 2017

bnoordhuis commented Dec 20, 2017

addaleax commented Dec 20, 2017

AndreasMadsen commented Dec 20, 2017

addaleax commented Dec 21, 2017

addaleax commented Dec 21, 2017

bnoordhuis left a comment

bnoordhuis Dec 21, 2017

Choose a reason for hiding this comment

addaleax Dec 21, 2017

Choose a reason for hiding this comment

bnoordhuis Dec 21, 2017

Choose a reason for hiding this comment

addaleax Dec 21, 2017

Choose a reason for hiding this comment

bnoordhuis Dec 21, 2017

Choose a reason for hiding this comment

addaleax Dec 21, 2017

Choose a reason for hiding this comment

addaleax commented Dec 27, 2017

addaleax commented Dec 27, 2017

MylesBorins commented Jan 9, 2018

addaleax commented Jan 10, 2018

evanlucas commented Jan 22, 2018

AndreasMadsen commented Dec 20, 2017 •

edited