Files
dtourolle ce72fe49a0 Record the catalog lessons of the 2026-09-25 performance pass
Three habits the pass found broken, in the style of the 2026-09-19 notes:
SQL text passed to `execute`/`query_row` in a loop is a prepare per row
(the merge, `persist` and the shard sync all paid it), a case-insensitive
`LIKE` cannot use the `source_ref` key where a range can, and the backfill
inside `Catalog::open` is paid by every worker thread, the develop view's
fetches included. And where the two new benches are and how to run them.
2026-09-25 22:06:58 -04:00

166 lines
9.1 KiB
Markdown

# Working in this repository
Notes for anyone — person or agent — changing this code. They record what
went wrong once and what the fix looked like, so the same shape is not
written again. Requirements live in `docs/dev/requirements.md`; this file is
about habits, not features.
## Catalog reads: work is proportional to what changed, never to library size
`docs/dev/catalog.md §1` states the rule. These are the ways it was broken on
the Identity screen, found when every confirm click cost half a second on a
24k-image library (2026-09-19), and what each fix looked like.
**A redraw must know what changed.** A click handler that calls "refresh
everything" pays for everything. `identity_ui::refresh` takes a `Changed`:
a confirm re-reads the rail and the grid and *not* the coverage line,
because moving a face between people cannot alter how many images are
indexed. Before adding a read to a shared refresh, ask which events can
change its answer, and gate it on those.
**Count with `COUNT(*)`, never with `.len()` on a list you then drop.**
`repairs::counts` used to build every repair's work list — a `Target` with
its path per row, sorted into visiting order — to report its length. Six
repairs, 350 ms, nothing kept. If the caller wants a number, the query
returns a number.
**One query, not one per row.** `ThumbStore::contains` in a filter over
5,000 rows is 5,000 prepared statements; `ThumbStore::held(size)` reads the
index once into a set. The same applies to any `query_row` inside a loop
over a result set — including `deep_count` per sidebar row, which is fine
at sidebar scale and would not be at grid scale. Aggregate in one
statement and look up in memory.
**SQL text in a loop is a prepare in a loop.** rusqlite's `execute` and
`query_row` compile their statement on every call. A loop that calls them
per row pays a prepare per row even when each query is a primary-key seek:
the merge of a synced catalog prepared four statements for each of 13,000
incoming faces (450 ms of a pass that changed nothing), `persist` four per
photograph a scan listed (1.5 s for a first scan), the shard sync one per
image each way. Hoist the statement, use `prepare_cached`, or — better, when
the loop asks the same table about every row — read that table once into a
map. And do not rewrite a row with what it already holds: an upsert of
identical values still dirties the page.
**A `LIKE` is case-insensitive, and no index here serves that.**
`source_ref LIKE 'stem.%'` read every name of the root per sidecar a pull
took in. When the check that decides is exact, spell the prefix as a range
(`>= 'stem.' AND < 'stem/'`, `/` being the byte after `.`), which the
`(root_id, source_ref)` key answers with a seek.
**`Catalog::open` is not free, and every worker thread calls it.** The
backfill runs on every open, and the develop view opens a catalog to fetch
each original and again for each neighbour it prefetches. Keep each
backfill step's no-op case to a read of the small side — the unpaired
JPEGs, not every RAW; the distinct keywords, not every assignment — and
measure an open with `catalog_bench` after adding one.
**Filter and aggregate in SQL, and aggregate the small side first.**
`faces::people` read 19,000 rows, grouped, sorted them by name, and the
screen threw 17,000 away (empty unnamed groups). `people_in_use` filters in
the `WHERE`, and joins `people` to a pre-aggregated `face_person` (2,000
groups) rather than grouping after a `LEFT JOIN` over every person. The
sort then sees only the rows that will be drawn.
**Wide rows make "just check one column" a table scan.** A `faces` row is
~8 KB (a 1 KB embedding and a ~5 KB crop, then the columns added later).
Any predicate that reads `quality`, `crop` or an eye column for every face
reads every row. V17 learned this for the eye filter; V19 applies it to the
repair counts with partial indexes (`faces_owed_*`) that hold only the rows
still owing, keyed on what the predicate joins on and carrying `model_id`
because the predicate reads it. Two things to know about them:
- **Drive the count from the small side.** SQLite uses a partial index
when the query starts from `faces` (`repairs::count`, `Needs::Face`) and
ignores it inside a correlated `EXISTS (... WHERE f.image_id = i.id ...)`.
That is why `Needs::Face` carries the per-face fragment and spells it two
ways.
- **Spell the predicate as the index's `WHERE` is spelled.** `NEEDS_EYES`
is `(f.eye_right IS NULL OR f.landmarks_dense IS NULL)` because
`faces_owed_eyes` is `WHERE eye_right IS NULL OR landmarks_dense IS NULL`.
Change one, change both, and `counts_are_the_sizes_of_the_lists` will
tell you if they drift.
Check a query's plan with `EXPLAIN QUERY PLAN` against a copy of a real
catalog before trusting an index exists for it: "SEARCH ... USING COVERING
INDEX" is the answer you want, "SEARCH f USING INDEX faces_image" on a wide
table means every probe opens a row.
## Catalog writes: one transaction per user action
`faces::confirm` opens a transaction. Calling it in a loop over a group is
a commit per face; `faces::confirm_all` is two statements and one commit,
`faces::reassign` one transaction for a whole split. When a UI action
touches N rows, give the catalog a function that takes the N, not a loop
that calls the one-row function N times — `unchecked_transaction` cannot
nest, so this has to be designed in at the catalog layer, not wrapped
from above.
## Screens: keep what is already decoded
`identity::load_faces` takes the crops the grid is currently showing and
hands them back into the new cells. Before that, a click re-read 4 MB of
crop blobs and decoded 700 JPEGs to produce the pixels already on screen.
When a redraw replaces a model, the expensive parts of the old model — a
decoded image, a cut portrait — are the first thing to reuse; only the row
that changed needs new work. Drain the old cells rather than cloning them.
## Remote calls: one round trip per file, not one per ancestor
`NextcloudBackend::move_to` guaranteed its destination's parent with a
`MKCOL` per ancestor from the account root, on every file of a batch —
three `405`s before each `MOVE`. The backend now remembers the collections
it has confirmed (`known_dirs`) for its lifetime, which is one job. When a
per-file operation has a per-batch precondition, satisfy it once.
## Providers: read the runtime's source for the version on disk, not the binding
Two things the MIGraphX rung (2026-09-20) got wrong before it was measured
right, both because `ort`'s builder was trusted to mean what its method
names say.
**A binding's option builder may fill a struct the runtime no longer
reads.** `ep::MIGraphX::with_save_model` sets fields of the legacy
`OrtMIGraphXProviderOptions`; ONNX Runtime 1.29 reads that struct for the
precision flags and ignores the rest, so every session compiled for 40 s
and the cache directory went nowhere. The option that works
(`migraphx_model_cache_dir`) exists only in the generic key/value
registration, which `session::migraphx` calls on the API table directly.
Before wiring a provider option, fetch the provider's source at the
runtime's exact version and find where the option is *read*.
**A provider's cache key may leave out what you are varying.** MIGraphX
keys a compiled program on graph, GPU and its own version — not precision.
The first fp16 measurement built in 0.3 s and matched f32 to the tenth of a
millisecond, because it had loaded the f32 program. A "from cache" build
that is suspiciously fast on the first run of a new configuration is a key
collision, not a fast provider; give each precision its own directory (the
engine does) and check the cache directory gained a file.
## Measuring
`cargo run --release -p dr-ui --example identity_bench -- CATALOG THUMBS`
times what one click on the Identity screen reads and what the batch
operations write. Run it against a **copy** of a real catalog (it writes),
never the library's own file; `sqlite3 catalog.sqlite ".backup copy.sqlite"`
takes a consistent one while the app runs. Compare the `cpu` column when
other builds are running on the machine — the wall clock doubles under
load, the CPU figure does not. Keep the binary from before the change and
run both back to back rather than trusting numbers taken an hour apart.
`cargo run --release -p dr-catalog --example catalog_bench -- CATALOG
[FACES_DIR]` does the same for opening the catalog (the backfill step by
step), the upload snapshot, a merge, and the face shard export and import;
`persist_bench`, an ignored test in `dr-ui`'s scan module, replays a scan's
`persist` and a sidecar pull (`DR_BENCH_CATALOG=copy.sqlite cargo test
--release -p dr-ui --lib persist_bench -- --ignored --nocapture
--test-threads=1`). Both take copies; hand `catalog_bench` a copy of the
face store directory too.
Reference figures from the 2026-09-19 fixes, largest person (754 faces),
24k images, 19k faces, before → after. What one click read: `load_people`
22 ms → 12 ms, `load_faces` 316 ms → 2.4 ms, `audit` 190 ms → not run
(66 ms when it is, on open and at the end of a sweep). What one click
wrote: `confirm_all` 16 ms → 2 ms, `split_off` 23 ms → 4.5 ms. A click on
the face grid went from ~530 ms of catalog work to ~15 ms.