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

Huge size of the InflateStream::next_state method #34

Closed
RazrFalcon opened this issue Jan 13, 2018 · 10 comments
Closed

Huge size of the InflateStream::next_state method #34

RazrFalcon opened this issue Jan 13, 2018 · 10 comments

Comments

@RazrFalcon
Copy link
Contributor

According to the cargo-bloat the InflateStream::next_state method is a whopping 200KiB. I presume it's because of heavy macro usage because according to cargo-expand this method is being expanded from 405 LOC up to 10000 LOC. libflate on other hand is just 30-60KiB total.

Also, libflate is faster, so it may be a good idea to use it instead of inflate.

I have no idea how this code work so I can't refract it by myself.

@eddyb
Copy link
Member

eddyb commented Jan 13, 2018

Yeah, this was intentional, although there might be ways to reduce it. I didn't know about libflate.
This library should match pretty much any other good one in the dynamic huffman coding mode.
If you want to, you could benchmark with this uncommented:
https://github.com/PistonDevelopers/inflate/blob/a39b7391f4284999cd77db1d0b25dc3584961788/src/lib.rs#L818-L845

Which uses the dynamic implementation for the fixed huffman codes (there could be a further optimization by using Cow and precomputing the tries).
The resulting library should be pretty small.

@eddyb
Copy link
Member

eddyb commented Jan 13, 2018

FWIW nowadays I'd do an on-line (aka "non-blocking") design much differently, by using generators directly (sadly they don't work on stable), which would result in easier to write and more contributor-friendly code, compared to manual state-machines.

@RazrFalcon
Copy link
Contributor Author

I've tried to uncomment this code, but nothing is changed. Nor size, not speed.

I've also tried to refract this code by myself, but there are too many macros to understand it.

@oyvindln
Copy link
Contributor

oyvindln commented Jan 13, 2018

There is also miniz_oxide (a rust port of miniz), which is now used as an alternate rust back-end in flate2.

I have a tool here that I originally wrote for deflate to compare deflate implementations, maybe you would find it useful.

Due to the lack of a switch fallthrough in rust, state machines will easily end up large.

@eddyb
Copy link
Member

eddyb commented Jan 13, 2018

@RazrFalcon oh I guess you'd also need to remove the BlockFixed enum variant and all of this:
https://github.com/PistonDevelopers/inflate/blob/a39b7391f4284999cd77db1d0b25dc3584961788/src/lib.rs#L865-L962

@RazrFalcon
Copy link
Contributor Author

@oyvindln currently, I'm only interested in the crate size. Performance is irrelevant. Also, I only need decompression.

@RazrFalcon
Copy link
Contributor Author

RazrFalcon commented Jan 13, 2018

@eddyb Wow! All tests are passed and the method size is down to 45.6KiB (from 200KiB)! Performance is mostly the same, maybe a bit faster.

Bench results:

-                libflate: elapsed=1.461629s, size=288632530
- libflate (non-blocking): elapsed=2.490778s, size=288632530
-                  flate2: elapsed=1.129617s, size=288632530
-                 inflate: elapsed=1.846745s, size=288632530

Original code has around 1.9s.

But why this 'method' is commented out?

@eddyb
Copy link
Member

eddyb commented Jan 13, 2018

@RazrFalcon It wasn't faster when I tried it! I left it commented out in case we'd eventually optimize the dynamic path further for this usecase. Feel free to open a PR 🎉

@RazrFalcon
Copy link
Contributor Author

Will do.

@RazrFalcon
Copy link
Contributor Author

Closed by #35

nwin added a commit that referenced this issue Feb 3, 2018
Publish 0.3.4.

Fixes issue #34.
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

3 participants