deps: update zlib to upstream 5edb52d4#47151

lpinca · 2023-03-18T20:03:06Z

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

nodejs-github-bot · 2023-03-20T20:25:08Z

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

nodejs-github-bot · 2023-03-22T20:26:19Z

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

lpinca · 2023-04-02T17:25:15Z

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 · 2023-04-04T16:52:13Z

Ping @nodejs/collaborators

richardlau · 2023-04-04T16:55:00Z

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

lpinca · 2023-04-04T16:56:21Z

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

They are statistically insignificant.

anonrig · 2023-04-04T16:56:31Z

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

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

lpinca · 2023-04-04T17:00:51Z

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

deps/zlib/zconf.h.cmakein

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 · 2023-04-04T20:40:58Z

PTAL.

nodejs-github-bot · 2023-04-04T20:44:34Z

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

mhdawson

LGTM

nodejs-github-bot · 2023-04-05T06:27:43Z

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

nodejs-github-bot · 2023-04-05T09:16:31Z

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

nodejs-github-bot · 2023-04-05T18:38:14Z

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

nodejs-github-bot · 2023-04-06T09:28:27Z

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 <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

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 <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

lpinca · 2023-04-06T14:43:38Z

Landed in 0e79635...509b1eb.

Updated as described in doc/contributing/maintaining-zlib.md. Refs: #45387 (comment) PR-URL: #47151 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

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 <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Updated as described in doc/contributing/maintaining-zlib.md. Refs: #45387 (comment) PR-URL: #47151 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

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 <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Updated as described in doc/contributing/maintaining-zlib.md. Refs: nodejs#45387 (comment) PR-URL: nodejs#47151 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

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 <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

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-actionsbot 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-actionsbot 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 pending requests for changes, 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

deps/zlib/BUILD.gnShow resolvedHide resolved
deps/zlib/zconf.h.cmakeinShow resolvedHide resolved

lpinca force-pushed the update/zlib branch from 8dceb84 to ab412abCompare 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 b0522c7Compare 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-actionsbot 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-actionsbot 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

Uh oh!

deps: update zlib to upstream 5edb52d4#47151

deps: update zlib to upstream 5edb52d4 #47151

Uh oh!

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