From 9cff677392043d22799118e939ec9287c4b17340 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Fri, 25 Sep 2026 22:04:19 -0400 Subject: [PATCH] Merge a synced catalog's face assignments without re-preparing per face A sync pass that brought nothing new cost 450-540 ms of CPU in `merge_remote_catalog` on the reference library (24k images, 19k faces), measured by catalog_bench merging a copy of the catalog with itself. Most of it was the loop over the other device's confirmed faces and the faces under its ignored groups -- 13,000 rows. For each one it prepared three statements from scratch (`query_row`/`execute` with a SQL string compile the statement every call) and then rewrote the `face_person` row with the values it already held, dirtying a page per face on every pass. The rejection loop prepared three more per row. The statements are now `prepare_cached`, the local assignment is read once per face (whether it is confirmed, and what it holds, come from the same row), and the upsert is skipped when the row already says exactly that. `faces_assigned` is still counted for those rows, so the report is the one the old code gave, and nothing else reads the difference: the row is byte-for-byte what the upsert would have written. After: 279 ms (best of 5, CPU), with every catalog table identical after the run to the old build's. --- core/dr-catalog/src/merge.rs | 93 ++++++++++++++++++++---------------- 1 file changed, 51 insertions(+), 42 deletions(-) diff --git a/core/dr-catalog/src/merge.rs b/core/dr-catalog/src/merge.rs index 97cce78..5da149a 100644 --- a/core/dr-catalog/src/merge.rs +++ b/core/dr-catalog/src/merge.rs @@ -931,16 +931,36 @@ fn merge_people_within(tx: &Connection, report: &mut MergeReport) -> Result<(), .query_map([], |r| Ok((r.get(0)?, r.get(1)?, r.get(2)?, r.get(3)?)))? .collect::>()?; + // Cached, not prepared per row: a steady-state pass walks every + // confirmed and every ignored face the other device holds -- 13,000 on + // the reference library -- and preparing four statements for each was + // most of the 780 ms a merge that changed nothing cost. + let mut person_of = tx.prepare_cached("SELECT id FROM people WHERE uuid = ?1")?; + let mut current = tx.prepare_cached( + "SELECT person_id, probability, confirmed FROM face_person WHERE face_id = ?1", + )?; + let mut rejected_q = tx.prepare_cached( + "SELECT EXISTS(SELECT 1 FROM face_person_rejected + WHERE face_id = ?1 AND person_id = ?2)", + )?; + let mut assign = tx.prepare_cached( + "INSERT INTO face_person (face_id, person_id, probability, confirmed) + VALUES (?1, ?2, ?3, ?4) + ON CONFLICT(face_id) DO UPDATE SET + person_id = excluded.person_id, + probability = excluded.probability, + confirmed = excluded.confirmed", + )?; + for (remote_face, uuid, probability, confirmed) in incoming { let Some(&local_face) = face_map.get(&remote_face) else { continue; }; - let person: Option = tx - .query_row("SELECT id FROM people WHERE uuid = ?1", [&uuid], |r| { - r.get(0) - }) - .optional()?; + let person: Option = person_of.query_row([&uuid], |r| r.get(0)).optional()?; let Some(person) = person else { continue }; + let held: Option<(i64, f64, i64)> = current + .query_row([local_face], |r| Ok((r.get(0)?, r.get(1)?, r.get(2)?))) + .optional()?; // A local confirmation is never overwritten, in either direction. // Two devices confirming the same face as different people is a @@ -948,13 +968,7 @@ fn merge_people_within(tx: &Connection, report: &mut MergeReport) -> Result<(), // settle it with; silently taking the remote's answer would let a // sync undo something the user did here. It stays as it is, and the // user can change it on the device they are looking at. - let locally_confirmed: bool = tx.query_row( - "SELECT EXISTS(SELECT 1 FROM face_person - WHERE face_id = ?1 AND confirmed = 1)", - [local_face], - |r| r.get(0), - )?; - if locally_confirmed { + if held.is_some_and(|(_, _, confirmed)| confirmed == 1) { report.faces_kept_local += 1; continue; } @@ -963,25 +977,22 @@ fn merge_people_within(tx: &Connection, report: &mut MergeReport) -> Result<(), // this user's judgement about this pair, and re-suggesting what // they pushed away is the behaviour that makes the feature feel // broken. - let rejected: bool = tx.query_row( - "SELECT EXISTS(SELECT 1 FROM face_person_rejected - WHERE face_id = ?1 AND person_id = ?2)", - [local_face, person], - |r| r.get(0), - )?; + let rejected: bool = rejected_q.query_row([local_face, person], |r| r.get(0))?; if rejected { continue; } - tx.execute( - "INSERT INTO face_person (face_id, person_id, probability, confirmed) - VALUES (?1, ?2, ?3, ?4) - ON CONFLICT(face_id) DO UPDATE SET - person_id = excluded.person_id, - probability = excluded.probability, - confirmed = excluded.confirmed", - rusqlite::params![local_face, person, probability, confirmed], - )?; + // Written only when it differs. Rewriting a row with the values it + // already holds dirtied a page per face, every pass, for nothing; + // the report still counts it, as it always has. + if held != Some((person, probability, i64::from(confirmed))) { + assign.execute(rusqlite::params![ + local_face, + person, + probability, + confirmed + ])?; + } report.faces_assigned += 1; } } @@ -997,32 +1008,30 @@ fn merge_people_within(tx: &Connection, report: &mut MergeReport) -> Result<(), .query_map([], |r| Ok((r.get(0)?, r.get(1)?)))? .collect::>()?; + let mut person_of = tx.prepare_cached("SELECT id FROM people WHERE uuid = ?1")?; + let mut reject = tx.prepare_cached( + "INSERT OR IGNORE INTO face_person_rejected (face_id, person_id) + VALUES (?1, ?2)", + )?; + let mut unsuggest = tx.prepare_cached( + "DELETE FROM face_person + WHERE face_id = ?1 AND person_id = ?2 AND confirmed = 0", + )?; + for (remote_face, uuid) in incoming { let Some(&local_face) = face_map.get(&remote_face) else { continue; }; - let person: Option = tx - .query_row("SELECT id FROM people WHERE uuid = ?1", [&uuid], |r| { - r.get(0) - }) - .optional()?; + let person: Option = person_of.query_row([&uuid], |r| r.get(0)).optional()?; let Some(person) = person else { continue }; - let n = tx.execute( - "INSERT OR IGNORE INTO face_person_rejected (face_id, person_id) - VALUES (?1, ?2)", - [local_face, person], - )?; + let n = reject.execute([local_face, person])?; report.faces_rejected += n; // A rejection that lands on a face currently *suggested* to be // that person has to take the suggestion with it, or the screen // keeps offering exactly what the other device just refused. - tx.execute( - "DELETE FROM face_person - WHERE face_id = ?1 AND person_id = ?2 AND confirmed = 0", - [local_face, person], - )?; + unsuggest.execute([local_face, person])?; } }