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

Switch from adler32 to adler crate #83

Merged
merged 1 commit into from
Jun 28, 2020
Merged

Switch from adler32 to adler crate #83

merged 1 commit into from
Jun 28, 2020

Conversation

jonas-schievink
Copy link
Contributor

The adler crate is my clean-room reimplementation of Adler-32. The adler32 crate uses the Zlib license, while adler uses MIT/Apache-2.0 (or 0BSD), which makes integration into the standard library easier.

adler is also faster in the benchmarks I've made so far, although that isn't reflected in the miniz_oxide benchmarks (which in fact seemed to slightly regress on my system, but that might just be noise).

@oyvindln
Copy link
Collaborator

This seems reasonable, will include this with the 0.4 release. Will this change work as is with no-std enabled as well?

@jonas-schievink
Copy link
Contributor Author

Yes, it should. adler is #![no_std] if default-features = false is used.

@oyvindln oyvindln merged commit ad0f8fe into Frommi:master Jun 28, 2020
@oyvindln
Copy link
Collaborator

As for performance differences, adler32 calculation is very fast in general, so maybe any difference is not significant enough to show in most comparisons. Maybe you could try compressing with custom settings and use only stored blocks, RLE or huffman-only to clearer show the difference.

@Shnatsel
Copy link
Contributor

Shnatsel commented Jul 4, 2020

FWIW I did see some cases where adler32 calculated dominated the execution time: #76

Past me made the mistake of not including the testcases in the bug report 😞

@Shnatsel
Copy link
Contributor

Shnatsel commented Jul 4, 2020

Hmm, adler crate is about to add unsafe code: jonas-schievink/adler#2

My PR for #![forbid(unsafe_code)] in adler was rejected because of that: jonas-schievink/adler#3

Since both miniz_oxide and adler32 crates currently declare #![forbid(unsafe_code)], I'd argue that there needs to be a really good reason to introduce unsafe code - at least some demonstrated performance improvements and not just "it uses a different license". I'm not convinced MIT/Apache is better than Zlib - the latter is actually very permissive because it doesn't require attribution.

@jonas-schievink
Copy link
Contributor Author

I'm not adding unsafe code for fun, the PR you linked does actually improve performance considerably. The only reason it uses unsafe is because [T]::align_to is unsafe. It currently uses it unsoundly, but I haven't reviewed the PR yet (and the unsoundness is only theoretical).

@Shnatsel
Copy link
Contributor

Shnatsel commented Jul 4, 2020

I'm not questioning whether the addition of unsafe code right for adler, I'm questioning whether the switch to adler is right for miniz_oxide at this point in time.

@jonas-schievink
Copy link
Contributor Author

Ah, I see.

I'm not convinced MIT/Apache is better than Zlib - the latter is actually very permissive because it doesn't require attribution.

The only reason for using MIT/Apache is to ease integration into libstd. adler also allows usage under 0BSD, and that is normally my preferred license, but I was asked to use MIT/Apache to simplify the legal process. I agree this shouldn't be necessary, but oh well.

@oyvindln
Copy link
Collaborator

oyvindln commented Jul 5, 2020

I would be very wary of depending on anything using [T]::align_to, as mentioned by the documentation, it's very easy to end up with undefined behaviour, even though the usage may look fine at first glance. Keep in mind that usage of zlib decoding frequently involves processing data from untrusted sources. Maybe there are some safer approaches to allow auto-vectorization.

Alternatively, maybe you could add it as an optional feature or something.

@Shnatsel
Copy link
Contributor

adler crate has achieved the desired performance gains without resorting to unsafe code and introduced !#[forbid(unsafe_code)] pragma, so I retract my objection.

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