add zlib-rs support via the libz-rs-sys C api for zlib-rs

[folkertdev]

This PR adds (experimental) support for using flate2 with the zlib-rs backend. The zlib-rs crate is a pure-rust implementation of the zlib api that makes use of SIMD to achieve performance close to zlib-ng (currently 5% to 10% slower).

• ISRG project page
• libz-rs-sys docs

For the moment, the libz-rs-sys C api is used for the integration. At some point in the future, zlib-rs will get a more convenient rust api that will require less (hopefully no) unsafe code.

cc @joshtriplett

[folkertdev]

the trick is that because zlib-rs can do equality comparisons on the function pointer, it can know when its built-in allocator is used, and this allocator guarantees 64-byte alignment. The allocation of zfree and zalloc below is not needed (and that implementation is less efficient to deal with allignment).

[folkertdev]

in the case of zlib-rs, the allocator logic is unused. That is an error for this crate, so it's only conditionally compiled now.

[folkertdev]

we should eventually be able to compile on mingw, but the current release doesn't, and we don't test for it, so disabling for now.

[joshtriplett]

This will need fixing before we could consider it as a potential default, but it's fine for now.

[Byron]

Thanks a lot for contributing what could one day be the new default backend, and replace minizoxide in this space.

As CI is failing on Windows, I think that might be a good reason to run tests on Windows (and maybe more) platforms as well.

Further, when looking at the dependencies of libz-rs-sys, I was surprised that zlib-rs has what looks like test-dependencies as part of its normal dependencies. It's clear that these are optional and behind features, but I would have thought that dev-dependencies work fine for unit-tests as well. It somehow worried me that this (what seems like a) workaround was necessary, and I hoped there is another way. Maybe APIs could be made public behind a hidden feature toggle, and a separate test-only crate could carry all the dependencies? Then the separation would be clear, and maybe some test-specific code could be extracted into a separate space. This comment is probably not relevant to this PR though, merely a side-note.

I would definitely wait for CI to be fixed, and a second reviewer.

[folkertdev]

thanks for the review

• we're working on windows support (already merged into our main branch but needs a little cleanup still). Should be ready soon.
• the dependency situation is kind of nasty. From what I observe, it seems that if crate A has a dev-dependency on crate B, then crate B is not compiled with cfg(test) to be true. Our crate B needs to expose/derive things for testing in crate A. So, we haven't yet found a better solution than the feature flags, but it is far from ideal, I'd really like to not be doing that.
• we've been in regular contact with @joshtriplett so he'd be a natural second reviewer here.

[Byron]

That all is great to hear!

Regarding the testing, thanks for the explanation - I understand now why these test-dependencies can't be dev-dependencies.

Besides that, I think with Josh approving this PR, it can be merged.

[oyvindln]

Thanks a lot for contributing what could one day be the new default backend, and replace minizoxide in this space.

Well it would have been nice to have some help to improve miniz-oxide rather than creating yet another rust deflate implementation I guess... but i digress

[Byron]

My apologies, that callout what unnecessary, and lacked the appreciation that is due for it as the backend which just works. I got carried away by the pure-Rust-powered zlib-ng like performance as default here, it's something that gitoxide benefits from a lot.

But maybe this could be a starting point for eventually bringing both projects together (without having any idea if this is technically or even legally possible).

[oyvindln]

Ah sorry, maybe I came across as overly grumpy, wasn't meant for you or anyone in particular, it's just happened a few times now. I kinda expect miniz-oxide to be obsoleted eventually unless someone else comes along and picks it up as I'm not really in a position to actively put in a lot of work into it myself - as of now I guess zlib-rs has some different aims though as it makes rather heavy use of unsafe code and probably has to keep doing so as long as explicit SIMD requires it.

[Byron]

Right, thanks for clarifying this! If miniz-oxide is 100% safe Rust, there is value in that alone, independently of performance. Then, even if zlib-rs is equivalent functionally and regarding portability, figuring out what the default backend should be wouldn't be straightforward anymore and probably stay as is :).

[folkertdev]

We certainly have no ill will versus miniz-oxide: it's simple, small, consistent between platforms, no unsafe besides some C api stuff, and performs slightly better than the original zlib, which is perfectly adequate for many use cases.

zlib-rs is more ambitious though: it aims to match zlib-ng performance and properly provide the full zlib api, meaning that we can create a drop-in replacement for the zlib dynamic library.

I'm not sure a generic, shared deflate implementation works, but we're happy to share code/knowlegde. It is our impression however that the majority of performance improvements stem from the explicit use of SIMD instructions, which miniz-oxide does not seem to want to use?

---

We believe to have fixed the windows issues. I'll try that here on CI now by setting the dependency to a git branch again. This should not be merged in that state. I'll report back after we've pushed another release to crates.io and think this is ready.

[oyvindln]

It is our impression however that the majority of performance improvements stem from the explicit use of SIMD instructions, which miniz-oxide does not seem to want to use?

We don't want to use any unsafe code in miniz_oxide (external C api requires it of course but that's a separate thing), and as far as I know at least there isn't any way to use explicit SIMD without unsafe on stable rust (the portable simd API could help solve that but it seems to have been stalled as of now). There may be some crates out there that implement something similar though those again will be using unsafe internally.

To what extent explicit SIMD is needed to reach performance parity and to what extent it can be achieved with autovectorization with the right code layout I don't know.

In any case zlib-rs will now provide a rust alternative to using the C implementation of zlib-ng that's close to the same performance for users that are okay with the safety tradeoffs (and that hopefully won't be patched out by debian maintainers like zlib-ng is)

[folkertdev]

The explicit SIMD is essential, e.g. https://github.com/memorysafety/zlib-rs/blob/main/zlib-rs/src/deflate/compare256.rs#L170, LLVM just can't see that idea with the wide comparison and bit counting.

There are a couple of tricks around breaking dependency chains in the bit reader that you may benefit from. E.g. https://github.com/Frommi/miniz_oxide/blob/master/miniz_oxide/src/inflate/core.rs#L423 the if here is very prone to misprediction. The optimized implementations just always add more bits to the buffer, e.g. we https://github.com/memorysafety/zlib-rs/blob/main/zlib-rs/src/inflate/bitreader.rs#L103 have no branches in that function at all. A bounds check would be acceptable because it is easy to predict.

What's the story behind removing zlib-ng from the flate2 debian package?

[folkertdev]

We have released version 0.1.1 that fixes various issues on windows and macos (x86 and aarch64). These targets are now tested in our CI so there should be no regressions.

From my side this PR is now done and ready to be merged

[Byron]

Thanks a lot for all your hard work and for seeing it through!

The only concern I have is that this CI is testing more platforms than the zlib-rs CI (even though Windows checks were added now), and I hope that this isn't prone to unexpected breakage here.

Then, I will leave it for @joshtriplett to finalize the review and possibly merge as well.

[folkertdev]

What platforms do we miss? mingw is deliberately omitted for now. I guess we could add a build with the nightly target?

https://github.com/memorysafety/zlib-rs/blob/main/.github/workflows/checks.yaml#L25

[Byron]

Actually, I confused this repo with the one from libz which builds on many, many different platforms. Here it's indeed only mingw that isn't tested in zlib-rs. I think my argument could still apply, but much more weakly.

I don't think that nightly should play a role as nightly shouldn't ever break anything (and if it does, it's a bug that will be fixed before it hits stable).

[joshtriplett]

libz-rs-sys has default = ["std", "rust-allocator"]. What's the rationale for disabling default features and then writing them out by hand, here? Defensive programming against future additions to the defaults?

[folkertdev]

yes exactly. it also clarifies that we're explicitly using the rust allocator; that will always be the default when the crate is used from rust, but not when we build a C dynamic library.

[joshtriplett]

Looks good to me! Wrote a comment about the feature usage, but I don't think that's a blocker to merging.

[folkertdev]

Thanks! would it be possible to make a release with these changes?

[Byron]

Yes, cold you create a PR that bumps the version? I can't self-approve my own PRs hence the need for help :).

[Byron]

A new release is now available: https://github.com/rust-lang/flate2-rs/releases/tag/1.0.29

[torokati44]

Does the README not mention this because it's still experimental (and therefore shouldn't be used "in production")?

[Byron]

That's a good point - it might have been an oversight, and it should probably be mentioned along with a note of being not necessarily production-proven yet.

[torokati44]

I see, thank you. We won't opt into it yet then.