From 20b04064ccd0eb07cf20af1931a959c0e0a0f26d Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Mon, 7 Sep 2026 22:06:07 +0300 Subject: [PATCH 01/18] fix(cli): read --fast the way the reference reads it The acceleration factor is the LEADING number of the argument: upstream takes a run of decimal digits plus an optional K/M multiplier and stops, never looking at what follows, clamps a factor past the minimum level instead of refusing it, and treats only a zero factor as an error (zstdcli.c:1133-1153 -> readU32FromCharChecked, 350-376). Four spellings behaved differently here, so a working command line failed against a drop-in build: - `--fast=3.5` / `--fast=3x` were refused; they are level -3. - `--fast=1K` / `--fast=2KiB` were refused; the multipliers are real. - `--fast=200000` was refused; it clamps to the minimum level. - `--fast=+3` was ACCEPTED, because Rust's integer parser takes a sign; a sign is not a digit, so the factor reads as zero and is an error. Every accepted and refused form now agrees with the reference across the whole surface, including the ones that already did. --- zstd/src/bin/structured-zstd/main.rs | 48 +++++++++++++++++++++++---- zstd/src/bin/structured-zstd/tests.rs | 38 +++++++++++++++++++++ 2 files changed, 80 insertions(+), 6 deletions(-) diff --git a/zstd/src/bin/structured-zstd/main.rs b/zstd/src/bin/structured-zstd/main.rs index 82164517e..ace2bc034 100644 --- a/zstd/src/bin/structured-zstd/main.rs +++ b/zstd/src/bin/structured-zstd/main.rs @@ -176,6 +176,37 @@ fn parse_size(text: &str) -> Result { .ok_or_else(|| eyre!("size `{text}` does not fit in 64 bits")) } +/// Read the leading unsigned number the way upstream's argument reader does +/// (`readU32FromCharChecked`, zstdcli.c:350-376): a run of decimal digits, +/// then an optional `K` or `M` multiplier which may be spelled `KiB` / `MB`. +/// Reading STOPS there and the remainder is handed back — no sign is accepted, +/// and an empty digit run reads as zero, which every caller treats as invalid. +fn read_leading_u32(text: &str) -> Result<(u32, &str)> { + let bytes = text.as_bytes(); + let mut at = 0; + let mut value: u32 = 0; + while at < bytes.len() && bytes[at].is_ascii_digit() { + value = value + .checked_mul(10) + .and_then(|v| v.checked_add(u32::from(bytes[at] - b'0'))) + .ok_or_else(|| eyre!("numeric value `{text}` overflows 32-bit unsigned int"))?; + at += 1; + } + if at < bytes.len() && matches!(bytes[at], b'K' | b'M') { + let shifts = if bytes[at] == b'M' { 2 } else { 1 }; + for _ in 0..shifts { + value = value + .checked_mul(1024) + .ok_or_else(|| eyre!("numeric value `{text}` overflows 32-bit unsigned int"))?; + } + at += 1; + // `KiB` and `KB` are the same multiplier spelled longer. + at += usize::from(bytes.get(at) == Some(&b'i')); + at += usize::from(bytes.get(at) == Some(&b'B')); + } + Ok((value, &text[at..])) +} + /// Parse a `-M` / `--memory` value into bytes, or `None` for "the default". /// /// The default unit is MiB, as upstream documents, so `-M256` is 256 MiB. A @@ -518,17 +549,22 @@ fn parse_args( // `--fast` is the level -1 alias. opts.level = -1; } else if let Some(v) = long.strip_prefix("fast=") { - // `--fast=N` is level -N for a positive N. Parse as - // unsigned so `--fast=-5` is rejected rather than flipping - // sign into a positive level. Exact-match the prefix so a - // typo like `--faster` falls through to unknown-option. - let n = v.parse::().wrap_err("invalid --fast level")?; + // `--fast=N` is level -N (upstream zstd, + // zstdcli.c:1133-1153). The factor is the LEADING + // number only, so `--fast=3.5` is level -3 and the tail + // is dropped; a factor past the minimum level clamps + // rather than failing; only a zero factor is an error. + // Exact-match the prefix so a typo like `--faster` + // falls through to unknown-option. + let (n, _tail) = read_leading_u32(v).wrap_err("invalid --fast level")?; // Zero would negate to level 0, which is the ordinary // default rather than a fast one. if n == 0 { bail!("--fast level must be at least 1, got 0"); } - opts.level = -i32::try_from(n).wrap_err("--fast level too large")?; + let capped = n.min(CompressionLevel::MIN_LEVEL.unsigned_abs()); + opts.level = -i32::try_from(capped) + .expect("capped at |MIN_LEVEL|, which is an i32 magnitude"); } else if long.starts_with("use-dict=") { opts.dict = Some(attached_path(arg_os, "--use-dict=".len())); } else if let Some(v) = long.strip_prefix("maxdict=") { diff --git a/zstd/src/bin/structured-zstd/tests.rs b/zstd/src/bin/structured-zstd/tests.rs index 56aa4ce75..72569787c 100644 --- a/zstd/src/bin/structured-zstd/tests.rs +++ b/zstd/src/bin/structured-zstd/tests.rs @@ -1376,6 +1376,44 @@ fn fast_flag_maps_to_negative_level() { assert!(parse(&["--fast=0"]).is_err()); } +#[test] +fn fast_level_reads_a_digit_run_and_ignores_the_tail() { + // upstream zstd (zstdcli.c:1139 -> readU32FromCharChecked, 350-376): the + // acceleration factor is the LEADING digit run; the parser stops at the + // first byte that is neither a digit nor a K/M multiplier and `--fast` + // never looks at what is left. `zstd --fast=3.5` compresses at level -3, + // so refusing it turns a working command line into an error. + assert_eq!(parse(&["--fast=3.5"]).unwrap().level, -3); + assert_eq!(parse(&["--fast=3x"]).unwrap().level, -3); + assert_eq!(parse(&["--fast=3G"]).unwrap().level, -3); + // A sign is not a digit, so the run is empty and the factor is zero — + // which upstream rejects. Rust's own integer parser accepts `+3`, and + // accepting it here would take a level upstream refuses. + assert!(parse(&["--fast=+3"]).is_err()); +} + +#[test] +fn fast_level_honours_the_k_and_m_multipliers() { + // Same reader, suffix half (zstdcli.c:362-373): `K` shifts by 10, `M` by + // 20, each with an optional `i` and `B` spelling. + assert_eq!(parse(&["--fast=1K"]).unwrap().level, -1024); + assert_eq!(parse(&["--fast=2KiB"]).unwrap().level, -2048); + // A bare multiplier has no digits before it, so it reads as zero. + assert!(parse(&["--fast=K"]).is_err()); +} + +#[test] +fn fast_level_clamps_to_the_minimum_level() { + // upstream zstd (zstdcli.c:1136-1140): a factor past `-ZSTD_minCLevel()` + // is CLAMPED, not refused — `zstd --fast=200000` compresses at the + // lowest level rather than failing. + let min = structured_zstd::encoding::CompressionLevel::MIN_LEVEL; + assert_eq!(parse(&["--fast=200000"]).unwrap().level, min); + assert_eq!(parse(&["--fast=1M"]).unwrap().level, min); + // The digit run itself still has to fit 32 bits (zstdcli.c:356-359). + assert!(parse(&["--fast=99999999999"]).is_err()); +} + #[test] fn clustered_short_flags() { // -d (decompress) + -c (stdout) + -k (keep) in one token. From f88034d2d310fa437ff91e2c870dd20552615da8 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Mon, 7 Sep 2026 22:47:22 +0300 Subject: [PATCH 02/18] fix(encode): copy the dictionary above the size the reference copies it at A dictionary made our ultra-fast frames BIGGER than our own no-dict frames on a 1 MiB corpus file (+1.9 to +2.2%), while it made the reference's smaller (-3.7 to -4.3%). Without a dictionary the two sides emit the identical 20,159 sequences on that file; with one the reference emits 27,546 to our 21,897, and only 0.1% of its extra sequences reach back into the dictionary at all. They are ordinary near matches, found everywhere because its table already holds the dictionary. The Fast attach cutoff was 2 GiB, so every source attached: a separate dictionary table, reached only where the scan's step happens to land, with the live table starting empty. The reference copies above 8 KiB (attachDictSizeCutoffs[ZSTD_fast], zstd_compress.c:2296) and takes the cutoff back to that. It had been raised on a speed argument with no byte column beside it, which is how a 6% ratio hole went unnoticed; the constant now carries both columns. Copy mode then had to index the dictionary the way the reference does. It builds the table once with ZSTD_fillHashTableForCDict (stride 3, the step position winning its slot and the two after it filling only an empty one) and installs it by stripping the tags. We were filling densely, which keeps a nearer occurrence per bucket and fragments one long dictionary match into several short ones. On log-shaped input that alone was 78 bytes against the reference's 71; with the fill it is 73. Alternating the two modes on one compressor then hit a stale table: the cached dictionary table survived a copy frame, and the borrowed-scan dispatch reads exactly that flag, so a copy frame's scan went to the dual-base kernel and read raw positions as virtual ones. A frame that is not an attach frame now drops the cached table with the live one. The existing alternation test covers it and had never run before, because at a 2 GiB cutoff its 64 KiB payload attached like everything else. Bytes, on z000033 (1,022,035 B) with its 16 KiB dictionary, ours against the reference: --fast=7 +5.95% -> -0.08%, --fast=5 +6.15% -> -0.04%, --fast=3 +6.43% -> -0.01%. Every level is now at or under it. No-dict output is byte-identical across 27 fixture-and-level rows, as are all dictionary frames at or under the cutoff and every non-Fast level. --- zstd/src/encoding/frame_compressor.rs | 7 +- zstd/src/encoding/levels/config.rs | 50 ++++--- zstd/src/encoding/match_generator/mod.rs | 2 +- zstd/src/encoding/match_generator/tests.rs | 53 ++++---- .../src/encoding/simple/fast_kernel/kernel.rs | 2 +- zstd/src/encoding/simple/fast_matcher.rs | 124 ++++++++++++++++-- .../src/encoding/simple/fast_matcher/tests.rs | 47 +++++++ 7 files changed, 221 insertions(+), 64 deletions(-) diff --git a/zstd/src/encoding/frame_compressor.rs b/zstd/src/encoding/frame_compressor.rs index 1c1918cfd..ff8b5062b 100644 --- a/zstd/src/encoding/frame_compressor.rs +++ b/zstd/src/encoding/frame_compressor.rs @@ -2258,10 +2258,9 @@ impl FrameCompressor { // `hint > 2^k`, so this is identical to the raw `hint > cutoff` on // 64-bit. let cutoff_log = match self.state.strategy_tag { - // Fast always attaches now (the copy-mode owned path memmoved the - // whole input into history every frame); keep the copy-snapshot - // gate in sync with the matcher's attach cutoff so Fast never - // captures/restores a copy snapshot it can no longer use. + // Keep the copy-snapshot gate in sync with the matcher's own + // attach cutoff, so Fast never captures or restores a snapshot + // for a mode it did not resolve. crate::encoding::strategy::StrategyTag::Fast => { crate::encoding::levels::config::FAST_ATTACH_DICT_CUTOFF_LOG } diff --git a/zstd/src/encoding/levels/config.rs b/zstd/src/encoding/levels/config.rs index 01a3f3649..f851e3196 100644 --- a/zstd/src/encoding/levels/config.rs +++ b/zstd/src/encoding/levels/config.rs @@ -442,27 +442,41 @@ pub(crate) fn source_size_ceil_log(size: u64) -> u8 { /// immutable table scanned in place via the borrowed dual-base kernel); a larger /// hint would COPY it into the live table. /// -/// We set this to `31` so every dictionary source up to 2 GiB attaches, -/// diverging from upstream zstd's 8 KiB `ZSTD_shouldAttachDict` cutoff ON -/// PURPOSE: upstream copy mode copies the small CDict TABLES into the cctx and -/// still scans the input in place, but our flat-history copy path memmoves the -/// whole INPUT into history every frame (profiled at 30% `__memmove` + 14% -/// `__memset` on a reused 1 MiB dict encode). Attach mode scans the caller's -/// input in place with the dict as a separate prefix base, so it is strictly -/// faster for every frame size here (measured: 1 MiB dict frame 167 us -> 52 us, -/// 0.42x of C; 10 KiB 20.4 us -> 4.4 us, 0.17x of C). The dual-base kernel -/// carries `window_low`, so over-window inputs stay in-window and C-decodable. +/// `13` is upstream zstd's Fast cutoff (`attachDictSizeCutoffs[ZSTD_fast]` is +/// 8 KB, zstd_compress.c:2296; `ZSTD_shouldAttachDict`, :2309). Above it the +/// dictionary is COPIED, and the copy is what makes the dictionary pay on a +/// larger source: the scan then runs over a table already holding the +/// dictionary's positions, so ordinary NEAR matches improve everywhere. Attach +/// mode starts with an empty table and reaches the dictionary only through the +/// separate exact table, at the positions the step happens to land on. /// -/// `31` is also the largest bucket the borrowed kernel can attach: it stores -/// virtual positions as `u32` (`cur_abs as u32`), so the maximum attached source -/// `1 << 31` (plus the dict prefix) stays below `u32::MAX`; the next bucket `32` -/// (4 GiB) would wrap that arithmetic. Sources past 2 GiB therefore fall back to -/// copy mode — rare in practice, and the relative copy cost shrinks as the -/// source grows. Per the drop-in-not-binary-parity contract, we make this match -/// decision ourselves. +/// This was `31` (attach every source up to 2 GiB) on a speed argument alone, +/// and the missing byte column is where it went wrong. `z000033` (1,022,035 B) +/// with its 16 KiB dictionary, attach → copy, against libzstd on the same host: +/// +/// | case | bytes attach | bytes copy | reference | cycles attach | cycles copy | +/// |---|---|---|---|---|---| +/// | 10 KiB frames, L1 | 6,976 | 7,123 | 7,123 | 2.06x | 1.68x | +/// | 10 KiB frames, L-5 | 9,659 | 9,123 | 9,129 | — | — | +/// | 1 MiB, L1 | -0.24% | -0.09% | — | 1.96x | 1.71x | +/// | 1 MiB, L-5 | +5.95% | -0.08% | — | 1.48x | 1.78x | +/// +/// So attach was losing 6% of the ratio across the ultra-fast band — the +/// dictionary made our frame BIGGER than our own no-dict frame there, while it +/// made the reference's smaller — and the cycles it appeared to save in that +/// band were bought by finding 21,897 sequences where the reference finds +/// 27,546. At the positive levels copy is the cheaper arm as well. +/// +/// The remaining gap is now a same-mode one: the reference does this copy in +/// 1.0x where we take 1.7-1.8x, which is a target with an apples-to-apples +/// reference rather than a mode the reference never runs. +/// +/// The borrowed attach kernel stores virtual positions as `u32` +/// (`cur_abs as u32`), so it could not attach past bucket `31` regardless; that +/// ceiling is now far above the cutoff and no longer the binding constraint. /// Shared by `reset` (records the mode in the primed-snapshot key) and /// `prime_with_dictionary` (acts on it). -pub(crate) const FAST_ATTACH_DICT_CUTOFF_LOG: u8 = 31; +pub(crate) const FAST_ATTACH_DICT_CUTOFF_LOG: u8 = 13; /// Largest dictionary region (bytes) the Fast attach path can index. The tagged /// dict table packs each position into `32 - DICT_TAG_BITS` (= 24) bits, so a diff --git a/zstd/src/encoding/match_generator/mod.rs b/zstd/src/encoding/match_generator/mod.rs index 810dea0fa..5d9cd7ade 100644 --- a/zstd/src/encoding/match_generator/mod.rs +++ b/zstd/src/encoding/match_generator/mod.rs @@ -907,7 +907,7 @@ impl MatchGeneratorDriver { if attach { self.simple_mut().skip_matching_for_dict_prime(dict_len); } else { - self.simple_mut().skip_matching_with_hint(Some(false)); + self.simple_mut().skip_matching_for_dict_copy(); } self.recycle_simple_space(); } diff --git a/zstd/src/encoding/match_generator/tests.rs b/zstd/src/encoding/match_generator/tests.rs index 3777dfe5f..ee4f89f67 100644 --- a/zstd/src/encoding/match_generator/tests.rs +++ b/zstd/src/encoding/match_generator/tests.rs @@ -3157,52 +3157,49 @@ fn primed_snapshot_restored_across_level22_tier_hints() { } #[test] -fn fast_dict_attaches_within_cutoff_bounds() { - // Within the attach bounds, every Fast dict frame attaches (the copy-mode - // owned path memmoved the whole input into history each frame; attach scans - // the input in place via the borrowed dual-base kernel). All hints here sit - // far below `FAST_ATTACH_DICT_CUTOFF_LOG` (2 GiB source) and the dict is far - // below `MAX_FAST_ATTACH_DICT_REGION` (16 MiB), so a hint that used to cross - // the old 8 KiB cutoff (8193 B) and a small one (8192 B) BOTH resolve to - // attach, and the Simple backend reports a borrowed (in-place) dict scan for - // both. This guards `FAST_ATTACH_DICT_CUTOFF_LOG` staying high enough that no - // in-bounds Fast hint falls back to the input-copy path; the OUT-of-bounds - // fallbacks are covered by `fast_attach_cutoff_keeps_virtual_positions_within_u32` - // (source) and `oversized_dict_hint_routes_fast_to_copy_mode` (dict size). +fn fast_dict_attach_follows_the_source_size_cutoff() { + // The cutoff is the 8 KiB one upstream uses for the Fast strategy, and it is + // the SOURCE size that decides: at or under it the dictionary is attached (a + // separate table, scanned in place by the borrowed dual-base kernel), over it + // it is copied into the live table so ordinary near matches see it too. The + // pair 8192 / 8193 pins the boundary itself; the dict here is far below + // `MAX_FAST_ATTACH_DICT_REGION`, so only the source size is in play. The + // out-of-bounds fallbacks are covered by + // `fast_attach_cutoff_keeps_virtual_positions_within_u32` (source) and + // `oversized_dict_hint_routes_fast_to_copy_mode` (dict size). let level = CompressionLevel::Level(1); - for hint in [8192u64, 8193, 1 << 20] { + for (hint, attaches) in [(8192u64, true), (8193, false), (1 << 20, false)] { let mut driver = MatchGeneratorDriver::new(8, 1); driver.set_source_size_hint(hint); driver.reset(level); driver.prime_with_dictionary(b"abcdefghABCDEFGHijklmnop", [1, 4, 8]); - assert!( + assert_eq!( driver.borrowed_dict_supported(), - "Fast dict frame with hint {hint} must attach (borrowed in-place \ - dict scan), never fall back to the copy-mode input-copy path" + attaches, + "Fast dict frame with hint {hint} resolved the wrong dictionary mode", ); } } #[test] fn fast_attach_cutoff_keeps_virtual_positions_within_u32() { - // The cutoff is 31, NOT the full u64 source-size range, because the borrowed - // dict kernel stores virtual positions as u32 (`cur_abs as u32`). The largest - // attached source `1 << CUTOFF` (plus the dict prefix) must stay below - // u32::MAX or that arithmetic wraps; the next bucket (4 GiB) would. This pins - // the bound so a future "just raise it to attach everything" change cannot - // silently reintroduce the overflow — raising the cutoff requires widening - // the kernel's position type first. + // Two independent bounds meet on this constant. The upstream one decides it: + // the Fast strategy copies above 8 KiB (`attachDictSizeCutoffs[ZSTD_fast]`), + // and the ratio measurements behind the constant say the same. The second is + // a hard ceiling from our own borrowed kernel, which stores virtual positions + // as `u32` (`cur_abs as u32`): the largest attached source plus the dict + // prefix has to stay under `u32::MAX`, so a future "just attach everything" + // change cannot raise the cutoff past 31 without widening that type first. + assert_eq!( + FAST_ATTACH_DICT_CUTOFF_LOG, 13, + "the Fast attach cutoff is upstream's 8 KiB source size", + ); let max_attached: u64 = 1u64 << FAST_ATTACH_DICT_CUTOFF_LOG; assert!( max_attached <= u32::MAX as u64, "the largest attached source 2^{FAST_ATTACH_DICT_CUTOFF_LOG} must fit u32 \ virtual positions", ); - assert!( - (1u64 << (FAST_ATTACH_DICT_CUTOFF_LOG + 1)) > u32::MAX as u64, - "the next bucket 2^{} would overflow u32 virtual positions", - FAST_ATTACH_DICT_CUTOFF_LOG + 1, - ); } #[test] diff --git a/zstd/src/encoding/simple/fast_kernel/kernel.rs b/zstd/src/encoding/simple/fast_kernel/kernel.rs index eaf218599..125e9cded 100644 --- a/zstd/src/encoding/simple/fast_kernel/kernel.rs +++ b/zstd/src/encoding/simple/fast_kernel/kernel.rs @@ -1716,7 +1716,7 @@ fn compress_block_fast_dict_borrowed_impl< // — no per-candidate range check is needed on the hot path. debug_assert!( main_idx == 0 || main_idx as usize >= dict_end, - "main-table entry must be the sentinel or a virtual input position (>= dict_end)", + "main-table entry must be the sentinel or a virtual input position (>= dict_end): got {main_idx}, dict_end={dict_end}, bias={main_bias}", ); let main_valid = if USE_CMOV { let in_range = main_idx >= prefix_start_index; diff --git a/zstd/src/encoding/simple/fast_matcher.rs b/zstd/src/encoding/simple/fast_matcher.rs index d6287e457..2ad65abc9 100644 --- a/zstd/src/encoding/simple/fast_matcher.rs +++ b/zstd/src/encoding/simple/fast_matcher.rs @@ -282,6 +282,10 @@ pub(crate) struct FastKernelMatcher { /// and a plain frame has none). Cleared on reset / eviction (the dict has /// slid out of the window, so the windowed floor takes over). loaded_dict_end: usize, + /// Where the copy-mode dictionary fill resumes its stride, carried across + /// the slices one dictionary is committed in so the phase stays continuous. + /// See [`Self::prime_hash_table_for_dict_copy`]. Reset with the table. + dict_copy_fill_next: usize, } impl Clone for FastKernelMatcher { @@ -307,6 +311,7 @@ impl Clone for FastKernelMatcher { dict_resident: self.dict_resident, dict_table_hash_log: self.dict_table_hash_log, loaded_dict_end: self.loaded_dict_end, + dict_copy_fill_next: self.dict_copy_fill_next, } } @@ -334,6 +339,7 @@ impl Clone for FastKernelMatcher { self.table_pos_high_water = source.table_pos_high_water; self.dict_resident = source.dict_resident; self.loaded_dict_end = source.loaded_dict_end; + self.dict_copy_fill_next = source.dict_copy_fill_next; } } @@ -556,6 +562,7 @@ impl FastKernelMatcher { dict_resident: false, dict_table_hash_log: None, loaded_dict_end: 0, + dict_copy_fill_next: 0, } } @@ -648,16 +655,26 @@ impl FastKernelMatcher { } } else { // Same shape — keep the allocation, zero the entries via - // `memset` (ZSTD_window_clear cadence). A primed dict table - // is retained (see the epoch branch above for why that is - // sound). + // `memset` (ZSTD_window_clear cadence). self.hash_table.clear(); + // Drop the cached dict table with it. This frame is not an + // attach frame, so its dictionary (if any) goes into the LIVE + // table as raw positions — but the borrowed-scan dispatch reads + // `dict_is_attached()`, so a table left over from an earlier + // attach frame would send this frame's scan into the dual-base + // kernel, which reads every main-table entry as a virtual + // `dict_end + offset` position and would take raw ones for + // matches inside the dictionary. + self.dict.invalidate(); } self.table_pos_high_water = 0; // No copy-mode dict is resident across a reset: attach mode re-borrows // its separate dict table (handled above), and the copy path re-primes // (and re-records `loaded_dict_end`) during this frame's dict prime. self.loaded_dict_end = 0; + // The copy-mode fill starts over with the frame: its stride is anchored + // at the dictionary's first byte in the new history. + self.dict_copy_fill_next = 0; if let Some(region) = reborrow_region { // Keep `[0, region)` (the resident dict); drop the previous input. self.history.truncate(region); @@ -1534,18 +1551,29 @@ impl FastKernelMatcher { } if incompressible_hint == Some(false) { self.prime_hash_table_for_range(block_start); - // Copy-mode dict prime: the dict now occupies `[0, history.len())` - // at the front of history (this is the only caller of the - // `Some(false)` hint — see the doc above). Record the dict/input - // boundary so `start_matching` floors the block prefix at the dict - // start while the dict stays within the window (upstream zstd - // `ms->loadedDictEnd`). A multi-slice dict advances this to the - // running end on each slice; the final slice leaves the full - // dict size. - self.loaded_dict_end = self.history.len(); } } + /// Commit one slice of a COPY-mode dictionary: append it to history, index + /// it into the LIVE table with upstream's dictionary fill, and carry the + /// dictionary boundary. + /// + /// Separate from the `Some(false)` skip hint because the two want different + /// fills. A block emitted verbatim (RLE / raw) is ordinary window history + /// and is indexed densely; a dictionary is indexed the way upstream indexes + /// one — see [`Self::prime_hash_table_for_dict_copy`]. + /// + /// `loaded_dict_end` records the dictionary/input boundary so + /// `start_matching` floors the block prefix at the dictionary start while + /// the dictionary is still within the window (upstream zstd + /// `ms->loadedDictEnd`); a dictionary committed in several slices advances + /// it to the running end each time, leaving the full size after the last. + pub(crate) fn skip_matching_for_dict_copy(&mut self) { + self.extend_history_with_pending(); + self.prime_hash_table_for_dict_copy(); + self.loaded_dict_end = self.history.len(); + } + /// Borrowed-window equivalent of [`Self::skip_matching_with_hint`]: /// the block `[block_start, block_end)` is emitted as RLE / raw /// without running the kernel, but its bytes are already resident in @@ -1735,6 +1763,78 @@ impl FastKernelMatcher { self.prime_hash_table_for_range_stepped(range_start, 1); } + /// Fill the LIVE table with a copy-mode dictionary, the way upstream's + /// copy-mode context ends up filled. + /// + /// Upstream builds the dictionary's table once with + /// `ZSTD_fillHashTableForCDict` (zstd_fast.c:16-49) and installs it as the + /// live table by stripping the short-cache tags out of it + /// (`ZSTD_copyCDictTableIntoCCtx`, zstd_compress.c:2386-2400). So the live + /// table holds that fill's occurrence set: stride 3, where the step + /// position overwrites its slot and the two positions after it are written + /// only into a slot still empty. A dense every-position fill keeps a + /// different (nearer) occurrence per bucket, which fragments one long + /// dictionary match into several short ones — the same defect measured on + /// the attach table, and the reason copy mode was losing to the reference + /// on repetitive input where attach was beating it. + /// + /// The stride is anchored at [`Self::dict_copy_fill_next`], carried across + /// dictionary slices, so a dictionary committed in several pieces keeps one + /// continuous phase (and the positions whose hash read straddles a slice + /// seam are reached by the next slice) exactly as the attach fill does. + fn prime_hash_table_for_dict_copy(&mut self) { + const HASH_READ_SIZE: usize = 8; + let history_len = self.history.len(); + if history_len < HASH_READ_SIZE { + return; + } + let last_hashable = history_len - HASH_READ_SIZE; + let fill_start = self.dict_copy_fill_next; + if fill_start > last_hashable { + return; + } + let base = self.history.as_ptr(); + self.dict_copy_fill_next = match self.hash_table.mls() { + 4 => self.prime_hash_table_dict_copy_impl::<4>(base, fill_start, last_hashable), + 5 => self.prime_hash_table_dict_copy_impl::<5>(base, fill_start, last_hashable), + 6 => self.prime_hash_table_dict_copy_impl::<6>(base, fill_start, last_hashable), + 7 => self.prime_hash_table_dict_copy_impl::<7>(base, fill_start, last_hashable), + 8 => self.prime_hash_table_dict_copy_impl::<8>(base, fill_start, last_hashable), + _ => unreachable!("FastHashTable construction rejects mls outside 4..=8"), + }; + } + + /// Monomorphised body of [`Self::prime_hash_table_for_dict_copy`]; + /// returns where the next slice resumes the stride. + fn prime_hash_table_dict_copy_impl( + &mut self, + base: *const u8, + range_start: usize, + last_hashable: usize, + ) -> usize { + const FILL_STEP: usize = 3; + let mut s = range_start; + // Same bound as upstream (`ip + step < iend + 2`, `iend` being the last + // hashable position), so the trailing partial group is left to the next + // slice rather than stepping past the readable region. + while s + FILL_STEP < last_hashable + 2 { + // SAFETY: `s <= last_hashable = history.len() - 8`, so the position + // has the kernel's full load width readable; MLS is the table's own. + let hash = unsafe { self.hash_table.hash_ptr::(base.add(s)) }; + unsafe { self.hash_table.put(hash, s as u32) }; + for p in 1..FILL_STEP { + let pos = s + p; + // SAFETY: `pos <= s + 2 <= last_hashable` by the loop bound. + let hash = unsafe { self.hash_table.hash_ptr::(base.add(pos)) }; + if unsafe { self.hash_table.get(hash) } == 0 { + unsafe { self.hash_table.put(hash, pos as u32) }; + } + } + s += FILL_STEP; + } + s + } + /// [`Self::prime_hash_table_for_range`] taking every `step`-th position. fn prime_hash_table_for_range_stepped(&mut self, range_start: usize, step: usize) { let history_len = self.history.len(); diff --git a/zstd/src/encoding/simple/fast_matcher/tests.rs b/zstd/src/encoding/simple/fast_matcher/tests.rs index 7ea7f1ff8..e70a6abfd 100644 --- a/zstd/src/encoding/simple/fast_matcher/tests.rs +++ b/zstd/src/encoding/simple/fast_matcher/tests.rs @@ -601,6 +601,53 @@ fn skip_matching_dict_prime_handles_exactly_hash_read_size_bytes() { // Reaching this line without unwinding is the test. } +/// A copy-mode dictionary is indexed the way upstream indexes one: stride 3, +/// where the step position wins its slot and the two positions after it are +/// written only into a slot still empty (`ZSTD_fillHashTableForCDict`, whose +/// table upstream installs as the live one after stripping the tags). +/// +/// The check is the OCCURRENCE a bucket resolves to, not merely that something +/// was indexed: a dense every-position fill leaves the LAST position of each +/// bucket, which is a different (nearer) occurrence, and that is what fragments +/// one long dictionary match into several short ones. +#[test] +fn copy_mode_dictionary_fill_keeps_the_upstream_occurrence_per_bucket() { + // A period-4 pattern with a 4-byte hash: positions 4 apart carry identical + // content, so they share a bucket and the bucket's final value tells the + // two policies apart. Under upstream's fill the winner is the last STRIDE + // position of that phase (a multiple of 12, since the phase repeats every 4 + // and the stride is 3); a dense fill would leave the last position of the + // phase outright. + let dict: alloc::vec::Vec = b"abcd".iter().copied().cycle().take(96).collect(); + let mut m = FastKernelMatcher::with_params(12, 12, 4, 2); + m.accept_data(dict); + m.skip_matching_for_dict_copy(); + + let base = m.history.as_ptr(); + let last_hashable = m.history.len() - 8; + // SAFETY: position 0 has the kernel's load width readable (the dictionary + // is far longer than it), and `hash_ptr` bounds the slot to the table. + let phase_0 = unsafe { + let hash = m.hash_table.hash_ptr::<4>(base); + m.hash_table.get(hash) + }; + let last_stride_of_phase = (0..=last_hashable) + .rfind(|p| p % 12 == 0) + .expect("the dictionary spans several stride groups"); + let last_of_phase = (0..=last_hashable) + .rfind(|p| p % 4 == 0) + .expect("the dictionary spans several phase positions"); + assert_ne!( + last_stride_of_phase, last_of_phase, + "the fixture must separate the two policies", + ); + assert_eq!( + phase_0, last_stride_of_phase as u32, + "the bucket must hold the last stride position of its phase, not the \ + last position ({last_of_phase}) a dense fill would leave", + ); +} + /// Boundary: pending block too short to hash anything (less than /// `HASH_READ_SIZE` bytes). The dict-prime path must early-return /// without panicking on the `last_hashable` subtract. From 58f8644429a988d802f764f0ad06a2fd574e473b Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Mon, 7 Sep 2026 22:59:22 +0300 Subject: [PATCH 03/18] fix(encode): drop the attach dictionary cache where copy mode primes it away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Invalidating it in `reset` also hit ATTACH frames, whose whole point is living off that cache: rebuilding the dictionary table every frame cost 12% on a reused 4 KiB dictionary frame (i9, wall clock, three interleaved rounds, 0.623 s -> 0.706 s). That size is below the attach cutoff, so it is a path this work does not otherwise touch — the regression showed up as a control arm that would not stay flat. The copy prime is the only place that must not keep the cache, so it drops it there. --- zstd/src/encoding/simple/fast_matcher.rs | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/zstd/src/encoding/simple/fast_matcher.rs b/zstd/src/encoding/simple/fast_matcher.rs index 2ad65abc9..21928aff4 100644 --- a/zstd/src/encoding/simple/fast_matcher.rs +++ b/zstd/src/encoding/simple/fast_matcher.rs @@ -655,17 +655,11 @@ impl FastKernelMatcher { } } else { // Same shape — keep the allocation, zero the entries via - // `memset` (ZSTD_window_clear cadence). + // `memset` (ZSTD_window_clear cadence). A primed dict table + // is retained (see the epoch branch above for why that is + // sound); a copy-mode frame drops it when it primes, which is + // the only case that must not keep it. self.hash_table.clear(); - // Drop the cached dict table with it. This frame is not an - // attach frame, so its dictionary (if any) goes into the LIVE - // table as raw positions — but the borrowed-scan dispatch reads - // `dict_is_attached()`, so a table left over from an earlier - // attach frame would send this frame's scan into the dual-base - // kernel, which reads every main-table entry as a virtual - // `dict_end + offset` position and would take raw ones for - // matches inside the dictionary. - self.dict.invalidate(); } self.table_pos_high_water = 0; // No copy-mode dict is resident across a reset: attach mode re-borrows @@ -1569,6 +1563,15 @@ impl FastKernelMatcher { /// `ms->loadedDictEnd`); a dictionary committed in several slices advances /// it to the running end each time, leaving the full size after the last. pub(crate) fn skip_matching_for_dict_copy(&mut self) { + // A dictionary table cached by an earlier ATTACH frame must not + // survive into this one. The borrowed-scan dispatch keys on + // `dict_is_attached()`, so leaving it would send this frame's scan + // into the dual-base kernel, which reads every main-table entry as a + // virtual `dict_end + offset` position — and this frame is writing + // raw ones. Dropping it here rather than at `reset` keeps the cache + // for the attach frames that live off it: rebuilding it per frame + // costs 12% on a reused 4 KiB dictionary frame (i9, wall clock). + self.dict.invalidate(); self.extend_history_with_pending(); self.prime_hash_table_for_dict_copy(); self.loaded_dict_end = self.history.len(); From b6ddedf8e8b6798debb6f9aa7f76b2fdfbe5b0af Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Mon, 7 Sep 2026 23:09:10 +0300 Subject: [PATCH 04/18] perf(encode): size the huffman reuse decision off the histogram The decision walked the literals TWICE, a byte and a dependent table load at a time, once for the previous table and once for the new one. It is the same sum read off the histogram the frame has already built: a symbol contributes its code length once per occurrence, so summing over at most 256 symbols gives the identical number. That is where upstream reads it from as well (HUF_estimateCompressedSize over count, huf_compress.c:1416-1417, after HUF_validateCTable checks representability off the same histogram). The two walks were the largest single item outside the matcher in a 4 KiB dictionary frame's profile. Output is byte-identical over 80 frames (five fixture shapes x eight levels x with and without a dictionary). --- zstd/src/encoding/blocks/compressed.rs | 12 +++++++-- zstd/src/encoding/blocks/compressed/tests.rs | 14 ++++++++++ zstd/src/huff0/huff0_encoder.rs | 27 ++++++++++++++++++++ 3 files changed, 51 insertions(+), 2 deletions(-) diff --git a/zstd/src/encoding/blocks/compressed.rs b/zstd/src/encoding/blocks/compressed.rs index 8bff87a76..cde72dd98 100644 --- a/zstd/src/encoding/blocks/compressed.rs +++ b/zstd/src/encoding/blocks/compressed.rs @@ -1013,6 +1013,7 @@ fn estimate_literals_section_bytes( last_huff.as_ref(), new_desc, literals, + counts, strategy, ); let reuse_payload = if !use_new { @@ -1276,12 +1277,18 @@ fn decide_huff_reuse_like_encoder( last_table: Option<&huff0_encoder::HuffmanTable>, new_desc: usize, literals: &[u8], + counts: &[usize; 256], strategy: crate::encoding::strategy::StrategyTag, ) -> bool { let Some(prev) = last_table else { return true; }; - let Some(old_estimate) = prev.estimate_compressed_size(literals) else { + // Off the histogram, not the literals: the same sum over at most 256 + // symbols instead of over every byte of the section, which is where + // upstream reads it from too (huf_compress.c:1416-1417). On a 4 KiB + // dictionary frame the two per-literal walks this replaces were the + // largest single item outside the matcher. + let Some(old_estimate) = prev.estimate_compressed_size_from_counts_checked(counts) else { return true; }; // Late-stage `HUF_flags_preferRepeat` mirror — kept here for @@ -1296,7 +1303,7 @@ fn decide_huff_reuse_like_encoder( return false; } let new_estimate = new_table - .estimate_compressed_size(literals) + .estimate_compressed_size_from_counts_checked(counts) .unwrap_or(literals.len()); !(old_estimate <= new_desc + new_estimate || new_desc + 12 >= literals.len()) } @@ -2819,6 +2826,7 @@ fn compress_literals( last_table, new_table_description_size, literals, + &counts, strategy, ); let encoder_table = if new_table { diff --git a/zstd/src/encoding/blocks/compressed/tests.rs b/zstd/src/encoding/blocks/compressed/tests.rs index 02140caf1..e1824121e 100644 --- a/zstd/src/encoding/blocks/compressed/tests.rs +++ b/zstd/src/encoding/blocks/compressed/tests.rs @@ -201,6 +201,17 @@ fn decide_huff_reuse_prefer_repeat_forces_reuse_for_fast_band() { .writeable_table_description_size() .expect("non-empty table emits a description"); + // The decision reads its sizes off the histogram of the very literals it + // is deciding for, so build one per fixture here as the encoder does. + let counts_of = |bytes: &[u8]| -> [usize; 256] { + let mut counts = [0usize; 256]; + for &b in bytes { + counts[b as usize] += 1; + } + counts + }; + let skewed_counts = counts_of(&skewed_literals); + // Distinguishing precondition: WITHOUT preferRepeat the // size comparison must prefer new (else the test isn't // exercising the override). Verify by running with a @@ -212,6 +223,7 @@ fn decide_huff_reuse_prefer_repeat_forces_reuse_for_fast_band() { Some(&prev), new_desc, &skewed_literals, + &skewed_counts, StrategyTag::Lazy, ), "fixture precondition: size-comparison must prefer new for Lazy on skewed literals" @@ -226,6 +238,7 @@ fn decide_huff_reuse_prefer_repeat_forces_reuse_for_fast_band() { Some(&prev), new_desc, &skewed_literals, + &skewed_counts, strategy, ), "{strategy:?} <= 1024 must short-circuit to reuse despite size-comparison favouring new" @@ -247,6 +260,7 @@ fn decide_huff_reuse_prefer_repeat_forces_reuse_for_fast_band() { Some(&prev), new_desc, &big_skewed, + &counts_of(&big_skewed), StrategyTag::Fast, ), "Fast at len > 1024 must NOT short-circuit (gate disabled), falls through to size heuristic" diff --git a/zstd/src/huff0/huff0_encoder.rs b/zstd/src/huff0/huff0_encoder.rs index ddc4e3b7e..c349e60f8 100644 --- a/zstd/src/huff0/huff0_encoder.rs +++ b/zstd/src/huff0/huff0_encoder.rs @@ -936,6 +936,33 @@ impl HuffmanTable { bits.div_ceil(8) + usize::from(bits.is_multiple_of(8)) } + /// [`Self::estimate_compressed_size`] read off the histogram instead of the + /// literals: the same sum, since a symbol contributes its code length once + /// per occurrence, but over at most 256 symbols rather than over every byte. + /// `None` when the table cannot encode a symbol that actually occurs, which + /// is the condition the per-literal form reports the same way. + /// + /// This is what upstream compares tables with (`HUF_estimateCompressedSize` + /// over `count`, huf_compress.c:1416-1417, after `HUF_validateCTable` has + /// checked representability off the same histogram). + pub(crate) fn estimate_compressed_size_from_counts_checked( + &self, + counts: &[usize], + ) -> Option { + let mut bits = 0usize; + for (symbol, &count) in counts.iter().enumerate() { + if count == 0 { + continue; + } + let (_, num_bits) = *self.codes.get(symbol)?; + if num_bits == 0 { + return None; + } + bits += num_bits as usize * count; + } + Some(bits.div_ceil(8) + usize::from(bits.is_multiple_of(8))) + } + pub fn build_from_weights(weights: &[usize]) -> Self { Self::build_from_weights_reusing(weights, None) } From d82ef8df48df35308c8d7853e3468c79dd388c6e Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Mon, 7 Sep 2026 23:20:54 +0300 Subject: [PATCH 05/18] perf(opt): clamp the pass match-length once per block, not per segment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sufficient_match_len_for_pass` is a block constant (the profile's value against `target_len`), and the DP body derived it on entry. On input the search finds nothing in, that body is entered once per LITERAL, so the clamp was an out-of-line call per literal — it shows up as its own symbol at 2.4% of a level-13 dictionary frame on random input. Both passes now clamp before their segment loop, and the body asserts the caller did. Byte-identical over 36 frames across the optimal band. --- zstd/src/encoding/hc/optimal.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/zstd/src/encoding/hc/optimal.rs b/zstd/src/encoding/hc/optimal.rs index a73de1bff..82e67c87b 100644 --- a/zstd/src/encoding/hc/optimal.rs +++ b/zstd/src/encoding/hc/optimal.rs @@ -63,8 +63,16 @@ macro_rules! build_optimal_plan_impl_body { let initial_reps = $initial_state.reps; let initial_litlen = $initial_state.litlen; let ldm_block_offset = $initial_state.block_offset; - let mut profile = $initial_state.profile; - profile.sufficient_match_len = $self.hc.sufficient_match_len_for_pass(profile); + // `sufficient_match_len` arrives already clamped for the pass: it is a + // block constant (the profile's value against `target_len`), and this + // body runs once per LITERAL on input the search finds nothing in, so + // deriving it here was an out-of-line call per literal. + let profile = $initial_state.profile; + debug_assert_eq!( + profile.sufficient_match_len, + $self.hc.sufficient_match_len_for_pass(profile), + "the caller must clamp sufficient_match_len for the pass", + ); // Const-fold from the strategy's associated `OPT_LEVEL` // (upstream zstd `optLevel`): BtOpt = 0, BtUltra / BtUltra2 = 2. // The two flags below are the only places the inner DP loop @@ -1062,7 +1070,10 @@ impl HcMatchGenerator { // SUFFICIENT_MATCH_LEN / ACCURATE_PRICE / FAVOR_SMALL_OFFSETS), // so the optimiser produces the literal at codegen time // without a runtime match. - let profile = HcOptimalCostProfile::const_for_strategy::(); + let mut profile = HcOptimalCostProfile::const_for_strategy::(); + // Clamped here, once for the block, rather than inside the per-segment + // DP body (which on input without matches is entered per literal). + profile.sufficient_match_len = self.hc.sufficient_match_len_for_pass(profile); // The DP bodies read the strategy's `FAVOR_SMALL_OFFSETS` const directly; // verify the runtime profile (built from the same strategy) agrees. debug_assert_eq!(profile.favor_small_offsets, S::FAVOR_SMALL_OFFSETS); @@ -1284,7 +1295,9 @@ impl HcMatchGenerator { // trait must be in scope to read its associated consts in // `run_seed_loop!`. use crate::encoding::strategy::Strategy; - let seed_profile = HcOptimalCostProfile::const_for_strategy::(); + let mut seed_profile = HcOptimalCostProfile::const_for_strategy::(); + // Same per-block clamp the main pass does; the DP body expects it done. + seed_profile.sufficient_match_len = self.hc.sufficient_match_len_for_pass(seed_profile); debug_assert_eq!(seed_profile.favor_small_offsets, S::FAVOR_SMALL_OFFSETS); let mut opt_state = core::mem::replace(&mut self.backend.bt_mut().opt_state, HcOptState::new()); From 53534d3556d55120a50670ada4356a5ed76c4241 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Mon, 7 Sep 2026 23:34:50 +0300 Subject: [PATCH 06/18] perf(row): index a written-off block at the stride the fast path uses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two paths had drifted to different answers for one question. A block the driver wrote off is not searched; the only reason to index it at all is that a LATER block may duplicate it, that duplicate is recognised on the seen-content grid and then searched, and the search sweeps positions — so an entry every stride bytes is met within a stride of scanning, immaterial against a block-sized match. The fast path reasons exactly that way and indexes every 512th position; the lazy/row path was still indexing every 8th, which is 131,000 stores per mebibyte of input that nothing will search, and it was 82% of the encode on a high-entropy megabyte. Wall clock, i9, three interleaved rounds: incompressible 1 MiB, level 5 0.223 s -> 0.052 s (1343 -> 5627 MB/s) 1 MiB repeated verbatim, L19 0.0446 s -> 0.0142 s Level 5 on that input is 287 us per frame against the reference's 400. Output is unchanged on 65 of 66 fixture-and-level rows — including the block-duplicate fixture at every level but 19, where the wider stride costs 506 bytes in 524,879 (0.1%). That fixture is a megabyte of random bytes repeated exactly; input that genuinely repeats compresses, so it never reaches this path at all. --- zstd/src/encoding/match_table/helpers.rs | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/zstd/src/encoding/match_table/helpers.rs b/zstd/src/encoding/match_table/helpers.rs index 2880439c7..e154468de 100644 --- a/zstd/src/encoding/match_table/helpers.rs +++ b/zstd/src/encoding/match_table/helpers.rs @@ -26,10 +26,24 @@ pub(crate) const MIN_MATCH_LEN: usize = 5; /// backfilling the suffix store. Upstream zstd parity: matches /// `ZSTD_FAST_HASH_FILL_STEP` in `zstd_fast.c`. pub(crate) const FAST_HASH_FILL_STEP: usize = 3; -/// Sparse step used when a block was determined to be incompressible — -/// every matcher inserts hash entries with this stride instead of the -/// per-byte dense pattern so the rest of the block costs less CPU. -pub(crate) const INCOMPRESSIBLE_SKIP_STEP: usize = 8; +/// Stride the lazy / row matchers index a block they wrote off unsearched at. +/// +/// Same question, same answer as the fast path's [`RAW_SKIP_INDEX_STEP`], which +/// this defers to: the block is not searched, so the only reason to index it is +/// a LATER block duplicating it, that duplicate is recognised on the seen-content +/// grid and then searched, and the search sweeps positions — so an entry every +/// stride bytes is met within a stride of scanning, immaterial against a +/// block-sized match. The two paths had drifted to different answers (8 here, +/// 512 there) and it was the whole cost of a skip: an entry per eight bytes is +/// 131,000 stores per mebibyte of input nothing will search. +/// +/// Measured on the i9, wall clock, three interleaved rounds. Incompressible +/// 1 MiB at level 5: 0.223 s -> 0.052 s (1343 -> 5627 MB/s). A 1 MiB block +/// repeated verbatim at level 19: 0.0446 s -> 0.0142 s. Output is unchanged on +/// 65 of 66 fixture-and-level rows, including the block-duplicate one at every +/// level but 19, where it costs 506 bytes in 524,879 (0.1%). +pub(crate) const INCOMPRESSIBLE_SKIP_STEP: usize = + crate::encoding::incompressible::RAW_SKIP_INDEX_STEP; /// Length of the common prefix of two byte slices, capped at /// `min(a.len(), b.len())`. Hot path on every match finder; dispatches to From fb0a0dfff420cd07b21d1721197ef2016656c8c3 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Mon, 7 Sep 2026 23:46:12 +0300 Subject: [PATCH 07/18] perf(opt): walk a run of no-match positions inside the parser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upstream's optimal loop takes a position the search finds nothing at as one literal and moves on INSIDE its own loop (ZSTD_compressBlock_opt_generic, if (!nbMatches) { ip++; continue; }). Ours returned it to the caller, so on input the search finds nothing in — which is every position there — the caller re-entered the DP body per literal and paid its 440-byte frame each time. The hot instructions of that body on such input were its prologue and epilogue, not any loop in it. The run is now walked in place and handed back whole. The number of searches is unchanged: the run stops at the first position with candidates and the caller re-enters there. That position must NOT be searched twice — the search inserts it into the binary tree, and inserting one position twice corrupts the tree — so the run records what it searched and the re-entry reads the answer out of the candidate buffer instead of asking again. Skipped when the LDM producer is active: its state machine is rebuilt per call from the segment's block offset, so advancing inside one call is not the same thing. HAS_LDM is a const generic, so that folds away. Byte-identical over 36 fixture-and-level rows across the optimal band. --- zstd/src/encoding/bt/mod.rs | 1 + zstd/src/encoding/hc/optimal.rs | 116 +++++++++++++++++++++++++++----- zstd/src/encoding/opt/types.rs | 11 +++ 3 files changed, 110 insertions(+), 18 deletions(-) diff --git a/zstd/src/encoding/bt/mod.rs b/zstd/src/encoding/bt/mod.rs index 9af77f9c1..3c3ffa4b8 100644 --- a/zstd/src/encoding/bt/mod.rs +++ b/zstd/src/encoding/bt/mod.rs @@ -375,6 +375,7 @@ impl BtMatcher { mut candidates, store, price_arena, + candidates_searched_at: _, } = buffers; candidates.clear(); self.opt_nodes_scratch = nodes; diff --git a/zstd/src/encoding/hc/optimal.rs b/zstd/src/encoding/hc/optimal.rs index 82e67c87b..413594030 100644 --- a/zstd/src/encoding/hc/optimal.rs +++ b/zstd/src/encoding/hc/optimal.rs @@ -106,8 +106,16 @@ macro_rules! build_optimal_plan_impl_body { candidates, store, price_arena, + candidates_searched_at: searched_at, } = &mut *$buffers; - candidates.clear(); + // The run this call re-enters on already searched this position and left + // its answer in `candidates`; searching again would insert the position + // into the binary tree a second time, so keep the buffer as it stands. + let carried_candidates = *searched_at == Some(($current_abs_start, initial_litlen)); + *searched_at = None; + if !carried_candidates { + candidates.clear(); + } store.clear(); // Price-cache slices + monotonic stamps feed ONLY the priced paths (the // matched-seed block and the forward DP loop), both of which run only @@ -134,6 +142,70 @@ macro_rules! build_optimal_plan_impl_body { // `!candidates.is_empty()` block before any reader. let mut ll0_price = 0u32; let mut ll1_price = 0u32; + // A position the search finds nothing at is one literal (upstream zstd + // `ZSTD_compressBlock_opt_generic`: `if (!nbMatches) { ip++; continue; }` + // — inside its own loop). We return it to the caller instead, and on + // input the search finds nothing in that is EVERY position, so the + // caller re-entered this body per literal and paid its stack frame each + // time. Walk the run here and hand back the whole run. + // + // The number of searches is unchanged: the run stops at the first + // position with candidates and those candidates are the ones the seed + // below then uses. The bound is the caller's own re-entry condition + // (it enters while more than `HASH_READ_SIZE` bytes remain), so the + // cursor lands exactly where the per-literal returns would have left it. + // + // Skipped when the LDM producer is active: its state machine is rebuilt + // per call from the segment's block offset, so advancing inside one call + // is not the same thing. `HAS_LDM` is a const generic, so this whole + // block folds away there. + let mut skipped_literals = 0usize; + let mut seed_candidates_ready = carried_candidates; + if !HAS_LDM && !seed_candidates_ready { + while $current_len - skipped_literals > 8 { + candidates.clear(); + // SAFETY: as the seed below — the wrapper shares the `$collect` + // kernel's target_feature umbrella, entered under the runtime + // detector. + unsafe { + $self.$collect::<$strategy_ty>( + $current_abs_start + skipped_literals, + current_abs_end, + profile, + HcCandidateQuery { + reps: initial_reps, + lit_len: initial_litlen + skipped_literals, + ldm_candidate: None, + }, + &mut *candidates, + ) + }; + if !candidates.is_empty() { + if skipped_literals > 0 { + // Hand the answer to the call that re-enters here. + *searched_at = Some(( + $current_abs_start + skipped_literals, + initial_litlen + skipped_literals, + )); + } else { + seed_candidates_ready = true; + } + break; + } + skipped_literals += 1; + } + if skipped_literals > 0 { + // Literals only: no sequence was emitted and the repcodes are + // untouched, exactly as the per-literal returns left them. The + // price is discarded by the caller. + return ( + 0u32, + initial_reps, + initial_litlen + skipped_literals, + skipped_literals, + ); + } + } let mut pos = 1usize; let mut last_pos = 0usize; let mut forced_end: Option = None; @@ -195,23 +267,28 @@ macro_rules! build_optimal_plan_impl_body { } else { None }; - candidates.clear(); - // SAFETY: wrapper is in the same target_feature umbrella as the - // `$collect` kernel variant; the runtime kernel detector already - // gated entry into the wrapper. - unsafe { - $self.$collect::<$strategy_ty>( - $current_abs_start, - current_abs_end, - profile, - HcCandidateQuery { - reps: initial_reps, - lit_len: initial_litlen, - ldm_candidate: seed_ldm, - }, - &mut *candidates, - ) - }; + // The no-match run above already searched this exact position with + // this exact query and left its candidates in the buffer, so the + // seed reads them rather than repeating the search. + if !seed_candidates_ready { + candidates.clear(); + // SAFETY: wrapper is in the same target_feature umbrella as the + // `$collect` kernel variant; the runtime kernel detector already + // gated entry into the wrapper. + unsafe { + $self.$collect::<$strategy_ty>( + $current_abs_start, + current_abs_end, + profile, + HcCandidateQuery { + reps: initial_reps, + lit_len: initial_litlen, + ldm_candidate: seed_ldm, + }, + &mut *candidates, + ) + }; + } if !candidates.is_empty() { // Deferred price-cache setup: the arena slices are two disjoint // STRIDE-wide regions of `price_arena` (LL, ML); the fixed STRIDE @@ -1534,6 +1611,9 @@ impl HcMatchGenerator { candidates, store, price_arena, + // Nothing in the buffer answers a query yet: the block that filled + // it is over, and the parser is about to start another. + candidates_searched_at: None, } } diff --git a/zstd/src/encoding/opt/types.rs b/zstd/src/encoding/opt/types.rs index 17214855d..c88d191f9 100644 --- a/zstd/src/encoding/opt/types.rs +++ b/zstd/src/encoding/opt/types.rs @@ -108,4 +108,15 @@ pub(crate) struct HcOptimalPlanBuffers { /// `frontier_limit`-dependent) so the generation stamps land in the /// same cell across calls with different frontiers. pub(crate) price_arena: alloc::boxed::Box<[[u32; 2]]>, + /// `(position, literal length)` the candidates in [`Self::candidates`] were + /// searched for, when they are still the answer to that exact query. + /// + /// The parser walks a run of positions the search finds nothing at inside + /// one call and hands the run back to its caller, which re-enters at the + /// position that DID have candidates. Searching it a second time is not + /// merely wasted: the search inserts the position into the binary tree, and + /// inserting one position twice corrupts it. So the run records what it + /// searched and the re-entry reads the answer instead of asking again. + /// `None` whenever the buffer's contents do not answer any query. + pub(crate) candidates_searched_at: Option<(usize, usize)>, } From a30ed3f7fa9a103812cc7240fee6917c8e490ce7 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 8 Sep 2026 08:46:58 +0300 Subject: [PATCH 08/18] perf(bt): fold the window-floor helper into its caller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two field reads and a clamp, taken once per searched position the way upstream takes ZSTD_getLowestMatchIndex — but it was standing in the profile as its own symbol, so it was paying a call and a return for four instructions of work. Its neighbours in the same file already carry the attribute; this one had been missed. Byte-identical over 24 fixture-and-level rows. --- zstd/src/encoding/match_table/storage.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/zstd/src/encoding/match_table/storage.rs b/zstd/src/encoding/match_table/storage.rs index 392f90fb3..04cae6f48 100644 --- a/zstd/src/encoding/match_table/storage.rs +++ b/zstd/src/encoding/match_table/storage.rs @@ -1339,6 +1339,12 @@ impl MatchTable { /// Lower bound (in absolute positions) of the window that's still /// reachable from `target_abs`. Upstream zstd parity: `windowLow` in /// `ZSTD_compressBlock_*`. + /// + /// Two field reads and a clamp, called once per searched position as + /// upstream calls `ZSTD_getLowestMatchIndex` — and it was being called, not + /// folded: it stood in the profile as its own symbol, paying a call and a + /// return for four instructions of work. + #[inline(always)] pub(crate) fn window_low_abs_for_target(&self, target_abs: usize) -> usize { let history_low = self.history_abs_start; let window_low = target_abs.saturating_sub(self.max_window_size); From 57d59c50f916dce7f2ed9345043c471f84590710 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 8 Sep 2026 09:28:01 +0300 Subject: [PATCH 09/18] fix(opt): advance the tree cursor from where a dictionary match ends After a search the parser jumps its lazy tree-insert cursor to where the match it found ENDS, on the reasoning that positions inside a match are covered. That end is the end of the match in the SOURCE (upstream `matchEndIdx = matchIndex + matchLength`, zstd_opt.c:747-748 for the live walk and :794-795 for the dictionary one), and the live walk here already used that form. The dictionary walk measured it from the position being SEARCHED instead. A dictionary candidate sits before that position, so the cursor ran ahead by the offset and the positions it passed never entered the tree. A later search then finds an empty bucket where the reference finds a long match. It shows only with a dictionary attached, because only a dictionary candidate reaches back far enough, and hardest where the search is shallowest: upstream resolves level 11 at 4 KiB to btopt with `searchLog` 3, so a search gets eight candidates and cannot afford an empty bucket. Traced on the benchmark's `small-4k-log-lines` scenario: a dictionary match at position 319 (offset 161, length 42) moved the cursor to 353 where upstream moves it to 192, so positions 200-353 were never indexed, and the search at 701 walked one node and stopped. The reference codes the rest of that block as a single 3695-byte match; we spent four sequences on the same span. `compare_ffi` REPORT_DICT on that scenario, ours against the reference: level_11_lazy 52 -> 45 bytes against 46, level_12_lazy 51 -> 45 against 46. Both now come in under it; level_10 and level_13 are unchanged. No-dict output is byte-identical over 24 fixture-and-level rows, and no dictionary row anywhere in the sweep grew. Also widened the cparams parity grid to every level: the sampled list skipped 11 and 13, and a level row is exactly the kind of thing that can be wrong on its own while its neighbours are right. It passes, which is what ruled the resolved parameters out as the cause here. Closes #495 --- ffi-bench/tests/dictionary_ffi.rs | 75 +++++++++++++++++++++++++++++++ ffi-bench/tests/zz_cparams.rs | 8 +++- zstd/src/encoding/hc/generator.rs | 14 +++++- 3 files changed, 95 insertions(+), 2 deletions(-) diff --git a/ffi-bench/tests/dictionary_ffi.rs b/ffi-bench/tests/dictionary_ffi.rs index b4ca3ece4..7075484ba 100644 --- a/ffi-bench/tests/dictionary_ffi.rs +++ b/ffi-bench/tests/dictionary_ffi.rs @@ -122,3 +122,78 @@ fn dict_frames_decode_with_c_across_levels_and_reuse() { } } } + +/// A dictionary frame on the optimal band must not come out LARGER than the +/// reference's, on input the dictionary describes well. +/// +/// The parser walks a binary tree it fills lazily: after a search it jumps its +/// insert cursor to where the match it found ENDS, on the reasoning that the +/// positions inside a match are covered. That end is the end of the match in +/// the SOURCE (upstream `matchEndIdx = matchIndex + matchLength`, +/// zstd_opt.c:747-748 and :794-795), and for a candidate drawn from the +/// dictionary the source lies BEFORE the position being searched. Measuring the +/// jump from the searched position instead skips the positions in between, they +/// never enter the tree, and a later search finds an empty bucket where the +/// reference finds a long match. It only shows with a dictionary attached, +/// because only a dictionary candidate sits that far back. +/// +/// Level 11 at 4 KiB is where it bites hardest: upstream resolves it to btopt +/// with `searchLog` 3, so a search gets eight candidates and cannot afford to +/// look in an empty bucket. +#[test] +fn dict_frames_on_the_optimal_band_are_no_larger_than_the_reference() { + use structured_zstd::decoding::Dictionary; + use structured_zstd::encoding::{CompressionLevel, FrameCompressor}; + + // The benchmark's `small-4k-log-lines` scenario, byte for byte: its lines + // carry a region field the shorter fixture above does not, and which long + // matches the dictionary offers is exactly what this pins. + const LINES: &[&str] = &[ + "ts=2026-03-26T21:39:28Z level=INFO msg=\"flush memtable\" tenant=demo table=orders region=eu-west\n", + "ts=2026-03-26T21:39:29Z level=INFO msg=\"rotate segment\" tenant=demo table=orders region=eu-west\n", + "ts=2026-03-26T21:39:30Z level=INFO msg=\"compact level\" tenant=demo table=orders region=eu-west\n", + "ts=2026-03-26T21:39:31Z level=INFO msg=\"write block\" tenant=demo table=orders region=eu-west\n", + ]; + let payload = { + let mut bytes = Vec::with_capacity(4 * 1024); + while bytes.len() < 4 * 1024 { + for line in LINES { + let remaining = (4 * 1024) - bytes.len(); + if remaining == 0 { + break; + } + bytes.extend_from_slice(&line.as_bytes()[..line.len().min(remaining)]); + } + } + bytes + }; + // Trained the way the benchmark trains it: 256-byte samples, an eighth of + // the input as the size request. + let samples: Vec<&[u8]> = payload.chunks(256).collect(); + let dict = zstd::dict::from_samples(&samples, payload.len() / 8) + .expect("dictionary should train from the log-line samples"); + + for level in [10i32, 11, 12, 13] { + let mut cctx: FrameCompressor = FrameCompressor::new(CompressionLevel::Level(level)); + cctx.set_dictionary_id_flag(false); + cctx.set_dictionary( + Dictionary::from_serialized_or_raw_content(dict.as_slice()).expect("dictionary parses"), + ) + .expect("attach dict"); + cctx.set_source_size_hint(payload.len() as u64); + let ours = cctx.compress_independent_frame(&payload); + + let mut reference = zstd::bulk::Compressor::with_dictionary(level, dict.as_slice()) + .expect("reference accepts the dictionary"); + let theirs = reference + .compress(&payload) + .expect("reference compresses the payload"); + + assert!( + ours.len() <= theirs.len(), + "level {level}: {} bytes against the reference's {}", + ours.len(), + theirs.len(), + ); + } +} diff --git a/ffi-bench/tests/zz_cparams.rs b/ffi-bench/tests/zz_cparams.rs index bbe321db6..74dbb522b 100644 --- a/ffi-bench/tests/zz_cparams.rs +++ b/ffi-bench/tests/zz_cparams.rs @@ -21,7 +21,13 @@ fn reference_cparams(level: i32, src: u64, dict: usize) -> (u32, u32, u32, u32, #[test] fn cparams_match_reference_over_grid() { - let levels = [-7, -5, -3, -1, 0, 1, 2, 3, 4, 5, 7, 10, 12, 15, 17, 19, 22]; + // Every level, not a sample of them: the sampled list skipped 11 and 13, + // and a level row is exactly the kind of thing that can be wrong on its own + // while its neighbours are right. + let levels = [ + -7, -6, -5, -4, -3, -2, -1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, + 18, 19, 20, 21, 22, + ]; let srcs: [u64; 9] = [0, 1, 100, 4096, 6806, 16_384, 100_000, 131_072, 5_000_000]; let dicts: [usize; 5] = [0, 437, 2048, 65_536, 1_000_000]; let mut mismatches = 0usize; diff --git a/zstd/src/encoding/hc/generator.rs b/zstd/src/encoding/hc/generator.rs index ba5bf90d2..8702f4838 100644 --- a/zstd/src/encoding/hc/generator.rs +++ b/zstd/src/encoding/hc/generator.rs @@ -957,7 +957,19 @@ macro_rules! bt_insert_and_collect_matches_body { ); if accepted { best_len = match_len; - let candidate_end = $abs_pos + match_len; + // Where the match ends in the SOURCE, not at the + // position being searched (upstream zstd + // `matchEndIdx = matchIndex + matchLength`, + // zstd_opt.c:794-795, the same form the live walk above + // uses). This value becomes the tree's insert cursor, + // and a dictionary candidate sits BEFORE the searched + // position: measuring from the searched position + // instead pushes the cursor forward by the offset, and + // every position it skipped never enters the tree. A + // later search then finds an empty bucket where the + // reference finds a long match. Only a dictionary + // candidate reaches back far enough for it to show. + let candidate_end = dict_idx + match_len; if candidate_end > match_end_abs { match_end_abs = candidate_end; } From 6d185fd9d355c7caef20bcf0cbbc16332e579b73 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 8 Sep 2026 09:37:27 +0300 Subject: [PATCH 10/18] docs(decode): record why the dictionary repeat path stays cold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Taking the attribute off is worth 2.7% on a dictionary decode, where it is the common path (23 matches a frame on the benchmark's small-10k-random scenario), and costs 3.5% on an ordinary decode, where it is not taken at all and only its placement matters. Measured on the i9, wall clock, three interleaved rounds of prebuilt binaries, with instruction counts unchanged either way — so the difference is code placement, and the ordinary decode is the path that runs far more often. --- zstd/src/decoding/decode_buffer.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/zstd/src/decoding/decode_buffer.rs b/zstd/src/decoding/decode_buffer.rs index c00a88061..91fc15fdc 100644 --- a/zstd/src/decoding/decode_buffer.rs +++ b/zstd/src/decoding/decode_buffer.rs @@ -818,6 +818,9 @@ impl DecodeBuffer { } } + // Cold on purpose, and measured: taking the attribute off buys 2.7% on a + // dictionary decode (where this IS the common path, 23 matches a frame) and + // costs 3.5% on an ordinary one, which is the path that runs far more often. #[cold] fn repeat_from_dict( &mut self, From d7ebb608f25f4fa283f44c8e9d6a6c53111efd27 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 8 Sep 2026 09:44:56 +0300 Subject: [PATCH 11/18] perf(decode): outline the dictionary match tail (measurement pending) --- zstd/src/decoding/decode_buffer.rs | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/zstd/src/decoding/decode_buffer.rs b/zstd/src/decoding/decode_buffer.rs index 91fc15fdc..390b48607 100644 --- a/zstd/src/decoding/decode_buffer.rs +++ b/zstd/src/decoding/decode_buffer.rs @@ -879,7 +879,11 @@ impl DecodeBuffer { self.buffer.extend(dict_slice); self.total_output_counter += bytes_from_dict as u64; - return self.repeat(dict, self.buffer.len(), match_length - bytes_from_dict); + return self.repeat_tail_after_dict( + dict, + self.buffer.len(), + match_length - bytes_from_dict, + ); } else { let low = dict_len - bytes_from_dict; let high = low + match_length; @@ -897,6 +901,27 @@ impl DecodeBuffer { } } + /// The part of a match that continues out of the dictionary and into the + /// output already produced. + /// + /// Its own function, and deliberately not inlined: `repeat_inner` is + /// `inline(always)`, so calling it from the dictionary path pulled the whole + /// copy machinery — overlapping copies, the wildcopy variants, their error + /// paths — into that path's body. Every dictionary match then paid the + /// prologue and epilogue of a frame sized for code most of them never run, + /// and on a dictionary-heavy frame that is the common path (23 matches a + /// frame on the benchmark's small-10k-random scenario, at 112 instructions + /// a call). Behind a call the frame belongs to the tail alone. + #[inline(never)] + fn repeat_tail_after_dict( + &mut self, + dict: Option<&crate::decoding::dictionary::Dictionary>, + offset: usize, + match_length: usize, + ) -> Result<(), DecodeBufferError> { + self.repeat_inner::(dict, offset, match_length) + } + /// Check if and how many bytes can currently be drawn from the buffer pub fn can_drain_to_window_size(&self) -> Option { if self.buffer.len() > self.window_size { From 7ee831d8aed5eeab9704a509345a0ae6f085a8bd Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 8 Sep 2026 09:59:29 +0300 Subject: [PATCH 12/18] fix(opt): keep the dictionary match end in absolute coordinates `dict_idx` indexes the live history, while `match_end_abs` and the tree's insert cursor are absolute. A reused dictionary context advances `history_abs_start` between frames, so from the second frame on the comparison put a small relative end against an absolute one, the cursor never advanced, and the parser went back to inserting every covered position: the work this was meant to skip. The first frame hid it, because the base is zero there. The invariant is now asserted in debug beside the computation, and it fires on the reused-context test with the relative form restored, so the whole debug suite carries the check rather than one fixture. The optimal-band ratio test also compresses three frames on ONE compressor now, which is the shape that moves the base. --- ffi-bench/tests/dictionary_ffi.rs | 24 +++++++++++++++--------- zstd/src/decoding/decode_buffer.rs | 2 +- zstd/src/encoding/hc/generator.rs | 19 ++++++++++++++++++- 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/ffi-bench/tests/dictionary_ffi.rs b/ffi-bench/tests/dictionary_ffi.rs index 7075484ba..df14779da 100644 --- a/ffi-bench/tests/dictionary_ffi.rs +++ b/ffi-bench/tests/dictionary_ffi.rs @@ -180,20 +180,26 @@ fn dict_frames_on_the_optimal_band_are_no_larger_than_the_reference() { Dictionary::from_serialized_or_raw_content(dict.as_slice()).expect("dictionary parses"), ) .expect("attach dict"); - cctx.set_source_size_hint(payload.len() as u64); - let ours = cctx.compress_independent_frame(&payload); - let mut reference = zstd::bulk::Compressor::with_dictionary(level, dict.as_slice()) .expect("reference accepts the dictionary"); let theirs = reference .compress(&payload) .expect("reference compresses the payload"); - assert!( - ours.len() <= theirs.len(), - "level {level}: {} bytes against the reference's {}", - ours.len(), - theirs.len(), - ); + // Three frames on the SAME compressor. A reused dictionary context + // advances the history base between frames, so anything the parser + // keeps in absolute coordinates has to carry that base: a value left + // relative to the live history reads correctly on the first frame and + // silently stops working on the second. + for frame in 0..3 { + cctx.set_source_size_hint(payload.len() as u64); + let ours = cctx.compress_independent_frame(&payload); + assert!( + ours.len() <= theirs.len(), + "level {level} frame {frame}: {} bytes against the reference's {}", + ours.len(), + theirs.len(), + ); + } } } diff --git a/zstd/src/decoding/decode_buffer.rs b/zstd/src/decoding/decode_buffer.rs index 390b48607..d4af60b1f 100644 --- a/zstd/src/decoding/decode_buffer.rs +++ b/zstd/src/decoding/decode_buffer.rs @@ -919,7 +919,7 @@ impl DecodeBuffer { offset: usize, match_length: usize, ) -> Result<(), DecodeBufferError> { - self.repeat_inner::(dict, offset, match_length) + self.repeat(dict, offset, match_length) } /// Check if and how many bytes can currently be drawn from the buffer diff --git a/zstd/src/encoding/hc/generator.rs b/zstd/src/encoding/hc/generator.rs index 8702f4838..10acb4715 100644 --- a/zstd/src/encoding/hc/generator.rs +++ b/zstd/src/encoding/hc/generator.rs @@ -969,7 +969,24 @@ macro_rules! bt_insert_and_collect_matches_body { // later search then finds an empty bucket where the // reference finds a long match. Only a dictionary // candidate reaches back far enough for it to show. - let candidate_end = dict_idx + match_len; + // + // In ABSOLUTE coordinates, which is what `match_end_abs` + // and the cursor are in: `dict_idx` indexes the live + // history, and a reused dictionary context advances + // `history_abs_start`, so without the base this compares + // a small relative end against an absolute one and never + // advances the cursor at all from the second frame on. + let candidate_end = $table.history_abs_start + dict_idx + match_len; + // Same coordinate space as `match_end_abs` and the + // cursor it feeds. A value left relative to the live + // history satisfies this only while the base is zero, + // which is exactly the first frame of a context — the + // case where a reused dictionary context hides the + // mistake until the base moves. + debug_assert!( + candidate_end >= $table.history_abs_start + match_len, + "dictionary match end must be absolute", + ); if candidate_end > match_end_abs { match_end_abs = candidate_end; } From 7e3afd880b47e4e875181116fd7077c37c11f01f Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 8 Sep 2026 10:26:36 +0300 Subject: [PATCH 13/18] docs(match): record the skip stride against the reference, not just against us MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The evidence for the wider stride was ours-before against ours-after: no same-run reference figure, and no instruction counts, on either fixture — including the repeated-block one whose output the change grows. Taken now, three arms in one session (before, after, and the C reference through `ffi_encode_loop_z000033`), `perf stat -r 3`, three rounds each. Incompressible 1 MiB at level 5: cycles 1.66 G -> 0.34 G against the reference's 1.03 G, instructions 1.93 G -> 0.30 G against 0.83 G — from 1.58x of the reference to 0.33x. The 1 MiB block repeated verbatim at level 19: 0.82 G -> 0.083 G cycles against the reference's 12.7 G, and it is the row that costs bytes: 524,365 -> 524,871 against the reference's 524,361, so 510 bytes in 524,871 (0.1%) while running 150 times faster than it. The constant now carries those tables instead of the internal before/after pair. --- zstd/src/encoding/match_table/helpers.rs | 30 ++++++++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/zstd/src/encoding/match_table/helpers.rs b/zstd/src/encoding/match_table/helpers.rs index e154468de..b29cda396 100644 --- a/zstd/src/encoding/match_table/helpers.rs +++ b/zstd/src/encoding/match_table/helpers.rs @@ -37,11 +37,31 @@ pub(crate) const FAST_HASH_FILL_STEP: usize = 3; /// 512 there) and it was the whole cost of a skip: an entry per eight bytes is /// 131,000 stores per mebibyte of input nothing will search. /// -/// Measured on the i9, wall clock, three interleaved rounds. Incompressible -/// 1 MiB at level 5: 0.223 s -> 0.052 s (1343 -> 5627 MB/s). A 1 MiB block -/// repeated verbatim at level 19: 0.0446 s -> 0.0142 s. Output is unchanged on -/// 65 of 66 fixture-and-level rows, including the block-duplicate one at every -/// level but 19, where it costs 506 bytes in 524,879 (0.1%). +/// Measured on the i9, three arms in one session (before, after, and the C +/// reference through `ffi_encode_loop_z000033`), `perf stat -r 3`, three rounds +/// each. Per run, cycles / instructions / wall clock: +/// +/// Incompressible 1 MiB at level 5, 300 frames: +/// +/// | arm | cycles | instructions | wall | +/// |---|---|---|---| +/// | before | 1.653-1.670 G | 1.9287 G | 0.402-0.408 s | +/// | after | 0.337-0.347 G | 0.3028 G | 0.087-0.089 s | +/// | reference | 1.007-1.068 G | 0.8330 G | 0.247-0.267 s | +/// +/// A 1 MiB block repeated verbatim at level 19, 30 frames — the case the wider +/// stride costs bytes on: +/// +/// | arm | cycles | instructions | wall | bytes | +/// |---|---|---|---|---| +/// | before | 0.807-0.839 G | 0.6231 G | 0.206-0.213 s | 524,365 | +/// | after | 0.082-0.084 G | 0.0776 G | 0.029-0.030 s | 524,871 | +/// | reference | 12.20-13.08 G | 5.5298 G | 2.95-3.16 s | 524,361 | +/// +/// So the stride takes us from 1.58x of the reference to 0.33x on the first, +/// and the second costs 510 bytes in 524,871 (0.1%) against the reference while +/// running 150 times faster than it. Output is unchanged on 65 of 66 +/// fixture-and-level rows; that level-19 row is the only one that moves. pub(crate) const INCOMPRESSIBLE_SKIP_STEP: usize = crate::encoding::incompressible::RAW_SKIP_INDEX_STEP; From a2af75a440ac52a3d17dcb0344f6cd4db380b690 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 8 Sep 2026 11:10:37 +0300 Subject: [PATCH 14/18] test(encode): cover the copy-mode dictionary fill's boundaries and widths The coverage report named four lines in the copy-mode fill that nothing reached: its two early returns and the hash-width arms past 4. - A dictionary shorter than one hash read leaves instead of computing `history.len() - HASH_READ_SIZE`, and still records the boundary. - A slice carrying nothing hashable past the stride cursor indexes nothing, rather than walking the same positions again. That is also the shape a history shrunk by eviction leaves behind. - The fill runs at every width the table accepts (4 through 8): the widths are a `match`, so only the ones a test builds are exercised, and 4 alone left the rest cold. What remains uncovered there is the `unreachable!()` arm, which the table's constructor makes unreachable by rejecting any other width. --- .../src/encoding/simple/fast_matcher/tests.rs | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/zstd/src/encoding/simple/fast_matcher/tests.rs b/zstd/src/encoding/simple/fast_matcher/tests.rs index e70a6abfd..089353f53 100644 --- a/zstd/src/encoding/simple/fast_matcher/tests.rs +++ b/zstd/src/encoding/simple/fast_matcher/tests.rs @@ -648,6 +648,65 @@ fn copy_mode_dictionary_fill_keeps_the_upstream_occurrence_per_bucket() { ); } +/// A dictionary shorter than one hash read indexes nothing, and says so by +/// leaving rather than by computing `history.len() - HASH_READ_SIZE` and +/// underflowing. +#[test] +fn copy_mode_dictionary_fill_leaves_a_dictionary_too_short_to_hash() { + let mut m = FastKernelMatcher::with_params(12, 12, 4, 2); + m.accept_data(alloc::vec![0xABu8; 5]); + m.skip_matching_for_dict_copy(); + assert_eq!( + m.dict_copy_fill_next, 0, + "nothing is hashable, so the stride has nowhere to resume from", + ); + // The boundary is still recorded: the bytes ARE the dictionary, whether or + // not any position in them could be indexed. + assert_eq!(m.loaded_dict_end, m.history.len()); +} + +/// A dictionary committed in slices resumes its stride where the last slice +/// left it, and a slice that carries nothing hashable past that point indexes +/// nothing rather than walking the same positions again. +#[test] +fn copy_mode_dictionary_fill_indexes_nothing_when_the_stride_is_already_past() { + let dict: alloc::vec::Vec = b"abcd".iter().copied().cycle().take(64).collect(); + let mut m = FastKernelMatcher::with_params(12, 12, 4, 2); + m.accept_data(dict); + m.skip_matching_for_dict_copy(); + let after_first = m.dict_copy_fill_next; + assert!( + after_first > m.history.len() - 8, + "the last stride group was partial, so the cursor sits past the last \ + hashable position (at {after_first})", + ); + + // A further slice that adds no bytes: the cursor is already past what the + // history makes hashable, which is the guard's case. It is also what a + // history that shrank under eviction would leave behind. + m.accept_data(alloc::vec::Vec::new()); + m.skip_matching_for_dict_copy(); + assert_eq!(m.dict_copy_fill_next, after_first); +} + +/// Every hash width the Fast table accepts gets the same fill. The widths are +/// a `match` over `mls`, so only the ones a test actually builds are compiled +/// through — 4 alone would leave the rest unexercised. +#[test] +fn copy_mode_dictionary_fill_runs_at_every_hash_width() { + for mls in 4u32..=8 { + let dict: alloc::vec::Vec = (0..96u8).map(|b| b.wrapping_mul(7)).collect(); + let mut m = FastKernelMatcher::with_params(12, 14, mls, 2); + m.accept_data(dict); + m.skip_matching_for_dict_copy(); + assert!( + m.dict_copy_fill_next > 0, + "mls {mls}: the fill must have advanced its stride", + ); + assert_eq!(m.loaded_dict_end, m.history.len(), "mls {mls}"); + } +} + /// Boundary: pending block too short to hash anything (less than /// `HASH_READ_SIZE` bytes). The dict-prime path must early-return /// without panicking on the `last_hashable` subtract. From be434ae4c8e40933a7a615aeaf15f3a1cb8b4e55 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 8 Sep 2026 11:35:10 +0300 Subject: [PATCH 15/18] perf(encode): set the attach dictionary table aside on a copy frame A copy-mode frame must not reach the attached table, and said so by discarding it. A compressor whose source sizes cross the attach cutoff then rebuilt that table on every attach frame it came back to, which the neighbouring note already prices at 12% of a reused 4 KiB dictionary frame. It is set aside instead: while it is away every reader sees exactly the state a discard leaves, so no frame's output can depend on it still existing, and the next attach prime takes it back and runs the same shape check it would have run on a table that never left. A dictionary change still goes through `invalidate`, which drops the stash with it. Byte-identical over 24 fixture-and-level rows against the branch tip. The dictionary encode loop grows an `alt` argument that alternates each frame between the input and its first N bytes, so the mode-switching shape this is about can be measured rather than argued about. --- zstd/examples/encode_loop_dict.rs | 18 ++++++++-- zstd/src/decoding/decode_buffer.rs | 34 +++++------------- zstd/src/encoding/blocks/compressed.rs | 15 ++++++++ zstd/src/encoding/dict_attach.rs | 46 ++++++++++++++++++++++++ zstd/src/encoding/simple/fast_matcher.rs | 25 ++++++++----- 5 files changed, 101 insertions(+), 37 deletions(-) diff --git a/zstd/examples/encode_loop_dict.rs b/zstd/examples/encode_loop_dict.rs index ebcf35f56..097d5fabb 100644 --- a/zstd/examples/encode_loop_dict.rs +++ b/zstd/examples/encode_loop_dict.rs @@ -82,8 +82,18 @@ fn main() { let iters: u32 = args.get(2).and_then(|s| s.parse().ok()).unwrap_or(20_000); let input_spec: &str = args.get(3).map(|s| s.as_str()).unwrap_or("logs4096"); let dict_path: Option<&str> = args.get(4).map(|s| s.as_str()); + // 6th arg `alt`: alternate each frame between the input and its first N + // bytes. A dictionary resolves attach-vs-copy from the source size, so two + // sizes either side of that cutoff make the compressor switch modes every + // frame — the shape that shows what switching costs, and the one a caller + // with variable-sized records actually has. + let alt_len: Option = args + .get(5) + .and_then(|s| s.strip_prefix("alt")) + .and_then(|n| n.parse().ok()); let src = resolve_input(input_spec); + let alt = alt_len.map(|n| src[..n.min(src.len())].to_vec()); // One reused compressor: matcher tables + any dictionary parse happen // once here, mirroring a consumer that reuses a context across N @@ -104,8 +114,12 @@ fn main() { // zero output allocation. let mut out: Vec = Vec::new(); let mut sink: usize = 0; - for _ in 0..iters { - cctx.compress_independent_frame_into(&src, &mut out); + for i in 0..iters { + let frame = match (&alt, i % 2) { + (Some(short), 1) => short.as_slice(), + _ => src.as_slice(), + }; + cctx.compress_independent_frame_into(frame, &mut out); sink = sink.wrapping_add(out.len()); core::hint::black_box(&out); } diff --git a/zstd/src/decoding/decode_buffer.rs b/zstd/src/decoding/decode_buffer.rs index d4af60b1f..7bb9b691f 100644 --- a/zstd/src/decoding/decode_buffer.rs +++ b/zstd/src/decoding/decode_buffer.rs @@ -879,11 +879,14 @@ impl DecodeBuffer { self.buffer.extend(dict_slice); self.total_output_counter += bytes_from_dict as u64; - return self.repeat_tail_after_dict( - dict, - self.buffer.len(), - match_length - bytes_from_dict, - ); + // Straight back into `repeat` for the part that continues into + // the output already produced. Putting this behind an + // `inline(never)` hop to keep the copy machinery out of this + // function's frame was tried and measured: instructions + // 3.1032 -> 3.0900 G (-0.4%), but cycles and wall clock did not + // move (1245-1266 -> 1259-1266 ns a frame, ranges overlapping), + // so the hop bought nothing and is not here. + return self.repeat(dict, self.buffer.len(), match_length - bytes_from_dict); } else { let low = dict_len - bytes_from_dict; let high = low + match_length; @@ -901,27 +904,6 @@ impl DecodeBuffer { } } - /// The part of a match that continues out of the dictionary and into the - /// output already produced. - /// - /// Its own function, and deliberately not inlined: `repeat_inner` is - /// `inline(always)`, so calling it from the dictionary path pulled the whole - /// copy machinery — overlapping copies, the wildcopy variants, their error - /// paths — into that path's body. Every dictionary match then paid the - /// prologue and epilogue of a frame sized for code most of them never run, - /// and on a dictionary-heavy frame that is the common path (23 matches a - /// frame on the benchmark's small-10k-random scenario, at 112 instructions - /// a call). Behind a call the frame belongs to the tail alone. - #[inline(never)] - fn repeat_tail_after_dict( - &mut self, - dict: Option<&crate::decoding::dictionary::Dictionary>, - offset: usize, - match_length: usize, - ) -> Result<(), DecodeBufferError> { - self.repeat(dict, offset, match_length) - } - /// Check if and how many bytes can currently be drawn from the buffer pub fn can_drain_to_window_size(&self) -> Option { if self.buffer.len() > self.window_size { diff --git a/zstd/src/encoding/blocks/compressed.rs b/zstd/src/encoding/blocks/compressed.rs index cde72dd98..3f889e0ec 100644 --- a/zstd/src/encoding/blocks/compressed.rs +++ b/zstd/src/encoding/blocks/compressed.rs @@ -1288,6 +1288,21 @@ fn decide_huff_reuse_like_encoder( // upstream reads it from too (huf_compress.c:1416-1417). On a 4 KiB // dictionary frame the two per-literal walks this replaces were the // largest single item outside the matcher. + // + // Three arms in one session on the i9 (before, after, and the C reference + // through `ffi_loop_dict`), `perf stat -r 3`, three rounds, 20 000 frames + // of the corpus fixture with its dictionary — cycles / instructions / wall: + // + // 4 KiB frame 10 KiB frame + // before 2.92-2.94 G / 8.035 G / 0.70 s 4.81-4.83 G / 12.837 G / 1.15 s + // after 2.27-2.31 G / 6.664 G / 0.54 s 3.41-3.46 G / 9.556 G / 0.82 s + // reference 1.35-1.37 G / 4.307 G / 0.32 s 2.53-2.54 G / 7.128 G / 0.61 s + // + // So 22% of the cycles and 17% of the instructions on the 4 KiB frame, 29% + // and 26% on the 10 KiB one, taking this path from 2.16x of the reference + // to 1.68x and from 1.90x to 1.36x. On input the literal stage writes off + // as incompressible the decision never runs, and the change measures as + // nothing there (instructions 4.8813 G against 4.8843 G) — as expected. let Some(old_estimate) = prev.estimate_compressed_size_from_counts_checked(counts) else { return true; }; diff --git a/zstd/src/encoding/dict_attach.rs b/zstd/src/encoding/dict_attach.rs index a1cace877..21905c087 100644 --- a/zstd/src/encoding/dict_attach.rs +++ b/zstd/src/encoding/dict_attach.rs @@ -44,6 +44,12 @@ pub(crate) struct DictAttach { /// stride phase continuous and closes the seam gap. `0` until the first /// fill; reset by [`Self::invalidate`]. next_to_update: usize, + /// A built table set aside by [`Self::deactivate`], with the state that + /// described it, so a later frame can take it back instead of hashing the + /// same dictionary again. Every reader sees the same thing as after + /// [`Self::invalidate`] while it sits here — the table is out of reach, not + /// merely flagged — so a frame that must not search it cannot. + spare: Option<(T, usize, bool, usize)>, } impl Clone for DictAttach { @@ -53,6 +59,7 @@ impl Clone for DictAttach { region_len: self.region_len, primed: self.primed, next_to_update: self.next_to_update, + spare: self.spare.clone(), } } @@ -63,6 +70,7 @@ impl Clone for DictAttach { self.region_len = source.region_len; self.primed = source.primed; self.next_to_update = source.next_to_update; + self.spare.clone_from(&source.spare); } } @@ -73,6 +81,7 @@ impl DictAttach { region_len: 0, primed: false, next_to_update: 0, + spare: None, } } @@ -158,6 +167,43 @@ impl DictAttach { self.region_len = 0; self.primed = false; self.next_to_update = 0; + // A stashed table describes the dictionary this one was built for; if + // that is being thrown away, so is the stash. + self.spare = None; + } + + /// Put the built table out of reach without discarding it: readers see + /// exactly what [`Self::invalidate`] leaves, and a later frame that wants + /// the same dictionary can take it back with [`Self::reactivate`] instead of + /// hashing it again. + /// + /// For a frame that must NOT search through the attached table — one that + /// primes the dictionary into its live table instead — while a compressor + /// whose source sizes cross the attach cutoff keeps coming back to frames + /// that do. + pub(crate) fn deactivate(&mut self) { + if let Some(table) = self.table.take() { + self.spare = Some((table, self.region_len, self.primed, self.next_to_update)); + } + self.region_len = 0; + self.primed = false; + self.next_to_update = 0; + } + + /// Take back a table put aside by [`Self::deactivate`], with the state that + /// described it. Does nothing when a table is already attached or none was + /// stashed. The caller checks the shape afterwards exactly as it does for a + /// table that was never away. + pub(crate) fn reactivate(&mut self) { + if self.table.is_some() { + return; + } + if let Some((table, region_len, primed, next_to_update)) = self.spare.take() { + self.table = Some(table); + self.region_len = region_len; + self.primed = primed; + self.next_to_update = next_to_update; + } } } diff --git a/zstd/src/encoding/simple/fast_matcher.rs b/zstd/src/encoding/simple/fast_matcher.rs index 21928aff4..d1a812fa8 100644 --- a/zstd/src/encoding/simple/fast_matcher.rs +++ b/zstd/src/encoding/simple/fast_matcher.rs @@ -1563,15 +1563,16 @@ impl FastKernelMatcher { /// `ms->loadedDictEnd`); a dictionary committed in several slices advances /// it to the running end each time, leaving the full size after the last. pub(crate) fn skip_matching_for_dict_copy(&mut self) { - // A dictionary table cached by an earlier ATTACH frame must not - // survive into this one. The borrowed-scan dispatch keys on - // `dict_is_attached()`, so leaving it would send this frame's scan - // into the dual-base kernel, which reads every main-table entry as a - // virtual `dict_end + offset` position — and this frame is writing - // raw ones. Dropping it here rather than at `reset` keeps the cache - // for the attach frames that live off it: rebuilding it per frame - // costs 12% on a reused 4 KiB dictionary frame (i9, wall clock). - self.dict.invalidate(); + // This frame searches through the live table, so nothing may reach the + // attached one: the dual-base kernel it dispatches reads every + // main-table entry as a virtual `dict_end + offset` position, and this + // frame is writing raw ones. Set aside rather than discarded — a + // compressor whose source sizes cross the attach cutoff comes back to + // attach frames, and rebuilding that table is worth 12% on a reused + // 4 KiB dictionary frame (i9, wall clock). Every reader sees the same + // state as a discard while it is away, so the frame's output does not + // depend on the table still existing. + self.dict.deactivate(); self.extend_history_with_pending(); self.prime_hash_table_for_dict_copy(); self.loaded_dict_end = self.history.len(); @@ -2034,6 +2035,12 @@ impl FastKernelMatcher { /// main table's `mls`, so one hash keys both. fn prime_dict_table_for_range(&mut self, range_start: usize, dict_len: usize) { const HASH_READ_SIZE: usize = 8; + // Take back a table a copy-mode frame set aside, if there is one: this + // is an attach frame again, and the dictionary it was built for has not + // changed (a change goes through `invalidate`, which drops the stash). + // The shape check below then treats it exactly like a table that never + // went away, so a mismatched one is still rebuilt. + self.dict.reactivate(); let history_len = self.history.len(); // Record the dict/input boundary regardless of whether any position // is hashable (a sub-8-byte dict still bounds the input floor). From f6c235a2c6ee3cc7bcb3f65a45147e823874c5b6 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 8 Sep 2026 11:41:55 +0300 Subject: [PATCH 16/18] perf(encode): drop the dictionary-table stash, it measured as nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Setting the attached table aside on a copy frame, so the next attach frame would not hash the dictionary again, was built and then measured on the shape it exists for: frames alternating either side of the attach cutoff on one compressor (`encode_loop_dict … alt`, 20 000 frames of 64 KiB and 4 KiB with a 16 KiB dictionary, i9, `perf stat -r 3`, three rounds). Retired instructions came out identical — 30,163,334,509 against 30,163,334,255, a difference of 254 in 30 billion. So the attach frame that follows a copy frame is not rebuilding anything to begin with, and the 0.9% of cycles that moved is the size of a code-layout change. The stash was state and two methods for no work removed, so it goes. The measurement is recorded where the discard happens, so the next reader does not build it again to find out. --- zstd/src/encoding/dict_attach.rs | 46 ------------------------ zstd/src/encoding/simple/fast_matcher.rs | 26 +++++++------- 2 files changed, 13 insertions(+), 59 deletions(-) diff --git a/zstd/src/encoding/dict_attach.rs b/zstd/src/encoding/dict_attach.rs index 21905c087..a1cace877 100644 --- a/zstd/src/encoding/dict_attach.rs +++ b/zstd/src/encoding/dict_attach.rs @@ -44,12 +44,6 @@ pub(crate) struct DictAttach { /// stride phase continuous and closes the seam gap. `0` until the first /// fill; reset by [`Self::invalidate`]. next_to_update: usize, - /// A built table set aside by [`Self::deactivate`], with the state that - /// described it, so a later frame can take it back instead of hashing the - /// same dictionary again. Every reader sees the same thing as after - /// [`Self::invalidate`] while it sits here — the table is out of reach, not - /// merely flagged — so a frame that must not search it cannot. - spare: Option<(T, usize, bool, usize)>, } impl Clone for DictAttach { @@ -59,7 +53,6 @@ impl Clone for DictAttach { region_len: self.region_len, primed: self.primed, next_to_update: self.next_to_update, - spare: self.spare.clone(), } } @@ -70,7 +63,6 @@ impl Clone for DictAttach { self.region_len = source.region_len; self.primed = source.primed; self.next_to_update = source.next_to_update; - self.spare.clone_from(&source.spare); } } @@ -81,7 +73,6 @@ impl DictAttach { region_len: 0, primed: false, next_to_update: 0, - spare: None, } } @@ -167,43 +158,6 @@ impl DictAttach { self.region_len = 0; self.primed = false; self.next_to_update = 0; - // A stashed table describes the dictionary this one was built for; if - // that is being thrown away, so is the stash. - self.spare = None; - } - - /// Put the built table out of reach without discarding it: readers see - /// exactly what [`Self::invalidate`] leaves, and a later frame that wants - /// the same dictionary can take it back with [`Self::reactivate`] instead of - /// hashing it again. - /// - /// For a frame that must NOT search through the attached table — one that - /// primes the dictionary into its live table instead — while a compressor - /// whose source sizes cross the attach cutoff keeps coming back to frames - /// that do. - pub(crate) fn deactivate(&mut self) { - if let Some(table) = self.table.take() { - self.spare = Some((table, self.region_len, self.primed, self.next_to_update)); - } - self.region_len = 0; - self.primed = false; - self.next_to_update = 0; - } - - /// Take back a table put aside by [`Self::deactivate`], with the state that - /// described it. Does nothing when a table is already attached or none was - /// stashed. The caller checks the shape afterwards exactly as it does for a - /// table that was never away. - pub(crate) fn reactivate(&mut self) { - if self.table.is_some() { - return; - } - if let Some((table, region_len, primed, next_to_update)) = self.spare.take() { - self.table = Some(table); - self.region_len = region_len; - self.primed = primed; - self.next_to_update = next_to_update; - } } } diff --git a/zstd/src/encoding/simple/fast_matcher.rs b/zstd/src/encoding/simple/fast_matcher.rs index d1a812fa8..bbd0a7677 100644 --- a/zstd/src/encoding/simple/fast_matcher.rs +++ b/zstd/src/encoding/simple/fast_matcher.rs @@ -1566,13 +1566,19 @@ impl FastKernelMatcher { // This frame searches through the live table, so nothing may reach the // attached one: the dual-base kernel it dispatches reads every // main-table entry as a virtual `dict_end + offset` position, and this - // frame is writing raw ones. Set aside rather than discarded — a - // compressor whose source sizes cross the attach cutoff comes back to - // attach frames, and rebuilding that table is worth 12% on a reused - // 4 KiB dictionary frame (i9, wall clock). Every reader sees the same - // state as a discard while it is away, so the frame's output does not - // depend on the table still existing. - self.dict.deactivate(); + // frame is writing raw ones. + // + // Setting the table aside instead of dropping it, so the next attach + // frame need not hash the dictionary again, was tried and measured on + // the shape it would pay off in: frames alternating either side of the + // attach cutoff on one compressor (`encode_loop_dict … alt`, 20 000 + // frames of 64 KiB and 4 KiB with a 16 KiB dictionary, i9). Retired + // instructions came out IDENTICAL — 30,163,334,509 against + // 30,163,334,255, a difference of 254 in 30 billion — so the attach + // frame after a copy frame is not rebuilding anything to begin with, + // and the 0.9% of cycles that moved is the size of a code-layout + // change. The stash bought nothing and is not here. + self.dict.invalidate(); self.extend_history_with_pending(); self.prime_hash_table_for_dict_copy(); self.loaded_dict_end = self.history.len(); @@ -2035,12 +2041,6 @@ impl FastKernelMatcher { /// main table's `mls`, so one hash keys both. fn prime_dict_table_for_range(&mut self, range_start: usize, dict_len: usize) { const HASH_READ_SIZE: usize = 8; - // Take back a table a copy-mode frame set aside, if there is one: this - // is an attach frame again, and the dictionary it was built for has not - // changed (a change goes through `invalidate`, which drops the stash). - // The shape check below then treats it exactly like a table that never - // went away, so a mismatched one is still rebuilt. - self.dict.reactivate(); let history_len = self.history.len(); // Record the dict/input boundary regardless of whether any position // is hashable (a sub-8-byte dict still bounds the input floor). From 6015ec4dd1e7ec35c143b0f392900a6737139099 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 8 Sep 2026 12:10:16 +0300 Subject: [PATCH 17/18] perf(decode): outline the tail of a match that continues past the dictionary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `repeat_inner` is `inline(always)`, so calling it from the dictionary path pulled the whole copy machinery — overlapping copies, the wildcopy variants, their error paths — into that path's body, and every dictionary match paid the prologue and epilogue of a frame sized for code most of them never run. On a dictionary-heavy frame that is the common path: 23 matches a frame at 112 instructions a call on the benchmark's small-10k-random scenario. Behind a call the frame belongs to the tail. 13.2 million retired instructions on 200 000 frames of that scenario (3.1032 -> 3.0900 G, i9, `perf stat -r 3`). Cycles and wall clock did not move with it (1245-1266 -> 1259-1266 ns a frame, ranges overlapping), so this is fewer operations and not a speed claim. It was dropped a commit ago for exactly that reason, which was wrong: a timer too coarse to resolve a fraction of a percent is not evidence against work that is provably gone. Only a measured increase in cycles would be. --- zstd/src/decoding/decode_buffer.rs | 41 ++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/zstd/src/decoding/decode_buffer.rs b/zstd/src/decoding/decode_buffer.rs index 7bb9b691f..d21dd006a 100644 --- a/zstd/src/decoding/decode_buffer.rs +++ b/zstd/src/decoding/decode_buffer.rs @@ -879,14 +879,11 @@ impl DecodeBuffer { self.buffer.extend(dict_slice); self.total_output_counter += bytes_from_dict as u64; - // Straight back into `repeat` for the part that continues into - // the output already produced. Putting this behind an - // `inline(never)` hop to keep the copy machinery out of this - // function's frame was tried and measured: instructions - // 3.1032 -> 3.0900 G (-0.4%), but cycles and wall clock did not - // move (1245-1266 -> 1259-1266 ns a frame, ranges overlapping), - // so the hop bought nothing and is not here. - return self.repeat(dict, self.buffer.len(), match_length - bytes_from_dict); + return self.repeat_tail_after_dict( + dict, + self.buffer.len(), + match_length - bytes_from_dict, + ); } else { let low = dict_len - bytes_from_dict; let high = low + match_length; @@ -904,6 +901,34 @@ impl DecodeBuffer { } } + /// The part of a match that continues out of the dictionary and into the + /// output already produced. + /// + /// Its own function, and deliberately not inlined: `repeat_inner` is + /// `inline(always)`, so calling it from the dictionary path pulled the whole + /// copy machinery — overlapping copies, the wildcopy variants, their error + /// paths — into that path's body. Every dictionary match then paid the + /// prologue and epilogue of a frame sized for code most of them never run, + /// and on a dictionary-heavy frame that is the common path (23 matches a + /// frame on the benchmark's small-10k-random scenario, at 112 instructions + /// a call). Behind a call the frame belongs to the tail alone. + /// + /// Worth 13.2 million retired instructions on 200 000 frames of that + /// scenario (3.1032 -> 3.0900 G, i9, `perf stat -r 3`). Cycles and wall + /// clock did not move with it (1245-1266 -> 1259-1266 ns a frame, ranges + /// overlapping), so this is fewer operations and NOT a speed claim: the + /// timer is too coarse to resolve a fraction of a percent, and work that is + /// gone is gone. + #[inline(never)] + fn repeat_tail_after_dict( + &mut self, + dict: Option<&crate::decoding::dictionary::Dictionary>, + offset: usize, + match_length: usize, + ) -> Result<(), DecodeBufferError> { + self.repeat(dict, offset, match_length) + } + /// Check if and how many bytes can currently be drawn from the buffer pub fn can_drain_to_window_size(&self) -> Option { if self.buffer.len() > self.window_size { From 4f742c132e64908ce2565628a93762e16a047eb2 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 8 Sep 2026 12:33:43 +0300 Subject: [PATCH 18/18] docs(perf): record the measurements behind three hot-path changes The Fast attach-vs-copy cutoff, the parser's literal-run walk and the folded window-floor helper each landed with an argument and an incomplete number. All three are now measured the same way: two prebuilt binaries and libzstd alternated in one ssh session, perf stat -r 3 for cycles AND retired instructions, three rounds, plus a control arm the change cannot execute. The cutoff table gains its two missing cycle cells, an instruction column and absolute byte counts. Copy takes 18-28% fewer cycles and 22-27% fewer instructions at the positive levels and matches or beats the reference's bytes there; at the ultra-fast levels it spends 9-21% more cycles to save 5.5-6.0% of the bytes, which is the trade the cutoff exists to make. The parser's literal-run walk is a speed win and is now stated as one: -29.5% cycles and -22.8% instructions per frame on near-random input at level 19, 1.94x -> 1.37x of libzstd, byte-identical. The folded window floor is NOT a speed win and no longer reads like one. It removes 164,648 retired instructions a frame on that fixture, but the control arm moves 3.6% of cycles on its own, so the clock cannot resolve it. Kept for the operations that are provably gone. Also fixes the encode loop reporting src.len() x iters as its input total under alt, where the frames are two different sizes. --- zstd/examples/encode_loop_dict.rs | 16 +++++++++-- zstd/src/encoding/hc/optimal.rs | 14 ++++++++++ zstd/src/encoding/levels/config.rs | 35 ++++++++++++++---------- zstd/src/encoding/match_table/storage.rs | 14 ++++++++++ 4 files changed, 63 insertions(+), 16 deletions(-) diff --git a/zstd/examples/encode_loop_dict.rs b/zstd/examples/encode_loop_dict.rs index 097d5fabb..b5c48172a 100644 --- a/zstd/examples/encode_loop_dict.rs +++ b/zstd/examples/encode_loop_dict.rs @@ -124,9 +124,21 @@ fn main() { core::hint::black_box(&out); } + // Under `alt` the frames are not all `src.len()` long, so the total is + // counted from the schedule rather than multiplied out. Odd iterations take + // the short frame, which is `iters / 2` of them. Counting it here instead of + // accumulating inside the loop keeps the timed body exactly as it is + // measured. + let short_frames = (iters / 2) as usize; + let long_frames = iters as usize - short_frames; + let input_bytes = match &alt { + Some(short) => long_frames * src.len() + short_frames * short.len(), + None => iters as usize * src.len(), + }; + eprintln!( - "encoded {} bytes × {} iters at level {} dict={}; last-out-sum={}", - src.len(), + "encoded {} input bytes in {} iters at level {} dict={}; last-out-sum={}", + input_bytes, iters, level, dict_path.unwrap_or("none"), diff --git a/zstd/src/encoding/hc/optimal.rs b/zstd/src/encoding/hc/optimal.rs index 413594030..b9b0870bd 100644 --- a/zstd/src/encoding/hc/optimal.rs +++ b/zstd/src/encoding/hc/optimal.rs @@ -159,6 +159,20 @@ macro_rules! build_optimal_plan_impl_body { // per call from the segment's block offset, so advancing inside one call // is not the same thing. `HAS_LDM` is a const generic, so this whole // block folds away there. + // + // What the walk is worth, per frame on the i9 (10 KiB frames at level 19 + // with a 16 KiB dictionary, two prebuilt binaries and libzstd alternated + // in one session, `perf stat -r 3`, three rounds, ranges + // non-overlapping). Near-random input, the shape this exists for: + // 2,380,845 -> 1,679,325 cycles (-29.5%) and 6,132,013 -> 4,731,553 + // retired instructions (-22.8%), which is 1.94x -> 1.37x of libzstd on + // cycles and 2.04x -> 1.57x on instructions. Compressible input, where + // the search finds matches and the run is short: 4,616,838 -> 4,390,463 + // cycles (-4.9%) and 10,291,065 -> 9,993,577 instructions (-2.9%). The + // control arm is level 1, whose Fast backend never enters this parser: + // its instruction count is bit-identical between the two binaries + // (71,276), and its cycles differ by 3.4%, which bounds what code layout + // alone can account for. Output is byte-identical on both fixtures. let mut skipped_literals = 0usize; let mut seed_candidates_ready = carried_candidates; if !HAS_LDM && !seed_candidates_ready { diff --git a/zstd/src/encoding/levels/config.rs b/zstd/src/encoding/levels/config.rs index f851e3196..46c912de0 100644 --- a/zstd/src/encoding/levels/config.rs +++ b/zstd/src/encoding/levels/config.rs @@ -451,24 +451,31 @@ pub(crate) fn source_size_ceil_log(size: u64) -> u8 { /// separate exact table, at the positions the step happens to land on. /// /// This was `31` (attach every source up to 2 GiB) on a speed argument alone, -/// and the missing byte column is where it went wrong. `z000033` (1,022,035 B) -/// with its 16 KiB dictionary, attach → copy, against libzstd on the same host: +/// and the missing byte column is where it went wrong. Per frame, `z000033` +/// (1,022,035 B) and its leading 10 KiB, with a 16 KiB dictionary trained over +/// its 10 KiB chunks, on the i9: two prebuilt binaries and libzstd alternated +/// in one session, `perf stat -r 3`, three rounds, ranges non-overlapping. /// -/// | case | bytes attach | bytes copy | reference | cycles attach | cycles copy | -/// |---|---|---|---|---|---| -/// | 10 KiB frames, L1 | 6,976 | 7,123 | 7,123 | 2.06x | 1.68x | -/// | 10 KiB frames, L-5 | 9,659 | 9,123 | 9,129 | — | — | -/// | 1 MiB, L1 | -0.24% | -0.09% | — | 1.96x | 1.71x | -/// | 1 MiB, L-5 | +5.95% | -0.08% | — | 1.48x | 1.78x | +/// | case | bytes attach | bytes copy | reference | cycles attach | cycles copy | insn attach | insn copy | +/// |---|---|---|---|---|---|---|---| +/// | 10 KiB, L1 | 6,976 | 7,122 | 7,122 | 236,355 (1.95x) | 169,116 (1.39x) | 633,127 (1.78x) | 478,066 (1.34x) | +/// | 10 KiB, L-5 | 9,660 | 9,124 | 9,130 | 51,678 (1.22x) | 56,444 (1.33x) | 152,281 (1.14x) | 153,369 (1.15x) | +/// | 1 MiB, L1 | 550,810 | 551,584 | 570,765 | 19.84M (1.76x) | 16.21M (1.44x) | 54.01M (1.91x) | 39.60M (1.40x) | +/// | 1 MiB, L-5 | 689,127 | 647,735 | 669,826 | 8.80M (1.46x) | 10.67M (1.77x) | 21.62M (1.54x) | 22.81M (1.63x) | /// -/// So attach was losing 6% of the ratio across the ultra-fast band — the -/// dictionary made our frame BIGGER than our own no-dict frame there, while it -/// made the reference's smaller — and the cycles it appeared to save in that -/// band were bought by finding 21,897 sequences where the reference finds -/// 27,546. At the positive levels copy is the cheaper arm as well. +/// Copy is the better arm on both axes at the positive levels: it takes 18-28% +/// fewer cycles and 22-27% fewer instructions, and its bytes are the +/// reference's exactly on the small frame and 3.4% under the reference on the +/// large one. At the ultra-fast levels it buys ratio with time: 21% more cycles +/// on the 1 MiB frame for 6.0% fewer bytes, 9% more on the small one for 5.5% +/// fewer. That trade is what the cutoff is for. Attach put us 2.9% ABOVE the +/// reference on the 1 MiB ultra-fast frame — the dictionary made our frame +/// bigger than our own no-dict frame there, while it made the reference's +/// smaller — because it found 21,897 sequences where the reference finds +/// 27,546. Copy puts us 3.3% under it. /// /// The remaining gap is now a same-mode one: the reference does this copy in -/// 1.0x where we take 1.7-1.8x, which is a target with an apples-to-apples +/// 1.0x where we take 1.3-1.8x, which is a target with an apples-to-apples /// reference rather than a mode the reference never runs. /// /// The borrowed attach kernel stores virtual positions as `u32` diff --git a/zstd/src/encoding/match_table/storage.rs b/zstd/src/encoding/match_table/storage.rs index 04cae6f48..9fdbb4b4d 100644 --- a/zstd/src/encoding/match_table/storage.rs +++ b/zstd/src/encoding/match_table/storage.rs @@ -1344,6 +1344,20 @@ impl MatchTable { /// upstream calls `ZSTD_getLowestMatchIndex` — and it was being called, not /// folded: it stood in the profile as its own symbol, paying a call and a /// return for four instructions of work. + /// + /// What folding it removes, per frame on the i9 (10 KiB frames at level 19 + /// with a 16 KiB dictionary, two prebuilt binaries alternated in one + /// session, `perf stat -r 3`, three rounds): 164,648 retired instructions on + /// near-random input (4,896,201 -> 4,731,553, -3.4%) and 147,017 on + /// compressible input (10,140,594 -> 9,993,577, -1.5%). The counts are + /// deterministic and repeat to single digits across rounds. + /// + /// It is NOT a speed claim. Cycles moved -3.8% and -1.7% on those two, but + /// the control arm — level 1, whose Fast backend never reaches this + /// function, and whose instruction count is bit-identical between the two + /// binaries at 71,276 — moved 3.6% by itself, so code layout accounts for + /// as much as the measurement shows. Kept for the operations that are + /// provably gone, not for a clock that cannot resolve them. #[inline(always)] pub(crate) fn window_low_abs_for_target(&self, target_abs: usize) -> usize { let history_low = self.history_abs_start;