feat(prmi-sys): prmi_mem_search_lean — guards-removed FFI twin for per-call overhead - #24
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds ChangesFFI Lean Variant for Overhead Measurement
Sequence Diagram(s)(see hidden artifact for a sequence diagram of the lean FFI execution path) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 |
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: 2
🧹 Nitpick comments (1)
prmi-sys/tests/spectrum_ffi.rs (1)
1240-1242: ⚡ Quick winEnsure ISA-launch parity is always exercised.
Line 1240 can skip the
est_hint > 0branch entirely (e.g.,ss == 0 && occ == 1), so this test may pass without validating the hinted path.Proposed test hardening
- if ss > 0 && occ > 0 { - both(&query, ss, PRMI_MEM_WANT_INTERVAL); // est_hint = in-interval SA index - } + let hinted = if occ == 0 { + None + } else if ss > 0 { + Some(ss) + } else if occ > 1 { + Some(ss + 1) // still in-interval, and > 0 sentinel + } else { + None + }; + if let Some(hint) = hinted { + both(&query, hint, PRMI_MEM_WANT_INTERVAL); + } else { + panic!("test query produced no representable est_hint > 0; choose another query"); + }🤖 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/tests/spectrum_ffi.rs` around lines 1240 - 1242, The current conditional (if ss > 0 && occ > 0) can skip exercising the est_hint > 0 branch; when occ > 0 but ss == 0 the hinted path is never tested. Modify the test to always exercise the hinted/ISA-launch path: keep the existing call to both(&query, ss, PRMI_MEM_WANT_INTERVAL) when ss > 0, and add a fallback case when occ > 0 and ss == 0 that invokes both(&query, 1, PRMI_MEM_WANT_INTERVAL) (or otherwise supply a positive est_hint) so the est_hint > 0 branch inside both/est_hint logic is exercised; reference the variables ss, occ and the both(...) call to locate where to add the fallback.
🤖 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/examples/ffi_overhead.rs`:
- Around line 92-107: The benchmark loop currently uses debug_assert_eq!(rc, 0)
after calling prmi_mem_search, which is compiled out in --release and can let
non-zero rc silently corrupt results; replace the debug-only assertion with a
release-visible check (e.g., use assert_eq!(rc, 0) or an explicit if rc != 0 {
panic!(...); }) wherever debug_assert_eq! is used (notably the rc variable after
prmi_mem_search in the loop and the other occurrence around lines 116–131) so
the return code is validated at runtime in release builds and includes a clear
error message identifying prmi_mem_search and rc.
- Around line 27-29: Replace the machine-specific hardcoded fallback for
PRMI_HANDOFF: remove the local absolute path stored in bundle and instead derive
a portable default (e.g. a project-relative reports directory using
env!("CARGO_MANIFEST_DIR") or current working directory) or require PRMI_HANDOFF
to be set and return an error if not; update the hoff variable assignment (the
std::env::var("PRMI_HANDOFF").unwrap_or_else(...) call) and pac_path
construction (format!("{hoff}/chr22_A.fa.pac") ) accordingly so the code no
longer uses the absolute "/Users/nhomer/…" path.
---
Nitpick comments:
In `@prmi-sys/tests/spectrum_ffi.rs`:
- Around line 1240-1242: The current conditional (if ss > 0 && occ > 0) can skip
exercising the est_hint > 0 branch; when occ > 0 but ss == 0 the hinted path is
never tested. Modify the test to always exercise the hinted/ISA-launch path:
keep the existing call to both(&query, ss, PRMI_MEM_WANT_INTERVAL) when ss > 0,
and add a fallback case when occ > 0 and ss == 0 that invokes both(&query, 1,
PRMI_MEM_WANT_INTERVAL) (or otherwise supply a positive est_hint) so the
est_hint > 0 branch inside both/est_hint logic is exercised; reference the
variables ss, occ and the both(...) call to locate where to add the fallback.
🪄 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: ddfde074-6ace-49d7-9b2c-7123895fabd6
📒 Files selected for processing (3)
prmi-sys/examples/ffi_overhead.rsprmi-sys/src/lib.rsprmi-sys/tests/spectrum_ffi.rs
…r-call overhead Adds prmi_mem_search_lean, a diagnostic twin of prmi_mem_search that drops the per-call FFI WRAPPER overhead (no clear_last_error/set_last_error, no catch_unwind) while keeping the correctness guards (null/range checks + packed_pac_bytes narrowing). Outputs and return codes are byte-identical to prmi_mem_search; intended for measurement or panic=abort builds (where catch_unwind is already a no-op and this matches the production path). Adds examples/ffi_overhead.rs to A/B the FFI boundary cost vs the direct Rust idx.mem_search, and mem_search_lean_matches_mem_search asserting byte-identical (rc, match_len, sa_start, occ) across model/ISA launch and both flag modes. First PR of the v0.3 perf series, authored fresh against the merged v0.2 foundation (the perf branch diverged from the foundation internals, so it is re-derived rather than cherry-picked).
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.
First PR of the v0.3 perf series (carved from the consumer-collaboration branch onto the merged v0.2 foundation).
What
Adds
prmi_mem_search_lean, a diagnostic twin ofprmi_mem_searchwith the per-call FFI wrapper overhead removed — noclear_last_error/set_last_error, nocatch_unwind— to A/B the fixed cost the consumer pays on every seeding call. The correctness guards (null/range checks,packed_pac_bytesnarrowing) are kept, so outputs and return codes are byte-identical toprmi_mem_search. Intended for measurement, or forpanic=abortbuilds wherecatch_unwindis already a no-op and this matches the production path.Also adds
examples/ffi_overhead.rsto measure the FFI boundary cost vs the direct Rustidx.mem_search.Tests
mem_search_lean_matches_mem_searchasserts byte-identical(rc, match_len, sa_start, occ)vsprmi_mem_searchacross model-launch (est_hint == 0) and ISA-launch (est_hint > 0), with and withoutWANT_INTERVAL.Notes
Authored fresh against
main(the merged v0.2 O(log) foundation), not cherry-picked — the perf branch diverged from the foundation's internals, so the series is re-derived rather than replayed. The original perf-branch version computed the pac length with a rawdiv_ceil(4) as usize; this uses the foundation'spacked_pac_bytes()narrowing guard for consistency and to avoid u64→usize truncation.Summary by CodeRabbit
New Features
Tests