Open
Conversation
- align HC main hash with donor hash4 formula - keep donor regression coverage for level 22 parity gap - preserve current donor-path findings before full BT rewrite
|
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:
📝 WalkthroughWalkthroughAdds a post-split compressed-path for levels 16–22, refactors block/literals/FSE/Huffman table construction and selection, extends Matcher API to accept seeded entropy, tightens dictionary/FSE error handling, updates benches/scripts/README, and expands tests. ChangesEncoding core (blocks, FSE, Huffman, frame/matcher)
Dictionary FSE & dictionary creation
Level dispatch and fastest path
Tests, benches, scripts, docs, small cleanups
Sequence Diagram(s)sequenceDiagram
participant FrameComp as FrameCompressor
participant Matcher as Matcher
participant BlockComp as BlockCompressor
participant FSEEnc as FSE_Encoder
participant HuffEnc as Huffman_Encoder
FrameComp->>FrameComp: load cached prev FSE/Huff tables
FrameComp->>Matcher: seed_dictionary_entropy(huff?, ll?, ml?, of?)
Matcher-->>FrameComp: primed or noop
FrameComp->>FrameComp: read donor block (fixed-capacity)
FrameComp->>BlockComp: compress_block_with_post_split(input)
BlockComp->>FSEEnc: choose_table_for_field(...) -> (Custom | RLE | Repeat | Encoded)
FSEEnc-->>BlockComp: FseTableMode / table bytes
BlockComp->>HuffEnc: request literal table description
HuffEnc->>FSEEnc: encode_weight_description(...) (validate round-trip)
FSEEnc-->>HuffEnc: serialized weight bytes or reject
HuffEnc-->>BlockComp: table bytes or raw fallback
BlockComp->>BlockComp: estimate partition costs, emit compressed or raw (restore state on raw)
BlockComp-->>FrameComp: emit block(s)
FrameComp->>FrameComp: compute savings, store pending_input
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Ports C zstd “donor” behavior for level 22 into the Rust encoder (entropy/table decisions, block splitting, and framing), aiming for ratio parity and C decoder interoperability while addressing a level-22 performance hot path.
Changes:
- Add donor-parity pre-block splitting and revised framing/block handling for high compression levels.
- Rework Huffman/FSE table construction/selection and add additional interoperability/regression tests (including donor FFI validation).
- Update benchmarks/docs to include and describe level 22 behavior.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| zstd/tests/cross_validation.rs | Adds regression + ratio tests around Rust level 22 and FFI roundtrips. |
| zstd/src/tests/roundtrip_integrity.rs | Relaxes an assertion to tolerate small overhead deltas in a size comparison test. |
| zstd/src/huff0/mod.rs | Adjusts Huffman round-trip test helper behavior for non-writeable table descriptions. |
| zstd/src/huff0/huff0_encoder.rs | Major Huffman encoder/table build updates + new donor-FFI acceptance tests. |
| zstd/src/fse/fse_encoder.rs | Refactors FSE table construction/normalization toward donor parity and adds RLE tracking. |
| zstd/src/encoding/mod.rs | Updates compression level docs and extends Matcher with dictionary entropy seeding hook. |
| zstd/src/encoding/levels/fastest.rs | Routes level 16–22 through post-split compressed-block path and adds state rollback for raw fallback. |
| zstd/src/encoding/frame_compressor.rs | Implements donor-style pre-splitting and framing policy changes; threads entropy into matcher. |
| zstd/src/encoding/blocks/compressed.rs | Adds post-split block emission, RLE literals, and donor-like table selection heuristics. |
| zstd/src/dictionary/mod.rs | Switches dict FSE table build path to symbol-count construction. |
| zstd/src/decoding/sequence_section_decoder.rs | Removes debug printing from a test path. |
| zstd/benches/support/mod.rs | Adds a level 22 benchmark configuration and removes an unused helper. |
| zstd/Cargo.toml | Enables experimental feature on dev-dependency zstd for donor interop tests. |
| README.md | Updates strategy/level coverage documentation to reflect new level 16–22 behavior. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zstd/src/encoding/blocks/compressed.rs (1)
151-176:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDon't precompute repeat-offset codes before raw-fallback is decided.
collect_block_parts()bakesseq.ofagainst the full-block repeat-offset history, butemit_single_sequence_block()can later replace a partition withBlockType::Raw. A raw partition does not advance decoder repeat-offset history, so any later compressed partition — and the next block if the last partition falls back to raw — will interpret those precomputed repcodes against the wrong history. This can produce invalid streams on mixed raw/compressed split outcomes.Please defer
encode_offset_with_history()until a partition is definitively kept compressed, or snapshot/recomputestate.offset_histalongside the Huffman/FSE state when a partition falls back to raw.Also applies to: 256-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zstd/src/encoding/blocks/compressed.rs` around lines 151 - 176, collect_block_parts currently calls encode_offset_with_history() and stores seq.of using the live state.offset_hist, which is invalid if later emit_single_sequence_block chooses BlockType::Raw for a partition; instead stop precomputing repeat-offset codes in collect_block_parts: record the raw offset values (e.g., store offset as a plain u32 or keep a SequenceRaw with ll, ml, raw_offset) and defer calling encode_offset_with_history() until the partition is definitely emitted as compressed inside emit_single_sequence_block (or when the compressed partition is finalized); alternatively, if you prefer to keep the current structure, snapshot state.offset_hist (and any related decoder repeat state) when producing each candidate sequence so you can recompute encodings when the final decision is made—refer to collect_block_parts, encode_offset_with_history, emit_single_sequence_block, BlockType::Raw and state.offset_hist to locate the changes.
🤖 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/encoding/levels/fastest.rs`:
- Around line 73-83: The saved snapshot variables (saved_offset_hist,
saved_huff_table, saved_ll_previous, saved_ml_previous, saved_of_previous) are
cloned before an early return when compression_level matches
CompressionLevel::Level(16..=22) and state.matcher.window_size() is large, which
wastes work; move the snapshot/cloning to after that level/window-size check
(i.e., perform the check that calls compress_block_with_post_split and returns
BlockType::Compressed first) so you only clone the state when falling back to
compress_block where the saved_* values are actually used, or alternatively
ensure compress_block_with_post_split documents/manages state rollback if you
prefer keeping clones.
---
Outside diff comments:
In `@zstd/src/encoding/blocks/compressed.rs`:
- Around line 151-176: collect_block_parts currently calls
encode_offset_with_history() and stores seq.of using the live state.offset_hist,
which is invalid if later emit_single_sequence_block chooses BlockType::Raw for
a partition; instead stop precomputing repeat-offset codes in
collect_block_parts: record the raw offset values (e.g., store offset as a plain
u32 or keep a SequenceRaw with ll, ml, raw_offset) and defer calling
encode_offset_with_history() until the partition is definitely emitted as
compressed inside emit_single_sequence_block (or when the compressed partition
is finalized); alternatively, if you prefer to keep the current structure,
snapshot state.offset_hist (and any related decoder repeat state) when producing
each candidate sequence so you can recompute encodings when the final decision
is made—refer to collect_block_parts, encode_offset_with_history,
emit_single_sequence_block, BlockType::Raw and state.offset_hist to locate the
changes.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6c79ff9-5204-41ca-a93a-af3d309c2c25
📒 Files selected for processing (16)
README.mdzstd/Cargo.tomlzstd/benches/support/mod.rszstd/src/decoding/sequence_section_decoder.rszstd/src/dictionary/fastcover.rszstd/src/dictionary/mod.rszstd/src/encoding/blocks/compressed.rszstd/src/encoding/frame_compressor.rszstd/src/encoding/levels/fastest.rszstd/src/encoding/match_generator.rszstd/src/encoding/mod.rszstd/src/fse/fse_encoder.rszstd/src/huff0/huff0_encoder.rszstd/src/huff0/mod.rszstd/src/tests/roundtrip_integrity.rszstd/tests/cross_validation.rs
💤 Files with no reviewable changes (1)
- zstd/src/decoding/sequence_section_decoder.rs
Member
Author
|
@coderabbitai Re: repeat-offset codes before raw fallback. Fixed in c47314f: post-split collection now stores raw offsets, |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
![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 (x86_64-gnu)'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.30.
| Benchmark suite | Current: a1f04b7 | Previous: 95af26d | Ratio |
|---|---|---|---|
compress/fastest/small-4k-log-lines/matrix/pure_rust |
0.12 ms |
0.031 ms |
3.87 |
compress/default/small-4k-log-lines/matrix/pure_rust |
0.191 ms |
0.108 ms |
1.77 |
compress/better/small-4k-log-lines/matrix/pure_rust |
0.196 ms |
0.058 ms |
3.38 |
compress/level4-row/small-4k-log-lines/matrix/pure_rust |
0.159 ms |
0.07 ms |
2.27 |
compress/best/small-4k-log-lines/matrix/pure_rust |
0.196 ms |
0.059 ms |
3.32 |
compress/better/decodecorpus-z000033/matrix/pure_rust |
121.842 ms |
75.918 ms |
1.60 |
compress/best/decodecorpus-z000033/matrix/pure_rust |
129.261 ms |
93.58 ms |
1.38 |
compress/better/low-entropy-1m/matrix/pure_rust |
7.591 ms |
4.932 ms |
1.54 |
compress/best/low-entropy-1m/matrix/pure_rust |
8.241 ms |
5.103 ms |
1.61 |
decompress/fastest/small-4k-log-lines/rust_stream/matrix/pure_rust |
0.005 ms |
0.003 ms |
1.67 |
decompress/fastest/small-4k-log-lines/rust_stream/matrix/c_ffi |
0.002 ms |
0.001 ms |
2 |
decompress/default/small-4k-log-lines/rust_stream/matrix/pure_rust |
0.005 ms |
0.003 ms |
1.67 |
decompress/default/small-4k-log-lines/rust_stream/matrix/c_ffi |
0.002 ms |
0.001 ms |
2 |
decompress/better/small-4k-log-lines/rust_stream/matrix/pure_rust |
0.005 ms |
0.003 ms |
1.67 |
decompress/better/small-4k-log-lines/rust_stream/matrix/c_ffi |
0.002 ms |
0.001 ms |
2 |
decompress/level4-row/small-4k-log-lines/rust_stream/matrix/pure_rust |
0.005 ms |
0.003 ms |
1.67 |
decompress/level4-row/small-4k-log-lines/rust_stream/matrix/c_ffi |
0.002 ms |
0.001 ms |
2 |
decompress/best/small-4k-log-lines/rust_stream/matrix/pure_rust |
0.005 ms |
0.003 ms |
1.67 |
decompress/best/small-4k-log-lines/rust_stream/matrix/c_ffi |
0.002 ms |
0.001 ms |
2 |
decompress/fastest/decodecorpus-z000033/rust_stream/matrix/pure_rust |
7.147 ms |
2.635 ms |
2.71 |
decompress/fastest/decodecorpus-z000033/rust_stream/matrix/c_ffi |
1.091 ms |
0.609 ms |
1.79 |
decompress/default/decodecorpus-z000033/rust_stream/matrix/pure_rust |
6.69 ms |
2.132 ms |
3.14 |
decompress/default/decodecorpus-z000033/rust_stream/matrix/c_ffi |
1.039 ms |
0.553 ms |
1.88 |
decompress/better/decodecorpus-z000033/rust_stream/matrix/pure_rust |
6.945 ms |
2.748 ms |
2.53 |
decompress/better/decodecorpus-z000033/rust_stream/matrix/c_ffi |
1.198 ms |
0.626 ms |
1.91 |
decompress/level4-row/decodecorpus-z000033/rust_stream/matrix/pure_rust |
6.565 ms |
2.122 ms |
3.09 |
decompress/level4-row/decodecorpus-z000033/rust_stream/matrix/c_ffi |
0.982 ms |
0.51 ms |
1.93 |
decompress/best/decodecorpus-z000033/rust_stream/matrix/pure_rust |
6.891 ms |
2.698 ms |
2.55 |
decompress/best/decodecorpus-z000033/rust_stream/matrix/c_ffi |
1.188 ms |
0.616 ms |
1.93 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @polaz
There was a problem hiding this comment.
Pull request overview
Ports the donor (C zstd) level 22 “optimal” compression path into the Rust encoder to improve level22 performance and ratio parity while maintaining C decoder interoperability.
Changes:
- Implement donor-style pre-block splitting, entropy handling, and literal/sequence encoding behavior for higher compression levels (16–22).
- Rework Huffman/FSE table construction and selection to better match donor behavior (including RLE/repeat handling).
- Add regression and cross-validation tests plus benchmark wiring for level 22 parity checks.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| zstd/tests/cross_validation.rs | Adds level22 cross-validation against FFI decode and size comparisons on corpus proxy. |
| zstd/src/tests/roundtrip_integrity.rs | Relaxes a size assertion for a multi-block offset reuse test. |
| zstd/src/huff0/mod.rs | Tightens Huffman table description expectations in roundtrip helper; reduces test loop upper bound. |
| zstd/src/huff0/huff0_encoder.rs | Implements donor-like Huffman weight/table sizing/selection and FSE-based weight descriptions; adds extensive interoperability tests. |
| zstd/src/fse/fse_encoder.rs | Reworks interleaved encoding and donor-like normalization / table-log selection; adds start-state tracking. |
| zstd/src/encoding/mod.rs | Updates CompressionLevel docs and adds matcher hook for seeding dictionary entropy tables. |
| zstd/src/encoding/levels/fastest.rs | Adds level16–22 post-split path; adds state restoration on raw fallback for oversized compressed blocks. |
| zstd/src/encoding/frame_compressor.rs | Adds donor pre-split block sizing, contiguous-buffer chunking via pending suffix, and dictionary entropy seeding. |
| zstd/src/encoding/blocks/compressed.rs | Adds post-split compressed-block emission, literal RLE handling, and donor-like FSE table mode selection (including RLE/repeat). |
| zstd/src/dictionary/mod.rs | Switches FSE table building to symbol-count-based construction. |
| zstd/src/dictionary/fastcover.rs | Uses sort_by_key(Reverse(..)) for segment ordering. |
| zstd/src/decoding/sequence_section_decoder.rs | Removes debug-print loop from a test. |
| zstd/benches/support/mod.rs | Adds level22 to supported benchmark levels; removes unused Scenario helper. |
| zstd/Cargo.toml | Enables experimental feature on dev-dependency zstd crate for tests/interop. |
| README.md | Updates documented strategy coverage to include btopt/btultra/btultra2-style implementations for levels 16–22. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/dictionary/mod.rs`:
- Around line 349-354: serialize_fse_table_from_corpus can panic because
build_table_from_symbol_counts asserts total > 1 when symbols are degenerate
(e.g., bounded_fse_symbols returns [0u8]); change
serialize_fse_table_from_corpus to return io::Result<Vec<u8>> and detect
degenerate counts before calling build_table_from_symbol_counts: compute total
from counts, and if total <= 1 return an Ok(serialized_fallback) or an explicit
Err with context (mirror serialize_huffman_table behavior), then propagate that
Result through finalized_content_budget and callers such as
create_fastcover_dict_from_source and train_fastcover_raw_from_slice so the
panic is avoided and callers can handle the empty-input case.
In `@zstd/src/encoding/levels/fastest.rs`:
- Around line 73-77: The call to compress_block_with_post_split can emit
multiple partition blocks or raw fallbacks but the caller unconditionally
returns BlockType::Compressed; update the contract so callers can observe
multi-block emission: either (A) change compress_block_with_post_split's
documentation and signature to return a precise result (e.g.,
Result<BlockEmission, Error> or Vec<BlockType>/EmissionSummary) and have the
caller use that return value instead of unconditionally returning
BlockType::Compressed, or (B) add and return a new enum variant (e.g.,
BlockType::MultiPart or BlockEmission::Multiple) that represents multipart
emission and update the caller here to return that variant after calling
compress_block_with_post_split; ensure compress_block_with_post_split, its doc
comments, and the caller (the code currently returning BlockType::Compressed)
consistently reflect the new contract.
In `@zstd/src/huff0/huff0_encoder.rs`:
- Around line 151-154: The FSE/raw switch is using weights.len() / 2 which
undercounts the raw nibble byte cost for odd-length weights; update the
comparison that considers raw_description_is_representable and the branch using
encoded.len() to use the correct ceiling byte count for the raw nibble form
(i.e. compute raw bytes as (weights.len() + 1) / 2 or call a div_ceil helper) so
that both write_table() and try_table_description_size() use the true
weights.len().div_ceil(2) value when deciding between FSE vs raw description.
- Around line 271-280: The estimator estimate_compressed_size currently returns
bits.div_ceil(8) but must account for the byte-aligned terminator emitted by
encode_stream; change estimate_compressed_size to compute byte_count =
bits.div_ceil(8) and if bits % 8 == 0 (i.e., already byte-aligned) add one extra
byte to byte_count before returning; apply the identical adjustment to the other
payload-size estimator used by build_from_counts (the second estimator in this
module) so best_size and the early-break heuristic use the corrected sizes.
🪄 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: 4db2d9a4-b87d-4f8e-91c2-d2c0cd95bcb8
📒 Files selected for processing (3)
zstd/src/dictionary/mod.rszstd/src/encoding/levels/fastest.rszstd/src/huff0/huff0_encoder.rs
There was a problem hiding this comment.
Pull request overview
Ports the C zstd “donor” behavior for level 22 (bt* optimal paths, entropy/table selection, and framing/block splitting) into the Rust encoder to improve performance and maintain decoder interoperability.
Changes:
- Add level 22-specific encoding behaviors: pre-block splitting, post-split fallback, entropy/table selection updates, and RLE literal handling.
- Rework Huffman/FSE table building and serialization to better match donor semantics and improve interoperability validation.
- Expand tests/bench support and update docs to reflect the new level-to-strategy mapping.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| zstd/tests/cross_validation.rs | Adds regression and size-parity checks for Rust level 22 vs FFI decoder/donor output. |
| zstd/src/tests/roundtrip_integrity.rs | Relaxes a multi-block compression-size assertion to tolerate new framing/entropy behavior. |
| zstd/src/huff0/mod.rs | Strengthens Huffman round-trip helper by asserting table-description writeability; adjusts test coverage range. |
| zstd/src/huff0/huff0_encoder.rs | Adds FSE-based Huffman weight description path, size estimation helpers, and extensive interop/roundtrip tests. |
| zstd/src/fse/fse_encoder.rs | Refactors interleaved encoding, adds donor-normalization table-log selection, and updates start-state handling. |
| zstd/src/encoding/mod.rs | Updates documentation for levels 16–22 and adds a matcher hook for seeding dictionary entropy. |
| zstd/src/encoding/levels/fastest.rs | Uses post-split block compression for levels 16–22 and restores entropy/history on oversize raw fallback. |
| zstd/src/encoding/frame_compressor.rs | Implements donor-like pre-splitting and framing policy; adds entropy seeding from cached dictionary state. |
| zstd/src/encoding/blocks/compressed.rs | Introduces two-phase block encoding, post-split emission, table selection changes, and RLE literal sections. |
| zstd/src/dictionary/mod.rs | Replaces byte-slice FSE table build with explicit symbol counting; keeps vec! macro import explicit. |
| zstd/src/dictionary/fastcover.rs | Uses sort_by_key(Reverse(...)) for segment ordering. |
| zstd/src/decoding/sequence_section_decoder.rs | Removes debug printing from tests. |
| zstd/benches/support/mod.rs | Adds level 22 to benchmark level mapping and removes unused helper. |
| zstd/Cargo.toml | Enables experimental feature for dev zstd dependency to support donor interop tests. |
| README.md | Updates level strategy mapping documentation to include btopt/btultra/btultra2-style coverage. |
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/src/encoding/blocks/compressed.rs`:
- Around line 335-389: The estimator currently only measures encode_block_parts
output in estimate_subblock_size, causing derive_block_splits to pick splits
that ignore the real emission overhead (emit_single_sequence_block adds a 3-byte
BlockHeader and may switch to Raw based on min_gain); update
estimate_subblock_size to mirror the real emission cost model used in
emit_single_sequence_block by adding the fixed BlockHeader size (3 bytes) and
applying the same min_gain/raw conversion logic (i.e., if the compressed payload
+ header doesn't meet min_gain compared to raw, return the raw-emission size
instead) so the split scorer uses the exact same cost decisions as the emitter.
In `@zstd/src/huff0/huff0_encoder.rs`:
- Around line 434-487: The sentinel “infinity” used in nodes.resize and the
low_s < 0 branches (currently 1usize<<30 and 1usize<<31) is too small; replace
those magic constants with usize::MAX for the HuffNode.count sentinel (in the
nodes.resize initializer and in both places where you use the large constant in
the selection blocks). Also change the initial merge sum assignment
(nodes[node_nb].count = nodes[low_s as usize].count + nodes[(low_s - 1) as
usize].count) to use saturating_add to avoid overflow (e.g.,
.saturating_add(...)). Update uses of these symbols: nodes.resize(..., HuffNode
{ count: ... }), the low_s < 0 branches, and the nodes[node_nb].count assignment
to use usize::MAX and saturating_add accordingly.
🪄 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: 5f0c4cdd-1833-4037-8ee9-48744c509ea5
📒 Files selected for processing (5)
zstd/src/dictionary/mod.rszstd/src/encoding/blocks/compressed.rszstd/src/encoding/levels/fastest.rszstd/src/huff0/huff0_encoder.rszstd/src/tests/roundtrip_integrity.rs
There was a problem hiding this comment.
Pull request overview
Ports zstd “donor” behavior for high compression levels (notably level 22) into the Rust encoder, focusing on BT/optimal parsing parity, entropy/table handling, and block framing/splitting while maintaining C decoder interoperability.
Changes:
- Add level 22 cross-validation and interoperability regression tests, plus loosen one cross-block assertion to tolerate small framing overhead.
- Rework Huffman + FSE encoder table selection/normalization and add donor-compat validation tests (including calling into libzstd HUF routines).
- Implement donor-style pre-splitting / post-split fallback behavior in the encoder hot path, and expose matcher hooks for dictionary entropy seeding.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| zstd/tests/cross_validation.rs | Adds level 22 roundtrip + ratio parity/regression tests against the FFI decoder/encoder. |
| zstd/src/tests/roundtrip_integrity.rs | Relaxes a cross-block comparison to allow bounded slack for framing/table decisions. |
| zstd/src/huff0/mod.rs | Tightens Huffman round-trip assumptions and reduces a test loop range. |
| zstd/src/huff0/huff0_encoder.rs | Adds FSE-based Huffman weight description path, donor-compat tests, and table size estimation helpers. |
| zstd/src/fse/fse_encoder.rs | Reworks FSE table construction/normalization to match donor behavior; adds RLE constraints and start-state tracking. |
| zstd/src/encoding/mod.rs | Updates compression-level docs and adds a matcher hook for dictionary entropy seeding. |
| zstd/src/encoding/levels/fastest.rs | Adds post-split compressed path for levels 16–22 and restores entropy/history state on oversize fallback. |
| zstd/src/encoding/frame_compressor.rs | Implements donor-style pre-splitting across reads, savings tracking, single-segment framing policy adjustments, and entropy seeding wiring. |
| zstd/src/encoding/blocks/compressed.rs | Refactors block encoding into collect/encode phases, adds post-split block emission, and donor-style entropy/table selection (incl. RLE modes). |
| zstd/src/dictionary/mod.rs | Switches FSE table building to symbol-count path and adds validation for insufficient statistics. |
| zstd/src/dictionary/fastcover.rs | Minor sort refactor using Reverse key ordering. |
| zstd/src/decoding/sequence_section_decoder.rs | Removes debug print loop from a test. |
| zstd/benches/support/mod.rs | Adds level 22 to benchmark level mappings and removes an unused helper. |
| zstd/Cargo.toml | Enables experimental feature for the dev-dependency zstd crate (needed by new donor-interop tests). |
| README.md | Updates documented strategy/back-end mapping for levels 16–22. |
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/dictionary/mod.rs`:
- Around line 343-359: The code currently picks sample_data over raw_content,
computes bounded_fse_symbols(source, max_symbol) and then errors if total <= 1;
update this to fall back to raw_content when sample_data was used and the first
pass yields total <= 1: after computing counts and total from symbols, if total
<= 1 and source was sample_data (i.e., sample_data.is_empty() was false),
recompute symbols/counts/total using raw_content as source and proceed; only
return the io::Error if total is still <= 1 after this second pass. Ensure you
reference the same bounded_fse_symbols, counts, and total variables so
callers/logic remain unchanged.
🪄 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: 98827d74-7fc5-4c39-b1fe-2bf571525187
📒 Files selected for processing (3)
zstd/src/dictionary/mod.rszstd/src/encoding/blocks/compressed.rszstd/src/huff0/huff0_encoder.rs
There was a problem hiding this comment.
Pull request overview
Ports Zstd “donor” level 22 optimal (BT/price-parser) behavior into the Rust encoder to improve performance and maintain C decoder interoperability, with additional entropy/table-selection and framing/block-splitting parity work.
Changes:
- Adds level 16–22 strategy semantics (btopt/btultra/btultra2-style profiles), dictionary entropy seeding hooks, and donor-aligned single-segment framing and block pre-splitting.
- Reworks entropy coding paths (HUF weight descriptions via FSE when needed, FSE table construction/selection including RLE modes) and compressed-block post-split fallback behavior.
- Expands/adjusts regression and interoperability tests/benches (level 22 corpus proxy, donor HUF/FSE acceptance checks, dictionary empty-source rejection).
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| zstd/tests/cross_validation.rs | Adds level-22 corpus-proxy cross-validation and FFI decode regression tests. |
| zstd/src/tests/roundtrip_integrity.rs | Relaxes cross-block compression assertion with explicit overhead slack. |
| zstd/src/huff0/mod.rs | Strengthens round-trip invariant checks; narrows looped fixtures. |
| zstd/src/huff0/huff0_encoder.rs | Implements/validates FSE-based Huffman weight descriptions; adds donor interop tests. |
| zstd/src/fse/fse_encoder.rs | Reworks FSE encoder interleaving and donor-aligned table building/normalization. |
| zstd/src/encoding/mod.rs | Updates level→strategy documentation; adds matcher dictionary-entropy seeding hook. |
| zstd/src/encoding/levels/fastest.rs | Adds post-split compressed-block path and rollback for oversize fallback. |
| zstd/src/encoding/frame_compressor.rs | Adds donor pre-splitting and contiguous-input block handling; updates framing policy. |
| zstd/src/encoding/blocks/compressed.rs | Major refactor: post-split encoding, literals RLE handling, FSE RLE mode, table selection cost model. |
| zstd/src/dictionary/mod.rs | Switches FSE table build to symbol counts, propagates Result, rejects empty training source. |
| zstd/src/dictionary/fastcover.rs | Uses key-based sorting for segment scoring. |
| zstd/src/decoding/sequence_section_decoder.rs | Removes debug printing from tests. |
| zstd/benches/support/mod.rs | Adds level22 to benchmark matrix; removes unused helper. |
| zstd/Cargo.toml | Enables experimental feature on dev-dependency zstd for interoperability testing. |
| README.md | Updates documented strategy coverage for levels 5–22. |
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/fse/fse_encoder.rs`:
- Line 56: The assertion in zstd/src/fse/fse_encoder.rs currently forbids a
two-symbol interleaved stream by using assert!(data.len() > 2); relax this to
assert!(data.len() >= 2) so len == 2 is allowed (the even branch can init both
terminal states and write them out); do not change the singleton fallback
logic—leave handling of len == 1 to the caller/single-stream path.
🪄 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: 69702e5a-e994-44f7-accc-66d5bd668490
📒 Files selected for processing (2)
zstd/src/dictionary/mod.rszstd/src/fse/fse_encoder.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/fse/fse_encoder.rs`:
- Around line 338-345: The current check uses total > 1 which allows
single-symbol (repeated) histograms to proceed into donor normalization; change
the guard to reject one-symbol histograms by counting distinct non-zero symbols
(e.g., let distinct = counts.iter().filter(|&&c| c > 0).count();) and assert
distinct > 1 (or return the RLE path) before calling
donor_optimal_table_log/max_symbol; update the logic around total, max_symbol,
and donor_optimal_table_log so only histograms with more than one distinct
symbol build FSE tables (use the symbols counts, total, max_symbol,
donor_optimal_table_log, probs to locate the code to change).
🪄 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: 5e296704-c24a-4c74-a851-57da7c522b7f
📒 Files selected for processing (2)
zstd/src/encoding/match_generator.rszstd/src/fse/fse_encoder.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/encoding/blocks/compressed.rs`:
- Around line 389-393: The estimator currently resets the right partition to the
block-entry state by reassigning self.scratch_state.last_huff_table,
.fse_tables.ll_previous/.ml_previous/.of_previous, and .offset_hist from the
original block-entry values; instead, after scoring/encoding the left partition
(the "first" split) propagate the left partition's post-emit state into the
scratch_state used to score the right partition so the "second" split is
evaluated with the actual Huffman/FSE tables and repeat offsets that would be
present at emission time (adjust the code paths around derive_block_splits()/the
split-scoring routine to clone the post-state produced by the left partition and
use those values for last_huff_table, fse_tables.* and offset_hist when
computing the right-hand cost, rather than reinitializing from the block-entry
state).
🪄 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: 3ef26892-fa6d-4839-8e42-67d51051e63a
📒 Files selected for processing (1)
zstd/src/encoding/blocks/compressed.rs
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
Testing
Benchmark Notes
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests
Chores