Conversation
- Interleave decode operations across all 4 Huffman streams to hide memory latency: 4 independent table lookups then 4 state advances per iteration instead of sequential stream-by-stream processing - Replace entry-by-entry table filling with slice::fill() for bulk initialization (compiles to vectorized stores) - Add #[inline(always)] to decode_symbol, init_state, next_state for better codegen in the interleaved loop - Update dev-dependencies: criterion 0.5→0.8, rand 0.8→0.10, zstd 0.13.2→0.13.3, and bump cli deps to latest patch versions Closes #10
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an interleaved 4‑stream Huffman literals decoder with per‑stream bit-readers and guarded output bounds, applies slice-fill bulk initialization and inlining hints in the Huffman decoder, and updates Cargo dependency and bench/dev-dependency versions/imports. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant LiteralsDecoder as Literals\nSectionDecoder
participant Huff1 as HuffmanDecoder\n(Stream1)
participant Huff2 as HuffmanDecoder\n(Stream2)
participant Huff3 as HuffmanDecoder\n(Stream3)
participant Huff4 as HuffmanDecoder\n(Stream4)
participant Buffers as Output\nBuffers
Caller->>LiteralsDecoder: decode_literals(num_streams=4)
LiteralsDecoder->>LiteralsDecoder: parse headers & slice into 4 substreams
LiteralsDecoder->>Huff1: init_state(br1)
LiteralsDecoder->>Huff2: init_state(br2)
LiteralsDecoder->>Huff3: init_state(br3)
LiteralsDecoder->>Huff4: init_state(br4)
LiteralsDecoder->>Buffers: allocate target slice (base..base+regen)
rect rgba(100,150,200,0.5)
loop interleaved decode
LiteralsDecoder->>Huff1: decode_symbol()
Huff1->>Buffers: write symbol at cursor1
LiteralsDecoder->>Huff2: decode_symbol()
Huff2->>Buffers: write symbol at cursor2
LiteralsDecoder->>Huff3: decode_symbol()
Huff3->>Buffers: write symbol at cursor3
LiteralsDecoder->>Huff4: decode_symbol()
Huff4->>Buffers: write symbol at cursor4
LiteralsDecoder->>Huff1: next_state(br1)
LiteralsDecoder->>Huff2: next_state(br2)
LiteralsDecoder->>Huff3: next_state(br3)
LiteralsDecoder->>Huff4: next_state(br4)
end
end
rect rgba(200,150,100,0.5)
Note over LiteralsDecoder: per-stream drain & finalize
LiteralsDecoder->>Huff1: drain remaining -> buf1
LiteralsDecoder->>Huff2: drain remaining -> buf2
LiteralsDecoder->>Huff3: drain remaining -> buf3
LiteralsDecoder->>Huff4: drain remaining -> buf4
end
LiteralsDecoder->>LiteralsDecoder: validate total decoded count, truncate on mismatch
LiteralsDecoder->>Caller: return decoded output or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR optimizes zstd literal Huffman decoding by aligning the Rust implementation more closely with the C reference: it interleaves work across the 4 Huffman bitstreams to increase instruction-level parallelism and speeds up Huffman table construction via bulk slice initialization.
Changes:
- Interleave 4-stream Huffman decoding in
decompress_literals(decode 4 symbols, then advance 4 states per iteration). - Use
slice::fill()to bulk-initialize ranges of identical Huffman decode table entries. - Update dev-dependency versions for benchmarks/tests (criterion/rand/zstd) and bump several CLI dependencies.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
zstd/src/huff0/huff0_decoder.rs |
Adds forced inlining on hot decoder methods; bulk-fills decode table entries via slice::fill() |
zstd/src/decoding/literals_section_decoder.rs |
Reworks 4-stream literals decode into an interleaved loop + per-stream buffering then concatenation |
zstd/Cargo.toml |
Updates dev-dependencies for benches/tests (criterion/rand/zstd) |
cli/Cargo.toml |
Updates CLI dependency patch versions; updates CLI dev-dependencies |
- rand 0.10: RngCore → Rng (fill_bytes moved to Rng trait) - criterion 0.8: black_box → std::hint::black_box (deprecated)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/Cargo.toml`:
- Line 19: The dependency entry clap = { version = "4.6.0", features =
["derive"] } raises the MSRV to 1.85, so either update CI/toolchain and docs to
require Rust 1.85+ or pin clap to a pre-4.6.x version that supports our current
MSRV; modify the Cargo.toml dependency accordingly and update CI configuration
(rust-toolchain / workflow matrix) and any README or contribution docs to state
the new minimum Rust version if you choose to keep clap 4.6.0.
In `@zstd/src/decoding/literals_section_decoder.rs`:
- Around line 128-181: The per-stream buffers (bufs) are unbounded and you must
validate each of the 4 substream decoded lengths against the known per-stream
target sizes from section.regenerated_size before concatenation; modify the loop
that drains remaining symbols (using brs, decoders, max_bits) to either decode
directly into pre-sized target slices or accumulate into per-stream buffers and
then assert buf.len() == expected_len for each stream (compute expected_len from
section.regenerated_size split into 4 parts), returning DecompressLiteralsError
if any mismatch, and only call target.extend_from_slice(buf) after each
per-stream length has been validated.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 18931dbb-5e05-481d-93dc-a84875467781
📒 Files selected for processing (4)
cli/Cargo.tomlzstd/Cargo.tomlzstd/src/decoding/literals_section_decoder.rszstd/src/huff0/huff0_decoder.rs
…et slices - Use RFC 8878 segment sizes to pre-allocate target and write directly via cursor offsets instead of per-stream Vec buffers - Add decoded count validation with truncation for corrupted streams
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@zstd/benches/compare_ffi.rs`:
- Around line 14-15: Reorder the use statements to match rustfmt's canonical
ordering: place the std import(s) before external crate imports and sort items
within braces alphabetically; specifically move use std::hint::black_box; above
the criterion import and ensure the criterion::{criterion_group, criterion_main,
Criterion, SamplingMode, Throughput} list is alphabetically ordered (and apply
the same ordering rule to the other import on line 19 so the file is fully
rustfmt-compliant).
In `@zstd/benches/support/mod.rs`:
- Line 1: The `use rand::{rngs::SmallRng, Rng, SeedableRng};` import order is
failing rustfmt; fix by running `cargo fmt --all` (or manually reorder the items
inside the `use rand::{...}` to rustfmt's expected ordering), ensuring the
symbols SmallRng, Rng, and SeedableRng are arranged in the canonical sorted
order so `cargo fmt --check` passes.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: babe9a42-9f9a-438c-8131-74f39cb2be35
📒 Files selected for processing (2)
zstd/benches/compare_ffi.rszstd/benches/support/mod.rs
- Clamp starts[3] via min(3*seg, regen) to prevent OOB when regen < 6 - Add per-stream end indices and bound cursor writes in both interleaved and drain loops, matching C reference opX < oendX guards
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@zstd/src/decoding/literals_section_decoder.rs`:
- Around line 137-144: The computed starts/ends for the four streams can exceed
the true window (base..base+regen) for small/corrupted regen and cause
out-of-bounds accesses in the drain loop; update the logic around starts, ends,
and cursors (the variables base, seg, starts, ends, cursors, regen, target) so
every start and end is clamped into the valid slice bounds (base..base+regen or
target.len()) — e.g. after computing starts and ends, replace each with
usize::min(value, base + regen) (or target.len()) so cursors[i] < ends[i] can
never index past the vector. Ensure cursors is initialized from the clamped
starts so all downstream indexing is safe.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e942235f-b03e-4f4a-94f3-356dacc45775
📒 Files selected for processing (1)
zstd/src/decoding/literals_section_decoder.rs
![sw-release-bot[bot]](https://avatars.githubusercontent.com/u/228206483?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'structured-zstd vs C FFI'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.15.
| Benchmark suite | Current: 7d378cd | Previous: d402315 | Ratio |
|---|---|---|---|
compress/best/small-1k-random/matrix/c_ffi |
0.375 ms |
0.315 ms |
1.19 |
compress/default/large-log-stream/matrix/c_ffi |
3.898 ms |
3.203 ms |
1.22 |
decompress/default/high-entropy-1m/c_stream/matrix/c_ffi |
0.035 ms |
0.026 ms |
1.35 |
decompress/better/high-entropy-1m/c_stream/matrix/c_ffi |
0.036 ms |
0.027 ms |
1.33 |
decompress/best/high-entropy-1m/c_stream/matrix/c_ffi |
0.173 ms |
0.027 ms |
6.41 |
compress-dict/better/small-4k-log-lines/matrix/c_ffi_with_dict |
0.004 ms |
0.003 ms |
1.33 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @polaz
All starts (not just starts[3]) are clamped to base+regen via min(), preventing OOB indexing when regen < 4 (e.g. corrupted frame headers).
Member
Author
|
@coderabbitai resume and re-review |
|
🧠 Learnings used✅ Actions performedReviews resumed. Full review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@zstd/src/decoding/literals_section_decoder.rs`:
- Around line 198-203: In the short-decode branch of LiteralsSectionDecoder (the
code that calls short_decode and writes into target), detect a mismatch between
the number of bytes reported as decoded (decoded) and the expected target length
immediately and return the appropriate decode error (e.g.,
Err(DecodeError::Corrupted) or truncated/short-decode error) from that branch
instead of letting downstream length checks surface the problem; update the
branch that calls short_decode/short_decode_into (working with symbols target
and decoded) to return the short-decode error when decoded != target.len().
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a12477d4-f279-4e12-9949-c52ef834ca3a
📒 Files selected for processing (6)
cli/Cargo.tomlzstd/Cargo.tomlzstd/benches/compare_ffi.rszstd/benches/support/mod.rszstd/src/decoding/literals_section_decoder.rszstd/src/huff0/huff0_decoder.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@zstd/src/decoding/literals_section_decoder.rs`:
- Around line 133-145: The post-write length check wrongly compares target.len()
to section.regenerated_size without accounting for pre-existing bytes; move base
and regen to function scope so the final validation asserts target.len() == base
+ regen (not just regen), and update both places that build starts/ends (the
4-stream path that defines starts/ends and the single-stream path) to use this
base-relative check; reference the variables base, regen, starts, ends,
section.regenerated_size and ensure the error still returns
DecodedLiteralCountMismatch when the condition fails.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4024174f-abb0-47d9-aa7a-a35c06c66842
📒 Files selected for processing (1)
zstd/src/decoding/literals_section_decoder.rs
Move base/regen to function scope so the final length check compares target.len() against base+regen (not just regen). On 4-stream mismatch, truncate to base (clean boundary) instead of base+decoded (scattered segments). Both error paths now report regen-only counts consistently.
All error returns after target.resize() now truncate back to base, preventing callers from observing partial/zero-filled output after a failed decode when the Vec is reused across calls.
polaz
deleted the
feat/#10-perf-huffman-decoder--4-stream-parallel-decoding-a
branch
Merged
This was referenced Apr 3, 2026
This was referenced Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
slice::fill()for bulk initialization (compiles to vectorized stores, matching C referenceHUF_DEltX1_set4()pattern)#[inline(always)]todecode_symbol,init_state,next_statefor better codegen in the interleaved loop[base, base+regen]and cursors bounded by segment ends, matching C referenceopX < oendXguards — prevents OOB on corrupted inputsTechnical Details
The C reference (
huf_decompress.c) processes 4 Huffman streams with interleaved operations so that while one stream waits for a table lookup result, other streams can proceed. The previous Rust implementation decoded streams sequentially in aforloop.The new interleaved loop structure:
ceil(regen_size/4)symbols each), so output is written directly into pre-allocated target slices via cursor offsets — zero extra allocations on the hot pathKnown Limitations
Double-symbol (X2) decode variant and stream selection heuristic are not implemented yet — tracked separately for a future release.
Test Plan
decode_allbenchmark runs (~5.0ms on test corpus)compare_ffibenchmark compiles and runs with criterion 0.8Closes #10
Summary by CodeRabbit
Chores
Refactor
Tests