perf(spectrum): combine SA position+key into one entry() read in the compare loop - #37
Conversation
|
Warning Review limit reached
More reviews will be available in 25 minutes and 44 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR consolidates suffix array entry retrieval by adding a combined zero-copy read API, exposing it through the index layer, and refactoring all major spectrum and boundary-search code paths to use the new API instead of separate position and key lookups. ChangesSA Entry Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. 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.
🧹 Nitpick comments (1)
prmi/tests/sidecar_sa_file.rs (1)
172-184: ⚡ Quick winUse
key_at(i)in the mode-1 equivalence assertion to keep the contract exact.At Line 183, asserting against
Noneis correct today, but the test’s stated purpose is API equivalence. Comparing withr1.key_at(i)keeps that invariant explicit and symmetric with mode 2.Suggested test tweak
- for i in 0..r1.num_entries() { - assert_eq!(r1.entry(i), (r1.position(i), None), "entry({i}) mode 1"); - } + for i in 0..r1.num_entries() { + assert_eq!( + r1.entry(i), + (r1.position(i), r1.key_at(i)), + "entry({i}) != (position, key_at) in mode 1" + ); + }🤖 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/tests/sidecar_sa_file.rs` around lines 172 - 184, The test asserts mode-1 entries equal (r1.position(i), None) which is correct today but should use the API symmetry: replace the literal None with r1.key_at(i) so the assertion reads assert_eq!(r1.entry(i), (r1.position(i), r1.key_at(i)), "entry({i}) mode 1"); update the loop in the test that iterates over r1.num_entries() and uses r1.entry(i) and r1.position(i) to use r1.key_at(i) instead of None to keep the contract exact.
🤖 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/tests/sidecar_sa_file.rs`:
- Around line 172-184: The test asserts mode-1 entries equal (r1.position(i),
None) which is correct today but should use the API symmetry: replace the
literal None with r1.key_at(i) so the assertion reads assert_eq!(r1.entry(i),
(r1.position(i), r1.key_at(i)), "entry({i}) mode 1"); update the loop in the
test that iterates over r1.num_entries() and uses r1.entry(i) and r1.position(i)
to use r1.key_at(i) instead of None to keep the contract exact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b346c30f-8d98-4e4a-bf1d-133d950506be
📒 Files selected for processing (4)
prmi/src/index/mod.rsprmi/src/index/spectrum.rsprmi/src/sidecar/sa_file.rsprmi/tests/sidecar_sa_file.rs
…compare loop Every keyed-compare probe read the SA entry twice — sa_position_for(i) then key_at(i) — each recomputing the entry offset (header + i*bytes_per_entry) and re-running the bounds assert, on the same 13-byte entry (one cache line). Add SaFileReader::entry(i) -> (position, Option<key>) and LearnedIndex::sa_entry, and use it at every per-probe site (the forward/backward boundary loops, push_unique_suffix_tail, the hint paths, lcp_at, ref_less/shares_prefix, and the lockstep stepper). One bounds check and one offset computation per probe instead of two. The probe loop is memory-latency-bound (the cold entry read dominates and prefetch overlaps it), so this trims instruction count without moving wall time much — but it is the hottest loop. Byte-identical: entry(i) returns exactly (position(i), key_at(i)) (new entry_equals_position_and_key_at round-trip test, modes 1+2), the explicit bump_probe() per site is preserved (probe count unchanged), and the existing mem_search/spectrum/collect byte-identity oracle proptests pass unchanged. On-disk format unchanged.
1 participant
v0.3 perf series — the last net-new alloc/CPU trim (
17d506b). Closes out PR-C (C-alloc).What
Every keyed-compare probe read the SA entry twice —
sa_position_for(i)thenkey_at(i)— each recomputing the entry offset (header + i*bytes_per_entry) and re-running the bounds assert, on the same 13-byte entry / one cache line. This addsSaFileReader::entry(i) -> (position, Option<key>)+LearnedIndex::sa_entryand uses it at every per-probe site (the forward/backward boundary loops,push_unique_suffix_tail, the hint paths,lcp_at,ref_less/shares_prefix, the lockstep stepper) — one bounds check + one offset computation per probe instead of two.Honest scope
The probe loop is memory-latency-bound (the cold entry read dominates, prefetch overlaps it), so this trims instruction count without moving wall time much — but it is the hottest loop, and unlike the deferred LCP-accel (#36) it has no amortization question: it's strictly fewer ops per probe, never more. Net-positive or neutral, never a regression.
Byte-identity
entry(i)returns exactly(position(i), key_at(i))— proven by the newentry_equals_position_and_key_atround-trip test (modes 1+2). Each of the 15 call sites binds the same(pos, key)from one combined read; mode-1 still yieldskey = None; the explicitbump_probe()per site is preserved (probe count unchanged). The existing mem_search/spectrum/collect byte-identity oracle proptests pass unchanged. On-disk format unchanged.Series status
With this, the v0.3 perf carve is essentially complete — the only remaining item is the deferred LCP-accel wiring (#36), parked because it's net-neutral-to-negative for the latency-bound consumer (its foundation #35 stands as a ready primitive).
Pre-PR
/coderabbitai-review: 0 findings.Summary by CodeRabbit
Release Notes
Refactor
Tests