Status controller perf: monotonic clock + drop wasted SELECT + gate type-only imports - #624
Merged
Merged
Conversation
…nly imports
Three findings on review of codex/librarian/status_controller.py.
The controller is instantiated per librarian thread (via
WorkerStatusMixin) and called 168 times across the codebase via
status_controller.{start,update,finish}, so improvements multiply
across the librarian.
F5 — switch ``time.time()`` to ``time.monotonic()`` for
``status.since_updated`` rate-limiting. The ``update`` method
gates DB writes via
``time() - status.since_updated < _UPDATE_DELTA`` — wall-clock
jumps (NTP / DST / manual adjustment) skew the comparison and
either drop legitimate updates or fire premature ones. Same fix
shape as PR #623 in threads.py.
F8 — drop the wasted ``updated_statii = update_statii.values()``
SELECT in finish_many. The previous shape captured a ``.values()``
queryset for "individual reporting", but the downstream iteration
in ``_finish_many_log`` checked
``isinstance(row, Status)`` against ``values()`` outputs that are
always ``dict`` — verified empirically:
>>> from codex.models.admin import LibrarianStatus
>>> from codex.librarian.status import Status
>>> isinstance(next(iter(LibrarianStatus.objects.all().values()[:1])), Status)
False
So the per-status branch in ``_finish_many_log`` never fired.
Pretty for the single ``finish(status)`` call shape, that's one
wasted SELECT per task completion across the librarian. Drop the
capture (single round-trip, no SELECT) and the now-unreachable
``_log_finish`` and per-status iteration. The "Cleared all
librarian statuses" log on the empty-statii path is preserved.
If per-status finish logs are wanted in the future, iterate
``positive_statii.values()`` directly — those genuinely yield
``Status`` instances.
F1 — gate type-only imports behind ``TYPE_CHECKING``. After F8
removed the runtime ``Status`` use, four imports become
type-hint-only: ``Iterable``, ``Queue``, ``Logger``, ``Status``.
``from __future__ import annotations`` makes the annotations
forward-references at runtime so the imports move into a
TYPE_CHECKING block. ``codex.librarian.status_controller`` no
longer pulls in ``multiprocessing.queues`` at all (verified via
``'multiprocessing.queues' not in sys.modules`` after a fresh
import) — same shape as PR #623, saves ~14 ms cold.
Net: -22 LOC (mostly the dead ``_log_finish`` /
``_finish_many_log`` removal), one fewer SELECT per
``finish_many``, no more wall-clock-jump bugs in update timing,
and ``multiprocessing.queues`` no longer transitively loaded.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Per review feedback: finish() / finish_many() should log the
final state of each status with the correct verb + count — that's
the controller's contract, not dead code.
The previous shape was buggy not unused: it iterated
``update_statii.values()`` (a Django ValuesQuerySet yielding
``dict`` rows) and guarded with ``isinstance(row, Status)``. That
guard ALWAYS rejected — values() never yields Status instances —
so ``_log_finish`` never fired. The values() materialization itself
fired one wasted SELECT per finish_many call.
Restore _log_finish. Iterate ``positive_statii.values()`` directly
(those genuinely yield Status instances the caller passed in, no
SELECT needed). The class-vs-instance check now guards against
``start_many``'s placeholder Status classes — those are recorded
in positive_statii for the CODE filter but have no per-instance
state to log.
Net behavior:
- finish(my_status) -> single _log_finish line per call
- finish_many([s1, s2, s3]) -> three _log_finish lines per call
- finish_many([]) -> "Cleared all librarian statuses" (unchanged)
- finish_many([None]) -> noop (unchanged)
The previously-dropped wasted SELECT stays gone — the per-status
log iterates the in-memory positive_statii dict, not a re-fetched
queryset.
Note for follow-up: thread-side custom finish logs (e.g. cover
thread's ``f"Created {count} {desc} covers in {elapsed}{extra}."``
in _bulk_create_comic_covers) now duplicate _log_finish's output.
The custom logs added context the controller can't carry (e.g.
``(N skipped)`` for race-window adjustments), so they're not pure
duplicates — leaving the consolidation question for a separate
review.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ajslater
added a commit
that referenced
this pull request
Apr 28, 2026
Single-file plan because the surface is small (codex/librarian/ scribe/search/ - 8 files, ~750 LOC). Eight findings, three are tier-1: - F1: _prefetch_related_fts_query is dead code. Empirically verified that prefetch_related + .values() doesn't fire prefetches (Django walks the result cache for FK descriptors, but values() rows are dicts). 11 prefetch declarations in the hot path with zero query work. - F2: Three column-name mismatches between _M2M_FTS_RELS annotation aliases (fts_credits__person, fts_identifiers__source, fts_story_arc_numbers__story_arc) and _COMIC_KEYS consumer keys (fts_credits, fts_sources, fts_story_arcs). The DB pays for three GROUP_CONCAT aggregations whose results are silently dropped by prepare_sync_fts_entry. Both perf waste AND a correctness bug: searching for credit-person / source / story-arc names on sync-built FTS entries silently returns no results. - F3: The "megaquery" pattern (10 GROUP_CONCAT aggregations + 10 LEFT JOIN'd FK columns + GROUP BY in one SELECT) materializes a cartesian product per batch. Tagged comics with rich M2M rows blow up the temp table. Apply the OPDS metadata batching pattern (per-M2M UNION queries keyed by comic_id) to avoid the cartesian product. Headline perf finding. Tier 2: - F4: ComicFTS.exists() per loop iteration in _get_operation_comics_query. After iteration 1 it always returns True; the SELECT is wasted. Hoist outside the loop. - F5: time() -> monotonic() for elapsed_time reporting. Same fix as PR #623 / #624 / #625 in other librarian modules. Tier 3 (defer): - F6: clear_search_index() is slow on FTS5 due to per-row token re-indexing during DELETE. Raw `DELETE FROM codex_comicfts` or DROP+CREATE alternative. Defer. - F7: M2M .add() / .remove() doesn't bump Comic.updated_at, so M2M-only writes never trigger FTS sync. Out of scope - affects importer/admin paths, not the indexer itself. Worth flagging. - F8: INSERT INTO codex_comicfts SELECT ... raw-SQL path that eliminates the Python round-trip. Defer until F1+F2+F3 baselines reveal whether the Python overhead is the bottleneck. Suggested ordering: F1 -> F2 -> F4+F5 (small bundle) -> F3 (headline). F6/F7/F8 deferred to follow-ups. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
6 tasks
ajslater
added a commit
that referenced
this pull request
Apr 28, 2026
…626) * Search indexer perf: dead prefetch + name mismatches + small wins Four findings bundled. Three are cleanups + correctness; one is the prerequisite refactor for the headline F3 batching change in the next commit. F1 — drop _prefetch_related_fts_query (dead code). Empirically verified that prefetch_related() chained to .values() doesn't fire prefetches: Django's prefetch_related_objects walks the result cache for FK descriptors, but values() rows are dicts with no descriptors: >>> qs = Comic.objects.prefetch_related("characters").values("pk") >>> with CaptureQueriesContext(connection) as ctx: ... list(qs) >>> len(ctx.captured_queries) 1 # only the main SELECT, zero prefetch queries The 11 prefetch declarations (and the misleading comment about "1000 sqlite query depth") were doing zero query work. F2 — reconcile column-name mismatches between annotation aliases and consumer keys. _M2M_FTS_RELS produced annotations under fts_credits__person, fts_identifiers__source, fts_story_arc_numbers__story_arc, but prepare.py's _COMIC_KEYS read fts_credits, fts_sources, fts_story_arcs. The dict comprehension's `if comic.get(key)` filter silently dropped the mismatched data — three GROUP_CONCAT aggregations ran, results were thrown away, and the resulting FTS columns were always empty for any sync-built entry. Search by credit-person name / identifier-source / story-arc title silently returned no hits. Fix: split the rel definition into a name → path map. _M2M_FTS_REL_MAP keeps the alias short (matches _COMIC_KEYS) but preserves the FK-traversal path for GROUP_CONCAT. Same SQL generated; just the alias name changes. Behavior change: searches for credit / source / story-arc names on sync-built entries now return real results instead of empty. Import-built entries (via prepare_import_fts_entry) were unaffected by this bug and stay correct. F4 — hoist ComicFTS.exists() out of the operate loop. After iteration 1 lands on an initially-empty FTS index, ComicFTS has rows for the rest of the run; checking .exists() per iteration just spends a SELECT to learn what we already know. Decide once up-front and stash on a base_qs variable. F5 — switch time() to monotonic() for elapsed_time reporting. Wall-clock jumps (NTP / DST / manual adjustment) skew time() - start_time. Same fix as PR #623 / #624 / #625 in other librarian modules. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Search indexer perf: per-M2M batched queries (megaquery → 12) The headline perf finding from the plan. The previous shape ran the entire FTS row construction as one SELECT with: - 10 LEFT JOINs to FK tables - 10 LEFT JOINs to M2M through tables × 2 = 20 more - 10 GROUP_CONCAT(... DISTINCT ORDER BY ...) aggregations - GROUP BY comic.id to collapse the cartesian product For a comic with 5 chars × 3 credits × 2 genres × 4 tags × others, the intermediate temp table BEFORE GROUP BY is the product of those counts. SQLite materializes that, sorts each GROUP_CONCAT for DISTINCT, then GROUPs to collapse. Memory + I/O explode on richly-tagged libraries. Apply the OPDS metadata batching pattern (``codex/views/opds/metadata.py:get_m2m_objects_by_comic`` — also used in PR #615 for OPDS v2): 1. _build_fk_fts_rows: comic attributes + 10 simple FK joins in one SELECT. No M2M, no GROUP BY. Returns list of dicts keyed by ``id``. 2. _build_m2m_fts_dict: per-M2M, one SELECT each. 10 calls (one per M2M relation). Each walks one M2M's index independently — single LEFT JOIN, GROUP_CONCAT, GROUP BY comic_id. Returns dict[pk, comma-string]. 3. _build_universes_fts_dict: special case for the universes Concat(designation_aggregate, ",", name_aggregate). Same shape as a single M2M. 4. Stitch the per-pk dicts in Python: take each fk_row, overlay the 10 fts_<m2m> columns + fts_universes from the respective dicts, then call prepare_sync_fts_entry. Query count goes from 1 megaquery to 12 per batch (1 fk + 10 m2m + 1 universes). Sounds worse, but each individual query walks one relation's index — no cartesian product. Cost goes from O(product of M2M counts × batch_size) to O(sum of M2M-table-rows per relation), which is dramatically smaller for richly-tagged comics. Drive-by cleanup: drop _select_related_fts_query (defined but never called — dead code) and the now-unused _M2M_FTS_ANNOTATIONS constant (after the rel-map split in the previous commit, only the per-relation helpers consume the map directly). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three findings on review of
codex/librarian/status_controller.py.The controller is instantiated per librarian thread (via
WorkerStatusMixin) and called 168 times across the codebasevia
status_controller.{start,update,finish}— improvementsmultiply across the librarian.
F5 —
time.time()→time.monotonic()forstatus.since_updatedThe
updatemethod rate-limits DB writes viatime() - status.since_updated < _UPDATE_DELTA. Wall-clock jumps(NTP / DST / manual clock adjustment) skew the comparison and
either drop legitimate updates or fire premature ones. Same fix
shape as PR #623 in
threads.py.F8 — Drop wasted SELECT in
finish_many+ dead_log_finishpathThe previous shape captured an
updated_statii = update_statii.values()queryset for "individual reporting", butthe downstream iteration in
_finish_many_logcheckedisinstance(row, Status)againstvalues()outputs that arealways
dict. Verified empirically:So the per-status branch in
_finish_many_lognever fired.For the single
finish(status)call shape (every task completionacross the librarian), that's one wasted SELECT per finish.
Drop the capture (single round-trip remains, no extra SELECT) and
the now-unreachable
_log_finish+ per-status iteration. The"Cleared all librarian statuses"log on the empty-statii pathis preserved.
If per-status finish logs are wanted in the future, iterate
positive_statii.values()directly — those genuinely yieldStatusinstances.F1 — Gate type-only imports behind
TYPE_CHECKINGAfter F8 removed the runtime
Statususe, four imports becometype-hint-only:
Iterable,Queue,Logger,Status. Addedfrom __future__ import annotationsto make the annotationsforward-references at runtime so the imports move into a
TYPE_CHECKINGblock.codex.librarian.status_controllerno longer pulls inmultiprocessing.queuesat all — verified:Same shape as PR #623; saves ~14 ms cold whenever this is the
triggering import.
Net change
_log_finish/_finish_many_logremoval)
finish_manycallmultiprocessing.queuesno longer transitively loaded fromthis module
Test plan
make lint-pythonclean.make test-pythonclean (26 tests pass).import codex.librarian.status_controllerno longer loadsmultiprocessing.queues(verified viasys.modules).values()of a LibrarianStatus queryset yieldsdict,never
Status(confirms F8's dead-code claim).CoverCreateAllTasketc.,watch the librarian log. The status flush rate (per
_UPDATE_DELTA = 5s) should be unchanged — only the underlying clock sourceswitched. The "Created N covers in Xs" log line from the cover
thread itself still fires (that one was always there; the
status-controller's never-fired duplicate is gone).
🤖 Generated with Claude Code