deps: update zlib to upstream 5edb52d4 by lpinca · Pull Request #47151 · nodejs/node · GitHub

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

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

CI: https://ci.nodejs.org/job/node-test-pull-request/50497/

CI: https://ci.nodejs.org/job/node-test-pull-request/50557/

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

Ping @nodejs/collaborators

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

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

They are statistically insignificant.

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

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

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.

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.

PTAL.

CI: https://ci.nodejs.org/job/node-test-pull-request/50935/

LGTM

CI: https://ci.nodejs.org/job/node-test-pull-request/50952/

CI: https://ci.nodejs.org/job/node-test-pull-request/50957/

CI: https://ci.nodejs.org/job/node-test-pull-request/50983/

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

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>

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>

Landed in 0e79635...509b1eb.

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>

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>

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>

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>

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>

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>

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 mentioned this pull request Mar 19, 2023

deps: delete deps/zlib #47157

Closed

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 20, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 20, 2023

This was referenced Mar 21, 2023

CI Reliability 2023-03-21 nodejs/reliability#531

Open

CI Reliability 2023-03-22 nodejs/reliability#532

Open

github-actions Bot mentioned this pull request Mar 23, 2023

CI Reliability 2023-03-23 nodejs/reliability#533

Open

33 tasks

lpinca added the review wanted PRs that need reviews. label Mar 23, 2023

anonrig approved these changes Apr 4, 2023

View reviewed changes

anonrig added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 4, 2023

VoltrexKeyva approved these changes Apr 4, 2023

View reviewed changes

richardlau reviewed Apr 4, 2023

View reviewed changes

Comment thread deps/zlib/BUILD.gn Outdated

Comment thread deps/zlib/zconf.h.cmakein Outdated

lpinca force-pushed the update/zlib branch from 8dceb84 to ab412ab Compare April 4, 2023 18:20

richardlau approved these changes Apr 4, 2023

View reviewed changes

lpinca added 2 commits April 4, 2023 21:57

deps: update zlib to upstream 5edb52d4

3e91e31

Updated as described in doc/contributing/maintaining-zlib.md. Refs: nodejs#45387 (comment)

doc: do not create a backup file

b0522c7

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 force-pushed the update/zlib branch from ab412ab to b0522c7 Compare April 4, 2023 20:39

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2023

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 Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2023

mhdawson approved these changes Apr 4, 2023

View reviewed changes

github-actions Bot mentioned this pull request Apr 5, 2023

CI Reliability 2023-04-05 nodejs/reliability#540

Open

26 tasks

lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 6, 2023

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

lpinca closed this Apr 6, 2023

lpinca deleted the update/zlib branch April 6, 2023 14:44

RafaelGSS mentioned this pull request Apr 11, 2023

2023-04-18, Version 20.0.0 (Current) #47381

Merged

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

deps: update zlib to upstream 5edb52d4 by lpinca · Pull Request #47151 · nodejs/node · GitHub

Uh oh!

Uh oh!

deps: update zlib to upstream 5edb52d4 - #47151

deps: update zlib to upstream 5edb52d4#47151lpinca wants to merge 2 commits intonodejs:mainnodejs/node:mainfrom lpinca:update/zliblpinca/node:update/zlibCopy head branch name to clipboard

Conversation

lpinca commented Mar 18, 2023

Uh oh!

nodejs-github-bot commented Mar 20, 2023

Uh oh!

nodejs-github-bot commented Mar 22, 2023

Uh oh!

lpinca commented Apr 2, 2023

Uh oh!

lpinca commented Apr 4, 2023

Uh oh!

richardlau commented Apr 4, 2023

Uh oh!

lpinca commented Apr 4, 2023

Uh oh!

anonrig commented Apr 4, 2023

Uh oh!

lpinca commented Apr 4, 2023

Uh oh!

Uh oh!

Uh oh!

lpinca commented Apr 4, 2023

Uh oh!

nodejs-github-bot commented Apr 4, 2023

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 5, 2023

Uh oh!

nodejs-github-bot commented Apr 5, 2023

Uh oh!

nodejs-github-bot commented Apr 5, 2023

Uh oh!

nodejs-github-bot commented Apr 6, 2023

Uh oh!

lpinca commented Apr 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

deps: update zlib to upstream 5edb52d4#47151
lpinca wants to merge 2 commits into
nodejs:mainfrom
lpinca:update/zlib