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

fs: refactor to avoid unsafe array iteration #36699

Merged
merged 1 commit into from
Jan 6, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Dec 30, 2020

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

@aduh95 aduh95 added fs Issues and PRs related to the fs subsystem / file system. needs-benchmark-ci PR that need a benchmark CI run. labels Dec 30, 2020
@PoojaDurgad PoojaDurgad added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 2, 2021

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

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 2, 2021

Benchmark didn't show any significant regression except maybe one:

 fs/bench-stat-promise.js statType='fstat' n=200000                                                    **     -5.18 %       ±3.33%  ±4.43%  ±5.76%

If no one objects, I'd say that's good enough.

                                                                                               confidence improvement accuracy (*)    (**)   (***)
 fs/bench-mkdirp.js n=10000                                                                                   -0.78 %       ±3.46%  ±4.61%  ±6.01%
 fs/bench-opendir.js bufferSize=1024 mode='async' dir='lib' n=100                                       *      9.79 %       ±8.13% ±10.85% ±14.19%
 fs/bench-opendir.js bufferSize=1024 mode='async' dir='test/parallel' n=100                                    4.69 %       ±5.73%  ±7.63%  ±9.95%
 fs/bench-opendir.js bufferSize=1024 mode='callback' dir='lib' n=100                                          -2.77 %       ±6.49%  ±8.65% ±11.29%
 fs/bench-opendir.js bufferSize=1024 mode='callback' dir='test/parallel' n=100                                 1.36 %       ±4.16%  ±5.55%  ±7.23%
 fs/bench-opendir.js bufferSize=1024 mode='sync' dir='lib' n=100                                        *     11.60 %      ±10.36% ±13.78% ±17.94%
 fs/bench-opendir.js bufferSize=1024 mode='sync' dir='test/parallel' n=100                                     1.26 %       ±3.16%  ±4.21%  ±5.48%
 fs/bench-opendir.js bufferSize=32 mode='async' dir='lib' n=100                                               -2.26 %       ±5.21%  ±6.93%  ±9.02%
 fs/bench-opendir.js bufferSize=32 mode='async' dir='test/parallel' n=100                                     -1.23 %       ±4.98%  ±6.62%  ±8.62%
 fs/bench-opendir.js bufferSize=32 mode='callback' dir='lib' n=100                                            -0.92 %       ±7.47%  ±9.94% ±12.93%
 fs/bench-opendir.js bufferSize=32 mode='callback' dir='test/parallel' n=100                                   4.00 %       ±6.73%  ±8.97% ±11.71%
 fs/bench-opendir.js bufferSize=32 mode='sync' dir='lib' n=100                                                 6.73 %       ±7.86% ±10.46% ±13.62%
 fs/bench-opendir.js bufferSize=32 mode='sync' dir='test/parallel' n=100                                *      1.73 %       ±1.63%  ±2.17%  ±2.83%
 fs/bench-opendir.js bufferSize=4 mode='async' dir='lib' n=100                                          *     -7.30 %       ±7.05%  ±9.40% ±12.28%
 fs/bench-opendir.js bufferSize=4 mode='async' dir='test/parallel' n=100                                      -0.43 %       ±3.38%  ±4.50%  ±5.86%
 fs/bench-opendir.js bufferSize=4 mode='callback' dir='lib' n=100                                              1.32 %       ±6.82%  ±9.08% ±11.82%
 fs/bench-opendir.js bufferSize=4 mode='callback' dir='test/parallel' n=100                                    1.87 %       ±3.87%  ±5.17%  ±6.75%
 fs/bench-opendir.js bufferSize=4 mode='sync' dir='lib' n=100                                                 -1.21 %       ±7.08%  ±9.43% ±12.30%
 fs/bench-opendir.js bufferSize=4 mode='sync' dir='test/parallel' n=100                                 *      1.61 %       ±1.32%  ±1.76%  ±2.30%
 fs/bench-readdir.js withFileTypes='false' dir='lib' n=10                                                     -0.09 %       ±4.04%  ±5.37%  ±6.99%
 fs/bench-readdir.js withFileTypes='false' dir='test/parallel' n=10                                           -2.44 %       ±3.07%  ±4.10%  ±5.36%
 fs/bench-readdir.js withFileTypes='true' dir='lib' n=10                                                       0.33 %       ±5.84%  ±7.81% ±10.24%
 fs/bench-readdir.js withFileTypes='true' dir='test/parallel' n=10                                             0.36 %       ±2.27%  ±3.02%  ±3.93%
 fs/bench-readdirSync.js withFileTypes='false' dir='lib' n=10                                                 -2.18 %       ±6.60%  ±8.79% ±11.44%
 fs/bench-readdirSync.js withFileTypes='false' dir='test/parallel' n=10                                        1.67 %      ±10.07% ±13.49% ±17.74%
 fs/bench-readdirSync.js withFileTypes='true' dir='lib' n=10                                                   1.39 %       ±4.25%  ±5.65%  ±7.35%
 fs/bench-readdirSync.js withFileTypes='true' dir='test/parallel' n=10                                        -3.21 %       ±5.95%  ±7.92% ±10.30%
 fs/bench-realpath.js pathType='relative' n=10000                                                              1.97 %       ±4.24%  ±5.65%  ±7.37%
 fs/bench-realpath.js pathType='resolved' n=10000                                                              2.67 %       ±3.92%  ±5.22%  ±6.79%
 fs/bench-realpathSync.js pathType='relative' n=10000                                                          1.31 %       ±2.81%  ±3.74%  ±4.88%
 fs/bench-realpathSync.js pathType='resolved' n=10000                                                         -2.59 %       ±2.63%  ±3.50%  ±4.56%
 fs/bench-stat.js statType='fstat' n=200000                                                                   -0.83 %       ±4.48%  ±5.96%  ±7.76%
 fs/bench-stat.js statType='lstat' n=200000                                                                   -1.70 %       ±3.93%  ±5.23%  ±6.82%
 fs/bench-stat.js statType='stat' n=200000                                                                     0.09 %       ±4.36%  ±5.80%  ±7.55%
 fs/bench-stat-promise.js statType='fstat' n=200000                                                    **     -5.18 %       ±3.33%  ±4.43%  ±5.76%
 fs/bench-stat-promise.js statType='lstat' n=200000                                                           -0.64 %       ±2.82%  ±3.76%  ±4.90%
 fs/bench-stat-promise.js statType='stat' n=200000                                                            -1.64 %       ±2.71%  ±3.60%  ±4.69%
 fs/bench-statSync-failure.js statSyncType='noThrow' n=1000000                                                -0.10 %       ±3.54%  ±4.71%  ±6.13%
 fs/bench-statSync-failure.js statSyncType='throw' n=1000000                                                  -1.34 %       ±1.92%  ±2.56%  ±3.33%
 fs/bench-statSync.js statSyncType='fstatSync' n=1000000                                                       2.14 %       ±2.56%  ±3.41%  ±4.45%
 fs/bench-statSync.js statSyncType='lstatSync' n=1000000                                                       0.33 %       ±1.89%  ±2.52%  ±3.28%
 fs/bench-statSync.js statSyncType='statSync' n=1000000                                                        0.36 %       ±2.18%  ±2.90%  ±3.78%
 fs/readfile.js concurrent=10 len=1024 duration=5                                                              1.06 %       ±6.91%  ±9.19% ±11.97%
 fs/readfile.js concurrent=10 len=16777216 duration=5                                                         -1.03 %       ±4.38%  ±5.83%  ±7.58%
 fs/readfile.js concurrent=1 len=1024 duration=5                                                               0.81 %       ±2.75%  ±3.66%  ±4.76%
 fs/readfile.js concurrent=1 len=16777216 duration=5                                                           1.80 %      ±15.26% ±20.31% ±26.44%
 fs/readfile-partitioned.js concurrent=10 len=1024 dur=5                                                       1.17 %       ±8.31% ±11.06% ±14.39%
 fs/readfile-partitioned.js concurrent=10 len=16777216 dur=5                                                   0.37 %       ±5.36%  ±7.14%  ±9.30%
 fs/readfile-partitioned.js concurrent=1 len=1024 dur=5                                                        4.98 %       ±7.61% ±10.13% ±13.21%
 fs/readfile-partitioned.js concurrent=1 len=16777216 dur=5                                                   -0.14 %       ±3.41%  ±4.53%  ±5.91%
 fs/readFileSync.js n=600000                                                                                   2.55 %       ±6.87%  ±9.14% ±11.91%
 fs/read-stream-throughput.js n=1024 highWaterMark=1024 filesize=1024000 encodingType='asc'                   -0.68 %       ±3.31%  ±4.41%  ±5.74%
 fs/read-stream-throughput.js n=1024 highWaterMark=1024 filesize=1024000 encodingType='buf'                    0.80 %       ±3.21%  ±4.27%  ±5.58%
 fs/read-stream-throughput.js n=1024 highWaterMark=1024 filesize=1024000 encodingType='utf'                   -1.51 %       ±2.12%  ±2.82%  ±3.67%
 fs/read-stream-throughput.js n=1024 highWaterMark=1048576 filesize=1024000 encodingType='asc'                -0.20 %       ±7.27%  ±9.68% ±12.60%
 fs/read-stream-throughput.js n=1024 highWaterMark=1048576 filesize=1024000 encodingType='buf'                 0.40 %      ±13.36% ±17.85% ±23.38%
 fs/read-stream-throughput.js n=1024 highWaterMark=1048576 filesize=1024000 encodingType='utf'                 3.89 %      ±17.63% ±23.46% ±30.54%
 fs/read-stream-throughput.js n=1024 highWaterMark=4096 filesize=1024000 encodingType='asc'                    0.78 %       ±3.08%  ±4.10%  ±5.33%
 fs/read-stream-throughput.js n=1024 highWaterMark=4096 filesize=1024000 encodingType='buf'                   -0.83 %       ±3.57%  ±4.75%  ±6.18%
 fs/read-stream-throughput.js n=1024 highWaterMark=4096 filesize=1024000 encodingType='utf'                   -0.47 %       ±8.42% ±11.21% ±14.59%
 fs/read-stream-throughput.js n=1024 highWaterMark=65535 filesize=1024000 encodingType='asc'                   4.95 %       ±9.17% ±12.20% ±15.89%
 fs/read-stream-throughput.js n=1024 highWaterMark=65535 filesize=1024000 encodingType='buf'                   4.57 %      ±14.26% ±18.98% ±24.72%
 fs/read-stream-throughput.js n=1024 highWaterMark=65535 filesize=1024000 encodingType='utf'                   0.42 %       ±3.99%  ±5.32%  ±6.94%
 fs/write-stream-throughput.js size=1024 encodingType='asc' dur=5                                              1.32 %       ±5.57%  ±7.41%  ±9.65%
 fs/write-stream-throughput.js size=1024 encodingType='buf' dur=5                                             -0.07 %       ±8.90% ±11.84% ±15.41%
 fs/write-stream-throughput.js size=1024 encodingType='utf' dur=5                                              4.33 %       ±6.12%  ±8.14% ±10.60%
 fs/write-stream-throughput.js size=1048576 encodingType='asc' dur=5                                          -0.63 %       ±4.50%  ±5.99%  ±7.80%
 fs/write-stream-throughput.js size=1048576 encodingType='buf' dur=5                                           3.35 %      ±10.59% ±14.10% ±18.35%
 fs/write-stream-throughput.js size=1048576 encodingType='utf' dur=5                                           1.70 %       ±3.27%  ±4.35%  ±5.67%
 fs/write-stream-throughput.js size=2 encodingType='asc' dur=5                                                 0.09 %      ±11.28% ±15.01% ±19.55%
 fs/write-stream-throughput.js size=2 encodingType='buf' dur=5                                                 7.06 %      ±11.23% ±14.95% ±19.46%
 fs/write-stream-throughput.js size=2 encodingType='utf' dur=5                                                -1.31 %      ±11.01% ±14.65% ±19.07%
 fs/write-stream-throughput.js size=65535 encodingType='asc' dur=5                                            -8.08 %      ±12.60% ±16.77% ±21.83%
 fs/write-stream-throughput.js size=65535 encodingType='buf' dur=5                                             0.04 %      ±11.74% ±15.62% ±20.34%
 fs/write-stream-throughput.js size=65535 encodingType='utf' dur=5                                            -1.51 %       ±9.06% ±12.07% ±15.74%

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

@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. and removed needs-benchmark-ci PR that need a benchmark CI run. labels Jan 2, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 5, 2021
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 5, 2021
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 5, 2021
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 5, 2021
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 6, 2021
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 6, 2021
PR-URL: nodejs#36699
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@aduh95 aduh95 force-pushed the fs-array-iteration branch from 71c4c20 to 9e1e890 Compare January 6, 2021 10:32
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 6, 2021

Landed in 9e1e890

@aduh95 aduh95 merged commit 9e1e890 into nodejs:master Jan 6, 2021
@aduh95 aduh95 deleted the fs-array-iteration branch January 6, 2021 10:32
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
PR-URL: #36699
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
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. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants