perf(spectrum): decode the packed reference a word at a time - #47
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (3)
WalkthroughExtends ChangesPacked PAC validation contract and bulk decode refactor
wall_gate benchmark harness
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/examples/wall_gate.rs`:
- Around line 49-50: After reading the PAC file into the pac variable using
std::fs::read on pac_path, add a validation check to ensure the size of the pac
data is sufficient to encode l_pac bases before constructing the
PacEncoding::Packed structure. Implement a fail-fast size validation that
compares the pac buffer length against the expected minimum size required for
l_pac and either error out or panic with a clear message if the validation
fails, preventing downstream instability in probe/decode paths.
- Around line 97-100: The benchmark metrics computation in the println! macro
does not guard against zero-read scenarios, which would cause division by zero
or invalid output when n equals zero. Add a guard condition that checks if n ==
0 before the println! statement (which computes ns/read and reads/sec metrics),
and exit early using return or break if no reads were processed. This ensures
metrics are only calculated and displayed when valid data exists.
- Around line 54-57: The current FASTQ parsing loop silently ignores parse
errors and missing lines by using let _ = lines.next() for lines 3 and 4, which
can cause incomplete records to be counted. Refactor the loop to strictly
enforce parsing all 4 lines of each FASTQ record as a complete block by checking
that lines.next() returns Some(Ok(_)) for all four lines (header, sequence,
plus, and quality), and propagate any errors or unexpected None values by
breaking or returning an error instead of silently continuing. This ensures that
parsing failures and truncated records are caught and fail-closed rather than
silently skipped.
In `@prmi/src/index/spectrum.rs`:
- Around line 113-116: In the unpack_packed_forward function, the conversion of
the start parameter from u64 to usize on the line where pos is assigned must be
changed from an unchecked cast to a checked conversion. Replace the direct cast
`start as usize` with either `usize::try_from(start).expect("start exceeds
usize::MAX")` to fail explicitly if the value exceeds the maximum usize, or add
a `debug_assert!(start <= usize::MAX as u64)` before the assignment to document
the safety assumption. This ensures compliance with the fail-closed semantics
requirement for all u64 to usize narrowing conversions.
🪄 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: e48e13f2-341c-496b-9fde-bd0cdd08a2f5
📒 Files selected for processing (2)
prmi/examples/wall_gate.rsprmi/src/index/spectrum.rs
03ff4e9 to
3ed194b
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@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 `@prmi/examples/wall_gate.rs`:
- Around line 69-70: Replace the `div_ceil(4)` operation on `l_pac` in the
`needed_pac_bytes` assignment with checked arithmetic to prevent silent
wraparound in release builds. Instead of using `div_ceil(4)` directly, use
`checked_add(3)` followed by appropriate division (or use a checked division
method if available) to ensure overflow is detected and fails explicitly rather
than producing an incorrect value. The `try_from` call alone cannot catch the
wrapped result from `div_ceil(4)`, so the arithmetic itself must be checked
before the type conversion.
In `@prmi/src/index/spectrum.rs`:
- Around line 113-145: The function `unpack_packed_forward` performs blind
indexing into the `pac` buffer without validating that the `num_bases` count
from the packed encoding matches the actual length of the packed data (`l_pac`).
This can cause panics or out-of-bounds reads. You must add validation before any
indexing operations occur in this function and related bulk decoder functions
(also affecting lines 175-176 and 198-210). Either validate at the call site
that `num_bases == l_pac` and `pac.len() >= ceil(l_pac / 4)` before invoking
these helpers, or pass `num_bases` as a parameter to `unpack_packed_forward` and
return an error or perform bounds checking instead of using unwrap() and blind
indexing operations like `pac[pos >> 2]` and `pac[base..base + 8]`. Ensure the
validation fails closed rather than panicking on malformed public input.
- Around line 3864-3895: In the `packed_word_decode_all_offsets_equals_scalar`
test function, expand the query lengths being tested from just `[31, 32, 33]` to
also include `34` and `35` to ensure the u64 word loop executes even with offset
1 and 2. Additionally, modify the twist mutation strategy to mutate from the
actual generated query value instead of the current hardcoded approach of
forcing base 2 at position 5, ensuring that every twisted query is guaranteed to
differ from the original and trigger early-mismatch returns during decode, as
per the byte-identity-critical search-core testing guidelines.
🪄 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: 0703dbdb-2137-402d-aa3c-948fab3313c9
📒 Files selected for processing (2)
prmi/examples/wall_gate.rsprmi/src/index/spectrum.rs
3ed194b to
d075c8f
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@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`:
- Line 1032: Add validate_packed_pac(pac, num_bases, self.l_pac(),
<method_name>) guards to all public methods that accept PacEncoding::Packed
before they access or operate on the packed data. Specifically, add these guards
to the entry points of build_kmer_table, forward_spectrum_from_hint,
backward_spectrum_tabled, backward_spectrum_from_hint, and
backward_spectrum_lockstep methods. Each guard should fail closed by checking if
the validation returns an error and handling it appropriately, just like the
pattern shown in forward_spectrum at line 1032. This ensures that malformed
PacEncoding::Packed inputs cannot reach the bulk compare/decode helpers or
trigger SA probes without proper validation of the num_bases constraint.
🪄 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: 96865724-a82f-40d2-953f-0f7d478a15dd
📒 Files selected for processing (3)
prmi/examples/wall_gate.rsprmi/src/index/smem.rsprmi/src/index/spectrum.rs
d075c8f to
2137eec
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 3935-3978: Add a new test function that directly tests the
unaligned lead-in and u64 word decode path in unpack_packed_forward. Create this
test to call unpack_packed_forward directly with an output buffer length of at
least 35 bases and a start position where start % 4 is not 0 to ensure the
unaligned lead-in path executes. Compare the vectorized decode results against a
scalar oracle implementation across various unaligned start offsets and output
lengths to verify correctness, since the current
packed_word_decode_all_offsets_equals_scalar test does not actually exercise
this path due to CHUNK_BASES capping at 32 in fill_doubled_chunk.
🪄 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: 52d1cf5d-0aa5-4f51-89e7-3f55a91e9287
📒 Files selected for processing (3)
prmi/examples/wall_gate.rsprmi/src/index/smem.rsprmi/src/index/spectrum.rs
2137eec to
99fcbfa
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
prmi/src/index/spectrum.rs (1)
164-185:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck doubled-length arithmetic before using RC bounds.
One-line fix: compute
2 * l_pacwithchecked_mulonce and reuse it; otherwise a malformedl_pac > u64::MAX / 2wraps before the sentinel guard and can map packed RC probes to the wrong forward run.Proposed fix
fn fill_doubled_chunk(pac: &[u8], enc: PacEncoding, l_pac: u64, q: u64, out: &mut [u8]) -> usize { - if out.is_empty() || q >= 2 * l_pac { + if out.is_empty() { + return 0; + } + let Some(two_l_pac) = l_pac.checked_mul(2) else { + return 0; + }; + if q >= two_l_pac { return 0; } @@ - let avail = (2 * l_pac - q) as usize; + let avail = (two_l_pac - q) as usize; @@ - let hi = (2 * l_pac - 1 - q) as usize; // mirror of q (highest fwd pos) + let hi = (two_l_pac - 1 - q) as usize; // mirror of q (highest fwd pos) @@ - let hi = 2 * l_pac - 1 - q; // forward source of out[0] + let hi = two_l_pac - 1 - q; // forward source of out[0]As per coding guidelines,
usize/u64narrowing or arithmetic that can wrap requires checked/try_fromhandling.Also applies to: 208-209
🤖 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 164 - 185, The code uses `2 * l_pac` multiple times throughout the function without checking for overflow. When `l_pac` is larger than `u64::MAX / 2`, the multiplication wraps, causing incorrect bounds checks and potentially mapping RC probes to the wrong forward run. Use `checked_mul` to safely compute `2 * l_pac` once at the beginning of the function, store the result in a variable, and replace all instances of the expression `2 * l_pac` (found in the condition at the start of the function, in the else branch calculating available bases, and at lines 208-209) with references to this computed variable, handling the overflow case appropriately by returning early or returning zero as the safe value when the multiplication would overflow.Source: Coding guidelines
🤖 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`:
- Line 163: The function fill_doubled_chunk is a hot comparator helper that
lacks the #[inline] attribute required by the coding guidelines for
prmi/src/index/**/*.rs files. Add the #[inline] attribute directly before the
function signature of fill_doubled_chunk to enable inline optimization for this
hot path used during the seeding probe.
---
Outside diff comments:
In `@prmi/src/index/spectrum.rs`:
- Around line 164-185: The code uses `2 * l_pac` multiple times throughout the
function without checking for overflow. When `l_pac` is larger than `u64::MAX /
2`, the multiplication wraps, causing incorrect bounds checks and potentially
mapping RC probes to the wrong forward run. Use `checked_mul` to safely compute
`2 * l_pac` once at the beginning of the function, store the result in a
variable, and replace all instances of the expression `2 * l_pac` (found in the
condition at the start of the function, in the else branch calculating available
bases, and at lines 208-209) with references to this computed variable, handling
the overflow case appropriately by returning early or returning zero as the safe
value when the multiplication would overflow.
🪄 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: b523c38d-d239-42df-accc-8b82167b9d3a
📒 Files selected for processing (3)
prmi/examples/wall_gate.rsprmi/src/index/smem.rsprmi/src/index/spectrum.rs
99fcbfa to
ee0b632
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
prmi/src/index/smem.rs (1)
72-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn
InvalidInputfor malformed packed PAC arguments.
pac,num_bases, andl_pacvalidation rejects caller/user-like data, so surfacingError::Internalmisclassifies bad inputs as internal defects; switch these validation failures toError::InvalidInput { detail: ... }. Based on learnings, “returnError::InvalidInput { detail: ... }for failures caused by user-provided or ‘user-like’ input” and “ReserveError::Internalexclusively for genuine internal invariants/contract violations.”Proposed fix
- return Err(crate::error::Error::Internal { + return Err(crate::error::Error::InvalidInput { detail: format!( "{ctx}: packed num_bases={num_bases} does not match the index l_pac={l_pac}" ), }); @@ - usize::try_from(num_bases.div_ceil(4)).map_err(|_| crate::error::Error::Internal { + usize::try_from(num_bases.div_ceil(4)).map_err(|_| crate::error::Error::InvalidInput { detail: format!("{ctx}: num_bases={num_bases} is too large for this platform"), })?; @@ - return Err(crate::error::Error::Internal { + return Err(crate::error::Error::InvalidInput { detail: format!( "{ctx}: pac.len()={} is smaller than the required packed length {required} \🤖 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/smem.rs` around lines 72 - 85, In the validate_packed_pac function, replace all instances of Error::Internal with Error::InvalidInput for the three validation checks: when num_bases does not match l_pac, when num_bases is too large for the platform (in the div_ceil calculation), and when pac.len() is insufficient. These validation failures represent user-provided or user-like input errors, not internal defects, so they should be classified as InvalidInput rather than Internal to correctly surface the nature of the error to the caller.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.
Outside diff comments:
In `@prmi/src/index/smem.rs`:
- Around line 72-85: In the validate_packed_pac function, replace all instances
of Error::Internal with Error::InvalidInput for the three validation checks:
when num_bases does not match l_pac, when num_bases is too large for the
platform (in the div_ceil calculation), and when pac.len() is insufficient.
These validation failures represent user-provided or user-like input errors, not
internal defects, so they should be classified as InvalidInput rather than
Internal to correctly surface the nature of the error to the caller.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d94950d9-dd8a-49a3-ab87-7145ab207550
📒 Files selected for processing (3)
prmi/examples/wall_gate.rsprmi/src/index/smem.rsprmi/src/index/spectrum.rs
ee0b632 to
af62236
Compare
|
@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 3938-3948: The comment for the test block starting at the
`compare_query_vs_suffix_2x` test contains an incorrect coverage claim about the
unaligned word-loop path. The comment incorrectly states that offsets 1/2/3 can
reach the `i + 32 <= n` condition after the scalar lead-in in
`unpack_packed_forward`, but this is not possible because the test uses
`fill_doubled_chunk` with `n <= CHUNK_BASES`. Revise the comment to accurately
describe what this test actually covers: the public compare and RC coverage
through `compare_query_vs_suffix_2x`. Remove the stale claim about the unaligned
word-loop path and instead reference the
`unpack_packed_forward_unaligned_word_step_equals_scalar` test as the direct
coverage for the unaligned lead-in to u64 word-step path.
🪄 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: 93df5cb5-d66f-4356-ba9d-51a0c4264885
📒 Files selected for processing (3)
prmi/examples/wall_gate.rsprmi/src/index/smem.rsprmi/src/index/spectrum.rs
The native FMI compare reconstructs the packed (`bntpac`) reference one base at a time: `fill_doubled_chunk` called `pac_base_at` per base, paying a shift/mask plus a bounds check for every 2-bit field it unpacked into the compare buffer. On the compute-bound seeding path this per-probe base-by- base decode is a meaningful slice of the wall. Decode a `u64` worth (8 bytes = 32 bases) per step instead. A fixed 256- entry `UNPACK_LUT` (1 KiB, reference-size-independent) expands a packed byte to its four bases as a little-endian `u32`; `unpack_packed_forward` walks the run with a ≤3-base scalar lead-in to a byte boundary, an aligned word-step middle, a 4-base step, and a ≤3-base scalar tail. The forward region unpacks in place; the reverse-complement region unpacks the mirrored forward run and emits it reversed and XORed by 3. This is purely a faster decode of the same packed reference — no materialized key array, no added resident memory — so it is memory-neutral, unlike a stored-key approach. `Unpacked` is unchanged (it already bulk-copies), and the public API and the LCP / lexicographic / match-length semantics are preserved. No `unsafe`: the word loads use safe `u64::from_le_bytes` over in-range slices. Byte-identical to the per-base path: the existing `vectorized_equals_scalar` (2000 cases, both encodings, `sa_pos` spanning forward/RC/sentinel, queries crossing the 32-base chunk) and every `*_equals_oracle` proptest pass unchanged, and a new `packed_word_decode_all_offsets_equals_scalar` walks every sub-byte start offset across forward/RC/sentinel at query lengths 31/32/33 to pin the unpacker's lead-in, word-step, tail, and RC-reverse paths. End-to-end, the collect_gate SMEM dump matches main exactly (0 differences across a 24-point k/split_len/split_width/max_mem_intv grid, 2,271,848 SMEM rows). A `wall_gate` example measures the seeding wall (single-thread, warm, load-excluded): on a 1 Mbp reference the change is a reproducible ~2.6% reduction in ns/read (paired A/B, min/median/mean all lower) with identical SMEM output.
af62236 to
9d54e87
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
The native FMI compare reconstructed the packed (
bntpac) reference one base at a time:fill_doubled_chunkcalledpac_base_atper base, paying a shift/mask plus a bounds check for every 2-bit field it unpacked into the compare buffer. On the compute-bound seeding path this per-probe base-by-base decode is a meaningful slice of the wall.This decodes a
u64worth (8 bytes = 32 bases) per step instead:UNPACK_LUT(1 KiB, reference-size-independent) expands a packed byte to its four bases as a little-endianu32.unpack_packed_forwardwalks a run with a ≤3-base scalar lead-in to a byte boundary, an aligned word-step middle, a 4-base step, and a ≤3-base scalar tail.This is purely a faster decode of the same packed reference — no materialized key array, no added resident memory, so it is memory-neutral (unlike a stored-key / bwa-meme MODE-3 approach).
Unpackedis unchanged (it already bulk-copies); the public API and the LCP / lexicographic-direction / match-length semantics are preserved. Nounsafe— the word loads use safeu64::from_le_bytesover in-range slices;forbid(unsafe_code)is intact.Byte-identical to the per-base path. Verified by:
vectorized_equals_scalarproptest (2000 cases, both encodings,sa_posspanning forward/RC/sentinel, queries crossing the 32-base chunk) and every*_equals_oracleproptest, unchanged.packed_word_decode_all_offsets_equals_scalartest walking every sub-byte start offset across forward/RC/sentinel at query lengths 31/32/33, pinning the unpacker's lead-in, word-step, tail, and RC-reverse paths.collect_gateSMEM dump vsmain: 0 differences across a 24-pointk/split_len/split_width/max_mem_intvgrid (2,271,848 SMEM rows).cargo clippy --all-features --all-targets -D warningsandcargo +nightly fmt --checkclean; fullcargo testworkspace suite passes.Performance: a
wall_gateexample measures the seeding wall (single-thread, warm, load-excluded). On a 1 Mbp reference, paired interleaved A/B (16 trials each):mainA reproducible ~2.6% seeding-wall reduction (paired per-trial mean) with identical SMEM output.
Summary by CodeRabbit
num_basesmatches the index’s expectedl_pac, rejecting truncated or mismatched data earlier across spectrum/search entry points.