From b8a49e0e0f727bdb3832d0b00ddcec682dfb7895 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 23 May 2026 13:24:11 +0300 Subject: [PATCH 1/3] perf(row): drop eager block-boundary inserts in skip_matching_with_hint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For the default (None) hint, RowMatchGenerator::skip_matching_with_hint previously did a dense insert_positions across the entire skipped block (131K positions for a 128 KiB block). On streams with many block boundaries this dominated Rust self-time at ~25% per the L4 large-log-stream profile (104 MB / 800 blocks = ~104M inserts). Donor zstd_lazy.c:ZSTD_row_fillHashCache only pre-fills the next-scan cache (8 positions of SIMD prefetch lookahead) and does NOT retroactively insert every byte of a skipped block. Future scans cross-match into the skipped block's ROW_HASH_KEY_LEN-1 byte tail (backfilled above) but not its interior. Trade: rare cross-block matches into skipped interiors lost in exchange for O(block_size) → O(0) work per skipped block. The Some(false) dict-priming path keeps the dense insert — donor's ZSTD_loadDictionaryContent + ZSTD_row_update_internalImpl do the same dense fill so subsequent scans against the primed window find matches into the dict. The Some(true) incompressibility-sparse path is unchanged. All 588 tests pass. --- zstd/src/encoding/row/mod.rs | 61 +++++++++++++++++++++++++++--------- 1 file changed, 46 insertions(+), 15 deletions(-) diff --git a/zstd/src/encoding/row/mod.rs b/zstd/src/encoding/row/mod.rs index 1e520cb6b..9e6cdb47f 100644 --- a/zstd/src/encoding/row/mod.rs +++ b/zstd/src/encoding/row/mod.rs @@ -145,23 +145,54 @@ impl RowMatchGenerator { if backfill_start < current_abs_start { self.insert_positions(backfill_start, current_abs_start); } - if incompressible_hint == Some(true) { - self.insert_positions_with_step( - current_abs_start, - current_abs_end, - INCOMPRESSIBLE_SKIP_STEP, - ); - let dense_tail = ROW_MIN_MATCH_LEN + INCOMPRESSIBLE_SKIP_STEP; - let tail_start = current_abs_end - .saturating_sub(dense_tail) - .max(current_abs_start); - for pos in tail_start..current_abs_end { - if !(pos - current_abs_start).is_multiple_of(INCOMPRESSIBLE_SKIP_STEP) { - self.insert_position(pos); + match incompressible_hint { + Some(true) => { + // Sparse step + dense tail: caller declared the block + // unlikely to compress, so we seed only every + // `INCOMPRESSIBLE_SKIP_STEP` position plus a small tail to + // keep cross-block continuity at the boundary. + self.insert_positions_with_step( + current_abs_start, + current_abs_end, + INCOMPRESSIBLE_SKIP_STEP, + ); + let dense_tail = ROW_MIN_MATCH_LEN + INCOMPRESSIBLE_SKIP_STEP; + let tail_start = current_abs_end + .saturating_sub(dense_tail) + .max(current_abs_start); + for pos in tail_start..current_abs_end { + if !(pos - current_abs_start).is_multiple_of(INCOMPRESSIBLE_SKIP_STEP) { + self.insert_position(pos); + } } } - } else { - self.insert_positions(current_abs_start, current_abs_end); + Some(false) => { + // Dictionary priming: the driver explicitly asked for the + // entire skipped range to be queryable by subsequent + // blocks. Donor's `ZSTD_loadDictionaryContent` does the + // same dense fill via `ZSTD_row_update_internalImpl` over + // every dict byte, so future scans against the primed + // window find matches into the dict. Keep dense here. + self.insert_positions(current_abs_start, current_abs_end); + } + None => { + // Donor parity: a plain `skip_matching` (no hint) leaves + // the row table untouched for the skipped range. Donor's + // `ZSTD_row_fillHashCache` only pre-fills the next-scan + // cache (8 positions of lookahead for SIMD prefetch); it + // does NOT retroactively insert every byte of a skipped + // block. Subsequent blocks can only cross-match into the + // ROW_HASH_KEY_LEN-1 byte tail backfilled above. + // + // Trade: cross-block matches into a skipped block's + // interior are lost (rare in practice — `skip_matching` + // is called on blocks the driver upstream identified as + // not worth scanning), but the per-block O(block_size) + // `insert_position` storm is gone. On the L4 large-log- + // stream bench (~104 MB / 800 blocks) the prior dense + // fill dominated ~25% of Rust self-time at 131K inserts + // per block × 800 = ~104M inserts. + } } } From 34af99aee7533bef212d2a13213a9d8aafddaa22 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 23 May 2026 13:51:18 +0300 Subject: [PATCH 2/3] docs(row): clarify backfill scope in skip_matching None arm Comment in skip_matching_with_hint None arm previously said subsequent blocks cross-match "into the ROW_HASH_KEY_LEN-1 byte tail backfilled above", which conflated the PREVIOUS block's tail (backfilled at the current call's entry) with the CURRENT skipped block's tail (backfilled by the NEXT call's own backfill_start insert). Rewrite the comment so the boundary mechanism is spelled out without ambiguity. Also add row_skip_matching_with_none_hint_leaves_interior_empty test: asserts that with block_start=0 (no possible backfill) the None hint leaves the row table fully empty, while the Some(false) dict-priming control still inserts densely. All 589 tests pass. --- zstd/src/encoding/match_generator.rs | 55 ++++++++++++++++++++++++++++ zstd/src/encoding/row/mod.rs | 16 +++++++- 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/zstd/src/encoding/match_generator.rs b/zstd/src/encoding/match_generator.rs index 3c9945a8a..0bbafc98f 100644 --- a/zstd/src/encoding/match_generator.rs +++ b/zstd/src/encoding/match_generator.rs @@ -5781,6 +5781,61 @@ fn row_skip_matching_with_incompressible_hint_uses_sparse_prefix() { ); } +/// Regression for the `None` arm of `skip_matching_with_hint`: the +/// row table must NOT receive dense inserts across the skipped range. +/// Donor parity (`ZSTD_row_fillHashCache` only pre-fills the next-scan +/// cache, not the skipped block's interior) trades cross-block +/// matches into the skipped interior for the per-block O(block_size) +/// insert cost. +/// +/// At input < 1 block (4096 B with default 128 KiB block boundary), +/// the only positions in the row table after the call should be those +/// produced by the `backfill_start` lookback at the block's start +/// (≤ `ROW_HASH_KEY_LEN - 1` positions when block_start < +/// ROW_HASH_KEY_LEN). For `current_abs_start == 0`, even that backfill +/// is empty — so the table stays fully empty. +#[test] +fn row_skip_matching_with_none_hint_leaves_interior_empty() { + let data = deterministic_high_entropy_bytes(0x9B47_F2A1_8C5E_3306, 4096); + + let mut none_hint = RowMatchGenerator::new(1 << 22); + none_hint.configure(ROW_CONFIG); + none_hint.add_data(data.clone(), |_| {}); + none_hint.skip_matching_with_hint(None); + let none_slots = none_hint + .row_positions + .iter() + .filter(|&&pos| pos != ROW_EMPTY_SLOT) + .count(); + + // Dense (Some(false), dict-priming path) for comparison — that + // path inserts every position in the skipped range. + let mut dense = RowMatchGenerator::new(1 << 22); + dense.configure(ROW_CONFIG); + dense.add_data(data, |_| {}); + dense.skip_matching_with_hint(Some(false)); + let dense_slots = dense + .row_positions + .iter() + .filter(|&&pos| pos != ROW_EMPTY_SLOT) + .count(); + + // Two assertions pin the contract: + // 1) None hint is dramatically sparser than dense (the whole point). + // 2) None hint at block-start==0 inserts ZERO positions (no + // backfill possible before position 0). + assert_eq!( + none_slots, 0, + "None hint at block_start=0 must leave row table fully empty \ + (donor parity — interior NOT inserted, no pre-block backfill possible)", + ); + assert!( + dense_slots > 0, + "Some(false) dict-priming path must still insert densely \ + (sanity check: control case for the `none_slots == 0` assertion)", + ); +} + #[test] fn driver_unhinted_level2_keeps_default_dfast_hash_table_size() { let mut driver = MatchGeneratorDriver::new(32, 2); diff --git a/zstd/src/encoding/row/mod.rs b/zstd/src/encoding/row/mod.rs index 9e6cdb47f..c83685b4e 100644 --- a/zstd/src/encoding/row/mod.rs +++ b/zstd/src/encoding/row/mod.rs @@ -181,8 +181,20 @@ impl RowMatchGenerator { // `ZSTD_row_fillHashCache` only pre-fills the next-scan // cache (8 positions of lookahead for SIMD prefetch); it // does NOT retroactively insert every byte of a skipped - // block. Subsequent blocks can only cross-match into the - // ROW_HASH_KEY_LEN-1 byte tail backfilled above. + // block. + // + // Boundary handling: the `backfill_start` insert above + // covers the `ROW_HASH_KEY_LEN - 1` bytes immediately + // BEFORE `current_abs_start` (i.e. the previous block's + // tail), keeping the current block's start hashable as + // a cross-block match target. The CURRENT skipped + // block's tail (the `ROW_HASH_KEY_LEN - 1` bytes ending + // at `current_abs_end`) is itself backfilled lazily — + // by the NEXT call's own `backfill_start` insert when + // that call's `current_abs_start` lands at + // `current_abs_end`. So a parse of block N+1 sees + // block N's tail in the row table but not its + // interior, matching donor. // // Trade: cross-block matches into a skipped block's // interior are lost (rare in practice — `skip_matching` From 723713081e04a7ede9d77ae0ac69af10eaeff579 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sat, 23 May 2026 15:35:52 +0300 Subject: [PATCH 3/3] docs(row): reword Some(false) arm to describe dense-seed semantic --- zstd/src/encoding/row/mod.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/zstd/src/encoding/row/mod.rs b/zstd/src/encoding/row/mod.rs index c83685b4e..73771bf6b 100644 --- a/zstd/src/encoding/row/mod.rs +++ b/zstd/src/encoding/row/mod.rs @@ -167,12 +167,16 @@ impl RowMatchGenerator { } } Some(false) => { - // Dictionary priming: the driver explicitly asked for the - // entire skipped range to be queryable by subsequent - // blocks. Donor's `ZSTD_loadDictionaryContent` does the - // same dense fill via `ZSTD_row_update_internalImpl` over - // every dict byte, so future scans against the primed - // window find matches into the dict. Keep dense here. + // Dense seeding requested by the caller: the entire + // skipped range must remain queryable so subsequent + // blocks can match into it. Currently only used by the + // dictionary-priming path (donor's + // `ZSTD_loadDictionaryContent` does the same dense fill + // via `ZSTD_row_update_internalImpl` over every dict + // byte), but the semantic is "dense fill on demand" and + // future fast-paths (e.g. an RLE / raw-block emitter + // that still wants cross-block matches into the skipped + // bytes) can reuse it without rewording the contract. self.insert_positions(current_abs_start, current_abs_end); } None => {