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

New ISA-L backend? #433

Closed
milesgranger opened this issue Oct 4, 2024 · 5 comments
Closed

New ISA-L backend? #433

milesgranger opened this issue Oct 4, 2024 · 5 comments

Comments

@milesgranger
Copy link

Hi there,

I've recently picked up an old project and started a gzip, deflate, zlib implementation using ISA-L here: https://github.com/milesgranger/isal-rs. As promised by isa-l, it has good performance.

I'm curious if there would be interest in adding it as another back-end to flate2? Either by merging it into this repository, a conditional dependency or perhaps it should be just be left unrelated.


Note I know it compiles and tests well on a few 32bit platforms, but seems they're dropping 32bit support:

@Byron
Copy link
Member

Byron commented Oct 4, 2024

I think this would be a question for @jongiddy.
If I would be answering, I'd think less is more when it comes to C.

@jongiddy
Copy link
Contributor

jongiddy commented Oct 5, 2024

Merging isal-rs into flate2 does not really work. At a high-level both crates provide similar functionality and I would be fine with them staying as separate crates. If isal is to be added as another back-end, it should be done as a conditional compilation, with the isal-rs crate as an optional dependency, similar to the other zlib backends.

To integrate through the ffi interface flate2 only needs the crate to provide the deflate algorithm, with flate2 handling the gzip and zlib headers. It might be possible to integrate at a higher level to use isal-rs header handling but this would be a bigger change to the flate2 crate.

I would be interested in seeing a PR with isal as a back-end. The criteria for acceptance would be how easily it fits with the existing ffi interface and how many changes are required there, and whether any changes are required in the other directories.

@oyvindln
Copy link
Contributor

oyvindln commented Oct 17, 2024

Would there be any advantage to using this library over the current zlib-ng backend? Handling the differences in backends is already causing some challenges so adding another C one would result in even more difficulty and we already have a very optimized C backend in zlib-ng. (and also the work in progress zlib-ng rust port if one wants something rust-based and is okay with a fair bit of unsafe code.) The only special feature from what see isa-l has is support for multi-threaded compression though I don't know how much help that is when used via streaming compression apis (there are specialized deflate libraries like libdeflater that can take advantage of use cases where one doesn't need streaming support and can allocate the full output buffer up front.)

From some quick testing it seems the "best" compression level in this library compresses much worse than the default levels in zlib-ng, miniz and standard zlib (none of which are exactly equal) as well so comparing the compression speed of isa-l "best" level vs even the "default" level of any of the current backends don't seem to be very representative. (also there may be some slight overhead from flate2 vs using the underlying libraries directly I think)

@oyvindln
Copy link
Contributor

Judging by this it seems it might be mainly focused on fast/low compression levels and using the stack rather than dynamically allocating memory? so maybe it's better served by just using it directly if one needs those things instead of getting since it already has a pretty similar API.

@milesgranger
Copy link
Author

milesgranger commented Oct 17, 2024

Would there be any advantage to using this library over the current zlib-ng backend?

The benchmarks are using level 3 compression (ISA-L supports 0, 1, 3) and zlib-ng.

ISA-L is typically 2-3x faster than flate2 with zlib-ng using the same compression level. That was the proposed advantage; if people are good w/ one of these supported levels then it's faster, that's all.

ISA-L vs zlib-ng : level 3 gzip
roundtrip/isal/alice29.txt
                        time:   [752.21 µs 755.62 µs 759.85 µs]
roundtrip/flate2/alice29.txt
                        time:   [1.6277 ms 1.6318 ms 1.6364 ms]

roundtrip/isal/html     time:   [364.65 µs 366.62 µs 369.14 µs]
roundtrip/flate2/html   time:   [434.40 µs 435.38 µs 436.66 µs]

roundtrip/isal/mr       time:   [40.955 ms 41.026 ms 41.110 ms]
roundtrip/flate2/mr     time:   [87.171 ms 87.565 ms 87.993 ms]

roundtrip/isal/sao      time:   [39.207 ms 39.360 ms 39.539 ms]
roundtrip/flate2/sao    time:   [109.40 ms 109.74 ms 110.13 ms]

roundtrip/isal/dickens   time:   [49.982 ms 50.202 ms 50.441 ms]
roundtrip/flate2/dickens time:   [109.78 ms 109.97 ms 110.17 ms]

roundtrip/isal/webster  time:   [236.03 ms 236.65 ms 237.34 ms]
roundtrip/flate2/webster
                        time:   [388.43 ms 389.28 ms 390.34 ms]

io::Read Gzip Encoder/isal/alice29.txt
                        time:   [525.34 µs 527.48 µs 530.25 µs]
io::Read Gzip Encoder/flate2/alice29.txt
                        time:   [1.2067 ms 1.2104 ms 1.2151 ms]

io::Read Gzip Encoder/isal/html
                        time:   [278.29 µs 279.31 µs 280.47 µs]
io::Read Gzip Encoder/flate2/html
                        time:   [318.45 µs 319.92 µs 321.58 µs]

io::Read Gzip Encoder/isal/mr
                        time:   [27.645 ms 27.743 ms 27.863 ms]
io::Read Gzip Encoder/flate2/mr
                        time:   [66.752 ms 66.999 ms 67.290 ms]

io::Read Gzip Encoder/isal/sao
                        time:   [23.977 ms 24.047 ms 24.129 ms]
io::Read Gzip Encoder/flate2/sao
                        time:   [92.505 ms 92.704 ms 92.980 ms]

io::Read Gzip Encoder/isal/dickens
                        time:   [36.097 ms 36.225 ms 36.366 ms]
io::Read Gzip Encoder/flate2/dickens
                        time:   [89.979 ms 90.109 ms 90.267 ms]

io::Read Gzip Encoder/isal/webster
                        time:   [154.72 ms 155.14 ms 155.63 ms]
io::Read Gzip Encoder/flate2/webster
                        time:   [300.54 ms 301.62 ms 302.89 ms]

io::Write Grzip Encoder/isal/alice29.txt
                        time:   [509.12 µs 511.19 µs 513.73 µs]
io::Write Grzip Encoder/flate2/alice29.txt
                        time:   [1.2760 ms 1.2788 ms 1.2826 ms]

io::Write Grzip Encoder/isal/html
                        time:   [264.40 µs 265.27 µs 266.28 µs]
io::Write Grzip Encoder/flate2/html
                        time:   [340.11 µs 340.66 µs 341.35 µs]
Found 6 outliers among 50 measurements (12.00%)

io::Write Grzip Encoder/isal/mr
                        time:   [28.592 ms 28.662 ms 28.746 ms]
io::Write Grzip Encoder/flate2/mr
                        time:   [71.242 ms 71.384 ms 71.554 ms]

io::Write Grzip Encoder/isal/sao
                        time:   [25.262 ms 25.381 ms 25.516 ms]
io::Write Grzip Encoder/flate2/sao
                        time:   [91.647 ms 91.956 ms 92.331 ms]

io::Write Grzip Encoder/isal/dickens
                        time:   [37.828 ms 37.935 ms 38.050 ms]
io::Write Grzip Encoder/flate2/dickens
                        time:   [89.638 ms 89.823 ms 90.032 ms]

io::Write Grzip Encoder/isal/webster
                        time:   [156.89 ms 157.27 ms 157.73 ms]
io::Write Grzip Encoder/flate2/webster
                        time:   [299.39 ms 300.50 ms 301.67 ms]

with that said, seems the consensus is not to move forward with adding it as a backend, thank you all for your input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants