perf(binary): inflate into uninitialized buffer, drop the zero-init memset - #933
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR swaps the workspace and wacore/binary zlib backend from flate2 to zlib-rs. It updates dependency declarations and rewrites pooled, streaming, and one-shot decompression paths to use Changeszlib-rs decompression migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The history-sync decompressor spends ~6% of its time in a memset that zeroes the output window before every inflate call. Root cause: flate2's zlib-rs backend doesn't override decompress_uninit, so decompress_vec falls back to the default that zero-initializes the whole spare capacity — bytes inflate overwrites immediately. Drive zlib-rs's Inflate directly and decompress into the vector's spare capacity via decompress_uninit + set_len, skipping the zeroing. zlib-rs is already in the tree (transitively via flate2), is no_std/wasm-safe, and is pulled with the same std + rust-allocator features flate2 uses, so no new external crate and no wasm regression. flate2 moves to dev-dependencies since only the test/bench compression fixtures still need it.
87249eb to
5b99262
Compare
Patch release: AArch64 MSRV fix (relevant to the multi-arch Docker image) plus deflate/LoongArch tweaks. No changes to the inflate path this PR uses.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Line 105: The zlib-rs dependency is pinned to an unpublished version, so Cargo
cannot resolve it. Update the dependency entry in Cargo.toml to a
crates.io-published zlib-rs release while keeping the existing feature flags
unchanged, and verify the manifest still points to the same dependency symbol
zlib-rs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 16939dba-0659-4286-b214-32b0562bffc0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
Cargo.toml
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
What
Removes an avoidable
memsetfrom the history-sync decompression path. The binary decoder's zlib inflate spends ~6% ofbench_process_history_sync(~2ms) zero-initializing the output buffer before every inflate call — bytes that inflate then overwrites immediately.Root cause
flate2::Decompress::decompress_vecwrites into the vector's spare capacity via the backend'sdecompress_uninit. flate2's zlib-rs backend doesn't overridedecompress_uninit, so it falls back to the trait default (initialize_buffer→write_bytes(0, len)), which memsets the entire spare region first. That zeroing is pure waste for inflate, which produces exactly the bytes it writes. The flate2 docs themselves call this out and point to the lower-level uninit API.This showed up clearly in the deterministic Simulation flamegraph:
__memset_avx2_unaligned_ermscalled from<flate2::mem::Decompress>::decompress_vec, 6% self time, dwarfed only by the inflate core itself.How
Drive
zlib_rs::Inflatedirectly inzlib_pool.rsand decompress intoVec::spare_capacity_mut()throughdecompress_uninit, then bump the length by the produced count (set_len). No zeroing. A smallinflate_into_sparehelper wraps the uninit + set_len so both call sites (the streamingInflateReader::pumpand the one-shotdecompress_zlib_pooled) share it.zlib-rsis already in the dependency tree (transitively via flate2), isno_stdand wasm/esp32-safe, and is pulled with the samestd+rust-allocatorfeatures flate2 already selects for it — so no new external crate and no wasm regression. The format is unchanged (still zlib/deflate).flate2moves todev-dependenciessince only the test/bench compression fixtures still use it (ZlibEncoder).Why measure
The win is real and deterministic in the flamegraph, but it's ~6% of a path dominated by the inflate core (~25%) and buffer copies, so the end-to-end move on
bench_process_history_syncwill be modest. Opening non-draft so CodSpeed measures it and the AI reviewers run. libdeflate (a ~2x faster decoder) was considered and rejected: it's C (breaks the wasm32/esp32 builds this crate must support) and one-shot only (breaks the streaming reader).Verification
cargo test -p wacore-binary— 99 unit + roundtrip/proptest pass, incl. all 8 zlib pool tests (cross-chunk streaming, high-ratio expansion, reuse-after-error, max-size enforcement, oversized-buffer shrink)cargo clippy -p wacore-binary --all-targets -- -D warnings— cleancargo check --workspace— cleancargo shear— no new unused-dependency findings (flate2/zlib-rs both clean)https://claude.ai/code/session_01NGEfhAP41Csiy7ptQWZrf3