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

Don't leak flate2 as an implementation detail by default #564

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Shnatsel
Copy link
Contributor

This is a hopefully simpler alternative to #563 that makes easy things easy and hard things possible.

@Shnatsel Shnatsel requested a review from fintelia December 30, 2024 21:35
@fintelia
Copy link
Contributor

A weird aspect about the flate2 crate is that enabling specific backend features doesn't guarantee you get them. If somewhere else in the dependency tree enables zlib-ng, then this PR will actually use that instead of zlib-rs. And if somewhere else enables cloudflare_zlib, the crate won't compile at all (at least until a bugfix gets rolled out).

@Shnatsel
Copy link
Contributor Author

Based on my experience of messing with zlib backends corpus-bench, zlib-sys will default to zlib if one crate asks for zlib and another for zlib-ng. But zlib-rs goes through its own bindings so it's not even participating in the fight over what ends up being the zlib that gets linked.

But there's probably also its own order of priority somewhere inside flate2, which adds another layer of complexity to this.

@fintelia
Copy link
Contributor

Yeah, the decision inside flate2 happens here and here. If those priorities result in zlib-sys being selected, then zlib-sys determines what actual zlib implementation is used.

I think part of the challenge is designing an API that makes sense both with the current state of having compression levels 1-9 always backed by flate2 and a potential future where fdeflate is always used with the default features and we want flate2 to be opt-in.

@Shnatsel
Copy link
Contributor Author

This PR makes the most sensible combination of flate2 with zlib-rs opt-in.

We can add an explicit opt-in to generic flate2 later as a separate feature if (a) we do end up replacing all levels with fdeflate and (b) anyone actually asks for a generic flate2 implementation. This PR keeps that as a possibility without sacrificing ergonomics today.

@fintelia
Copy link
Contributor

I think the key difference is that this PR adds a feature which opts-in use of zlib-rs via flate2rs for all PNG encoding while mine only adds a variant to the DeflateCompression enum that lets advanced users specifically pick it. Where possible, I think it is preferable to control options via crate API surface rather than global feature flags.

It also isn't great that a user can request zlib-rs and end up getting zlib-ng instead because some other library in their dependency tree set the relevant flate2 feature, but I don't think there's much we can do about that without more drastic changes. Not sure how much we need to document that pitfall.

@Shnatsel
Copy link
Contributor Author

I think the key difference is that this PR adds a feature which opts-in use of zlib-rs via flate2rs for all PNG encoding while mine only adds a variant to the DeflateCompression enum that lets advanced users specifically pick it. Where possible, I think it is preferable to control options via crate API surface rather than global feature flags.

I generally agree. I just find the API in #563 designed around future hypotheticals at the expense of being clear today. And there is plenty we could do later to make the API more explicit - e.g. start lowering the high-level Compression into other implementations but keep DeflateCompression::Level lowering into flate2, or only lower DeflateCompression::Level into flate2 when the zlib-rs feature is enabled, or switch Level over to the new implementation but add another variant to explicitly opt into flate2... We have lots of options there.

It also isn't great that a user can request zlib-rs and end up getting zlib-ng instead because some other library in their dependency tree set the relevant flate2 feature, but I don't think there's much we can do about that without more drastic changes. Not sure how much we need to document that pitfall.

I suppose it's worth noting. zlib-rs has been making strides in performance and I hope they'll become the preferred flate2 backend eventually.

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

Successfully merging this pull request may close these issues.

3 participants