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

Provide ways to disable compression support? #190

Open
NobodyXu opened this issue Jan 24, 2023 · 16 comments
Open

Provide ways to disable compression support? #190

NobodyXu opened this issue Jan 24, 2023 · 16 comments

Comments

@NobodyXu
Copy link
Contributor

NobodyXu commented Jan 24, 2023

Upstream zstd provides build macro ZSTD_LIB_COMPRESSION to disable compression support, which will then significantly reduce the build time of the zstd-sys crate and reducing the size of the final binary.

However, adding a feature to zstd-sys will certainly be a breaking change even if it's enabled by default (could be opt-out via default-features = false) and it's likely other crates using this will just switch them on by default.

I'm thinking of a new environment flag ZSTD_SYS_DISABLE_COMPRESSION:

When enabled, it will enable ZSTD_LIB_COMPRESSION and change all the compression API to just return an error, signaling that is not supported.

But that sounds really hacky and I would rather like other solutions, like using gc-sections in #188 or using cross-languages LTO #191.

@NobodyXu
Copy link
Contributor Author

Perhaps we should just let the users specify CFLAGS to disable compression support #192 ?

@nickbabcock
Copy link

I'm curious how large the size reduction is, as I'm testing out different compression algorithms in Wasm and as a point of comparison, the default brotli dictionary increases the Wasm size by 3.5x (from 400 KB to 1400 KB). I'm not expecting a similar drastic reduction, but I am curious what the impact would be, as any savings over the current 500 KB zstd wasm build may be greatly appreciated by the web community.

@NobodyXu
Copy link
Contributor Author

@nickbabcock You can use CFLAG to do that for now.
Simply define ZSTD_LIB_COMPRESSION=0 and ZSTD_LIB_DICTBUILDER=0 as suggested by modular-build in zstd, you should be able to disable compression support, but linking might fail.

Alternatively, you can compile zstd yourself with these flags to try that out.

PR #189 should also reduce the size if you enable feature thin and once cc makes a new release containing rust-lang/cc-rs#757 , I will try re-enabling LTO again.

@nickbabcock
Copy link

Thanks for the tip. I may be a bit slow this morning, but I tried:

CFLAGS="-DZSTD_LIB_COMPRESSION=0" cargo build -vv --release

And I see lines like the following that make me think I'm communicating ZSTD_LIB_COMPRESSION correctly.

[zstd-sys 2.0.5+zstd.1.5.2] running: "cc" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "-m64" "-DZSTD_LIB_COMPRESSION=0" "-I" "zstd/lib/" "-I" "zstd/lib/common" "-fvisibility=hidden" "-DZSTD_LIB_DEPRECATED=0" "-DXXH_PRIVATE_API=" "-DZSTDLIB_VISIBILITY=" "-DZSTDERRORLIB_VISIBILITY=" "-o" "/home/nick/projects/wasm-bench/target/release/build/zstd-sys-665d444381abd221/out/zstd/lib/compress/zstd_fast.o" "-c" "zstd/lib/compress/zstd_fast.c"

But I am not seeing any difference in output.

Maybe it's because the zstd-sys is always compiling files found in the zstd/lib/compress and including them in the output. I don't see ZSTD_LIB_COMPRESSION used in zstd outside the makefile.

@NobodyXu
Copy link
Contributor Author

@nickbabcock You are absolutely right on this and that means #189 is actually changing the def to no-op.
Would open another PR to fix that.

@NobodyXu
Copy link
Contributor Author

@nickbabcock In that case, I guess the only thing that couild help is cross language LTO #191

@nickbabcock
Copy link

@NobodyXu , my first thought would be to guard adding the files related to compression with a cargo feature (like zdict_builder):

"zstd/lib/compress",

Which would emulate how the makefile interprets ZSTD_LIB_COMPRESSION=0

https://github.com/facebook/zstd/blob/88b7088d2e4686bb383e1691e28d02399ec1ff17/lib/Makefile#L32-L34

@gyscos
Copy link
Owner

gyscos commented Jan 27, 2023

It's just difficult to do in a backward compatible way.

@NobodyXu
Copy link
Contributor Author

Yes, that's also my first thought.

Though TBH I'm worried about its adoption, since most libraries I use (tar, async-zip-rs) I use supports both building archive and extracting, so they likely will enable both at the same time.

@NobodyXu
Copy link
Contributor Author

It's just difficult to do in a backward compatible way.

Yeah it definitely needs a breaking release to add new feature flags that disables existing functionalities.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Jan 27, 2023

@gyscos Is it possible to have a new env/feature flag, when enabled, replace the zstd/lib/compress with an implementation that always returns an error for "not supported"?

I know this is also breaking change in terms of the API behavior, but at least it won't directly break the API hence don't need direct support in tar/async-zip-rs, etc, and as long as the compression API is not actually used, it will run just fine.

But that does sound like hacking together a solution instead of the proper one.

@gyscos
Copy link
Owner

gyscos commented Jan 27, 2023

Adding a "negative" feature (no-compress) to remove stuff is not technically a breaking change since no one could be using this feature at this point - it just goes against the additivity of features and could cause confusing build errors with conflicting sub-dependencies.

Also potentially breaking stuff at runtime sounds worse than a build error, especially if it can come from unusual causes like env variable or nested dependency.

It's best just to bump the version and bite the churn.

@NobodyXu
Copy link
Contributor Author

Yes, I agree that an additive feature and build-time failure is better.

It's best just to bump the version and bite the churn.

I would prefer a new minor release after new version of cc released before having a new major release, which would likely quite some time before dependents adopt it.

@NobodyXu
Copy link
Contributor Author

@nickbabcock cargo-bins/cargo-binstall#817 enabled cross-lang-fat-lto in cargo-binstall and is able to reduce >50% of the final binary size on linux.

We haven't figured out for MacOS and Windows yet.

I suppose the reduce of size mainly comes from removal of zstd compression related code since it is unused but there could be other stuff as well.

@nickbabcock
Copy link

nickbabcock commented Feb 20, 2023

Nice, I'm seeing a pure zstd decompressor in Wasm take up around 40-50 kB (gzipped), which agrees with the >50% figure you see too. Excellent job. I'm not sure how much more you think is left to shave off, but I think it's pretty close for one to be able to recommend zstd for the web without too many reservations.

For reference, I looked at the size of @oneidentity/zstd-js and the size of zstd-codec and they were 546 kB and 818 kB (both gzipped) respectively, so I think an implementation based on this could be enticing.

EDIT: @hpcc-js/wasm comes close with 117 kB

@NobodyXu
Copy link
Contributor Author

Thanks!

lto is really the most effective way of shaving off unused bits, but I haven't figured out how to do this for MacOS and Windows yet...

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

No branches or pull requests

3 participants