perf(huf+lazy): gate HUF table-log on the size-adaptive strategy, unify the lazy parser#455
Merged
Conversation
Hoist the lazy lookahead's commit-vs-defer decision (the gain comparison, depth-1/depth-2 bias, bounds) into one shared helper, mirroring upstream's single ZSTD_compressBlock_lazy_generic instead of a per-strategy copy. Wire the HC matcher to it (byte-identical) and route HcMatcher::match_gain through the shared gain fn. Row and dfast follow.
Row's lazy lookahead used a raw match-length comparison; route it through lazy_parse::lazy_should_commit so it weighs candidates by upstream's gain (match_len*4 - offset_bits) like HC/C, keeping Row's carry-forward (the FnMut closure captures the ip+1 result) and its no-sufficient-length policy (target_len = MAX). Behavior-changing on the Row band (the lazy outsiders); to be ratio+speed verified on the bench host before merge.
This reverts commit dc493a5.
This reverts commit d6eb1f3.
The fn+closure driver de-inlined Row's #[target_feature] finder across the call boundary (+~6% on the lazy band, measured at dc493a5). Replace it with a macro_rules! that splices the finder inline at the call site — upstream's lazy parse is one FORCE_INLINE_TEMPLATE with the search method as a template param, so the finder is never behind a call boundary. HC keeps its out-of-line finder (it ignores the carry and re-picks), Row inlines its finder and uses the carry. One source for the C-faithful gain decision, no per-strategy copies.
…hared HC carried its own copy of the catch-up loop; Row already delegates to match_table::helpers::extend_backwards_shared. Make HC a thin wrapper too so the back-extension (upstream zstd_lazy.c:1707-1718 parity) has one implementation. Byte-identical.
…istic The gain weighting (committed earlier) cost ~6% on the lazy band with no ratio win — the length heuristic that Row already used is faster and still beats C (drop-in, not binary parity). Switch lazy_decide! to length: Row's monolith returns to its prior (faster) behaviour, HC moves from gain to length (one source). best_off is bound once; with target_len=usize::MAX (Row) the sufficient-length early-out folds away, so the hot loop gains no branch.
pick_lazy_match_rl (a dead-in-production, test-only path) carried its own copy of the lazy decision via pick_lazy_match_shared + LazyMatchConfig. Route it through the same lazy_decide! macro the production monolith uses and delete the now-unused fn + config struct. The tests now exercise the exact decision that ships. Byte-identical.
The literal-compression / HUF table-log-search gate read strategy_tag from the bare compression level, but the matcher resolves strategy size-adaptively (upstream ZSTD_getCParams: a <=16 KiB frame promotes levels 13-17 to btultra/btultra2). On small frames the gate therefore thought btlazy2/btopt and skipped the HUF table-log search the matcher's btultra frame should run, overshooting C by 4 bytes on the small-4k-log-lines literal section (L13-17). Resolve strategy_tag through the same resolve_level_params -> get_cparams path the matcher uses, so the gate and the parse agree. small-4k-log-lines L13-17 go 150 -> 146, matching C; full local ratio matrix shows no new rust>ffi cases.
…hain walk HcMatcher::better_candidate ranked the repcode probe against the hash-chain match by gain (ml*4 - offset_bits), while the chain walk itself, lazy_decide!, and the shared Row/Dfast repcode probe all rank by length. Upstream ZSTD_compressBlock_lazy_generic compares the repcode against the searched match by length at depth 0 (ml2 > matchLength, the repcode keeps ties). Rank by length (ties to the smaller offset) so HC's selection is internally consistent and C-faithful. Drops the now-unused match_gain / lazy_match_gain.
The HC lazy loop hand-inlined the catch-up back-extension with slice indexing, a fourth copy of the same logic the Row / Dfast probes reach through match_table::helpers::extend_backwards_shared. Call the shared helper (it is #[inline] and raw-pointer, so the hot loop also sheds the per-step slice bounds checks). One back-extension source; byte-identical output.
…ed position The HC lazy loop searched the lookahead position inside pick_lazy_match, threw it away, then re-searched it next iteration (double work on every defer). Carry the lookahead match forward like the Row parser and upstream's lazy depth loop (each position searched once). Insert abs_pos before the lookahead so the abs_pos+1 probe sees it at offset 1 — matching upstream, where the searched position is inserted during its own search before the depth loop probes the next. Behavior-changing (the defer decision now sees the offset-1 candidate); i9 ratio + speed verification follows.
lazy_decide_carry returned Option<Option<HcMatch>> (>16 bytes, stack-returned via sret), marshalled across the hot per-position boundary on every match — it spilled the register-tight lazy2 (depth-2) monomorph and cost ~2.3% on the small-10k-random L10/L12 band. Flatten the return to a bare 16-byte HcMatch (System V rax:rdx): the NONE sentinel means commit, a real match means defer-carry. Byte-identical output.
|
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)
📝 WalkthroughWalkthroughThe PR centralizes lazy-parse decisions into ChangesLazy parse flow
Table-log selection and strategy hints
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❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/src/encoding/frame_compressor.rs`:
- Around line 1664-1669: The eager strategy synchronization in set_parameters
still uses the level-only mapping, so it can leave state.strategy_tag stale
before prepare_frame runs. Update the set_parameters path to resolve the
strategy with the same size-aware call used by prepare_frame, specifically
through resolve_level_params(self.compression_level, self.source_size_hint), so
the early state stays consistent with the existing size hint.
In `@zstd/src/encoding/hc/mod.rs`:
- Around line 730-745: The match selection in the HC lazy path is collapsing the
depth-2 defer-without-carry case into HcMatch::NONE, which loses the intended
one-byte advance behavior when lazy_decide! returns Some(None). Update the match
around lazy_decide! in hc::mod so that Some(None) is preserved as an explicit
defer state (or handle it at the loop callsite by treating Some(carry) as
advance with carried = carry), while only using HcMatch::NONE for the true
no-defer case.
🪄 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: bfd70299-c43a-4ce9-aec7-d7e752cafb85
📒 Files selected for processing (10)
ffi-bench/tests/frame_compressor_ffi.rszstd/src/encoding/frame_compressor.rszstd/src/encoding/hc/generator.rszstd/src/encoding/hc/mod.rszstd/src/encoding/lazy_parse.rszstd/src/encoding/match_table/helpers.rszstd/src/encoding/mod.rszstd/src/encoding/row/mod.rszstd/src/fse/fse_encoder.rszstd/src/huff0/huff0_encoder.rs
💤 Files with no reviewable changes (1)
- zstd/src/encoding/match_table/helpers.rs
|
Declaring pub(crate) mod lazy_parse wedged it between the #[cfg(feature = hash)] and the mod ldm that attribute guards, so lazy_parse became hash-gated (absent in no_std core compression) and ldm lost its gate (unresolved twox_hash without the hash feature). lazy_parse is core; ldm is the hash-gated module.
…itting lazy_decide_carry collapsed the macro's Some(None) into the NONE sentinel, which the generator read as COMMIT. Some(None) is reachable at lazy_depth>=2 when the abs_pos+2 probe wins but abs_pos+1 has no match (cold bucket); it must DEFER one byte (searching the next position fresh), or lazy2 misses the two-ahead match. Return Option<HcMatch>: None=commit, Some(real)=defer-with-carry, Some(NONE)=defer-without-carry. Resolve the eager set_parameters strategy_tag sync size-adaptively through the same resolve_level_params path prepare_frame uses, not the bare level mapping. Reword the row lazy_parse_body inline note: the decision weighs by length (ties to the smaller offset), matching the macro after the gain helper removal. Add lazy_parse macro-contract tests: the Some(None) defer, the carry defer, the plain commit, and the target_len early-out.
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 (3)
zstd/src/encoding/hc/generator.rs (1)
1273-1277: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the defer-without-carry state through the next miss.
When
lazy_decide_carry()returns theNONEsentinel, Line 1316 erases the fact that we deferred. Ifabs_pos + 1then has no match, the loop falls into the normal skip heuristic and can jump past the already-provenabs_pos + 2winner.Suggested fix
- let mut carried: Option<HcMatch> = None; + let mut carried: Option<HcMatch> = None; + let mut deferred_without_carry = false; while pos + HC_MIN_MATCH_LEN <= current_len { let abs_pos = current_abs_start + pos; let lit_len = pos - literals_start; + let forced_single_step = core::mem::take(&mut deferred_without_carry); let best = match carried.take() { Some(m) => m, None => self.hc.find_best_match::<DICT>( @@ if let Some(carry) = self.hc.lazy_decide_carry::<DICT>( concat, dms_primed, &self.table, abs_pos, lit_len, best, ) { + deferred_without_carry = !carry.is_match(); carried = carry.is_match().then_some(carry); pos += 1; continue; } @@ - let step = ((pos - literals_start) >> 8) + 1; + let step = if forced_single_step { + 1 + } else { + ((pos - literals_start) >> 8) + 1 + }; pos += step;Also applies to: 1286-1295, 1302-1317
🤖 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/hc/generator.rs` around lines 1273 - 1277, The lazy carry state in generator.rs is being cleared too early: when lazy_decide_carry() returns the NONE sentinel, the loop loses the fact that the previous position was deferred and should keep that defer state through the next miss. Update the carry-handling logic around carried and the main search/skip loop so a NONE result preserves the “deferred without carry” state instead of resetting to a normal fresh-search path, and ensure the fallback skip heuristic does not bypass the already-proven winner at abs_pos + 2. Refer to lazy_decide_carry(), carried, and the NONE sentinel handling when adjusting the control flow.zstd/src/encoding/row/mod.rs (2)
549-580: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Some(None)still degrades into a plain miss here.This block preserves the depth-2 defer-without-carry result as
carried = Some(None), but the next iteration'scarried.take()clears that outer defer flag before the deferredabs_pos + 1miss is handled. Theelsebranch then takes the normal skip path, which can hop over the knownabs_pos + 2winner.Suggested fix
- let mut carried: Option<Option<MatchCandidate>> = None; + let mut carried: Option<Option<MatchCandidate>> = None; while pos + mls <= current_len { + let deferred_from_prev = carried.is_some(); let abs_pos = current_abs_start + pos; let lit_len = pos - literals_start; @@ } else { match hash { Some((row, tag)) => $m.insert_at::<$rl>(abs_pos, row, tag), None => $m.insert_position::<$rl>(abs_pos), } - if carried.is_some() { + if carried.is_some() || deferred_from_prev { pos += 1; } else { const SKIP_STRENGTH: u32 = 10; pos += ((lit_len as u32) >> SKIP_STRENGTH) as usize + 1; }🤖 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/row/mod.rs` around lines 549 - 580, The deferred result handling in the row parser is dropping the outer “defer one more step” state when `lazy_decide!` returns `Some(None)`, so the next iteration treats it like a plain miss. Update the loop around `carried`/`carried.take()` in `mod.rs` so `Some(None)` preserves the defer-without-carry flag through the `abs_pos + 1` check and only falls back to the normal skip path after that miss has been processed. Use the existing `decision`, `carried`, and `best` flow to keep the pending deferred position alive until the winner at `abs_pos + 2` can still be reached.
1897-1910: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
usize::MAXhere to match the production row lazy path.The monolithic row parser above disables the sufficient-length early-out with
target_len = usize::MAX, but this helper passesself.target_len. For finite targets, this helper can commit where production would still probe lazily.Suggested fix
match crate::encoding::lazy_parse::lazy_decide!( best_len = best.match_len, best_off = best.offset, - target_len = self.target_len, + target_len = usize::MAX, lazy_depth = self.lazy_depth, abs_pos = abs_pos, lit_len = lit_len, history_end = self.history_abs_end(),🤖 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/row/mod.rs` around lines 1897 - 1910, The lazy decision path in `row::mod.rs` is using `self.target_len` instead of the production-equivalent unbounded target, which can change commit behavior for finite targets. Update the `lazy_decide!` call in the row parser helper to pass `usize::MAX` for `target_len`, matching the monolithic row lazy path and keeping the same lazy probing semantics.
🤖 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/src/encoding/hc/generator.rs`:
- Around line 1273-1277: The lazy carry state in generator.rs is being cleared
too early: when lazy_decide_carry() returns the NONE sentinel, the loop loses
the fact that the previous position was deferred and should keep that defer
state through the next miss. Update the carry-handling logic around carried and
the main search/skip loop so a NONE result preserves the “deferred without
carry” state instead of resetting to a normal fresh-search path, and ensure the
fallback skip heuristic does not bypass the already-proven winner at abs_pos +
2. Refer to lazy_decide_carry(), carried, and the NONE sentinel handling when
adjusting the control flow.
In `@zstd/src/encoding/row/mod.rs`:
- Around line 549-580: The deferred result handling in the row parser is
dropping the outer “defer one more step” state when `lazy_decide!` returns
`Some(None)`, so the next iteration treats it like a plain miss. Update the loop
around `carried`/`carried.take()` in `mod.rs` so `Some(None)` preserves the
defer-without-carry flag through the `abs_pos + 1` check and only falls back to
the normal skip path after that miss has been processed. Use the existing
`decision`, `carried`, and `best` flow to keep the pending deferred position
alive until the winner at `abs_pos + 2` can still be reached.
- Around line 1897-1910: The lazy decision path in `row::mod.rs` is using
`self.target_len` instead of the production-equivalent unbounded target, which
can change commit behavior for finite targets. Update the `lazy_decide!` call in
the row parser helper to pass `usize::MAX` for `target_len`, matching the
monolithic row lazy path and keeping the same lazy probing semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 767b6c53-e94e-4e80-a9c3-635862671617
📒 Files selected for processing (7)
zstd/src/encoding/frame_compressor.rszstd/src/encoding/hc/generator.rszstd/src/encoding/hc/mod.rszstd/src/encoding/lazy_parse.rszstd/src/encoding/lazy_parse/tests.rszstd/src/encoding/mod.rszstd/src/encoding/row/mod.rs
When the lazy lookahead defers because abs_pos+2 won but abs_pos+1 was cold (the Some(None) / defer-without-carry case), the deferred position carries no match. If abs_pos+1 then misses, the no-match skip heuristic (step grows with the literal-run length) could hop past the already-proven abs_pos+2 winner. Track that state in both lazy parsers and pin the next advance to one byte on the following miss so abs_pos+2 stays reachable, matching upstream's lazy depth loop (it steps one position at a time, never skipping a proven match): - HcMatcher generator: deferred_without_carry -> forced_single_step. - Row lazy_parse_body: deferred_from_prev keeps the one-byte advance. Also align the test-only Row pick_lazy_match_rl with the production row body: target_len = usize::MAX (lazy has no sufficient-length early-out).
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
Aligns the literal HUF table-log decision with upstream zstd's strategy gating
and consolidates the lazy parser onto a single shared decision, closing a small
ratio regression and removing several duplicated copies of the same logic.
Builds on the HUF optimal-depth fast-path from #167.
HUF literal table-log: gate on the size-adaptive strategy
Upstream gates the optimal-depth HUF table-log search to
btultra+(
HUF_OPTIMAL_DEPTH_THRESHOLD = ZSTD_btultra). We mirror that, but theliteral-compression / HUF-search gate read
strategy_tagfrom the barecompression level while the match-finder resolves strategy size-adaptively
(
ZSTD_getCParams: a<=16 KiBframe promotes levels 13-17 tobtultra/btultra2). On small frames the gate therefore thought btlazy2/btopt and
skipped the table-log search the matcher's btultra frame actually runs,
overshooting the C reference by 4 bytes on the
small-4k-log-linesliteralsection (L13-17).
Resolving
strategy_tagthrough the sameresolve_level_params->get_cparamspath the matcher uses makes the gate and the parse agree.small-4k-log-linesL13-17 go 150 -> 146 bytes, matching the C reference onboth x86_64 (BMI2/AVX2) and aarch64; the full local + i9 ratio matrix shows no
new cases where our output exceeds the C reference. Compress on the affected
small bands is ~2% slower — exactly the table-log search the C reference also
runs at those levels.
One lazy decision, length heuristic
Upstream has a single
ZSTD_compressBlock_lazy_genericparameterized by searchmethod + depth; the commit/defer decision is identical across strategies. We had
diverging copies (the hash-chain matcher ranked the lookahead by a gain
weighting, the row matcher by length), which is how the strategies drifted
apart. Extracted one
lazy_decide!macro (spliced inline so each matcher's#[target_feature]finder stays inlined) and reconciled it to the lengthheuristic: it is faster than the gain weighting and still beats the C reference
on ratio (drop-in, not byte-for-byte parity). HC and Row both use it; the now
test-only Row probe routes through it too, so the tests exercise the shipping
decision.
Deduplication (byte-identical)
extend_backwards_shared(raw-pointer, no per-stepbounds checks) across HC (fn + the hash-chain hot loop), Row, and Dfast — was
four copies.
offset) like the chain walk,
lazy_decide!, and the shared Row/Dfast probe,matching the C reference's depth-0 comparison. Drops the now-dead gain helper.
pick_lazy_match_shared+LazyMatchConfig.Carry the lazy lookahead instead of re-searching
The hash-chain lazy loop searched the lookahead position, threw it away, then
re-searched it next iteration (double work on every defer). It now carries the
lookahead match forward like the row parser and upstream's depth loop (each
position searched once), inserting the current position before the lookahead so
the
+1probe sees it at offset 1 — matching the C reference. Byte-identicaloutput; ~10-11% faster compress on the small hash-chain-lazy band
(
small-10k-randomL6/L8, the per-label-dictionary target), neutral withinnoise on large frames. The decision returns in registers (16-byte value) so the
register-tight depth-2 monomorph doesn't pay a stack-returned
Option.Testing
cargo nextest run -p structured-zstd --features hash,std,dict_builder— 833 passcargo nextest run -p ffi-bench --features bench_internals,dict_builder— 59 passcargo clippy(lib + ffi,--tests -D warnings) clean;cargo fmt --checkcleansmall_hinted_frames_match_c_literal_section_levels_13_to_17(our one-shot frame is never larger than the C reference at L13-17 on the
4 KiB log fixture)
flat
c_fficontrol arm; aarch64 ratio cross-checkSummary by CodeRabbit