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.

Sign up for GitHub

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

Jump to bottom

deps: update zlib to upstream fc5cfd78a3 #41745

Closed
wants to merge 6 commits into from
Closed

deps: update zlib to upstream fc5cfd78a3 #41745

wants to merge 6 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Jan 29, 2022

Fixes: #41744

targos and others added 3 commits January 29, 2022 13:13
@targos
deps: update zlib to upstream fc5cfd78a3
6170ee4 
Updated as described in doc/contributing/maintaining-zlib.md.
@addaleax @targos
deps: fix zlib compilation for CPUs without SIMD features
56e6a51 
Fix the compile flags so that zlib can run on CPUs that do
not have SSSE3/SSE4.2/etc. Do not compile zlib with flags that
indicate that those features are available, and instead enable
them selectively for functions that use them.

There are probably better way to do this, e.g. through gyp file
modifications as suggested in the issue. However, this patch
should do just fine until that happens.

Fixes: nodejs#32553

PR-URL: nodejs#32627
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RaisinTen @targos
build: fix zlib inlining for IA-32
ec258fe 
PR-URL: nodejs#35679
Fixes: nodejs#35629
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib subsystem. labels Jan 29, 2022
@targos targos mentioned this pull request Jan 29, 2022
Closed
@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2022
@targos targos mentioned this pull request Jan 29, 2022
Closed
@targos
Copy link
Member Author

targos commented Jan 29, 2022 •
edited

Windows error: D:\a\node\node\deps\zlib\cpu_features.c(52,1): fatal error C1189: #error: cpu_features.c CPU feature detection in not defined for your platform [D:\a\node\node\deps\zlib\zlib.vcxproj]

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2022
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member Author

targos commented Jan 29, 2022

Error remains on ARM64 Windows but that should be fixable in a similar way.

@RaisinTen
Copy link
Contributor

Don't we need #33044 to land first?

@github-actions github-actions bot mentioned this pull request Jan 30, 2022
Open
25 tasks
@targos
Copy link
Member Author

targos commented Jan 30, 2022

Don't we need #33044 to land first?

Why? I'd like that PR to land but it is not being worked on currently.

@RaisinTen
Copy link
Contributor

If the gyp files are not in sync with the upstream gn files, the compiler won't be passed the expected options which might lead to issues like incorrect code generation - #39313 (comment) or slowdowns - #33044 (comment).

@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member Author

targos commented Jan 30, 2022

I think we'll need help from @nodejs/platform-windows-arm

benjamingr
benjamingr approved these changes Jan 30, 2022
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

RSLGTM

This was referenced Jan 31, 2022
Open
Open
Open
Open
@RaisinTen
Copy link
Contributor

RaisinTen commented Feb 3, 2022 •
edited

Zlib benchmark CI: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1093/

                                                                       confidence improvement accuracy (*)    (**)   (***)
zlib/createInflate.js n=100 chunkLen=1024 inputLen=16777216                            0.35 %       ±7.56% ±10.05% ±13.09%
zlib/creation.js n=500000 options='false' type='BrotliCompress'                       -1.65 %       ±4.25%  ±5.65%  ±7.36%
zlib/creation.js n=500000 options='false' type='BrotliDecompress'                     -3.81 %       ±4.78%  ±6.35%  ±8.27%
zlib/creation.js n=500000 options='false' type='Deflate'                               0.25 %       ±4.22%  ±5.62%  ±7.31%
zlib/creation.js n=500000 options='false' type='DeflateRaw'                           -2.12 %       ±3.54%  ±4.71%  ±6.13%
zlib/creation.js n=500000 options='false' type='Gunzip'                                1.68 %       ±4.18%  ±5.57%  ±7.27%
zlib/creation.js n=500000 options='false' type='Gzip'                                  2.04 %       ±4.08%  ±5.43%  ±7.07%
zlib/creation.js n=500000 options='false' type='Inflate'                              -0.78 %       ±3.50%  ±4.66%  ±6.07%
zlib/creation.js n=500000 options='false' type='InflateRaw'                           -2.67 %       ±2.78%  ±3.70%  ±4.82%
zlib/creation.js n=500000 options='false' type='Unzip'                                -2.04 %       ±3.31%  ±4.41%  ±5.74%
zlib/creation.js n=500000 options='true' type='BrotliCompress'                         0.84 %       ±4.88%  ±6.49%  ±8.44%
zlib/creation.js n=500000 options='true' type='BrotliDecompress'                       0.72 %       ±5.75%  ±7.65%  ±9.97%
zlib/creation.js n=500000 options='true' type='Deflate'                               -3.15 %       ±3.92%  ±5.22%  ±6.80%
zlib/creation.js n=500000 options='true' type='DeflateRaw'                            -3.66 %       ±4.16%  ±5.54%  ±7.23%
zlib/creation.js n=500000 options='true' type='Gunzip'                                -0.02 %       ±3.74%  ±4.98%  ±6.49%
zlib/creation.js n=500000 options='true' type='Gzip'                                  -2.11 %       ±4.29%  ±5.71%  ±7.43%
zlib/creation.js n=500000 options='true' type='Inflate'                                1.08 %       ±3.96%  ±5.27%  ±6.86%
zlib/creation.js n=500000 options='true' type='InflateRaw'                             2.51 %       ±4.65%  ±6.19%  ±8.07%
zlib/creation.js n=500000 options='true' type='Unzip'                          **     -5.13 %       ±3.42%  ±4.55%  ±5.93%
zlib/deflate.js n=400000 inputLen=1024 method='createDeflate'                         -1.70 %       ±9.96% ±13.25% ±17.25%
zlib/deflate.js n=400000 inputLen=1024 method='deflate'                       ***    -10.67 %       ±5.75%  ±7.65%  ±9.97%
zlib/deflate.js n=400000 inputLen=1024 method='deflateSync'                   ***    -23.99 %      ±10.14% ±13.49% ±17.56%
zlib/inflate.js n=400000 inputLen=1024 method='inflate'                               -1.77 %       ±3.60%  ±4.80%  ±6.26%
zlib/inflate.js n=400000 inputLen=1024 method='inflateSync'                           -3.80 %       ±4.35%  ±5.79%  ±7.55%
zlib/pipe.js algorithm='brotli' type='buffer' duration=5 inputLen=1024                 2.06 %       ±2.10%  ±2.79%  ±3.65%
zlib/pipe.js algorithm='brotli' type='string' duration=5 inputLen=1024                -0.82 %       ±2.79%  ±3.72%  ±4.84%
zlib/pipe.js algorithm='gzip' type='buffer' duration=5 inputLen=1024                  -2.51 %       ±4.97%  ±6.62%  ±8.63%
zlib/pipe.js algorithm='gzip' type='string' duration=5 inputLen=1024            *     -7.94 %       ±6.27%  ±8.38% ±10.97%

RaisinTen
RaisinTen suggested changes Feb 4, 2022
View reviewed changes
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

There are some performance regressions which need to be fixed before landing this.

@targos
Copy link
Member Author

targos commented Feb 4, 2022

Well, I tried :(
I'm closing the PR because I personally don't know how to go further with it, but keeping the branch in case someone wants to use it as a base.

@targos targos closed this Feb 4, 2022
@RaisinTen RaisinTen mentioned this pull request Mar 13, 2022
Merged
@aduh95 aduh95 mentioned this pull request Jul 19, 2022
Merged
@targos targos mentioned this pull request Aug 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen requested changes

@benjamingr benjamingr benjamingr approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bundled zlib is missing an upstream UB fix
6 participants
@targos @nodejs-github-bot @RaisinTen @benjamingr @cjihrig @addaleax