Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

vm: refactor to avoid unsafe array iteration #36752

Merged
merged 1 commit into from
Jan 13, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 3, 2021

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@aduh95 aduh95 added vm Issues and PRs related to the vm subsystem. needs-benchmark-ci PR that need a benchmark CI run. labels Jan 3, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 3, 2021

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

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 3, 2021

No perf regression or improvement

                                                                    confidence improvement accuracy (*)    (**)   (***)
 vm/create-context.js n=100                                                        -1.06 %       ±8.00% ±10.65% ±13.88%
 vm/run-in-context.js withSigintListener=0 breakOnSigint=0 n=1                     -1.67 %       ±7.09%  ±9.43% ±12.27%
 vm/run-in-context.js withSigintListener=0 breakOnSigint=1 n=1                      2.73 %       ±7.58% ±10.11% ±13.21%
 vm/run-in-context.js withSigintListener=1 breakOnSigint=0 n=1                      1.23 %       ±6.48%  ±8.65% ±11.31%
 vm/run-in-context.js withSigintListener=1 breakOnSigint=1 n=1                     -2.97 %       ±4.42%  ±5.89%  ±7.66%
 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=0 n=1                -2.35 %       ±5.64%  ±7.50%  ±9.77%
 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=1 n=1                 5.30 %       ±6.35%  ±8.48% ±11.10%
 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=0 n=1                 1.00 %       ±5.77%  ±7.69% ±10.03%
 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=1 n=1                -3.33 %       ±4.76%  ±6.33%  ±8.24%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 9 comparisons, you can thus
expect the following amount of false-positive results:
  0.45 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.09 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)
++ mv output030121-121422.csv /w/bnch-comp

@aduh95 aduh95 added review wanted PRs that need reviews. and removed needs-benchmark-ci PR that need a benchmark CI run. labels Jan 3, 2021
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 12, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2021
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

PR-URL: nodejs#36752
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95 aduh95 force-pushed the vm-array-iteration branch from 9b615d8 to e70e793 Compare January 13, 2021 14:49
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 13, 2021

Landed in e70e793

@aduh95 aduh95 merged commit e70e793 into nodejs:master Jan 13, 2021
@aduh95 aduh95 deleted the vm-array-iteration branch January 13, 2021 14:50
ruyadorno pushed a commit that referenced this pull request Jan 22, 2021
PR-URL: #36752
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Jan 22, 2021
targos pushed a commit that referenced this pull request May 25, 2021
PR-URL: #36752
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 30, 2021
PR-URL: #36752
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
PR-URL: #36752
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jun 11, 2021
PR-URL: #36752
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants