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

Change default backend to zlib-rs instead #469

Open
Xuanwo opened this issue Feb 27, 2025 · 5 comments
Open

Change default backend to zlib-rs instead #469

Xuanwo opened this issue Feb 27, 2025 · 5 comments

Comments

@Xuanwo
Copy link
Contributor

Xuanwo commented Feb 27, 2025

Hi, I'm starting this thread to discuss whether it would be a good idea to switch the default backend to zlib-rs, as it outperforms both zlib-ng and miniz_oxide.

Or at the very least, we can mention this in our README and revise any statements referring to a purely Rust backend.

I'm willing to perform this change.

@Byron
Copy link
Member

Byron commented Feb 27, 2025

I think very uncontested approach could be a documentation update, both in the README as well as in the crate, to advertise zlib-rs more.

As for switching the default backend, that's a big step and I can't predict if it's the right choice. To me it's about compatibility more than it is about performance - if I'd know that zlib-rs works similarly correct even in the edge-cases, then it should certainly be done.

But that one will only know once it's too late.

In theory, crater could be used to conduct such an experiment. If we had that, I think it would be easy to justify such a step.
The authors of zlib-rs and @jongiddy should certainly also be involved then.

@Xuanwo
Copy link
Contributor Author

Xuanwo commented Feb 27, 2025

Makes sense. I will update the README first.

@oyvindln
Copy link
Contributor

oyvindln commented Feb 27, 2025

The compression behaviour and safety guarantees are not the same between the two - whether those differences would be acceptable in a major version bump is something you would have to decide I guess:

  • miniz_oxide uses no unsafe code (or optionally a small bit of unsafe for easily auditable SIMD code for checksums via simd-adler32 if the simd feature is enabled)

  • zlib-rs makes use of quite a lot of unsafe not just for SIMD code for checksums

  • zlib-rs (and zlib-ng) trades some compression ratio for speed so it doesn't compress as well as standard zlib and miniz_oxide. This may or may not be desirable depending on application so it's somthing that has to be considered. When testing cloudflare-zlib it seemed like it was somewhere inbetween zlib and zlib-ng/zlib-rs on compression ratio and speed so maybe does some speed/ratio tradeoffs but not all (zlib and miniz/miniz_oxide also has some slight differences in compression ratio characteristics but that's more a small variance that can go slightly either way due to slight difference in implementation rather than better/worse).

Something I think we can at least consider is to drop the zlib-ng backend (and proxy the zlib-ng feature to zlib-rs) provided zlib-rs implement all the features of it since the two should then be basically equivalent.

Not quite sure about the cloudflare-zlib one since it's not strictly equivalent to zlib-ng though it is much less used and has much limited platform support compared to other backends so maybe it could be okay to deprecate that too in favour of zlib-rs even though it isn't strictly the same just to simplify things.

@Byron
Copy link
Member

Byron commented Feb 28, 2025

Thanks a million for chiming in! I found the comment very insightful.

Something I think we can at least consider is to drop the zlib-ng backend (and proxy the zlib-ng feature to zlib-rs) provided zlib-rs implement all the features of it since the two should then be basically equivalent.

Maintaining it thus far has been easy enough, and I think having both available is still valuable. However, I wouldn't rule out that in the far future there truly is no need for the C-implementation anymore.

Not quite sure about the cloudflare-zlib one since it's not strictly equivalent to zlib-ng though it is much less used and has much limited platform support compared to other backends so maybe it could be okay to deprecate that too in favour of zlib-rs even though it isn't strictly the same just to simplify things.

That seems more interesting to me, but probably for the wrong reasons. I can't really tell what the cloudflare version is good for in the light of zlib-ng and zlib-rs. It also seems far less active. Then again, this makes maintenance even easier, so it seems like an option to keep around as well even if just for the sake of it.

@jongiddy
Copy link
Contributor

jongiddy commented Mar 2, 2025

I see no problem with keeping zlib-ng and cloudflare-zlib backends while they apparently continue to work and while we can get support for minor problems.

If we do need to drop support for either backend we should not proxy the feature to another backend. It must be clear to callers that we have dropped support and that they will need to select another backend.

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

4 participants