diff --git a/zstd/src/encoding/dfast/mod.rs b/zstd/src/encoding/dfast/mod.rs index 489167e99..9c998a91c 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,92 @@ 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 { + 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, + }; + 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, @@ -712,3 +820,363 @@ 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:?}"), + } + } + + /// 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, indices `0..=31`): + /// `"ABCD????ABCD!??????????ABCDX????"` + /// 01234567890123456789012345678901 (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) + "??????????" (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 + // 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"); + } + + /// 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. + /// + /// 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]. + 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()); + // 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!( + 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() + ); + } +} diff --git a/zstd/src/encoding/hc/mod.rs b/zstd/src/encoding/hc/mod.rs index 3a9e6531f..fe86b666a 100644 --- a/zstd/src/encoding/hc/mod.rs +++ b/zstd/src/encoding/hc/mod.rs @@ -228,11 +228,91 @@ 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 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 — 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; + 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; + } + 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 = @@ -644,4 +724,216 @@ 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 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, indices `0..=39`): + /// `"AAAabcdefZMQabcdefIJBAAAabcdefIJKKKKKKKK"` + /// 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 + /// 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. + /// + /// 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"AAAabcdefZMQabcdefIJBAAAabcdefIJKKKKKKKK".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); + + // `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}" + ); + + // `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!( + 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." + ); + } + + /// 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 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 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 `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 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 + // 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); + // 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 0ad4c29b7..821b53553 100644 --- a/zstd/src/encoding/match_generator.rs +++ b/zstd/src/encoding/match_generator.rs @@ -401,25 +401,83 @@ 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 { + /// 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), + /// 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), + /// Donor `ZSTD_greedy` family with row hashing. Selected for any + /// level that resolves to [`StrategyTag::Greedy`] (currently + /// `Level(4)` only). + Row(RowMatchGenerator), + /// 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)`, 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), +} + +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 +501,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,54 +515,91 @@ 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) { + /// 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 { + 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(); @@ -524,16 +615,20 @@ impl MatchGeneratorDriver { matcher.table.max_window_size.saturating_sub(reclaimed); } } + true } 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); @@ -579,14 +674,27 @@ impl MatchGeneratorDriver { if evicted_bytes == 0 { break; } - self.retire_dictionary_budget(evicted_bytes); + // 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); } } 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 +723,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 +741,89 @@ impl Matcher for MatchGeneratorDriver { suffix_pool.push(suffixes); }); } + 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); + 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 +831,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 +851,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 +864,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 +878,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 +898,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 +928,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 +950,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 +983,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 +997,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 +1021,70 @@ 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); + }); } } + // 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>)) { @@ -1000,9 +1115,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) @@ -1033,7 +1148,15 @@ impl MatchGeneratorDriver { use super::strategy::BackendTag; match S::BACKEND { BackendTag::Simple => { - while self.match_generator.next_sequence(&mut *handle_sequence) {} + // 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), @@ -3661,7 +3784,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 +3825,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 +3841,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 +5043,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 +5107,23 @@ 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 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)); - - // 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 +5140,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 +5155,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 +5243,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 +5336,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 +5349,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" ); @@ -5332,10 +5458,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(); @@ -5345,20 +5481,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 +5507,20 @@ 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); + // `>` 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() ); } -/// 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 +6284,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 +6304,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 +6498,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));