enhancement(deps): Use zlib-rs for much faster zlib decoding/encoding #22533
Conversation
|
|
|
I think adding a changelog to promote this would be cool! These days there is a lot of hype around this https://www.reddit.com/r/rust/comments/1ixt1ei/zlibrs_is_faster_than_c_trifecta_tech_foundation/ |
69c0ca7 to
dbc7c24
Compare
|
Rebased to master. |
|
Test failures: https://github.com/vectordotdev/vector/actions/runs/13676581269/job/38244029801?pr=22533 Does this indicate that this change breaks existing behavior? |
|
flate2 offers 4 different backends. Vector now uses default
So only miniz_oxide returns error for that bitstream. For me it looks like that miniz_oxide behavior is non standard. |
|
I used 0x12 0x34 in that test because I thought it wasn't a valid compressed payload. If it is valid, maybe we should change it to an invalid payload (following the zlib rfc, maybe?) |
I changed testing sequence, so now it fails for all flate2 backends. |
|
Thank you @JakubOnderka, I opened Frommi/miniz_oxide#169 (comment) based on your findings. |
|
I think it would be interesting to trigger benchmarks for this PR @pront. Although I don't know if current benchmarks do use zlib decompression |
|
Hello, we will need to fix the failure here: https://github.com/vectordotdev/vector/actions/runs/13676581269/job/38244029801?pr=22533 Also, let's add a changelog to call out this change. |
I think @JakubOnderka fixed it in cecb9c2. I seems that |
thank you for following up with them @jorgehermo9! |
|
Update: Frommi/miniz_oxide#169 (comment) It was a bug in |
Rebased to current master branch. |
|
Let's also add a changelog fragment since this is not a small change 👍 |
Changelog added. |
|
Tests are still failing, I will need to check deeper where is the problem. |
|
It looks like that this bug will be fixed with |
Agreed. Reverting: vectordotdev/vrl#1366 |
|
@pront New flate2 was released today, so I also created new pull request for vrl: vectordotdev/vrl#1368 |
Can you point the VRL dependency (in this PR) to your VRL branch? I want to run the windows tests to see if the failure went away. |
Done, just do not forget to remote last commit before merging :) |
Doesn't work on forks for security reasons. Check is running here: Also, this PR needs to update the licenses |
|
Attempting to merge 🤞 |
…vectordotdev#22533) * chore: Use zlib-rs for much faster zlib decoding/encoding * fix spellchecking * rename * dd-rust-license-tool write * chore: Bump flate2 to 1.1.1 to fix linking vector on windows * test: Change vrl to zlib-rs branch to check building on windows * Revert "test: Change vrl to zlib-rs branch to check building on windows" This reverts commit 446dbc3. * cargo update -p vrl * updated licenses * fix bad rebase --------- Co-authored-by: Pavlos Rontidis <pavlos.rontidis@gmail.com>
Summary
zlib-rs is much faster for library for zlib decoding/encoding than default miniz_oxide and it is even faster than zlib-ng (https://trifectatech.org/blog/zlib-rs-is-faster-than-c/)
Change Type
Is this a breaking change?
How did you test this PR?
Standard tests
Does this PR include user facing changes?
Checklist
make check-allis a good command to run locally. This check isdefined here. Some of these
checks might not be relevant to your PR. For Rust changes, at the very least you should run:
cargo fmt --allcargo clippy --workspace --all-targets -- -D warningscargo nextest run --workspace(alternatively, you can runcargo test --all)Cargo.lock), pleaserun
dd-rust-license-tool writeto regenerate the license inventory and commit the changes (if any). More details here.References
vectordotdev/vrl#1301