perf(spectrum): hybrid point/window boundary seed + keyed-accessor fast path - #64
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughAdds keyed-mode fast-path entry accessors to SaFileReader and LearnedIndex, an option-free keyed suffix comparator, a branchless find_boundary binary-search update, and a refactored seeded boundary-search that unifies point/window seeding via forward_boundary_windowed_fallback. Also bumps Rust MSRV and switches a bloom-file modulo check to is_multiple_of. ChangesKeyed SA access and boundary search refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Risk: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nh13
force-pushed
the
perf/interval-recovery
branch
from
July 6, 2026 05:34
ad9d557 to
0213444
Compare
…st path
Seed forward_maximal_len_seeded's boundary gallop by model-window width:
* NARROW window (confident, width <= 64 -- the common case): seed
find_boundary at the model's POINT prediction ([center, center+1)). It
gallops outward with probes clustered near the prediction (bwa-meme's
mem_search_tradeoff access pattern), which is cache-local once the SA
spills to DRAM.
* WIDE window (uncertain): seed at the whole [pred-err, pred+err+1), which
already brackets the boundary.
Both seed the SAME seed-independent find_boundary, so the choice is
byte-identical -- width only picks the seed.
Measured on x86 (c7i.4xlarge, 100M SA, DRAM-bound, same-box base vs this
commit): cold forward mem_search on UNIQUE reads (~96% of SMEMs) is 46.7 ->
33.0 us = -29%. The high-occ tandem-repeat stress case is 180.5 -> 196.0 us =
+8.6% (an earlier pure point-seed revision was +22% there; the hybrid softens
but does not eliminate it -- a repeat 32-mer has a narrow model window yet a
wide SA interval, and occ is unknown before the search, so no a-priori
window-width test routes it to the window seed). Net wall-positive under any
realistic occurrence mix; the regression is confined to pathological high-occ
regions.
Add an Option-free keyed probe for mode-2/3 sidecars: has_stored_keys() +
sa_entry_keyed() read (position, key) from one cache line with no release-mode
bounds assert, no mode branch, and no Option wrap, feeding
compare_query_vs_suffix_2x_keyed_with_mask_k. The generic mode-1 path is
unchanged.
Make the final bisection in find_boundary branchless via
core::hint::select_unpredictable: the direction test is a ~50/50 coin flip
near the boundary, so a data-dependent branch mispredicts ~half the probes;
the conditional-select leaves the cold SA read as the only stall. This needs
Rust >= 1.88, so raise the workspace MSRV to 1.96 (latest stable) and, for a
clean -D warnings build on that toolchain, adopt u64::is_multiple_of in
bloom_file.rs (a lint stabilized in the newer clippy; no behavior change).
Byte-identity gates: forward_boundary_seed_independent pins the seeded search
(point seeds AND a wide window) against a full-SA galloping oracle;
keyed_equals_scalar_mode2 now also covers the _k comparator against the scalar
reference across a full SA sweep (including near-sentinel entries).
nh13
force-pushed
the
perf/boundary-search
branch
from
July 6, 2026 07:04
3d8fb77 to
5491c6d
Compare
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Author
|
@coderabbitai review |
Action performedReview triggered.
|
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.
Second of a three-PR stack. Stacked on #63 (
perf/interval-recovery) — review that first; this PR's diff is against it.What
Three byte-identical changes to the forward maximal-match boundary search:
forward_maximal_len_seedednow seeds the seed-independentfind_boundaryby model-window width: a narrow window (width <= 64, the confident/common case) seeds at the model's POINT prediction ([center, center+1)), so the gallop's probes cluster near the prediction (cache-local once the SA spills to DRAM); a wide window seeds at the whole[pred-err, pred+err+1), which already brackets the boundary. Byte-identical either way — width only picks the seed.Option-free keyed probe for mode-2/3 sidecars:has_stored_keys()+sa_entry_keyed()read(position, key)from one cache line with no release-mode bounds assert, no mode branch, and noOptionwrap, feeding a newcompare_query_vs_suffix_2x_keyed_with_mask_k. The generic mode-1 path is unchanged.find_boundaryviacore::hint::select_unpredictable— the direction test is a ~50/50 coin flip near the boundary, so a data-dependent branch mispredicts ~half the probes; the conditional-select leaves the cold SA read as the only stall.Because (3) needs
core::hint::select_unpredictable(stable since Rust 1.88), this PR raises the workspace MSRV to 1.96 (latest stable) and adoptsu64::is_multiple_ofinbloom_file.rsfor a clean-D warningsbuild on that toolchain (a lint stabilized in the newer clippy; no behavior change).Wall (x86, measured)
mem_search_benchonc7i.4xlarge(AVX-512),PRMI_BENCH_REFLEN=100M(≈2.6 GB SA, DRAM-bound), same-boxmainvs this branch:mem_search_forward/model_launchThe benefit is a cache-locality effect: at 500 kbp (SA fits in L3) the same bench is neutral; the −29% appears only once the SA is DRAM-resident. The hybrid softens but does not eliminate the high-occ regression — a tandem-repeat 32-mer has a narrow model window (the RMI predicts its sort position tightly) yet a wide SA interval, and occ is unknown before the search, so no a-priori window-width test routes it to the window seed. Net wall-positive under any realistic occurrence mix; the regression is confined to pathological high-occ regions.
Correctness
Byte-identical.
forward_boundary_seed_independentpins the seeded search (point seeds and a wide window) against a full-SA galloping oracle;keyed_equals_scalar_mode2now also covers the_kcomparator against the scalar reference across a full SA sweep including near-sentinel entries; the existingmem_search/warm-start oracle proptests continue to gate the integrated result.Summary by CodeRabbit
New Features
Bug Fixes
Chores