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 8bbd6c31 #45387

Closed
wants to merge 2 commits into from
Closed

deps: update zlib to upstream 8bbd6c31 #45387

wants to merge 2 commits into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Nov 9, 2022 •
edited
Loading

Update zlib to upstream 8bbd6c31.

Supersedes #44412.

@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 Nov 9, 2022
@lpinca lpinca mentioned this pull request Nov 9, 2022
Closed
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2022
@lpinca
Copy link
Member Author

lpinca commented Nov 9, 2022

Benchmark CI: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1215/

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

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
RafaelGSS
RafaelGSS approved these changes Nov 9, 2022
View reviewed changes
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM.

@Neustradamus
Copy link

To follow

@github-actions github-actions bot mentioned this pull request Nov 10, 2022
Open
20 tasks
@mscdex
Copy link
Contributor

mscdex commented Nov 10, 2022

FWIW it looks like there is no difference in results after re-running the deflate benchmarks.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@lpinca
Copy link
Member Author

lpinca commented Nov 10, 2022 •
edited
Loading

Build on Windows ARM fails, but I see no specific error.

@RaisinTen
Copy link
Contributor

If you view the logs as plain text and search for the last occurrence of error: you'll see:

C:\workspace\node-compile-windows\node\deps\zlib\cpu_features.c(55,1): fatal error C1189: #error:  cpu_features.c CPU feature detection in not defined for your platform [C:\workspace\node-compile-windows\node\deps\zlib\zlib.vcxproj]

@lpinca
Copy link
Member Author

lpinca commented Nov 10, 2022

@RaisinTen thanks. I'll take a look. We might need to patch the gyp file as suggested here #44412 (comment).

@lpinca lpinca force-pushed the update/zlib branch 2 times, most recently from 5fe09af to 02dfc58 Compare November 10, 2022 09:21
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 10, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 10, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Nov 17, 2022
Open
13 tasks
This was referenced Nov 17, 2022
Closed
Closed
Closed
Closed
@github-actions github-actions bot mentioned this pull request Nov 18, 2022
Open
12 tasks
This was referenced Nov 21, 2022
Closed
Closed
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
@lpinca @ruyadorno
deps: update zlib to upstream 8bbd6c31
c41e67f 
Updated as described in doc/contributing/maintaining-zlib.md.

PR-URL: #45387
Reviewed-By: Rafael Gonzaga <[email protected]>
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
@addaleax @ruyadorno
deps: fix zlib compilation for CPUs without SIMD features
45ba14b 
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: #32553

PR-URL: #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: #45387
Reviewed-By: Rafael Gonzaga <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@lpinca @danielleadams
deps: update zlib to upstream 8bbd6c31
71e8723 
Updated as described in doc/contributing/maintaining-zlib.md.

PR-URL: #45387
Reviewed-By: Rafael Gonzaga <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@addaleax @danielleadams
deps: fix zlib compilation for CPUs without SIMD features
93b9cc4 
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: #32553

PR-URL: #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: #45387
Reviewed-By: Rafael Gonzaga <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@lpinca @danielleadams
deps: update zlib to upstream 8bbd6c31
17893de 
Updated as described in doc/contributing/maintaining-zlib.md.

PR-URL: #45387
Reviewed-By: Rafael Gonzaga <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@addaleax @danielleadams
deps: fix zlib compilation for CPUs without SIMD features
bbd60e2 
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: #32553

PR-URL: #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: #45387
Reviewed-By: Rafael Gonzaga <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@lpinca @danielleadams
deps: update zlib to upstream 8bbd6c31
978cfad 
Updated as described in doc/contributing/maintaining-zlib.md.

PR-URL: #45387
Reviewed-By: Rafael Gonzaga <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@addaleax @danielleadams
deps: fix zlib compilation for CPUs without SIMD features
43419ad 
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: #32553

PR-URL: #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: #45387
Reviewed-By: Rafael Gonzaga <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
@lpinca @danielleadams
deps: update zlib to upstream 8bbd6c31
f2f853f 
Updated as described in doc/contributing/maintaining-zlib.md.

PR-URL: #45387
Reviewed-By: Rafael Gonzaga <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
@addaleax @danielleadams
deps: fix zlib compilation for CPUs without SIMD features
5b52e92 
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: #32553

PR-URL: #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: #45387
Reviewed-By: Rafael Gonzaga <[email protected]>
@nappy
Copy link
Contributor

nappy commented Feb 20, 2023 •
edited
Loading

This (or maybe the other commit changing the gyp file) seems to break the Android arm64 build; zlib now references cpu-features.h on arm64 but this is not something provided by the platform it seems, but a module that needs to be compiled?!?

@Adenilson
Copy link

I noticed that it seems that node.js is lagging 4 months behind Chromium zlib (i.e. in sync with hash 8bbd6c31 from Nov 7th of 2022) at the time of this writing.

It may be a good idea to sync with ToT (Top of the Tree), since I landed last week an optimization that improved in average data decompression speed in average +12% to +16% for Intel processors.

In some cases (e.g. HTML) the improvement was massive (i.e. +46% faster decompression).

Is there someone within the node.js project that I could reach out from time to time to share whenever a sync with Chromium zlib may be in order (i.e. contact email)?

@lpinca lpinca mentioned this pull request Mar 18, 2023
Closed
lpinca added a commit to lpinca/node that referenced this pull request Mar 18, 2023
@lpinca
deps: update zlib to upstream 5edb52d4
8dceb84 
Updated as described in doc/contributing/maintaining-zlib.md.

Refs: nodejs#45387 (comment)
lpinca added a commit to lpinca/node that referenced this pull request Apr 4, 2023
@lpinca
deps: update zlib to upstream 5edb52d4
fb368cc 
Updated as described in doc/contributing/maintaining-zlib.md.

Refs: nodejs#45387 (comment)
lpinca added a commit to lpinca/node that referenced this pull request Apr 4, 2023
@lpinca
deps: update zlib to upstream 5edb52d4
3e91e31 
Updated as described in doc/contributing/maintaining-zlib.md.

Refs: nodejs#45387 (comment)
lpinca added a commit that referenced this pull request Apr 6, 2023
@lpinca
deps: update zlib to upstream 5edb52d4
be2791b 
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]>
RafaelGSS pushed a commit that referenced this pull request Apr 13, 2023
@lpinca @RafaelGSS
deps: update zlib to upstream 5edb52d4
e7ca09f 
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]>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@lpinca @danielleadams
deps: update zlib to upstream 5edb52d4
69a0050 
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]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@lpinca @MoLow
deps: update zlib to upstream 5edb52d4
443a72e 
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. 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.
9 participants
@lpinca @nodejs-github-bot @Neustradamus @mscdex @RaisinTen @RafaelGSS @nappy @Adenilson @addaleax