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.

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

Increase zstd_compression_level to 19 #217

Merged
merged 2 commits into from
Dec 1, 2022

Conversation

mbargull
Copy link
Member

@mbargull mbargull commented Nov 30, 2022

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Copy/paste a comment of mine from conda/conda-package-handling#167 :

I highly recommend going for -19 at max. Not just because of the resources during compression but also for the decompression on the users' side. For decompression you'd need some sub 10M base memory plus the window size (which is capped by the input size, i.e., it's most relevant for big packages), e.g.,

* level 19: window size: `2**23=8M`

* level 20: window size: `2**25=32M`

* level 21: window size: `2**26=64M`

* level 22: window size: `2**27=128M`

For compression the higher levels additionally use bigger search tables which is why the memory increases more. See https://github.com/facebook/zstd/blob/v1.5.2/lib/compress/clevels.h#L44-L50 for the actual (logarithmic) values used for each level and https://manpages.ubuntu.com/manpages/focal/man1/zstd.1.html for a short description of those.

You get slightly higher resource usage for -19 compared to -18 during compression (and the same low max x+8M for decompression). I'd go for one of those and try -19 first and see if we encounter cases that demand lowering it to -18 if needed.

refs:

@mbargull mbargull requested a review from a team as a code owner November 30, 2022 22:44
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@mbargull
Copy link
Member Author

zstd.ZstdError: zstd compress error: Allocation error : not enough memory

hehe

@jakirkham
Copy link
Member

jakirkham commented Nov 30, 2022

For the viewers at home, the error above will be fixed by PR ( conda-forge/conda-build-feedstock#190 )

@jakirkham
Copy link
Member

jakirkham commented Dec 1, 2022

Restarting the failed CI builds since conda-build packages are up

@jakirkham jakirkham added the automerge Merge the PR when CI passes label Dec 1, 2022
@beckermr
Copy link
Member

beckermr commented Dec 1, 2022

Technically a decrease since due to the conda build bug we were at 22.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2022

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

@jakirkham
Copy link
Member

jakirkham commented Dec 1, 2022

Failure here is due to this issue ( conda-forge/conda-build-feedstock#190 (comment) )

Updating boa's conda-build pin will fix it ( conda-forge/conda-build-feedstock#190 (comment) ). Filed issue ( conda-forge/boa-feedstock#58 )

@mbargull
Copy link
Member Author

mbargull commented Dec 1, 2022

Technically a decrease since due to the conda build bug we were at 22.

I didn't mention the effective level :P.
(But a valid point ^_^ .)

@mbargull
Copy link
Member Author

mbargull commented Dec 1, 2022

@conda-forge-admin, please restart ci

@mbargull
Copy link
Member Author

mbargull commented Dec 1, 2022

Over 50 jobs for this PR and not a single one failed due OOM issue -- looks like the newest conda-build+boa builds work fine and this was built with zstd_compression_level: 16 🎉.

@github-actions github-actions bot merged commit 767e042 into conda-forge:main Dec 1, 2022
@beckermr
Copy link
Member

beckermr commented Dec 1, 2022

Actually built with 19. This feedstock installs the local copy of ci setup and builds with that. Helps for testing and bootstrapping.

@jakirkham
Copy link
Member

Awesome thanks Marcel! 🙏

This feedstock installs the local copy of ci setup and builds with that. Helps for testing and bootstrapping.

Definitely, this was a motivating design principle early on. Particularly as things broke more often. We needed an easy way to get out of a jam and bootstrapping helps with that

@mbargull
Copy link
Member Author

mbargull commented Dec 1, 2022

Actually built with 19. This feedstock installs the local copy of ci setup and builds with that. Helps for testing and bootstrapping.

ooh, I'm always happy to be corrected, plus getting bits and pieces of information around the infra from you in comments like that one :)

@beckermr
Copy link
Member

beckermr commented Dec 1, 2022

Yes thank you @mbargull!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the PR when CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants