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: refactor to avoid unsafe array iteration #37125

Merged
merged 1 commit into from
Feb 1, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 29, 2021

No description provided.

@nodejs-github-bot nodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Jan 29, 2021
@TrySound
Copy link

What's wrong with modern syntax?

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 29, 2021

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/916/ (queued, will 404 until it starts).

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 29, 2021

What's wrong with modern syntax?

Nothing wrong with the syntax, the problem is with array iteration which relies on user-mutables methods:

delete Array.prototype[Symbol.iterator];
const [a, b, c] = [1,2,3];

@Lxxyx Lxxyx added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 29, 2021
@nodejs-github-bot
Copy link
Collaborator

@RaisinTen
Copy link
Contributor

@aduh95 The benchmark didn't run properly. Here is the new link:
https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/918/
Hope there are no regressions! 🤞

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 30, 2021

The benchmark stops working after two minutes… I'm not sure if it's a problem with our CI or the benchmark itself.

I'm splitting it into several runs to see if that helps:
https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/920/
https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/921/
https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/922/

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 31, 2021

No significant perf improvements or regressions.

                                                                            confidence improvement accuracy (*)    (**)   (***)
async_hooks/gc-tracking.jsmethod='trackingDisabled' n=1000000                              -4.16 %       ±8.67% ±11.59% ±15.19%
async_hooks/gc-tracking.jsmethod='trackingEnabled' n=1000000                               -1.90 %       ±5.38%  ±7.16%  ±9.32%
async_hooks/gc-tracking.jsmethod='trackingEnabledWithDestroyHook' n=1000000                 0.46 %       ±3.89%  ±5.17%  ±6.73%
                                                                                             confidence improvement accuracy (*)    (**)   (***)
async_hooks/http-server.jsduration=5 connections=50 asyncHooks='after' benchmarker='wrk'                    -1.80 %      ±13.73% ±24.39% ±51.16%
async_hooks/http-server.jsduration=5 connections=50 asyncHooks='all' benchmarker='wrk'                       0.12 %      ±14.36% ±23.32% ±42.11%
async_hooks/http-server.jsduration=5 connections=50 asyncHooks='before' benchmarker='wrk'                    1.73 %      ±12.36% ±19.18% ±32.09%
async_hooks/http-server.jsduration=5 connections=50 asyncHooks='disabled' benchmarker='wrk'                  7.56 %      ±20.85% ±36.49% ±74.63%
async_hooks/http-server.jsduration=5 connections=50 asyncHooks='init' benchmarker='wrk'                      0.33 %       ±8.25% ±12.79% ±21.38%
async_hooks/http-server.jsduration=5 connections=50 asyncHooks='none' benchmarker='wrk'                     -6.26 %      ±12.16% ±19.96% ±36.65%
async_hooks/http-server.jsduration=5 connections=500 asyncHooks='after' benchmarker='wrk'                    6.97 %      ±26.03% ±45.59% ±93.38%
async_hooks/http-server.jsduration=5 connections=500 asyncHooks='all' benchmarker='wrk'                      7.82 %      ±15.04% ±31.93% ±89.26%
async_hooks/http-server.jsduration=5 connections=500 asyncHooks='before' benchmarker='wrk'                  -2.09 %      ±16.85% ±25.64% ±41.50%
async_hooks/http-server.jsduration=5 connections=500 asyncHooks='disabled' benchmarker='wrk'                 7.77 %      ±17.60% ±28.56% ±51.49%
async_hooks/http-server.jsduration=5 connections=500 asyncHooks='init' benchmarker='wrk'                     0.82 %      ±13.66% ±20.69% ±33.26%
async_hooks/http-server.jsduration=5 connections=500 asyncHooks='none' benchmarker='wrk'                    -8.57 %      ±20.70% ±37.58% ±81.69%
                                                                  confidence improvement accuracy (*)   (**)  (***)
async_hooks/promises.jsasyncHooks='disabled' n=1000000                           -0.93 %       ±1.72% ±2.29% ±2.98%
async_hooks/promises.jsasyncHooks='enabled' n=1000000                            -1.14 %       ±1.67% ±2.22% ±2.89%
async_hooks/promises.jsasyncHooks='enabledWithDestroy' n=1000000                  0.85 %       ±2.15% ±2.87% ±3.75%
async_hooks/promises.jsasyncHooks='enabledWithInitOnly' n=1000000                -0.40 %       ±1.67% ±2.23% ±2.91%

PR-URL: nodejs#37125
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@aduh95 aduh95 force-pushed the async_hooks-array-iteration branch from 53c3c19 to f8853dd Compare February 1, 2021 15:16
@aduh95
Copy link
Contributor Author

aduh95 commented Feb 1, 2021

Landed in f8853dd

@aduh95 aduh95 merged commit f8853dd into nodejs:master Feb 1, 2021
@aduh95 aduh95 deleted the async_hooks-array-iteration branch February 1, 2021 15:17
targos pushed a commit that referenced this pull request Feb 2, 2021
PR-URL: #37125
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@targos targos mentioned this pull request Feb 2, 2021
targos pushed a commit that referenced this pull request May 25, 2021
PR-URL: #37125
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request May 30, 2021
PR-URL: #37125
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
PR-URL: #37125
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request Jun 11, 2021
PR-URL: #37125
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants