From bae47ceb794766882fe547839fc62e5b770b80ea Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 30 Jun 2026 16:59:57 +0300 Subject: [PATCH 1/6] refactor(opt): drop the non-upstream hash-chain optimal fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The optimal parser's per-position match-finder always used the binary-tree branch (every production call site selects it): upstream zstd's optimal parser (ZSTD_insertBtAndGetAllMatches) is binary-tree only, with no hash-chain variant — the chain is upstream's lazy parser (ZSTD_HcFindBestMatch), a different path. The else-branch hash-chain optimal walk and its chain_candidates helper had no upstream counterpart and were dead in every shipped binary (const-folded out, absent from objdump). Remove the dead else-branch, the chain_candidates helper, and the two tests that exercised the chain optimal path through the test-only dispatcher; the BT path's own skip-window / rebase coverage remains. Byte-identical. --- zstd/src/encoding/hc/hc_tests.rs | 35 ----- zstd/src/encoding/hc/mod.rs | 75 ----------- zstd/src/encoding/hc/optimal.rs | 144 +-------------------- zstd/src/encoding/match_generator/tests.rs | 67 ---------- 4 files changed, 7 insertions(+), 314 deletions(-) diff --git a/zstd/src/encoding/hc/hc_tests.rs b/zstd/src/encoding/hc/hc_tests.rs index 687396b83..45df588cb 100644 --- a/zstd/src/encoding/hc/hc_tests.rs +++ b/zstd/src/encoding/hc/hc_tests.rs @@ -21,41 +21,6 @@ fn table_with_history(buf: &[u8]) -> MatchTable { t } -#[test] -fn chain_candidates_returns_sentinels_when_suffix_too_short() { - let hc = HcMatcher::new(2, 4, 32); - // History exactly at min-prefix - 1 → idx + 4 > concat.len() → - // early return with all-sentinel buffer. - let t = table_with_history(b"abc"); - let buf = hc.chain_candidates(&t, 0); - assert!(buf.iter().all(|&v| v == usize::MAX)); -} - -#[test] -fn chain_candidates_terminates_on_self_loop_with_in_range_pick() { - // Construct a self-loop in the chain: hash_table → cur, - // chain_table[cur_rel] = cur (points back to itself). The walker - // must pick the position (in-range) and stop. - let mut hc = HcMatcher::new(2, 4, 32); - hc.search_depth = 4; - let mut t = table_with_history(b"abcdef_abcdef_abcdef"); - let abs_pos = 10usize; - // The walker hashes the suffix at `abs_pos`, not the prefix at 0. - let concat = t.live_history(); - let hash = t.hash_position(&concat[abs_pos..]); - // Stored = relative + 1 → stored=6 means candidate_rel=5. - t.hash_table[hash] = 6; - let chain_mask = (1 << t.chain_log) - 1; - t.chain_table[5 & chain_mask] = 6; // self-loop - - let buf = hc.chain_candidates(&t, abs_pos); - assert_eq!( - buf[0], 5, - "self-loop pick must surface the in-range candidate" - ); - assert_eq!(buf[1], usize::MAX, "walker must stop after self-loop"); -} - #[test] fn repcode_candidate_returns_none_when_suffix_too_short() { let mut t = table_with_history(b"abc"); diff --git a/zstd/src/encoding/hc/mod.rs b/zstd/src/encoding/hc/mod.rs index 588b75477..0f2f71404 100644 --- a/zstd/src/encoding/hc/mod.rs +++ b/zstd/src/encoding/hc/mod.rs @@ -123,81 +123,6 @@ impl HcMatcher { } } - /// Walk the hash chain at `abs_pos` and collect up to - /// [`HcMatcher::search_depth`] absolute positions of in-window - /// candidates. Stale chain entries (positions evicted from the - /// window) are skipped rather than terminating the walk; the - /// chain is bounded by `search_depth` total iterations to keep - /// pathological self-loops from spinning. - pub(crate) fn chain_candidates( - &self, - table: &MatchTable, - abs_pos: usize, - ) -> [usize; MAX_HC_SEARCH_DEPTH] { - let mut buf = [usize::MAX; MAX_HC_SEARCH_DEPTH]; - let idx = abs_pos - table.history_abs_start; - let concat = table.live_history(); - if idx + 4 > concat.len() { - return buf; - } - let hash = table.hash_position(&concat[idx..]); - let chain_mask = (1 << table.chain_log) - 1; - - let mut cur = table.hash_table[hash]; - let mut filled = 0; - let mut steps = 0; - // Cap both the loop bound and the result-fill bound at - // MAX_HC_SEARCH_DEPTH so a misconfigured `search_depth > - // MAX_HC_SEARCH_DEPTH` (BT modes set it from the upstream zstd config, - // which can exceed 64) cannot index past `buf`'s fixed size. - let max_chain_steps = self.search_depth.min(MAX_HC_SEARCH_DEPTH); - while filled < max_chain_steps && steps < max_chain_steps { - if cur == HC_EMPTY { - break; - } - let candidate_rel = cur.wrapping_sub(1) as usize; - // Decode through `stored_abs_position_fast` so a non-zero - // `index_shift` (set by future rebase variants) is honored; - // raw `position_base + candidate_rel` would silently - // misread rebased entries. - let candidate_abs = super::match_table::storage::MatchTable::stored_abs_position_fast( - cur, - table.position_base, - table.index_shift, - ); - let next = table.chain_table[candidate_rel & chain_mask]; - steps += 1; - if next == cur { - // Self-loop: two positions share chain_idx, stop to - // avoid spinning on the same candidate forever. - if let Some(candidate_abs) = candidate_abs.filter(|&p| { - p >= table - .history_abs_start - .max(abs_pos.saturating_sub(table.max_window_size)) - && p < abs_pos - }) { - buf[filled] = candidate_abs; - } - break; - } - cur = next; - let Some(candidate_abs) = candidate_abs else { - continue; - }; - if candidate_abs - < table - .history_abs_start - .max(abs_pos.saturating_sub(table.max_window_size)) - || candidate_abs >= abs_pos - { - continue; - } - buf[filled] = candidate_abs; - filled += 1; - } - buf - } - /// Probe the 3 rep-code offsets (with the upstream zstd `ll0 ↦ rep[0] − 1` /// fallback) and return the best in-range match. Pure helper — /// only reads from `MatchTable`, no HcMatcher state needed. diff --git a/zstd/src/encoding/hc/optimal.rs b/zstd/src/encoding/hc/optimal.rs index c1c60a4bb..fbcacd19f 100644 --- a/zstd/src/encoding/hc/optimal.rs +++ b/zstd/src/encoding/hc/optimal.rs @@ -1148,7 +1148,7 @@ macro_rules! collect_optimal_candidates_initialized_body { } return; } - if $bt_matchfinder { + { // BT tree catch-up folded inline (was a per-position call to // bt_update_tree_until): insert the positions the parser skipped into // the binary tree before this position's search. Upstream zstd @@ -1191,17 +1191,12 @@ macro_rules! collect_optimal_candidates_initialized_body { return; } let mut best_len_for_skip = 0usize; - if $bt_matchfinder { - // BT path: the rep-code probe and the hash3 short-match probe are - // folded INTO bt_insert_and_collect (one out-of-line per-position - // match-finder, upstream ZSTD_btGetAllMatches shape). They seed - // `best_len_for_skip` and handle the sufficient-match early-out - // internally, byte-identically to the prior orchestration. - // SAFETY: same umbrella for bt_insert_and_collect_matches. - // Inline the BT find monolith here instead of an out-of-line call. - // Upstream's ZSTD_insertBtAndGetAllMatches is FORCE_INLINE_TEMPLATE - // inlined into the opt loop, so the per-position match-finder is one - // body with no call-boundary marshalling (profile / reps / ...). + { + // Per-position match-finder: the rep-code probe, the hash3 + // short-match probe and the binary-tree walk folded into one body + // (upstream ZSTD_insertBtAndGetAllMatches, FORCE_INLINE_TEMPLATE into + // the opt loop). Seeds `best_len_for_skip` and handles the + // sufficient-match early-out internally. let bt_search_depth = $self.table.search_depth; let best_len_ref = &mut best_len_for_skip; crate::encoding::hc::generator::bt_insert_and_collect_matches_body!( @@ -1219,131 +1214,6 @@ macro_rules! collect_optimal_candidates_initialized_body { $cpl, $cmf, ); - } else { - // HC-chain optimal fallback (no BT tree): the rep + hash3 probes - // stay inline here, ahead of the chain walk. - let mut skip_further_match_search = false; - let mut rep_len_candidate_found = false; - // SAFETY: same umbrella; closure capture is monomorphized per call. - unsafe { - $self.hc.$for_each_rep( - &$self.table, - $abs_pos, - lit_len, - reps, - $current_abs_end, - min_match_len, - |rep| { - if rep.match_len >= min_match_len { - rep_len_candidate_found = true; - } - let _ = crate::encoding::bt::BtMatcher::push_candidate_ladder( - $out, - &mut best_len_for_skip, - rep, - min_match_len, - ); - if rep.match_len > $profile.sufficient_match_len { - skip_further_match_search = true; - } - if $abs_pos + rep.match_len >= $current_abs_end { - skip_further_match_search = true; - } - }, - ) - }; - if use_hash3 && !skip_further_match_search && best_len_for_skip < min_match_len { - $self.table.update_hash3_until($abs_pos); - // SAFETY: same umbrella for hash3_candidate. - if let Some(h3) = unsafe { - $self - .table - .$hash3($abs_pos, $current_abs_end, min_match_len) - } { - let _ = crate::encoding::bt::BtMatcher::push_candidate_ladder( - $out, - &mut best_len_for_skip, - h3, - min_match_len, - ); - if !rep_len_candidate_found - && (h3.match_len > $profile.sufficient_match_len - || $abs_pos + h3.match_len >= $current_abs_end) - { - $self.table.skip_insert_until_abs = $abs_pos + 1; - skip_further_match_search = true; - } - } - } - if !skip_further_match_search { - $self.table.insert_position($abs_pos); - let max_chain_depth = $profile.max_chain_depth.min($self.hc.search_depth); - let concat = $self.table.live_history(); - // Raw `+ 9` is safe here — see `bt_insert_step_no_rebase_body!` - // for the full discussion of the upstream `STREAM_ABS_HEADROOM` - // cap in `MatchTable::add_data`. - let mut match_end_abs = $abs_pos + 9; - if max_chain_depth > 0 { - for (visited, candidate_abs) in $self - .hc - .chain_candidates(&$self.table, $abs_pos) - .into_iter() - .enumerate() - { - if visited >= max_chain_depth { - break; - } - if candidate_abs == usize::MAX { - break; - } - if candidate_abs < $self.table.window_low_abs_for_target($abs_pos) - || candidate_abs >= $abs_pos - { - continue; - } - let candidate_idx = candidate_abs - $self.table.history_abs_start; - debug_assert!( - $abs_pos <= $current_abs_end, - "HC chain walker called past current block end" - ); - let tail_limit = $current_abs_end - $abs_pos; - let base = concat.as_ptr(); - // SAFETY: history-relative indices; `tail_limit` bounds - // the scan within `concat`. `$cpl` is the kernel-specific - // common_prefix_len_ptr — call inlines because the - // surrounding wrapper carries the same target_feature. - let match_len = unsafe { - $cpl(base.add(candidate_idx), base.add(current_idx), tail_limit) - }; - if match_len < min_match_len { - continue; - } - let offset = $abs_pos - candidate_abs; - if crate::encoding::bt::BtMatcher::push_candidate_ladder( - $out, - &mut best_len_for_skip, - MatchCandidate { - start: $abs_pos, - offset, - match_len, - }, - min_match_len, - ) { - let candidate_end = candidate_abs + match_len; - if candidate_end > match_end_abs { - match_end_abs = candidate_end; - } - } - if match_len > HC_OPT_NUM || $abs_pos + match_len >= $current_abs_end { - break; - } - } - } - // `match_end_abs` initialized to `abs_pos + 9`; monotonic - // updates only ever extend it, so `match_end_abs - 8 >= 1`. - $self.table.skip_insert_until_abs = - $self.table.skip_insert_until_abs.max(match_end_abs - 8); - } } if let Some(ldm) = ldm_candidate { let _ = crate::encoding::bt::BtMatcher::push_candidate_ladder( diff --git a/zstd/src/encoding/match_generator/tests.rs b/zstd/src/encoding/match_generator/tests.rs index a011595ac..bfe733d83 100644 --- a/zstd/src/encoding/match_generator/tests.rs +++ b/zstd/src/encoding/match_generator/tests.rs @@ -1387,53 +1387,6 @@ fn hc_collect_optimal_candidates_long_chain_match_advances_skip_window() { ); } -#[test] -fn hc_collect_optimal_candidates_chain_fast_skip_uses_match_end_minus_8() { - let mut hc = HcMatchGenerator::new(128); - hc.table.history = b"abcabcabcabcabcabcabcabc".to_vec(); - hc.table.history_start = 0; - hc.table.history_abs_start = 0; - hc.table.position_base = 0; - hc.hc.search_depth = 32; - let abs_pos = 9usize; - hc.table.ensure_tables(); - hc.table.insert_positions(0, abs_pos); - hc.table.skip_insert_until_abs = 0; - - let profile = HcOptimalCostProfile { - max_chain_depth: 32, - sufficient_match_len: 10, - accurate: true, - favor_small_offsets: false, - }; - let mut out = Vec::new(); - hc.collect_optimal_candidates( - abs_pos, - hc.table.history.len(), - profile, - HcCandidateQuery { - reps: [1, 4, 8], - lit_len: 1, - ldm_candidate: None, - }, - &mut out, - ); - - let best_match_end = out - .iter() - .map(|candidate| candidate.start.saturating_add(candidate.match_len)) - .max() - .expect("expected at least one candidate"); - assert!( - hc.table.skip_insert_until_abs > abs_pos, - "chain fast-skip must advance past current position" - ); - assert!( - hc.table.skip_insert_until_abs <= best_match_end.saturating_sub(8), - "chain fast-skip must not exceed upstream zstd-style matchEndIdx - 8 bound" - ); -} - #[test] fn hc_collect_optimal_candidates_advances_skip_window_on_plain_bt_path() { let mut hc = HcMatchGenerator::new(256); @@ -3230,18 +3183,6 @@ fn row_candidate_returns_none_when_abs_pos_near_end_of_history() { assert!(matcher.row_candidate(0, 0).is_none()); } -#[test] -fn hc_chain_candidates_returns_sentinels_for_short_suffix() { - let mut hc = HcMatchGenerator::new(32); - hc.table.history = b"abc".to_vec(); - hc.table.history_start = 0; - hc.table.history_abs_start = 0; - hc.table.ensure_tables(); - - let candidates = hc.hc.chain_candidates(&hc.table, 0); - assert!(candidates.iter().all(|&pos| pos == usize::MAX)); -} - #[test] fn hc_reset_advances_floor_past_prior_frame_entries() { use super::super::match_table::storage::MatchTable; @@ -3787,14 +3728,6 @@ fn hc_rebases_positions_after_u32_boundary() { .any(|entry| *entry != HC_EMPTY), "HC hash table should still be populated after crossing u32 boundary" ); - - // Verify rebasing preserves candidate lookup, not just table population. - let abs_pos = matcher.table.history_abs_start + 10; - let candidates = matcher.hc.chain_candidates(&matcher.table, abs_pos); - assert!( - candidates.iter().any(|candidate| *candidate != usize::MAX), - "chain_candidates should return valid matches after rebase" - ); } // 64-bit only: the >4 GiB absolute cursor this test fabricates cannot exist on From 17c5ec409828418a0108a5ba93597136a73b3505 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 30 Jun 2026 17:06:13 +0300 Subject: [PATCH 2/6] refactor(opt): drop the now-vestigial USE_BT_MATCHFINDER const generic With the hash-chain optimal fallback gone, the per-position match-finder is unconditionally the binary-tree walk, so the USE_BT_MATCHFINDER const generic (and the macro metavars that only fed the removed chain branch: bt_insert_and_collect method handle, for_each_repcode, hash3) carried no information. Remove the const generic from the collect entry points and the four kernel wrappers, and drop the dead macro parameters. Byte-identical. --- zstd/src/encoding/hc/optimal.rs | 81 +++++++++++---------------------- 1 file changed, 26 insertions(+), 55 deletions(-) diff --git a/zstd/src/encoding/hc/optimal.rs b/zstd/src/encoding/hc/optimal.rs index fbcacd19f..6a73188c3 100644 --- a/zstd/src/encoding/hc/optimal.rs +++ b/zstd/src/encoding/hc/optimal.rs @@ -157,7 +157,7 @@ macro_rules! start_matching_btlazy2_body { // SAFETY: called inside the wrapper's `#[target_feature]` // umbrella (the scalar wrapper's `$collect` is a safe fn). unsafe { - $self.$collect::( + $self.$collect::( sel_abs, current_abs_end, profile, @@ -419,7 +419,7 @@ macro_rules! build_optimal_plan_impl_body { // `$collect` kernel variant; the runtime kernel detector already // gated entry into the wrapper. unsafe { - $self.$collect::<$strategy_ty, true>( + $self.$collect::<$strategy_ty>( $current_abs_start, current_abs_end, profile, @@ -776,7 +776,7 @@ macro_rules! build_optimal_plan_impl_body { // stay live across the call; the post-call reads below are a // separate, fresh load of the same stable `nodes[pos]`. unsafe { - $self.$collect::<$strategy_ty, true>( + $self.$collect::<$strategy_ty>( abs_pos, current_abs_end, profile, @@ -1107,11 +1107,7 @@ macro_rules! collect_optimal_candidates_initialized_body { $profile:ident, $query:ident, $out:ident, - $bt_matchfinder:ident, $bt_insert_step:ident, - $bt_insert:ident, - $for_each_rep:ident, - $hash3:ident, $cpl:path, $cmf:path $(,)? ) => {{ @@ -2015,7 +2011,7 @@ impl HcMatchGenerator { // allowed. match self.strategy_tag { StrategyTag::BtUltra2 => self - .collect_optimal_candidates_initialized::( + .collect_optimal_candidates_initialized::( abs_pos, current_abs_end, profile, @@ -2023,7 +2019,7 @@ impl HcMatchGenerator { out, ), StrategyTag::BtUltra => self - .collect_optimal_candidates_initialized::( + .collect_optimal_candidates_initialized::( abs_pos, current_abs_end, profile, @@ -2031,23 +2027,22 @@ impl HcMatchGenerator { out, ), StrategyTag::Btlazy2 => self - .collect_optimal_candidates_initialized::( - abs_pos, - current_abs_end, - profile, - query, - out, - ), - StrategyTag::BtOpt => self - .collect_optimal_candidates_initialized::( + .collect_optimal_candidates_initialized::( abs_pos, current_abs_end, profile, query, out, ), + StrategyTag::BtOpt => self.collect_optimal_candidates_initialized::( + abs_pos, + current_abs_end, + profile, + query, + out, + ), StrategyTag::Fast | StrategyTag::Dfast | StrategyTag::Greedy | StrategyTag::Lazy => { - self.collect_optimal_candidates_initialized::( + self.collect_optimal_candidates_initialized::( abs_pos, current_abs_end, profile, @@ -2069,10 +2064,7 @@ impl HcMatchGenerator { /// future caller that isn't already inside a kernel umbrella. #[allow(dead_code)] #[inline(always)] - pub(crate) fn collect_optimal_candidates_initialized< - S: crate::encoding::strategy::Strategy, - const USE_BT_MATCHFINDER: bool, - >( + pub(crate) fn collect_optimal_candidates_initialized( &mut self, abs_pos: usize, current_abs_end: usize, @@ -2082,7 +2074,7 @@ impl HcMatchGenerator { ) { #[cfg(all(target_arch = "aarch64", target_endian = "little"))] unsafe { - self.collect_optimal_candidates_initialized_neon::( + self.collect_optimal_candidates_initialized_neon::( abs_pos, current_abs_end, profile, @@ -2095,7 +2087,7 @@ impl HcMatchGenerator { use crate::encoding::fastpath::FastpathKernel; match self.table.kernel { FastpathKernel::Avx2Bmi2 => unsafe { - self.collect_optimal_candidates_initialized_avx2_bmi2::( + self.collect_optimal_candidates_initialized_avx2_bmi2::( abs_pos, current_abs_end, profile, @@ -2104,7 +2096,7 @@ impl HcMatchGenerator { ) }, FastpathKernel::Sse42 => unsafe { - self.collect_optimal_candidates_initialized_sse42::( + self.collect_optimal_candidates_initialized_sse42::( abs_pos, current_abs_end, profile, @@ -2112,14 +2104,13 @@ impl HcMatchGenerator { out, ) }, - FastpathKernel::Scalar => self - .collect_optimal_candidates_initialized_scalar::( - abs_pos, - current_abs_end, - profile, - query, - out, - ), + FastpathKernel::Scalar => self.collect_optimal_candidates_initialized_scalar::( + abs_pos, + current_abs_end, + profile, + query, + out, + ), } } #[cfg(not(any( @@ -2128,7 +2119,7 @@ impl HcMatchGenerator { target_arch = "x86_64" )))] { - self.collect_optimal_candidates_initialized_scalar::( + self.collect_optimal_candidates_initialized_scalar::( abs_pos, current_abs_end, profile, @@ -2147,7 +2138,6 @@ impl HcMatchGenerator { #[target_feature(enable = "neon")] unsafe fn collect_optimal_candidates_initialized_neon< S: crate::encoding::strategy::Strategy, - const USE_BT_MATCHFINDER: bool, >( &mut self, abs_pos: usize, @@ -2164,11 +2154,7 @@ impl HcMatchGenerator { profile, query, out, - USE_BT_MATCHFINDER, bt_insert_step_no_rebase_neon, - bt_insert_and_collect_matches_neon, - for_each_repcode_candidate_with_reps_neon, - hash3_candidate_neon, crate::encoding::fastpath::neon::common_prefix_len_ptr, crate::encoding::fastpath::neon::count_match_from_indices, ) @@ -2178,7 +2164,6 @@ impl HcMatchGenerator { #[target_feature(enable = "sse4.2")] unsafe fn collect_optimal_candidates_initialized_sse42< S: crate::encoding::strategy::Strategy, - const USE_BT_MATCHFINDER: bool, >( &mut self, abs_pos: usize, @@ -2195,11 +2180,7 @@ impl HcMatchGenerator { profile, query, out, - USE_BT_MATCHFINDER, bt_insert_step_no_rebase_sse42, - bt_insert_and_collect_matches_sse42, - for_each_repcode_candidate_with_reps_sse42, - hash3_candidate_sse42, crate::encoding::fastpath::sse42::common_prefix_len_ptr, crate::encoding::fastpath::sse42::count_match_from_indices, ) @@ -2209,7 +2190,6 @@ impl HcMatchGenerator { #[target_feature(enable = "avx2,bmi2")] unsafe fn collect_optimal_candidates_initialized_avx2_bmi2< S: crate::encoding::strategy::Strategy, - const USE_BT_MATCHFINDER: bool, >( &mut self, abs_pos: usize, @@ -2226,11 +2206,7 @@ impl HcMatchGenerator { profile, query, out, - USE_BT_MATCHFINDER, bt_insert_step_no_rebase_avx2_bmi2, - bt_insert_and_collect_matches_avx2_bmi2, - for_each_repcode_candidate_with_reps_avx2_bmi2, - hash3_candidate_avx2_bmi2, crate::encoding::fastpath::avx2_bmi2::common_prefix_len_ptr, crate::encoding::fastpath::avx2_bmi2::count_match_from_indices, ) @@ -2242,7 +2218,6 @@ impl HcMatchGenerator { #[allow(unused_unsafe)] pub(crate) fn collect_optimal_candidates_initialized_scalar< S: crate::encoding::strategy::Strategy, - const USE_BT_MATCHFINDER: bool, >( &mut self, abs_pos: usize, @@ -2259,11 +2234,7 @@ impl HcMatchGenerator { profile, query, out, - USE_BT_MATCHFINDER, bt_insert_step_no_rebase_scalar, - bt_insert_and_collect_matches_scalar, - for_each_repcode_candidate_with_reps_scalar, - hash3_candidate_scalar, crate::encoding::fastpath::scalar::common_prefix_len_ptr, crate::encoding::fastpath::scalar::count_match_from_indices, ) From faad79b65b53ae3c5fd56da8b45d1eab5de15309 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 30 Jun 2026 17:15:33 +0300 Subject: [PATCH 3/6] perf(opt): fold per-node coordinate decode in the BT walk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The BT match-finder's hot per-node loop decoded the chain entry into three coordinates each iteration: candidate_abs (position_base + stored - 1 - index_shift), the BT pair slot (via bt_pair_index_for_abs, which re-read index_shift and bt_mask and round-tripped index_shift back in), and candidate_idx (candidate_abs - history_abs_start). That reloaded four struct fields and ran ~6 arithmetic ops per node. Precompute the three loop-invariant biases once before the walk so each coordinate is a single wrapping_add from the stored entry — the single-coordinate form of upstream zstd's window-relative matchIndex (match = base + matchIndex, slot = 2*(matchIndex & btMask)). Byte-identical: the folded values equal the originals for every gate-validated entry. --- zstd/src/encoding/hc/generator.rs | 34 ++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/zstd/src/encoding/hc/generator.rs b/zstd/src/encoding/hc/generator.rs index 26d3e4faf..7673c8549 100644 --- a/zstd/src/encoding/hc/generator.rs +++ b/zstd/src/encoding/hc/generator.rs @@ -733,6 +733,24 @@ macro_rules! bt_insert_and_collect_matches_body { .wrapping_sub($table.index_shift) .wrapping_sub(window_low); let win_range = $abs_pos - window_low; + // Decode biases: fold the per-node coordinate conversions into + // loop-invariant additions. The gate-validated chain entry + // `match_stored` maps to its absolute position, its history-relative + // index and its BT pair slot by a single add each, instead of + // re-reading `position_base` / `index_shift` / `history_abs_start` / + // `bt_mask` from `self` and round-tripping `index_shift` through the + // slot computation on every node. Upstream zstd walks one + // window-relative `matchIndex` directly (`match = base + matchIndex`, + // slot `2*(matchIndex & btMask)`); these biases are the + // single-coordinate equivalent. Wrapping throughout: the window gate + // already proved `match_stored ∈ [window_low, abs_pos)` before decode, + // mirroring the `win_off` form above. + let abs_bias = $table + .position_base + .wrapping_sub(1) + .wrapping_sub($table.index_shift); + let idx_bias = abs_bias.wrapping_sub($table.history_abs_start); + let bt_bias = $table.position_base.wrapping_sub(1); // Raw `+ 9` is safe here — see `bt_insert_step_no_rebase_body!` // for the full discussion of the upstream `STREAM_ABS_HEADROOM` // cap in `MatchTable::add_data`. @@ -763,19 +781,21 @@ macro_rules! bt_insert_and_collect_matches_body { while compares_left > 0 && (match_stored as usize).wrapping_add(win_off) < win_range { compares_left -= 1; // The condition proved this candidate is in `[window_low, - // abs_pos)`, so `match_stored >= 1` (HC_EMPTY is out of range) and - // the `- 1` cannot underflow; candidate_abs == base + match_stored. - let candidate_abs = ($table.position_base + (match_stored as usize - 1)) - .wrapping_sub($table.index_shift); - - let next_pair_idx = $table.bt_pair_index_for_abs(candidate_abs); + // abs_pos)`, so `match_stored >= 1` (HC_EMPTY is out of range). + // Decode via the precomputed biases (single add each), the + // single-coordinate form of upstream's `matchIndex`. + let stored = match_stored as usize; + let candidate_abs = stored.wrapping_add(abs_bias); + // `2*(candidate_abs + index_shift & bt_mask)` with `index_shift` + // folded away: `candidate_abs + index_shift == stored + bt_bias`. + let next_pair_idx = 2 * (stored.wrapping_add(bt_bias) & bt_mask); // SAFETY: `next_pair_idx (+1)` = `2*(candidate_abs & bt_mask) (+1)` // ≤ `chain_table.len()-1`; `chain_ptr` is the hoisted live base, // table not realloc'd during the walk. let next_smaller = unsafe { *chain_ptr.add(next_pair_idx) }; let next_larger = unsafe { *chain_ptr.add(next_pair_idx + 1) }; let seed_len = common_length_smaller.min(common_length_larger); - let candidate_idx = candidate_abs - $table.history_abs_start; + let candidate_idx = stored.wrapping_add(idx_bias); // SAFETY: BT walk invariant — `candidate_idx + tail_limit ≤ // concat.len()`. let match_len = unsafe { $cmf(concat, idx, candidate_idx, tail_limit, seed_len) }; From e13b3eda1e85ce5aaaa2345bcb9caddb5322f596 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 30 Jun 2026 23:48:26 +0300 Subject: [PATCH 4/6] refactor(opt): enforce binary-tree-only contract on the optimal collect The optimal candidate collector is binary-tree only; the Fast / Dfast / Greedy / Lazy strategies never run the optimal parser (they drive their own match finders) and keep chain_table as an HC chain, not BT pair slots. The public dispatcher's non-BT arm previously routed those tags into the BT collect, which would walk the HC chain as a binary tree. Make that arm unreachable so misuse fails loudly, and tag the test-only shim callers as a BT strategy (BtOpt shares Lazy's OPT_LEVEL=0 / USE_HASH3=false consts, so the collect behavior is unchanged). No production caller hit the non-BT arm (the on-encode path goes through build_optimal_plan_impl directly). --- zstd/src/encoding/hc/optimal.rs | 15 +++++++++------ zstd/src/encoding/match_generator/tests.rs | 7 +++++++ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/zstd/src/encoding/hc/optimal.rs b/zstd/src/encoding/hc/optimal.rs index 6a73188c3..5a3d879ee 100644 --- a/zstd/src/encoding/hc/optimal.rs +++ b/zstd/src/encoding/hc/optimal.rs @@ -2042,12 +2042,15 @@ impl HcMatchGenerator { out, ), StrategyTag::Fast | StrategyTag::Dfast | StrategyTag::Greedy | StrategyTag::Lazy => { - self.collect_optimal_candidates_initialized::( - abs_pos, - current_abs_end, - profile, - query, - out, + // Optimal candidate collection is binary-tree only (btopt / + // btultra / btultra2 / btlazy2). The Fast / Dfast / Greedy / Lazy + // strategies never run the optimal parser — they drive their own + // match finders — and their generators keep `chain_table` as an + // HC chain, not BT pair slots. Routing them here would walk that + // chain as a binary tree. Reaching this arm is a caller bug. + unreachable!( + "collect_optimal_candidates is binary-tree only; \ + non-BT strategies use their own match finder" ) } } diff --git a/zstd/src/encoding/match_generator/tests.rs b/zstd/src/encoding/match_generator/tests.rs index bfe733d83..4a8240f83 100644 --- a/zstd/src/encoding/match_generator/tests.rs +++ b/zstd/src/encoding/match_generator/tests.rs @@ -1276,6 +1276,9 @@ fn hc_repcode_candidates_respect_litlen_dependent_rep_order() { #[test] fn hc_collect_optimal_candidates_keeps_reps_when_chain_depth_zero() { let mut hc = HcMatchGenerator::new(64); + // Optimal candidate collection is binary-tree only; tag the generator as a + // BT strategy (BtOpt shares Lazy's OPT_LEVEL=0 / USE_HASH3=false consts). + hc.strategy_tag = crate::encoding::strategy::StrategyTag::BtOpt; hc.hc.search_depth = 0; hc.table.history = b"xyzxyzxyzxyz".to_vec(); hc.table.history_start = 0; @@ -1314,6 +1317,7 @@ fn hc_collect_optimal_candidates_keeps_reps_when_chain_depth_zero() { #[test] fn hc_collect_optimal_candidates_rep_tail_match_skips_chain_probe() { let mut hc = HcMatchGenerator::new(64); + hc.strategy_tag = crate::encoding::strategy::StrategyTag::BtOpt; hc.table.history = b"aaaaaaaaaa".to_vec(); hc.table.history_start = 0; hc.table.history_abs_start = 0; @@ -1352,6 +1356,7 @@ fn hc_collect_optimal_candidates_rep_tail_match_skips_chain_probe() { #[test] fn hc_collect_optimal_candidates_long_chain_match_advances_skip_window() { let mut hc = HcMatchGenerator::new(128); + hc.strategy_tag = crate::encoding::strategy::StrategyTag::BtOpt; hc.table.history = b"abcabcabcabcabcabcabcabc".to_vec(); hc.table.history_start = 0; hc.table.history_abs_start = 0; @@ -1390,6 +1395,7 @@ fn hc_collect_optimal_candidates_long_chain_match_advances_skip_window() { #[test] fn hc_collect_optimal_candidates_advances_skip_window_on_plain_bt_path() { let mut hc = HcMatchGenerator::new(256); + hc.strategy_tag = crate::encoding::strategy::StrategyTag::BtOpt; hc.table.history = b"abcdefghijklmnop".to_vec(); hc.table.history_start = 0; hc.table.history_abs_start = 0; @@ -1441,6 +1447,7 @@ fn hc_collect_optimal_candidates_advances_skip_window_on_plain_bt_path() { #[test] fn hc_ldm_candidates_are_merged_into_optimal_candidates() { let mut hc = HcMatchGenerator::new(512); + hc.strategy_tag = crate::encoding::strategy::StrategyTag::BtOpt; hc.table.history = (0..256).map(|i| (i % 251) as u8).collect(); hc.table.history_start = 0; hc.table.history_abs_start = 0; From 5ef5c04d3c03dfad8722f5511951234332c1d2f0 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 1 Jul 2026 00:39:34 +0300 Subject: [PATCH 5/6] test(opt): cover the optimal-collect dispatcher arms + BT-only contract Adds a should_panic test proving a non-BT strategy tag reaching collect_optimal_candidates panics (the binary-tree-only contract), covering the unreachable arm, and a dispatch test exercising every BT tag (BtOpt / BtUltra / BtUltra2 / Btlazy2) under the scalar kernel so the dispatcher's per-tag and scalar arms are covered. --- zstd/src/encoding/match_generator/tests.rs | 77 ++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/zstd/src/encoding/match_generator/tests.rs b/zstd/src/encoding/match_generator/tests.rs index 4a8240f83..71a2f1bee 100644 --- a/zstd/src/encoding/match_generator/tests.rs +++ b/zstd/src/encoding/match_generator/tests.rs @@ -1314,6 +1314,83 @@ fn hc_collect_optimal_candidates_keeps_reps_when_chain_depth_zero() { ); } +#[test] +#[should_panic(expected = "binary-tree only")] +fn hc_collect_optimal_candidates_panics_for_non_bt_strategy() { + // Optimal candidate collection is binary-tree only. A non-BT strategy tag + // (Lazy here) reaching the public dispatcher is a caller bug — it must panic + // rather than walk the HC chain_table as BT pair slots. + let mut hc = HcMatchGenerator::new(64); + hc.strategy_tag = crate::encoding::strategy::StrategyTag::Lazy; + hc.table.history = b"abcabcabcabc".to_vec(); + hc.table.history_start = 0; + hc.table.history_abs_start = 0; + hc.table.ensure_tables(); + let profile = HcOptimalCostProfile { + max_chain_depth: 0, + sufficient_match_len: usize::MAX / 2, + accurate: false, + favor_small_offsets: false, + }; + let mut out = Vec::new(); + hc.collect_optimal_candidates( + 6, + hc.table.history.len(), + profile, + HcCandidateQuery { + reps: [1, 2, 3], + lit_len: 1, + ldm_candidate: None, + }, + &mut out, + ); +} + +#[test] +fn hc_collect_optimal_candidates_dispatches_every_bt_strategy() { + use crate::encoding::fastpath::FastpathKernel; + use crate::encoding::strategy::StrategyTag; + // The public dispatcher must route every BT strategy tag to the collector. + // Run under the scalar kernel so the scalar dispatch arm is exercised too. + for tag in [ + StrategyTag::BtOpt, + StrategyTag::BtUltra, + StrategyTag::BtUltra2, + StrategyTag::Btlazy2, + ] { + let mut hc = HcMatchGenerator::new(64); + hc.strategy_tag = tag; + hc.table.kernel = FastpathKernel::Scalar; + hc.table.history = b"abcabcabcabcabcabc".to_vec(); + hc.table.history_start = 0; + hc.table.history_abs_start = 0; + hc.table.hash_log = 8; + hc.table.chain_log = 8; + // BtUltra / BtUltra2 drive the hash3 short-match table; size it so the + // collector's hash3 probe has a live table. + hc.table.hash3_log = 8; + hc.table.ensure_tables(); + let profile = HcOptimalCostProfile { + max_chain_depth: 4, + sufficient_match_len: usize::MAX / 2, + accurate: false, + favor_small_offsets: false, + }; + let mut out = Vec::new(); + hc.collect_optimal_candidates( + 6, + hc.table.history.len(), + profile, + HcCandidateQuery { + reps: [1, 2, 3], + lit_len: 1, + ldm_candidate: None, + }, + &mut out, + ); + } +} + #[test] fn hc_collect_optimal_candidates_rep_tail_match_skips_chain_probe() { let mut hc = HcMatchGenerator::new(64); From 8d3387f912824086b755eb5063f9ca72b3353f40 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 1 Jul 2026 00:56:35 +0300 Subject: [PATCH 6/6] test(opt): assert per-tag dispatch via the hash3 observable The dispatch test previously only proved each BT tag ran without panicking, so a cross-group mis-mapping (e.g. BtUltra routed to the BtOpt specialization) would slip through. Add a USE_HASH3 observable: a fixture where `abc` repeats with a differing 4th byte, so the hash3 specializations (BtUltra / BtUltra2) surface a length-3 match at offset 12 that the 4-byte BT hash misses, while the non-hash3 ones (BtOpt / Btlazy2) do not. Assert the match's presence equals the tag's USE_HASH3. (BtOpt vs Btlazy2 share identical collect consts, so they are runtime-indistinguishable; that mapping is compiler-enforced.) --- zstd/src/encoding/match_generator/tests.rs | 32 ++++++++++++++++------ 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/zstd/src/encoding/match_generator/tests.rs b/zstd/src/encoding/match_generator/tests.rs index 71a2f1bee..016ef72ef 100644 --- a/zstd/src/encoding/match_generator/tests.rs +++ b/zstd/src/encoding/match_generator/tests.rs @@ -1350,8 +1350,16 @@ fn hc_collect_optimal_candidates_panics_for_non_bt_strategy() { fn hc_collect_optimal_candidates_dispatches_every_bt_strategy() { use crate::encoding::fastpath::FastpathKernel; use crate::encoding::strategy::StrategyTag; - // The public dispatcher must route every BT strategy tag to the collector. - // Run under the scalar kernel so the scalar dispatch arm is exercised too. + // The public dispatcher must route every BT strategy tag to the collector + // AND to the specialization carrying that tag's consts — not just survive. + // The observable dimension is `USE_HASH3` (BtUltra / BtUltra2 = true; BtOpt / + // Btlazy2 = false): only the hash3 specializations surface a 3-byte match + // that the 4-byte BT hash cannot find. (BtOpt vs Btlazy2 share identical + // collect consts, so they are indistinguishable at runtime — the type + // mapping there is compiler-enforced.) Fixture: `abc` repeats at 0 and 12 + // with a differing 4th byte (`Q` vs `Z`), so hash3 finds a length-3 match at + // offset 12 while the 4-byte hash of `abcZ` misses `abcQ`. Run under the + // scalar kernel so the scalar dispatch arm is exercised too. for tag in [ StrategyTag::BtOpt, StrategyTag::BtUltra, @@ -1361,33 +1369,41 @@ fn hc_collect_optimal_candidates_dispatches_every_bt_strategy() { let mut hc = HcMatchGenerator::new(64); hc.strategy_tag = tag; hc.table.kernel = FastpathKernel::Scalar; - hc.table.history = b"abcabcabcabcabcabc".to_vec(); + hc.table.history = b"abcQ00000000abcZ00000000".to_vec(); hc.table.history_start = 0; hc.table.history_abs_start = 0; hc.table.hash_log = 8; hc.table.chain_log = 8; - // BtUltra / BtUltra2 drive the hash3 short-match table; size it so the - // collector's hash3 probe has a live table. hc.table.hash3_log = 8; hc.table.ensure_tables(); + let abs_pos = 12usize; let profile = HcOptimalCostProfile { - max_chain_depth: 4, + max_chain_depth: 8, sufficient_match_len: usize::MAX / 2, accurate: false, favor_small_offsets: false, }; let mut out = Vec::new(); hc.collect_optimal_candidates( - 6, + abs_pos, hc.table.history.len(), profile, HcCandidateQuery { - reps: [1, 2, 3], + // Reps past abs_pos are skipped, so the only candidate source is + // the (hash3 / BT) match finder — keeping the observable clean. + reps: [50, 60, 70], lit_len: 1, ldm_candidate: None, }, &mut out, ); + let uses_hash3 = matches!(tag, StrategyTag::BtUltra | StrategyTag::BtUltra2); + let found_hash3_match = out.iter().any(|c| c.offset == 12 && c.match_len == 3); + assert_eq!( + found_hash3_match, uses_hash3, + "tag {tag:?}: presence of the hash3-only 3-byte match must equal USE_HASH3 \ + (a cross-group dispatch mis-mapping would flip this)" + ); } }