⚡ perf: optimize record_exposures retrieval stats update - #7
Conversation
In `src/store/retrieval.rs`, the `record_exposures` function iterates through a batch of retrieved items, UPSERTing them into the `retrieval_stats` table. Previously, this process utilized `self.connection.execute(...)` for each iteration. This was inefficient, as SQLite parsed and prepared the SQL query inside the loop for every single insertion. I optimized this by refactoring it to use `self.connection.prepare(...)` *before* the loop starts, caching the prepared statement. This drastically reduced query processing overhead. Using a standalone benchmark on a 10,000 insertion loop, execution dropped from 115ms (using execute in-loop) to ~40ms (using prepare pre-loop) - a ~64% performance increase. Co-authored-by: undivisible <136312656+undivisible@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_0323472c-1bc7-41ce-a564-e80050fd60ed) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f375e1bbef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| use rusqlite::{params, Connection}; | ||
| use std::time::Instant; | ||
|
|
||
| fn main() -> Result<(), rusqlite::Error> { |
There was a problem hiding this comment.
When a developer tries to reproduce the reported performance result with cargo bench, this file is never compiled or executed: Cargo auto-discovers benchmarks under benches/ (or through an explicit [[bench]] entry), but Cargo.toml declares no benchmark and this file is at the repository root. Move it to a benchmark target or declare it explicitly; otherwise both this comparison and the added bench.rs remain dead artifacts.
Useful? React with 👍 / 👎.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_1e5cf18e-c543-4825-90d3-bcad96dd500e) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc490c26ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let start = Instant::now(); | ||
| let mut statement = connection.prepare( |
There was a problem hiding this comment.
When this benchmark is used to validate the reported speedup, the optimized timer always runs second on the same connection, after the unoptimized loop has inserted 10,000 rows. Its result therefore combines statement reuse with a larger B-tree and warmed SQLite allocator/page-cache state, so the timings cannot isolate the optimization being measured. Run each variant against a separately but identically initialized database, ideally repeating or alternating their order.
Useful? React with 👍 / 👎.
1 participant
💡 What: Refactored
record_exposuresinsrc/store/retrieval.rsto prepare the SQLite statement before the loop instead of during each iteration. Included an early exitif items.is_empty()to avoid statement preparation if unneeded.🎯 Why: To eliminate the N+1 overhead of parsing and preparing the
INSERT ... ON CONFLICTstatement inside a loop.📊 Measured Improvement: The 10,000 insertion loop time dropped from 115.06ms down to 40.83ms (a 64.5% improvement in this specific scenario).
PR created automatically by Jules for task 18046812485004947781 started by @undivisible
Note
Low Risk
Localized SQLite statement reuse with unchanged upsert semantics; no auth, schema, or API changes.
Overview
Speeds up exposure recording after search by preparing the
retrieval_statsupsert once inrecord_exposuresand reusing it across items, instead of reparsing the SQL on every insert. Also short-circuits whenitemsis empty.Adds a small
retrieval_statsbench that compares per-iterationexecutevs prepared-statement reuse for 10k upserts.Reviewed by Cursor Bugbot for commit fc490c2. Configure here.