Threads + worker: gate type-only imports + monotonic clock - #623
Merged
Merged
Conversation
Two changes to the librarian thread infrastructure both ride this
diff because the file lives in the import chain of all 9 librarian
daemons (cover, bookmark, notifier, scribe, fs.event_batcher,
fs.watcher, fs.poller, cron, librariand). Saving here multiplies.
F1 - type-only imports gated behind TYPE_CHECKING. Both
``multiprocessing.queues.Queue`` and ``loguru._logger.Logger`` are
referenced ONLY in function-signature type annotations
(NamedThread.__init__ and WorkerMixin.init_worker). Adding
``from __future__ import annotations`` makes the annotations
forward-references at runtime, so the imports can move into a
TYPE_CHECKING block.
$ uv run python -X importtime -c \
"from multiprocessing.queues import Queue" 2>&1 | grep multiprocessing
import time: 14045 us | multiprocessing.queues # cold
After this change ``codex.librarian.threads`` no longer pulls in
``multiprocessing.queues`` at all — confirmed via
``'multiprocessing.queues' in sys.modules`` after fresh import.
``loguru._logger`` was already loaded transitively by Django's
logging configuration, so the saving for it is structural rather
than measurable, but the type-only import was misleading.
F2 - switch ``time.time()`` to ``time.monotonic()`` in
AggregateMessageQueuedThread. The thread does elapsed-time math
(``time.time() - self._last_send``) on every queued item to
decide whether to flush the cache. ``time.time()`` returns
wall-clock and can jump (NTP correction, daylight saving, manual
clock change), which would skew the flush-timing logic in
subclasses that rely on it (BookmarkThread, NotifierThread).
``time.monotonic()`` is immune to clock jumps and is also
slightly cheaper on most platforms (CLOCK_MONOTONIC vs
gettimeofday). Correctness fix with an incidental perf win on
the hot per-item path.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
5 tasks
ajslater
added a commit
that referenced
this pull request
Apr 28, 2026
…ype-only imports (#624) * Status controller: monotonic clock + drop wasted SELECT + gate type-only 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> * Status controller: restore _log_finish, fix bug to fire per status 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> --------- 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
1 participant
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
Two small changes to
codex/librarian/threads.pyand its siblingcodex/librarian/worker.py. Both files sit at the head of theimport chain for all 9 librarian daemons (cover / bookmark /
notifier / scribe / fs.event_batcher / fs.watcher / fs.poller /
cron / librariand), so savings here multiply across the
codebase's startup graph.
F1 — Type-only imports gated behind
TYPE_CHECKINGmultiprocessing.queues.Queueandloguru._logger.Loggerarereferenced only in function-signature type annotations
(
NamedThread.__init__andWorkerMixin.init_worker). Addingfrom __future__ import annotationsmakes the annotationsforward-references at runtime, so the imports can move into a
TYPE_CHECKINGblock.multiprocessing.queuescold import cost:After this change
codex.librarian.threadsno longer pulls inmultiprocessing.queuesat all — confirmed via:loguru._loggerwas already loaded transitively by Django'slogging configuration, so the savings for it is structural
rather than measurable. The type-only import in worker.py was
misleading either way.
F2 —
time.time()→time.monotonic()inAggregateMessageQueuedThreadThe aggregator thread does elapsed-time math
(
time.time() - self._last_send) on every queued item to decidewhether to flush the cache.
time.time()returns wall-clock andcan jump (NTP correction, daylight saving, manual clock change),
which would skew the flush-timing logic in subclasses that rely
on it (
BookmarkThread,NotifierThread).time.monotonic()is immune to clock jumps and is also slightlycheaper on most platforms (
CLOCK_MONOTONICvsgettimeofday).Correctness fix with an incidental perf win on the per-item hot
path.
Test plan
make lint-pythonclean (ruff + format + basedpyright +vulture + complexipy + codespell).
make test-pythonclean (26 tests pass).import codex.librarian.threadsno longer loadsmultiprocessing.queues(verified viasys.modules).flush-timing unaffected. Aggregator threads should still flush
per
MAX_DELAYcycle; the only behavior change is immunity towall-clock jumps.
Why this surface specifically
9 importers × the cost of any redundant
runtime work cascade across startup and per-item dispatch. F1
benefits cold import; F2 benefits per-item runtime.
🤖 Generated with Claude Code