FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

async_hooks: update defaultTriggerAsyncIdScope for perf by apapirovski · Pull Request #18004 · nodejs/node · GitHub

/ node Public

async_hooks: update defaultTriggerAsyncIdScope for perf - #18004

Closed
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:patch-default-trigger-async-id-scope-perf
ConversationCommits (1)ChecksFiles changed
Closed

async_hooks: update defaultTriggerAsyncIdScope for perf#18004
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:patch-default-trigger-async-id-scope-perf

Conversation

apapirovski commented Jan 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 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 commented Jan 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Jan 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

^ 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 approved these changes Jan 5, 2018
View reviewed changes

AndreasMadsen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

AndreasMadsen commented Jan 5, 2018

Copy link
Copy Markdown
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 added dont-land-on-v4.x author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Jan 8, 2018

apapirovski commented Jan 9, 2018

Copy link
Copy Markdown
Contributor Author

Landed in fbad8bb

apapirovski closed this Jan 9, 2018
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
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 <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>
apapirovski mentioned this pull request Jan 9, 2018
Closed
3 tasks

MylesBorins commented Jan 9, 2018

Copy link
Copy Markdown
Contributor

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

AndreasMadsen commented Jan 10, 2018

Copy link
Copy Markdown
Member

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

AndreasMadsen mentioned this pull request Jan 10, 2018
Closed
4 tasks
TimothyGu removed the author ready PRs that have at least one approval, no outstanding review comments, 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
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 <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
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 <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 commented Jan 30, 2018

Copy link
Copy Markdown
Member

Backported (nothing changed): #18143

AndreasMadsen mentioned this pull request Jan 31, 2018
Closed
3 tasks
evanlucas pushed a commit that referenced this pull request Jan 31, 2018
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 <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
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
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
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
+1 more reviewer
AndreasMadsen AndreasMadsen approved these changes
Reviewers whose approvals may not affect merge requirements

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.

Back | FazBrowse Home | New Git URL