perf(spectrum): windowed model-locate skips find_boundary gallop edge-checks - #58
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 (1)
WalkthroughAdds ChangesWindowed forward-boundary fast path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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
🧪 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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 3535-3536: The condition at line 3535 performs arithmetic on `lo0`
with `lo0 + 2`, which can overflow if `lo0` is near the maximum u64 value.
Instead, compute the window width once as `hi0 - lo0` (which is safe since the
invariant ensures `hi0 >= lo0`), store it in a variable, and then use this width
for both the comparison with 2 and the comparison with WINDOW_FAST_PATH_CAP.
Replace the condition `hi0 <= lo0 + 2` with a comparison of the computed width
(e.g., `width <= 2`), eliminating the wrapping risk from direct arithmetic on
`lo0`.
- Around line 3490-3571: Add tests to verify the correctness of the new windowed
boundary search logic. Create test cases in the spectrum_oracle.rs test suite
that exercise the fast path of forward_boundary_windowed when the boundary falls
strictly inside the window (lo0, hi0), the fallback path when the window is too
small (<=2) or exceeds WINDOW_FAST_PATH_CAP (>64), and verify that
forward_maximal_len_seeded produces byte-identical results when called with
seed_win=None (trusted window) versus seed_win=Some(...) (hinted window). Use
deterministic or proptest cases to run both paths on the same input and assert
output equality, ensuring the reused neighbor LCPs and fallback edge recovery
paths are correct.
🪄 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
Run ID: 2e9c273e-c7cd-4413-8db8-51c31dc2e0e7
📒 Files selected for processing (1)
prmi/src/index/spectrum.rs
0c1534c to
e8fd6f4
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 `@prmi/src/index/spectrum.rs`:
- Around line 5683-5694: The strictly-inside window defined in the windows array
at index 0 (with the comment about strictly-inside) is not actually strict when
ip is at boundary values like 0 or sa_num. To fix this, add a reference-lifted
query that sets up test conditions where ip is positioned in the interior of the
search space, and then add an assertion that verifies at least one window
satisfies the strict interior property: win_lo < ip < win_hi with a window width
between 3 and 64 inclusive. This ensures the fast path is actually being
exercised in the test rather than only testing fallback behavior.
🪄 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
Run ID: 11e0aebe-d78e-4b59-b387-cca01b0802fb
📒 Files selected for processing (1)
prmi/src/index/spectrum.rs
…-checks The forward model-launch locate runs find_boundary, which pays 2 gallop edge-check probes on top of the bsearch to stay robust to a boundary that falls outside the model window. bwa-meme's cold locate (CURR_SEARCH_METHOD==1) binary-searches its model window directly. forward_boundary_windowed does the same: it bsearches the model window [win_lo, win_hi) first; by monotonicity of the ref-vs-query comparator over the SA, an insertion point STRICTLY inside the window IS the global boundary (both neighbors were probed by the bsearch), so it returns byte-identical (ip, lcp_lo, lcp_hi) with zero edge probes and no separate lcp_at calls (the window bsearch's ip-1/ip probes ARE the neighbor LCPs). Only an ip landing AT a window edge falls back to the robust gallop. Wide windows (uncertain model / high-occ repeats) frequently put the boundary OUTSIDE the window, where a full window bsearch is wasted work vs find_boundary's 2-probe edge check; a WINDOW_FAST_PATH_CAP (64) sends those straight to the gallop so the fast path is taken only for tight, confident windows. The cap only selects which path computes the identical boundary, so the result is byte- identical for any window. Applied to the model-window locate (seed_win=None) only; hinted/seeded callers (warm-start) keep the gallop. Measured (probe_audit, synthetic 4M-bp ref, mean SA probes/call): forward model_launch / unique 13.9 -> 10.0 (-28%) forward model_launch / repeat (high-occ)42.0 -> 42.0 (neutral, cap) est_hint_interval / unique (untouched) 8.0 -> 8.0 Byte-identical: prmi lib + integration suites pass (the lone release-mode should_panic failure is the pre-existing debug_assert artifact, passes in debug).
e8fd6f4 to
180d443
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
What
The forward model-launch locate (
forward_maximal_len_seededwithseed_win = None) runsfind_boundary, which pays 2 gallop edge-check probes on top of the bsearch to stay robust to a boundary that falls outside the model window. bwa-meme's cold locate (CURR_SEARCH_METHOD==1) binary-searches its model window directly.This adds
forward_boundary_windowed, which does the same: it bsearches the model window[win_lo, win_hi)first. By monotonicity of the ref-vs-query comparator over the SA, an insertion point strictly inside the window IS the global boundary (both neighbors were probed by the bsearch), so it returns the byte-identical(ip, lcp_lo, lcp_hi)with zero edge probes and no separatelcp_atcalls — the window bsearch'sip-1/ipprobes ARE the neighbor LCPs. Only aniplanding AT a window edge falls back to the robust gallop.This is the un-upstreamed remnant of the stale
feat/v0.2-2x-spectrumbranch's10374cc, re-expressed against currentmain'sfind_boundary+ separate-lcp_atshape (the old fusedforward_boundary_with_lcpit patched no longer exists). The companionefd9fcf(#[inline]oncompare_query_vs_suffix_2x_from) is already inmain, so it is not included.Repeat-safe cap (new vs the original)
A/B benching surfaced a regression the original commit (measured on unique reads only) never saw: for wide model windows — an uncertain model, e.g. high-occ repeats — the boundary frequently falls outside the window, where a full window bsearch is wasted work vs
find_boundary's 2-probe edge-check. AWINDOW_FAST_PATH_CAP(64) sends wide windows straight to the gallop, so the fast path is taken only for tight, confident windows. The cap only selects which path computes the identical boundary, so the result is byte-identical for any window.Measured (
probe_audit, synthetic 4M-bp ref, mean SA probes/call)Without the cap, the repeat corpus regressed to 54.0 (+29%); the cap restores it to baseline while keeping the unique win.
Byte-identity
seed_win = Some(..)(warm-start / hinted) callers keep the gallop unchanged.should_panicfailure (doubled_text_rejects_out_of_range_base) is the pre-existingdebug_assertartifact (passes in debug), present on every revision — not introduced here.mem_search_equals_maximal_forward_step,mem_search_warmstart_equals_cold,forward_spectrum_equals_oracle, backward oracles — all green.Review
Dual-reviewed pre-push: CodeRabbit CLI (
coderabbit review --agent) + the local CodeRabbitAI-style skill. CLI flagged two#[inline]consistency findings (matchingfind_boundary/ref_less/lcp_at), both applied.Summary by CodeRabbit