fix(encoding): bound the dms-table hash-log floor by the matcher hash_log#459
Conversation
Extract the dms-table hash-log sizing into `storage::dms_hash_log` and cover it: a tiny source adjusts a BT matcher's `hash_log` below 10, and the `ceil_log2(region).clamp(10, hash_log)` sizing then panics 'min > max' (min = 10, max = 7) when priming the dictionary match binary-tree (level >= 13 with a dictionary). The unit test calls `dms_hash_log` with hash_log < 10 and FAILS on this commit; a BtUltra2 dict round-trip exercises the path end to end. Fix follows separately.
…_log prime_dms_bt sized the dictionary match binary-tree with ceil_log2(region).clamp(10, hash_log). When a tiny source adjusts the BT matcher's hash_log below 10, the clamp floor (10) exceeds the ceiling (hash_log), so std clamp panics 'min > max'. Lower the floor to min(10, hash_log): at hash_log >= 10 it is the unchanged 10-bit minimum, and below 10 it collapses to hash_log so the dms table just matches the (small) live-table width. Byte-identical for every hash_log >= 10 configuration (all existing fixtures); only the previously-panicking small-BT-dict path changes. Closes the level >= 13 + dictionary regression from 0.0.46.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughExtracts a new DMS hash-log fix and tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Summary
Fixes a panic when compressing with a dictionary on a binary-tree strategy
(level ≥ 13) and a small source:
prime_dms_bt(the dictionary matchbinary-tree, a
ZSTD_dictMatchStateanalog) sized its hash table withA tiny source makes
ZSTD_adjustCParamslegitimately shrink the matcher'shash_logbelow 10, so the clamp floor (10) exceeds the ceiling (hash_log),and
std'sclamppanics:Non-BT strategies (Fast / Dfast / Lazy, levels 1–12) never call
prime_dms_bt,so dictionary compression there was unaffected; only the BT dict path hit it.
Root cause
The
10floor was a spurious minimum with no upstream-C counterpart: C sizesthe dms table from the dictionary's own (
ZSTD_createCDict) cParamshashLog,which is itself adjusted down for small dictionaries — there is no fixed 10-bit
floor. A
hash_logbelow 10 is therefore a valid adjusted value, not aninvariant violation; the bug is that the floor could exceed the ceiling.
Fix
Lower the floor to
min(10, hash_log):hash_log ≥ 10→ the floor stays 10 (unchanged; byte-identical for everyexisting fixture and the whole bench corpus).
hash_log < 10→ the floor collapses tohash_log, so the clamp stays validand the dms table simply matches the (small) live-table width — exactly what
the dictionary's cParams
hashLogyields in upstream for the same small dict.The sizing was extracted into
storage::dms_hash_logso the bound is covered bya direct unit test (no need to coax the full compressor into a
hash_log < 10configuration, which the current window-log floor otherwise masks).
Testing (test-first)
storage::dms_hash_log_tests— callsdms_hash_logwithhash_log < 10;this failed with the exact
min > maxpanic before the fix and asserts asane table width after, plus pins the unchanged
hash_log ≥ 10floor/cap.frame_compressor::tests::dict_compress_bt_level_tiny_source_round_trips_through_prime_dms_bt— end-to-end BtUltra2 + raw-content dictionary + tiny source round-trip
through the BT dict-prime + dict-match path.
cargo nextest run -p structured-zstd --features hash,std,dict_builder— 841 passcargo nextest run -p ffi-bench --features bench_internals,dict_builder— 59 pass--tests,--no-default-features --features kernel_scalar,hash),cargo fmt --check— cleanNote
The
10floor forhash_log ≥ 10is pre-existing and intentionally preservedhere for byte-identity. Dropping it entirely (fully matching upstream, which has
no floor) would change the dms table width for tiny dictionaries at
hash_log ≥ 10and thus the emitted bytes — a separate behaviour-changingoptimisation, out of scope for this fix.
Summary by CodeRabbit