fix(dfast): upstream-identical sequence generation + faster HUF decode#450
Merged
Conversation
Add a REPORT_BLK diagnostic line alongside the existing REPORT_HDR: walk each frame's block list (type / size / last flag) for both the rust and ffi encoders. Surfaces block-count and per-block-size divergences (e.g. a spurious trailing empty block, or a different pre-split boundary) that the aggregate byte-size REPORT line hides.
When the input length was an exact multiple of MAX_BLOCK_SIZE, the owned
block loop emitted the final full block with last_block=false and then
appended a spurious trailing empty Raw block (R0) carrying the last flag
— 3 wasted bytes per such frame, and a divergence from the C encoder
which marks the last real block last. It hit EVERY multi-block frame whose
content ends exactly on a block boundary (e.g. the 1 MiB = 8x128 KiB bench
fixtures), independent of compressibility.
Root cause: both `OwnedBlockSource` feeders reported EOF only when a block
could NOT be filled to capacity, so a block that filled exactly never
signalled end-of-input until the next zero-byte read.
- Slice feeder: report EOF when the slice is exhausted even if the block
filled exactly (the slice knows its own length).
- Reader feeder: when a block fills exactly to capacity, probe one byte
ahead — a 0-byte read marks this block last; a real byte is stashed and
prepended to the next block. Mirrors the C encoder on ZSTD_e_end.
Regression test covers both feeders x {incompressible, compressible} x
{1,2,3} full blocks, asserting no trailing empty block, the last real
block carries the flag, and round-trip equality. Verified across strategy
tiers (fast / dfast / btopt / btultra2) via the block-structure bench
report: high-entropy-1m and decodecorpus-1m now emit 8 blocks matching C
(was 9 with the trailing R0).
Upstream HUF_writeCTable_wksp keeps the FSE-compressed weight description only when hSize < maxSymbolValue/2 (FLOOR). Our gate used div_ceil, one byte too permissive on odd symbol counts, keeping FSE weights in a boundary band where upstream emits the cheap direct nibbles — a fractionally smaller frame that costs the decoder an FSE weight-table build instead of a nibble unpack. Also add a REPORT_HEX frame-head dump to compare_ffi (alongside REPORT_BLK) so rust-vs-ffi literal-section / Huffman-tree-description generation can be diffed byte-for-byte.
Align the dfast hash-table inserts with upstream zstd_double_fast.c so the match-finder resolves the same candidates and produces C-comparable offset streams (the level_3 ratio gap on repetitive log data traces to divergent hash state, not the search itself): - Immediate-repcode chain inserted curr+2 / ip-2 / ip-1 (the primary-match complementary set). Upstream instead writes BOTH tables at the rep position itself (zstd_double_fast.c:314-315). Insert at the rep position; the previous set left rep-position keys stale. - Complementary insertion after a stored match is ASYMMETRIC upstream (:300-304): hashLong at curr+2 and ip-2; hashSmall at curr+2 and ip-1. The previous form wrote curr+2/ip-2/ip-1 into both tables, polluting the long table with ip-1 and the short table with ip-2. - Add the `if (step < 4) hashLong[hl1] = ip1` lookahead insert (:287) on the long-at-ip0 and short/_search_next_long commit paths. Adds `insert_long` / `insert_short` over a const-generic `insert_masked` core, so the symmetric `insert_position` stays byte-identical while the asymmetric variants emit a single write each. byte-identical round-trip across 796 tests.
The dfast rep1 peek required the repcode candidate to sit at or after the current literal cursor (`cand_pos_r >= literals_start`). After a stored match `literals_start` sits right at the cursor, so essentially every back-reference failed that test: the repcode path almost never fired and every position fell through to the long hash, which minted a creeping / far offset stream instead of a stable repcode. Upstream zstd (zstd_double_fast.c:190) gates the rep on nothing but a non-zero offset and the 4-byte equality at `ip+1-offset_1`, so keep only the window-low bound. Effect on the level_3 dfast ratio (rust vs c_ffi bytes): large-log-stream 11222 -> 8942 (was +15% over C, now beats C 9744) decodecorpus-1m 249 -> 229 (now beats C 239) low-entropy-1m 154 -> 148 Adds a `DFTRACE`-gated per-commit path/offset trace (off by default) used to pin the divergence.
Two divergences made the dfast match-finder emit a different sequence stream than upstream zstd, compounding across a block (worse ratio, and our own decoder is slower on the non-canonical stream): - Step ramp: the skip step grew one position every "1 << kSearchStrength" bytes (upstream kStepIncr; kSearchStrength = 8, so 256). The port used 6 (64 bytes) and capped the step at 8. So it accelerated ~4x too soon and stopped growing, skipping source positions upstream still inserts and missing the short matches upstream finds near a block start. Restore 8 and drop the cap. - Complementary insertion anchor: upstream inserts curr+2 where curr is the iteration SCAN position (fixed at zstd_double_fast.c:184), not the match start. For a rep1 the match begins at scan+1 and a backward catch-up rewinds the start, so anchoring on the match start shifted every rep1/extended insert by +1, seeding the short hash with wrong positions. Thread the scan position through and anchor curr+2 on it. Level_3 sequence comparator is now 100% identical to upstream across all fixtures (was 70-90%); small-4k-log-lines compresses to a byte-identical frame. byte-identical round-trip across 796 tests.
The optional table-log search trades encode time for a smaller literal section. The Fast strategy already skipped it below one block (128 KiB), where per-frame HUF setup dominates and the cheap single-build ties upstream zstd's ratio. DoubleFast has the same profile (little matching work, so HUF setup is a large fraction on a small frame), so extend the size gate to it. Now that dfast emits an upstream-identical sequence stream, a small dfast frame with the search off compresses byte-for-byte like upstream; large frames keep the search and beat upstream on the literal section. Renames fast_huf_search_enabled to huf_search_enabled since it no longer covers only Fast.
…treamX1 The 4-stream HUF literal drain reloaded the bit reader on every symbol, so each trailing symbol near a stream end paid the cold refill_slow path - the dominant drain cost on a literal-heavy frame. Mirror upstream zstd HUF_decodeStreamX1: decode in groups of four with one reload per group, then the final under-four symbols after a single reload with NO per-symbol reload. Termination is output-based (cursor < segment end), like upstream's p < pEnd, so the loops never read past the last symbol into the zero padding. The end-of-stream validation is unchanged and still exact: bits_remaining = (64 - bits_consumed) - extra_bits is independent of reload timing (padding consumed at the stream end lands in either extra_bits via refill or bits_consumed without it, and the difference is identical), so dropping the per-symbol reloads does not shift the -max_bits check. A group reload keeps bits_consumed bounded, so the no-reload decodes never overflow the container. byte-identical round-trip across 796 tests incl. the 1000-iteration random suite.
The frame-header parser issued a separate trait read_exact per field (magic, descriptor, window descriptor, dictionary id, frame content size). On a tiny frame those per-field reads - each a generic Read::read_exact loop plus a map_err - dominated decode (the header parse was ~45% of a 1 KiB random-frame decompress). The variable tail sizes are all encoded in the descriptor, so read the whole tail (window descriptor + dict id + frame content size, at most 13 bytes) in one read_exact and parse it by direct indexing, mirroring upstream zstd ZSTD_getFrameHeader slicing the header out of the source in one shot. Five reads drop to three. byte-identical across 796 tests.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe PR updates compression, decoding, and match-finding logic, adds block-structure reporting in the benchmark, and adjusts no-std slice reads and frame/block boundary handling. ChangesCompression, decoding, and reporting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@zstd/benches/compare_ffi.rs`:
- Around line 1128-1149: The block parsing in compare_ffi’s frame summary helper
silently treats truncated or malformed input as a valid report with n_blocks=0,
which hides parse failures. Update the block walk around the header-derived
pos/payload_end calculation so that any overrun of compressed.len() or invalid
block advance is detected and reported as a parse error instead of continuing to
emit REPORT_BLK. Use the existing parsing logic in the compare_ffi benchmark
helper to distinguish a cleanly parsed frame from a truncated/invalid one, and
only build the block list when the entire descriptor and block payload are fully
consumed.
In `@zstd/src/encoding/frame_compressor.rs`:
- Around line 711-712: Update the doc comment for `huf_optimal_search` in
`FrameCompressor` so it matches the current `huf_search_enabled()` logic; the
current invariant text is stale because it only mentions `Fast`, but
`StrategyTag::Dfast` is now also gated. Adjust the nearby field docs to
explicitly mention `Dfast` alongside `Fast` and describe that the search is not
always kept for those strategies.
In `@zstd/src/encoding/frame_compressor/tests.rs`:
- Around line 2556-2618: This regression test only covers the default 128 KiB
block cap, so add one exact-multiple case using a non-default target_block_size
to exercise the same EOF behavior through both compress_independent_frame and
the streaming set_source/compress path. Update
exact_block_multiple_marks_last_real_block to include a smaller configured block
size scenario and verify it still sets last_block on the final real block
without emitting a trailing empty Raw block, reusing the existing
frame_block_list and round-trip assertions.
In `@zstd/src/encoding/match_generator/mod.rs`:
- Around line 106-113: The fast-loop comment is stale and still documents
DFAST_SKIP_STEP_GROWTH_INTERVAL as 64 with an upstream deviation, but the
current DFAST_SKIP_SEARCH_STRENGTH and DFAST_SKIP_STEP_GROWTH_INTERVAL values
are 8 and 256 and match upstream. Update the explanatory comment near these
constants in dfast/mod.rs to reflect the current 256-byte growth interval and
remove any wording that implies this path diverges from upstream behavior.
🪄 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 Plus
Run ID: aa43cb57-28cc-4de5-ae47-814f90a3485f
📒 Files selected for processing (12)
zstd/benches/compare_ffi.rszstd/src/decoding/frame.rszstd/src/decoding/frame_decoder.rszstd/src/decoding/literals_section_decoder.rszstd/src/encoding/dfast/mod.rszstd/src/encoding/frame_compressor.rszstd/src/encoding/frame_compressor/tests.rszstd/src/encoding/match_generator/mod.rszstd/src/encoding/streaming_encoder.rszstd/src/huff0/huff0_decoder.rszstd/src/huff0/huff0_encoder.rszstd/src/io_nostd.rs
|
| Filename | Overview |
|---|---|
| zstd/src/encoding/dfast/mod.rs | Core dfast match-finder fixes: removes the bogus rep-candidate gate, corrects asymmetric per-table complementary insertion, anchors curr on the scan position not the match start, adds upstream _match_found lookahead long-hash insert under step<4, and removes the step cap. |
| zstd/src/decoding/literals_section_decoder.rs | HUF tail drain rewritten to mirror upstream HUF_decodeStreamX1: Phase 1 groups of 4 with one ensure_bits per group; Phase 2 one ensure_bits then up to 3 symbols with the new no-refill variant. |
| zstd/src/encoding/frame_compressor.rs | ReaderBlockSource gains a peeked byte for EOF detection on exact block boundaries; huf_search_enabled extended to gate DoubleFast the same as Fast. |
| zstd/src/huff0/huff0_encoder.rs | FSE-vs-direct-nibble threshold corrected from div_ceil(2) to /2, matching upstream's floor comparison. |
| zstd/src/huff0/huff0_decoder.rs | New decode_symbol_and_advance_no_refill using get_bits_unchecked; caller guarantees bits via ensure_bits. |
| zstd/src/encoding/match_generator/mod.rs | DFAST_SKIP_SEARCH_STRENGTH raised 6->8; DFAST_MAX_SKIP_STEP removed as step is now unbounded like upstream. |
| zstd/src/io_nostd.rs | Direct read_exact override for &[u8] and delegation from &mut T so the fast path is reached on the in-memory decode route. |
Reviews (2): Last reviewed commit: "fix(bench): require a terminal last-bloc..." | Re-trigger Greptile
The bulk-read of the variable header tail collapsed the per-field reads into a single read_exact, leaving WindowDescriptorReadError / DictionaryIdReadError / FrameContentSizeReadError unreachable in the public error surface. Read each field with its own error again (the slice read_exact override keeps each a single bounded copy, so the per-field reads carry no real cost), and address the review round: - frame_compressor: the huf_optimal_search field docs now mention DoubleFast is size-gated too, matching huf_search_enabled. - dfast: drop the stale "growth interval 64, deviates from upstream" comment; it is 256 and matches upstream kStepIncr now. - tests: the exact-block-multiple regression now also runs a non-default target_block_size, exercising the EOF path for both one-shot and streaming. - bench: compare_ffi's block-structure helper reports parse=error on a truncated/invalid frame instead of a misleading n_blocks=0.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
zstd/benches/compare_ffi.rs (1)
1128-1183: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire a terminal last-block before reporting success.
This still falls through to a normal
REPORT_BLKwhen the payload ends without ever seeinglast(including the case where 1-2 trailing bytes remain and cannot form a full block header). Those truncated frames will still be reported as valid block lists.Proposed fix
let mut blocks: Vec<(char, usize, bool)> = Vec::new(); + let mut saw_last = false; while pos + 3 <= payload_end { let h = compressed[pos] as usize | (compressed[pos + 1] as usize) << 8 | (compressed[pos + 2] as usize) << 16; @@ blocks.push((kind, bsize, last)); pos += 3 + advance; if last { + saw_last = true; break; } } + if !saw_last || pos != payload_end { + println!( + "REPORT_BLK scenario={} level={} encoder={} parse=error", + scenario.id, level.name, encoder + ); + return; + } let list: Vec<String> = blocks🤖 Prompt for 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. In `@zstd/benches/compare_ffi.rs` around lines 1128 - 1183, The block parsing in compare_ffi.rs still reports success even when the payload ends without encountering a terminal last block, so add an explicit check in the REPORT_BLK parsing path to require that the loop saw a block with last=true before printing the normal summary. Use the existing parsing flow around pos, payload_end, and blocks in the REPORT_BLK scenario handling to return parse=error whenever no final last block is reached, including cases where 1-2 trailing bytes remain and cannot form a full header.zstd/src/encoding/dfast/mod.rs (1)
1213-1263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winContradictory rationale in the rep-extension insertion comment.
The comment block spanning Lines 1213-1253 describes a sparse 3-target complementary insertion (
curr+2 / ip-2 / ip-1) and explicitly argues, at Lines 1237-1245, whyabs_posis not inserted on a rep extension. The trailing block at Lines 1254-1262 then states the opposite — that upstream writes both hash tables at the rep position — and the actual code does exactly that viaself.insert_position(abs_pos). The earlier half is stale and directly contradicts the final behavior, which is misleading in anunsafehot path that's tuned for upstream parity.Recommend trimming the obsolete 3-target/“
abs_posnot inserted” narrative so only thezstd_double_fast.c:314-315immediate-repcode rationale that matches the code remains.🤖 Prompt for 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. In `@zstd/src/encoding/dfast/mod.rs` around lines 1213 - 1263, The rep-extension comment in the dfast insertion path is internally inconsistent: the older sparse 3-target explanation and the claim that abs_pos is not inserted conflict with the actual immediate-repcode behavior. Update the comment around self.insert_position(abs_pos) to remove the stale curr+2 / ip-2 / ip-1 narrative and keep only the upstream parity explanation that matches the rep chain path and the code in insert_position.
🤖 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.
Outside diff comments:
In `@zstd/benches/compare_ffi.rs`:
- Around line 1128-1183: The block parsing in compare_ffi.rs still reports
success even when the payload ends without encountering a terminal last block,
so add an explicit check in the REPORT_BLK parsing path to require that the loop
saw a block with last=true before printing the normal summary. Use the existing
parsing flow around pos, payload_end, and blocks in the REPORT_BLK scenario
handling to return parse=error whenever no final last block is reached,
including cases where 1-2 trailing bytes remain and cannot form a full header.
In `@zstd/src/encoding/dfast/mod.rs`:
- Around line 1213-1263: The rep-extension comment in the dfast insertion path
is internally inconsistent: the older sparse 3-target explanation and the claim
that abs_pos is not inserted conflict with the actual immediate-repcode
behavior. Update the comment around self.insert_position(abs_pos) to remove the
stale curr+2 / ip-2 / ip-1 narrative and keep only the upstream parity
explanation that matches the rep chain path and the code in insert_position.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 27730b73-2d7b-47d5-9f97-030393238de3
📒 Files selected for processing (5)
zstd/benches/compare_ffi.rszstd/src/decoding/frame.rszstd/src/encoding/dfast/mod.rszstd/src/encoding/frame_compressor.rszstd/src/encoding/frame_compressor/tests.rs
The compare_ffi block-structure helper reported a normal REPORT_BLK even when the payload ended without a terminal `last` block (including 1-2 trailing bytes that cannot form a block header), making truncated frames look valid. Track `saw_last` and require both a terminal block and exact payload consumption before printing the block list, else report `parse=error`. Also trim the stale sparse-3-target narrative from the dfast immediate-repcode insertion comment: it argued `abs_pos` is not inserted, contradicting the actual `insert_position(abs_pos)` (upstream zstd_double_fast.c:314-315). Keep only the immediate-repcode rationale that matches the code.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Jun 26, 2026
Closed
Merged
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
1 participant
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
Two threads, both anchored on matching upstream zstd more closely:
level_3 dfast frame on repetitive data was larger than C's and our own
decoder was slower on the non-canonical stream. The match-finder had drifted
from upstream in several places; with them fixed the level_3 sequence
comparator is now 100% identical to upstream across every fixture, and a
small structured-log frame compresses to a byte-identical frame.
non-inlined per-frame init chain dominated tiny-frame decode; both are now
shaped like upstream.
byte-identical round-trip across 796 tests incl. the 1000-iteration random
suite. Perf measured on an i9 (BMI2 + AVX2), ours vs
c_ffi(libzstd) on thesame host.
dfast sequence fidelity
cand_pos_r >= literals_startgatethat rejected essentially every rep, forcing the long hash to mint a creeping
offset stream instead of a stable rep.
position (both tables), and make the post-match complementary insertion
asymmetric (long: curr+2, ip-2; short: curr+2, ip-1).
kSearchStrength = 8(one step per 256 bytes) and letthe step grow unbounded, instead of 6 (64 bytes) capped at 8 -- the port
accelerated ~4x too soon and missed the short matches upstream finds near a
block start.
curr), not the match start -- for a rep1 the match begins atscan+1and abackward catch-up rewinds the start, so the old anchor shifted every insert
and seeded the short hash with the wrong positions (the compounding bug).
Effect on level_3 (rust vs
c_ffibytes):large-log-stream11222 -> 8941(was +15% over C, now beats C 9744);
decodecorpus-1m249 -> 228; the sequencecomparator is 100% identical to upstream on all fixtures.
#167 table-log search size gate
The optional table-log search trades encode time for a smaller literal section.
Fast already skipped it below one block; extend that gate to DoubleFast. Small
dfast frames now use the cheap upstream-equivalent build (byte-identical frame),
large frames keep the search and beat upstream on the literal section.
Decode perf
HUF_decodeStreamX1): decode the 4-stream tailin groups of four with one reload per group, then the final under-four with no
per-symbol reload. The end-of-stream check is unchanged and still exact
(
(64 - bits_consumed) - extra_bitsis reload-timing-independent). small-4kliteral-heavy decode 2.2 -> 1.9 us (1.59x -> 1.40x C).
#[inline]thedecode_all -> init -> reset -> reset_with_format -> headerchain so LLVM collapses the per-frame setup intoa flat path like upstream. small-1k 82 -> 58 ns (1.95x -> 1.31x C);
small-10k / decodecorpus / high-entropy now beat C.
read_exactforslice readers.
Decode results (level_3 dfast, i9, rust vs c_ffi)
Also
emitted when the input was an exact multiple of 128 KiB).
Testing
cargo nextest run -p structured-zstd: 796 passed (incl. 1000-iterationrandom round-trip and borrowed/owned equivalence).
Summary by CodeRabbit
no_stdsliceread_exactbehavior.