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

Limit ZSTD_maxCLevel to 21 for 32-bit binaries. #2885

Merged
merged 1 commit into from
Dec 1, 2021

Conversation

yoniko
Copy link
Contributor

@yoniko yoniko commented Nov 30, 2021

Adjusted the max level for 32-bit binaries to be 21 instead of 22.
Added CI test to check 32-bit binaries in a memory constrained environment.
The purpose of this change is to reduce the number of failures created by memory constrained 32-bit platforms.

CI tests for constrained memory runs with max level on 32-bit binaries.
@yoniko yoniko changed the title ZSTD_maxCLevel now limited to 21 for 32-bit binaries. Limit ZSTD_maxCLevel to 21 for 32-bit binaries. Nov 30, 2021
@Cyan4973
Copy link
Contributor

Cyan4973 commented Dec 1, 2021

Thanks @yoniko , I think this covers the topic pretty well.

I also really like the new test !

@Cyan4973 Cyan4973 merged commit 8031dc7 into facebook:dev Dec 1, 2021
@ghost
Copy link

ghost commented Dec 2, 2021

If someone uses compression level 22 to compress small files, will this change reduce the compression rate?

@Cyan4973
Copy link
Contributor

Cyan4973 commented Dec 2, 2021

Yes of course, at least theoretically, since level 21 is obviously less powerful than level 22 (though not by much).
But in practice, chances are that level 22 will crash a 32-bit system anyway, due to exhaustion of address space. So there might not be anything to compare too.

Furthermore, someone who absolutely wants stronger compression parameters can take matters into its own hand, and manually specify the compression parameters they wish, advanced compression parameters remain applied as requested.

@ghost
Copy link

ghost commented Dec 2, 2021

If there are no complaints from many users, and it's mainly to prevent CI failure, maybe no need to modify it. Just my two cents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants