diff --git a/ffi-bench/tests/dictionary_ffi.rs b/ffi-bench/tests/dictionary_ffi.rs index b4ca3ece4..df14779da 100644 --- a/ffi-bench/tests/dictionary_ffi.rs +++ b/ffi-bench/tests/dictionary_ffi.rs @@ -122,3 +122,84 @@ 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"); + 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"); + + // 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/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/examples/encode_loop_dict.rs b/zstd/examples/encode_loop_dict.rs index ebcf35f56..b5c48172a 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,15 +114,31 @@ 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); } + // 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/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. diff --git a/zstd/src/decoding/decode_buffer.rs b/zstd/src/decoding/decode_buffer.rs index c00a88061..d21dd006a 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, @@ -876,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; @@ -894,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 { diff --git a/zstd/src/encoding/blocks/compressed.rs b/zstd/src/encoding/blocks/compressed.rs index 8bff87a76..3f889e0ec 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,33 @@ 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. + // + // 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; }; // Late-stage `HUF_flags_preferRepeat` mirror — kept here for @@ -1296,7 +1318,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 +2841,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/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/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/hc/generator.rs b/zstd/src/encoding/hc/generator.rs index ba5bf90d2..10acb4715 100644 --- a/zstd/src/encoding/hc/generator.rs +++ b/zstd/src/encoding/hc/generator.rs @@ -957,7 +957,36 @@ 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. + // + // 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; } diff --git a/zstd/src/encoding/hc/optimal.rs b/zstd/src/encoding/hc/optimal.rs index a73de1bff..b9b0870bd 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 @@ -98,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 @@ -126,6 +142,84 @@ 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. + // + // 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 { + 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; @@ -187,23 +281,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 @@ -1062,7 +1161,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 +1386,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()); @@ -1521,6 +1625,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/levels/config.rs b/zstd/src/encoding/levels/config.rs index 01a3f3649..46c912de0 100644 --- a/zstd/src/encoding/levels/config.rs +++ b/zstd/src/encoding/levels/config.rs @@ -442,27 +442,48 @@ 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. 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 | 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) | +/// +/// 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.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` +/// (`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/match_table/helpers.rs b/zstd/src/encoding/match_table/helpers.rs index 2880439c7..b29cda396 100644 --- a/zstd/src/encoding/match_table/helpers.rs +++ b/zstd/src/encoding/match_table/helpers.rs @@ -26,10 +26,44 @@ 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, 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; /// Length of the common prefix of two byte slices, capped at /// `min(a.len(), b.len())`. Hot path on every match finder; dispatches to diff --git a/zstd/src/encoding/match_table/storage.rs b/zstd/src/encoding/match_table/storage.rs index 392f90fb3..9fdbb4b4d 100644 --- a/zstd/src/encoding/match_table/storage.rs +++ b/zstd/src/encoding/match_table/storage.rs @@ -1339,6 +1339,26 @@ 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. + /// + /// 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; let window_low = target_abs.saturating_sub(self.max_window_size); 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)>, } 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..bbd0a7677 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, } } @@ -650,7 +657,8 @@ impl FastKernelMatcher { // 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). + // sound); a copy-mode frame drops it when it primes, which is + // the only case that must not keep it. self.hash_table.clear(); } self.table_pos_high_water = 0; @@ -658,6 +666,9 @@ impl FastKernelMatcher { // 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 +1545,45 @@ 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) { + // 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. + // + // 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(); + } + /// 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 +1773,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..089353f53 100644 --- a/zstd/src/encoding/simple/fast_matcher/tests.rs +++ b/zstd/src/encoding/simple/fast_matcher/tests.rs @@ -601,6 +601,112 @@ 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", + ); +} + +/// 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. 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) }