perf(levels): consolidate cparams + negative-band byte-parity with C#454
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ffi-bench benchmark and example targets, routes level resolution through cparams, propagates literal-compression disablement through encoder paths, and updates sequence/FSE handling plus row and decoder internals. ChangesEncoder, entropy, and benchmark updates
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 |
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@ffi-bench/examples/zrip_compare.rs`:
- Around line 73-109: The benchmark in zrip_compare should verify round-trip
correctness for every codec before timing so invalid decodes are never reported
as valid performance results. In the loop over levels, add decode-and-compare
checks for the libzstd path and the zrip path using their existing helpers
(`zstd::bulk::decompress`, `zrip::decompress`, and the
`FrameDecoder`/`compress_slice_to_vec` pattern), and skip or fail the
measurement if the decoded bytes do not match `data`. Also make the current
structured-zstd `ok` check actually gate the recorded timings instead of just
computing a boolean and continuing.
In `@zstd/benches/compare_ffi_sequences.rs`:
- Around line 300-315: Reuse the existing corpus-path fallback logic for the new
small fixtures in compare_ffi_sequences. The new z000002 and z000000 probes
should use the same path resolution strategy already applied for z000033, so
they work in direct-binary and repo-root layouts as well as the ffi-bench
manifest-relative case. Update the small-fixtures loading block to factor in the
existing fallback path handling rather than only reading from
env!("CARGO_MANIFEST_DIR")/../zstd/decodecorpus_files.
- Around line 172-177: The negative level parsing in compare_ffi_sequences.rs
still misclassifies comma-separated inputs containing minus signs as ranges, so
values like -7,-5, 3,-1, and -7--1 fall back to defaults. Update the parsing
logic around the trimmed value handling so the comma-separated list path is
tried before any split_once('-') range parsing, while keeping the existing bare
integer handling in the same parsing block.
In `@zstd/src/encoding/frame_compressor.rs`:
- Around line 1051-1054: `set_compression_level()` leaves
`state.literal_compression_disabled` stale, so switching a reused
`FrameCompressor` to a negative `CompressionLevel::Level(n)` can keep literal
Huffman compression enabled for the next frame. Update `set_compression_level()`
in `FrameCompressor` to also recompute and store
`state.literal_compression_disabled` from the new `compression_level`, matching
the logic already used in the constructors and `set_parameters()` so
`prepare_frame()` sees the correct flag.
In `@zstd/src/encoding/levels/config.rs`:
- Around line 643-645: The source-size clamp in adjust_params_for_source_size
still uses the old MIN_HINTED_WINDOW_LOG floor, so hinted inputs keep the 16 KiB
minimum via match_generator::mod. Update adjust_params_for_source_size in
config.rs to use the same lower-window clamp behavior as the main resolver path,
ensuring the window/hash/chain logs are reduced consistently for small source
sizes and hinted inputs no longer retain the old floor.
🪄 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: e5b4c374-4f8b-4d7d-92ac-f6aa279befbe
📒 Files selected for processing (16)
ffi-bench/Cargo.tomlffi-bench/examples/encode_small.rsffi-bench/examples/interop_small.rsffi-bench/examples/ratio_parity.rsffi-bench/examples/zrip_compare.rszstd/benches/compare_ffi_sequences.rszstd/src/encoding/blocks/compressed.rszstd/src/encoding/blocks/compressed/tests.rszstd/src/encoding/cparams.rszstd/src/encoding/frame_compressor.rszstd/src/encoding/levels/config.rszstd/src/encoding/levels/fastest/tests.rszstd/src/encoding/match_generator/tests.rszstd/src/encoding/row/mod.rszstd/src/encoding/streaming_encoder.rszstd/src/fse/fse_encoder.rs
…perf Resolve every level's compression parameters through the single C-faithful `get_cparams` source (port of `ZSTD_getCParams`), dropping the dead hand-tuned `LEVEL_TABLE` / BtUltra2 configs and the parallel resolver. Route the param-override source-size re-cap through the same byte-verified `adjust_cparams` (`ZSTD_adjustCParams_internal`) so the override and level paths down-size identically, removing a divergent helper and its non-C 16 KiB hinted-window floor. Negative compression levels now match C byte-for-byte: raw (uncompressed) literals when literal compression is disabled, the fast-matcher repcode policy (offBase 1 only), and the buildCTable last-symbol histogram adjustment. `set_compression_level` resyncs the literal-disable gate so a reused compressor switched to a negative level emits raw literals (regression-tested). Decode: port C's two-stage branchless FSE decode-table spread for the no-low-prob-count case, and make the inline FSE decode table `MaybeUninit` so a fresh `FrameDecoder` skips the ~13 KiB per-table memset that dominated tiny-frame decode (z000001 24 B decode 0.13x -> 0.36x of C). Benches: per-level byte-diff in ratio_parity, round-trip validation across all codecs in the comparison example, negative-band level-list parsing + corpus-path fallback in compare_ffi_sequences, and small-input encode/decode harnesses. 797 tests pass; our-vs-C cross-validation and fuzz-interop round-trips green; clippy + fmt clean.
2b9f2bd to
0e06202
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
zstd/benches/compare_ffi_sequences.rs (1)
214-248: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
STRUCTURED_ZSTD_BENCH_LEVEL=allstill skips the negative band.This parser now accepts
-7..=-1, but the"all"fast path above still returns only positive levels, so the new negative-level coverage disappears for the natural “run everything” setting.Proposed fix
- if trimmed.eq_ignore_ascii_case("all") { - return (1..=MAX_SUPPORTED_LEVEL).collect(); - } + if trimmed.eq_ignore_ascii_case("all") { + return (-7..=MAX_SUPPORTED_LEVEL).filter(|&l| l != 0).collect(); + }🤖 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_sequences.rs` around lines 214 - 248, The `STRUCTURED_ZSTD_BENCH_LEVEL` parser in `compare_ffi_sequences.rs` still drops the negative levels when the `"all"` path is used. Update the `"all"` branch in the same parsing logic that builds `parsed`/`supported` so it includes the full supported band, including `-7..=-1`, instead of returning only positive levels. Keep the existing single-value, list, and range handling intact, but make sure the `all` fast path matches the new negative-level coverage.zstd/src/encoding/levels/config.rs (1)
564-570: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClamp BT
search_mlsto the upstream 4..6 range.
HcConfig’s own contract here isBOUNDED(4, minMatch, 6), but this path copiescp.min_matchverbatim. For BtUltra/BtUltra2 rows whereget_cparams()returnsmin_match = 3, the resolver now builds a 3-byte BT hash width instead of the required 4-byte one, which changes matcher geometry for levels 18+.Suggested fix
let hc = HcConfig { hash_log: cp.hash_log as usize, chain_log: cp.chain_log as usize, search_depth, target_len, - search_mls: cp.min_match as usize, + search_mls: cp.min_match.clamp(4, 6) as usize, };🤖 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/levels/config.rs` around lines 564 - 570, Clamp the BT matcher width in the HcConfig construction so `search_mls` always stays within the upstream 4..6 contract instead of copying `cp.min_match` directly. Update the `HcConfig` setup in `config.rs` to derive `search_mls` from `cp.min_match` with a lower bound of 4 and upper bound of 6, so the BtUltra/BtUltra2 paths used by `get_cparams()` cannot produce a 3-byte width. Keep the change localized to the `HcConfig` initializer so the resolver behavior for levels 18+ matches the expected matcher geometry.zstd/src/encoding/match_generator/tests.rs (1)
4888-4919: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a BT-level regression for bounded
search_mls.This suite now checks the fast negative-level mapping, but it still misses the BT rows where
min_match = 3must resolve tohc.search_mls == 4. Without that assertion, the newlevel_params_from_cparams()bug on levels 18+ slips through.Example assertion to add
let pf = resolve_level_params(CompressionLevel::Fastest, None); let ff = pf.fast.unwrap(); assert_eq!( (pf.window_log, ff.hash_log, ff.mls, ff.step_size), (19, 14, 7, 2), ); + + let l18 = resolve_level_params(CompressionLevel::Level(18), None); + let hc18 = l18.hc.unwrap(); + assert_eq!(hc18.search_mls, 4, "BT min_match=3 must still hash 4 bytes");🤖 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/match_generator/tests.rs` around lines 4888 - 4919, Add a BT-level regression in the existing match_generator tests to verify that bounded search_mls resolves correctly for BT rows. Update the test suite around resolve_level_params()/level_params_from_cparams() to assert that when min_match is 3, the BT config produces hc.search_mls == 4, covering the levels 18+ path that is currently missing.zstd/src/encoding/frame_compressor.rs (1)
1041-1054: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDerive
literal_compression_disabledfrom the resolved parameters, not justlevel < 0.Lines 1041-1048 let
set_parameters()override the effective strategy, but Lines 1051-1054 still force this flag purely from the base level. That makes a config likeCompressionParameters::builder(CompressionLevel::Level(-5)).strategy(Strategy::BtUltra2)run withstrategy_tag == BtUltra2while still forcing RAW literals, which contradicts the documented upstream rule (strategy == fast && targetLength > 0). Please compute this from the resolved/overridden cParams instead of the signed level.🤖 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/frame_compressor.rs` around lines 1041 - 1054, `set_parameters()` in `FrameCompressor` currently updates `strategy_tag` from overrides but derives `literal_compression_disabled` only from `compression_level`, which can leave the flag inconsistent with the resolved parameters. Update the `FrameCompressor::set_parameters` logic to compute `literal_compression_disabled` from the effective cParams/overridden strategy and target length, using the same resolved values that drive `strategy_tag` and `huf_optimal_search`, so the flag matches the documented upstream rule instead of relying on a signed level check.
🤖 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 `@ffi-bench/examples/ratio_parity.rs`:
- Around line 17-20: The parity example is treating a failed libzstd decode of
ours as just false, so the regression still exits successfully. Update the logic
in ratio_parity.rs around the c_decodes_ours check to fail fast when
zstd::bulk::decompress cannot decode ours, using the existing ratio_parity-style
flow and any surrounding main/benchmark control to return a non-zero exit or
panic with context instead of swallowing the error.
In `@ffi-bench/examples/zrip_compare.rs`:
- Around line 103-118: The zrip benchmark fallback currently maps unsupported
compression levels to zero timing via the zrip::compress match, which makes them
look like real throughput results. Update the zrip roundtrip/timing logic in
zrip_compare.rs so the unsupported case is represented as N/A/None instead of
(0, 0), and propagate that through the per-level table and worst-first summary
formatting so zrip entries are skipped or printed as unavailable rather than
0.00×.
In `@zstd/src/fse/fse_decoder.rs`:
- Around line 648-691: The fast-spread branch in build_decoding_table_inner
assumes a fixed 512+8 scratch buffer, but build_from_probabilities can produce
larger table_size values, so the in_order lay-down can overflow for valid
inputs. Restrict this optimization to table_size <= 512 or allocate the scratch
based on table_size before the symbol-distribution loop. Keep the check near the
no-low-probability fast path so the generic decoder logic still handles larger
tables safely.
---
Outside diff comments:
In `@zstd/benches/compare_ffi_sequences.rs`:
- Around line 214-248: The `STRUCTURED_ZSTD_BENCH_LEVEL` parser in
`compare_ffi_sequences.rs` still drops the negative levels when the `"all"` path
is used. Update the `"all"` branch in the same parsing logic that builds
`parsed`/`supported` so it includes the full supported band, including
`-7..=-1`, instead of returning only positive levels. Keep the existing
single-value, list, and range handling intact, but make sure the `all` fast path
matches the new negative-level coverage.
In `@zstd/src/encoding/frame_compressor.rs`:
- Around line 1041-1054: `set_parameters()` in `FrameCompressor` currently
updates `strategy_tag` from overrides but derives `literal_compression_disabled`
only from `compression_level`, which can leave the flag inconsistent with the
resolved parameters. Update the `FrameCompressor::set_parameters` logic to
compute `literal_compression_disabled` from the effective cParams/overridden
strategy and target length, using the same resolved values that drive
`strategy_tag` and `huf_optimal_search`, so the flag matches the documented
upstream rule instead of relying on a signed level check.
In `@zstd/src/encoding/levels/config.rs`:
- Around line 564-570: Clamp the BT matcher width in the HcConfig construction
so `search_mls` always stays within the upstream 4..6 contract instead of
copying `cp.min_match` directly. Update the `HcConfig` setup in `config.rs` to
derive `search_mls` from `cp.min_match` with a lower bound of 4 and upper bound
of 6, so the BtUltra/BtUltra2 paths used by `get_cparams()` cannot produce a
3-byte width. Keep the change localized to the `HcConfig` initializer so the
resolver behavior for levels 18+ matches the expected matcher geometry.
In `@zstd/src/encoding/match_generator/tests.rs`:
- Around line 4888-4919: Add a BT-level regression in the existing
match_generator tests to verify that bounded search_mls resolves correctly for
BT rows. Update the test suite around
resolve_level_params()/level_params_from_cparams() to assert that when min_match
is 3, the BT config produces hc.search_mls == 4, covering the levels 18+ path
that is currently missing.
🪄 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: 28a870c5-9ad2-4808-8826-4548bf2268cd
📒 Files selected for processing (19)
ffi-bench/Cargo.tomlffi-bench/examples/encode_small.rsffi-bench/examples/interop_small.rsffi-bench/examples/ratio_parity.rsffi-bench/examples/zrip_compare.rszstd/benches/compare_ffi_sequences.rszstd/benches/decode_loop.rszstd/src/encoding/blocks/compressed.rszstd/src/encoding/blocks/compressed/tests.rszstd/src/encoding/cparams.rszstd/src/encoding/frame_compressor.rszstd/src/encoding/frame_compressor/tests.rszstd/src/encoding/levels/config.rszstd/src/encoding/levels/fastest/tests.rszstd/src/encoding/match_generator/tests.rszstd/src/encoding/row/mod.rszstd/src/encoding/streaming_encoder.rszstd/src/fse/fse_decoder.rszstd/src/fse/fse_encoder.rs
The FSE table selector takes eight cohesive inputs, each carrying its own perf / correctness rationale inline (the no-copy `&mut` histogram, the caller-tracked `max_symbol` / `last_code` that avoid a per-stream rescan). `cargo clippy -D warnings` (the CI gate) rejects the 8/7 count; `#[expect]` it with a reason so the documented per-arg rationale stays at the signature instead of being relocated into a struct.
`build_from_probabilities` accepts acc_log up to ENTRY_MAX_ACCURACY_LOG (16),
but the no-low-prob fast-spread path lays symbols into a fixed `512 + 8` scratch
and writes the decode table into a `[_; CAP]` array (CAP = 512 for sequence
tables). A valid no-`-1` table with table_size > CAP overruns both. This test
drives a 1024-entry sequence table through the public entry and currently panics
("range end index 528 out of range for slice of length 520"); it passes once the
build rejects table_size > CAP.
`build_decoding_table_inner` writes the decode table into a `[E; CAP]` array and
the no-low-prob fast path lays symbols into a fixed `FSE_FAST_SPREAD_BUF` (512+8)
scratch. Both assume `table_size <= CAP`, which the wire path guarantees
(sequence acc_log <= 9, HUF <= 6) but `build_from_probabilities` did not — it
caps acc_log at ENTRY_MAX_ACCURACY_LOG (16), so a fuzz / external caller could
drive a table_size > CAP and panic-overrun the scratch ("range end index 528 out
of range for slice of length 520") before the table was built.
Reject `table_size > CAP` up front as a typed `AccLogTooBig`, so the invariant
both the decode array and the fast-spread scratch rely on is enforced once.
Passes the regression test from the preceding commit; full suite green (798).
…els N/A ratio_parity: panic with context when the C decoder cannot decode `ours` (`expect` + `assert_eq`) instead of printing `false` and exiting 0 on the exact drop-in regression the example exists to catch. zrip_compare: carry the zrip/C ratios as `Option` and print `N/A` for levels zrip does not support, instead of `0.00x` that read as real throughput misses in both the per-level table and the worst-first summaries. Also clarify the `build_seq_ctable` last-symbol normalization: C gates both `count[last]--` and `nbSeq_1--` on `count > 1` (zstd_compress_sequences.c:271-273), so a count of exactly 1 normalizes against the full `nbSeq`, not `nbSeq - 1` — the doc and an inline comment now spell this out so it is not misread as a parity divergence.
`set_parameters` derives `literal_compression_disabled` from the signed level, but C `ZSTD_literalsCompressionIsDisabled` keys it off the RESOLVED cParams (`strategy == fast && targetLength > 0`). A negative level overridden onto a non-fast strategy (e.g. BtUltra2) should keep literal (Huffman) compression enabled; the current code wrongly forces raw literals. This test fails until the flag is computed from the resolved strategy/target length.
…d level `set_parameters` forced `literal_compression_disabled` from `level < 0`, so a negative level overridden onto a non-fast strategy (e.g. BtUltra2) kept raw literals even though the resolved strategy is no longer fast. Compute it like C `ZSTD_literalsCompressionIsDisabled` (ps_auto) instead: `strategy_tag == Fast && targetLength > 0`, using the already-resolved (overridden) strategy tag and the target-length override (falling back to `level < 0`, which for the fast strategy is exactly `targetLength > 0`). The constructors and `set_compression_level` take no strategy override, so their native-strategy `level < 0` stays correct. Passes the regression test from the preceding commit; full suite green (799).
…mls doc compare_ffi_sequences: `STRUCTURED_ZSTD_BENCH_LEVEL=all` now sweeps the negative band too (`-7..=MAX`, dropping 0), so the natural "run everything" setting does not silently skip the negative-level coverage the per-level parser already accepts. config.rs: the BT-finder `search_mls` doc claimed `BOUNDED(4, minMatch, 6)`, but upstream `ZSTD_selectBtGetAllMatches` (zstd_opt.c:896) uses `BOUNDED(3, minMatch, 6)`. Every BT level's minMatch is already in `[3, 6]` (btultra/btultra2 L18-22 = 3), so the existing `search_mls = cp.min_match` is correct; clamping up to 4 would build a 4-byte BT hash on levels 18+ and diverge from C's 3-byte one. Doc corrected to prevent that mis-"fix".
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/benches/compare_ffi_sequences.rs (1)
226-230: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject malformed list items instead of silently dropping them.
filter_map(...ok())turns inputs like-7,fooor3,into partial success, so requested levels can be skipped with no warning. Parse the whole list strictly and fall back if any element is invalid.Proposed fix
} else if trimmed.contains(',') { - trimmed - .split(',') - .filter_map(|s| s.trim().parse::<i32>().ok()) - .collect() + { + let mut levels = Vec::new(); + for part in trimmed.split(',') { + match part.trim().parse::<i32>() { + Ok(level) => levels.push(level), + Err(_) => { + eprintln!( + "warn: STRUCTURED_ZSTD_BENCH_LEVEL={trimmed:?} is not a valid list; \ + falling back to default levels {DEFAULT_LEVELS:?}", + ); + return DEFAULT_LEVELS.to_vec(); + } + } + } + levels + }🤖 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_sequences.rs` around lines 226 - 230, The comma-separated parsing in compare_ffi_sequences.rs is too permissive because the current filter_map(...ok()) path silently drops invalid items and can partially accept malformed inputs. Update the list parsing branch in the trimmed.contains(',') handling to validate every element strictly, and if any item fails to parse in the sequence, reject the entire list and fall back to the non-list behavior instead of returning a partial Vec<i32>. Use the existing parsing logic in this branch as the place to tighten validation.
🤖 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 `@ffi-bench/examples/zrip_compare.rs`:
- Around line 204-215: The decode “misses” summary in the `dec_miss` block is
excluding ratios from 0.95 to 1.00 even though the printed label says “ours/C,”
so those regressions are hidden. Update the filtering in this reporting section
to include all cases below 1.0, or if the 0.95 tolerance is intentional, rename
the summary/label to make the threshold explicit; keep the change localized
around `dec_miss` and the decode summary print loop.
---
Outside diff comments:
In `@zstd/benches/compare_ffi_sequences.rs`:
- Around line 226-230: The comma-separated parsing in compare_ffi_sequences.rs
is too permissive because the current filter_map(...ok()) path silently drops
invalid items and can partially accept malformed inputs. Update the list parsing
branch in the trimmed.contains(',') handling to validate every element strictly,
and if any item fails to parse in the sequence, reject the entire list and fall
back to the non-list behavior instead of returning a partial Vec<i32>. Use the
existing parsing logic in this branch as the place to tighten validation.
🪄 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: c6122ef0-b9aa-4438-b8ef-7f4debd331e2
📒 Files selected for processing (11)
ffi-bench/Cargo.tomlffi-bench/examples/ratio_parity.rsffi-bench/examples/zrip_compare.rszstd/benches/compare_ffi_sequences.rszstd/src/encoding/blocks/compressed.rszstd/src/encoding/frame_compressor.rszstd/src/encoding/frame_compressor/tests.rszstd/src/encoding/levels/config.rszstd/src/fse/fse_decoder.rszstd/src/fse/fse_decoder/tests.rszstd/src/fse/fse_encoder.rs
The cparams consolidation set `search_mls = cp.min_match`, which is 3 on the btultra/btultra2 rows (L18-22). C's BT finder does use mls=3 there (`BOUNDED(3, minMatch, 6)`, zstd_opt.c:896) and surfaces 3-byte matches via a fallback-only HC3 finder, but our optimal parser does not yet replicate that 3-byte handling — it emits short matches C prices out, which broke `level22_sequences_match_reference` (our sequences diverged from C's: 1217 vs 1160 sequences, first split at idx 44). Restore the long-standing workaround: clamp the BT hash width up to 4 so the finder stops surfacing the 3-byte matches our parser mis-prices, matching C's output again. This is deliberately NOT C's finder width; the proper fix is to make the optimal parser C-faithful at minMatch 3 (tracked in #337), after which the clamp can be dropped. Corrects the earlier doc that wrongly claimed the clamp would diverge from C. Verified: full lib suite (799) and the bench_internals ffi suite (58, incl. the level-22 sequence-parity test) green.
The speed-misses summary filters at ours/C < 0.95, not < 1.0, so sub-5% slower-than-C decodes are intentionally excluded. Rename the header to state the >5% threshold so the report does not read as "every case below 1.0".
The dict test compressed a payload whose lines only shared a 24-byte prefix with the trained dictionary while otherwise self-repeating, so the dict's marginal gain was below the dict-id frame overhead — the assertion passed by a single byte. The cparams consolidation legitimately improved the no-dict baseline (level-1 Fastest 234 -> 221 bytes, now at/under C), which erased that 1-byte margin and flipped the test. Use a payload the dictionary actually covers (the training line shape with unseen `idx` values) so the dict's benefit is the first occurrence's full-line literals — substantial and unambiguous — instead of a fragile sub-overhead margin. The deeper partial-match dict ratio gap is a separate concern, not gated by this round-trip + basic-benefit test. The failure message now reports both sizes. Verified with the CI feature set (-F hash,std,dict_builder): full lib suite and the bench_internals ffi suite green.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
Consolidates all compression-level resolution onto the single C-faithful
get_cparamssource, then closes the remaining encoder divergences from the Creference so the entire negative-level band is byte-for-byte identical to
upstream zstd.
Level-param consolidation
level_params_from_cparams(get_cparams(...)). Drops the parallel hand-tunedLEVEL_TABLE, the per-strategyROW_*/DFAST_*/FAST_*configs, thecparams_tiermachinery, and the bespoke BtUltra2 source-size table — all ofwhich had drifted from the C parameter formula and were a recurring source of
param mismatches.
our small-window frames still decode in the C reference).
Negative-band byte-parity with the C reference
Three independent encoder divergences, each found by reading the C source in
full and confirmed by decoding both frames:
ZSTD_literalsCompressionIsDisabled(
strategy == fast && targetLength > 0): emit raw literals, skipping theHuffman pass the C reference deliberately skips for speed. We previously
Huffman-coded them, doing extra work for a smaller frame.
(rep[0] / rep[1]) or an explicit offset, never offBase 2/3. We probed all
three repeats per sequence and rewrote explicit offsets that coincided with a
deeper repeat, paying a probe the fast matcher never pays and shifting the FSE
histogram. Gated to the fast strategy with the branch hoisted out of the
per-sequence loop.
the final sequence's symbol (it is coded via the FSE init-state) and
normalizes against
nbSeq - 1, with the table-log still from the fullnbSeq. The histogram is borrowed&mutand restored, so there is noper-table copy.
Also removes the lazy row parse's non-C
match_len >= target_lenearly-out,which had collapsed the source-size-tiered lazy levels to greedy.
Results
-7..-1) on the small decodecorpus frame are nowbyte-for-byte identical to the C reference; no ratio-floor regression
anywhere, and lazy-band ratio floors improve (e.g. L7 0.929x -> 0.969x of C).
op-reduction (one fewer per-sequence probe), and the FSE adjustment is two
integer ops per table with no allocation.
Testing
cargo nextest run -p structured-zstd: 796 passed, 1 skipped.Adds three encoder-profiling / parity bench harnesses (
encode_small,ratio_parity, plus negative-level support in the sequence-capture bench).Summary by CodeRabbit