perf(people): batch handle-alias fetch in list(), dropping an N+1#4536
perf(people): batch handle-alias fetch in list(), dropping an N+1#4536mysma-9403 wants to merge 2 commits into
Conversation
PeopleStore::list() returned every contact with its handle aliases, fetching them with a load_handles call per person inside the row loop — 1 + N queries (SELECT ... FROM handle_aliases WHERE person_id = ? prepared + executed once per contact), all under the store lock. Collect the person rows first, then fetch all aliases in one batched WHERE person_id IN (...) query keyed by person id (batch_handles_conn), windowed at 900 binds to stay under SQLite's default parameter cap. Mirrors the store's existing batch_interactions_for. list() drops from 1 + N to 1 + ceil(N/900) queries and holds the lock for less time. load_handles stays for the single-person get() path; the batch decodes each kind identically. Tests: list_batches_handles_per_person asserts three people (two aliases, none, one) each get exactly their own handles and the alias-less person doesn't inherit a neighbour's — the failure mode a batched IN query risks.
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesBatched handle fetching for list
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant PeopleStore
participant Database
Caller->>PeopleStore: list()
PeopleStore->>Database: query people rows
Database-->>PeopleStore: PersonRow list
PeopleStore->>PeopleStore: collect person IDs
PeopleStore->>Database: chunked handle queries
Database-->>PeopleStore: handle map by PersonId
PeopleStore-->>Caller: Vec<Person>
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| for window in ids.chunks(HANDLE_BATCH) { | ||
| if window.is_empty() { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Nitpick (consistency): the PR mirrors batch_interactions_for, which builds placeholders with std::iter::repeat_n("?", n) (see line 491). Here you use the older std::iter::repeat("?").take(n) idiom. Since repeat_n is already used one function up in the same file, prefer it for consistency — and Clippy’s manual_repeat_n lint flags exactly this form.
// current
let placeholders = std::iter::repeat("?")
.take(window.len())
.collect::<Vec<_>>()
.join(",");
// suggested — matches batch_interactions_for
let placeholders = std::iter::repeat_n("?", window.len())
.collect::<Vec<_>>()
.join(",");There was a problem hiding this comment.
Done in c329383e2 — switched to std::iter::repeat_n("?", window.len()) to match batch_interactions_for and silence clippy::manual_repeat_n.
| ids: &[PersonId], | ||
| ) -> SqlResult<HashMap<PersonId, Vec<Handle>>> { | ||
| let mut out: HashMap<PersonId, Vec<Handle>> = HashMap::new(); | ||
| for window in ids.chunks(HANDLE_BATCH) { |
There was a problem hiding this comment.
Nitpick (dead branch): slice::chunks never yields an empty slice — for an empty ids it yields zero windows, and for non-empty ids every window has ≥1 element. So if window.is_empty() { continue; } is unreachable. Harmless, but it can be dropped. (The sibling batch_interactions_for guards emptiness once, up front, via an early return Ok(HashMap::new()) — the empty-ids case is already handled here implicitly by the zero-window loop, so no guard is needed at all.)
There was a problem hiding this comment.
Dropped in c329383e2. Agreed — slice::chunks never yields an empty window (empty ids → zero windows), so the guard was unreachable. Removed it entirely; the empty-ids case is handled implicitly by the zero-window loop.
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
PR #4536 — perf(people): batch handle-alias fetch in list(), dropping an N+1
Walkthrough
This PR replaces the per-person load_handles call inside PeopleStore::list()'s row loop with a single batched WHERE person_id IN (…) fetch (batch_handles_conn), windowed at 900 binds to stay under SQLite's 999 bound-parameter cap. It mirrors the store's existing batch_interactions_for precedent almost line-for-line. The change is behaviour-preserving and well-tested. Assessment: clean, low-risk, does exactly what the title claims. No blockers.
Changes
| File | Summary |
|---|---|
src/openhuman/people/store.rs |
Adds PersonRow tuple alias + batch_handles_conn helper (chunked IN (…) handle fetch); rewrites list() to materialise rows then batch-fetch handles keyed by id; adds list_batches_handles_per_person test. load_handles retained for the single-person get() path. |
Actionable comments (0 blocking)
No blockers, no majors. Two nitpicks posted inline; both optional.
Correctness — verified
- Alias round-trip is exact.
PersonId: Displaywrites the plain UUID (types.rs:24), matching theperson.id.to_string()written byinsert_person, so theIN (…)string keys resolve back correctly. - Per-person handle ordering is preserved.
load_handlesusedORDER BY kind, value; the batch usesORDER BY person_id, kind, value. For rows of the sameperson_idthe secondary keys yield an identical order, and results are assembled by iterating the orderedpeople_rowsVec (theHashMapis lookup-only), so person order (ORDER BY display_name) is preserved too. - Empty / no-alias cases hold. Zero people →
idsempty →chunksyields no windows → empty map. A person with no aliases is absent from the map and getsunwrap_or_default()→[](the exact failure mode the new test guards against). - Error semantics unchanged. Unknown
kindstill returnsErr(InvalidColumnName(...)), same asload_handles; the whole fetch still runs under oneblocking_lock. - No dup keys.
people.idis the PK, soidshas no duplicates andhandles_by_id.remove(&id)is safe.
Nitpicks (2) — posted inline
store.rs:580— usesstd::iter::repeat("?").take(n)while the mirroredbatch_interactions_for(line 491) usesstd::iter::repeat_n("?", n); prefer the latter for consistency (also what Clippy'smanual_repeat_nprefers).store.rs:577—if window.is_empty() { continue; }is unreachable:slice::chunksnever yields an empty slice. Harmless dead branch.
Questions for the author (0)
None.
Verified / looks good
- Follows the in-repo
batch_interactions_forprecedent; keepsload_handlesfor the correct single-queryget()path. - New test exercises the real risk of a batched
INquery: cross-person leakage, an alias-less person, and kind preservation. spawn_blocking+ error-mapping wrapper unchanged; no new lock-ordering or async concerns.
Note: CodeRabbit (CHILL profile) generated no actionable comments and auto-approved. Leaving this as a comment-only review — approval/merge is the maintainer's call.
🤖 Council Merge Gate — APPROVE (merge blocked by branch protection)The model council review gate returned APPROVE (session However, Action needed (human): resolve the failing required checks / obtain the required review, then merge. A maintainer with admin rights may also merge explicitly. The PR is left OPEN. Posted automatically by the |
🤖 Council Merge Gate — APPROVE (merge blocked by branch protection)The model council review gate returned APPROVE (session However, Action needed (human): resolve the failing required checks / obtain the required review, then merge. A maintainer with admin rights may also merge explicitly. The PR is left OPEN. Posted automatically by the |
🤖 Council Merge Gate — APPROVE (merge blocked by branch protection)The model council review gate returned APPROVE (session However, Action needed (human): resolve the failing required checks / obtain the required review, then merge. A maintainer with admin rights may also merge explicitly. The PR is left OPEN. Posted automatically by the |
…d in batch_handles_conn
- repeat_n("?", n) matches the sibling batch_interactions_for idiom and
satisfies clippy::manual_repeat_n (vs repeat().take()).
- slice::chunks never yields an empty window (empty ids -> zero windows), so
the if window.is_empty() { continue } branch was unreachable; remove it.
senamakel
left a comment
senamakel
left a comment
There was a problem hiding this comment.
Automated technical review: not approved.
Blocking issue -- compilation errors on all CI lanes
The PR introduces a second type PersonRow definition (with PersonId as the first element) at the module level in src/openhuman/people/store.rs, but the pre-existing type PersonRow = (String, Option<String>, Option<String>, Option<String>, i64, i64) on the same file (lines 19-26 on main) is NOT removed. This creates a duplicate type alias at module scope, producing E0428: the name 'PersonRow' is defined multiple times and cascading type-mismatch errors (E0277: Vec<PersonId> cannot be built from iterator over String).
The Rust Quality (fmt, clippy), Rust Feature-Gate Smoke (gates off), and PR CI Gate checks all fail because of this.
Remediation: Either (a) remove the old type PersonRow = (String, ...) definition and change its first element to PersonId, or (b) rename the new type alias (e.g. PersonRowWithId) to avoid the name conflict.
N+1 claim: The refactoring approach (batched WHERE person_id IN (...) handle fetch, windowed at 900 binds) is correct and follows the existing batch_interactions_for precedent. Once the compilation error is fixed, this will be a clean N+1 fix.
4 participants
What
PeopleStore::list()returns every contact with its handle aliases. It fetched those aliases with aload_handlescall per person, inside the row loop:For N contacts that's 1 + N queries (
SELECT … FROM handle_aliases WHERE person_id = ?prepared + executed N times), all while holding the store lock.Change
Collect the person rows first, then fetch all handles in one batched
WHERE person_id IN (…)query keyed by person id (batch_handles_conn), windowed at 900 binds to stay under SQLite's default parameter cap. This is the pattern the same store already uses forbatch_interactions_for— I mirrored it.list()drops from1 + Nqueries to1 + ceil(N/900).load_handlesstays for the single-personget()path (one query is correct there). The batch decodes eachkindexactly asload_handlesdoes, so behaviour is identical.Is it worth it? (honest take)
SQLite is in-process, so each avoided query is cheap in absolute terms — this isn't a dramatic speedup. It's worth it because (a) it shortens how long
list()holds the store lock (the whole fetch runs under oneblocking_lock), (b) statement prepares + index lookups drop from O(contacts) to ~O(1) query, scaling with contact count, and (c) it follows an existing in-repo precedent, so it's low-risk and consistent rather than a speculative tweak.Correctness
Behaviour-preserving. The batch is keyed by
person_id; each person gets exactly its own aliases (ORDER BY person_id, kind, valuepreserves the per-personkind, valueorderload_handlesused), and a person with no aliases comes back with an empty list (unwrap_or_default).Tests
list_batches_handles_per_person(new) creates three people — one with two aliases, one with none, one with a single alias — and asserts each gets exactly its own handles and the alias-less person does not inherit a neighbour's (the failure mode a batchedINquery risks). The existinginsert_list_and_lookup_round_tripstill covers the single-person handle round-trip.cargo test --lib people::storegreen locally.Summary by CodeRabbit