fix(bench): skip template-snapshot capture in the query-store builds - #351
Conversation
Stack-sampled on the baseline box mid-run: the B2 store build over LogHub HDFS_v2 (16 GiB) sat in harness snapshot capture -> MinerCluster::templates_for -> full template-set clone. Every new (template_id, template_version) pair pays that walk, and HDFS_v2's widening churn produces version bumps every few lines — quadratic, measured at ~3 KB/s (a ~57-day build). The store builds never read the snapshots (their callbacks ignore the argument); only C1 does. run_streaming now takes capture_snapshots and build_store passes false — the gates path is unchanged (run() passes true). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughA ChangesSnapshot Capture Gating
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR removes a major benchmark-harness performance bottleneck by making per-record template snapshot capture optional, and disabling it for query-store/B1/B2 store builds where snapshots are not consumed.
Changes:
- Extend
harness::run_streamingwith acapture_snapshotsflag and gate snapshot capture behind it. - Update the eager gates path (
run()) to preserve existing snapshot behavior (capture_snapshots = true). - Update the store-build path to skip snapshot capture (
capture_snapshots = false) to avoid quadratic behavior on churny corpora.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/ourios-bench/src/store.rs | Disables snapshot capture for store builds by passing capture_snapshots = false to the streaming harness. |
| crates/ourios-bench/src/harness.rs | Adds capture_snapshots to run_streaming and uses it to skip template snapshot capture work when not needed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/ourios-bench/src/harness.rs (1)
130-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider replacing the two adjacent bool params with a small options type.
capture_auditandcapture_snapshotsare now two same-typed positional bools next to each other; both call sites (run_streaming(stream, false, false, ...)in store.rs,run_streaming(..., capture_audit, true, ...)here) rely on remembering argument order with no compiler backing. Clippy's ownfn_params_excessive_boolspedantic lint (denied per this crate's guidelines) exists precisely because such signatures are "confusing and error prone" — the function is now one more bool away from tripping it at the default threshold of 3.♻️ Suggested direction
-pub(crate) fn run_streaming<T, I, F>( - corpus: I, - capture_audit: bool, - capture_snapshots: bool, - mut on_record: F, -) -> Result<HarnessResult, BenchError> +pub(crate) struct StreamingOptions { + pub capture_audit: bool, + pub capture_snapshots: bool, +} + +pub(crate) fn run_streaming<T, I, F>( + corpus: I, + opts: StreamingOptions, + mut on_record: F, +) -> Result<HarnessResult, BenchError>🤖 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 `@crates/ourios-bench/src/harness.rs` around lines 130 - 134, The run_streaming API in harness.rs uses two adjacent positional bools, capture_audit and capture_snapshots, which is confusing and fragile at its call sites. Replace them with a small options type (or equivalent grouped config) and update run_streaming plus its callers in store.rs and the current caller to pass named configuration instead of relying on bool order. Keep the change localized around run_streaming so the intent is explicit and the signature no longer depends on remembering argument positions.
🤖 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 `@crates/ourios-bench/src/harness.rs`:
- Around line 190-201: `harness.rs` does not currently cover the
`capture_snapshots = false` path in `run_streaming`, so add a direct test that
calls `run_streaming(..., false, false, ...)` with a non-lossy record that has a
real `template_id` and string body, then assert the callback receives `None` for
the snapshot. Use the existing `run_streaming` and `want_snapshot` logic in the
harness to locate the change and mirror the current `run()`-based test setup.
---
Nitpick comments:
In `@crates/ourios-bench/src/harness.rs`:
- Around line 130-134: The run_streaming API in harness.rs uses two adjacent
positional bools, capture_audit and capture_snapshots, which is confusing and
fragile at its call sites. Replace them with a small options type (or equivalent
grouped config) and update run_streaming plus its callers in store.rs and the
current caller to pass named configuration instead of relying on bool order.
Keep the change localized around run_streaming so the intent is explicit and the
signature no longer depends on remembering argument positions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b662ea1-0022-4889-b6be-8831e447a81d
📒 Files selected for processing (2)
crates/ourios-bench/src/harness.rscrates/ourios-bench/src/store.rs
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e snapshot test Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Found live on the baseline box during the first 10–100 GiB scale run (HDFS_v2, 16 GiB): the B2 store build ran at ~3 KB/s — extrapolated ~57 days. Three gdb stack samples all landed in the bench harness's per-record snapshot capture: every new
(template_id, template_version)pair callsMinerCluster::templates_for, which walks and clones the entire template set, and HDFS_v2's node logs drive template widening (a version bump) every few lines → quadratic in practice.Two facts make the fix surgical:
build_query_store/build_b1_store) never read the snapshots — their callbacks ignore the argument. Only C1 (reconstruction) consumes them.templates_forper record — this is bench-harness overhead, not a miner pathology. (Worth stating clearly since miner correctness/perf is hazard docs: add verification process spec #1: the stacks exonerate the miner itself.)run_streaminggains acapture_snapshotsflag;build_storepassesfalse; the gates path (run()) passestrue— C1/A1/C2 behavior unchanged.Follow-up worth tracking (not this PR): C1 itself pays the same walk on churny corpora; an incremental
templates_for_id(template_id)on the miner would make gate runs scale too.Verification: full
ourios-benchsuite green, fmt + clippy-D warningsclean. The scale run resumes on the already-provisioned box once this merges (corpus staged, ~5 min rebuild).🤖 Generated with Claude Code
Summary by CodeRabbit