From 82e85fcf3b319ce3f83eaecff8b4f375b46d1699 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Fri, 15 May 2026 01:49:57 +0300 Subject: [PATCH 01/12] =?UTF-8?q?perf(encoding):=20#124=20Phase=204.1=20?= =?UTF-8?q?=E2=80=94=20HC=20speculative=20tail=20check?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Port donor `zstd_lazy.c:714` (`ZSTD_HcFindBestMatch`) speculative 4-byte tail compare into `HcMatcher::hash_chain_candidate`. Once a `best` candidate is set with `match_len >= 4`, gate the per-byte `common_prefix_len` walk on `concat[i+best_ml-3..best_ml+1]` vs `concat[c+best_ml-3..best_ml+1]`. A mismatch proves the candidate cannot reach `best_ml + 1` and the walk would only confirm `len <= best_ml`. Correctness rests on the monotonic-offset property of LIFO chain walks: each subsequent candidate has `new.offset_bits >= best.offset_bits`, so `better_candidate` (gain = `len*4 - offset_bits`) can return a strict winner only when `new.match_len > best_ml`. Equal-length or shorter candidates can only tie or lose, and `better_candidate` keeps the existing best on ties. Bounds: when `current_idx + best_ml + 1 > history_tail`, the current position cannot reach `best_ml + 1` either, so the candidate cannot win — same skip. Verification: - `cargo nextest run -p structured-zstd --features hash,std,dict_builder` 476/476 pass (incl. `level22_sequences_match_donor_on_corpus_proxy` and the cross-validation roundtrip + parity gates). - `cargo test --doc` 10/10 pass. - `cargo clippy -p structured-zstd --features hash,std,dict_builder --all-targets -- -D warnings` clean. - `cargo bench --quick '^compress/better'` (Lazy backend, the actual HC chain-walker user) on x86_64-darwin: one scenario −5.4 % (small-10k-random, p = 0.05), the rest within ±2 % of stored baseline — `--quick` noise floor swallows whatever steady-state win remains. Full baseline-vs-feature bench will be measured on the CI bench-matrix. --- zstd/src/encoding/hc/mod.rs | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/zstd/src/encoding/hc/mod.rs b/zstd/src/encoding/hc/mod.rs index 3a9e6531f..2cb99fc79 100644 --- a/zstd/src/encoding/hc/mod.rs +++ b/zstd/src/encoding/hc/mod.rs @@ -228,11 +228,41 @@ impl HcMatcher { } let mut best: Option = None; + // Donor speculative tail check (`zstd_lazy.c:714`, + // `ZSTD_HcFindBestMatch`): once `best` is set, gate the + // expensive `common_prefix_len` walk on a 4-byte tail compare + // proving the candidate can reach `best_ml + 1`. See the + // monotonic-offset argument inside the loop for correctness. + let history_tail = concat.len(); for candidate_abs in self.chain_candidates(table, abs_pos) { if candidate_abs == usize::MAX { break; } let candidate_idx = candidate_abs - table.history_abs_start; + if let Some(best_ml) = best.map(|b| b.match_len) + && best_ml >= 4 + { + let tail_off = best_ml - 3; + let m_end = candidate_idx + tail_off + 4; + let i_end = current_idx + tail_off + 4; + // Chain walk is LIFO (newest first → strictly + // increasing offset). Once `best` is set, every + // subsequent candidate has `new.offset_bits >= + // best.offset_bits`, so `better_candidate` (gain = + // `len*4 - offset_bits`) can only return a new + // winner when `new.match_len > best_ml`. The 4-byte + // tail compare proves that pre-condition without + // running `common_prefix_len`. If either side runs + // out of room before `best_ml + 1`, the candidate + // cannot exceed `best_ml` either — same skip. + if i_end > history_tail || m_end > history_tail { + continue; + } + if concat[candidate_idx + tail_off..m_end] != concat[current_idx + tail_off..i_end] + { + continue; + } + } let match_len = common_prefix_len(&concat[candidate_idx..], &concat[current_idx..]); if match_len >= HC_MIN_MATCH_LEN { let candidate = From 8809bc01583492191d4036b20cbf0873d3f75711 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Fri, 15 May 2026 02:15:26 +0300 Subject: [PATCH 02/12] =?UTF-8?q?refactor(encoding):=20#124=20Phase=204.3?= =?UTF-8?q?=20=E2=80=94=20MatcherStorage=20enum=20dispatch?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Collapse the driver's four parallel backend fields (`match_generator: MatchGenerator`, `dfast_match_generator: Option`, `row_match_generator: Option`, `hc_match_generator: Option`) plus the `active_backend: BackendTag` discriminator into a single `storage: MatcherStorage` enum with four variants (`Simple` / `Dfast` / `Row` / `HashChain`). The active backend is now derived directly from the storage variant via `MatcherStorage::backend()` — no separate runtime tag that could drift against the variant. Why - The prior layout kept drained-but-allocated inner structures for every backend the driver had ever touched (the lazy `Option`s + the always-present `match_generator` retained their backing metadata across backend switches). With the enum, the inactive variants are dropped on switch, so the driver carries exactly one backend's state at any time. - Every per-frame / per-slice driver operation (`reset` / `prime_with_dictionary` / `commit_space` / `get_last_space` / `skip_matching_with_hint` / `retire_dictionary_budget` / `trim_after_budget_retire` / `skip_matching_for_dictionary_priming`) used to dispatch on `active_backend` to pick the matching field. The match-on-tag + field-access pair is replaced with `match &mut self.storage`, which fuses dispatch and accessor and gives the borrow checker field-disjoint splits "for free" (manual destructure where the closure captures `vec_pool` / `suffix_pool` alongside the backend borrow). - This is non-hot-path dispatch (per block at most, per frame typically). The hot per-block `compress_block::` already dispatches via `match S::BACKEND` over a `const`, which the compiler const-folds out per monomorphisation — that path is unchanged. Notes - Backend transitions inside `Matcher::reset` drain the old variant's allocations into `vec_pool` / `suffix_pool`, then swap `self.storage` to a freshly constructed variant for the new backend. The old variant is dropped at the swap; with the HashChain → Simple transition the pre-swap `hash_table` / `chain_table` / `hash3_table` zeroing kept from the prior field-shaped impl ensures the BtUltra2 hash3 allocation (`1 << HC3_HASH_LOG`) does not survive the switch. - Three tests went obsolete because they exercised the old lazy-init recovery / drained-but-allocated invariant (`driver_reset_from_row_backend_tolerates_missing_row_matcher` flipped `row_match_generator = None` by hand; `driver_best_to_fastest_releases_oversized_hc_tables` and `…reclaims_row_buffer_pool` inspected the post-switch state of a backend that no longer exists). Replaced with one new test proving the new invariant — buffer pool grows across the HashChain → Simple switch, backend tag flips, no field-shaped inspection of the dropped variant. - Immutable accessors (`simple` / `dfast_matcher` / `row_matcher` / `hc_matcher`) are now `#[cfg(test)]` — production code only reads via the mutable counterparts or pattern-matches `storage` directly. Verification - `cargo nextest run -p structured-zstd --features hash,std,dict_builder` 475/475 pass, 3 skipped (`level22_sequences_match_donor_on_corpus_proxy` + full cross-validation + roundtrip + parity gates). - `cargo test --doc` 10/10 pass. - `cargo clippy -p structured-zstd --features hash,std,dict_builder --all-targets -- -D warnings` clean. --- zstd/src/encoding/match_generator.rs | 526 ++++++++++++++------------- 1 file changed, 281 insertions(+), 245 deletions(-) diff --git a/zstd/src/encoding/match_generator.rs b/zstd/src/encoding/match_generator.rs index 0ad4c29b7..ba6f20553 100644 --- a/zstd/src/encoding/match_generator.rs +++ b/zstd/src/encoding/match_generator.rs @@ -401,25 +401,67 @@ fn resolve_level_params(level: CompressionLevel, source_size: Option) -> Le } } +/// Backend storage for [`MatchGeneratorDriver`]. Exactly one match-finder +/// state lives in the driver at a time — the active variant. Backend +/// transitions in [`Matcher::reset`] drain the current variant's allocations +/// into the shared `vec_pool` and then replace `storage` with a freshly +/// constructed variant for the new backend. +/// +/// Replaces the prior pattern of four parallel fields (`match_generator`, +/// `dfast_match_generator: Option<…>`, `row_match_generator: Option<…>`, +/// `hc_match_generator: Option<…>`) + an `active_backend: BackendTag` +/// discriminator: the parallel layout kept drained inner structures +/// allocated across backend switches, and every per-frame/per-slice +/// driver operation had to dispatch on `active_backend` to pick the +/// right field. A single enum collapses the storage and makes the +/// dispatcher pattern-match on the storage variant directly — same +/// number of arms, but `storage.backend()` is now the canonical source +/// of truth and dead variants are dropped when the active backend +/// changes. +enum MatcherStorage { + /// Level 1 — donor `ZSTD_fast`. Single-table Simple matcher; always + /// the initial variant constructed by [`MatchGeneratorDriver::new`]. + Simple(MatchGenerator), + /// Levels 2-3 — donor `ZSTD_dfast`. Two-table hash chain. + Dfast(DfastMatchGenerator), + /// Level 4 — donor `ZSTD_greedy` with row hashing. + Row(RowMatchGenerator), + /// Levels 5-22 — donor `ZSTD_lazy2` and the BT-based optimal modes + /// (`btopt` / `btultra` / `btultra2`). Holds an [`HcMatchGenerator`] + /// whose internal [`HcBackend`] discriminator decides whether BT + /// scratch is allocated. + HashChain(HcMatchGenerator), +} + +impl MatcherStorage { + /// [`super::strategy::BackendTag`] family of the active variant. + fn backend(&self) -> super::strategy::BackendTag { + use super::strategy::BackendTag; + match self { + Self::Simple(_) => BackendTag::Simple, + Self::Dfast(_) => BackendTag::Dfast, + Self::Row(_) => BackendTag::Row, + Self::HashChain(_) => BackendTag::HashChain, + } + } +} + /// This is the default implementation of the `Matcher` trait. It allocates and reuses the buffers when possible. pub struct MatchGeneratorDriver { vec_pool: Vec>, suffix_pool: Vec, - match_generator: MatchGenerator, - dfast_match_generator: Option, - row_match_generator: Option, - hc_match_generator: Option, - active_backend: super::strategy::BackendTag, + /// Active match-finder state. Exactly one backend lives here at a + /// time; [`Matcher::reset`] drains the previous variant into + /// `vec_pool` before swapping in a freshly constructed variant for + /// the new backend. `storage.backend()` is the canonical source of + /// truth for the parse family; `strategy_tag` carries the + /// compile-time strategy chosen at the last `reset()`. + storage: MatcherStorage, // Compile-time strategy tag resolved at `reset()` from the // requested `CompressionLevel`'s `LevelParams`. The driver's // hot-block dispatcher in `blocks/compressed.rs` matches on // this tag to enter the corresponding `Strategy` - // monomorphisation (`compress_block::`). `active_backend` - // is the parse-family derivation of the tag and is kept here - // only because matcher state ownership (`row_match_generator` - // vs `hc_match_generator`) is reused across levels within the - // same backend; `strategy_tag.backend()` is the canonical - // source of truth. + // monomorphisation (`compress_block::`). strategy_tag: super::strategy::StrategyTag, slice_size: usize, base_slice_size: usize, @@ -443,11 +485,7 @@ impl MatchGeneratorDriver { Self { vec_pool: Vec::new(), suffix_pool: Vec::new(), - match_generator: MatchGenerator::new(max_window_size), - dfast_match_generator: None, - row_match_generator: None, - hc_match_generator: None, - active_backend: super::strategy::BackendTag::Simple, + storage: MatcherStorage::Simple(MatchGenerator::new(max_window_size)), strategy_tag: super::strategy::StrategyTag::Fast, slice_size, base_slice_size: slice_size, @@ -461,40 +499,70 @@ impl MatchGeneratorDriver { resolve_level_params(level, source_size) } + /// Active backend family derived from the storage variant. Single + /// source of truth — no separate runtime tag to drift against. + fn active_backend(&self) -> super::strategy::BackendTag { + self.storage.backend() + } + + #[cfg(test)] + fn simple(&self) -> &MatchGenerator { + match &self.storage { + MatcherStorage::Simple(m) => m, + _ => panic!("simple backend must be initialized by reset() before use"), + } + } + + fn simple_mut(&mut self) -> &mut MatchGenerator { + match &mut self.storage { + MatcherStorage::Simple(m) => m, + _ => panic!("simple backend must be initialized by reset() before use"), + } + } + + #[cfg(test)] fn dfast_matcher(&self) -> &DfastMatchGenerator { - self.dfast_match_generator - .as_ref() - .expect("dfast backend must be initialized by reset() before use") + match &self.storage { + MatcherStorage::Dfast(m) => m, + _ => panic!("dfast backend must be initialized by reset() before use"), + } } fn dfast_matcher_mut(&mut self) -> &mut DfastMatchGenerator { - self.dfast_match_generator - .as_mut() - .expect("dfast backend must be initialized by reset() before use") + match &mut self.storage { + MatcherStorage::Dfast(m) => m, + _ => panic!("dfast backend must be initialized by reset() before use"), + } } + #[cfg(test)] fn row_matcher(&self) -> &RowMatchGenerator { - self.row_match_generator - .as_ref() - .expect("row backend must be initialized by reset() before use") + match &self.storage { + MatcherStorage::Row(m) => m, + _ => panic!("row backend must be initialized by reset() before use"), + } } fn row_matcher_mut(&mut self) -> &mut RowMatchGenerator { - self.row_match_generator - .as_mut() - .expect("row backend must be initialized by reset() before use") + match &mut self.storage { + MatcherStorage::Row(m) => m, + _ => panic!("row backend must be initialized by reset() before use"), + } } + #[cfg(test)] fn hc_matcher(&self) -> &HcMatchGenerator { - self.hc_match_generator - .as_ref() - .expect("hash chain backend must be initialized by reset() before use") + match &self.storage { + MatcherStorage::HashChain(m) => m, + _ => panic!("hash chain backend must be initialized by reset() before use"), + } } fn hc_matcher_mut(&mut self) -> &mut HcMatchGenerator { - self.hc_match_generator - .as_mut() - .expect("hash chain backend must be initialized by reset() before use") + match &mut self.storage { + MatcherStorage::HashChain(m) => m, + _ => panic!("hash chain backend must be initialized by reset() before use"), + } } fn retire_dictionary_budget(&mut self, evicted_bytes: usize) { @@ -503,12 +571,10 @@ impl MatchGeneratorDriver { return; } self.dictionary_retained_budget -= reclaimed; - match self.active_backend { + match self.active_backend() { super::strategy::BackendTag::Simple => { - self.match_generator.max_window_size = self - .match_generator - .max_window_size - .saturating_sub(reclaimed); + let matcher = self.simple_mut(); + matcher.max_window_size = matcher.max_window_size.saturating_sub(reclaimed); } super::strategy::BackendTag::Dfast => { let matcher = self.dfast_matcher_mut(); @@ -529,11 +595,14 @@ impl MatchGeneratorDriver { fn trim_after_budget_retire(&mut self) { loop { let mut evicted_bytes = 0usize; - match self.active_backend { + match self.active_backend() { super::strategy::BackendTag::Simple => { let vec_pool = &mut self.vec_pool; let suffix_pool = &mut self.suffix_pool; - self.match_generator.reserve(0, |mut data, mut suffixes| { + let MatcherStorage::Simple(m) = &mut self.storage else { + unreachable!("active_backend() == Simple proven above"); + }; + m.reserve(0, |mut data, mut suffixes| { evicted_bytes += data.len(); data.resize(data.capacity(), 0); vec_pool.push(data); @@ -584,9 +653,9 @@ impl MatchGeneratorDriver { } fn skip_matching_for_dictionary_priming(&mut self) { - match self.active_backend { + match self.active_backend() { super::strategy::BackendTag::Simple => { - self.match_generator.skip_matching_with_hint(Some(false)) + self.simple_mut().skip_matching_with_hint(Some(false)) } super::strategy::BackendTag::Dfast => self.dfast_matcher_mut().skip_matching_dense(), super::strategy::BackendTag::Row => { @@ -615,12 +684,17 @@ impl Matcher for MatchGeneratorDriver { let next_backend = params.backend(); let max_window_size = 1usize << params.window_log; self.dictionary_retained_budget = 0; - if self.active_backend != next_backend { - match self.active_backend { - super::strategy::BackendTag::Simple => { + if self.active_backend() != next_backend { + // Drain the outgoing backend's allocations into the shared + // pool. The `match &mut self.storage { ... }` block runs to + // completion before the assignment below replaces the + // variant, so the inner state we just drained is dropped + // with the old variant. + match &mut self.storage { + MatcherStorage::Simple(m) => { let vec_pool = &mut self.vec_pool; let suffix_pool = &mut self.suffix_pool; - self.match_generator.reset(|mut data, mut suffixes| { + m.reset(|mut data, mut suffixes| { data.resize(data.capacity(), 0); vec_pool.push(data); suffixes.slots.clear(); @@ -628,65 +702,75 @@ impl Matcher for MatchGeneratorDriver { suffix_pool.push(suffixes); }); } + MatcherStorage::Dfast(m) => { + let vec_pool = &mut self.vec_pool; + m.reset(|mut data| { + data.resize(data.capacity(), 0); + vec_pool.push(data); + }); + } + MatcherStorage::Row(m) => { + m.row_heads = Vec::new(); + m.row_positions = Vec::new(); + m.row_tags = Vec::new(); + let vec_pool = &mut self.vec_pool; + m.reset(|mut data| { + data.resize(data.capacity(), 0); + vec_pool.push(data); + }); + } + MatcherStorage::HashChain(m) => { + // Release oversized tables when switching away from + // HashChain so Best's larger allocations don't persist. + // hash3_table must be released alongside the other + // two: BtUltra2's `1 << HC3_HASH_LOG` entries would + // otherwise stay pinned across the backend switch, + // even though no future caller of this backend will + // touch them. + m.table.hash_table = Vec::new(); + m.table.chain_table = Vec::new(); + m.table.hash3_table = Vec::new(); + let vec_pool = &mut self.vec_pool; + m.reset(|mut data| { + data.resize(data.capacity(), 0); + vec_pool.push(data); + }); + } + } + // Swap in a fresh variant for the new backend. The previous + // `storage` is dropped here. + self.storage = match next_backend { + super::strategy::BackendTag::Simple => { + MatcherStorage::Simple(MatchGenerator::new(max_window_size)) + } super::strategy::BackendTag::Dfast => { - if let Some(dfast) = self.dfast_match_generator.as_mut() { - let vec_pool = &mut self.vec_pool; - dfast.reset(|mut data| { - data.resize(data.capacity(), 0); - vec_pool.push(data); - }); - } + MatcherStorage::Dfast(DfastMatchGenerator::new(max_window_size)) } super::strategy::BackendTag::Row => { - if let Some(row) = self.row_match_generator.as_mut() { - row.row_heads = Vec::new(); - row.row_positions = Vec::new(); - row.row_tags = Vec::new(); - let vec_pool = &mut self.vec_pool; - row.reset(|mut data| { - data.resize(data.capacity(), 0); - vec_pool.push(data); - }); - } + MatcherStorage::Row(RowMatchGenerator::new(max_window_size)) } super::strategy::BackendTag::HashChain => { - if let Some(hc) = self.hc_match_generator.as_mut() { - // Release oversized tables when switching away from - // HashChain so Best's larger allocations don't persist. - // hash3_table must be released alongside the other - // two: BtUltra2's `1 << HC3_HASH_LOG` entries would - // otherwise stay pinned across the backend switch, - // even though no future caller of this backend will - // touch them. - hc.table.hash_table = Vec::new(); - hc.table.chain_table = Vec::new(); - hc.table.hash3_table = Vec::new(); - let vec_pool = &mut self.vec_pool; - hc.reset(|mut data| { - data.resize(data.capacity(), 0); - vec_pool.push(data); - }); - } + MatcherStorage::HashChain(HcMatchGenerator::new(max_window_size)) } - } + }; } // Single source of truth: `LevelParams::strategy_tag` is the // authoritative mapping from `CompressionLevel` to strategy. - // Both runtime fields are derived from it — no separate - // `for_compression_level` recomputation that could drift - // against `LEVEL_TABLE`. + // `storage.backend()` derives the parse family from the variant, + // so there is no separate runtime tag that could drift against + // `LEVEL_TABLE`. self.strategy_tag = params.strategy_tag; - self.active_backend = next_backend; self.slice_size = self.base_slice_size.min(max_window_size); self.reported_window_size = max_window_size; - match self.active_backend { - super::strategy::BackendTag::Simple => { + let strategy_tag = self.strategy_tag; + match &mut self.storage { + MatcherStorage::Simple(m) => { let vec_pool = &mut self.vec_pool; let suffix_pool = &mut self.suffix_pool; - self.match_generator.max_window_size = max_window_size; - self.match_generator.hash_fill_step = params.hash_fill_step; - self.match_generator.reset(|mut data, mut suffixes| { + m.max_window_size = max_window_size; + m.hash_fill_step = params.hash_fill_step; + m.reset(|mut data, mut suffixes| { data.resize(data.capacity(), 0); vec_pool.push(data); suffixes.slots.clear(); @@ -694,10 +778,7 @@ impl Matcher for MatchGeneratorDriver { suffix_pool.push(suffixes); }); } - super::strategy::BackendTag::Dfast => { - let dfast = self - .dfast_match_generator - .get_or_insert_with(|| DfastMatchGenerator::new(max_window_size)); + MatcherStorage::Dfast(dfast) => { dfast.max_window_size = max_window_size; dfast.lazy_depth = params.lazy_depth; dfast.use_fast_loop = matches!( @@ -717,10 +798,7 @@ impl Matcher for MatchGeneratorDriver { vec_pool.push(data); }); } - super::strategy::BackendTag::Row => { - let row = self - .row_match_generator - .get_or_insert_with(|| RowMatchGenerator::new(max_window_size)); + MatcherStorage::Row(row) => { row.max_window_size = max_window_size; row.lazy_depth = params.lazy_depth; row.configure(params.row); @@ -733,13 +811,10 @@ impl Matcher for MatchGeneratorDriver { vec_pool.push(data); }); } - super::strategy::BackendTag::HashChain => { - let hc = self - .hc_match_generator - .get_or_insert_with(|| HcMatchGenerator::new(max_window_size)); + MatcherStorage::HashChain(hc) => { hc.table.max_window_size = max_window_size; hc.hc.lazy_depth = params.lazy_depth; - hc.configure(params.hc, self.strategy_tag, params.window_log); + hc.configure(params.hc, strategy_tag, params.window_log); let vec_pool = &mut self.vec_pool; hc.reset(|mut data| { data.resize(data.capacity(), 0); @@ -750,8 +825,8 @@ impl Matcher for MatchGeneratorDriver { } fn prime_with_dictionary(&mut self, dict_content: &[u8], offset_hist: [u32; 3]) { - match self.active_backend { - super::strategy::BackendTag::Simple => self.match_generator.offset_hist = offset_hist, + match self.active_backend() { + super::strategy::BackendTag::Simple => self.simple_mut().offset_hist = offset_hist, super::strategy::BackendTag::Dfast => { self.dfast_matcher_mut().offset_hist = offset_hist } @@ -770,12 +845,11 @@ impl Matcher for MatchGeneratorDriver { // Dictionary bytes should stay addressable until produced frame output // itself exceeds the live window size. let retained_dict_budget = dict_content.len(); - match self.active_backend { + match self.active_backend() { super::strategy::BackendTag::Simple => { - self.match_generator.max_window_size = self - .match_generator - .max_window_size - .saturating_add(retained_dict_budget); + let matcher = self.simple_mut(); + matcher.max_window_size = + matcher.max_window_size.saturating_add(retained_dict_budget); } super::strategy::BackendTag::Dfast => { let matcher = self.dfast_matcher_mut(); @@ -801,7 +875,7 @@ impl Matcher for MatchGeneratorDriver { // insert_position needs 4 bytes of lookahead for hashing; // backfill_boundary_positions re-visits tail positions once the // next slice extends history, but cannot hash <4 byte fragments. - let min_primed_tail = match self.active_backend { + let min_primed_tail = match self.active_backend() { super::strategy::BackendTag::Simple => MIN_MATCH_LEN, super::strategy::BackendTag::Dfast | super::strategy::BackendTag::Row @@ -823,10 +897,10 @@ impl Matcher for MatchGeneratorDriver { let uncommitted_tail_budget = retained_dict_budget.saturating_sub(committed_dict_budget); if uncommitted_tail_budget > 0 { - match self.active_backend { + match self.active_backend() { super::strategy::BackendTag::Simple => { - self.match_generator.max_window_size = self - .match_generator + let matcher = self.simple_mut(); + matcher.max_window_size = matcher .max_window_size .saturating_sub(uncommitted_tail_budget); } @@ -856,7 +930,7 @@ impl Matcher for MatchGeneratorDriver { .dictionary_retained_budget .saturating_add(committed_dict_budget); } - if self.active_backend == super::strategy::BackendTag::HashChain { + if self.active_backend() == super::strategy::BackendTag::HashChain { self.hc_matcher_mut() .table .set_dictionary_limit_from_primed_bytes(committed_dict_budget); @@ -870,7 +944,7 @@ impl Matcher for MatchGeneratorDriver { ml: Option<&crate::fse::fse_encoder::FSETable>, of: Option<&crate::fse::fse_encoder::FSETable>, ) { - if self.active_backend == super::strategy::BackendTag::HashChain { + if self.active_backend() == super::strategy::BackendTag::HashChain { self.hc_matcher_mut() .seed_dictionary_entropy(huff, ll, ml, of); } @@ -894,82 +968,60 @@ impl Matcher for MatchGeneratorDriver { } fn get_last_space(&mut self) -> &[u8] { - match self.active_backend { - super::strategy::BackendTag::Simple => { - self.match_generator.window.last().unwrap().data.as_slice() - } - super::strategy::BackendTag::Dfast => self.dfast_matcher().get_last_space(), - super::strategy::BackendTag::Row => self.row_matcher().get_last_space(), - super::strategy::BackendTag::HashChain => self.hc_matcher().table.get_last_space(), + match &self.storage { + MatcherStorage::Simple(m) => m.window.last().unwrap().data.as_slice(), + MatcherStorage::Dfast(m) => m.get_last_space(), + MatcherStorage::Row(m) => m.get_last_space(), + MatcherStorage::HashChain(m) => m.table.get_last_space(), } } fn commit_space(&mut self, space: Vec) { - match self.active_backend { - super::strategy::BackendTag::Simple => { - let vec_pool = &mut self.vec_pool; - let mut evicted_bytes = 0usize; - let suffixes = match self.suffix_pool.pop() { + let mut evicted_bytes = 0usize; + // Split borrows manually so the `add_data` closures can write + // into `vec_pool`/`suffix_pool` while the backend itself holds + // an exclusive borrow via `storage`. + let vec_pool = &mut self.vec_pool; + let suffix_pool = &mut self.suffix_pool; + match &mut self.storage { + MatcherStorage::Simple(m) => { + let suffixes = match suffix_pool.pop() { Some(store) if store.slots.len() >= space.len() => store, _ => SuffixStore::with_capacity(space.len()), }; - let suffix_pool = &mut self.suffix_pool; - self.match_generator - .add_data(space, suffixes, |mut data, mut suffixes| { - evicted_bytes += data.len(); - data.resize(data.capacity(), 0); - vec_pool.push(data); - suffixes.slots.clear(); - suffixes.slots.resize(suffixes.slots.capacity(), None); - suffix_pool.push(suffixes); - }); - self.retire_dictionary_budget(evicted_bytes); - self.trim_after_budget_retire(); + m.add_data(space, suffixes, |mut data, mut suffixes| { + evicted_bytes += data.len(); + data.resize(data.capacity(), 0); + vec_pool.push(data); + suffixes.slots.clear(); + suffixes.slots.resize(suffixes.slots.capacity(), None); + suffix_pool.push(suffixes); + }); } - super::strategy::BackendTag::Dfast => { - let vec_pool = &mut self.vec_pool; - let mut evicted_bytes = 0usize; - self.dfast_match_generator - .as_mut() - .expect("dfast backend must be initialized by reset() before use") - .add_data(space, |mut data| { - evicted_bytes += data.len(); - data.resize(data.capacity(), 0); - vec_pool.push(data); - }); - self.retire_dictionary_budget(evicted_bytes); - self.trim_after_budget_retire(); + MatcherStorage::Dfast(m) => { + m.add_data(space, |mut data| { + evicted_bytes += data.len(); + data.resize(data.capacity(), 0); + vec_pool.push(data); + }); } - super::strategy::BackendTag::Row => { - let vec_pool = &mut self.vec_pool; - let mut evicted_bytes = 0usize; - self.row_match_generator - .as_mut() - .expect("row backend must be initialized by reset() before use") - .add_data(space, |mut data| { - evicted_bytes += data.len(); - data.resize(data.capacity(), 0); - vec_pool.push(data); - }); - self.retire_dictionary_budget(evicted_bytes); - self.trim_after_budget_retire(); + MatcherStorage::Row(m) => { + m.add_data(space, |mut data| { + evicted_bytes += data.len(); + data.resize(data.capacity(), 0); + vec_pool.push(data); + }); } - super::strategy::BackendTag::HashChain => { - let vec_pool = &mut self.vec_pool; - let mut evicted_bytes = 0usize; - self.hc_match_generator - .as_mut() - .expect("hash chain backend must be initialized by reset() before use") - .table - .add_data(space, |mut data| { - evicted_bytes += data.len(); - data.resize(data.capacity(), 0); - vec_pool.push(data); - }); - self.retire_dictionary_budget(evicted_bytes); - self.trim_after_budget_retire(); + MatcherStorage::HashChain(m) => { + m.table.add_data(space, |mut data| { + evicted_bytes += data.len(); + data.resize(data.capacity(), 0); + vec_pool.push(data); + }); } } + self.retire_dictionary_budget(evicted_bytes); + self.trim_after_budget_retire(); } fn start_matching(&mut self, mut handle_sequence: impl for<'a> FnMut(Sequence<'a>)) { @@ -1000,9 +1052,9 @@ impl Matcher for MatchGeneratorDriver { } fn skip_matching_with_hint(&mut self, incompressible_hint: Option) { - match self.active_backend { + match self.active_backend() { super::strategy::BackendTag::Simple => self - .match_generator + .simple_mut() .skip_matching_with_hint(incompressible_hint), super::strategy::BackendTag::Dfast => { self.dfast_matcher_mut().skip_matching(incompressible_hint) @@ -1032,9 +1084,7 @@ impl MatchGeneratorDriver { ) { use super::strategy::BackendTag; match S::BACKEND { - BackendTag::Simple => { - while self.match_generator.next_sequence(&mut *handle_sequence) {} - } + BackendTag::Simple => while self.simple_mut().next_sequence(&mut *handle_sequence) {}, BackendTag::Dfast => self.dfast_matcher_mut().start_matching(handle_sequence), BackendTag::Row => self.row_matcher_mut().start_matching(handle_sequence), BackendTag::HashChain => self @@ -3661,7 +3711,7 @@ fn driver_switches_backends_and_initializes_dfast_via_reset() { let mut driver = MatchGeneratorDriver::new(32, 2); driver.reset(CompressionLevel::Default); - assert_eq!(driver.active_backend, super::strategy::BackendTag::Dfast); + assert_eq!(driver.active_backend(), super::strategy::BackendTag::Dfast); assert_eq!(driver.window_size(), (1u64 << 22)); let mut first = driver.get_next_space(); @@ -3702,7 +3752,7 @@ fn driver_switches_backends_and_initializes_dfast_via_reset() { fn driver_level4_selects_row_backend() { let mut driver = MatchGeneratorDriver::new(32, 2); driver.reset(CompressionLevel::Level(4)); - assert_eq!(driver.active_backend, super::strategy::BackendTag::Row); + assert_eq!(driver.active_backend(), super::strategy::BackendTag::Row); } #[test] @@ -3718,7 +3768,7 @@ fn driver_reset_keeps_strategy_tag_in_sync_with_active_backend() { ); assert_eq!( driver.strategy_tag.backend(), - driver.active_backend, + driver.active_backend(), "strategy_tag backend disagrees with active_backend for {level:?}" ); } @@ -4920,7 +4970,7 @@ fn simple_backend_rejects_undersized_pooled_suffix_store() { driver.commit_space(space); let last_suffix_slots = driver - .match_generator + .simple() .window .last() .expect("window entry must exist after commit") @@ -4984,20 +5034,19 @@ fn driver_best_to_fastest_releases_oversized_hc_tables() { driver.commit_space(space); driver.skip_matching_with_hint(None); - // Switch to Fastest — must release HC tables. + // Switch to Fastest — the [`MatcherStorage`] enum swaps to the + // `Simple` variant and drops the `HashChain` variant entirely (the + // HC reset closure pre-drop pushes any retained data buffers into + // `vec_pool`, and the `hash_table` / `chain_table` / `hash3_table` + // are emptied by hand before drop so their backing allocations + // release immediately rather than waiting for the variant's `Drop` + // to run on the way out of the swap). Post-switch the HC variant + // no longer exists — there is nothing to inspect by accessor; the + // assertion that storage is now `Simple` covers the invariant the + // old hash_table/chain_table checks were proxying for. driver.reset(CompressionLevel::Fastest); assert_eq!(driver.window_size(), (1u64 << 17)); - - // HC matcher should have empty tables after backend switch. - let hc = driver.hc_match_generator.as_ref().unwrap(); - assert!( - hc.table.hash_table.is_empty(), - "HC hash_table should be released after switching away from Best" - ); - assert!( - hc.table.chain_table.is_empty(), - "HC chain_table should be released after switching away from Best" - ); + assert_eq!(driver.active_backend(), super::strategy::BackendTag::Simple); } #[test] @@ -5014,7 +5063,7 @@ fn driver_better_to_best_resizes_hc_tables() { driver.commit_space(space); driver.skip_matching_with_hint(None); - let hc = driver.hc_match_generator.as_ref().unwrap(); + let hc = driver.hc_matcher(); let better_hash_len = hc.table.hash_table.len(); let better_chain_len = hc.table.chain_table.len(); @@ -5029,7 +5078,7 @@ fn driver_better_to_best_resizes_hc_tables() { driver.commit_space(space); driver.skip_matching_with_hint(None); - let hc = driver.hc_match_generator.as_ref().unwrap(); + let hc = driver.hc_matcher(); assert!( hc.table.hash_table.len() > better_hash_len, "Best hash_table ({}) should be larger than Better ({})", @@ -5117,7 +5166,7 @@ fn prime_with_dictionary_applies_offset_history_even_when_content_is_empty() { driver.prime_with_dictionary(&[], [11, 7, 3]); - assert_eq!(driver.match_generator.offset_hist, [11, 7, 3]); + assert_eq!(driver.simple_mut().offset_hist, [11, 7, 3]); } #[test] @@ -5210,7 +5259,7 @@ fn prime_with_dictionary_does_not_reuse_tiny_suffix_store() { assert!( driver - .match_generator + .simple() .window .iter() .all(|entry| entry.data.len() >= MIN_MATCH_LEN), @@ -5223,12 +5272,12 @@ fn prime_with_dictionary_counts_only_committed_tail_budget() { let mut driver = MatchGeneratorDriver::new(8, 1); driver.reset(CompressionLevel::Fastest); - let before = driver.match_generator.max_window_size; + let before = driver.simple_mut().max_window_size; // One full slice plus a 1-byte tail that cannot be committed. driver.prime_with_dictionary(b"abcdefghi", [1, 4, 8]); assert_eq!( - driver.match_generator.max_window_size, + driver.simple_mut().max_window_size, before + 8, "retention budget must account only for dictionary bytes actually committed to history" ); @@ -5345,20 +5394,25 @@ fn row_get_last_space_and_reset_to_fastest_clears_window() { assert_eq!(driver.get_last_space(), b"row-data"); driver.reset(CompressionLevel::Fastest); - assert_eq!(driver.active_backend, super::strategy::BackendTag::Simple); - assert!(driver.row_matcher().window.is_empty()); -} - -/// Ensures switching from Row to Simple returns pooled buffers and row tables. + assert_eq!(driver.active_backend(), super::strategy::BackendTag::Simple); +} + +/// Switching from Row to Simple drains row-side buffers into `vec_pool` +/// before swapping the storage variant. With the [`MatcherStorage`] enum +/// the old [`RowMatchGenerator`] is dropped on swap, so this test +/// guards only the pool-recycling side of the transition. The +/// pre-enum tests (`…reclaims_row_buffer_pool` / +/// `…tolerates_missing_row_matcher`) checked that the row matcher +/// stayed allocated across the switch with its internals cleared — +/// the new invariant is "dead variants are dropped", and the +/// `row_match_generator: Option<_>` field whose lazy-init recovery +/// they exercised no longer exists. #[test] -fn driver_reset_from_row_backend_reclaims_row_buffer_pool() { +fn driver_reset_from_row_backend_recycles_row_buffers_into_pool() { let mut driver = MatchGeneratorDriver::new(8, 1); driver.reset(CompressionLevel::Level(4)); - assert_eq!(driver.active_backend, super::strategy::BackendTag::Row); + assert_eq!(driver.active_backend(), super::strategy::BackendTag::Row); - // Ensure the row matcher option is initialized so reset() executes - // the Row backend retirement path. - let _ = driver.row_matcher(); let mut space = driver.get_next_space(); space.extend_from_slice(b"row-data-to-recycle"); driver.commit_space(space); @@ -5366,32 +5420,13 @@ fn driver_reset_from_row_backend_reclaims_row_buffer_pool() { let before_pool = driver.vec_pool.len(); driver.reset(CompressionLevel::Fastest); - assert_eq!(driver.active_backend, super::strategy::BackendTag::Simple); - let row = driver - .row_match_generator - .as_ref() - .expect("row matcher should remain allocated after switch"); - assert!(row.row_heads.is_empty()); - assert!(row.row_positions.is_empty()); - assert!(row.row_tags.is_empty()); + assert_eq!(driver.active_backend(), super::strategy::BackendTag::Simple); assert!( driver.vec_pool.len() >= before_pool, "row reset should recycle row history buffers" ); } -/// Guards the optional row backend retirement path when no row matcher was allocated. -#[test] -fn driver_reset_from_row_backend_tolerates_missing_row_matcher() { - let mut driver = MatchGeneratorDriver::new(8, 1); - driver.active_backend = super::strategy::BackendTag::Row; - driver.row_match_generator = None; - - driver.reset(CompressionLevel::Fastest); - - assert_eq!(driver.active_backend, super::strategy::BackendTag::Simple); -} - #[test] fn adjust_params_for_zero_source_size_uses_min_hinted_window_floor() { let mut params = resolve_level_params(CompressionLevel::Level(4), None); @@ -6155,12 +6190,12 @@ fn prime_with_dictionary_budget_shrinks_after_simple_eviction() { driver.reset(CompressionLevel::Fastest); // Use a small live window so dictionary-primed slices are evicted // quickly and budget retirement can be asserted deterministically. - driver.match_generator.max_window_size = 8; + driver.simple_mut().max_window_size = 8; driver.reported_window_size = 8; - let base_window = driver.match_generator.max_window_size; + let base_window = driver.simple_mut().max_window_size; driver.prime_with_dictionary(b"abcdefghABCDEFGHijklmnop", [1, 4, 8]); - assert_eq!(driver.match_generator.max_window_size, base_window + 24); + assert_eq!(driver.simple_mut().max_window_size, base_window + 24); for block in [b"AAAAAAAA", b"BBBBBBBB"] { let mut space = driver.get_next_space(); @@ -6175,7 +6210,8 @@ fn prime_with_dictionary_budget_shrinks_after_simple_eviction() { "dictionary budget should be fully retired once primed dict slices are evicted" ); assert_eq!( - driver.match_generator.max_window_size, base_window, + driver.simple_mut().max_window_size, + base_window, "retired dictionary budget must not remain reusable for live history" ); } @@ -6368,16 +6404,16 @@ fn fastest_reset_uses_interleaved_hash_fill_step() { let mut driver = MatchGeneratorDriver::new(32, 2); driver.reset(CompressionLevel::Uncompressed); - assert_eq!(driver.match_generator.hash_fill_step, 1); + assert_eq!(driver.simple().hash_fill_step, 1); driver.reset(CompressionLevel::Fastest); - assert_eq!(driver.match_generator.hash_fill_step, FAST_HASH_FILL_STEP); + assert_eq!(driver.simple().hash_fill_step, FAST_HASH_FILL_STEP); // Better uses the HashChain backend with lazy2; verify that the backend switch // happened and the lazy_depth is configured correctly. driver.reset(CompressionLevel::Better); assert_eq!( - driver.active_backend, + driver.active_backend(), super::strategy::BackendTag::HashChain ); assert_eq!(driver.window_size(), (1u64 << 23)); From c16ca32b75f5ac28a2e69f24d8f2318afe576098 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Fri, 15 May 2026 03:07:45 +0300 Subject: [PATCH 03/12] =?UTF-8?q?perf(encoding):=20#124=20Phase=204=20?= =?UTF-8?q?=E2=80=94=20Dfast=20post-match=20rep-0=20+=20review=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bundles the Dfast donor port and the HC speculative-gate correctness fix from Copilot review on PR #125. Same Phase 4 hot-path family, shares one retest cycle. ## Dfast post-match rep-0 extension Port donor `zstd_double_fast.c` post-match rep chain into the Dfast fast-loop and general-loop emit paths. After each `emit_candidate`, the new helper `extend_with_repcode_after_match` chains additional zero-literal rep sequences while 4 bytes at the new cursor keep matching `cursor - offset_hist[1]`. Each iteration emits one rep sequence with the swapped-in offset, advances `pos` and `literals_start` past the rep match, updates `offset_hist` via `encode_offset_with_history` (the donor `offset_2 = offset_1; offset_1 = old_offset_2;` swap), inserts every position of the rep match into the long/short hash tables so subsequent positions can hit them, and skips the full hash-table probe entirely on every extra match. Uses donor's `MINMATCH = 4` for the rep threshold rather than the stricter `DFAST_MIN_MATCH_LEN = 6` enforced on the main search — donor accepts any 4-byte rep extension and we mirror that because the rep emission carries no offset cost. ## HC speculative tail check — correctness fix Copilot flagged a backward-extension-unaware bound in the gate introduced by 82e85fc. The old code used `best.match_len` directly as the forward-extent reference, but `extend_backwards` may have added up to `lit_len` backward bytes to that total. A new chain candidate can in turn replace `B_best` of its own length with up to `lit_len` backward bytes, so the worst-case forward length it needs to outscore `best` (under the LIFO offset-monotonic walk argument) is `best.match_len - lit_len + 1`. The 4-byte tail probe now anchors at `tail_off = best.match_len - lit_len - 3` (via `checked_sub(lit_len + 3)`), which covers exactly that boundary. When `best.match_len <= lit_len + 3` the worst-case forward target is so close to `current_idx` that the chain-walk cost is already trivial — the gate is skipped via the `checked_sub`. When `lit_len == 0` the formula reduces to the pre-fix `best.match_len - 3` (no backward extension possible). Comment block in `hash_chain_candidate` now enumerates both the backward-extension bound and the offset-monotonicity argument. ## Driver test tightening (Copilot threads #2 / #3 / #4) - `driver_reset_from_row_backend_recycles_row_buffers_into_pool`: assertion changed from `vec_pool.len() >= before_pool` to `>` so the test fails on the regression it is meant to cover. With `before_pool = 0` on a fresh driver the weaker bound passed trivially even if the Row to Simple drain closure never pushed anything back into `vec_pool`. - `row_get_last_space_and_reset_to_fastest_clears_window` renamed to `..._drops_row_variant` with a doc comment explaining why the original `clears_window` assertion is intentionally gone (the variant itself is dropped on swap; no `row_matcher()` to inspect post-switch). Points at the sibling pool-recycling test by name. - `MatcherStorage` variant docs rewritten to enumerate the full `CompressionLevel` to backend mapping that `resolve_level_params` produces for each variant (Simple covers `Uncompressed`, `Fastest`, `Level(1)`, and any non-positive `Level(n) != 0`; Dfast covers `Default`, `Level(0)`, `Level(2)`, `Level(3)`; Row covers `Level(4)`; HashChain covers `Better`, `Best`, and `Level(5..=22)` across Lazy / BtOpt / BtUltra / BtUltra2). The pre-fix doc said "Level 1" / "Levels 2-3" which was misleading for the named-level variants. ## Verification - nextest 475/475 pass, 3 skipped (incl. `level22_sequences_match_donor_on_corpus_proxy` parity gate and the full cross-validation + roundtrip + ratio gates for default / better / best / level22). - doctests 10/10 pass. - clippy --all-targets -D warnings clean. - bench --quick on `^compress/default` (x86_64-darwin): every default-level scenario improves relative to the c_ffi baseline noise floor. Most relevant numbers: `decodecorpus-z000033` (the 27x C-FFI gap target from #111) gains -11 % vs c_ffi noise; `high-entropy-1m` -16 %; `low-entropy-1m` -8 %. --- zstd/src/encoding/dfast/mod.rs | 98 ++++++++++++++++++++++++++++ zstd/src/encoding/hc/mod.rs | 49 +++++++++----- zstd/src/encoding/match_generator.rs | 51 +++++++++++---- 3 files changed, 172 insertions(+), 26 deletions(-) diff --git a/zstd/src/encoding/dfast/mod.rs b/zstd/src/encoding/dfast/mod.rs index 489167e99..3c184c5f5 100644 --- a/zstd/src/encoding/dfast/mod.rs +++ b/zstd/src/encoding/dfast/mod.rs @@ -249,6 +249,14 @@ impl DfastMatchGenerator { handle_sequence, ); pos = start + candidate.match_len; + // Donor's opportunistic rep-0 extension after every emit. + pos = self.extend_with_repcode_after_match( + current_abs_start, + current_len, + pos, + &mut literals_start, + handle_sequence, + ); skip_step = 1; next_skip_growth_pos = pos + DFAST_SKIP_STEP_GROWTH_INTERVAL; miss_run = 0; @@ -339,6 +347,13 @@ impl DfastMatchGenerator { handle_sequence, ); pos = start + rep.match_len; + pos = self.extend_with_repcode_after_match( + current_abs_start, + current_len, + pos, + &mut literals_start, + handle_sequence, + ); skip_step = 1; next_skip_growth_pos = pos + DFAST_SKIP_STEP_GROWTH_INTERVAL; miss_run = 0; @@ -355,6 +370,13 @@ impl DfastMatchGenerator { handle_sequence, ); pos = start + candidate.match_len; + pos = self.extend_with_repcode_after_match( + current_abs_start, + current_len, + pos, + &mut literals_start, + handle_sequence, + ); skip_step = 1; next_skip_growth_pos = pos + DFAST_SKIP_STEP_GROWTH_INTERVAL; miss_run = 0; @@ -389,6 +411,82 @@ impl DfastMatchGenerator { self.emit_trailing_literals(literals_start, handle_sequence); } + /// Donor `zstd_double_fast.c` post-match rep-0 extension. After the + /// primary match has been emitted and `pos` advanced past it, donor + /// opportunistically chains additional `rep_2`-coded matches at the + /// new cursor as long as 4 bytes at `ip` keep matching the bytes at + /// `ip - offset_2` (in donor naming; in Rust offset terms this is + /// `offset_hist[1]` once `lit_len == 0` after the just-emitted + /// primary). Each iteration: + /// + /// * emits one zero-literal sequence with the old `offset_hist[1]`, + /// * swaps `offset_hist[0]` ↔ `offset_hist[1]` via + /// [`encode_offset_with_history`] (the donor `offset_2 = offset_1; + /// offset_1 = old_offset_2;` swap), + /// * skips the hash-table probe entirely on every extra match. + /// + /// Critically uses donor's `MINMATCH = 4` here rather than the + /// stricter `DFAST_MIN_MATCH_LEN = 6` enforced on the main search + /// loop. The donor accepts any 4-byte rep extension; we mirror that + /// because the rep emission carries no offset cost — even a 4-byte + /// rep is a net win over re-running the full hash search. Returns + /// the new value of `pos` and updates `literals_start` in place to + /// the post-rep-chain anchor. + fn extend_with_repcode_after_match( + &mut self, + current_abs_start: usize, + current_len: usize, + mut pos: usize, + literals_start: &mut usize, + handle_sequence: &mut impl for<'a> FnMut(Sequence<'a>), + ) -> usize { + const DONOR_REP_MIN_MATCH_LEN: usize = 4; + loop { + // Need at least DONOR_REP_MIN_MATCH_LEN bytes of room past `pos`. + if pos + DONOR_REP_MIN_MATCH_LEN > current_len { + break; + } + // After a primary emit `literals_start == pos`, so `lit_len` + // on the next sequence is zero — donor's rep probe uses + // `offset_2` (== `offset_hist[1]` under our encoding). + let rep = self.offset_hist[1] as usize; + if rep == 0 || rep > pos { + break; + } + let abs_pos = current_abs_start + pos; + let cur_idx = abs_pos - self.history_abs_start; + let cand_idx = match cur_idx.checked_sub(rep) { + Some(idx) => idx, + None => break, + }; + let concat = &self.history[self.history_start..]; + if cur_idx + DONOR_REP_MIN_MATCH_LEN > concat.len() { + break; + } + // Cheap 4-byte gate before the SIMD `common_prefix_len`. + if concat[cur_idx..cur_idx + 4] != concat[cand_idx..cand_idx + 4] { + break; + } + let match_len = common_prefix_len(&concat[cand_idx..], &concat[cur_idx..]); + if match_len < DONOR_REP_MIN_MATCH_LEN { + break; + } + // Insert the rep range into hash tables so future positions + // hashing into this area find these candidates. + self.insert_positions(abs_pos, abs_pos + match_len); + // Emit zero-literal rep sequence. + handle_sequence(Sequence::Triple { + literals: &[], + offset: rep, + match_len, + }); + let _ = encode_offset_with_history(rep as u32, 0, &mut self.offset_hist); + pos += match_len; + *literals_start = pos; + } + pos + } + pub(crate) fn seed_remaining_hashable_starts( &mut self, current_abs_start: usize, diff --git a/zstd/src/encoding/hc/mod.rs b/zstd/src/encoding/hc/mod.rs index 2cb99fc79..229eeaa50 100644 --- a/zstd/src/encoding/hc/mod.rs +++ b/zstd/src/encoding/hc/mod.rs @@ -231,30 +231,49 @@ impl HcMatcher { // Donor speculative tail check (`zstd_lazy.c:714`, // `ZSTD_HcFindBestMatch`): once `best` is set, gate the // expensive `common_prefix_len` walk on a 4-byte tail compare - // proving the candidate can reach `best_ml + 1`. See the - // monotonic-offset argument inside the loop for correctness. + // proving the new candidate can possibly reach the *forward* + // length required to outscore `best` under + // [`Self::better_candidate`] (gain = `len*4 - offset_bits`). + // + // Correctness — backward-extension–aware bound: + // `best.match_len` is the *total* length stored by + // [`Self::extend_backwards`]: forward bytes from + // `current_idx` plus up to `lit_len` backward bytes + // (`B_best = abs_pos − best.start`, capped by `lit_len`). + // A new candidate can in principle replace `B_best` of its + // own length with up to `lit_len` backward bytes, so the + // worst-case forward length it needs to outscore `best` + // is `best.match_len − lit_len + 1`. The 4-byte tail probe + // at offset `best.match_len − lit_len − 3` covers exactly + // that boundary (the read includes the byte at the + // required-forward-length position itself). A mismatch + // there is a proof the candidate cannot win regardless of + // how much it later extends backwards. + // When `best.match_len ≤ lit_len + 3` the worst-case + // forward target is so close to `current_idx` that the + // `common_prefix_len` cost is already trivial — the gate + // is skipped via the `checked_sub`. + // + // Walk-order argument (offset monotonicity): + // Chain walks are LIFO (newest first → strictly increasing + // offset). Once `best` is set, every subsequent candidate + // has `new.offset_bits ≥ best.offset_bits`, so + // `better_candidate` can only return a new winner when the + // *total* match length (forward + backward) strictly + // exceeds `best.match_len`. Combined with the worst-case + // backward-extension bound above, the required forward + // length is `best.match_len − lit_len + 1`. let history_tail = concat.len(); for candidate_abs in self.chain_candidates(table, abs_pos) { if candidate_abs == usize::MAX { break; } let candidate_idx = candidate_abs - table.history_abs_start; - if let Some(best_ml) = best.map(|b| b.match_len) - && best_ml >= 4 + if let Some(best_ref) = best + && let Some(tail_off) = best_ref.match_len.checked_sub(lit_len + 3) { - let tail_off = best_ml - 3; let m_end = candidate_idx + tail_off + 4; let i_end = current_idx + tail_off + 4; - // Chain walk is LIFO (newest first → strictly - // increasing offset). Once `best` is set, every - // subsequent candidate has `new.offset_bits >= - // best.offset_bits`, so `better_candidate` (gain = - // `len*4 - offset_bits`) can only return a new - // winner when `new.match_len > best_ml`. The 4-byte - // tail compare proves that pre-condition without - // running `common_prefix_len`. If either side runs - // out of room before `best_ml + 1`, the candidate - // cannot exceed `best_ml` either — same skip. if i_end > history_tail || m_end > history_tail { continue; } diff --git a/zstd/src/encoding/match_generator.rs b/zstd/src/encoding/match_generator.rs index ba6f20553..f5a161014 100644 --- a/zstd/src/encoding/match_generator.rs +++ b/zstd/src/encoding/match_generator.rs @@ -419,17 +419,29 @@ fn resolve_level_params(level: CompressionLevel, source_size: Option) -> Le /// of truth and dead variants are dropped when the active backend /// changes. enum MatcherStorage { - /// Level 1 — donor `ZSTD_fast`. Single-table Simple matcher; always - /// the initial variant constructed by [`MatchGeneratorDriver::new`]. + /// Donor `ZSTD_fast` family. Constructed by + /// [`MatchGeneratorDriver::new`] as the initial variant and + /// re-selected by [`Matcher::reset`] for any [`CompressionLevel`] + /// that `resolve_level_params` maps to [`StrategyTag::Fast`] + /// (`Uncompressed`, `Fastest`, `Level(1)`, and any non-positive + /// `Level(n)` not equal to `0`). Simple(MatchGenerator), - /// Levels 2-3 — donor `ZSTD_dfast`. Two-table hash chain. + /// Donor `ZSTD_dfast` family — two-table hash chain. Selected for + /// any level that resolves to [`StrategyTag::Dfast`] in + /// `resolve_level_params` (`Default`, `Level(0)`, `Level(2)`, + /// `Level(3)`). Dfast(DfastMatchGenerator), - /// Level 4 — donor `ZSTD_greedy` with row hashing. + /// Donor `ZSTD_greedy` family with row hashing. Selected for any + /// level that resolves to [`StrategyTag::Greedy`] (currently + /// `Level(4)` only). Row(RowMatchGenerator), - /// Levels 5-22 — donor `ZSTD_lazy2` and the BT-based optimal modes - /// (`btopt` / `btultra` / `btultra2`). Holds an [`HcMatchGenerator`] - /// whose internal [`HcBackend`] discriminator decides whether BT - /// scratch is allocated. + /// Donor `ZSTD_lazy2` and the BT-based optimal modes + /// (`btopt` / `btultra` / `btultra2`). Selected for any level that + /// resolves to [`StrategyTag::Lazy`], [`StrategyTag::BtOpt`], + /// [`StrategyTag::BtUltra`], or [`StrategyTag::BtUltra2`] + /// (`Better`, `Best`, `Level(5..=22)`). The + /// [`HcMatchGenerator`]'s internal [`HcBackend`] discriminator + /// decides whether BT scratch is allocated. HashChain(HcMatchGenerator), } @@ -5381,10 +5393,20 @@ fn prime_with_dictionary_budget_shrinks_after_row_eviction() { ); } +/// Row → Simple transition drops the Row variant and the +/// post-switch active backend is exactly Simple. The window-emptied +/// check from the pre-enum era (`driver.row_matcher().window.is_empty()`) +/// is intentionally gone — the `Row` variant no longer exists after +/// the swap, so there is nothing to inspect by accessor; the "window +/// cleared" invariant is replaced by "variant dropped", and a +/// subsequent `row_matcher()` call would panic by design. The +/// pool-recycling side of the same transition is covered by +/// [`driver_reset_from_row_backend_recycles_row_buffers_into_pool`]. #[test] -fn row_get_last_space_and_reset_to_fastest_clears_window() { +fn row_get_last_space_then_reset_to_fastest_drops_row_variant() { let mut driver = MatchGeneratorDriver::new(8, 1); driver.reset(CompressionLevel::Level(4)); + assert_eq!(driver.active_backend(), super::strategy::BackendTag::Row); let mut space = driver.get_next_space(); space.clear(); @@ -5421,9 +5443,16 @@ fn driver_reset_from_row_backend_recycles_row_buffers_into_pool() { driver.reset(CompressionLevel::Fastest); assert_eq!(driver.active_backend(), super::strategy::BackendTag::Simple); + // `>` not `>=`: a fresh driver starts with `before_pool == 0`, so the + // weaker bound passes even if the Row→Simple transition failed to + // drain the committed buffer back into `vec_pool`. Strict growth + // proves the drain ran. Single fixture buffer → exactly one Vec + // returned to the pool. assert!( - driver.vec_pool.len() >= before_pool, - "row reset should recycle row history buffers" + driver.vec_pool.len() > before_pool, + "row reset must recycle the committed row history buffer into vec_pool \ + (before_pool = {before_pool}, after = {})", + driver.vec_pool.len() ); } From 06dbcd15b7b2b138efdcbcd7d5a532348b582165 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Fri, 15 May 2026 03:42:48 +0300 Subject: [PATCH 04/12] =?UTF-8?q?fix(encoding):=20#124=20Phase=204=20?= =?UTF-8?q?=E2=80=94=20review=20feedback=20on=20post-match=20rep=20+=20spe?= =?UTF-8?q?culative=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review fixes from CodeRabbit / Copilot on PR #125. Bundles the behavioural fix called out by both reviewers (block-local `rep > pos` guard wrongly rejected retained-history offsets) plus targeted regression tests for the helpers that previously had only roundtrip coverage. ## Behavioural fix: `extend_with_repcode_after_match` accepts cross-block offsets CodeRabbit (Major / Quick win) and Copilot both flagged that the helper's `if rep == 0 || rep > pos { break; }` guard rejected valid repeat offsets pointing into retained history whenever the source lived past the current block-local cursor. Since `DfastMatchGenerator` keeps a contiguous `live_history()` buffer covering `history_abs_start..history_abs_end`, the only correct bound is "does the candidate land at or after `history_abs_start`" — which `cur_idx.checked_sub(rep)` already enforces (`None` ⇒ rep points before `history_abs_start`, break). Replaced the guard with `if rep == 0 { break; }` and added a comment explaining that `checked_sub` is the authoritative bound. Near block boundaries this restores the donor-parity chain win the helper is meant to recover. ## Regression tests * `dfast::extend_with_repcode_tests::dfast_repcode_extension_emits_zero_literal_rep_on_constant_run` exercises the post-match rep helper directly with a hand-built post-primary-match state: a constant-byte block, `offset_hist[1] = 1`, and an in-block cursor mid-block. Verifies the helper emits a zero-literal Triple with `offset = 1` (donor parity) and advances `pos` / `literals_start` consistently. Going through `start_matching` was unreliable because the primary `best_match` greedily consumes the entire constant run in a single Triple (`offset = 1, match_len = block - 1`), leaving the helper nothing to extend. * `dfast::extend_with_repcode_tests::dfast_repcode_extension_walks_into_retained_history` builds a two-block fixture and probes with `rep = 40` against a block-local cursor at `pos = 5`. The pre-fix `rep > pos` guard would have rejected this; the post-fix code emits the rep correctly. * `hc::hc_tests::hash_chain_candidate_speculative_gate_handles_lit_len_backward_extension` guards the post-fix `tail_off = best.match_len - lit_len - 3` formula with a monotonicity check: for the same probe position and chain content, the returned `match_len` must be non-decreasing in `lit_len` because more backward-extension headroom can only enlarge (or leave unchanged) every candidate's effective length. A regression where the gate skips candidates that backward extension would have rescued surfaces as `len_high_lit < len_low_lit`. ## Doc / comment clarifications (Copilot threads on `MatcherStorage`) * The post-switch state comment in `driver_best_to_fastest_releases_oversized_hc_tables` previously said the manual `hash_table = Vec::new()` assignments release allocations "immediately rather than waiting for `Drop`". That was misleading — assigning `self.storage = MatcherStorage::Simple(...)` would drop the old `HashChain` variant in the same call regardless. The real reason for the up-front reassignment is to cap peak memory during the swap: by releasing the table allocations *before* constructing the replacement variant, the peak resident set is "old data buffers being drained into `vec_pool` + new `MatchGenerator` skeleton" instead of "old tables still resident + new variant under construction". Comment updated to reflect that. ## Verification - nextest 478/478 pass, 3 skipped (478 = 475 + the three new tests). - doctests 10/10 pass. - clippy --all-targets -D warnings clean. --- zstd/src/encoding/dfast/mod.rs | 205 ++++++++++++++++++++++++++- zstd/src/encoding/hc/mod.rs | 71 ++++++++++ zstd/src/encoding/match_generator.rs | 22 +-- 3 files changed, 288 insertions(+), 10 deletions(-) diff --git a/zstd/src/encoding/dfast/mod.rs b/zstd/src/encoding/dfast/mod.rs index 3c184c5f5..9e644f63a 100644 --- a/zstd/src/encoding/dfast/mod.rs +++ b/zstd/src/encoding/dfast/mod.rs @@ -450,11 +450,21 @@ impl DfastMatchGenerator { // on the next sequence is zero — donor's rep probe uses // `offset_2` (== `offset_hist[1]` under our encoding). let rep = self.offset_hist[1] as usize; - if rep == 0 || rep > pos { + if rep == 0 { break; } let abs_pos = current_abs_start + pos; let cur_idx = abs_pos - self.history_abs_start; + // `checked_sub` is the authoritative bound here: a valid rep + // can reach beyond the current block into retained history + // (the contiguous `live_history()` buffer covers + // `history_abs_start..history_abs_end`), so the only hard + // constraint is `cur_idx >= rep` (i.e. the candidate is in + // the addressable history range). A previous draft also + // gated on `rep > pos`, which over-rejected valid offsets + // that point into retained history near block boundaries — + // exactly the donor-style chain win this helper is meant to + // recover. let cand_idx = match cur_idx.checked_sub(rep) { Some(idx) => idx, None => break, @@ -810,3 +820,196 @@ impl DfastMatchGenerator { (mixed >> (64 - self.hash_bits)) as usize } } + +#[cfg(test)] +mod extend_with_repcode_tests { + //! Targeted regression coverage for `extend_with_repcode_after_match`. + //! + //! These tests intentionally bypass the higher-level + //! `compress_to_vec` roundtrip path used by `cross_validation` so + //! that a failure pinpoints the post-match rep helper rather than + //! firing somewhere downstream (block writer / huff0 / FSE / decode). + //! The capture closure records the exact sequence stream the matcher + //! emits, which is what the assertions check. + use alloc::vec; + use alloc::vec::Vec; + + use super::*; + + /// Capture every sequence the matcher emits into an owned record, + /// so the assertions can match on `lit_len` / `offset` / `match_len` + /// shape directly. `Sequence::Triple` carries borrowed literals; we + /// take their length and discard the bytes (the test only cares + /// about the structural shape, not the literal content). + #[derive(Debug, Clone, PartialEq, Eq)] + enum CapturedSeq { + Triple { + lit_len: usize, + offset: usize, + match_len: usize, + }, + Literals { + lit_len: usize, + }, + } + + fn record_seq<'a>(out: &'a mut Vec) -> impl FnMut(Sequence<'_>) + 'a { + move |seq| match seq { + Sequence::Triple { + literals, + offset, + match_len, + } => out.push(CapturedSeq::Triple { + lit_len: literals.len(), + offset, + match_len, + }), + Sequence::Literals { literals } => out.push(CapturedSeq::Literals { + lit_len: literals.len(), + }), + } + } + + fn build_dfast_with(data: &[u8]) -> DfastMatchGenerator { + // Window sized to the block so the matcher does not start + // trimming history mid-test. + let mut dfast = DfastMatchGenerator::new(data.len().next_power_of_two().max(64)); + dfast.use_fast_loop = false; // exercise `start_matching_general` + dfast.ensure_hash_tables(); + dfast.add_data(data.to_vec(), |_| {}); + dfast + } + + /// Direct call into [`DfastMatchGenerator::extend_with_repcode_after_match`] + /// with a hand-built post-primary-match state. Going through + /// `start_matching` is unreliable for this assertion because the + /// primary `best_match` greedily consumes a constant run in a + /// single `Triple` (offset 1, match_len = block - 1), leaving the + /// helper nothing to extend. Instead we set up the state the + /// helper expects after a primary emit and verify it chains + /// rep-0 sequences for as many bytes as the rep predicate + /// matches. + #[test] + fn dfast_repcode_extension_emits_zero_literal_rep_on_constant_run() { + let data: Vec = vec![b'A'; 64]; + let mut dfast = build_dfast_with(&data); + + // Post-primary-match state: pretend a previous sequence emitted + // with offset = 4 (`offset_hist[0]`). Under the donor swap the + // post-match rep probe consults `offset_hist[1]`, here set to + // 1 so every subsequent byte (constant 'A') matches its + // predecessor. + dfast.offset_hist = [4, 1, 8]; + let current_abs_start = dfast.history_abs_start + dfast.window_size - data.len(); + let current_len = data.len(); + // Start the helper mid-block; the leading bytes are the + // "literals + match" the (simulated) primary would have + // covered. `literals_start == pos` is the post-emit invariant + // — `lit_len` for the next sequence is zero. + let pos = 10usize; + let mut literals_start = pos; + + let mut seqs = Vec::new(); + let new_pos = { + let mut rec = record_seq(&mut seqs); + dfast.extend_with_repcode_after_match( + current_abs_start, + current_len, + pos, + &mut literals_start, + &mut rec, + ) + }; + + assert!( + new_pos > pos, + "helper must advance pos past at least one rep match \ + (pos={pos}, new_pos={new_pos})" + ); + assert_eq!( + literals_start, new_pos, + "helper must keep literals_start == new_pos so the caller's main \ + loop sees zero pending literals after the rep chain" + ); + assert!(!seqs.is_empty(), "helper must emit at least one Triple"); + for seq in &seqs { + match seq { + CapturedSeq::Triple { + lit_len, + offset, + match_len: _, + } => { + assert_eq!( + *lit_len, 0, + "rep emission must be zero-literal (got {seq:?})" + ); + assert_eq!( + *offset, 1, + "rep emission must use the swapped-in offset_hist[1] = 1 \ + (got {seq:?})" + ); + } + CapturedSeq::Literals { .. } => { + panic!("rep extension must not emit a Literals tail: {seq:?}"); + } + } + } + } + + /// Cross-block / retained-history case: probe with `offset > pos` + /// (where `pos` is block-local) so the candidate lives in retained + /// history from a previously committed block. The + /// CodeRabbit-flagged `rep > pos` guard would have rejected + /// exactly this path — the current implementation only gates on + /// `cur_idx.checked_sub(rep)` so the helper accepts the cross- + /// block offset and emits the rep sequence. + #[test] + fn dfast_repcode_extension_walks_into_retained_history() { + let block_a: Vec = vec![b'C'; 64]; + let block_b: Vec = vec![b'C'; 32]; + let mut dfast = DfastMatchGenerator::new(256); + dfast.use_fast_loop = false; + dfast.ensure_hash_tables(); + dfast.add_data(block_a, |_| {}); + dfast.add_data(block_b.clone(), |_| {}); + + // Post-primary-match state targeting cross-block rep: probe + // offset = 40 (a candidate inside block A bytes), block-local + // cursor = 5 (so `rep > pos` under the rejected guard). + dfast.offset_hist = [4, 40, 8]; + let current_len = block_b.len(); + let current_abs_start = dfast.history_abs_start + dfast.window_size - current_len; + let pos = 5usize; + let mut literals_start = pos; + + let mut seqs = Vec::new(); + let new_pos = { + let mut rec = record_seq(&mut seqs); + dfast.extend_with_repcode_after_match( + current_abs_start, + current_len, + pos, + &mut literals_start, + &mut rec, + ) + }; + + assert!( + new_pos > pos, + "rep with offset > block-local pos must still emit a match when the \ + candidate lives in retained history (pos={pos}, new_pos={new_pos})" + ); + assert_eq!(seqs.len(), 1, "expected one rep emit, got {seqs:?}"); + match &seqs[0] { + CapturedSeq::Triple { + lit_len, + offset, + match_len: _, + } => { + assert_eq!(*lit_len, 0, "rep emit must be zero-literal"); + assert_eq!(*offset, 40, "rep emit must use the cross-block offset 40"); + } + other => panic!("expected Triple, got {other:?}"), + } + } +} diff --git a/zstd/src/encoding/hc/mod.rs b/zstd/src/encoding/hc/mod.rs index 229eeaa50..232e3c952 100644 --- a/zstd/src/encoding/hc/mod.rs +++ b/zstd/src/encoding/hc/mod.rs @@ -693,4 +693,75 @@ mod hc_tests { let t = table_with_history(b"abc"); assert!(hc.find_best_match(&t, 0, 1).is_none()); } + + /// Regression test for the speculative tail check's + /// backward-extension bound. The post-fix formula is + /// `tail_off = best.match_len − lit_len − 3` (via + /// `checked_sub(lit_len + 3)`); the pre-fix gate used + /// `best_ml − 3` directly and over-rejected later chain candidates + /// when `lit_len > 0` because `extend_backwards` may have added up + /// to `lit_len` backward bytes to `best.match_len`. + /// + /// The test exercises behavioural parity across `lit_len` values: + /// for the same probe position and chain content, the helper must + /// return a match length that is *non-decreasing* in `lit_len`, + /// because more backward-extension headroom can only enlarge (or + /// leave unchanged) every candidate's effective length. A + /// regression where the gate skips candidates that backward + /// extension would have rescued shows up as `len_high_lit < + /// len_low_lit`. + /// + /// Fixture: a repeating "ABCDEFGH" pattern at idx 0, 8, 16, 24 + /// inside a 40-byte history. Probing at `abs_pos = 24` finds + /// chunks 0, 8, 16 as chain candidates (LIFO order 16 → 8 → 0). + /// Every candidate is an 8-byte forward match; the gate is what + /// decides whether each is *evaluated*. Without the `lit_len` + /// correction, the pre-fix gate would drop the later candidates + /// after seeing the first match, even when backward extension + /// could grow them. + #[test] + fn hash_chain_candidate_speculative_gate_handles_lit_len_backward_extension() { + let mut t = MatchTable::new(64); + t.history = b"ABCDEFGHABCDEFGHABCDEFGHABCDEFGHZZZZZZZZ".to_vec(); + t.history_start = 0; + t.history_abs_start = 0; + t.window_size = t.history.len(); + t.position_base = 0; + t.hash_log = 8; + t.chain_log = 8; + t.hash3_log = 0; + t.ensure_tables(); + t.window.push_back(t.history.clone()); + t.insert_positions(0, 24); + + let hc = HcMatcher::new(2, 16, 64); + + let len_for = |lit_len: usize| -> usize { + hc.hash_chain_candidate(&t, 24, lit_len) + .map(|c| c.match_len) + .unwrap_or(0) + }; + + let len0 = len_for(0); + let len1 = len_for(1); + let len4 = len_for(4); + + assert!( + len0 >= 4, + "lit_len=0 must still find at least a 4-byte (HC_MIN_MATCH_LEN) \ + chain match on the repeating fixture, got {len0}" + ); + assert!( + len1 >= len0, + "monotonicity violation: lit_len=1 returned shorter match than \ + lit_len=0 ({len1} < {len0}) — speculative gate must not drop \ + candidates that backward extension could rescue" + ); + assert!( + len4 >= len1, + "monotonicity violation: lit_len=4 returned shorter match than \ + lit_len=1 ({len4} < {len1}) — speculative gate's `lit_len` term \ + must widen, not narrow, the candidate set as headroom grows" + ); + } } diff --git a/zstd/src/encoding/match_generator.rs b/zstd/src/encoding/match_generator.rs index f5a161014..a80058e4f 100644 --- a/zstd/src/encoding/match_generator.rs +++ b/zstd/src/encoding/match_generator.rs @@ -5047,15 +5047,19 @@ fn driver_best_to_fastest_releases_oversized_hc_tables() { driver.skip_matching_with_hint(None); // Switch to Fastest — the [`MatcherStorage`] enum swaps to the - // `Simple` variant and drops the `HashChain` variant entirely (the - // HC reset closure pre-drop pushes any retained data buffers into - // `vec_pool`, and the `hash_table` / `chain_table` / `hash3_table` - // are emptied by hand before drop so their backing allocations - // release immediately rather than waiting for the variant's `Drop` - // to run on the way out of the swap). Post-switch the HC variant - // no longer exists — there is nothing to inspect by accessor; the - // assertion that storage is now `Simple` covers the invariant the - // old hash_table/chain_table checks were proxying for. + // `Simple` variant and the `HashChain` variant is dropped. The + // drain block in `Matcher::reset` reassigns + // `m.table.hash_table` / `chain_table` / `hash3_table` to + // `Vec::new()` BEFORE constructing the replacement variant so the + // table backing allocations are released up front — this caps + // peak memory during the swap to "old data buffers being drained + // into `vec_pool` + new `MatchGenerator` skeleton" rather than + // "old tables still resident + new variant under construction". + // The eventual `Drop` on the old variant would release the tables + // anyway, but only after the new variant is built, so the early + // reassign shifts the peak. Post-switch the HC variant no longer + // exists; the assertion that storage is now `Simple` covers the + // invariant the old hash_table/chain_table checks were proxying. driver.reset(CompressionLevel::Fastest); assert_eq!(driver.window_size(), (1u64 << 17)); assert_eq!(driver.active_backend(), super::strategy::BackendTag::Simple); From 59f5e718697288ae550808a8bf34fd19a3ea8500 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Fri, 15 May 2026 09:13:52 +0300 Subject: [PATCH 05/12] =?UTF-8?q?fix(encoding):=20#124=20Phase=204=20?= =?UTF-8?q?=E2=80=94=20HC=20speculative=20gate=20non-monotonic=20guard=20+?= =?UTF-8?q?=20precise=20regression?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review fixes from Copilot on PR #125. Tightens the HC speculative tail check's correctness preconditions and replaces the monotonicity-only regression test with one that actually fails for the pre-fix gate. ## Non-monotonic chain walks (Copilot thread on hc/mod.rs:283) The gate's correctness argument was "chain walks are LIFO → newest first → strictly increasing offset". But the chain table is `chain_log`-bits wide and cyclic: when a position is re-inserted at the same masked chain index after the cycle wraps, an older chain link can point into a slot that has since been OVERWRITTEN with a newer (closer) position. The walker then surfaces a candidate with a SMALLER offset than ones it already returned, breaking monotonicity. With non-monotonic ordering, a smaller-offset candidate can outscore `best` via offset bits at *equal* total length, and the gate (which only proves `match_len > best.match_len`) would skip that valid winner. Fix: add `new_offset >= best_ref.offset` to the gate's precondition. On non-monotonic walks the iteration falls through to the full `common_prefix_len` so the offset-bits advantage is given a chance to win. The comment block now distinguishes "backward-extension-aware bound" (the `lit_len` correction) from "monotonicity precondition" (the new offset check) and notes that the latter is enforced per-iteration, not assumed globally. ## Precise regression test (Copilot thread on hc/mod.rs:721) The previous test on a repeating "ABCDEFGH" fixture asserted monotonicity of `match_len` in `lit_len`, but every chain candidate could extend backward by the same amount on that fixture — so skipping the later candidates still produced non-decreasing lengths, and the pre-fix `best.match_len - 3` gate passed the test. Replaced with a hand-crafted 40-byte fixture `"AAAabcdefZMQabcdefIJBAAAabcdefIJKKKKKKKK"` where: * The first LIFO candidate (`idx 12`, offset 12) has 8 forward bytes but cannot extend backward (`concat[11] = 'Q'` ≠ `concat[23] = 'A'`) → `match_len = 8`. * The second LIFO candidate (`idx 3`, offset 21) has only 6 forward bytes (terminator `'Z'` at idx 9 ≠ probe byte `'I'` at idx 30) but CAN extend 3 bytes backward (`concat[0..3] = "AAA" = concat[21..24]`) → total `match_len = 9`. Pre-fix gate at `tail_off = 8 - 3 = 5` reads `concat[8..12] = "fZMQ"` vs probe `concat[29..33] = "fIJK"` for the second candidate — mismatch → SKIP — helper returns `match_len = 8` (loses the 9-byte backward-extended win). Post-fix gate at `tail_off = 8 - 3 - 3 = 2` reads `concat[5..9] = "cdef"` vs probe `concat[26..30] = "cdef"` — match → PASS → full count + backward extend → `match_len = 9`. Assertions: * `len_for(0) == 8` (no backward headroom, both candidates capped at forward). * `len_for(3) == 9` (post-fix gate lets the second candidate through). * `len_for(3) > len_for(0)` (strict gain — the signal the pre-fix gate would not have produced). The fixture also exercises the non-monotonic guard from the first fix: candidate offsets are `[12, 21]`, both monotonic so the gate applies — the test would not exercise the `new_offset < best.offset` branch added today, but the broader nextest / cross-validation suite has chain walks that do hit it. ## Verification - nextest 478/478 pass, 3 skipped. - doctests 10/10 pass. - clippy --all-targets -D warnings clean. --- zstd/src/encoding/hc/mod.rs | 162 ++++++++++++++++++++++++------------ 1 file changed, 107 insertions(+), 55 deletions(-) diff --git a/zstd/src/encoding/hc/mod.rs b/zstd/src/encoding/hc/mod.rs index 232e3c952..9c11d135d 100644 --- a/zstd/src/encoding/hc/mod.rs +++ b/zstd/src/encoding/hc/mod.rs @@ -254,22 +254,40 @@ impl HcMatcher { // `common_prefix_len` cost is already trivial — the gate // is skipped via the `checked_sub`. // - // Walk-order argument (offset monotonicity): - // Chain walks are LIFO (newest first → strictly increasing - // offset). Once `best` is set, every subsequent candidate - // has `new.offset_bits ≥ best.offset_bits`, so - // `better_candidate` can only return a new winner when the - // *total* match length (forward + backward) strictly - // exceeds `best.match_len`. Combined with the worst-case - // backward-extension bound above, the required forward - // length is `best.match_len − lit_len + 1`. + // Walk-order argument (offset monotonicity — REQUIRED gate + // precondition, enforced per-iteration): + // Chain walks are LIFO in their dense form (newest first → + // strictly increasing offset). But the chain table is + // `chain_log`-bits wide; when a position is re-inserted at + // the same masked chain index after the cycle wraps, an + // older chain link can point into a slot that has since + // been OVERWRITTEN with a newer (closer) position. The + // walker then surfaces a candidate with a SMALLER offset + // than ones it has already returned, breaking monotonicity. + // + // The gate's bound (`tail_off = best.match_len − lit_len − + // 3` covers exactly the "new forward must reach + // best.match_len − lit_len + 1" requirement) is only sound + // when `new.offset_bits ≥ best.offset_bits`. Otherwise a + // smaller-offset candidate can outscore `best` by gain at + // *equal* total length — and the gate would skip it because + // it only proves `match_len > best.match_len`. The + // `new_offset >= best.offset` per-iteration check below + // enforces the monotonicity precondition; on non-monotonic + // walks we fall through to the full `common_prefix_len` so + // the offset-bits advantage is given a chance to win. let history_tail = concat.len(); for candidate_abs in self.chain_candidates(table, abs_pos) { if candidate_abs == usize::MAX { break; } let candidate_idx = candidate_abs - table.history_abs_start; + // `abs_pos > candidate_abs` is invariant for in-range chain + // entries (filtered by `chain_candidates`), so the subtraction + // never underflows. + let new_offset = abs_pos - candidate_abs; if let Some(best_ref) = best + && new_offset >= best_ref.offset && let Some(tail_off) = best_ref.match_len.checked_sub(lit_len + 3) { let m_end = candidate_idx + tail_off + 4; @@ -695,34 +713,60 @@ mod hc_tests { } /// Regression test for the speculative tail check's - /// backward-extension bound. The post-fix formula is - /// `tail_off = best.match_len − lit_len − 3` (via - /// `checked_sub(lit_len + 3)`); the pre-fix gate used - /// `best_ml − 3` directly and over-rejected later chain candidates - /// when `lit_len > 0` because `extend_backwards` may have added up - /// to `lit_len` backward bytes to `best.match_len`. + /// backward-extension bound. The pre-fix gate used `tail_off = + /// best.match_len − 3` and was unaware that `extend_backwards` + /// could have added up to `lit_len` backward bytes to + /// `best.match_len`. The post-fix formula subtracts `lit_len` + /// via `checked_sub(lit_len + 3)`. + /// + /// To actually fail for the pre-fix gate (Copilot review on + /// `c16ca32b` flagged that an earlier round of this test did + /// not), the fixture is constructed so the first LIFO candidate + /// cannot extend backward but a later candidate can — only the + /// later candidate's *total* match length (`forward + + /// backward_extension`) reaches the new best. + /// + /// Fixture (40 bytes): + /// `"AAAabcdefZMQabcdefIJBAAAabcdefIJKKKKKKKK"` + /// 01234567890123456789012345678901234567890 (indices) + /// 1111111111222222222233333333334 /// - /// The test exercises behavioural parity across `lit_len` values: - /// for the same probe position and chain content, the helper must - /// return a match length that is *non-decreasing* in `lit_len`, - /// because more backward-extension headroom can only enlarge (or - /// leave unchanged) every candidate's effective length. A - /// regression where the gate skips candidates that backward - /// extension would have rescued shows up as `len_high_lit < - /// len_low_lit`. + /// Probing `abs_pos = 24, lit_len = 3`: + /// - The 4-byte hash at `idx 24` ("abcd") collides with the + /// hashes at `idx 3` and `idx 12` (also "abcd"). All other + /// positions in `0..24` hash to other buckets, so the chain + /// walker visits exactly `[12, 3]` in LIFO order. + /// - Candidate at `idx 12`: forward 8 bytes (`"abcdefIJ"` + /// matches the probe `"abcdefIJ"` at `24..32` exactly), but + /// the byte right before — `concat[11] = 'Q'` — does NOT + /// equal `concat[23] = 'A'`, so `extend_backwards` cannot + /// extend even one byte despite `lit_len = 3` of available + /// headroom. Total `match_len = 8`, offset = 12. + /// - Candidate at `idx 3`: forward only 6 bytes (`"abcdef"` + /// matches, byte 7 at `idx 9 = 'Z'` differs from probe byte + /// 7 at `idx 30 = 'I'`). But the 3 bytes before it — + /// `concat[0..3] = "AAA"` — exactly match `concat[21..24] = + /// "AAA"`, so `extend_backwards` adds 3 backward bytes. + /// Total `match_len = 6 + 3 = 9`, offset = 21. /// - /// Fixture: a repeating "ABCDEFGH" pattern at idx 0, 8, 16, 24 - /// inside a 40-byte history. Probing at `abs_pos = 24` finds - /// chunks 0, 8, 16 as chain candidates (LIFO order 16 → 8 → 0). - /// Every candidate is an 8-byte forward match; the gate is what - /// decides whether each is *evaluated*. Without the `lit_len` - /// correction, the pre-fix gate would drop the later candidates - /// after seeing the first match, even when backward extension - /// could grow them. + /// Gate behaviour at `lit_len = 3`: + /// - Pre-fix: `tail_off = best.match_len - 3 = 5`. For + /// candidate at `idx 3` the gate reads `concat[3+5..3+5+4] + /// = concat[8..12] = "fZMQ"` and compares to probe + /// `concat[29..33] = "fIJK"`. Mismatch → gate SKIPS. The + /// helper never runs `common_prefix_len` on candidate `3`, + /// never extends backwards, and returns `match_len = 8` + /// (the first candidate's match) — losing the 9-byte + /// backward-extended win. + /// - Post-fix: `tail_off = best.match_len - lit_len - 3 = 2`. + /// The gate reads `concat[3+2..3+2+4] = concat[5..9] = + /// "cdef"` and compares to probe `concat[26..30] = "cdef"`. + /// Match → gate PASSES → full count runs, finds forward 6, + /// extends backwards 3, returns `match_len = 9`. #[test] fn hash_chain_candidate_speculative_gate_handles_lit_len_backward_extension() { let mut t = MatchTable::new(64); - t.history = b"ABCDEFGHABCDEFGHABCDEFGHABCDEFGHZZZZZZZZ".to_vec(); + t.history = b"AAAabcdefZMQabcdefIJBAAAabcdefIJKKKKKKKK".to_vec(); t.history_start = 0; t.history_abs_start = 0; t.window_size = t.history.len(); @@ -736,32 +780,40 @@ mod hc_tests { let hc = HcMatcher::new(2, 16, 64); - let len_for = |lit_len: usize| -> usize { - hc.hash_chain_candidate(&t, 24, lit_len) - .map(|c| c.match_len) - .unwrap_or(0) - }; - - let len0 = len_for(0); - let len1 = len_for(1); - let len4 = len_for(4); - - assert!( - len0 >= 4, - "lit_len=0 must still find at least a 4-byte (HC_MIN_MATCH_LEN) \ - chain match on the repeating fixture, got {len0}" + // `lit_len = 0`: no backward extension headroom — neither + // candidate can grow past its forward match length, so the + // best wins at forward-only length 8. Both pre-fix and + // post-fix gates produce the same answer here. + let result_lit0 = hc.hash_chain_candidate(&t, 24, 0); + let len0 = result_lit0.map(|c| c.match_len).unwrap_or(0); + assert_eq!( + len0, 8, + "lit_len=0 must return the forward-only 8-byte match at offset 12, \ + got {len0}" ); - assert!( - len1 >= len0, - "monotonicity violation: lit_len=1 returned shorter match than \ - lit_len=0 ({len1} < {len0}) — speculative gate must not drop \ - candidates that backward extension could rescue" + + // `lit_len = 3`: the second candidate (`idx 3`) can extend 3 + // bytes backwards, giving a total length of 9. Pre-fix gate + // would skip it; post-fix gate lets it through. + let result_lit3 = hc.hash_chain_candidate(&t, 24, 3); + let len3 = result_lit3.map(|c| c.match_len).unwrap_or(0); + assert_eq!( + len3, 9, + "lit_len=3 must return the backward-extended 9-byte match \ + (forward 6 + backward 3); a value of 8 means the gate over-rejected \ + the second LIFO candidate and the helper missed the backward-extension \ + win (pre-fix regression). Got {len3}" ); + + // Strict-increase between `lit_len=0` and `lit_len=3` is the + // signal the pre-fix gate would NOT have produced. Keep this + // assertion explicitly so the test's failure message points + // at exactly the regression it guards. assert!( - len4 >= len1, - "monotonicity violation: lit_len=4 returned shorter match than \ - lit_len=1 ({len4} < {len1}) — speculative gate's `lit_len` term \ - must widen, not narrow, the candidate set as headroom grows" + len3 > len0, + "speculative gate must allow `lit_len`-dependent strict gains: \ + lit_len=0 → {len0}, lit_len=3 → {len3}. Equal values means the \ + gate skipped the backward-extending candidate." ); } } From 3a18ff035f07a7681e3f41b1b6f1ebf272d51c0d Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Fri, 15 May 2026 09:48:11 +0300 Subject: [PATCH 06/12] =?UTF-8?q?fix(encoding):=20#124=20Phase=204=20?= =?UTF-8?q?=E2=80=94=20pre-drain=20Dfast=20hash=20tables=20on=20backend=20?= =?UTF-8?q?switch=20+=20test=20comment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 review fixes from Copilot on PR #125. Two small follow-ups to the round-3 work. ## Pre-drain Dfast hash tables on backend transition The `MatcherStorage::Dfast` arm in `Matcher::reset`'s drain block called `m.reset` without first clearing `m.short_hash` and `m.long_hash`. `DfastMatchGenerator::reset` then ran `self.short_hash.fill(DFAST_EMPTY_SLOT)` and the same on long_hash before the variant was dropped by the subsequent `self.storage = ...` swap — wasted work, since the table backing allocations are about to be released anyway. The fill loop's short-circuit (`if !self.short_hash.is_empty()`) means handing it an empty Vec skips the work entirely. Mirrors the pre-drain pattern already used in the HashChain arm (release table allocations before constructing the replacement variant, keeping peak memory during the swap to just the data-buffer drain + new variant skeleton). ## Test fixture index guide The `hash_chain_candidate_speculative_gate_handles_lit_len_backward_extension` fixture is 40 bytes (indices `0..=39`) but the comment's index guide laid out 41 positions (`0..=40`) on the ones-digit line and left a stray `4` at the end of the tens-digit line. Trimmed both to match the actual fixture length, split the legend into separate "ones digit" / "tens digit" labels so the alignment is obvious at a glance. ## Verification - nextest 478/478 pass, 3 skipped. - clippy --all-targets -D warnings clean. --- zstd/src/encoding/hc/mod.rs | 6 +++--- zstd/src/encoding/match_generator.rs | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/zstd/src/encoding/hc/mod.rs b/zstd/src/encoding/hc/mod.rs index 9c11d135d..0417da2f4 100644 --- a/zstd/src/encoding/hc/mod.rs +++ b/zstd/src/encoding/hc/mod.rs @@ -726,10 +726,10 @@ mod hc_tests { /// later candidate's *total* match length (`forward + /// backward_extension`) reaches the new best. /// - /// Fixture (40 bytes): + /// Fixture (40 bytes, indices `0..=39`): /// `"AAAabcdefZMQabcdefIJBAAAabcdefIJKKKKKKKK"` - /// 01234567890123456789012345678901234567890 (indices) - /// 1111111111222222222233333333334 + /// 0123456789012345678901234567890123456789 (ones digit) + /// 1111111111222222222233333333 (tens digit, aligned) /// /// Probing `abs_pos = 24, lit_len = 3`: /// - The 4-byte hash at `idx 24` ("abcd") collides with the diff --git a/zstd/src/encoding/match_generator.rs b/zstd/src/encoding/match_generator.rs index a80058e4f..b37195e28 100644 --- a/zstd/src/encoding/match_generator.rs +++ b/zstd/src/encoding/match_generator.rs @@ -715,6 +715,20 @@ impl Matcher for MatchGeneratorDriver { }); } MatcherStorage::Dfast(m) => { + // Drop the long / short hash table allocations + // before calling `m.reset`. Without this prepass, + // `DfastMatchGenerator::reset` would `fill` both + // tables with `DFAST_EMPTY_SLOT` sentinels — wasted + // work given the next assignment to `self.storage` + // is about to drop `m` entirely. `reset` itself + // short-circuits on `if !self.short_hash.is_empty()`, + // so handing it an empty `Vec` skips the fill loop. + // Mirrors the pre-drain pattern in the HashChain + // arm below (and serves the same peak-memory + // purpose: release the table-allocation footprint + // before constructing the replacement variant). + m.short_hash = Vec::new(); + m.long_hash = Vec::new(); let vec_pool = &mut self.vec_pool; m.reset(|mut data| { data.resize(data.capacity(), 0); From 3c0ed89b263ce892e31e9523ad626be99f1c62da Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Fri, 15 May 2026 10:10:17 +0300 Subject: [PATCH 07/12] =?UTF-8?q?fix(encoding):=20#124=20Phase=204=20?= =?UTF-8?q?=E2=80=94=20non-monotonic=20walk=20regression=20+=204-byte=20re?= =?UTF-8?q?p=20+=20retire=5Fbudget=20bool?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-5 review fixes from CodeRabbit / Copilot on PR #125. Four findings, all addressed: ## Non-monotonic walk regression test (CR #14 / Copilot #15) The `new_offset >= best.offset` guard added in round 3 prevented the speculative gate from over-rejecting smaller-offset candidates surfaced by cyclic `chain_table` overwrites. The previous round's fixture only exercised the `lit_len` backward-extension bound — it did not force the non-monotonic walk path. Added `hash_chain_candidate_non_monotonic_walk_accepts_smaller_offset` which hand-wires `MatchTable::hash_table` and `MatchTable::chain_table` to surface a chain walk that visits `pos 9` first (offset 18) and `pos 18` second (offset 9 — strictly smaller). Both positions hold an identical 8-byte forward prefix match against the probe at `abs_pos = 27`, so the FORWARD lengths are equal and only the offset-bits difference can decide the winner. The test asserts the helper returns `offset = 9` (the smaller-offset winner). Without the `new_offset >= best.offset` fallback, the gate would activate for the second candidate, fail the 4-byte tail compare (the chunks have different terminator bytes at offset 8 from the chunk start), and skip the smaller- offset candidate — leaving `best.offset = 18`. Constructing this scenario via organic insertion order is not possible: LIFO insertion always points chain links at strictly older positions, so the non-monotonic visit order requires the cyclic-mask-collision case. Hand-wiring the chain is the only deterministic way to lock the regression down. ## 4-byte rep extension test (Copilot #16) The Dfast post-match rep helper uses donor's `MINMATCH = 4` (not the main-loop `DFAST_MIN_MATCH_LEN = 6` floor). The previous test fixtures only ran on long runs that extended much further than 4 bytes; a regression back to 6 would still have passed. Added `dfast_repcode_extension_accepts_exactly_four_byte_rep` on a 32-byte fixture where the rep match terminates at exactly 4 bytes (`concat[12] = '!'` vs `concat[4] = '?'` mismatches at the 5th rep byte). Asserts the helper emits a single Triple with `match_len = 4`. A regression to 6-byte threshold would skip this emission entirely and the test would fail with 0 emitted seqs. ## Variant docs: `Level(n > MAX_LEVEL)` clamp (Copilot #17) The `MatcherStorage::HashChain` doc claimed it covered "`Level(5..=22)`" but `resolve_level_params` clamps positive numeric levels at `MAX_LEVEL = 22` via `Level(n).clamp(1, MAX_LEVEL)`, so `Level(23..=i32::MAX)` also land on `BtUltra2` / HashChain. Added the clamp note to the doc so future callers don't reach for the wrong variant assuming the band is half-open at 22. ## `trim_after_budget_retire` gated on reclamation (CR #18) `commit_space` called `retire_dictionary_budget` + `trim_after_budget_retire` unconditionally on every slice commit. On the common no-dictionary / no-eviction path, `retire_dictionary_budget` early-returns at `reclaimed == 0` and `trim_after_budget_retire` does a backend `match` ladder + a single `trim_to_window` early-out — work that produces nothing. Changed `retire_dictionary_budget` to return `bool` (`#[must_use]`, `true` iff reclamation happened) and gated `trim_after_budget_retire` on that return at the `commit_space` call site. The `trim_after_budget_retire` body itself still calls `retire_dictionary_budget` in a loop driven by `evicted_bytes == 0` — the bool there is consumed by `let _ = …` because the loop's own termination condition is the right driver inside that scope. ## Verification - nextest 480/480 pass (478 + 2 new tests), 3 skipped. - doctests 10/10 pass. - clippy --all-targets -D warnings clean. --- zstd/src/encoding/dfast/mod.rs | 79 ++++++++++++++++++++++ zstd/src/encoding/hc/mod.rs | 99 ++++++++++++++++++++++++++++ zstd/src/encoding/match_generator.rs | 48 +++++++++++--- 3 files changed, 218 insertions(+), 8 deletions(-) diff --git a/zstd/src/encoding/dfast/mod.rs b/zstd/src/encoding/dfast/mod.rs index 9e644f63a..e0f64dcfb 100644 --- a/zstd/src/encoding/dfast/mod.rs +++ b/zstd/src/encoding/dfast/mod.rs @@ -1012,4 +1012,83 @@ mod extend_with_repcode_tests { other => panic!("expected Triple, got {other:?}"), } } + + /// The helper accepts 4-byte rep extensions (donor `MINMATCH = 4`), + /// not the main-loop `DFAST_MIN_MATCH_LEN = 6` floor. A regression + /// back to 6 would still pass the constant-run / cross-block tests + /// above (their rep matches extend much further), so this fixture + /// is built so the rep matches EXACTLY 4 bytes before terminating: + /// the byte at `pos + 4` differs from the byte at `pos + 4 - rep`. + /// + /// Fixture (32 bytes): + /// `"ABCD????ABCD!??????????ABCDX????"` + /// 0123456789012345678901234567890 (ones digit) + /// 1111111111222222222233 (tens digit, aligned) + /// + /// Probe state: + /// * `offset_hist[1] = 8` → rep probe reads `concat[pos - 8..]`. + /// * `pos = 8` → `concat[8..12] = "ABCD"`, `concat[0..4] = "ABCD"` + /// → 4 bytes match. + /// * `concat[12] = '!'` vs `concat[4] = '?'` → 5th byte mismatch, + /// so the rep extension stops at exactly 4 bytes. + #[test] + fn dfast_repcode_extension_accepts_exactly_four_byte_rep() { + // Block: "ABCD????" (8) + "ABCD!" (5) + "???????????" (11) + + // "ABCDX" (5) + "????" (3) = 32 bytes total. The + // important invariants are `concat[0..4] == "ABCD"`, + // `concat[8..12] == "ABCD"`, and `concat[12] = '!'` + // (so byte 12 ≠ byte 4 = '?', stopping the rep at + // length 4). The trailing bytes are irrelevant — we + // only iterate the helper at `pos = 8` and the rep + // chain terminates after one 4-byte emit because the + // next rep probe (post-swap) would need bytes at + // `pos + 4` to match a different offset. + let data: Vec = b"ABCD????ABCD!??????????ABCDX????".to_vec(); + assert_eq!(data.len(), 32, "fixture invariant: 32 bytes"); + let mut dfast = DfastMatchGenerator::new(64); + dfast.use_fast_loop = false; + dfast.ensure_hash_tables(); + dfast.add_data(data.clone(), |_| {}); + + dfast.offset_hist = [12, 8, 4]; + let current_abs_start = dfast.history_abs_start + dfast.window_size - data.len(); + let current_len = data.len(); + let pos = 8usize; + let mut literals_start = pos; + + let mut seqs = Vec::new(); + let new_pos = { + let mut rec = record_seq(&mut seqs); + dfast.extend_with_repcode_after_match( + current_abs_start, + current_len, + pos, + &mut literals_start, + &mut rec, + ) + }; + + // Helper must emit a single 4-byte rep, then stop because + // the 5th byte mismatches. + assert_eq!(seqs.len(), 1, "expected one 4-byte rep emit, got {seqs:?}"); + match &seqs[0] { + CapturedSeq::Triple { + lit_len, + offset, + match_len, + } => { + assert_eq!(*lit_len, 0, "rep emit must be zero-literal"); + assert_eq!(*offset, 8, "rep emit must use offset 8 (offset_hist[1])"); + assert_eq!( + *match_len, 4, + "rep emit must be exactly 4 bytes (donor MINMATCH floor). \ + A regression back to DFAST_MIN_MATCH_LEN = 6 would skip \ + this emission entirely and the test would fail with 0 seqs." + ); + } + other => panic!("expected Triple, got {other:?}"), + } + assert_eq!(new_pos, pos + 4, "pos must advance by exactly 4"); + assert_eq!(literals_start, new_pos, "literals_start must follow pos"); + } } diff --git a/zstd/src/encoding/hc/mod.rs b/zstd/src/encoding/hc/mod.rs index 0417da2f4..9668823d0 100644 --- a/zstd/src/encoding/hc/mod.rs +++ b/zstd/src/encoding/hc/mod.rs @@ -816,4 +816,103 @@ mod hc_tests { gate skipped the backward-extending candidate." ); } + + /// Regression test for the non-monotonic-walk fallback. When the + /// cyclic `chain_table & chain_mask` mask overwrites a slot, the + /// chain walker can surface a candidate with a SMALLER offset than + /// ones it has already returned. The speculative gate's + /// monotonicity precondition (`new.offset_bits ≥ best.offset_bits`) + /// is enforced per-iteration via the `new_offset ≥ best.offset` + /// check: when monotonicity breaks the gate falls through to + /// `common_prefix_len` so the offset-bits advantage is given a + /// chance to win. + /// + /// Construction: organic LIFO insertion order would never produce + /// this layout — when positions are inserted in monotonic order + /// the chain links naturally point at strictly older positions + /// (the previous `hash_table[hash]`). To force the bug-prone + /// scenario this test reaches into `MatchTable` and hand-wires the + /// chain so the walker visits `pos 8` first (offset 16) and then + /// `pos 16` second (offset 8). Both positions hold the same + /// 8-byte prefix `"abcdefgh"` (matching the probe at `pos 24`), so + /// the FORWARD lengths are equal and only the offset-bits + /// difference can decide the winner. + /// + /// With the new `new_offset >= best.offset` precondition: + /// * Iter 1: cand_abs 8, offset 16. `best = None` → no gate, + /// full count, `best = (len 8, offset 16)`. + /// * Iter 2: cand_abs 16, offset 8. `new_offset = 8 < + /// best.offset = 16` → gate skipped → full count runs → + /// `better_candidate` picks the smaller-offset winner (equal + /// length, smaller offset_bits → strictly higher gain). + /// + /// Final `best.offset` must be `8` (the smaller-offset winner). + /// Pre-fix code (gate applied unconditionally) would have + /// inspected the tail at `tail_off = 8 − 0 − 3 = 5` and the + /// 4-byte read at offsets `5..9` covers byte 8 which is the + /// forward-terminator mismatch (different ninth byte between + /// chunks) — gate would fail and the second candidate gets + /// skipped, leaving `best.offset = 16`. + #[test] + fn hash_chain_candidate_non_monotonic_walk_accepts_smaller_offset() { + // Four 8-byte chunks: `"abcdefgh"` at 0/8/16/24, terminated by + // different bytes (`'A'/'B'/'C'/'D'`) right after so the + // forward match between any two chunks caps at exactly 8. + let mut t = MatchTable::new(64); + t.history = b"abcdefghAabcdefghBabcdefghCabcdefghDZZZZ".to_vec(); + assert_eq!(t.history.len(), 40); + // After each "abcdefgh" the next byte is unique ('A'/'B'/'C' + // /'D'), capping cross-chunk forward matches at length 8. + t.history_start = 0; + t.history_abs_start = 0; + t.window_size = t.history.len(); + t.position_base = 0; + t.hash_log = 8; + t.chain_log = 8; + t.hash3_log = 0; + t.ensure_tables(); + t.window.push_back(t.history.clone()); + + // The probe is at abs_pos 27 (start of the fourth + // "abcdefgh" chunk). Hand-wire the chain so the walker + // visits pos 9 first and pos 18 second. + // + // Layout: chunks at 0..8, 9..17, 18..26, 27..35. The byte + // BEFORE each chunk is the terminator from the previous + // chunk's match. Probing at abs_pos 27: + // * candidate 9 → offset 27 − 9 = 18 + // * candidate 18 → offset 27 − 18 = 9 (SMALLER — second + // visit, non-monotonic) + let abs_pos = 27usize; + let concat = t.live_history(); + let probe_hash = t.hash_position(&concat[abs_pos..]); + // `stored = pos + 1` per `MatchTable::stored_abs_position_fast`. + // Hand-wire the chain head and the link OUT of pos 9 so the + // walk surfaces 9 first, then 18. + t.hash_table[probe_hash] = 9 + 1; + let chain_mask = (1usize << t.chain_log) - 1; + t.chain_table[9 & chain_mask] = 18 + 1; + // Terminate the walk after pos 18. + t.chain_table[18 & chain_mask] = HC_EMPTY; + + let hc = HcMatcher::new(2, 16, 64); + let result = hc.hash_chain_candidate(&t, abs_pos, 0); + let cand = result.expect("non-monotonic walk must still produce a match"); + assert_eq!( + cand.match_len, 8, + "both chain candidates have an 8-byte forward prefix match — \ + expected match_len = 8, got {}", + cand.match_len + ); + assert_eq!( + cand.offset, 9, + "non-monotonic fallback must surface the smaller-offset winner: \ + expected offset = 9 (cand_abs 18), got offset = {}. \ + A regression in the `new_offset >= best.offset` check would \ + keep the gate active for the smaller-offset second candidate, \ + skip its full count, and leave best.offset at 18 (the \ + larger-offset first-visited candidate).", + cand.offset + ); + } } diff --git a/zstd/src/encoding/match_generator.rs b/zstd/src/encoding/match_generator.rs index b37195e28..dd544d767 100644 --- a/zstd/src/encoding/match_generator.rs +++ b/zstd/src/encoding/match_generator.rs @@ -439,9 +439,13 @@ enum MatcherStorage { /// (`btopt` / `btultra` / `btultra2`). Selected for any level that /// resolves to [`StrategyTag::Lazy`], [`StrategyTag::BtOpt`], /// [`StrategyTag::BtUltra`], or [`StrategyTag::BtUltra2`] - /// (`Better`, `Best`, `Level(5..=22)`). The - /// [`HcMatchGenerator`]'s internal [`HcBackend`] discriminator - /// decides whether BT scratch is allocated. + /// (`Better`, `Best`, `Level(5..=22)`, and any `Level(n)` with + /// `n > MAX_LEVEL` — `resolve_level_params` clamps positive + /// numeric levels at `MAX_LEVEL = 22` via + /// `Level(n).clamp(1, MAX_LEVEL)`, so `Level(23..=i32::MAX)` all + /// land on `BtUltra2` here). The [`HcMatchGenerator`]'s internal + /// [`HcBackend`] discriminator decides whether BT scratch is + /// allocated. HashChain(HcMatchGenerator), } @@ -577,10 +581,19 @@ impl MatchGeneratorDriver { } } - fn retire_dictionary_budget(&mut self, evicted_bytes: usize) { + /// Shrink the active backend's `max_window_size` by the bytes + /// reclaimed from the dictionary-retention budget. Returns `true` + /// iff any reclamation happened — the caller uses that as the + /// gate for [`Self::trim_after_budget_retire`] (which is a no-op + /// otherwise: with `max_window_size` unchanged the backend's + /// `trim_to_window` cannot find anything to evict, so calling it + /// just runs an extra `match` ladder + a single early-out check + /// per slice commit). + #[must_use] + fn retire_dictionary_budget(&mut self, evicted_bytes: usize) -> bool { let reclaimed = evicted_bytes.min(self.dictionary_retained_budget); if reclaimed == 0 { - return; + return false; } self.dictionary_retained_budget -= reclaimed; match self.active_backend() { @@ -602,6 +615,7 @@ impl MatchGeneratorDriver { matcher.table.max_window_size.saturating_sub(reclaimed); } } + true } fn trim_after_budget_retire(&mut self) { @@ -660,7 +674,15 @@ impl MatchGeneratorDriver { if evicted_bytes == 0 { break; } - self.retire_dictionary_budget(evicted_bytes); + // Inside this loop, `evicted_bytes != 0` means the backend + // had bytes to evict because `max_window_size` was shrunk + // by a prior `retire_dictionary_budget` call — so + // `dictionary_retained_budget > 0` is the invariant the + // outer caller upholds, and the return-value of the retire + // call is consumed by `let _ = …` rather than gating the + // next loop pass on it (the loop's natural termination is + // `evicted_bytes == 0`, not `retire returns false`). + let _ = self.retire_dictionary_budget(evicted_bytes); } } @@ -1046,8 +1068,18 @@ impl Matcher for MatchGeneratorDriver { }); } } - self.retire_dictionary_budget(evicted_bytes); - self.trim_after_budget_retire(); + // Gate the second backend trim pass on actual budget + // reclamation. Without it, every slice commit on the + // no-dictionary / no-eviction path (the common case) would + // run a backend `match` ladder + `trim_to_window` early-out + // for no reason — `trim_after_budget_retire` only does + // meaningful work when `retire_dictionary_budget` shrank + // `max_window_size` enough to make the backend's + // `window_size > max_window_size` invariant trigger + // eviction. + if self.retire_dictionary_budget(evicted_bytes) { + self.trim_after_budget_retire(); + } } fn start_matching(&mut self, mut handle_sequence: impl for<'a> FnMut(Sequence<'a>)) { From 758c8d0864ae8bec761c60943ab1d3833176080e Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Fri, 15 May 2026 10:33:12 +0300 Subject: [PATCH 08/12] =?UTF-8?q?test(encoding):=20#124=20Phase=204=20?= =?UTF-8?q?=E2=80=94=20fast-loop=20helper=20roundtrip=20+=20index=20ruler?= =?UTF-8?q?=20trim?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-6 review fixes from Copilot on PR #125. Two findings on the Dfast test additions from round 5. ## Index ruler trimmed to 32 bytes (Copilot #19) The `dfast_repcode_extension_accepts_exactly_four_byte_rep` fixture header had a 31-char ones-digit ruler labelling positions 0..30 for a 32-byte fixture. Trimmed the ones-digit row to 32 chars (`0..=31`) and extended the tens-digit row to match. Also fixed an off-by-one in the inline byte-layout comment: the `"?"` segment between the `'!'` and the `"ABCDX"` block is 10 bytes, not 11, and the trailing `"?"` block is 4 bytes, not 3. Same math (32 = 8 + 5 + 10 + 5 + 4), labels now accurate. ## Fast-loop call-site coverage (Copilot #20) The post-match rep helper call sites inside `start_matching_fast_loop` (after both the `ip + 2` rep emit and the `ip0` `best_match` emit) had no direct test — every other test either invoked the helper directly or set `use_fast_loop = false`. `CompressionLevel::Default` / `Level(3)` enables the fast loop, so a regression there would silently break the helper on the production default path. Added `dfast_default_level_roundtrip_with_repetitive_breaks_exercises_fast_loop` which drives the fast loop through the production `compress_to_vec` + `StreamingDecoder` pipeline on a 4 KiB fixture engineered to exercise the post-match rep chain (60-byte runs of `'A'` separated by single `'B'` break bytes, plus a short trailing `'A'` tail per cycle). The breaks terminate the fast loop's primary match early, so subsequent iterations have runway for the helper to chain additional reps. Asserts byte-for-byte roundtrip parity and a conservative 2:1 ratio floor — a regression breaking the helper call would either invalidate the frames (decode error) or degrade the ratio noticeably. Constructing `DfastMatchGenerator` directly and asserting on captured sequences was attempted but the fixture engineering is brittle: the fast loop's primary match on simple constant fixtures consumes the entire remaining block in a single Triple, leaving no bytes for the helper to extend. The high-level roundtrip sidesteps that fragility while still routing through the same call site via the production driver. ## Verification - nextest 481/481 pass (480 + 1 new test), 3 skipped. - clippy --all-targets -D warnings clean. --- zstd/src/encoding/dfast/mod.rs | 83 ++++++++++++++++++++++++++++++++-- 1 file changed, 78 insertions(+), 5 deletions(-) diff --git a/zstd/src/encoding/dfast/mod.rs b/zstd/src/encoding/dfast/mod.rs index e0f64dcfb..d32bd1eb3 100644 --- a/zstd/src/encoding/dfast/mod.rs +++ b/zstd/src/encoding/dfast/mod.rs @@ -1020,10 +1020,10 @@ mod extend_with_repcode_tests { /// is built so the rep matches EXACTLY 4 bytes before terminating: /// the byte at `pos + 4` differs from the byte at `pos + 4 - rep`. /// - /// Fixture (32 bytes): + /// Fixture (32 bytes, indices `0..=31`): /// `"ABCD????ABCD!??????????ABCDX????"` - /// 0123456789012345678901234567890 (ones digit) - /// 1111111111222222222233 (tens digit, aligned) + /// 01234567890123456789012345678901 (ones digit) + /// 1111111111222222222233 (tens digit, aligned) /// /// Probe state: /// * `offset_hist[1] = 8` → rep probe reads `concat[pos - 8..]`. @@ -1033,8 +1033,8 @@ mod extend_with_repcode_tests { /// so the rep extension stops at exactly 4 bytes. #[test] fn dfast_repcode_extension_accepts_exactly_four_byte_rep() { - // Block: "ABCD????" (8) + "ABCD!" (5) + "???????????" (11) + - // "ABCDX" (5) + "????" (3) = 32 bytes total. The + // Block: "ABCD????" (8) + "ABCD!" (5) + "??????????" (10) + + // "ABCDX" (5) + "????" (4) = 32 bytes total. The // important invariants are `concat[0..4] == "ABCD"`, // `concat[8..12] == "ABCD"`, and `concat[12] = '!'` // (so byte 12 ≠ byte 4 = '?', stopping the rep at @@ -1091,4 +1091,77 @@ mod extend_with_repcode_tests { assert_eq!(new_pos, pos + 4, "pos must advance by exactly 4"); assert_eq!(literals_start, new_pos, "literals_start must follow pos"); } + + /// Integration coverage for the **fast-loop** call sites of + /// `extend_with_repcode_after_match` inside + /// `start_matching_fast_loop`. The direct-call tests above pin + /// down the helper's contract; this test drives the full fast + /// loop end-to-end through the production `compress_to_vec` + /// pipeline on a fixture engineered to exercise the post-match + /// rep chain on the fast-loop path. + /// + /// `CompressionLevel::Default` is the production config that + /// enables `use_fast_loop = true` (see `Matcher::reset` in + /// `match_generator.rs`). The fixture alternates 60-byte runs of + /// `'A'` with single `'B'` break bytes and a short `'A'` tail + /// per cycle — the breaks terminate the fast loop's primary match + /// early, so subsequent iterations have runway for the helper to + /// chain additional reps. A regression that broke either fast- + /// loop helper call site surfaces as a roundtrip failure (decoded + /// != input) or a ratio explosion. Constructing + /// `DfastMatchGenerator` directly and asserting on captured + /// sequences was attempted but the fixture engineering is + /// brittle: the fast loop's primary match on simple constant + /// fixtures consumes the entire remaining block in a single + /// Triple, leaving no bytes for the helper to extend. The + /// high-level roundtrip sidesteps that fragility while still + /// routing through the same call site via the production driver. + #[test] + fn dfast_default_level_roundtrip_with_repetitive_breaks_exercises_fast_loop() { + // ~4 KiB of input: 64 cycles of [60 'A's, 1 'B', 3 'A's]. + let mut data: Vec = Vec::with_capacity(64 * 64); + for _ in 0..64 { + data.extend_from_slice(&[b'A'; 60]); + data.push(b'B'); + data.extend_from_slice(&[b'A'; 3]); + } + assert!( + data.len() > 4000, + "fixture invariant: long enough for fast loop" + ); + + let compressed = crate::encoding::compress_to_vec( + data.as_slice(), + crate::encoding::CompressionLevel::Default, + ); + + // Decompress and assert byte-for-byte parity. A regression + // that broke the fast-loop helper call would either produce + // invalid frames (decode error) or wrong bytes (mismatch). + let mut decoder = crate::decoding::StreamingDecoder::new(compressed.as_slice()) + .expect("default-level frame must decode"); + let mut decoded = Vec::with_capacity(data.len()); + std::io::Read::read_to_end(&mut decoder, &mut decoded) + .expect("fast-loop output must round-trip cleanly"); + assert_eq!( + decoded, data, + "Default-level (use_fast_loop = true) roundtrip must be \ + byte-for-byte exact on the repetitive-breaks fixture" + ); + + // Ratio sanity: the post-match rep helper is what makes + // repetitive runs compress aggressively on the fast-loop + // path. A regression to a no-op helper would still produce + // some compression via the primary match, but the ratio + // would degrade. A 2:1 floor is conservative enough not to + // flake on small fixture changes while still catching + // structural failures of the fast loop. + assert!( + compressed.len() * 2 < data.len(), + "fast loop must compress repetitive runs to at least 2:1, \ + got {} → {} bytes", + data.len(), + compressed.len() + ); + } } From 1937a5441f7b0a32e0d75a1d7e893f8922ec6fdc Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Fri, 15 May 2026 11:24:38 +0300 Subject: [PATCH 09/12] =?UTF-8?q?fix(encoding):=20#124=20Phase=204=20?= =?UTF-8?q?=E2=80=94=20gate=20fast-loop=20test=20on=20std=20+=20comment=20?= =?UTF-8?q?corrections?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-7 review fixes from Copilot on PR #125. Three findings — all comment / test-gating cleanups on the round-6 additions. ## Gate fast-loop roundtrip test on `feature = "std"` (Copilot #21) The `dfast_default_level_roundtrip_with_repetitive_breaks_exercises_fast_loop` test calls `std::io::Read::read_to_end` on the `StreamingDecoder`, but `StreamingDecoder` implements the crate's `io_nostd::Read` alias instead of `std::io::Read` when the `std` feature is off, so the test fails to compile under `cargo check --no-default-features --tests`. The fast-loop helper itself is exercised under both configurations by: * the three direct-call tests above (`*_emits_zero_literal_rep_on_constant_run`, `*_walks_into_retained_history`, `*_accepts_exactly_four_byte_rep`), * the `cross_validation` Default-level roundtrip on the corpus fixture. Gating this single integration test on `feature = "std"` loses no coverage and avoids dual-trait rewriting through `crate::io::Read`. Doc comment now explains the gate. ## HC fixture chunk positions (Copilot #22) The `hash_chain_candidate_non_monotonic_walk_accepts_smaller_offset` fixture comment claimed the four `"abcdefgh"` chunks start at `0/8/16/24`, but each chunk is followed by a unique terminator byte (`'A' / 'B' / 'C' / 'D'`) as part of the same 40-byte stream, so the actual chunk starts are `0/9/18/27`. The hand-wiring code below already uses the `9/18/27` layout. Comment corrected to match. ## `trim_after_budget_retire` invariant comment (Copilot #23) The comment claimed `dictionary_retained_budget > 0` was an invariant inside the loop, but the budget can reach zero exactly when `retire_dictionary_budget` reclaims the last retained bytes — and the loop still needs to keep evicting because `max_window_size` just shrunk. The discarded retire-return-value is consumed via `let _ = …` precisely because the loop termination is driven by `evicted_bytes == 0`, not by whether the budget has more to reclaim. Comment now describes the prior-shrink condition instead of asserting a positive-budget invariant. ## Verification - nextest 481/481 pass under `--features hash,std,dict_builder`. - `cargo check --no-default-features` clean (no-std builds OK). - clippy --all-targets -D warnings clean. --- zstd/src/encoding/dfast/mod.rs | 15 +++++++++++++++ zstd/src/encoding/hc/mod.rs | 11 ++++++++--- zstd/src/encoding/match_generator.rs | 21 +++++++++++++-------- 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/zstd/src/encoding/dfast/mod.rs b/zstd/src/encoding/dfast/mod.rs index d32bd1eb3..9c998a91c 100644 --- a/zstd/src/encoding/dfast/mod.rs +++ b/zstd/src/encoding/dfast/mod.rs @@ -1116,6 +1116,18 @@ mod extend_with_repcode_tests { /// Triple, leaving no bytes for the helper to extend. The /// high-level roundtrip sidesteps that fragility while still /// routing through the same call site via the production driver. + /// + /// Gated on `feature = "std"`: the `Read::read_to_end` method + /// used to drain `StreamingDecoder` resolves to `std::io::Read` + /// only when std is enabled. Under no-std `StreamingDecoder` + /// implements the crate's `io_nostd::Read` alias instead, and + /// the call site has to be rewritten through that trait. The + /// fast-loop helper itself is exercised under both + /// configurations by the direct-call tests above plus the + /// `cross_validation` Default-level roundtrip — gating this one + /// integration test on std loses no coverage, only saves the + /// dual-trait rewrite. + #[cfg(feature = "std")] #[test] fn dfast_default_level_roundtrip_with_repetitive_breaks_exercises_fast_loop() { // ~4 KiB of input: 64 cycles of [60 'A's, 1 'B', 3 'A's]. @@ -1141,6 +1153,9 @@ mod extend_with_repcode_tests { let mut decoder = crate::decoding::StreamingDecoder::new(compressed.as_slice()) .expect("default-level frame must decode"); let mut decoded = Vec::with_capacity(data.len()); + // Under `feature = "std"` (the gate above) `StreamingDecoder` + // implements `std::io::Read`, so `Read::read_to_end` resolves + // through the standard library's blanket implementation. std::io::Read::read_to_end(&mut decoder, &mut decoded) .expect("fast-loop output must round-trip cleanly"); assert_eq!( diff --git a/zstd/src/encoding/hc/mod.rs b/zstd/src/encoding/hc/mod.rs index 9668823d0..075eac51e 100644 --- a/zstd/src/encoding/hc/mod.rs +++ b/zstd/src/encoding/hc/mod.rs @@ -855,9 +855,14 @@ mod hc_tests { /// skipped, leaving `best.offset = 16`. #[test] fn hash_chain_candidate_non_monotonic_walk_accepts_smaller_offset() { - // Four 8-byte chunks: `"abcdefgh"` at 0/8/16/24, terminated by - // different bytes (`'A'/'B'/'C'/'D'`) right after so the - // forward match between any two chunks caps at exactly 8. + // Four 8-byte `"abcdefgh"` chunks, each followed by a unique + // terminator byte (`'A' / 'B' / 'C' / 'D'`). The terminators + // are part of the same 40-byte stream, so each chunk start + // sits 9 bytes after the previous one — chunk starts are at + // `0`, `9`, `18`, `27` (not `0/8/16/24` as a naive + // chunk-width calculation would suggest). The cross-chunk + // forward match between any two chunks caps at exactly 8 + // because the byte right after each chunk is unique. let mut t = MatchTable::new(64); t.history = b"abcdefghAabcdefghBabcdefghCabcdefghDZZZZ".to_vec(); assert_eq!(t.history.len(), 40); diff --git a/zstd/src/encoding/match_generator.rs b/zstd/src/encoding/match_generator.rs index dd544d767..c03b2efa2 100644 --- a/zstd/src/encoding/match_generator.rs +++ b/zstd/src/encoding/match_generator.rs @@ -674,14 +674,19 @@ impl MatchGeneratorDriver { if evicted_bytes == 0 { break; } - // Inside this loop, `evicted_bytes != 0` means the backend - // had bytes to evict because `max_window_size` was shrunk - // by a prior `retire_dictionary_budget` call — so - // `dictionary_retained_budget > 0` is the invariant the - // outer caller upholds, and the return-value of the retire - // call is consumed by `let _ = …` rather than gating the - // next loop pass on it (the loop's natural termination is - // `evicted_bytes == 0`, not `retire returns false`). + // The loop's invariant is "the backend's previous + // `max_window_size` shrink had downstream bytes left to + // evict" — that's what `evicted_bytes != 0` proves at + // this point. `dictionary_retained_budget` is NOT + // guaranteed to be positive here: the outer + // `retire_dictionary_budget` call may have already + // drained it to zero by reclaiming the last retained + // bytes, while the backend still has bytes above the + // freshly-shrunk window cap waiting for this loop to + // evict. The return value of the retire call below is + // therefore intentionally discarded — the loop's + // termination is driven by `evicted_bytes == 0`, not by + // whether the budget has more bytes left to reclaim. let _ = self.retire_dictionary_budget(evicted_bytes); } } From 42199b364c41489287edcc8c8c8cd210615b1add Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Fri, 15 May 2026 11:45:38 +0300 Subject: [PATCH 10/12] =?UTF-8?q?perf(encoding):=20#124=20Phase=204=20?= =?UTF-8?q?=E2=80=94=20hoist=20Simple=20matcher=20borrow=20out=20of=20sequ?= =?UTF-8?q?ence=20loop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `compress_block::` Simple arm previously read `self.simple_mut().next_sequence(...)` inside the `while` condition, which re-evaluated the `MatcherStorage` enum match (plus its `unreachable!` guard) on every emitted sequence. On the Fast backend's hot loop that's per-Triple dispatch overhead the compiler can't easily fold across iterations because the borrow only lives for the condition. Hoist the borrow once: `let matcher = self.simple_mut();` before the `while`, then drive the loop through `matcher`. The storage match now runs exactly once per block-start, not once per emitted sequence. Code comment added at the call site so future refactors don't re-collapse the borrow back into the loop condition. ## Verification - nextest 481/481 pass. - clippy --all-targets -D warnings clean. --- zstd/src/encoding/match_generator.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/zstd/src/encoding/match_generator.rs b/zstd/src/encoding/match_generator.rs index c03b2efa2..821b53553 100644 --- a/zstd/src/encoding/match_generator.rs +++ b/zstd/src/encoding/match_generator.rs @@ -1147,7 +1147,17 @@ impl MatchGeneratorDriver { ) { use super::strategy::BackendTag; match S::BACKEND { - BackendTag::Simple => while self.simple_mut().next_sequence(&mut *handle_sequence) {}, + BackendTag::Simple => { + // Hoist the storage match out of the per-sequence loop. + // `self.simple_mut()` runs an `enum MatcherStorage` match + // arm + an `unreachable!` guard on every call, so emitting + // it inside `while self.simple_mut().next_sequence(...)` + // (the previous form) re-paid that dispatch on every + // sequence the Fast backend produced. Borrow once and + // drive the loop through the local handle. + let matcher = self.simple_mut(); + while matcher.next_sequence(&mut *handle_sequence) {} + } BackendTag::Dfast => self.dfast_matcher_mut().start_matching(handle_sequence), BackendTag::Row => self.row_matcher_mut().start_matching(handle_sequence), BackendTag::HashChain => self From 26f4fa1f7618624e092e81284475983332bb132d Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Fri, 15 May 2026 12:13:21 +0300 Subject: [PATCH 11/12] docs(encoding): align non-monotonic gate test doc comment with fixture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The outer doc comment for `hash_chain_candidate_non_monotonic_walk_accepts_smaller_offset` still described the round-5 draft layout (positions 8/16, offsets 16/8, final best.offset = 8) — but the inner comment + fixture were corrected in round 7 to use positions 9/18/27 (with the intervening terminator bytes 'A'/'B'/'C'/'D'), offsets 18/9, and final `best.offset = 9`. The stale outer block made the rationale inconsistent with the code. Updated outer doc: * "pos 9 first (offset 18) and then pos 18 second (offset 9)" * fixture description names the 4 chunks at `0 / 9 / 18 / 27` * iter trace uses cand_abs 9 (offset 18) → cand_abs 18 (offset 9) * final best.offset = 9 * pre-fix trace cites the chunk-terminator byte mismatch (`'A'/'B'/'C'/'D'`) as the gate-fail cause, matching the actual byte arrangement The test code itself was already correct — this is a comment-only correction so no behavioural change. nextest 481/481 still pass. --- zstd/src/encoding/hc/mod.rs | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/zstd/src/encoding/hc/mod.rs b/zstd/src/encoding/hc/mod.rs index 075eac51e..a9d309a6c 100644 --- a/zstd/src/encoding/hc/mod.rs +++ b/zstd/src/encoding/hc/mod.rs @@ -832,27 +832,30 @@ mod hc_tests { /// the chain links naturally point at strictly older positions /// (the previous `hash_table[hash]`). To force the bug-prone /// scenario this test reaches into `MatchTable` and hand-wires the - /// chain so the walker visits `pos 8` first (offset 16) and then - /// `pos 16` second (offset 8). Both positions hold the same - /// 8-byte prefix `"abcdefgh"` (matching the probe at `pos 24`), so - /// the FORWARD lengths are equal and only the offset-bits - /// difference can decide the winner. + /// chain so the walker visits `pos 9` first (offset 18) and then + /// `pos 18` second (offset 9). The fixture sits four 8-byte + /// `"abcdefgh"` chunks at positions `0 / 9 / 18 / 27` (each chunk + /// followed by a unique terminator byte that caps cross-chunk + /// forward matches at exactly 8); the probe at `abs_pos = 27` + /// hashes the same prefix as the earlier chunks, so all chain + /// candidates produce an 8-byte forward match and only the + /// offset-bits difference can decide the winner. /// /// With the new `new_offset >= best.offset` precondition: - /// * Iter 1: cand_abs 8, offset 16. `best = None` → no gate, - /// full count, `best = (len 8, offset 16)`. - /// * Iter 2: cand_abs 16, offset 8. `new_offset = 8 < - /// best.offset = 16` → gate skipped → full count runs → + /// * Iter 1: cand_abs 9, offset 18. `best = None` → no gate, + /// full count, `best = (len 8, offset 18)`. + /// * Iter 2: cand_abs 18, offset 9. `new_offset = 9 < + /// best.offset = 18` → gate skipped → full count runs → /// `better_candidate` picks the smaller-offset winner (equal /// length, smaller offset_bits → strictly higher gain). /// - /// Final `best.offset` must be `8` (the smaller-offset winner). + /// Final `best.offset` must be `9` (the smaller-offset winner). /// Pre-fix code (gate applied unconditionally) would have /// inspected the tail at `tail_off = 8 − 0 − 3 = 5` and the - /// 4-byte read at offsets `5..9` covers byte 8 which is the - /// forward-terminator mismatch (different ninth byte between - /// chunks) — gate would fail and the second candidate gets - /// skipped, leaving `best.offset = 16`. + /// 4-byte read at offsets `5..9` covers the chunk-terminator byte + /// (different `'A' / 'B' / 'C' / 'D'` per chunk) — gate fails on + /// the mismatching terminator and the second candidate gets + /// skipped, leaving `best.offset = 18`. #[test] fn hash_chain_candidate_non_monotonic_walk_accepts_smaller_offset() { // Four 8-byte `"abcdefgh"` chunks, each followed by a unique From 78b1debd1b3b0c84b43a5dbc83cce2442e7e480b Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Fri, 15 May 2026 13:17:01 +0300 Subject: [PATCH 12/12] docs(encoding): explain why bounds-fail skip in HC gate is sound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Annotates the `i_end > history_tail || m_end > history_tail` skip in `hash_chain_candidate` with the soundness proof: under the per-iteration precondition `new_offset >= best.offset`, a new candidate has equal-or-worse `offset_bits` and therefore can only outscore `best` by a strictly larger `match_len`. Bounds-fail is algebraically equivalent to `F_max := history_tail - current_idx ≤ best.match_len - lit_len`, which bounds every candidate at `current_idx` to `match_len ≤ F_max + lit_len ≤ best.match_len` — no possible win. Falling through to `common_prefix_len` would only run a wasted walk. Comment-only — no behavioural change. All 7 HC tests still pass. --- zstd/src/encoding/hc/mod.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/zstd/src/encoding/hc/mod.rs b/zstd/src/encoding/hc/mod.rs index a9d309a6c..fe86b666a 100644 --- a/zstd/src/encoding/hc/mod.rs +++ b/zstd/src/encoding/hc/mod.rs @@ -292,6 +292,19 @@ impl HcMatcher { { let m_end = candidate_idx + tail_off + 4; let i_end = current_idx + tail_off + 4; + // Bounds-fail (`i_end > history_tail`) is a SAFE skip — not + // a missed optimization. Under the per-iteration + // precondition `new_offset >= best.offset`, new candidates + // have equal-or-worse `offset_bits`, so to outscore `best` + // they need strictly *larger* `match_len`. Bounds fail + // ⟺ `current_idx + best.match_len − lit_len + 1 > + // history_tail` ⟺ forward bytes at `current_idx` + // (`F_max := history_tail − current_idx`) satisfy + // `F_max ≤ best.match_len − lit_len`. Any candidate at + // `current_idx` has `match_len ≤ F_max + lit_len ≤ + // best.match_len`, so it cannot strictly outscore. Falling + // through to `common_prefix_len` would only run a wasted + // walk that can never improve `best`. if i_end > history_tail || m_end > history_tail { continue; }