perf(spectrum): cross-query lockstep primitives (forward-spectrum + maximal-match) - #50
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 (7)
WalkthroughAdds probe-driven stepper state machines ( ChangesLockstep batch execution for forward-spectrum and mem-search
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 |
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Author
|
@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: 8
🤖 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/batch_wall.rs`:
- Around line 67-70: The FASTQ parser in the while loop silently ignores errors
and missing lines when reading the third and fourth lines using let _ =
lines.next() calls. Replace these silent ignores with proper error handling:
change the third line (which should be "+") to use a proper else break or error
check that validates the line is Ok and contains the plus character, and change
the fourth line (quality) similarly to use an else break that handles I/O errors
and truncation. This ensures that parsing failures fail fast rather than
silently corrupting the benchmark input data.
In `@prmi/examples/make_fixture.rs`:
- Line 81: The variable p assignment uses rng2.below() which creates an
exclusive upper bound, preventing the last valid read window position from ever
being sampled. Change the bound calculation for p to be inclusive so that p can
equal nbases - readlen, which represents a valid start position for a read of
length readlen that extends to the end of the sequence. Either use an inclusive
range method instead of below() or adjust the calculation to below((nbases -
readlen + 1) as u64) to make the upper bound inclusive.
- Around line 85-87: The substitution logic in the conditional block where
subs_pct > 0 is true does not ensure the replacement base is actually different
from the original base. When rng2.base() is called in the statement `b =
rng2.base();`, it can return any of the 4 possible bases including the original
value, which means no actual substitution occurs. Modify this line to guarantee
the new base is different from the original base by either repeatedly calling
rng2.base() until a different value is obtained, or by selecting from only the 3
alternative bases rather than all 4 bases.
In `@prmi/examples/mem_search_batch_wall.rs`:
- Around line 63-66: The FASTQ record parsing loop starting with the header read
is silently discarding errors by using let _ = lines.next() for sequence quality
and separator lines, making read failures invisible and measurements
non-reproducible. Replace the silent discards with explicit error checking for
each of the four required FASTQ lines (header, sequence, separator, quality),
ensuring that each lines.next() call is properly validated and errors are
propagated rather than ignored. Validate that all four records are present and
handle missing or malformed records with appropriate error handling.
In `@prmi/examples/predicted_sort_wall.rs`:
- Around line 65-68: The FASTQ parsing loop in the predicted_sort_wall.rs
example silently discards errors and EOF conditions from the third and fourth
lines of FASTQ records. Replace the lines containing let _ = lines.next() (which
ignore both errors and truncation) with explicit error handling that validates
all four required lines of each FASTQ record are successfully read. Use proper
error propagation (such as match statements or if-let patterns with else
branches) on all lines.next() calls to ensure the program fails fast when
encountering malformed or truncated records, rather than silently continuing
with incomplete data.
- Around line 142-166: The warm phase for benchmarking has two issues: the
lockstep function uses a different sink reduction algebra (matching_len XOR occ)
compared to serial and sorted functions, and there is no verification that all
three approaches produce equivalent results before timing. Unify the sink
reduction logic across all three closures (serial, sorted, and lockstep) to use
the same algebra, then capture the results from each black_box call in the warm
phase and add an assertion that verifies all three results are equal before
proceeding to the actual benchmark measurements.
In `@prmi/src/index/spectrum.rs`:
- Around line 762-831: The hot stepper helper functions `begin_m`, `next_probe`,
and `advance` that are called frequently in the SA probe loop are missing
`#[inline]` attributes, causing unnecessary call overhead from the state-machine
dispatch. Add the `#[inline]` attribute to these functions to allow the compiler
to inline them and eliminate avoidable call overhead on the probe path. Also
apply the same `#[inline]` attribute to any other hot helper or comparator
functions in the lines 963-1042 range that are called in tight loops.
- Around line 5647-5690: The test mem_search_stepper_edge_cases claims to cover
insertion-point boundaries ip==0 and ip==sa_num but only verifies byte-identity
across three search methods without confirming the fixtures actually produce
those boundary conditions. Add scalar assertions before the check() calls for
cases (c) and (d) that directly verify the insertion point returned by
mem_search equals 0 for the all-0 query on idx_mix and equals sa_num for the
all-3 query on idx_mix respectively, ensuring the edge cases are genuinely being
tested rather than silently becoming generic byte-identity checks.
🪄 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: 079848c5-5f7e-4944-8414-f925b1d97567
📒 Files selected for processing (7)
prmi-sys/src/lib.rsprmi-sys/tests/spectrum_ffi.rsprmi/examples/batch_wall.rsprmi/examples/make_fixture.rsprmi/examples/mem_search_batch_wall.rsprmi/examples/predicted_sort_wall.rsprmi/src/index/spectrum.rs
nh13
force-pushed
the
feat/spectrum-lockstep
branch
3 times, most recently
from
June 20, 2026 18:16
f03ffb1 to
5ba3c07
Compare
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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-sys/src/lib.rs`:
- Around line 867-873: The code calls `std::slice::from_raw_parts_mut` on a
potentially NULL pointer when `steps_arena` is NULL with zero length, which
violates Rust safety guarantees. Before performing pointer arithmetic with
`steps_arena.add()` or calling `from_raw_parts_mut()`, add a check: if
`steps.len()` equals 0, branch to use an empty mutable slice reference instead
of accessing the raw pointer. Apply this defensive check at all affected
locations where `from_raw_parts_mut(steps_arena.add(...))` is called to handle
the canonical empty-slice case (NULL + 0) that C callers may pass without
attempting pointer arithmetic on NULL.
- Around line 804-820: The documentation for this new C ABI entrypoint (the
LOCKSTEP strategy function) currently references error codes indirectly by
stating "Same arguments, arena contract, and error codes as
prmi_forward_spectrum_batch", but this does not meet the coding guidelines
requirement for explicit, exhaustive per-entrypoint return-code contracts. Add a
dedicated "Return codes" or "Error codes" section in the documentation that
explicitly lists all possible return values (0, -1, -2, -3, and -4) with their
exact meanings copied from the forward_spectrum_batch_impl implementation,
ensuring the contract is completely self-contained and not dependent on external
references.
In `@prmi/examples/batch_wall.rs`:
- Around line 65-110: The cap check for max_reads currently happens after a
complete record has been parsed in the while loop, causing one extra record to
be consumed even when max_reads is zero. Move the queries.len() >= max_reads
check to the beginning of the loop (before calling lines.next() to read the
header line) so that the limit is enforced before attempting to read the next
record rather than after parsing it.
In `@prmi/examples/make_fixture.rs`:
- Around line 54-55: The FIX_SUBS_PCT environment variable read via env_u64 on
line 54 is not validated to ensure it represents a valid percentage. Add
validation after retrieving the subs_pct value to ensure it falls within the
valid percentage range of 0 to 100, and handle the invalid case appropriately by
either panicking with a descriptive error message or using a sensible default
value. This validation should prevent values above 100 from being accepted,
which currently causes incorrect substitution behavior.
- Around line 50-52: The three env_u64 calls for FIX_BASES, FIX_READS, and
FIX_READLEN are casting u64 values directly to usize without validation, which
can silently truncate oversized environment values. Instead of using direct as
usize casts for nbases, nreads, and readlen, validate that each u64 value fits
within the usize range by using try_into() or checking against usize::MAX, and
panic with a descriptive error message if any value exceeds the maximum allowed
size for the current platform.
In `@prmi/examples/mem_search_batch_wall.rs`:
- Around line 65-106: The PRMI_MAX_READS cap check is performed after a FASTQ
record is parsed and added to queries, which allows one extra record to be
processed when the limit is zero. Move the `if queries.len() >= max_reads {
break; }` check from the end of the while loop to the beginning, right after the
header is read and before processing the sequence line, so the capacity limit is
enforced before any additional records are parsed.
In `@prmi/examples/predicted_sort_wall.rs`:
- Around line 67-108: The max_reads cap enforcement is currently checked after
parsing and pushing a FASTQ record to the queries vector, which means it allows
one extra record to be processed before respecting the limit. Move the cap check
(the if queries.len() >= max_reads break condition) to the beginning of the
while loop, before reading any FASTQ lines, so that the limit is enforced
strictly before processing each record rather than after. This ensures that
strict caps like 0 are honored exactly without processing any extra records
beyond the specified limit.
In `@prmi/src/index/spectrum.rs`:
- Line 2846: Replace the unsafe cast and saturating_sub operation in the `lcap`
assignment with proper bounds checking to prevent silent wraparound on oversized
input. Instead of casting `lmax` directly to `u32` and using
`saturating_sub(1)`, first use `checked_sub(1)` on `lmax` to safely subtract 1,
then use `u32::try_from()` on the result to perform the type conversion with
proper error handling. Add early return logic to fail closed when either the
subtraction or the conversion fails, ensuring that caller-supplied values larger
than `u32::MAX` result in an error rather than a silently truncated value.
🪄 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: 8f5a0d6d-efa1-4079-8672-983c770a3ece
📒 Files selected for processing (7)
prmi-sys/src/lib.rsprmi-sys/tests/spectrum_ffi.rsprmi/examples/batch_wall.rsprmi/examples/make_fixture.rsprmi/examples/mem_search_batch_wall.rsprmi/examples/predicted_sort_wall.rsprmi/src/index/spectrum.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
🤖 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-sys/src/lib.rs`:
- Around line 867-873: The code calls `std::slice::from_raw_parts_mut` on a
potentially NULL pointer when `steps_arena` is NULL with zero length, which
violates Rust safety guarantees. Before performing pointer arithmetic with
`steps_arena.add()` or calling `from_raw_parts_mut()`, add a check: if
`steps.len()` equals 0, branch to use an empty mutable slice reference instead
of accessing the raw pointer. Apply this defensive check at all affected
locations where `from_raw_parts_mut(steps_arena.add(...))` is called to handle
the canonical empty-slice case (NULL + 0) that C callers may pass without
attempting pointer arithmetic on NULL.
- Around line 804-820: The documentation for this new C ABI entrypoint (the
LOCKSTEP strategy function) currently references error codes indirectly by
stating "Same arguments, arena contract, and error codes as
prmi_forward_spectrum_batch", but this does not meet the coding guidelines
requirement for explicit, exhaustive per-entrypoint return-code contracts. Add a
dedicated "Return codes" or "Error codes" section in the documentation that
explicitly lists all possible return values (0, -1, -2, -3, and -4) with their
exact meanings copied from the forward_spectrum_batch_impl implementation,
ensuring the contract is completely self-contained and not dependent on external
references.
In `@prmi/examples/batch_wall.rs`:
- Around line 65-110: The cap check for max_reads currently happens after a
complete record has been parsed in the while loop, causing one extra record to
be consumed even when max_reads is zero. Move the queries.len() >= max_reads
check to the beginning of the loop (before calling lines.next() to read the
header line) so that the limit is enforced before attempting to read the next
record rather than after parsing it.
In `@prmi/examples/make_fixture.rs`:
- Around line 54-55: The FIX_SUBS_PCT environment variable read via env_u64 on
line 54 is not validated to ensure it represents a valid percentage. Add
validation after retrieving the subs_pct value to ensure it falls within the
valid percentage range of 0 to 100, and handle the invalid case appropriately by
either panicking with a descriptive error message or using a sensible default
value. This validation should prevent values above 100 from being accepted,
which currently causes incorrect substitution behavior.
- Around line 50-52: The three env_u64 calls for FIX_BASES, FIX_READS, and
FIX_READLEN are casting u64 values directly to usize without validation, which
can silently truncate oversized environment values. Instead of using direct as
usize casts for nbases, nreads, and readlen, validate that each u64 value fits
within the usize range by using try_into() or checking against usize::MAX, and
panic with a descriptive error message if any value exceeds the maximum allowed
size for the current platform.
In `@prmi/examples/mem_search_batch_wall.rs`:
- Around line 65-106: The PRMI_MAX_READS cap check is performed after a FASTQ
record is parsed and added to queries, which allows one extra record to be
processed when the limit is zero. Move the `if queries.len() >= max_reads {
break; }` check from the end of the while loop to the beginning, right after the
header is read and before processing the sequence line, so the capacity limit is
enforced before any additional records are parsed.
In `@prmi/examples/predicted_sort_wall.rs`:
- Around line 67-108: The max_reads cap enforcement is currently checked after
parsing and pushing a FASTQ record to the queries vector, which means it allows
one extra record to be processed before respecting the limit. Move the cap check
(the if queries.len() >= max_reads break condition) to the beginning of the
while loop, before reading any FASTQ lines, so that the limit is enforced
strictly before processing each record rather than after. This ensures that
strict caps like 0 are honored exactly without processing any extra records
beyond the specified limit.
In `@prmi/src/index/spectrum.rs`:
- Line 2846: Replace the unsafe cast and saturating_sub operation in the `lcap`
assignment with proper bounds checking to prevent silent wraparound on oversized
input. Instead of casting `lmax` directly to `u32` and using
`saturating_sub(1)`, first use `checked_sub(1)` on `lmax` to safely subtract 1,
then use `u32::try_from()` on the result to perform the type conversion with
proper error handling. Add early return logic to fail closed when either the
subtraction or the conversion fails, ensuring that caller-supplied values larger
than `u32::MAX` result in an error rather than a silently truncated value.
🪄 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: 8f5a0d6d-efa1-4079-8672-983c770a3ece
📒 Files selected for processing (7)
prmi-sys/src/lib.rsprmi-sys/tests/spectrum_ffi.rsprmi/examples/batch_wall.rsprmi/examples/make_fixture.rsprmi/examples/mem_search_batch_wall.rsprmi/examples/predicted_sort_wall.rsprmi/src/index/spectrum.rs
🛑 Comments failed to post (8)
prmi-sys/src/lib.rs (2)
804-820:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an explicit return-code table for this new C ABI entrypoint.
Same ... error codesis not an exhaustive per-entrypoint contract; list0,-1,-2,-3, and-4with the exact meanings fromforward_spectrum_batch_impl.Suggested doc patch
/// # Safety /// All pointers valid for their declared sizes; arenas at least as large as /// declared; `out_nsteps` has `ntasks` u64 slots. +/// +/// # Returns +/// * `0` on success. +/// * `-1` for a null required pointer. +/// * `-2` for size/range validation failures (`pac_num_bases`, `ntasks`, +/// task query windows, task step windows, or platform narrowing). +/// * `-3` if Rust panics internally. +/// * `-4` if any task's `max_steps` is too small; `out_nsteps[i]` contains +/// each task's produced step count.As per coding guidelines,
prmi-sys/src/**/*.rs: “For everyextern "C"entrypoint require ... a documented, exhaustive return-code list.”📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./// Run `ntasks` forward spectra with the LOCKSTEP strategy: all tasks are driven /// in parallel, batching their cold SA probe loads each round so the memory /// latency overlaps (memory-level parallelism). Byte-identical output to /// `prmi_forward_spectrum_batch` — same breakpoints, same order — differing only /// in execution strategy. Faster on high-DRAM-latency microarchitectures (server /// x86 / Graviton); can be SLOWER on low-latency ones (e.g. Apple Silicon, which /// already hides the latency). A/B both entry points on the target hardware and /// pick the winner. Same arguments, arena contract, and error codes as /// `prmi_forward_spectrum_batch`. /// /// Drives the table-free forward search (it does not consult a loaded `.kmt`); /// the MLP win is on the cold deep-probe path, which is also the byte-identity- /// critical production path. Output still equals the kmt-accelerated serial entry. /// /// # Safety /// All pointers valid for their declared sizes; arenas at least as large as /// declared; `out_nsteps` has `ntasks` u64 slots. /// /// # Returns /// * `0` on success. /// * `-1` for a null required pointer. /// * `-2` for size/range validation failures (`pac_num_bases`, `ntasks`, /// task query windows, task step windows, or platform narrowing). /// * `-3` if Rust panics internally. /// * `-4` if any task's `max_steps` is too small; `out_nsteps[i]` contains /// each task's produced step count.🤖 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-sys/src/lib.rs` around lines 804 - 820, The documentation for this new C ABI entrypoint (the LOCKSTEP strategy function) currently references error codes indirectly by stating "Same arguments, arena contract, and error codes as prmi_forward_spectrum_batch", but this does not meet the coding guidelines requirement for explicit, exhaustive per-entrypoint return-code contracts. Add a dedicated "Return codes" or "Error codes" section in the documentation that explicitly lists all possible return values (0, -1, -2, -3, and -4) with their exact meanings copied from the forward_spectrum_batch_impl implementation, ensuring the contract is completely self-contained and not dependent on external references.Source: Coding guidelines
867-873:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
NULL + 0slice inputs before callingadd/from_raw_parts.For empty task queries or zero-capacity step arenas, C callers may pass canonical empty slices as
NULL + 0. Relax validation only for zero declared lengths, then branch to&[]/&mut [](or return early for emptysteps) before pointer arithmetic.As per coding guidelines,
prmi-sys/src/**/*.rsC ABI entrypoints must handle the canonical empty-slice caseNULL + len 0withoutslice::from_raw_parts(NULL, 0).Also applies to: 908-929, 988-996, 1011-1026
🤖 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-sys/src/lib.rs` around lines 867 - 873, The code calls `std::slice::from_raw_parts_mut` on a potentially NULL pointer when `steps_arena` is NULL with zero length, which violates Rust safety guarantees. Before performing pointer arithmetic with `steps_arena.add()` or calling `from_raw_parts_mut()`, add a check: if `steps.len()` equals 0, branch to use an empty mutable slice reference instead of accessing the raw pointer. Apply this defensive check at all affected locations where `from_raw_parts_mut(steps_arena.add(...))` is called to handle the canonical empty-slice case (NULL + 0) that C callers may pass without attempting pointer arithmetic on NULL.Source: Coding guidelines
prmi/examples/batch_wall.rs (1)
65-110:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHonor
PRMI_MAX_READSbefore consuming the next record.The current cap check happens after parsing a record; with
PRMI_MAX_READS=0, input is still consumed and may include one query.Proposed fix
- while let Some(_hdr) = lines.next().transpose().expect("read FASTQ header line") { + while queries.len() < max_reads { + let Some(_hdr) = lines.next().transpose().expect("read FASTQ header line") else { + break; + }; let seq = lines .next() .transpose() @@ - if queries.len() >= max_reads { - break; - } }🤖 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/examples/batch_wall.rs` around lines 65 - 110, The cap check for max_reads currently happens after a complete record has been parsed in the while loop, causing one extra record to be consumed even when max_reads is zero. Move the queries.len() >= max_reads check to the beginning of the loop (before calling lines.next() to read the header line) so that the limit is enforced before attempting to read the next record rather than after parsing it.prmi/examples/make_fixture.rs (2)
50-52:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against silent truncation in env size parsing.
These
as usizecasts can silently truncate oversizedu64env values and produce wrong fixture sizes instead of failing fast.Proposed fix
- let nbases = env_u64("FIX_BASES", 64 * 1024 * 1024) as usize; - let nreads = env_u64("FIX_READS", 20_000) as usize; - let readlen = env_u64("FIX_READLEN", 150) as usize; + let nbases = usize::try_from(env_u64("FIX_BASES", 64 * 1024 * 1024)) + .expect("FIX_BASES exceeds usize"); + let nreads = usize::try_from(env_u64("FIX_READS", 20_000)) + .expect("FIX_READS exceeds usize"); + let readlen = usize::try_from(env_u64("FIX_READLEN", 150)) + .expect("FIX_READLEN exceeds usize");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let nbases = usize::try_from(env_u64("FIX_BASES", 64 * 1024 * 1024)) .expect("FIX_BASES exceeds usize"); let nreads = usize::try_from(env_u64("FIX_READS", 20_000)) .expect("FIX_READS exceeds usize"); let readlen = usize::try_from(env_u64("FIX_READLEN", 150)) .expect("FIX_READLEN exceeds usize");🤖 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/examples/make_fixture.rs` around lines 50 - 52, The three env_u64 calls for FIX_BASES, FIX_READS, and FIX_READLEN are casting u64 values directly to usize without validation, which can silently truncate oversized environment values. Instead of using direct as usize casts for nbases, nreads, and readlen, validate that each u64 value fits within the usize range by using try_into() or checking against usize::MAX, and panic with a descriptive error message if any value exceeds the maximum allowed size for the current platform.
54-55:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
FIX_SUBS_PCTas a real percentage.
FIX_SUBS_PCTis documented as percent, but values above 100 are currently accepted and force near-always substitution, violating the input contract.Proposed fix
let subs_pct = env_u64("FIX_SUBS_PCT", 1); + assert!(subs_pct <= 100, "FIX_SUBS_PCT must be in 0..=100");🤖 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/examples/make_fixture.rs` around lines 54 - 55, The FIX_SUBS_PCT environment variable read via env_u64 on line 54 is not validated to ensure it represents a valid percentage. Add validation after retrieving the subs_pct value to ensure it falls within the valid percentage range of 0 to 100, and handle the invalid case appropriately by either panicking with a descriptive error message or using a sensible default value. This validation should prevent values above 100 from being accepted, which currently causes incorrect substitution behavior.prmi/examples/mem_search_batch_wall.rs (1)
65-106:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winApply
PRMI_MAX_READScap pre-ingest to preserve contract.The loop currently checks the cap after a record is parsed;
PRMI_MAX_READS=0still allows one record path through parsing.Proposed fix
- while let Some(_hdr) = lines.next().transpose().expect("read FASTQ header line") { + while queries.len() < max_reads { + let Some(_hdr) = lines.next().transpose().expect("read FASTQ header line") else { + break; + }; let seq = lines .next() .transpose() @@ - if queries.len() >= max_reads { - break; - } }🤖 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/examples/mem_search_batch_wall.rs` around lines 65 - 106, The PRMI_MAX_READS cap check is performed after a FASTQ record is parsed and added to queries, which allows one extra record to be processed when the limit is zero. Move the `if queries.len() >= max_reads { break; }` check from the end of the while loop to the beginning, right after the header is read and before processing the sequence line, so the capacity limit is enforced before any additional records are parsed.prmi/examples/predicted_sort_wall.rs (1)
67-108:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnforce
PRMI_MAX_READSbefore reading each FASTQ record.Cap enforcement is currently delayed until after parse/push, so strict caps (notably
0) are not honored exactly.Proposed fix
- while let Some(_hdr) = lines.next().transpose().expect("read FASTQ header line") { + while queries.len() < max_reads { + let Some(_hdr) = lines.next().transpose().expect("read FASTQ header line") else { + break; + }; let seq = lines .next() .transpose() @@ - if queries.len() >= max_reads { - break; - } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.while queries.len() < max_reads { let Some(_hdr) = lines.next().transpose().expect("read FASTQ header line") else { break; }; let seq = lines .next() .transpose() .expect("read FASTQ sequence line") .expect("truncated FASTQ: missing sequence line"); let plus = lines .next() .transpose() .expect("read FASTQ separator line") .expect("truncated FASTQ: missing '+' line"); let qual = lines .next() .transpose() .expect("read FASTQ quality line") .expect("truncated FASTQ: missing quality line"); assert!( plus.starts_with('+'), "invalid FASTQ separator line: {plus:?}" ); assert_eq!( qual.len(), seq.len(), "FASTQ quality length != sequence length" ); let mut q = Vec::with_capacity(seq.len()); for b in seq.bytes() { let code = match b { b'A' | b'a' => 0u8, b'C' | b'c' => 1, b'G' | b'g' => 2, b'T' | b't' => 3, _ => break, }; q.push(code); } if !q.is_empty() { queries.push(q); } }🤖 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/examples/predicted_sort_wall.rs` around lines 67 - 108, The max_reads cap enforcement is currently checked after parsing and pushing a FASTQ record to the queries vector, which means it allows one extra record to be processed before respecting the limit. Move the cap check (the if queries.len() >= max_reads break condition) to the beginning of the while loop, before reading any FASTQ lines, so that the limit is enforced strictly before processing each record rather than after. This ensures that strict caps like 0 are honored exactly without processing any extra records beyond the specified limit.prmi/src/index/spectrum.rs (1)
2846-2846:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify the unchecked lmax narrowing is removed. rg -n -C2 '\blmax\s+as\s+u32\b' prmi/src/index/spectrum.rsRepository: fg-labs/prmi
Length of output: 371
Use
try_fromandchecked_subto fail closed on oversizedlmax.Line 2846 casts public
maximal.match_lentou32without bounds checking; a caller-suppliedmatch_len > u32::MAXwraps silently and returns an incorrect truncated value instead of failing closed. Usechecked_sub(1)followed byu32::try_from()and return early on failure.Proposed fix
- let lcap = (lmax as u32).saturating_sub(1); + let lmax_minus_one = match lmax.checked_sub(1) { + Some(v) => v, + None => return zero, + }; + let lcap = match u32::try_from(lmax_minus_one) { + Ok(v) => v, + Err(_) => return zero, + };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let lmax_minus_one = match lmax.checked_sub(1) { Some(v) => v, None => return zero, }; let lcap = match u32::try_from(lmax_minus_one) { Ok(v) => v, Err(_) => return zero, };🤖 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` at line 2846, Replace the unsafe cast and saturating_sub operation in the `lcap` assignment with proper bounds checking to prevent silent wraparound on oversized input. Instead of casting `lmax` directly to `u32` and using `saturating_sub(1)`, first use `checked_sub(1)` on `lmax` to safely subtract 1, then use `u32::try_from()` on the result to perform the type conversion with proper error handling. Add early return logic to fail closed when either the subtraction or the conversion fails, ensuring that caller-supplied values larger than `u32::MAX` result in an error rather than a silently truncated value.Source: Coding guidelines
nh13
force-pushed
the
feat/spectrum-lockstep
branch
from
June 21, 2026 19:43
5ba3c07 to
085d07d
Compare
nh13
force-pushed
the
feat/spectrum-lockstep
branch
2 times, most recently
from
June 21, 2026 20:28
085d07d to
bece0ee
Compare
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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-sys/src/lib.rs`:
- Around line 1024-1055: The lockstep path materializes the full vector of steps
via forward_spectrum_lockstep before write_fwd_task_steps checks max_steps,
allowing unbounded transient allocations when a C caller provides undersized
buffers. Add a check before calling forward_spectrum_lockstep to validate
whether the tasks would exceed max_steps; if they would, either route those
tasks through the bounded serial fill path instead of lockstep, or implement a
cap-aware lockstep sink that counts steps before materializing the full trace to
prevent the unintended transient allocation and enforce the max_steps limit as
intended.
In `@prmi-sys/tests/spectrum_ffi.rs`:
- Around line 2338-2385: The NULL+nonzero length rejection test cases for both
queries_arena and steps_arena are only being tested against the
prmi_forward_spectrum_batch entrypoint. Refactor these two assertions (the one
checking rc_bad and the one checking rc_bad_steps) into a loop or parameterized
test that runs both test scenarios against both prmi_forward_spectrum_batch and
prmi_forward_spectrum_batch_lockstep to ensure consistency between the two C ABI
wrapper implementations and catch any future drift between them.
In `@prmi/examples/make_fixture.rs`:
- Line 65: The assertion on line 65 in make_fixture.rs uses a strictly greater
than comparison (>) which rejects the valid edge case where nbases equals
readlen. Change the comparison operator from > to >= in the assert!() macro to
allow this valid scenario where a single read can span the entire sequence (as
computed on line 92 with rng2.below(1) = 0).
In `@prmi/src/index/spectrum.rs`:
- Around line 5887-5888: Before the check call that tests the no-match fixture
with parameters &idx_abc, &fwd_abc, &[3, 1, 2, 1], add an explicit assertion to
verify that the maximum LCP value is actually 0. This scalar max-LCP guard
ensures that the fixture truly represents a no-match case and prevents the test
from silently skipping the no-match code path if the fixture unexpectedly starts
matching.
- Around line 5793-5825: The test mem_search_lockstep_equals_serial currently
only covers the code path where an index is built without the `.kmt` file, but
line 3588 implements a different fallback path when the loaded `.kmt` is
present. Add a second property test (or extend the existing one) that creates an
index fixture with a loaded `.kmt` file, then verify that both hinted and
unhinted lockstep operations (mem_search_lockstep with seed_hint values and
without) produce byte-identical results to the serial mem_search method, similar
to the assertions already present in the current test with the same
prop_assert_eq patterns.
🪄 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: 306991c6-0419-41a6-a1c1-ae86c1a0cda6
📒 Files selected for processing (7)
prmi-sys/src/lib.rsprmi-sys/tests/spectrum_ffi.rsprmi/examples/batch_wall.rsprmi/examples/make_fixture.rsprmi/examples/mem_search_batch_wall.rsprmi/examples/predicted_sort_wall.rsprmi/src/index/spectrum.rs
nh13
force-pushed
the
feat/spectrum-lockstep
branch
from
June 22, 2026 15:45
bece0ee to
5519b83
Compare
…tch) Drive N independent queries' boundary searches in lockstep so their cold suffix-array reads overlap each round (memory-level parallelism), turning the latency-bound search into a bandwidth-bound one. Two primitives, mirroring the existing FbState/BwdStepper/backward_spectrum_lockstep pattern: - FwdStepper + forward_spectrum_lockstep + prmi_forward_spectrum_batch_lockstep (FFI): re-expresses forward_spectrum_into (the full breakpoint trace) as a resumable probe-driven state machine; output byte-identical to serial forward_spectrum. Exposed via a shared forward_spectrum_batch_impl(lockstep). - MemSearchStepper + mem_search_lockstep: the maximal-match locate (forward_maximal_len_seeded + interval recovery) as a stepper; three FbState searches + two interstitial lcp probes; optional warm-start seed. Falls back to serial mem_search when a kmt is loaded (the stepper models only the no-kmt path), so the pub API is byte-identical for any index. Both drivers track live steppers in a compacting O(active) list (not an O(batch) full scan) so cheap queries on a large batch are not dominated by the per-round sweep over finished steppers. Byte-identity (the arbiters): fwd_lockstep_equals_serial, mem_search_lockstep_ equals_serial (+ via_stepper, warm-start hints, and deterministic edge cases: empty / match_len==0 / ip==0 / ip==sa_num), and the forward batch FFI test (lockstep == serial == single-query oracle). Measured on Graviton c8g (128 Mbp, 3.49 GB SA): maximal-match lockstep +7.6% at batch~64, forward-spectrum +4.5% at batch~4096; both latency-hiding (helps on high-DRAM-latency arches, neutral/negative on Apple Silicon) so the caller dispatches by batch size.
- make_fixture: deterministic synthetic bwa-format .pac + FASTQ generator (no rand/Date), so a large-ref fixture reproduces identically on a bench host. - batch_wall / mem_search_batch_wall: paired-interleaved serial-vs-lockstep ns/query A/B for the forward-spectrum and maximal-match paths; each warm-up asserts the two strategies agree (end-to-end byte-identity smoke check). - predicted_sort_wall: experiment that sorts a query batch by model-predicted SA position; records the (negative) result that with err~2 and a sparse query set the sort gives no SA locality — documents why that lever was not pursued.
nh13
force-pushed
the
feat/spectrum-lockstep
branch
from
June 22, 2026 17:15
5519b83 to
4dcde57
Compare
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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.
Byte-identical memory-level-parallelism (MLP) primitives for the learned-index search: drive N independent queries in lockstep so their cold suffix-array reads overlap each round, turning the latency-bound boundary search into a bandwidth-bound one. Mirrors the existing
FbState/BwdStepper/backward_spectrum_locksteppattern. Independent of #48 (additive new functions; no reseed-path changes).Reading order
FwdStepper+forward_spectrum_lockstep+prmi_forward_spectrum_batch_lockstep(FFI) — re-expresses the fullforward_spectrum_intobreakpoint trace as a resumable probe-driven state machine; output byte-identical to serialforward_spectrum. Exposed via a sharedforward_spectrum_batch_impl(lockstep).MemSearchStepper+mem_search_lockstep— the maximal-match locate (forward_maximal_len_seeded+ interval recovery) as a stepper (threeFbStatesearches + two interstitiallcpprobes; optional warm-start seed). Falls back to serialmem_searchwhen a.kmtis loaded (the stepper models only the no-kmt path), so the public API is byte-identical for any index.make_fixture,batch_wall,mem_search_batch_wall,predicted_sort_wall).Byte-identity (the arbiters)
fwd_lockstep_equals_serial,mem_search_lockstep_equals_serial(+ single-taskvia_stepper, warm-start hints, and deterministic edge cases: empty /match_len==0/ip==0/ip==sa_num), and the forward batch FFI test (lockstep == serial == single-query oracle). Fullcargo test --workspacegreen; clippy + nightly-fmt clean.Measurements (Graviton c8g, 128 Mbp synthetic ref, 3.49 GB SA)
Both are latency-hiding: they help on high-DRAM-latency arches (server x86 / Graviton) and are neutral/negative on low-latency arches (Apple Silicon, which already hides the latency). Callers should A/B on the target and dispatch by batch size; the primitives never change output.
Scope note
mem_search_lockstepis the foundational Stage-1 primitive of a (separately-tracked) effort to bring MLP to the full per-readcollect_smemsdriver; this PR ships the standalone byte-identical primitives + the forward-spectrum FFI entry point.Summary by CodeRabbit
NULL + 0, rejects invalidNULL + nonzero).