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: update defaultTriggerAsyncIdScope for perf #18004

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Jan 5, 2018

The existing version of defaultTriggerAsyncIdScope creates an Array for the callback's arguments which is highly inefficient. Instead, use rest syntax and allow V8 to do that work for us. This yields roughly 2x performance for this particular function.

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

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, dgram, net

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. dgram Issues and PRs related to the dgram subsystem / UDP. net Issues and PRs related to the net subsystem. labels Jan 5, 2018
@AndreasMadsen
Copy link
Member

AndreasMadsen commented Jan 5, 2018

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

Not yet available, as 85 is currently running.

@apapirovski
Copy link
Member Author

apapirovski commented Jan 5, 2018

^ I doubt this will make a difference in that benchmark. It's a small speck within what it does. I just benchmarked the defaultTriggerAsyncIdScope function itself.

Here's an excerpt of those results:

old:
async_hooks/defaultTriggerAsyncIdScope.js n=1000000: 35,684,512.086736776
async_hooks/defaultTriggerAsyncIdScope.js n=1000000: 27,054,187.23743066
async_hooks/defaultTriggerAsyncIdScope.js n=1000000: 36,778,665.54937985
async_hooks/defaultTriggerAsyncIdScope.js n=1000000: 36,295,460.29086093
async_hooks/defaultTriggerAsyncIdScope.js n=1000000: 36,083,989.38235829

new:
async_hooks/defaultTriggerAsyncIdScope.js n=1000000: 55,556,571.00621451
async_hooks/defaultTriggerAsyncIdScope.js n=1000000: 57,059,087.8808095
async_hooks/defaultTriggerAsyncIdScope.js n=1000000: 57,721,190.48800958
async_hooks/defaultTriggerAsyncIdScope.js n=1000000: 55,131,991.7771737
async_hooks/defaultTriggerAsyncIdScope.js n=1000000: 72,751,584.98421836

Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

LGTM

@AndreasMadsen
Copy link
Member

@apapirovski you will be happy to know it is not a meaningless optimization:

                                                           improvement confidence      p.value
 dgram/single-buffer.js dur=5 type="recv" num=100 len=1         4.28 %        *** 0.0001523605
 dgram/single-buffer.js dur=5 type="recv" num=100 len=1024      2.58 %          * 0.0182186849
 dgram/single-buffer.js dur=5 type="recv" num=100 len=256      -0.84 %            0.4351849467
 dgram/single-buffer.js dur=5 type="recv" num=100 len=64        3.06 %         ** 0.0018264249
 dgram/single-buffer.js dur=5 type="send" num=100 len=1         3.75 %        *** 0.0002631968
 dgram/single-buffer.js dur=5 type="send" num=100 len=1024      1.69 %            0.1382915155
 dgram/single-buffer.js dur=5 type="send" num=100 len=256       2.72 %          * 0.0272523107
 dgram/single-buffer.js dur=5 type="send" num=100 len=64        1.55 %            0.0990570053

@AndreasMadsen AndreasMadsen added dont-land-on-v4.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 8, 2018
@apapirovski
Copy link
Member Author

Landed in fbad8bb

@apapirovski apapirovski closed this Jan 9, 2018
@apapirovski apapirovski deleted the patch-default-trigger-async-id-scope-perf branch January 9, 2018 15:51
apapirovski added a commit that referenced this pull request Jan 9, 2018
The existing version of defaultTriggerAsyncIdScope creates an Array
for the callback's arguments which is highly inefficient. Instead,
use rest syntax and allow V8 to do that work for us. This yields
roughly 2x performance for this particular function.

PR-URL: #18004
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

This doesn't land cleanly on v9.x, should it be backported?

@AndreasMadsen
Copy link
Member

@MylesBorins This depends on #17273. I've backported that in #18079. If you land #18079 first then this lands cleanly.

@TimothyGu TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2018
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jan 17, 2018
The existing version of defaultTriggerAsyncIdScope creates an Array
for the callback's arguments which is highly inefficient. Instead,
use rest syntax and allow V8 to do that work for us. This yields
roughly 2x performance for this particular function.

PR-URL: nodejs#18004
Backport-PR-URL: nodejs#18179
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2018
The existing version of defaultTriggerAsyncIdScope creates an Array
for the callback's arguments which is highly inefficient. Instead,
use rest syntax and allow V8 to do that work for us. This yields
roughly 2x performance for this particular function.

Backport-PR-URL: #18179
PR-URL: #18004
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@AndreasMadsen
Copy link
Member

Backported (nothing changed): #18143

evanlucas pushed a commit that referenced this pull request Jan 31, 2018
The existing version of defaultTriggerAsyncIdScope creates an Array
for the callback's arguments which is highly inefficient. Instead,
use rest syntax and allow V8 to do that work for us. This yields
roughly 2x performance for this particular function.

Backport-PR-URL: #18474
PR-URL: #18004
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
gibfahn added a commit that referenced this pull request Mar 6, 2018
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
gibfahn added a commit that referenced this pull request Mar 7, 2018
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
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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
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. dgram Issues and PRs related to the dgram subsystem / UDP. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.