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

zlib: refactor to avoid unsafe array iteration #36722

Merged
merged 1 commit into from
Jan 13, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 1, 2021

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

@nodejs-github-bot nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Jan 1, 2021
@aduh95 aduh95 added the needs-benchmark-ci PR that need a benchmark CI run. label Jan 1, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 2, 2021

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 2, 2021

Benchmark didn't show any significant regression.

                                                                        confidence improvement accuracy (*)    (**)   (***)
 zlib/createInflate.js n=100 chunkLen=1024 inputLen=16777216                            0.31 %       ±5.68%  ±7.56%  ±9.84%
 zlib/creation.js n=500000 options='false' type='BrotliCompress'                       -1.14 %       ±4.72%  ±6.28%  ±8.18%
 zlib/creation.js n=500000 options='false' type='BrotliDecompress'                      0.60 %       ±4.53%  ±6.03%  ±7.86%
 zlib/creation.js n=500000 options='false' type='Deflate'                       **      5.07 %       ±3.77%  ±5.01%  ±6.52%
 zlib/creation.js n=500000 options='false' type='DeflateRaw'                            3.22 %       ±4.26%  ±5.68%  ±7.41%
 zlib/creation.js n=500000 options='false' type='Gunzip'                                0.84 %       ±3.61%  ±4.80%  ±6.25%
 zlib/creation.js n=500000 options='false' type='Gzip'                                  1.22 %       ±4.25%  ±5.68%  ±7.43%
 zlib/creation.js n=500000 options='false' type='Inflate'                               1.38 %       ±4.14%  ±5.51%  ±7.18%
 zlib/creation.js n=500000 options='false' type='InflateRaw'                            2.72 %       ±3.12%  ±4.15%  ±5.41%
 zlib/creation.js n=500000 options='false' type='Unzip'                                 0.11 %       ±3.68%  ±4.90%  ±6.39%
 zlib/creation.js n=500000 options='true' type='BrotliCompress'                        -0.32 %       ±4.39%  ±5.84%  ±7.61%
 zlib/creation.js n=500000 options='true' type='BrotliDecompress'                       3.16 %       ±4.11%  ±5.46%  ±7.11%
 zlib/creation.js n=500000 options='true' type='Deflate'                               -1.45 %       ±3.39%  ±4.51%  ±5.87%
 zlib/creation.js n=500000 options='true' type='DeflateRaw'                             2.96 %       ±4.09%  ±5.45%  ±7.09%
 zlib/creation.js n=500000 options='true' type='Gunzip'                          *      3.67 %       ±3.21%  ±4.27%  ±5.56%
 zlib/creation.js n=500000 options='true' type='Gzip'                                   1.18 %       ±3.66%  ±4.88%  ±6.35%
 zlib/creation.js n=500000 options='true' type='Inflate'                               -0.70 %       ±4.25%  ±5.66%  ±7.37%
 zlib/creation.js n=500000 options='true' type='InflateRaw'                      *     -3.95 %       ±3.94%  ±5.25%  ±6.83%
 zlib/creation.js n=500000 options='true' type='Unzip'                                 -0.03 %       ±5.02%  ±6.68%  ±8.70%
 zlib/deflate.js n=400000 inputLen=1024 method='createDeflate'                         -2.77 %       ±9.03% ±12.02% ±15.65%
 zlib/deflate.js n=400000 inputLen=1024 method='deflate'                                4.60 %       ±4.99%  ±6.64%  ±8.66%
 zlib/deflate.js n=400000 inputLen=1024 method='deflateSync'                            4.71 %       ±9.58% ±12.74% ±16.59%
 zlib/inflate.js n=400000 inputLen=1024 method='inflate'                               -0.84 %       ±2.95%  ±3.93%  ±5.11%
 zlib/inflate.js n=400000 inputLen=1024 method='inflateSync'                            1.52 %       ±4.60%  ±6.12%  ±7.96%
 zlib/pipe.js algorithm='brotli' type='buffer' duration=5 inputLen=1024                -0.77 %       ±2.11%  ±2.80%  ±3.65%
 zlib/pipe.js algorithm='brotli' type='string' duration=5 inputLen=1024                -0.17 %       ±1.01%  ±1.34%  ±1.74%
 zlib/pipe.js algorithm='gzip' type='buffer' duration=5 inputLen=1024                  -4.83 %       ±6.40%  ±8.52% ±11.10%
 zlib/pipe.js algorithm='gzip' type='string' duration=5 inputLen=1024                  -0.63 %       ±4.49%  ±5.98%  ±7.78%

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

@aduh95 aduh95 added dont-land-on-v10.x review wanted PRs that need reviews. and removed needs-benchmark-ci PR that need a benchmark CI run. labels Jan 2, 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#36722
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Pooja D P <Pooja.D.P@ibm.com>
@aduh95 aduh95 force-pushed the zlib-array-iteration branch from b7f6ee1 to 251a0ff Compare January 13, 2021 14:52
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 13, 2021

Landed in 251a0ff

@aduh95 aduh95 merged commit 251a0ff into nodejs:master Jan 13, 2021
@aduh95 aduh95 deleted the zlib-array-iteration branch January 13, 2021 14:52
@ruyadorno ruyadorno removed the review wanted PRs that need reviews. label Jan 21, 2021
ruyadorno pushed a commit that referenced this pull request Jan 22, 2021
PR-URL: #36722
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Pooja D P <Pooja.D.P@ibm.com>
@ruyadorno ruyadorno mentioned this pull request Jan 22, 2021
targos pushed a commit that referenced this pull request May 25, 2021
PR-URL: #36722
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Pooja D P <Pooja.D.P@ibm.com>
targos pushed a commit that referenced this pull request May 30, 2021
PR-URL: #36722
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Pooja D P <Pooja.D.P@ibm.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
PR-URL: #36722
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Pooja D P <Pooja.D.P@ibm.com>
targos pushed a commit that referenced this pull request Jun 11, 2021
PR-URL: #36722
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Pooja D P <Pooja.D.P@ibm.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. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants