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

lib: refactor to avoid unsafe array iteration #37029

Closed
wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 23, 2021

No description provided.

@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Jan 23, 2021
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM. Does this need a benchmark?

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 24, 2021

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

EDIT: Didn't show any significant improvement or regression.

                                                      confidence improvement accuracy (*)    (**)   (***)
 process/bench-env.js operation='delete' n=1000000                   -0.29 %       ±5.56%  ±7.40%  ±9.63%
 process/bench-env.js operation='enumerate' n=1000000                -1.52 %       ±1.56%  ±2.07%  ±2.70%
 process/bench-env.js operation='get' n=1000000                      -1.75 %       ±3.90%  ±5.19%  ±6.76%
 process/bench-env.js operation='query' n=1000000                     1.02 %       ±2.91%  ±3.91%  ±5.15%
 process/bench-env.js operation='set' n=1000000                      -3.65 %       ±5.15%  ±6.86%  ±8.96%
 process/bench-hrtime.js type='bigint' n=1000000                      0.25 %       ±3.85%  ±5.12%  ±6.67%
 process/bench-hrtime.js type='diff' n=1000000                        2.55 %       ±3.67%  ±4.90%  ±6.41%
 process/bench-hrtime.js type='raw' n=1000000                        -2.05 %       ±3.28%  ±4.37%  ±5.72%
 process/memoryUsage.js n=100000                                     -0.10 %       ±1.79%  ±2.38%  ±3.11%
 process/next-tick-breadth-args.js n=10000000                        -0.06 %       ±3.86%  ±5.14%  ±6.69%
 process/next-tick-breadth.js n=10000000                             -2.16 %       ±3.73%  ±4.96%  ±6.46%
 process/next-tick-depth-args.js n=7000000                           -0.34 %       ±2.68%  ±3.58%  ±4.69%
 process/next-tick-depth.js n=7000000                                 0.91 %       ±2.03%  ±2.70%  ±3.53%
 process/next-tick-exec-args.js n=4000000                            -5.10 %      ±14.18% ±18.88% ±24.58%
 process/next-tick-exec.js n=4000000                                 -5.23 %       ±9.37% ±12.50% ±16.34%
 process/queue-microtask-breadth.js n=400000                          6.51 %       ±7.38%  ±9.82% ±12.80%
 process/queue-microtask-depth.js n=1200000                          -0.37 %       ±1.84%  ±2.44%  ±3.18%
 process/resourceUsage.js n=100000                                   -0.60 %       ±3.01%  ±4.00%  ±5.21%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 18 comparisons, you can thus
expect the following amount of false-positive results:
  0.90 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.18 false positives, when considering a   1% risk acceptance (**, ***),
  0.02 false positives, when considering a 0.1% risk acceptance (***)

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 24, 2021
@RaisinTen
Copy link
Contributor

Since buildAllowedFlags is only used here:

get() {
const flags = perThreadSetup.buildAllowedFlags();
process.allowedNodeEnvironmentFlags = flags;
return process.allowedNodeEnvironmentFlags;
},

and grep allowedNodeEnvironmentFlags benchmark/process/* gives nothing, I think the benchmark didn't run the new code.

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 26, 2021

RaisinTen pushed a commit that referenced this pull request Jan 27, 2021
PR-URL: #37029
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
@RaisinTen
Copy link
Contributor

Landed in 2e1e02a

@RaisinTen RaisinTen closed this Jan 27, 2021
@aduh95 aduh95 deleted the per_thread-array-iteration branch January 27, 2021 10:24
targos pushed a commit that referenced this pull request Feb 2, 2021
PR-URL: #37029
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
@targos targos mentioned this pull request Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants