-
Notifications
You must be signed in to change notification settings - Fork 113
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
Comments
Perhaps we should just let the users specify |
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. |
@nickbabcock You can use 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 |
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
But I am not seeing any difference in output. Maybe it's because the zstd-sys is always compiling files found in the |
@nickbabcock You are absolutely right on this and that means #189 is actually changing the def to no-op. |
@nickbabcock In that case, I guess the only thing that couild help is cross language LTO #191 |
@NobodyXu , my first thought would be to guard adding the files related to compression with a cargo feature (like zstd-rs/zstd-safe/zstd-sys/build.rs Line 94 in bd75cb4
Which would emulate how the makefile interprets https://github.com/facebook/zstd/blob/88b7088d2e4686bb383e1691e28d02399ec1ff17/lib/Makefile#L32-L34 |
It's just difficult to do in a backward compatible way. |
Yes, that's also my first thought. Though TBH I'm worried about its adoption, since most libraries I use ( |
Yeah it definitely needs a breaking release to add new feature flags that disables existing functionalities. |
@gyscos Is it possible to have a new env/feature flag, when enabled, replace the 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 But that does sound like hacking together a solution instead of the proper one. |
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. |
Yes, I agree that an additive feature and build-time failure is better.
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. |
@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. |
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 EDIT: @hpcc-js/wasm comes close with 117 kB |
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... |
Upstream zstd provides build macro
ZSTD_LIB_COMPRESSION
to disable compression support, which will then significantly reduce the build time of thezstd-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 viadefault-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.
The text was updated successfully, but these errors were encountered: