From f3a2caa0e2344912a37d6c5877e124a3a6fc0bbc Mon Sep 17 00:00:00 2001 From: ddidderr Date: Mon, 20 Jul 2026 05:22:25 +0200 Subject: [PATCH] fix(dict-builder): stabilize optimized dictionary scoring Optimized dictionary scoring used the requested dictionary ID while building its scoring CDict, so --dictID could change candidate quality and output content. Parallel FastCover could also choose different equal-score candidates based on completion order. Clear the ID only while constructing the scoring CDict, restore it immediately afterward, and use deterministic (d, k) tie-breakers in Cover and FastCover. The requested ID now remains output metadata instead of influencing dictionary content or repeat-run stability. Test Plan: - ulimit -v 41943040; CARGO_BUILD_JOBS=1 cargo test --manifest-path rust/Cargo.toml scoring_dictionary_id_is_restored_after_the_score --lib - ulimit -v 41943040; CARGO_BUILD_JOBS=1 cargo clippy --manifest-path rust/Cargo.toml --all-targets -- -D warnings - ulimit -v 41943040; make -j1 - ulimit -v 41943040; CARGO_BUILD_JOBS=1 cargo test --manifest-path rust/Cargo.toml --all-targets - ulimit -v 41943040; CARGO_BUILD_JOBS=1 cargo test --manifest-path rust/cli/Cargo.toml --all-targets - ulimit -v 41943040; make -j1 -C tests test --- rust/src/dict_builder_cover.rs | 54 ++++++++++++++++++++++++++---- rust/src/dict_builder_fastcover.rs | 5 ++- 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/rust/src/dict_builder_cover.rs b/rust/src/dict_builder_cover.rs index 3cb0a15fc..6a039508e 100644 --- a/rust/src/dict_builder_cover.rs +++ b/rust/src/dict_builder_cover.rs @@ -541,7 +541,10 @@ fn best_finish_slot( .lock() .unwrap_or_else(|poison| poison.into_inner()); data.live_jobs = data.live_jobs.wrapping_sub(1); - if selection.totalCompressedSize < data.compressed_size { + let improves_score = selection.totalCompressedSize < data.compressed_size; + let wins_equal_score = selection.totalCompressedSize == data.compressed_size + && (parameters.d, parameters.k) < (data.parameters.d, data.parameters.k); + if improves_score || wins_equal_score { if data.dict.is_null() || data.dict_capacity < selection.dictSize { let replacement = unsafe { malloc_bytes(selection.dictSize) }; if replacement.is_null() && selection.dictSize != 0 { @@ -629,6 +632,22 @@ unsafe extern "C" { ) -> usize; } +unsafe fn with_dictionary_id_cleared( + dict: *mut u8, + dict_buffer_capacity: usize, + action: impl FnOnce() -> T, +) -> T { + if dict_buffer_capacity < 8 { + return action(); + } + let dict_id = unsafe { dict.add(4).cast::() }; + let saved_id = unsafe { ptr::read_unaligned(dict_id) }; + unsafe { ptr::write_unaligned(dict_id, 0) }; + let result = action(); + unsafe { ptr::write_unaligned(dict_id, saved_id) }; + result +} + #[no_mangle] pub unsafe extern "C" fn COVER_sum(samples_sizes: *const usize, nb_samples: c_uint) -> usize { if nb_samples == 0 { @@ -781,12 +800,19 @@ pub unsafe extern "C" fn COVER_checkTotalCompressedSize( } let dst = unsafe { malloc_bytes(dst_capacity) }; let cctx = unsafe { ZSTD_createCCtx() }; + /* The dictionary ID is output metadata, not dictionary content. The + * fixed-frame `ZSTD_compress_usingCDict()` score includes that metadata, + * so temporarily clear it while constructing the scoring CDict. This + * keeps `--dictID` from changing which candidate wins optimization while + * leaving the requested ID in the finalized dictionary. */ let cdict = unsafe { - ZSTD_createCDict( - dict.cast(), - dict_buffer_capacity, - parameters.zParams.compressionLevel, - ) + with_dictionary_id_cleared(dict, dict_buffer_capacity, || { + ZSTD_createCDict( + dict.cast(), + dict_buffer_capacity, + parameters.zParams.compressionLevel, + ) + }) }; if dst.is_null() || cctx.is_null() || cdict.is_null() { if !cctx.is_null() { @@ -1287,6 +1313,22 @@ mod tests { ); } + #[test] + fn scoring_dictionary_id_is_restored_after_the_score() { + let mut dictionary = [0u8; 16]; + dictionary[4..8].copy_from_slice(&0x1234_5678u32.to_ne_bytes()); + let observed_id = unsafe { + with_dictionary_id_cleared(dictionary.as_mut_ptr(), dictionary.len(), || { + ptr::read_unaligned(dictionary.as_ptr().add(4).cast::()) + }) + }; + assert_eq!(observed_id, 0); + assert_eq!( + u32::from_ne_bytes(dictionary[4..8].try_into().unwrap()), + 0x1234_5678 + ); + } + #[test] fn context_groups_repeated_dmers_by_sample() { let samples = [b'a'; 45]; diff --git a/rust/src/dict_builder_fastcover.rs b/rust/src/dict_builder_fastcover.rs index 5f8fd4b21..8d75342bc 100644 --- a/rust/src/dict_builder_fastcover.rs +++ b/rust/src/dict_builder_fastcover.rs @@ -524,7 +524,10 @@ impl FastBest { .lock() .unwrap_or_else(|poison| poison.into_inner()); state.live_jobs = state.live_jobs.wrapping_sub(1); - if selection.totalCompressedSize < state.compressed_size { + let improves_score = selection.totalCompressedSize < state.compressed_size; + let wins_equal_score = selection.totalCompressedSize == state.compressed_size + && (parameters.d, parameters.k) < (state.parameters.d, state.parameters.k); + if improves_score || wins_equal_score { if state.dict.is_null() || state.dict_size < selection.dictSize { unsafe { free_bytes(state.dict) }; let replacement = unsafe { malloc_bytes(selection.dictSize) };