perf(opt): fold BT-walk coordinate decode; drop dead hash-chain optimal path#462
Merged
Conversation
The optimal parser's per-position match-finder always used the binary-tree branch (every production call site selects it): upstream zstd's optimal parser (ZSTD_insertBtAndGetAllMatches) is binary-tree only, with no hash-chain variant — the chain is upstream's lazy parser (ZSTD_HcFindBestMatch), a different path. The else-branch hash-chain optimal walk and its chain_candidates helper had no upstream counterpart and were dead in every shipped binary (const-folded out, absent from objdump). Remove the dead else-branch, the chain_candidates helper, and the two tests that exercised the chain optimal path through the test-only dispatcher; the BT path's own skip-window / rebase coverage remains. Byte-identical.
With the hash-chain optimal fallback gone, the per-position match-finder is unconditionally the binary-tree walk, so the USE_BT_MATCHFINDER const generic (and the macro metavars that only fed the removed chain branch: bt_insert_and_collect method handle, for_each_repcode, hash3) carried no information. Remove the const generic from the collect entry points and the four kernel wrappers, and drop the dead macro parameters. Byte-identical.
The BT match-finder's hot per-node loop decoded the chain entry into three coordinates each iteration: candidate_abs (position_base + stored - 1 - index_shift), the BT pair slot (via bt_pair_index_for_abs, which re-read index_shift and bt_mask and round-tripped index_shift back in), and candidate_idx (candidate_abs - history_abs_start). That reloaded four struct fields and ran ~6 arithmetic ops per node. Precompute the three loop-invariant biases once before the walk so each coordinate is a single wrapping_add from the stored entry — the single-coordinate form of upstream zstd's window-relative matchIndex (match = base + matchIndex, slot = 2*(matchIndex & btMask)). Byte-identical: the folded values equal the originals for every gate-validated entry.
|
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 (1)
📝 WalkthroughWalkthroughThis PR precomputes BT-walk decode biases, removes ChangesHC/BT matchfinder candidate decoding refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
The optimal candidate collector is binary-tree only; the Fast / Dfast / Greedy / Lazy strategies never run the optimal parser (they drive their own match finders) and keep chain_table as an HC chain, not BT pair slots. The public dispatcher's non-BT arm previously routed those tags into the BT collect, which would walk the HC chain as a binary tree. Make that arm unreachable so misuse fails loudly, and tag the test-only shim callers as a BT strategy (BtOpt shares Lazy's OPT_LEVEL=0 / USE_HASH3=false consts, so the collect behavior is unchanged). No production caller hit the non-BT arm (the on-encode path goes through build_optimal_plan_impl directly).
Adds a should_panic test proving a non-BT strategy tag reaching collect_optimal_candidates panics (the binary-tree-only contract), covering the unreachable arm, and a dispatch test exercising every BT tag (BtOpt / BtUltra / BtUltra2 / Btlazy2) under the scalar kernel so the dispatcher's per-tag and scalar arms are covered.
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/encoding/match_generator/tests.rs`:
- Around line 1350-1392: The test in
hc_collect_optimal_candidates_dispatches_every_bt_strategy only checks that each
StrategyTag variant runs without panicking, so it can miss dispatch mistakes.
Strengthen it by adding a tag-specific observable from
collect_optimal_candidates or the resulting out buffer that differs per BT
strategy, and assert on that value inside the loop. Use the existing
HcMatchGenerator, StrategyTag variants, and FastpathKernel::Scalar setup to
verify each tag is routed to its intended collector specialization rather than
just surviving execution.
🪄 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: 38d29417-7c24-48ba-9872-a0052c252603
📒 Files selected for processing (2)
zstd/src/encoding/hc/optimal.rszstd/src/encoding/match_generator/tests.rs
The dispatch test previously only proved each BT tag ran without panicking, so a cross-group mis-mapping (e.g. BtUltra routed to the BtOpt specialization) would slip through. Add a USE_HASH3 observable: a fixture where `abc` repeats with a differing 4th byte, so the hash3 specializations (BtUltra / BtUltra2) surface a length-3 match at offset 12 that the 4-byte BT hash misses, while the non-hash3 ones (BtOpt / Btlazy2) do not. Assert the match's presence equals the tag's USE_HASH3. (BtOpt vs Btlazy2 share identical collect consts, so they are runtime-indistinguishable; that mapping is compiler-enforced.)
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
Cleans up the optimal parser's match-finder and shaves the per-node cost of its
hot binary-tree walk. Three changes, all byte-identical output:
Drop the non-upstream hash-chain optimal fallback. The optimal parser's
per-position match-finder always took the binary-tree branch in every shipped
build (the
USE_BT_MATCHFINDER=falsechain path was const-folded out — absentfrom the optimized binary). Upstream zstd's optimal parser
(
ZSTD_insertBtAndGetAllMatches) is binary-tree only; the hash chain is itslazy parser (
ZSTD_HcFindBestMatch), a separate path. The fallback'selse-branch + thechain_candidateshelper had no upstream counterpart andwere dead. Removed them and the two tests that exercised the chain path through
the test-only dispatcher; the BT path keeps its own skip-window / rebase
coverage.
Drop the now-vestigial
USE_BT_MATCHFINDERconst generic. With the chainfallback gone the per-position finder is unconditionally the BT walk, so the
const generic (and the macro metavars that only fed the removed branch) carried
no information. Removed from the collect entry points and the four kernel
wrappers.
Fold the per-node coordinate decode in the BT walk (perf). The hot
per-node loop decoded each chain entry into three coordinates per iteration
(absolute position, BT pair slot, history index), reloading four struct fields
and round-tripping
index_shiftthrough the slot computation. Precompute theloop-invariant biases once before the walk so each coordinate is a single
wrapping_addfrom the stored entry — the single-coordinate form of upstream'swindow-relative
matchIndex(match = base + matchIndex, slot2*(matchIndex & btMask)). The BT slots are now keyed context-relative tomatch upstream. Byte-identical: the folded values equal the originals for every
gate-validated entry.
Results (i9, x86_64, ours-vs-
c_ffi, flat control)compress-dictlevel_19_btultra2small-10k-random: −1.3 % on theper-position BT find (flat
c_fficontrol), byte-identical. Net−55lines fromthe dead-code removal.
Testing
cargo nextest run -p structured-zstd --features hash,std,dict_builder— 837 pass;-p ffi-bench --features bench_internals,dict_builder— 59 pass(cross-validation round-trips + fuzz_interop).
--tests,--no-default-features --features kernel_scalar,hash)and
cargo fmt --check— clean.Note on the optimal-collect dispatcher
Optimal candidate collection is binary-tree only (btopt / btultra / btultra2
/ btlazy2). The
Fast/Dfast/Greedy/Lazystrategies never run theoptimal parser — they drive their own match finders and keep
chain_tableas anHC chain, not BT pair slots. The public
collect_optimal_candidatesdispatcher'snon-BT arm is therefore
unreachable!(): a non-BT tag reaching it is a callerbug and panics loudly instead of walking the HC chain as a binary tree. Only the
test-only shim calls this entry, and it tags a BT strategy. The on-encode hot
path bypasses the dispatcher entirely (it calls
build_optimal_plan_impl_<kernel>with a BT strategy directly).
Summary by CodeRabbit