perf(opt): close the btopt dict-band gap via upstream getAllMatches shape#448
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 52 minutes and 5 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR updates HC/BT match collection and optimal-parse dispatch, refactors decoder dictionary handling across decode paths, and changes Huffman height-limiting validation with new tests. ChangesEncoder match and optimal-parse flow
Decoder dictionary handling
Huffman height limiting
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…hape Reshape the optimal-parser per-position find toward upstream's ZSTD_insertBtAndGetAllMatches shape (one inlined body, one call per position, match-length template resolved once per block), and fix a Huffman height-limiter panic reachable on realistic literal histograms. Perf (byte-identical output across the suite): - Inline the find monolith into collect (upstream ZSTD_btGetAllMatches shape): rep probe + hash3 probe + BT tree walk folded into one out-of-line body, one call per position, replacing a nested call chain that paid per-position argument marshalling upstream never pays. - Resolve the SIMD kernel tier once per block, not per segment: the per-position loop runs under one kernel-monomorphized expansion instead of re-entering the runtime CPU-tier dispatch on every call. - Pass the cost profile by reference into the deepest per-position crossing (the 24-byte struct was stack-copied every call). - Take the DP scratch buffers once per block and share them across both btultra2 passes, dropping a take + restore round-trip per segment. - Trim the no-match literal path: defer price-cache / stamp / base-node setup past the no-match seed, drop a discarded price recompute, skip the no-op plan-stats update on no-match segments. i9-9900K (BMI2 + AVX2), L16 btopt small-10k-random compress-dict, libzstd control flat at 388 us: pure Rust 932 us -> 657 us (-29%), 2.40x -> 1.70x vs libzstd. Huffman fix: The height limiter (upstream HUF_setMaxHeight equivalent) walked the cost-repair fill in the wrong direction on certain <=128 KB literal histograms, leaving a code whose Kraft sum was not a power of two. The table builder rejects such a code, surfacing as a panic. The limiter now fills downward correctly and validates the Kraft sum, falling back to the distributed-weight construction when it cannot reach the bit-length cap; the dead repair helper is removed. Adds a regression test (pinned trigger histogram plus 300k fuzzed histograms capped at the 128 KB literal limit).
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zstd/src/encoding/hc/generator.rs (1)
877-902: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGate dictionary descent on
dms.is_primed().Line 877 walks
$table.dms.table()directly; use the canonical primed predicate before descending so a present-but-invalid/stale DMS table is not searched.🐛 Proposed fix
- if let Some(dms) = $table.dms.table().filter(|_| compares_left > 0) { + if compares_left > 0 && $table.dms.is_primed() { + let dms = $table.dms.table().expect("primed dms has table"); let region = $table.dms.region_len();Based on learnings, use
table.dms.is_primed()as the canonical validity predicate before walking/reusing DMS state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zstd/src/encoding/hc/generator.rs` around lines 877 - 902, The dictionary descent in the HC generator is using $table.dms.table() directly, which can walk stale or invalid DMS state. Update the check around the existing MatchTable::hash_position_at and dms.hash_table traversal to first require table.dms.is_primed() as the canonical predicate, then only descend when that returns true and compares_left > 0. Keep the rest of the dcur/common_smaller/common_larger logic unchanged.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@zstd/src/encoding/levels/config.rs`:
- Around line 1160-1162: The clamping logic in the level-resolution path is
bypassing the special Level(22) resolver for values above the max level, so
route any clamped high level through the same Level::Level22 handling used by
resolve_level_params_for_source_size and level22_btultra2_params_for_source_size
instead of directly calling default_cparams with 22; update the
cparams_tier/default_cparams flow so Level(23)+ resolves identically to
Level(22).
In `@zstd/src/huff0/huff0_encoder/tests.rs`:
- Around line 54-101: The large randomized sweep in the huff0_encoder tests is
too expensive for normal runs, so keep the fixed TRIGGER regression in this test
but reduce the default iteration count or move the 300k-case loop into an
ignored/stress test. Update the loop in the tests around build_limited_weights
and HuffmanTable::build_from_counts_gated so regular unit tests stay fast while
still preserving the high-coverage fuzz path separately.
---
Outside diff comments:
In `@zstd/src/encoding/hc/generator.rs`:
- Around line 877-902: The dictionary descent in the HC generator is using
$table.dms.table() directly, which can walk stale or invalid DMS state. Update
the check around the existing MatchTable::hash_position_at and dms.hash_table
traversal to first require table.dms.is_primed() as the canonical predicate,
then only descend when that returns true and compares_left > 0. Keep the rest of
the dcur/common_smaller/common_larger logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 602ccde7-7fae-469e-ac17-a93e415fcc2e
📒 Files selected for processing (8)
zstd/src/encoding/hc/generator.rszstd/src/encoding/hc/optimal.rszstd/src/encoding/levels/config.rszstd/src/encoding/match_generator/tests.rszstd/src/encoding/match_table/storage.rszstd/src/encoding/strategy.rszstd/src/huff0/huff0_encoder.rszstd/src/huff0/huff0_encoder/tests.rs
The HC / optimal / btlazy2 strategy entries and the BT-walker dispatchers called select_kernel() per block (btultra2 twice: seed + main pass), re-reading the cached OnceLock atomic inside the hot strategy path. The Fast / Row / Dfast matchers already cache the resolved tier in a field at construction; mirror that for the binary-tree path. MatchTable now caches the resolved tier in a field, set once via select_kernel() at construction (once per encoder invoke, since the matcher is built once per FrameCompressor). Every per-block strategy entry and BT dispatcher reads the cached field instead of re-detecting. select_kernel() is now called exactly once per matcher (the documented "once at the entry point of each encoder call" contract), never inside a strategy or hot loop. Byte-identical: the cached value equals what select_kernel() returned per block, because the CPU tier is constant for the process lifetime.
Forcing the cached table.kernel runs each per-CPU tier's monomorphized BT-collect / DP wrapper on one machine, pinning the scalar-vs-SIMD bit-identity invariant and covering the per-tier wrappers the runtime dispatch leaves cold (only the selected tier would otherwise execute).
… alphabets - level_params_strategy_and_search_method_agree_across_tiers: resolves positive levels over every source-size tier, asserting each derived strategy pairs with its search method; exercises the cParams -> LevelParams strategy arms the existing level tests (which used StrategyTag::for_level) left cold. - build_limited_weights_handles_degenerate_alphabets: single-symbol and all-zero histograms hit the height limiter's leaves.len() <= 1 early-out.
resolve_level_params routes Level(22) through the btultra2-specific source-size resolver but lets Level(23+) fall through to the generic cParams path, so an out-of-range level resolves to a DIFFERENT config than the max level. This test asserts Level(MAX_LEVEL + k) == Level(MAX_LEVEL) across source sizes and fails until the clamp routes through the same path.
resolve_level_params special-cased only Level(22) for the btultra2 source-size resolver; Level(23+) fell through to the generic cParams derivation and produced a different config than Level(22). Match Level(n) for n >= MAX_LEVEL so any out-of-range level clamps to the same max-level configuration.
Keep the deterministic TRIGGER regression plus a 4k randomized sweep in the default suite (fast), and move the full 300k sweep behind #[ignore] so the normal run stays quick; invoke with --run-ignored for the deep pass. Also gate the HC dictionary-match descent on dms.is_primed() (the canonical validity predicate) instead of walking dms.table() directly, so a present-but-stale DMS table is never searched.
5e371b0 to
6a783d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@zstd/src/huff0/huff0_encoder/tests.rs`:
- Around line 123-153: The test comment in
build_limited_weights_handles_degenerate_alphabets claims the all-zero histogram
case is covered, but the test body only checks single-symbol and two-symbol
inputs. Either add an explicit assertion for the all-zero histogram behavior in
build_limited_weights and huffman_weight_sum_is_power_of_two, or revise the
comment to remove that claim; use the existing build_limited_weights helper and
the degenerate-case setup in this test to keep the coverage consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5a0c82be-7df8-49ba-ac23-5364ef349040
📒 Files selected for processing (9)
zstd/src/encoding/hc/generator.rszstd/src/encoding/hc/mod.rszstd/src/encoding/hc/optimal.rszstd/src/encoding/levels/config.rszstd/src/encoding/match_generator/tests.rszstd/src/encoding/match_table/storage.rszstd/src/encoding/strategy.rszstd/src/huff0/huff0_encoder.rszstd/src/huff0/huff0_encoder/tests.rs
The degenerate-alphabets test comment claimed all-zero coverage but the body only checked single/two-symbol inputs. Add an explicit all-zero assertion for the early-out's actual behavior (all weights stay zero) and reword the comment: an all-zero histogram never reaches production (a real block always has >= 1 literal) and cannot meet the power-of-two invariant a zero-symbol code has no terms for, so only the symbol-bearing cases assert it.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
…nt apply Upstream zstd's ZSTD_copyDDictParameters stores RAW POINTERS into a caller-owned DDict every frame (dctx->LLTptr = ddict->entropy.LLTable, prefixStart = dictContent, ...) — no refcount, no per-frame clone. We held an owned Arc<Dictionary> in three places (FSE + Huffman scratch + decode buffer) and re-cloned all three each frame, dropping them at teardown: six atomic refcount ops per frame on a value the dictionary owner already keeps alive for the whole decode. On a tiny frame that fixed per-frame cost is a measurable share of decode time. Mirror C: the three scratch sites now hold Option<NonNull<Dictionary>>, set unconditionally from a live &DictionaryHandle each frame (one pointer store, no atomic) and read through a checked unsafe deref while the COW source is Dict. The pointee is kept alive across the whole decode by the dictionary owner — the FrameDecoder owned_dicts/shared_dicts registry, or the &DictionaryHandle the caller holds across decode_all_with_dict_handle — so the borrow is valid for every read (documented contract on each dict field). NonNull suppresses auto-Send/Sync, so unsafe impl Send/Sync is restored on each scratch type (the pointee is Send+Sync, read-only, owner-kept-alive); a compile-time assert pins FrameDecoder: Send + Sync against regression. Byte-identical (same dict tables; only the reference's ownership changed from owned-Arc to borrowed-pointer). i9 small-10k-random decompress-dict, flat c_ffi control: L3 dfast -2.3%, L16 btopt -1.8%.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@zstd/src/decoding/decode_buffer.rs`:
- Around line 221-222: `DecodeBuffer::set_dict` currently stores only a raw
dictionary pointer, so temporary `DictionaryHandle` values can be dropped while
`dict_content` and `repeat_from_dict` still use them. Update `DecodeBuffer` to
own the dictionary handle or another guard alongside the `NonNull` pointer, or
thread an explicit lifetime through the buffer/scratch path so the handle
outlives all reads. Adjust the `set_dict` API and any call sites that pass
temporaries like `into_handle` or `DictionaryHandle::from_dictionary`
accordingly.
In `@zstd/src/decoding/decode_buffer/tests.rs`:
- Line 25: The decode_buffer tests are passing temporary dictionary handles into
buf.set_dict, but set_dict only keeps a raw pointer, so the handle can be
dropped too early and later repeat/dictionary reads may use freed memory. Update
the affected test cases to bind each dictionary handle to a local variable
before calling buf.set_dict, and keep that variable alive for the full scope
where the buffer is used. Apply this fix in the repeated set_dict call sites in
the decode_buffer tests.
In `@zstd/src/decoding/scratch.rs`:
- Around line 258-263: Update the ownership comment in scratch::attach_dict to
match the current zero-clone behavior: it should no longer mention cloning on
fresh/changed dictionaries, since the implementation now stores borrowed NonNull
pointers only. Clarify that no handle is retained by attach_dict and that the
caller/registry must keep the dictionary alive for the duration of use, so the
comment accurately reflects the lifetime-sensitive unsafe contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 929dc440-accb-4fc8-889a-873e5b786b44
📒 Files selected for processing (6)
zstd/src/decoding/decode_buffer.rszstd/src/decoding/decode_buffer/tests.rszstd/src/decoding/frame_decoder.rszstd/src/decoding/frame_decoder/tests.rszstd/src/decoding/scratch.rszstd/src/huff0/huff0_encoder/tests.rs
`set_dict` now stores a borrowed `NonNull` into the handle's dictionary (07f85a9), so the handle must outlive the `DecodeBuffer`. Four tests passed a temporary handle (`&dict()`, `&dict.into_handle()`, `&DictionaryHandle::from_dictionary(...)`) whose drop at the end of the statement left the stored pointer dangling; the subsequent `repeat_from_dict` reads then hit freed memory, surfacing as an access violation (0xc0000005) on Windows and silent use-after-free elsewhere. Bind each handle to a local that outlives the buffer's reads.
Replace the raw-pointer dictionary hold (NonNull stored in the decode scratch + buffer, with `unsafe impl Send/Sync` to restore the auto traits) with a borrow-checked design that the compiler proves sound and that mirrors C's per-frame pointer hand-off without any refcount churn. - The decode scratch / buffer no longer OWN or POINT AT the dictionary. Every `Dict`-sourced table read (FSE ll/of/ml, HUF, dict content) takes the dictionary as a call-scoped `dict: Option<&Dictionary>` argument, threaded through the whole decode pipeline (block decoder, literals, sequence executors, per-tier SIMD monoliths). Source flags on the scratch still select Local vs Dict; the borrow supplies the bytes only when the flag says Dict. - `FrameDecoderState` gains a single owned liveness root, `active_dict: Option<DictionaryHandle>`, set at dict-apply via `set_active_dict` with a `ptr::eq` skip so re-applying the SAME dictionary frame-over-frame (the reuse hot path) clones zero times; one clone only on a genuine dictionary swap. The block loop borrows `&Dictionary` from it, disjoint from the mutable decoder scratch. - Removes `NonNull` dict fields, the `unsafe impl Send/Sync` blocks, `DecodeBuffer::set_dict`, and the handle-alive test scaffolding the raw-pointer API forced on callers. `FrameDecoder: Send + Sync` now auto-derives. byte-identical decode output; 794 tests pass.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
zstd/src/decoding/frame_decoder.rs (2)
1391-1405: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpdate
force_dictto installactive_dict.
force_dictarms Dict-sourced scratch tables but never callsset_active_dict, so later reads either panic onNoneor use a stale dictionary handle from a previous frame.Proposed fix
let dict = owned_dicts .get(&dict_id) @@ }) .ok_or(err::DictNotProvided { dict_id })?; state.decoder_scratch.init_from_dict(dict); + state.set_active_dict(dict); state.using_dict = Some(dict_id);Based on learnings, dictionary ownership should be stored once on decoder state and passed through the decode pipeline as a borrow-checked, call-scoped dictionary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zstd/src/decoding/frame_decoder.rs` around lines 1391 - 1405, `force_dict` in `frame_decoder::force_dict` only initializes `decoder_scratch` and updates `using_dict`, but never installs the active dictionary handle. Update this path to call `set_active_dict` with the resolved `dict` before returning, so later decode steps always see the current dictionary instead of `None` or a stale handle. Keep the borrow-scoped ownership flow consistent with the existing `owned_dicts`/`shared_dicts` lookup and the `state` fields used by the decoder.Source: Learnings
675-695: 🎯 Functional Correctness | 🔴 CriticalThread
dictthrough the LSM entropy snapshot calls
FSEScratch::reinit_fromandHuffmanScratch::reinit_resolved_fromnow take a dictionary borrow, butFrameDecoder::export_entropy/restore_entropystill call them with the old arity. This breaks thelsmbuild and leaves Dict-sourced tables unresolved in resume snapshots.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zstd/src/decoding/frame_decoder.rs` around lines 675 - 695, Thread the dictionary borrow through the LSM entropy snapshot flow by updating FrameDecoder::export_entropy and FrameDecoder::restore_entropy to pass the dict argument into FSEScratch::reinit_from and HuffmanScratch::reinit_resolved_from. Use the existing FrameDecoder/ResumeState paths to ensure both snapshot creation and restore use the same dictionary-backed table resolution, and update any helper calls in the Ring and Flat branches to match the new arity.zstd/src/decoding/sequence_section_decoder.rs (1)
139-155: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep
active_dictin sync with dict-attached scratch tables.These dict-aware table lookups now require every caller that has attached dictionary FSE state to pass the same active dictionary borrow. The supplied
FrameDecoder::force_dictcontext initializes scratch withstate.decoder_scratch.init_from_dict(dict)but does not callstate.set_active_dict(dict), while block decode later derivesdictfromstate.active_dict; forced-dictionary frames can therefore enter here with Dict-mode table sources anddict == None.Suggested fix in the caller
state.decoder_scratch.init_from_dict(dict); +state.set_active_dict(dict); state.using_dict = Some(dict_id);Based on learnings, dictionary ownership should be stored once on decoder state and threaded through as a call-scoped
dict: Option<&Dictionary>argument.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zstd/src/decoding/sequence_section_decoder.rs` around lines 139 - 155, The dictionary state used by sequence decoding is getting out of sync: `FrameDecoder::force_dict` initializes `decoder_scratch` from a dictionary but never updates `active_dict`, so `decode_sequence_section` can reach `SeqFSEDecoder::new` and `fse.*_table(dict)` with `dict == None`. Update the caller to set the active dictionary whenever scratch is initialized from a dict, and thread the same call-scoped `dict: Option<&Dictionary>` through the sequence decode path so `SequenceSectionDecoder::decode` and the `fse.*_table` lookups always use the same borrow.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@zstd/src/decoding/block_decoder.rs`:
- Around line 297-306: The forced-dictionary path is missing the state update
that makes the borrow visible to block decoding, so
`decompress_block_inplace_with_parts` can still see `dict` as `None` when
scratch tables expect a dictionary. Update `FrameDecoder::force_dict` to call
`state.set_active_dict(dict)` after initializing scratch from the provided
dictionary, and verify the `dict_ref` lookup in block decode continues to use
the decoder state’s active dictionary consistently. Add coverage for the
forced-dict Treeless/Repeat decode path to ensure dictionary-backed blocks do
not panic or reject matches.
In `@zstd/src/decoding/frame_decoder.rs`:
- Around line 915-921: `FrameDecoder::reset_with_format` leaves `active_dict`
populated, but the decode paths still derive `dict_ref` from it even after a
no-dict frame. Update the borrow flow in `FrameDecoder` so ext-dict match copies
only use `active_dict` when the current frame is actually dictionary-backed (for
example by checking `using_dict`/current frame state before building
`dict_ref`), or clear `active_dict` during reset; keep the change consistent
across the other decode sites that reuse the same `dict_ref` logic.
In `@zstd/src/decoding/frame_decoder/tests.rs`:
- Around line 9-15: Add a regression test for the forced-dictionary path in
FrameDecoder::force_dict to cover dict-backed frames that use the live borrow
from state.active_dict. Update force_dict so it calls
state.set_active_dict(dict) alongside decoder_scratch.init_from_dict(dict) and
using_dict, then extend the tests in frame_decoder/tests.rs to verify forced
dict decoding still works when the dictionary is owned by decoder state rather
than passed as a separate runtime argument.
---
Outside diff comments:
In `@zstd/src/decoding/frame_decoder.rs`:
- Around line 1391-1405: `force_dict` in `frame_decoder::force_dict` only
initializes `decoder_scratch` and updates `using_dict`, but never installs the
active dictionary handle. Update this path to call `set_active_dict` with the
resolved `dict` before returning, so later decode steps always see the current
dictionary instead of `None` or a stale handle. Keep the borrow-scoped ownership
flow consistent with the existing `owned_dicts`/`shared_dicts` lookup and the
`state` fields used by the decoder.
- Around line 675-695: Thread the dictionary borrow through the LSM entropy
snapshot flow by updating FrameDecoder::export_entropy and
FrameDecoder::restore_entropy to pass the dict argument into
FSEScratch::reinit_from and HuffmanScratch::reinit_resolved_from. Use the
existing FrameDecoder/ResumeState paths to ensure both snapshot creation and
restore use the same dictionary-backed table resolution, and update any helper
calls in the Ring and Flat branches to match the new arity.
In `@zstd/src/decoding/sequence_section_decoder.rs`:
- Around line 139-155: The dictionary state used by sequence decoding is getting
out of sync: `FrameDecoder::force_dict` initializes `decoder_scratch` from a
dictionary but never updates `active_dict`, so `decode_sequence_section` can
reach `SeqFSEDecoder::new` and `fse.*_table(dict)` with `dict == None`. Update
the caller to set the active dictionary whenever scratch is initialized from a
dict, and thread the same call-scoped `dict: Option<&Dictionary>` through the
sequence decode path so `SequenceSectionDecoder::decode` and the `fse.*_table`
lookups always use the same borrow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6264f0a4-2e75-42df-a690-2d110569a89c
📒 Files selected for processing (17)
zstd/src/decoding/block_decoder.rszstd/src/decoding/block_decoder/tests.rszstd/src/decoding/decode_buffer.rszstd/src/decoding/decode_buffer/tests.rszstd/src/decoding/frame_decoder.rszstd/src/decoding/frame_decoder/tests.rszstd/src/decoding/literals_section_decoder.rszstd/src/decoding/literals_section_decoder/burst_gate_tests.rszstd/src/decoding/literals_section_decoder/zerocopy_robustness_tests.rszstd/src/decoding/scratch.rszstd/src/decoding/scratch/tests.rszstd/src/decoding/seq_decoder_avx2.rszstd/src/decoding/seq_decoder_bmi2.rszstd/src/decoding/seq_decoder_scalar.rszstd/src/decoding/seq_decoder_vbmi2.rszstd/src/decoding/sequence_section_decoder.rszstd/src/decoding/sequence_section_decoder/tests.rs
A dictless-header frame decoded with a runtime dictionary applied via force_dict arms Dict-sourced scratch tables but, before the fix, never installs the owning dictionary handle (active_dict). The block loop then derives a None dictionary borrow while the scratch source says Dict, so the first dict-table read panics. This test fails on the current code.
… paths Three gaps in the borrow-checked dictionary threading, all surfaced by review of the prior refactor: - `force_dict` armed `Dict`-sourced scratch tables but never installed the owning `active_dict` handle (the other two dict-apply sites did). A dictless-header frame decoded with a runtime dictionary then derived a `None` borrow while the scratch said `Dict`, panicking on the first dict-table read. Add the missing `set_active_dict`. (regression test in the preceding commit) - The `lsm` resume-snapshot path (`export_entropy` / `restore_entropy`) still called `FSEScratch::reinit_from` / `HuffmanScratch::reinit_resolved_from` with the old arity, breaking the `lsm` build. Thread the dictionary borrow through: export passes the live frame's dictionary (the snapshot resolves Dict axes into self-contained Local bytes); restore passes `None` (the snapshot is already Local). - A reused decoder keeps `active_dict` across a no-dict frame so the per-apply `ptr::eq` reuse-skip works, but the decode loop derived the dictionary borrow from it unconditionally. A no-dict frame following a dict frame could then resolve a stray out-of-window offset against the stale dictionary content instead of erroring. Gate every `dict_ref` derivation (all four decode sites) on `using_dict.is_some()` so only a genuinely dict-backed frame sees the held dictionary. default + lsm test suites green (795 / 846); byte-identical decode output.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Jun 25, 2026
Closed
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the bulk of the btopt / btultra optimal-parser gap against libzstd on the
per-label-dictionary path, resolves the SIMD kernel tier once per encoder invoke
instead of per block, and fixes two correctness bugs (a Huffman height-limiter
panic and an out-of-range compression-level mis-resolution).
Perf: reshape the optimal-parse per-position find toward upstream's shape
Upstream's
ZSTD_insertBtAndGetAllMatchesis aFORCE_INLINE_TEMPLATE: the wholeper-position match-finder (rep probe + hash3 + binary-tree walk) is one inlined body,
called once per position, match-length template resolved once per block. Our path had
drifted into a chain of nested out-of-line calls paying per-position argument
marshalling upstream never pays.
ZSTD_btGetAllMatchesshape): repprobe + hash3 probe + BT tree walk folded into one out-of-line body, one call per
position, replacing a nested call chain.
24-byte struct was stack-copied every call).
past the no-match seed, dropped a discarded price recompute, skipped the no-op
plan-stats update on no-match segments.
Output is byte-identical to before across the suite; the encoder makes the same match
decisions, the work is just marshalled the way upstream marshals it.
Measured (i9-9900K, BMI2 + AVX2; L16 btopt, small-10k-random, compress-dict; libzstd
control flat at ~388 µs): pure Rust 932 µs -> 657 µs (-29%), 2.40x -> 1.70x vs
libzstd.
Refactor: resolve the SIMD kernel tier once per invoke, not per block
The HC / optimal / btlazy2 strategy entries and the BT-walker dispatchers called
select_kernel()per block (btultra2 twice: seed + main pass), re-reading the cachedOnceLockatomic inside the hot strategy path. The Fast / Row / Dfast matchers alreadycache the resolved tier in a field at construction; the binary-tree path now mirrors
that.
MatchTablecaches the tier, set once viaselect_kernel()at construction (onceper encoder invoke, since the matcher is built once per
FrameCompressor); everyper-block strategy entry and BT dispatcher reads the cached field.
select_kernel()isnow called exactly once per matcher, never inside a strategy or hot loop. Byte-identical
(the tier is constant for the process lifetime).
Fix: Huffman height limiter could leave a non-power-of-two Kraft sum
The height limiter (upstream
HUF_setMaxHeightequivalent) walked the cost-repair fillin the wrong direction on certain <=128 KB literal histograms, leaving a code whose
Kraft sum was not a power of two. The table builder rejects such a code, surfacing as a
panic. The limiter now fills downward correctly and validates the Kraft sum, falling
back to the distributed-weight construction when it cannot reach the bit-length cap; the
dead repair helper is removed.
Fix: out-of-range compression levels mis-resolved
resolve_level_paramsspecial-cased onlyLevel(22)for the btultra2 source-sizeresolver;
Level(23+)fell through to the generic cParams path and resolved a differentconfiguration than the max level. Now
Level(n)forn >= MAX_LEVELclamps through thesame resolver.
Perf: hold the decode dictionary by borrowed pointer (mirror C zero-refcount apply)
Upstream zstd's
ZSTD_copyDDictParametersstores RAW POINTERS into a caller-ownedDDictevery frame (
dctx->LLTptr = ddict->entropy.LLTable,prefixStart = dictContent) — norefcount, no per-frame clone. We held an owned
Arc<Dictionary>in three places (FSE +Huffman scratch + decode buffer) and re-cloned all three each frame, dropping them at
teardown: six atomic refcount ops per frame on a value the dictionary owner already keeps
alive for the whole decode. The three scratch sites now hold
Option<NonNull<Dictionary>>,set from a live
&DictionaryHandleeach frame (one pointer store) and read through achecked
unsafederef while the COW table-source isDict. The pointee is kept aliveacross the decode by the dictionary owner (the
FrameDecoderregistry, or the&DictionaryHandlethe caller holds acrossdecode_all_with_dict_handle).NonNullsuppresses auto-
Send/Sync, sounsafe impl Send/Syncis restored on each scratch type(read-only borrow of
Send + Syncdata), with a compile-time assert pinningFrameDecoder: Send + Sync. Byte-identical. i9 small-10k-randomdecompress-dict, flatc_fficontrol: L3 dfast -2.3%, L16 btopt -1.8%.Tests
bt_optimal_all_kernel_tiers_emit_identical_sequences— forces each cached kernel tierand asserts identical sequence streams (scalar/SIMD bit-identity + per-tier wrapper
coverage on one machine).
levels_above_max_resolve_identically_to_max— regression for the level-clamp fix.level_params_strategy_and_search_method_agree_across_tiers— strategy/search-methodconsistency across the cParams source-size tiers.
build_limited_weights_handles_degenerate_alphabets— single-symbol / all-zeroheight-limiter early-out.
plus a 300k-case
#[ignore]stress sweep.Validation: full suite green (851 on x86 incl. dict_builder),
cargo clippy -D warningsclean,
cargo fmt --checkclean.Summary by CodeRabbit
New Features
Bug Fixes
Tests