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.

Sign up for GitHub

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

Jump to bottom

async_hooks: update defaultTriggerAsyncIdScope for perf #18004

Closed
wants to merge 1 commit into from
Closed

async_hooks: update defaultTriggerAsyncIdScope for perf #18004

wants to merge 1 commit into from

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Jan 5, 2018 •
edited

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 •
edited

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 •
edited

^ 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

AndreasMadsen
AndreasMadsen approved these changes Jan 5, 2018
View reviewed changes
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
@apapirovski
async_hooks: update defaultTriggerAsyncIdScope for perf
fbad8bb 
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@apapirovski apapirovski mentioned this pull request Jan 9, 2018
Closed
3 tasks
@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.

@AndreasMadsen AndreasMadsen mentioned this pull request Jan 10, 2018
Closed
4 tasks
@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
This was referenced Jan 16, 2018
Closed
Closed
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jan 17, 2018
@apapirovski @gibfahn
async_hooks: update defaultTriggerAsyncIdScope for perf
a8b7e43 
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2018
@apapirovski @MylesBorins
async_hooks: update defaultTriggerAsyncIdScope for perf
5dab90b 
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@AndreasMadsen
Copy link
Member

Backported (nothing changed): #18143

@AndreasMadsen AndreasMadsen mentioned this pull request Jan 31, 2018
Closed
3 tasks
evanlucas pushed a commit that referenced this pull request Jan 31, 2018
@apapirovski @evanlucas
async_hooks: update defaultTriggerAsyncIdScope for perf
0a68018 
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
gibfahn added a commit that referenced this pull request Mar 6, 2018
@gibfahn
2018-03-06 Version 8.10.0 'Carbon' (LTS)
2fce636 
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
@gibfahn
2018-03-06 Version 8.10.0 'Carbon' (LTS)
fc9ddd9 
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
@gibfahn @MayaLekova
2018-03-06 Version 8.10.0 'Carbon' (LTS)
185ac06 
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
Reviewers

@jasnell jasnell jasnell approved these changes

@AndreasMadsen AndreasMadsen AndreasMadsen approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet