switch to zlib-rs by default#542
Conversation
c1edd9f to
19bc922
Compare
There was a problem hiding this comment.
Pull request overview
This PR switches flate2’s default pure-Rust backend from miniz_oxide to zlib-rs, updates the minimum supported Rust toolchain accordingly, and adjusts CI to validate additional backend configurations.
Changes:
- Make
zlib-rsthe default backend viarust_backendand bumpzlib-rsto0.6.3. - Raise
rust-version(and CI minimum toolchain) to Rust1.75.0. - Update CI to explicitly test the
miniz_oxidebackend without default features, and modernizeio::Errorconversions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/mem.rs |
Uses io::Error::other(...) for converting compression/decompression errors into io::Error. |
Cargo.toml |
Switches default backend selection to zlib-rs, bumps rust-version, and updates the zlib-rs dependency version. |
.github/workflows/main.yml |
Updates minimum toolchain version, adds a miniz_oxide CI test run, and renames/adjusts a minimum-build job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| license = "MIT OR Apache-2.0" | ||
| readme = "README.md" | ||
| rust-version = "1.67.0" | ||
| rust-version = "1.75.0" |
There was a problem hiding this comment.
rust-version is being bumped to 1.75.0. Per the repository’s MSRV policy in README, when rust-version increases the next release should be a minor version so affected users can pin; please ensure the release/versioning plan reflects this (or document why it’s not needed).
|
Thanks a lot! As early feedback, I always like to see Git-project level commit message and contain detailed reasoning, for instance, to explain why the MSRV bump is needed. When it's time, probably also why it's a patch or breaking change. I can also do that when the time comes. |
|
If you are going to do this imo it should be a major version bump to 2.0 so it's an explicit upgrade for projects depending on flate2. |
In what way is it a breaking change? The API is compatible, and every crate that compiles will continue to compile. |
19bc922 to
2a630b1
Compare
|
Would it be possible to bump miniz_oxide to 0.9 here as well? Not sure about MSRV though. (I can of course open a separate PR, but then there will either be conflicts in a few locations or I'll have to keep track of when this is merged...) |
|
This won't be merged for a while. I think you can just open a PR to update the miniz_oxide version (MSRV is fine based on crates.io), and also bump the flate2 version. similar to https://github.com/rust-lang/flate2-rs/pull/490/changes. |
It's not an API break but given the very large user base of flate2, you still want to be careful with such a major change and make sure users of the crate are aware. The crates have different safety guarantees and the output will vary a little depending on back-end (which in theory someone downstream could depend on even though they probably shouldn't). |
|
I think this PR should be switched so that zlib-rs has lower precedence than any other backend. It is currently the case that the default backend has the lowest precedence, which means a downstream user can override the backend to any other one just by enabling the relevant feature. After this PR, Another thing that might not be obvious is that while making this change isn't API breaking, reverting this PR in a subsequent release would be API breaking. That's because it makes the |
That's the only thing that determines whether something needs a major version bump.
Yes, that's the intention here: post the draft PR, communicate it widely, wait a while to make the change. |
(EDIT: ignore my comment here, withdrawn below) On the surface, that would seem reasonable. However, as I understand it, the primary reason to use Dependencies really shouldn't be able to affect this; the top-level crate should be making this choice. But we don't yet have a way to handle that. The only reason we need a default feature at all is that we can't pull in a dependency otherwise; without that, if you depend on a library that depends on flate2, you would have to explicitly set a backend yourself or get a compilation error. One of these days I hope we have proper mutually-exclusive features with the ability for the top-level crate to always override the chosen feature. |
|
What's the harm in giving all other backends higher-than-default precedence? Yes, anyone who wants to purge zlib-rs from their dependency tree needs This also potentially puts downstream crate authors in an awkward position if a few of their users want to use miniz_oxide. They either ignore the request, make the decision for all their users to prefer miniz_oxide, or wire though their own Cargo feature to expose both options. I should probably also add that part of my interest in this PR is avoiding changes that would make future backends harder to add, as the author of fdeflate which might go through that process. (There's no current funding/capacity for adding it as a flate2 backend, but I'm hopeful it may happen longer term) |
One way or another, either people using one backend or the other backend will be unhappy with their dependencies overriding that. If we make zlib-rs higher priority, people who want to use miniz_oxide will be frustrated that anything anywhere in their dependencies using zlib-rs will prevent using miniz_oxide. If we make miniz_oxide higher priority, people who want to use zlib-rs will be frustrated that anything anywhere in their dependencies using miniz_oxide will prevent using zlib-rs.
Interesting! Specialized backends for specific purposes (people have also asked for zopfli) are the kinds of things that make me wonder if we should have a mechanism for supporting more than one backend in a dependency tree. I can easily imagine someone wanting to use fdeflate for their PNGs and other data it particularly shines on, and zlib-rs for other random data, for instance. That's not something we should try to change in this major version of flate2, but I wonder if it would be worth considering, as an alternative to or in addition to having mutually exclusive features. Even with mutually exclusive features, I can imagine someone wanting to use multiple backends within the scope of a set of dependencies. And I can think of architectures that would support that without overhead. That's a problem for another day, though. In the meantime, we're going to have to make a judgement call about which case we want to make easier without going through your entire dependency tree: using zlib-rs, or using miniz_oxide. I stand by the rationale that the use case for miniz_oxide seems to already require going through your dependency tree; "just want to test" can be done other ways. |
|
My understanding is that there's a de-facto rule across the ecosystem that libraries shouldn't override the flate2 backend. As long as libraries pull in flate2 with only the default features, and flate2's default backend is the lowest precedence, then binaries can pick any backend they want. It isn't as clean as a hypothetical mutually-exclusive features API would be, but it works relatively well. Having a backend with lower-than-default precedence upsets that balance. |
|
I stand by the point that it's going to be suboptimal for one or the other backend's users. But nonetheless, on further consideration, the cure is probably worse than the disease here. We lived for years with with having an unneeded Ultimately, we should probably just preserve the structure as we had it before, make the default backend the lowest priority, and wait for mutually exclusive features to solve the underlying problem. |
|
I am opposed to this change until clear evidence on the trade-offs involved is published. Lack of evidence and unclear trade-offsFirst, I believe this change needs far more rigorous evaluation than what is currently presented.
This sentence alone is not compelling evidence. To properly evaluate this change, we would need to see the specific performance numbers on both compression and decompression on a variety of inputs. The benchmarking harness should be made publicly available, so that others can reproduce the results and measure performance on their hardware and on their inputs. Both average and geometric mean should be tracked to avoid a handful of large inputs skewing the results. I believe this change should not proceed until it is evaluated this way. Significantly larger outputs in some compression levelsAnecdotally, I have seen significant regressions in the size of compressed output when switching from miniz_oxide to zlib-rs. There are similar reports by other people, see e.g. trifectatechfoundation/zlib-rs#500 This ties into my first point - no evidence for the size of compressed outputs has been presented. It is therefore plausible that performance improvements are only attained at the cost of lower compression ratio, in which case the switch to zlib-rs by default may be undesirable. Soundness issuesThere are still some known soundness bugs in zlib-rs, see e.g. trifectatechfoundation/zlib-rs#491 As a faithful port of zlib-ng, zlib-rs relies in quite a few We are also in the beginning stages of AI-driven vulnerability discovery (1, 2, 3). If it is anything like the wave of vulnerabilities that followed the creation of AFL in 2014, then now is not the time to roll out more memory-unsafe code in contexts exposed to untrusted data. I believe it is prudent to delay this change until the field settles down again, and sticking to code that's guaranteed to be memory-safe in the meantime. Safety trade-offsWhile I have no doubt zlib-rs is a safety upgrade over zlib-ng (I'd take it over zlib-ng every time!), it appears to be a safety downgrade compared to miniz_oxide. The requirement to expose a C API and keep the data structures ABI-compatible with zlib, the very thing that makes it a drop-in replacement for zlib, also necessitates a multitude of Both flate2 and Rust in general have a history of defaulting to a slow but safe option, and letting people opt into higher performance if they understand the trade-offs. For example, I have not yet seen evidence that the performance gains are compelling enough that they justify sacrificing safety. AlternativesCrucially, I have also not seen evidence that performance improvements cannot be obtained without sacrificing safety. Once a reproducible benchmarking harness is in place, I'd like to propose fdeflate to be evaluated as a potential new default backend. It is 100% safe code and has been used in production in Chromium within PNG decoding for over 6 months, so its correctness and soundness are assured. It is not clear |
Nice. How massive? I'd love to see the numbers. |
I'm still tinkering with this a bit, but here is a benchmarking setup https://github.com/folkertdev/flate2_bench. This uses the Overall I'm seeing a ~30% speedup for inflate and ~3X speedup for deflate at level 6 versus result of `sh run.sh` from the linked repoClangBuiltLinux clang version 22.1.3 (https://github.com/llvm/llvm-project.git e9846648fd6183ee6d8cbdb4502213fcf902a211) rustc 1.96.0-nightly (02c7f9bec 2026-04-10) -- inflate (chunks of 4096 bytes) -- target/release/flate2_bench_miniz_oxide -- deflate level 1 (chunks of 4096 bytes) -- target/release/flate2_bench_miniz_oxide -- deflate level 6 (chunks of 4096 bytes) -- target/release/flate2_bench_miniz_oxide -- deflate level 9 (chunks of 4096 bytes) -- target/release/flate2_bench_miniz_oxide @Shnatsel hopefully those numbers go some way towards having a clearer picture here, though I realize we'd need to run more inputs on more machines (working on it). I'll need some more time to respond to your other points in full. I don't think |
|
I've prototyped a While these results are from just a proof-of-concept, they are in the right ballpark to make a proper |
|
Super exciting! And when reading |
|
That sounds like a good plan to me. |
Totally!
I'll nitpick on this slightly by mentioning that the checksum calculations, that are in a dependency, do (and should) use explicit SIMD, hence unsafe code. But zlib-rs needs unsafe code for allocation (because no custom allocators) and hence needs unsafe code in more places. |
...but as soon as they bump MSRV to 1.87, it should be possible to get rid of much of that |
|
FWIW, for those of us on nightly who don't mind activating a nightly crate feature gate, you can write many algorithms in portable SIMD in safe code (such as adler). You still need multi versioning and sometimes a const lane parameter but it's cleaner to read than you might think. |
|
The checksum algorithms use some very specific instructions like It's really the loads and stores that are unsafe, and even there using e.g. It's a nitpick exactly because I don't think there are actually errors, but because I think it's incorrect to prefer a crate that is all safe code if actually what it does is have a bunch of dependencies that have all of the unsafety. You don't gain anything there. |
|
I've also been experimenting with a version of the adler32 checksum that just uses Edit: Feel free to open issues on that repo if you want to discuss more |
|
@fintelia We use the multiversion crate macro, by one of the leads of the portable SIMD wg, and gate the macro itself on a feature. It works quite well in practice albeit AVX512 use is flaky. |
We aim to make zlib-rs the default implementation in flate2 soon. The switch to zlib-rs (from the current default of
miniz_oxide) massively improves performance.The release enabling zlib-rs by default will be a minor release, and will bump the MSRV from 1.67 (January 2023) to 1.75 (December 2023). It's a significant change, but unless we've missed something it is not a breaking change.
Note that zlib-rs uses a different compression algorithm than
miniz_oxide. That means the output is not byte-for-byte equal, and your application may need to adjust its compression level for an optimal size/speed tradefoff.The current timeline is to give this a couple of weeks to bake. Tentatively we'd make the switch around RustWeek (mid-May).