Skip to content

Conversation

@targos
Copy link
Member

Fixes: #41744

targosand others added 3 commits January 29, 2022 13:13
Updated as described in doc/contributing/maintaining-zlib.md.
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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#35679Fixes: nodejs#35629 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
@nodejs-github-botnodejs-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
@targostargos added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2022
@targos
Copy link
MemberAuthor

targos commented Jan 29, 2022

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-actionsgithub-actionsbot 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.

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
MemberAuthor

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

@RaisinTen
Copy link
Member

Don't we need #33044 to land first?

@targos
Copy link
MemberAuthor

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
Member

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

@targos
Copy link
MemberAuthor

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

Copy link
Member

@benjamingrbenjamingr left a comment

Choose a reason for hiding this comment

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

RSLGTM

@RaisinTen
Copy link
Member

RaisinTen commented Feb 3, 2022

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% 

Copy link
Member

@RaisinTenRaisinTen 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
MemberAuthor

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.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.zlibIssues and PRs related to the zlib subsystem.

Projects

None yet

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