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

deps: update zlib to upstream 5edb52d4 #47151

Closed
wants to merge 2 commits into from
Closed

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Mar 18, 2023

Updated as described in doc/contributing/maintaining-zlib.md.

Refs: #45387 (comment)

@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 Mar 18, 2023
kvakil added a commit to kvakil/node that referenced this pull request Mar 19, 2023
We currently have two copies of Chromium's zlib: one in deps/zlib and
another in deps/v8/third_party/zlib. This has a couple of disadvantages:

1. There is an additional cost to keeping both dependencies up-to-date,
    and in fact they were already out-of-sync (see the refs).
2. People who compile with --shared-zlib (i.e. distro maintainers) will
   probably not be thrilled to learn that there is still a copy of zlib
   inside.
3. It's aesthetically unpleasing.

Centralize on the version in V8 rather than the one in deps, and delete
the one in deps. Basically I just copied deps/zlib/zlib.gyp to
tools/v8_gypfiles/zlib.gyp, since the former had a better build
configuration (see the refs). This approach seemed better than the
opposite approach of centralizing on deps/zlib because:

1. We would need to occasionally bump deps/zlib manually after bumping
   deps/v8, which seemed annoying.
2. The maintenance steps for bumping zlib seemed more onerous than the
   maintenance steps for bumping V8.

(If we would prefer the opposite approach, I actually have another patch
 locally.)

One discrepancy was that V8's version of zlib had all symbols to be
prefixed with `Cr_z_` per deps/v8/third_party/zlib/chromeconf.h, which
seemed undesirable, so I added define `CHROMIUM_ZLIB_NO_CHROMECONF`
to the build to remove it. (deps/zlib handled this by just commenting
out the relevant include, but floating a patch seemed less desirable.)

I tested this on Linux with the default build and a --shared-zlib build.
I checked that the shared-zlib build correctly linked zlib according to
ldd. I would appreciate if the reviewers could suggest some other build
configurations to try.

Refs: nodejs#47145
Refs: nodejs#47151
@kvakil kvakil mentioned this pull request Mar 19, 2023
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 20, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 20, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the review wanted PRs that need reviews. label Mar 23, 2023
@lpinca
Copy link
Member Author

lpinca commented Apr 2, 2023

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

                                                                       confidence improvement accuracy (*)    (**)   (***)
zlib/createInflate.js n=100 chunkLen=1024 inputLen=16777216                     *     -3.28 %       ±2.57%  ±3.44%  ±4.50%
zlib/creation.js n=500000 options='false' type='BrotliCompress'                        1.77 %       ±3.47%  ±4.62%  ±6.02%
zlib/creation.js n=500000 options='false' type='BrotliDecompress'                      1.85 %       ±3.69%  ±4.91%  ±6.39%
zlib/creation.js n=500000 options='false' type='Deflate'                               1.01 %       ±2.73%  ±3.63%  ±4.72%
zlib/creation.js n=500000 options='false' type='DeflateRaw'                            2.79 %       ±3.19%  ±4.24%  ±5.52%
zlib/creation.js n=500000 options='false' type='Gunzip'                               -1.61 %       ±2.50%  ±3.33%  ±4.33%
zlib/creation.js n=500000 options='false' type='Gzip'                                 -1.21 %       ±2.89%  ±3.85%  ±5.01%
zlib/creation.js n=500000 options='false' type='Inflate'                               0.10 %       ±3.21%  ±4.28%  ±5.57%
zlib/creation.js n=500000 options='false' type='InflateRaw'                           -0.03 %       ±3.16%  ±4.21%  ±5.48%
zlib/creation.js n=500000 options='false' type='Unzip'                                -1.26 %       ±2.50%  ±3.32%  ±4.32%
zlib/creation.js n=500000 options='true' type='BrotliCompress'                         0.70 %       ±3.31%  ±4.40%  ±5.74%
zlib/creation.js n=500000 options='true' type='BrotliDecompress'                       3.30 %       ±4.00%  ±5.33%  ±6.93%
zlib/creation.js n=500000 options='true' type='Deflate'                               -0.79 %       ±2.29%  ±3.05%  ±3.98%
zlib/creation.js n=500000 options='true' type='DeflateRaw'                            -0.11 %       ±2.54%  ±3.38%  ±4.40%
zlib/creation.js n=500000 options='true' type='Gunzip'                                 1.12 %       ±2.90%  ±3.87%  ±5.05%
zlib/creation.js n=500000 options='true' type='Gzip'                                   0.69 %       ±2.14%  ±2.84%  ±3.70%
zlib/creation.js n=500000 options='true' type='Inflate'                               -1.19 %       ±3.10%  ±4.13%  ±5.39%
zlib/creation.js n=500000 options='true' type='InflateRaw'                             1.26 %       ±3.17%  ±4.22%  ±5.49%
zlib/creation.js n=500000 options='true' type='Unzip'                                  0.03 %       ±3.33%  ±4.43%  ±5.77%
zlib/deflate.js n=400000 inputLen=1024 method='createDeflate'                         -2.41 %       ±6.16%  ±8.19% ±10.66%
zlib/deflate.js n=400000 inputLen=1024 method='deflate'                                2.85 %       ±7.73% ±10.29% ±13.39%
zlib/deflate.js n=400000 inputLen=1024 method='deflateSync'                            0.94 %       ±2.75%  ±3.66%  ±4.77%
zlib/inflate.js n=400000 inputLen=1024 method='inflate'                         *     -3.01 %       ±2.40%  ±3.19%  ±4.16%
zlib/inflate.js n=400000 inputLen=1024 method='inflateSync'                           -0.02 %       ±2.26%  ±3.00%  ±3.91%
zlib/pipe.js algorithm='brotli' type='buffer' duration=5 inputLen=1024                -0.45 %       ±2.61%  ±3.47%  ±4.51%
zlib/pipe.js algorithm='brotli' type='string' duration=5 inputLen=1024                -0.00 %       ±0.94%  ±1.25%  ±1.63%
zlib/pipe.js algorithm='gzip' type='buffer' duration=5 inputLen=1024                  -3.71 %       ±4.61%  ±6.15%  ±8.04%
zlib/pipe.js algorithm='gzip' type='string' duration=5 inputLen=1024                  -0.06 %       ±3.66%  ±4.87%  ±6.34%

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 (***)

@lpinca
Copy link
Member Author

lpinca commented Apr 4, 2023

Ping @nodejs/collaborators

@richardlau
Copy link
Member

Are the benchmarks showing a regression, or are the negative values statsiticly insignificant?

@lpinca
Copy link
Member Author

lpinca commented Apr 4, 2023

Are the benchmarks showing a regression, or are the negative values statsiticly insignificant?

They are statistically insignificant.

@anonrig
Copy link
Member

anonrig commented Apr 4, 2023

Are the benchmarks showing a regression, or are the negative values statsiticly insignificant?

They are statistically insignificant. There isn't any regression.

@anonrig anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 4, 2023
@lpinca
Copy link
Member Author

lpinca commented Apr 4, 2023

FWIW there is now support for AVX-512 based CRC-32 checksum upstream but I would wait a few days before updating to https://chromium.googlesource.com/chromium/src/third_party/zlib/+/b890619bc2b193b8fbe9c1c053f4cd19a9791d92.

deps/zlib/BUILD.gn Show resolved Hide resolved
deps/zlib/zconf.h.cmakein Show resolved Hide resolved
Updated as described in doc/contributing/maintaining-zlib.md.

Refs: nodejs#45387 (comment)
The `sed -i -- 's_^#include "chromeconf.h"_//#include "chromeconf.h"_' deps/zlib/zconf.h`
command creates a `zconf.h--` backup file on macOS. Replace it with an
equivalent perl one-liner so that it works on both Linux and macOS.
@lpinca
Copy link
Member Author

lpinca commented Apr 4, 2023

PTAL.

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2023
@lpinca lpinca added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Apr 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 6, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 6, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/47151
✔  Done loading data for nodejs/node/pull/47151
----------------------------------- PR info ------------------------------------
Title      deps: update zlib to upstream 5edb52d4 (#47151)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     lpinca:update/zlib -> nodejs:main
Labels     zlib, author ready, needs-ci, review wanted, dependencies, commit-queue-rebase
Commits    2
 - deps: update zlib to upstream 5edb52d4
 - doc: do not create a backup file
Committers 1
 - Luigi Pinca 
PR-URL: https://github.com/nodejs/node/pull/47151
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Mohammed Keyvanzadeh 
Reviewed-By: Richard Lau 
Reviewed-By: Michael Dawson 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/47151
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Mohammed Keyvanzadeh 
Reviewed-By: Richard Lau 
Reviewed-By: Michael Dawson 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 18 Mar 2023 20:03:06 GMT
   ✔  Approvals: 4
   ✔  - Yagiz Nizipli (@anonrig): https://github.com/nodejs/node/pull/47151#pullrequestreview-1371406611
   ✔  - Mohammed Keyvanzadeh (@VoltrexKeyva): https://github.com/nodejs/node/pull/47151#pullrequestreview-1371408287
   ✔  - Richard Lau (@richardlau) (TSC): https://github.com/nodejs/node/pull/47151#pullrequestreview-1371581122
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/47151#pullrequestreview-1371856389
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2023-04-05T18:38:14Z: https://ci.nodejs.org/job/node-test-pull-request/50983/
   ℹ  Last Benchmark CI on 2023-04-02T17:25:15Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1312/
- Querying data for job/node-test-pull-request/50983/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/4627449295

lpinca added a commit that referenced this pull request Apr 6, 2023
Updated as described in doc/contributing/maintaining-zlib.md.

Refs: #45387 (comment)
PR-URL: #47151
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
lpinca added a commit that referenced this pull request Apr 6, 2023
The `sed -i -- 's_^#include "chromeconf.h"_//#include "chromeconf.h"_' deps/zlib/zconf.h`
command creates a `zconf.h--` backup file on macOS. Replace it with an
equivalent perl one-liner so that it works on both Linux and macOS.

PR-URL: #47151
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@lpinca
Copy link
Member Author

lpinca commented Apr 6, 2023

Landed in 0e79635...509b1eb.

@lpinca lpinca closed this Apr 6, 2023
@lpinca lpinca deleted the update/zlib branch April 6, 2023 14:44
RafaelGSS pushed a commit that referenced this pull request Apr 13, 2023
Updated as described in doc/contributing/maintaining-zlib.md.

Refs: #45387 (comment)
PR-URL: #47151
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
RafaelGSS pushed a commit that referenced this pull request Apr 13, 2023
The `sed -i -- 's_^#include "chromeconf.h"_//#include "chromeconf.h"_' deps/zlib/zconf.h`
command creates a `zconf.h--` backup file on macOS. Replace it with an
equivalent perl one-liner so that it works on both Linux and macOS.

PR-URL: #47151
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
Updated as described in doc/contributing/maintaining-zlib.md.

Refs: #45387 (comment)
PR-URL: #47151
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
The `sed -i -- 's_^#include "chromeconf.h"_//#include "chromeconf.h"_' deps/zlib/zconf.h`
command creates a `zconf.h--` backup file on macOS. Replace it with an
equivalent perl one-liner so that it works on both Linux and macOS.

PR-URL: #47151
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
Updated as described in doc/contributing/maintaining-zlib.md.

Refs: nodejs#45387 (comment)
PR-URL: nodejs#47151
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
The `sed -i -- 's_^#include "chromeconf.h"_//#include "chromeconf.h"_' deps/zlib/zconf.h`
command creates a `zconf.h--` backup file on macOS. Replace it with an
equivalent perl one-liner so that it works on both Linux and macOS.

PR-URL: nodejs#47151
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.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. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. 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.

6 participants