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

build: rework gyp files for zlib #45589

Closed
wants to merge 2 commits into from
Closed

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Nov 22, 2022

This is #33044 rebased and updated. It allows us to stop floating 26991f7.

Fixes: #32856

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib subsystem. labels Nov 22, 2022
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 22, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 22, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot

This comment was marked as outdated.

@mscdex
Copy link
Contributor

mscdex commented Nov 23, 2022

Something seems not right. This seems to consistently cause a ~23-26% performance regression with zlib/createInflate.js. Example zlib results:

zlib/createInflate.js n=100 chunkLen=1024 inputLen=16777216                   ***    -26.42 %       ±6.37%  ±8.48% ±11.04%
zlib/creation.js n=500000 options='false' type='BrotliCompress'                        1.05 %       ±4.04%  ±5.37%  ±7.00%
zlib/creation.js n=500000 options='false' type='BrotliDecompress'                      1.33 %       ±4.81%  ±6.39%  ±8.32%
zlib/creation.js n=500000 options='false' type='Deflate'                              -1.38 %       ±3.66%  ±4.88%  ±6.38%
zlib/creation.js n=500000 options='false' type='DeflateRaw'                           -0.66 %       ±3.63%  ±4.83%  ±6.28%
zlib/creation.js n=500000 options='false' type='Gunzip'                                0.59 %       ±3.51%  ±4.67%  ±6.08%
zlib/creation.js n=500000 options='false' type='Gzip'                                  0.74 %       ±4.61%  ±6.14%  ±8.00%
zlib/creation.js n=500000 options='false' type='Inflate'                              -0.76 %       ±3.93%  ±5.23%  ±6.81%
zlib/creation.js n=500000 options='false' type='InflateRaw'                           -2.86 %       ±3.99%  ±5.31%  ±6.91%
zlib/creation.js n=500000 options='false' type='Unzip'                                -0.45 %       ±4.21%  ±5.61%  ±7.30%
zlib/creation.js n=500000 options='true' type='BrotliCompress'                 **     -5.64 %       ±4.13%  ±5.50%  ±7.15%
zlib/creation.js n=500000 options='true' type='BrotliDecompress'                       1.55 %       ±3.93%  ±5.22%  ±6.80%
zlib/creation.js n=500000 options='true' type='Deflate'                                1.30 %       ±4.24%  ±5.64%  ±7.34%
zlib/creation.js n=500000 options='true' type='DeflateRaw'                             2.37 %       ±3.84%  ±5.11%  ±6.67%
zlib/creation.js n=500000 options='true' type='Gunzip'                                -0.43 %       ±4.58%  ±6.10%  ±7.93%
zlib/creation.js n=500000 options='true' type='Gzip'                                  -1.79 %       ±3.94%  ±5.26%  ±6.87%
zlib/creation.js n=500000 options='true' type='Inflate'                               -0.54 %       ±5.50%  ±7.32%  ±9.52%
zlib/creation.js n=500000 options='true' type='InflateRaw'                             0.02 %       ±3.85%  ±5.12%  ±6.67%
zlib/creation.js n=500000 options='true' type='Unzip'                                  0.66 %       ±3.86%  ±5.14%  ±6.71%
zlib/deflate.js n=400000 inputLen=1024 method='createDeflate'                          5.66 %      ±10.05% ±13.37% ±17.40%
zlib/deflate.js n=400000 inputLen=1024 method='deflate'                               -1.48 %       ±5.39%  ±7.17%  ±9.34%
zlib/deflate.js n=400000 inputLen=1024 method='deflateSync'                            9.48 %      ±19.18% ±25.51% ±33.21%
zlib/inflate.js n=400000 inputLen=1024 method='inflate'                               -1.64 %       ±3.74%  ±4.98%  ±6.48%
zlib/inflate.js n=400000 inputLen=1024 method='inflateSync'                    **     -8.61 %       ±5.63%  ±7.49%  ±9.75%
zlib/pipe.js algorithm='brotli' type='buffer' duration=5 inputLen=1024                 0.89 %       ±2.82%  ±3.76%  ±4.92%
zlib/pipe.js algorithm='brotli' type='string' duration=5 inputLen=1024                -0.05 %       ±1.97%  ±2.63%  ±3.44%
zlib/pipe.js algorithm='gzip' type='buffer' duration=5 inputLen=1024          ***     10.93 %       ±6.13%  ±8.21% ±10.79%
zlib/pipe.js algorithm='gzip' type='string' duration=5 inputLen=1024                   4.82 %       ±7.09%  ±9.43% ±12.28%

@richardlau
Copy link
Member

Something seems not right. This seems to consistently cause a ~23-26% performance regression with zlib/createInflate.js. Example zlib results:

Yeah, that's pretty much why #33044 got stuck.

@lpinca
Copy link
Member Author

lpinca commented Nov 23, 2022

A possible alternative is to propose dccdc51 upstream but I'm not sure if it will be accepted.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@lpinca
Copy link
Member Author

lpinca commented Nov 23, 2022

I see no performance regressions after the last commit

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1235/console

                                                                       confidence improvement accuracy (*)    (**)   (***)
zlib/createInflate.js n=100 chunkLen=1024 inputLen=16777216                            2.39 %       ±6.36%  ±8.47% ±11.02%
zlib/creation.js n=500000 options='false' type='BrotliCompress'                       -2.78 %       ±4.04%  ±5.38%  ±7.00%
zlib/creation.js n=500000 options='false' type='BrotliDecompress'                      2.62 %       ±3.99%  ±5.31%  ±6.92%
zlib/creation.js n=500000 options='false' type='Deflate'                               1.67 %       ±3.58%  ±4.77%  ±6.22%
zlib/creation.js n=500000 options='false' type='DeflateRaw'                           -0.37 %       ±3.74%  ±4.98%  ±6.49%
zlib/creation.js n=500000 options='false' type='Gunzip'                                2.78 %       ±4.33%  ±5.76%  ±7.50%
zlib/creation.js n=500000 options='false' type='Gzip'                                  0.68 %       ±3.56%  ±4.74%  ±6.17%
zlib/creation.js n=500000 options='false' type='Inflate'                               0.87 %       ±4.33%  ±5.77%  ±7.51%
zlib/creation.js n=500000 options='false' type='InflateRaw'                            2.31 %       ±3.68%  ±4.89%  ±6.37%
zlib/creation.js n=500000 options='false' type='Unzip'                                -1.62 %       ±3.81%  ±5.09%  ±6.65%
zlib/creation.js n=500000 options='true' type='BrotliCompress'                         1.83 %       ±5.21%  ±6.95%  ±9.07%
zlib/creation.js n=500000 options='true' type='BrotliDecompress'                      -2.27 %       ±4.28%  ±5.70%  ±7.42%
zlib/creation.js n=500000 options='true' type='Deflate'                                0.90 %       ±3.88%  ±5.16%  ±6.72%
zlib/creation.js n=500000 options='true' type='DeflateRaw'                            -1.35 %       ±3.78%  ±5.03%  ±6.55%
zlib/creation.js n=500000 options='true' type='Gunzip'                                 1.41 %       ±3.81%  ±5.07%  ±6.61%
zlib/creation.js n=500000 options='true' type='Gzip'                                  -0.01 %       ±3.35%  ±4.46%  ±5.81%
zlib/creation.js n=500000 options='true' type='Inflate'                               -1.99 %       ±3.64%  ±4.84%  ±6.31%
zlib/creation.js n=500000 options='true' type='InflateRaw'                             2.47 %       ±4.07%  ±5.43%  ±7.07%
zlib/creation.js n=500000 options='true' type='Unzip'                                  1.47 %       ±3.65%  ±4.86%  ±6.33%
zlib/deflate.js n=400000 inputLen=1024 method='createDeflate'                         -0.54 %      ±11.15% ±14.84% ±19.33%
zlib/deflate.js n=400000 inputLen=1024 method='deflate'                                2.14 %       ±6.05%  ±8.06% ±10.50%
zlib/deflate.js n=400000 inputLen=1024 method='deflateSync'                            3.71 %      ±18.09% ±24.07% ±31.33%
zlib/inflate.js n=400000 inputLen=1024 method='inflate'                               -1.45 %       ±3.46%  ±4.60%  ±5.99%
zlib/inflate.js n=400000 inputLen=1024 method='inflateSync'                           -0.71 %       ±5.50%  ±7.32%  ±9.53%
zlib/pipe.js algorithm='brotli' type='buffer' duration=5 inputLen=1024                -0.81 %       ±2.82%  ±3.75%  ±4.89%
zlib/pipe.js algorithm='brotli' type='string' duration=5 inputLen=1024          *      3.19 %       ±2.65%  ±3.53%  ±4.61%
zlib/pipe.js algorithm='gzip' type='buffer' duration=5 inputLen=1024                  -3.50 %       ±5.48%  ±7.34%  ±9.65%
zlib/pipe.js algorithm='gzip' type='string' duration=5 inputLen=1024                   1.89 %       ±5.89%  ±7.84% ±10.21%

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1236/console

                                                                       confidence improvement accuracy (*)    (**)   (***)
zlib/createInflate.js n=100 chunkLen=1024 inputLen=16777216                            2.11 %       ±7.19%  ±9.57% ±12.46%
zlib/creation.js n=500000 options='false' type='BrotliCompress'                 *      4.72 %       ±4.46%  ±5.93%  ±7.73%
zlib/creation.js n=500000 options='false' type='BrotliDecompress'               *      4.47 %       ±4.46%  ±5.93%  ±7.72%
zlib/creation.js n=500000 options='false' type='Deflate'                               0.03 %       ±3.62%  ±4.82%  ±6.28%
zlib/creation.js n=500000 options='false' type='DeflateRaw'                            0.49 %       ±4.94%  ±6.58%  ±8.56%
zlib/creation.js n=500000 options='false' type='Gunzip'                               -1.79 %       ±3.66%  ±4.88%  ±6.35%
zlib/creation.js n=500000 options='false' type='Gzip'                                  1.15 %       ±3.55%  ±4.72%  ±6.15%
zlib/creation.js n=500000 options='false' type='Inflate'                               2.92 %       ±3.40%  ±4.53%  ±5.90%
zlib/creation.js n=500000 options='false' type='InflateRaw'                           -0.07 %       ±3.30%  ±4.39%  ±5.73%
zlib/creation.js n=500000 options='false' type='Unzip'                                -3.96 %       ±4.35%  ±5.80%  ±7.58%
zlib/creation.js n=500000 options='true' type='BrotliCompress'                         0.78 %       ±3.94%  ±5.24%  ±6.84%
zlib/creation.js n=500000 options='true' type='BrotliDecompress'                       1.69 %       ±4.07%  ±5.42%  ±7.06%
zlib/creation.js n=500000 options='true' type='Deflate'                               -3.07 %       ±4.42%  ±5.88%  ±7.65%
zlib/creation.js n=500000 options='true' type='DeflateRaw'                             1.68 %       ±3.75%  ±5.00%  ±6.53%
zlib/creation.js n=500000 options='true' type='Gunzip'                                -1.02 %       ±4.31%  ±5.74%  ±7.47%
zlib/creation.js n=500000 options='true' type='Gzip'                                   2.04 %       ±3.27%  ±4.35%  ±5.66%
zlib/creation.js n=500000 options='true' type='Inflate'                                0.89 %       ±3.73%  ±4.96%  ±6.46%
zlib/creation.js n=500000 options='true' type='InflateRaw'                             0.66 %       ±4.42%  ±5.88%  ±7.66%
zlib/creation.js n=500000 options='true' type='Unzip'                                 -2.21 %       ±3.43%  ±4.57%  ±5.94%
zlib/deflate.js n=400000 inputLen=1024 method='createDeflate'                          8.23 %      ±10.88% ±14.48% ±18.86%
zlib/deflate.js n=400000 inputLen=1024 method='deflate'                               -3.03 %       ±6.48%  ±8.62% ±11.22%
zlib/deflate.js n=400000 inputLen=1024 method='deflateSync'                           11.92 %      ±19.99% ±26.60% ±34.62%
zlib/inflate.js n=400000 inputLen=1024 method='inflate'                         *     -3.83 %       ±3.43%  ±4.56%  ±5.95%
zlib/inflate.js n=400000 inputLen=1024 method='inflateSync'                     *      6.14 %       ±6.08%  ±8.09% ±10.54%
zlib/pipe.js algorithm='brotli' type='buffer' duration=5 inputLen=1024                 0.73 %       ±2.52%  ±3.35%  ±4.37%
zlib/pipe.js algorithm='brotli' type='string' duration=5 inputLen=1024                -1.47 %       ±2.28%  ±3.04%  ±3.97%
zlib/pipe.js algorithm='gzip' type='buffer' duration=5 inputLen=1024                  -1.18 %       ±4.99%  ±6.64%  ±8.65%
zlib/pipe.js algorithm='gzip' type='string' duration=5 inputLen=1024                   3.73 %       ±4.00%  ±5.32%  ±6.92%

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

lpinca and others added 2 commits November 25, 2022 14:53
Restructure the zlib.gyp file based on the upstream gn file, breaking
out the files with optimizations that need additional compiler flags.

Use a copy of the GN-scraper.py script to reduce the amount of hand
editing when the zlib dependency is updated.
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Dec 4, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 4, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 81fa992...7811d2d

nodejs-github-bot pushed a commit that referenced this pull request Dec 4, 2022
This reverts commit 26991f7.

PR-URL: #45589
Fixes: #32856
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
nodejs-github-bot pushed a commit that referenced this pull request Dec 4, 2022
Restructure the zlib.gyp file based on the upstream gn file, breaking
out the files with optimizations that need additional compiler flags.

Use a copy of the GN-scraper.py script to reduce the amount of hand
editing when the zlib dependency is updated.

PR-URL: #45589
Fixes: #32856
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@lpinca lpinca deleted the rework/zlib.gyp branch December 4, 2022 08:15
@lpinca lpinca mentioned this pull request Dec 4, 2022
3 tasks
@Neustradamus
Copy link

To follow this PR

targos pushed a commit that referenced this pull request Dec 12, 2022
This reverts commit 26991f7.

PR-URL: #45589
Fixes: #32856
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
targos pushed a commit that referenced this pull request Dec 12, 2022
Restructure the zlib.gyp file based on the upstream gn file, breaking
out the files with optimizations that need additional compiler flags.

Use a copy of the GN-scraper.py script to reduce the amount of hand
editing when the zlib dependency is updated.

PR-URL: #45589
Fixes: #32856
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
This reverts commit 26991f7.

PR-URL: #45589
Fixes: #32856
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
Restructure the zlib.gyp file based on the upstream gn file, breaking
out the files with optimizations that need additional compiler flags.

Use a copy of the GN-scraper.py script to reduce the amount of hand
editing when the zlib dependency is updated.

PR-URL: #45589
Fixes: #32856
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
This reverts commit 26991f7.

PR-URL: #45589
Fixes: #32856
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
Restructure the zlib.gyp file based on the upstream gn file, breaking
out the files with optimizations that need additional compiler flags.

Use a copy of the GN-scraper.py script to reduce the amount of hand
editing when the zlib dependency is updated.

PR-URL: #45589
Fixes: #32856
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
This reverts commit 26991f7.

PR-URL: #45589
Fixes: #32856
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
Restructure the zlib.gyp file based on the upstream gn file, breaking
out the files with optimizations that need additional compiler flags.

Use a copy of the GN-scraper.py script to reduce the amount of hand
editing when the zlib dependency is updated.

PR-URL: #45589
Fixes: #32856
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
This reverts commit 26991f7.

PR-URL: #45589
Fixes: #32856
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
Restructure the zlib.gyp file based on the upstream gn file, breaking
out the files with optimizations that need additional compiler flags.

Use a copy of the GN-scraper.py script to reduce the amount of hand
editing when the zlib dependency is updated.

PR-URL: #45589
Fixes: #32856
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
This reverts commit 26991f7.

PR-URL: #45589
Fixes: #32856
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
Restructure the zlib.gyp file based on the upstream gn file, breaking
out the files with optimizations that need additional compiler flags.

Use a copy of the GN-scraper.py script to reduce the amount of hand
editing when the zlib dependency is updated.

PR-URL: #45589
Fixes: #32856
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@nappy
Copy link
Contributor

nappy commented Feb 20, 2023

On Android arm64 builds of 18.14.2 the cpu-features.h header/library referenced by cpu_features.c cannot be found. I suspect due to these changes,

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. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. review wanted PRs that need reviews. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

maintaining-zlib.md is out of sync with chromium upstream
7 participants