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

Introduce reset policy to control InflateState::reset #91

Merged
merged 2 commits into from
Aug 15, 2020

Conversation

DoumanAsh
Copy link
Contributor

This change introduce customization to reset, allowing user to decide how to perform reset.
Following approaches are introduced:

  • Full reset where user wants to change data format and zero memory;
  • Zero reset where user is only interested in zeroing internal state;
  • Minimal reset that only resets minimum necessary (without zeroing buffer which is expensive)

Also corrected reset tests to actually re-perform decompression to test that reset is ok.

Closes #89

P.s. honestly the whole API needs re-work, but meanwhile it would be useful to allow user reset as they see fit.

@oyvindln
Copy link
Collaborator

oyvindln commented Aug 12, 2020

I'm thinking this approach is a bit nicer than the other PR #92. And yeah, API needs rework. Only thing I would add is a warning to the comment of the potential risk of not zeroing data when dealing with untrusted input. (Haven't found any bug that would end up giving out a read from old data through testing or fuzzing so it's likely not normally an issue, but it's not being verified statically as of now.)

Does this one work fine for your needs @Byron ?

@Byron
Copy link

Byron commented Aug 12, 2020

I absolutely love it, that will work perfectly!
Thanks so much!

@Byron
Copy link

Byron commented Aug 13, 2020

For gitoxide, using miniz_oxide as fast and safe pure-Rust implementation will always be the way to go. However, after having seen this unscientific comparison it's clear that for compression performance, gitoxide eventually needs access to zlib-ng at least behind a feature toggle. They are now investigating ways to speed up a particular git requirement that involves finding the end of a compressed stream as quickly as possible, and I thought you folks might be interested in taking a look as well.

@DoumanAsh
Copy link
Contributor Author

@oyvindln Updated, let me know if you need anything fix

@oyvindln oyvindln merged commit 1f95a16 into Frommi:master Aug 15, 2020
@DoumanAsh DoumanAsh deleted the inflate_state_reset branch August 15, 2020 12:28
@Shnatsel
Copy link
Contributor

If you want maximum zlib performance on a bounded set of data, you should look into libdeflate. It has Rust bindings too: https://crates.io/crates/libdeflater

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.

[Q&A] InflateState reset has measurable costs when decompressing millions of objects
4 participants