perf: O(log) cold one-shot maximal exact match (forward + backward) - #23
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors forward/backward SMEM search to use O(log n) maximal-prefix probes via new LCP helpers; ChangesSMEM Spectrum Maximal Search Optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@prmi/src/index/spectrum.rs`:
- Around line 1463-1486: The new one-shot branches under the self.kmt.is_none()
path bypass forward_spectrum_* / backward_spectrum_* and thus must reintroduce
the packed-PAC fail-closed guard: before calling
tokenize_32mer/forward_maximal_len/lookup/find_boundary, check the PacEncoding
variant (PacEncoding::Packed) and validate the PAC size/stride the same way
forward_spectrum_* does (so packed PACs that are undersized cause an early
return of the zero/empty MemMatch rather than proceeding into
fill_doubled_chunk/pac_base_at(...).unwrap()). Implement this guard at the start
of the block (the branch that computes match_len, qm, pred, lower/upper) and
return zero if the packed check fails; reference the functions/paths involved:
forward_maximal_len, lookup, find_boundary, ref_less, shares_prefix,
fill_doubled_chunk, and pac_base_at to locate the relevant logic to mirror.
- Around line 1634-1643: In mem_search_backward: anchor_end is computed with
pivot + anchor_len as usize which truncates a u64 and can overflow; change to
try converting anchor_len to usize with usize::try_from and then use
pivot.checked_add to compute anchor_end, returning the zero-length MemMatch
early on conversion failure or overflow (same pattern used in
mem_search_backward_from_hint and the backward spectrum prototypes); update all
uses of anchor_end, anchor_len, and pivot in mem_search_backward to follow that
fail-closed behavior to avoid panics/wraps.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a08c8617-2e62-44f0-b953-b64afdfba49c
📒 Files selected for processing (2)
prmi/examples/probe_audit.rsprmi/src/index/spectrum.rs
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
prmi/src/index/spectrum.rs (1)
1697-1701: Clarify overflow risk inmem_search_backwarddepends on the 2-bit input contract
mem_search_backwardoperates on 2-bit encoded bases (0..=3): callers/docstrings for the spectrum APIs assume this, andtokenize_32merdebug-assertsv <= 3. Under that contract,3 - bat line 1700 cannot overflow, and it’s the correct 2-bit reverse-complement. If malformed (bytes>3) inputs must be handled defensively, this code currently lacks a guard for the anchor span; mask/complement like(b & 0x3) ^ 0x3(same semantics asreverse_complement_2bit) or validate and fail closed.🤖 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 `@prmi/src/index/spectrum.rs` around lines 1697 - 1701, The slice reversal uses 3 - b assuming 2-bit bases, which is safe only if callers uphold that contract (see mem_search_backward and tokenize_32mer); to be defensive either (a) mask-and-complement each byte to 2 bits like (b & 0x3) ^ 0x3 (matching reverse_complement_2bit semantics) when building q, or (b) validate the anchor bytes are <= 3 and return an error/None on violation; update the code that computes q (the read[q_start..anchor_end].iter().rev().map(...) chain) to use one of these two approaches and ensure callers of mem_search_backward/documentation reflect the chosen behavior.
🤖 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.
Nitpick comments:
In `@prmi/src/index/spectrum.rs`:
- Around line 1697-1701: The slice reversal uses 3 - b assuming 2-bit bases,
which is safe only if callers uphold that contract (see mem_search_backward and
tokenize_32mer); to be defensive either (a) mask-and-complement each byte to 2
bits like (b & 0x3) ^ 0x3 (matching reverse_complement_2bit semantics) when
building q, or (b) validate the anchor bytes are <= 3 and return an error/None
on violation; update the code that computes q (the
read[q_start..anchor_end].iter().rev().map(...) chain) to use one of these two
approaches and ensure callers of mem_search_backward/documentation reflect the
chosen behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 371ecfc9-8d6c-49d4-9aca-6c69f07d6160
📒 Files selected for processing (2)
prmi/examples/probe_audit.rsprmi/src/index/spectrum.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- prmi/examples/probe_audit.rs
mem_search_backward routed every call through a per-base re-search loop — a model launch plus two bounded searches at every left base, O(extension-length). The consumer's zigzag calls it with a length-1 pivot anchor (occ ~ n/4), so the interval is maximally ambiguous and the loop walks the entire ~16-20 bp head one base at a time. BWA-MEME uses the same length-1 anchor yet costs ~3 probes/call, because its extension is a single O(log) bounded search. Two fast paths replace the loop: - occ_count == 1: the 1-wide interval pins the genomic locus, so the maximal left extension is a probe-free leftward reference walk (doubled_base_at) plus one interval recovery — sa_start is itself a valid est_hint (422 -> 7 probes/call). - occ_count > 1: a single bounded LCP search. The maximal left extension is the longest SUFFIX of read[..anchor_end] that occurs, which is the longest PREFIX of its reverse-complement that occurs — and prefixes nest, so one maximal- prefix search finds the whole extension regardless of length. The 2x index already holds the RC suffixes and occ is strand-symmetric (occ(RC(P)) == occ(P)), so match_len and occ are exact. forward_maximal_len finds the length (a model-launched insertion-point search plus the two neighbor LCPs — the deepest match is always adjacent to where the query sorts); the forward interval of the maximal pattern is then recovered for sa_start/occ. Synthetic cold backward one-shot: genuine repeat (occ ~2028) 1880 -> 53, collapse 16 -> 26, unique 422 -> 7. O(log n) for every occ>1 case, independent of extension length. The per-base backward TRACE (backward_spectrum) is unchanged; add the probe_audit "collapse" corpus to exercise the ambiguous-head/unique-tail shape. Byte-identical: match_len is the same maximal span and the interval is recovered for the same pattern, proven against two independent oracles (mem_search_backward_equals_maximal_backward_step, mem_search_backward_hint_equals_unhinted). On-disk index format unchanged.
This was referenced Jun 11, 2026
Merged
This was referenced Jun 22, 2026
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.
PR #10 of the v0.2 stack (carries
8f85620, with65b99c4folded in per the plan). Base isfeat/v0.2-gallop(#22). Tracked inV0.2_PR_STACK.mdrow #10.What this does
O(log n) cold one-shot maximal exact match for both directions (BWA-MEME's cold-search cost), replacing the per-base O(extension-length) walk in
mem_search/mem_search_backwardwhen no.kmtis loaded:65b99c4) —forward_maximal_lenfinds the maximal match length with one O(log) maximal-prefix search, then recovers the interval once.8f85620) — the longest left extension is the longest prefix of the anchor's reverse-complement that occurs (the 2× index already holds RC suffixes;occis strand-symmetric), so one O(log) RC-strand search finds it. Plus a unique-anchor (occ==1) probe-free fast path viamem_search_backward_from_hint.forward_maximal_len, so65b99c4is folded into8f85620(one commit). Depends onmem_search_backward_from_hintfrom feat(prmi): cleanroom trainer (uniform weighting) + shared lookup math #6 (3acd729) — satisfied (it's below this in the stack).Byte-identical by construction, and gated by the oracle tests:
mem_search_equals_maximal_forward_step,mem_search_backward_equals_maximal_backward_step,mem_search_*_hint_equals_unhinted, and the FFI parity tests.Conflict resolution
spectrum.rs(both commits): #10 rewrites the bodies ofmem_search/mem_search_backwardthat #5 had refactored to the sink form. Took #10's complete O(log) rewrite for both (it supersedes the sink-based one-shot); the carried packed/bounds guards in the spectrum cores are untouched (non-conflicting regions).Green-pass
-D warnings✅ · nightly fmt ✅ ·cargo test --workspace✅ (162 lib + all FFI; the byte-identity + oracle proptests above; only Plan-3 deferralsignored).Summary by CodeRabbit
Tests
Refactor
Bug Fixes
Documentation