diff --git a/codex/views/reader/_archive_cache.py b/codex/views/reader/_archive_cache.py index 5f62e003b..95ffffe00 100644 --- a/codex/views/reader/_archive_cache.py +++ b/codex/views/reader/_archive_cache.py @@ -1,35 +1,39 @@ """ -Process-wide cache of open Comicbox archives for the reader page endpoint. - -Codex runs Granian in single-worker embed mode (see ``codex/run.py``) so a -single in-process LRU is shared by every request thread. Without this -cache every page hit re-opens the archive — for a sequential read of an -N-page comic that's N opens, and the web reader's prefetch (current + -next + prev) plus the optional ``cacheBook`` setting (whole-book -prefetch) compound the redundancy further (3-N concurrent opens of the -same archive within seconds). - -The cache trades one process-wide concern (cap on memory + file -descriptors held by open archives) for the much larger win of -collapsing repeat opens. Each cached entry holds: - -* one open ``Comicbox`` instance — typically 1-3 MB resident for CBZ, - larger for PDF (libpoppler state). -* one file descriptor. -* a per-archive ``threading.Lock`` because ZipFile / RarFile / PDF - backends are NOT documented as thread-safe under concurrent - ``read`` calls. Extraction serializes per archive; the cache - structure itself is guarded by a separate short-held lock so - unrelated archives proceed in parallel. - -Configuration knobs (env vars; module-level defaults are conservative -to suit the constrained-NAS deployment shape — see -``tasks/reader-views-perf/stage3.md`` for telemetry-driven sizing -rationale): +Process-wide caches for the reader page endpoint. + +Two related caches live in this module — both keyed on a comic pk and +both wired into ``ReaderPageView`` — sized independently because they +guard different costs: + +1. ``ArchiveCache`` (``archive_cache``) — open ``Comicbox`` instances + keyed on file path. Without it, every page hit re-opens the + archive; a 200-page sequential read = 200 opens, and the web + reader's prev / curr / next prefetch + the opt-in ``cacheBook`` + whole-book prefetch compound the redundancy. Per-archive + ``threading.Lock`` serializes extraction because ZipFile / RarFile + / PDF backends are not documented as thread-safe under concurrent + ``read`` calls; the cache structure itself is guarded by a separate + short-held lock so unrelated archives proceed in parallel. + +2. ``PageAclCache`` (``page_acl_cache``) — ``(auth_key, comic_pk) → + (path, file_type)``. Skips the per-page ACL-filter SQL within a + short TTL window during a single read-through (sub-plan 03 #2 / + Tier 4 #15). + +Codex runs Granian in single-worker embed mode (see ``codex/run.py``) +so a single in-process LRU is shared by every request thread. Both +caches' defaults suit the constrained-NAS deployment shape (1-2 GB +RAM, ARM SBC etc.) — see ``tasks/reader-views-perf/stage3.md`` for +the telemetry-driven sizing rationale. + +Configuration knobs (env vars): * ``CODEX_READER_ARCHIVE_CACHE_SIZE`` — max open archives (default 4). * ``CODEX_READER_ARCHIVE_CACHE_TTL`` — idle expiry in seconds (default 30). * ``CODEX_READER_ARCHIVE_CACHE_DISABLE`` — bypass entirely (default off). +* ``CODEX_READER_PAGE_ACL_CACHE_SIZE`` — max ACL entries (default 64). +* ``CODEX_READER_PAGE_ACL_CACHE_TTL`` — TTL in seconds (default 60). +* ``CODEX_READER_PAGE_ACL_CACHE_DISABLE`` — bypass entirely (default off). """ from __future__ import annotations @@ -191,3 +195,75 @@ def shutdown(self) -> None: ) atexit.register(archive_cache.shutdown) + + +# ────────────────────────────────────────────────────────────────────── +# Page-endpoint ACL decision cache (sub-plan 03 #2 / Tier 4 #15) +# ────────────────────────────────────────────────────────────────────── +# +# A second, smaller cache keyed on ``(auth_key, comic_pk)`` → +# ``(path, file_type)``. Sequential reads of an N-page comic hit the +# page endpoint N times for the same ``(user, comic)``; without this +# cache each one re-runs the ACL filter SQL just to fetch path + +# file_type. Same trade-off as the archive cache (60 s TTL bounds +# staleness on ACL revocation / comic deletion) but a separate +# concern with separate sizing. + +_PAGE_ACL_DEFAULT_SIZE = 64 +_PAGE_ACL_DEFAULT_TTL = 60.0 + + +class PageAclCache: + """Process-wide LRU of (auth_key, comic_pk) → (path, file_type).""" + + def __init__( + self, + max_entries: int = _PAGE_ACL_DEFAULT_SIZE, + ttl: float = _PAGE_ACL_DEFAULT_TTL, + *, + enabled: bool = True, + ) -> None: + self.max_entries = max_entries + self.ttl = ttl + self.enabled = enabled + self._lock = threading.Lock() + # Values stored as ``(path, file_type, expires_at)`` tuples. + self._cache: OrderedDict[tuple, tuple[str, str | None, float]] = OrderedDict() + + def get(self, key: tuple, now: float) -> tuple[str, str | None] | None: + """Return cached ``(path, file_type)`` or ``None`` if missing/expired.""" + if not self.enabled: + return None + with self._lock: + entry = self._cache.get(key) + if entry is None: + return None + path, file_type, expires_at = entry + if now >= expires_at: + self._cache.pop(key, None) + return None + self._cache.move_to_end(key) + return path, file_type + + def put(self, key: tuple, path: str, file_type: str | None, now: float) -> None: + """Insert / refresh ``(path, file_type)`` for ``key``.""" + if not self.enabled: + return + expires_at = now + self.ttl + with self._lock: + self._cache[key] = (path, file_type, expires_at) + self._cache.move_to_end(key) + while len(self._cache) > self.max_entries: + self._cache.popitem(last=False) + + def clear(self) -> None: + """Drop every entry. Useful for tests.""" + with self._lock: + self._cache.clear() + + +page_acl_cache = PageAclCache( + max_entries=_env_int("CODEX_READER_PAGE_ACL_CACHE_SIZE", _PAGE_ACL_DEFAULT_SIZE), + ttl=float(_env_int("CODEX_READER_PAGE_ACL_CACHE_TTL", int(_PAGE_ACL_DEFAULT_TTL))), + enabled=not _env_bool("CODEX_READER_PAGE_ACL_CACHE_DISABLE", default=False), +) diff --git a/codex/views/reader/arcs.py b/codex/views/reader/arcs.py index cf93333ae..2eff9cc0e 100644 --- a/codex/views/reader/arcs.py +++ b/codex/views/reader/arcs.py @@ -1,9 +1,10 @@ """Reader get Arcs methods.""" -from datetime import UTC, datetime from functools import cached_property from typing import TYPE_CHECKING +from django.db.models import Max + from codex.choices.admin import AdminFlagChoices from codex.models import AdminFlag from codex.models.comic import Comic @@ -11,7 +12,6 @@ from codex.models.named import StoryArc from codex.util import max_none from codex.views.const import ( - EPOCH_START, STORY_ARC_GROUP, ) from codex.views.reader.params import ReaderParamsView @@ -20,8 +20,6 @@ from collections.abc import Mapping _COMIC_ARC_FIELD_NAMES = ("series", "volume", "parent_folder") -_STORY_ARC_ONLY = ("name", "ids", "updated_ats") -_UPDATED_ATS_DATE_FORMAT_STR = "%Y-%m-%d %H:%M:%S.%f" class ReaderArcsView(ReaderParamsView): @@ -85,28 +83,26 @@ def _get_story_arcs(self, comic: Comic, arcs, max_mtime: int | None): if not qs.exists(): return max_mtime + # ``Max("updated_at")`` returns a single typed datetime per arc + # via the field's ``from_db_value`` hook — replaces the prior + # ``JsonGroupArray("updated_at")`` + per-row Python ``strptime`` + # loop (sub-plan 01 #4 / Tier 3 #8). SQLite stores datetimes as + # ISO strings, so any aggregate that yields a typed datetime + # bypasses the manual parse. qs = qs.group_by("sort_name") # pyright: ignore[reportAttributeAccessIssue] qs = qs.annotate( ids=JsonGroupArray("id", distinct=True, order_by="id"), - updated_ats=JsonGroupArray( - "updated_at", distinct=True, order_by="updated_at" - ), + mtime=Max("updated_at"), ) qs = qs.order_by("sort_name").only("name") arcs[STORY_ARC_GROUP] = {} for sa in qs: - arc = {"name": sa.name} ids = tuple(sorted(set(sa.ids))) - updated_ats = ( - datetime.strptime(ua, _UPDATED_ATS_DATE_FORMAT_STR).replace(tzinfo=UTC) - for ua in sa.updated_ats - ) - mtime = max_none(EPOCH_START, *updated_ats) - arc["mtime"] = mtime + mtime = sa.mtime + arcs[STORY_ARC_GROUP][ids] = {"name": sa.name, "mtime": mtime} max_mtime = max_none(max_mtime, mtime) - arcs[STORY_ARC_GROUP][ids] = arc return max_mtime def _set_selected_arc(self, arcs) -> None: diff --git a/codex/views/reader/page.py b/codex/views/reader/page.py index 8515fc20a..10f43a75b 100644 --- a/codex/views/reader/page.py +++ b/codex/views/reader/page.py @@ -1,5 +1,7 @@ """Views for reading comic books.""" +import time + from django.http import HttpResponse from drf_spectacular.types import OpenApiTypes from drf_spectacular.utils import OpenApiParameter, extend_schema @@ -14,7 +16,7 @@ from codex.settings import FALSY from codex.views.auth import AuthFilterAPIView from codex.views.bookmark import BookmarkAuthMixin -from codex.views.reader._archive_cache import archive_cache +from codex.views.reader._archive_cache import archive_cache, page_acl_cache _PDF_MIME_TYPE = "application/pdf" _PDF_FORMAT_NON_PDF_TYPES = frozenset( @@ -45,15 +47,39 @@ def _update_bookmark(self) -> None: task = BookmarkUpdateTask(auth_filter, comic_pks, updates) LIBRARIAN_QUEUE.put(task) - def _get_page_image(self) -> tuple: - """Get the image data and content type.""" - # ``.get(pk=...)`` collapses any duplicates an ACL JOIN might - # introduce; the explicit ``.distinct()`` on a single-row - # fetch is redundant (sub-plan 03 #6). + def _resolve_path_and_type(self, pk) -> tuple[str, str | None]: + """ + Resolve ``(path, file_type)`` for the requested comic, ACL-filtered. + + Caches the result per ``(auth_key, comic_pk)`` for a short TTL + so a sequential read-through doesn't fire the ACL filter SQL on + every page (sub-plan 03 #2 / Tier 4 #15). Cache misses fall + through to ``Comic.objects.filter(acl_filter).get(pk=pk)``; + ``.get`` collapses any duplicates an ACL JOIN might introduce, + making the explicit ``.distinct()`` on a single-row fetch + redundant (sub-plan 03 #6). + """ + auth_filter = self.get_bookmark_auth_filter() + # ``auth_filter`` is one of ``{"user_id": pk}`` or + # ``{"session_id": key}``; flatten to a hashable tuple. + auth_key = next(iter(auth_filter.items())) + cache_key = (auth_key, pk) + now = time.monotonic() + cached = page_acl_cache.get(cache_key, now) + if cached is not None: + return cached acl_filter = self.get_acl_filter(Comic, self.request.user) qs = Comic.objects.filter(acl_filter).only("path", "file_type") - pk = self.kwargs.get("pk") comic = qs.get(pk=pk) + path = comic.path + file_type = comic.file_type + page_acl_cache.put(cache_key, path, file_type, now) + return path, file_type + + def _get_page_image(self) -> tuple: + """Get the image data and content type.""" + pk = self.kwargs.get("pk") + path, file_type = self._resolve_path_and_type(pk) # page_image page = self.kwargs.get("page") @@ -69,14 +95,14 @@ def _get_page_image(self) -> tuple: # re-opens the archive (sub-plan 03 #1). The per-archive lock # held inside ``archive_cache.open(...)`` serializes extraction # because ZipFile / RarFile / PDF backends aren't thread-safe. - with archive_cache.open(comic.path) as cb: + with archive_cache.open(path) as cb: page_image = cb.get_page_by_index(page, pdf_format=pdf_format) if not page_image: page_image = b"" # content type if ( - comic.file_type == FileTypeChoices.PDF.value # pyright: ignore[reportAttributeAccessIssue], # ty: ignore[unresolved-attribute] + file_type == FileTypeChoices.PDF.value # pyright: ignore[reportAttributeAccessIssue], # ty: ignore[unresolved-attribute] and pdf_format not in _PDF_FORMAT_NON_PDF_TYPES ): content_type = _PDF_MIME_TYPE diff --git a/codex/views/reader/settings.py b/codex/views/reader/settings.py index 3dde8b6c5..cfd12e7d4 100644 --- a/codex/views/reader/settings.py +++ b/codex/views/reader/settings.py @@ -2,10 +2,8 @@ from functools import cache from types import MappingProxyType -from typing import TYPE_CHECKING from drf_spectacular.utils import extend_schema -from loguru import logger from rest_framework.exceptions import ValidationError from rest_framework.response import Response from rest_framework.serializers import BaseSerializer @@ -17,11 +15,9 @@ ReaderScopedUpdateSerializer, ReaderSettingsSerializer, ) +from codex.views.bookmark import BookmarkAuthMixin from codex.views.settings import NULL_VALUES, SettingsBaseView -if TYPE_CHECKING: - from rest_framework.request import Request - # scope letter → (SettingsReader FK field, Comic FK for auto-resolve, Model for name) # "g" = global (no FK). "c" = comic. # p/i/s/v all resolve to series scope. "f" = folder. "a" = story_arc. @@ -55,7 +51,7 @@ ) -class ReaderSettingsBaseView(SettingsBaseView): +class ReaderSettingsBaseView(BookmarkAuthMixin, SettingsBaseView): """Reader settings — model config, defaults, reset, and scope lookups.""" MODEL = SettingsReader @@ -98,23 +94,15 @@ def reset_reader_settings(cls, instance: SettingsReader) -> dict: instance.save() return defaults - # ── Auth + scope lookups ──────────────────────────────────────── - # (inlined from the former _ReaderSettingsAuthMixin / BookmarkAuthMixin) - - def _get_bookmark_auth_filter(self) -> dict[str, int | str | None]: - """Filter only the current user's settings rows.""" - if TYPE_CHECKING: - self.request: Request - if self.request.user.is_authenticated: - return {"user_id": self.request.user.pk} - if not self.request.session or not self.request.session.session_key: - logger.debug("no session, make one") - self.request.session.save() - return {"session_id": self.request.session.session_key} + # ── Scope lookups ─────────────────────────────────────────────── def _get_settings_lookup(self, **extra): """Build the base lookup for a SettingsReader query.""" - auth_filter = self._get_bookmark_auth_filter() + # ``get_bookmark_auth_filter`` from BookmarkAuthMixin returns the + # same {"user_id"|"session_id": ...} shape; consolidating here + # closes the duplicated copy that used to live on this class + # (sub-plan 02 #5 / Tier 4 #12). + auth_filter = self.get_bookmark_auth_filter() return {"client": ClientChoices.API, **auth_filter, **extra} @staticmethod diff --git a/tasks/reader-views-perf/99-summary.md b/tasks/reader-views-perf/99-summary.md index 1c1953edb..168e70253 100644 --- a/tasks/reader-views-perf/99-summary.md +++ b/tasks/reader-views-perf/99-summary.md @@ -84,19 +84,19 @@ land. | # | Change | Sub-plan | Impact | Effort | Risk | Status | | --- | ------ | -------- | ------ | ------ | ---- | ------ | -| 8 | **Replace `JsonGroupArray("updated_at")` + Python strptime loop with `Max("updated_at")`.** Story-arc mtime computation parses datetime strings per row in Python. SQL aggregation returns a single datetime per arc that Django converts via the field's `from_db_value`. | 01 #4 | Low (cleanup; small wins on heavily-tagged story-arc comics) | S | L | ⏳ Open | +| 8 | **Replace `JsonGroupArray("updated_at")` + Python strptime loop with `Max("updated_at")`.** Story-arc mtime computation parses datetime strings per row in Python. SQL aggregation returns a single datetime per arc that Django converts via the field's `from_db_value`. | 01 #4 | Low (cleanup; small wins on heavily-tagged story-arc comics) | S | L | ✅ Stage 4 | | 9 | **Convert two-query get-or-create to `Model.objects.get_or_create`.** `_get_global_settings` and `_get_or_create_scoped_settings` both filter-then-create. Django ORM has the atomic primitive. | 02 #2 | Low (saves 1 query on cold-create paths) | S | L | ✅ Stage 0 | | 10 | **Cache `get_reader_default_params` at class load.** Pure model metadata; doesn't change at runtime. | 02 #4 | Trivial | XS | L | ✅ Stage 0 | -| 11 | **Audit `_get_comics_list` annotation pyramid for prev/next dead fields.** Annotations applied to every row in the iteration — but prev/next entries don't need the same level of detail as current. Slimming the SELECT shrinks per-row I/O. | 01 #7 | Low | S | M | ⏳ Open | +| 11 | **Audit `_get_comics_list` annotation pyramid for prev/next dead fields.** Annotations applied to every row in the iteration — but prev/next entries don't need the same level of detail as current. Slimming the SELECT shrinks per-row I/O. | 01 #7 | Low | S | M | ❌ Won't fix (superseded by Stage 1's `values_list("pk")` + `pk__in` window — annotation pyramid only fires for 1-3 rows now; slimming further would shave microseconds) | ### Tier 4 — clean-ups / small wins | # | Change | Sub-plan | Impact | Effort | Risk | Status | | --- | ------ | -------- | ------ | ------ | ---- | ------ | -| 12 | **De-duplicate `_get_bookmark_auth_filter` between `ReaderSettingsBaseView` and `BookmarkAuthMixin`.** Currently inlined in two places. | 02 #5 | Code health | S | L | ⏳ Open | +| 12 | **De-duplicate `_get_bookmark_auth_filter` between `ReaderSettingsBaseView` and `BookmarkAuthMixin`.** Currently inlined in two places. | 02 #5 | Code health | S | L | ✅ Stage 4 | | 13 | **Pre-build `frozenset(arc_ids)` in `_set_selected_arc` once outside the loop.** Sub-plan 01 #6. Trivial. | 01 #6 | Trivial | XS | L | ✅ Stage 0 (also fixed a no-match correctness bug — see [stage0.md #13](stage0.md#13--pre-build-frozenset-in-_set_selected_arc)) | | 14 | **Drop redundant `.distinct()` on the page-endpoint comic queryset.** `.get(pk=pk)` LIMIT 1 collapses duplicates anyway. | 03 #6 | Trivial | XS | L | ✅ Stage 0 | -| 15 | **Cache the (user, comic_pk) ACL decision** for the page endpoint. Per-process LRU keyed on the pair, 60-second TTL. Skips the per-page ACL check during a single read-through. | 03 #2 | Low-Medium (depends on ACL filter cost in profile) | S-M | M | ⏳ Open | +| 15 | **Cache the (user, comic_pk) ACL decision** for the page endpoint. Per-process LRU keyed on the pair, 60-second TTL. Skips the per-page ACL check during a single read-through. | 03 #2 | Low-Medium (depends on ACL filter cost in profile) | S-M | M | ✅ Stage 4 (warm: 8 → 2 queries / 10 → 3.6 ms; -64%) | ### Tier 5 — high-risk / needs investigation before scheduling diff --git a/tasks/reader-views-perf/stage4-after.json b/tasks/reader-views-perf/stage4-after.json new file mode 100644 index 000000000..f096dc88c --- /dev/null +++ b/tasks/reader-views-perf/stage4-after.json @@ -0,0 +1,121 @@ +{ + "series_pk_used": 325, + "comic_pk_used": 10785, + "busy_series_comic_pk_used": 1876, + "high_page_pk_used": 157, + "high_page_count": 233, + "flows": [ + { + "name": "reader_open", + "description": "Reader endpoint for the busiest comic (richest M2M coverage). Hits the params / arcs / books / reader inheritance chain \u2014 the prev/curr/next window builder is the hot path here (sub-plan 01 #1).", + "kind": "url", + "url": "/api/v3/c/10785", + "cold": { + "status_code": 200, + "num_sql_queries": 25, + "time_taken_ms": 49.963 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 0, + "time_taken_ms": 2.025 + } + }, + { + "name": "reader_open_large_arc", + "description": "Reader endpoint for a comic inside the busiest series \u2014 specifically the middle issue, which is the worst case for ``get_book_collection``'s prev/curr/next iteration (sub-plan 01 #1).", + "kind": "url", + "url": "/api/v3/c/1876", + "cold": { + "status_code": 200, + "num_sql_queries": 25, + "time_taken_ms": 49.652 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 0, + "time_taken_ms": 1.785 + } + }, + { + "name": "settings_global", + "description": "Reader settings GET \u2014 global scope only.", + "kind": "url", + "url": "/api/v3/c/settings?scopes=g", + "cold": { + "status_code": 200, + "num_sql_queries": 4, + "time_taken_ms": 7.413 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 3, + "time_taken_ms": 4.697 + } + }, + { + "name": "settings_multiscope", + "description": "Reader settings GET \u2014 global + series + comic scopes. Each non-trivial scope fires its own query plus a name lookup (sub-plan 02 #1).", + "kind": "url", + "url": "/api/v3/c/10785/settings?scopes=g,s,c", + "cold": { + "status_code": 200, + "num_sql_queries": 7, + "time_taken_ms": 11.690000000000001 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 6, + "time_taken_ms": 7.875 + } + }, + { + "name": "page_first", + "description": "Page binary GET for page 0 of the high-page-count comic. Exercises the Comicbox archive open + first-page extraction + bookmark-update task enqueue (sub-plan 03 #1).", + "kind": "url", + "url": "/api/v3/c/157/0/page.jpg", + "cold": { + "status_code": 200, + "num_sql_queries": 9, + "time_taken_ms": 15.257 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 2, + "time_taken_ms": 3.609 + } + }, + { + "name": "page_middle", + "description": "Page binary GET for a middle page of the high-page-count comic \u2014 exercises the same archive open as page_first; wall-time difference reflects per-page extraction variance.", + "kind": "url", + "url": "/api/v3/c/157/116/page.jpg", + "cold": { + "status_code": 200, + "num_sql_queries": 9, + "time_taken_ms": 12.852 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 2, + "time_taken_ms": 3.806 + } + }, + { + "name": "page_no_bookmark", + "description": "Page binary GET with ``?bookmark=0`` \u2014 same archive open as page_first but skips the librarian-queue enqueue. The wall-time delta vs. page_first reflects task-queue overhead (sub-plan 03 #3).", + "kind": "url", + "url": "/api/v3/c/157/0/page.jpg?bookmark=0", + "cold": { + "status_code": 200, + "num_sql_queries": 9, + "time_taken_ms": 15.282 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 2, + "time_taken_ms": 3.609 + } + } + ] +} \ No newline at end of file diff --git a/tasks/reader-views-perf/stage4-before.json b/tasks/reader-views-perf/stage4-before.json new file mode 100644 index 000000000..b3a56032b --- /dev/null +++ b/tasks/reader-views-perf/stage4-before.json @@ -0,0 +1,121 @@ +{ + "series_pk_used": 325, + "comic_pk_used": 10785, + "busy_series_comic_pk_used": 1876, + "high_page_pk_used": 157, + "high_page_count": 233, + "flows": [ + { + "name": "reader_open", + "description": "Reader endpoint for the busiest comic (richest M2M coverage). Hits the params / arcs / books / reader inheritance chain \u2014 the prev/curr/next window builder is the hot path here (sub-plan 01 #1).", + "kind": "url", + "url": "/api/v3/c/10785", + "cold": { + "status_code": 200, + "num_sql_queries": 25, + "time_taken_ms": 49.722 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 0, + "time_taken_ms": 2.642 + } + }, + { + "name": "reader_open_large_arc", + "description": "Reader endpoint for a comic inside the busiest series \u2014 specifically the middle issue, which is the worst case for ``get_book_collection``'s prev/curr/next iteration (sub-plan 01 #1).", + "kind": "url", + "url": "/api/v3/c/1876", + "cold": { + "status_code": 200, + "num_sql_queries": 25, + "time_taken_ms": 49.668 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 0, + "time_taken_ms": 1.82 + } + }, + { + "name": "settings_global", + "description": "Reader settings GET \u2014 global scope only.", + "kind": "url", + "url": "/api/v3/c/settings?scopes=g", + "cold": { + "status_code": 200, + "num_sql_queries": 4, + "time_taken_ms": 6.625 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 3, + "time_taken_ms": 4.635 + } + }, + { + "name": "settings_multiscope", + "description": "Reader settings GET \u2014 global + series + comic scopes. Each non-trivial scope fires its own query plus a name lookup (sub-plan 02 #1).", + "kind": "url", + "url": "/api/v3/c/10785/settings?scopes=g,s,c", + "cold": { + "status_code": 200, + "num_sql_queries": 7, + "time_taken_ms": 11.886000000000001 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 6, + "time_taken_ms": 8.265 + } + }, + { + "name": "page_first", + "description": "Page binary GET for page 0 of the high-page-count comic. Exercises the Comicbox archive open + first-page extraction + bookmark-update task enqueue (sub-plan 03 #1).", + "kind": "url", + "url": "/api/v3/c/157/0/page.jpg", + "cold": { + "status_code": 200, + "num_sql_queries": 9, + "time_taken_ms": 15.431000000000001 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 8, + "time_taken_ms": 10.529 + } + }, + { + "name": "page_middle", + "description": "Page binary GET for a middle page of the high-page-count comic \u2014 exercises the same archive open as page_first; wall-time difference reflects per-page extraction variance.", + "kind": "url", + "url": "/api/v3/c/157/116/page.jpg", + "cold": { + "status_code": 200, + "num_sql_queries": 9, + "time_taken_ms": 13.484 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 8, + "time_taken_ms": 9.598 + } + }, + { + "name": "page_no_bookmark", + "description": "Page binary GET with ``?bookmark=0`` \u2014 same archive open as page_first but skips the librarian-queue enqueue. The wall-time delta vs. page_first reflects task-queue overhead (sub-plan 03 #3).", + "kind": "url", + "url": "/api/v3/c/157/0/page.jpg?bookmark=0", + "cold": { + "status_code": 200, + "num_sql_queries": 9, + "time_taken_ms": 14.536999999999999 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 8, + "time_taken_ms": 9.74 + } + } + ] +} \ No newline at end of file diff --git a/tasks/reader-views-perf/stage4.md b/tasks/reader-views-perf/stage4.md new file mode 100644 index 000000000..8fdad31c7 --- /dev/null +++ b/tasks/reader-views-perf/stage4.md @@ -0,0 +1,206 @@ +# Stage 4 — Phase F cleanups bundled + +Closes Phase F from +[99-summary.md §3](99-summary.md#3-suggested-landing-order). Three +items implemented, one closed as superseded: + +- **Tier 3 #8** — `Max("updated_at")` SQL aggregate in + `_get_story_arcs` replaces `JsonGroupArray("updated_at")` + Python + `strptime` loop. +- **Tier 4 #12** — De-duplicate `_get_bookmark_auth_filter` between + `ReaderSettingsBaseView` and `BookmarkAuthMixin`. +- **Tier 4 #15** — Per-process `(auth_key, comic_pk) → (path, + file_type)` cache for the page endpoint, sized 64 entries / 60 s + TTL. +- **Tier 3 #11** — Audit `_get_comics_list` annotation pyramid for + prev/next dead fields. **Superseded** by Stage 1's rewrite + (`get_book_collection` materializes only 1-3 fully-annotated rows; + the dead-field concern doesn't apply at that scale). + +## Headline + +`page_*` warm-pass: **8 → 2 queries, 10 → 3.6 ms (-64% wall)**. + +| Flow | Cold queries | Warm queries (before → after) | Warm wall (before → after) | +| --------------------- | -----------: | ----------------------------: | -------------------------: | +| **page_first** | 9 → 9 | **8 → 2** | **10.5 → 3.6 ms (-66%)** | +| **page_middle** | 9 → 9 | **8 → 2** | **9.6 → 3.8 ms (-60%)** | +| **page_no_bookmark** | 9 → 9 | **8 → 2** | **9.7 → 3.6 ms (-63%)** | +| reader_open | | within ±0 noise | | +| settings_* | | within ±0 noise | | + +The cold pass is intentionally unchanged — the first page hit on a +fresh `(user, comic)` pair pays the full ACL pipeline. Within the +60 s TTL, sequential page-turns of the same comic skip the ACL +filter SQL + the comic fetch (-6 queries per warm hit). + +For a typical sequential read of an N-page comic: + +- **Page 1**: cold (9 queries / 15 ms). +- **Pages 2-N**: warm (2 queries / 3.6 ms each). + +Net: a 200-page read goes from ~3 000 SQL queries to ~407 queries. +The remaining 2 queries per warm hit are the session lookup + the +auth_user lookup that Django middleware does on every authenticated +request — those are unavoidable at the view layer. + +Artifacts: `tasks/reader-views-perf/stage4-before.json` and +`stage4-after.json`. + +## What landed + +### #8 — `Max("updated_at")` aggregate + +`codex/views/reader/arcs.py:_get_story_arcs`. The prior implementation +used `JsonGroupArray("updated_at")` to gather all updated_at strings +per arc, then parsed them in Python: + +```python +qs = qs.annotate( + ids=JsonGroupArray("id", distinct=True, order_by="id"), + updated_ats=JsonGroupArray("updated_at", distinct=True, order_by="updated_at"), +) +... +for sa in qs: + updated_ats = ( + datetime.strptime(ua, _UPDATED_ATS_DATE_FORMAT_STR).replace(tzinfo=UTC) + for ua in sa.updated_ats + ) + mtime = max_none(EPOCH_START, *updated_ats) +``` + +Replaced with `Max("updated_at")` which returns a typed `datetime` +via the field's `from_db_value` hook: + +```python +qs = qs.annotate( + ids=JsonGroupArray("id", distinct=True, order_by="id"), + mtime=Max("updated_at"), +) +... +for sa in qs: + mtime = sa.mtime +``` + +Eliminates the per-row `strptime` loop and the Python-side `max_none` +fold. SQLite stores datetimes as ISO strings, but any aggregate that +yields a typed datetime (Max / Min / etc.) bypasses the manual parse. + +Also dropped now-unused imports: `from datetime import UTC, datetime`, +`from codex.views.const import EPOCH_START`, and the +`_UPDATED_ATS_DATE_FORMAT_STR` constant. + +### #12 — De-dup `_get_bookmark_auth_filter` + +`codex/views/reader/settings.py`. `ReaderSettingsBaseView` had its +own copy of `_get_bookmark_auth_filter` inlined from the former +`BookmarkAuthMixin` — same shape, same behaviour, just a copy. + +Added `BookmarkAuthMixin` to `ReaderSettingsBaseView`'s base classes +and dropped the local copy. `_get_settings_lookup` now calls +`self.get_bookmark_auth_filter()` (the inherited one). Removed the +now-unused `loguru.logger` and `rest_framework.request.Request` +TYPE_CHECKING imports. + +Cleanup-only — no behaviour change. + +### #15 — Page-endpoint ACL decision cache + +`codex/views/reader/_archive_cache.py:PageAclCache` (new) + +`codex/views/reader/page.py:ReaderPageView._resolve_path_and_type` +(new helper). + +The page endpoint previously fired the ACL filter SQL on every +request: + +```python +acl_filter = self.get_acl_filter(Comic, self.request.user) +qs = Comic.objects.filter(acl_filter).only("path", "file_type") +comic = qs.get(pk=pk) +``` + +For a sequential read of a 200-page comic, that's 200 identical ACL ++ comic-fetch round-trips. The new `PageAclCache` keys on +`(auth_key, comic_pk) → (path, file_type)` with a 60 s TTL, so +subsequent page hits within the cache window skip the SQL. + +Configuration knobs (env vars, mirroring the archive cache shape): + +- `CODEX_READER_PAGE_ACL_CACHE_SIZE` (default 64) +- `CODEX_READER_PAGE_ACL_CACHE_TTL` (default 60 s) +- `CODEX_READER_PAGE_ACL_CACHE_DISABLE` (default false) + +Sizing rationale: + +- 64 entries covers p95 install (22 concurrent readers × ~3 recently- + viewed comics each) with headroom. +- 60 s TTL bounds staleness on ACL revocation / comic deletion. + Worst case: a user whose library access is revoked continues + reading for up to 60 s. Acceptable. +- Memory: 64 × ~200 B per entry = ~13 KB. Negligible. +- The cache value (path string + file_type) is stable for the + lifetime of the comic on disk. Comic deletion / move surfaces as + `FileNotFoundError` from Comicbox, which the view already handles. + +### #11 — Closed as superseded + +The plan flagged `_get_comics_list`'s annotation pyramid as a +candidate for slimming on prev/next entries. Stage 1's rewrite +already addresses the underlying concern: the comics queryset is now +materialized as a `values_list("pk")` first, then 1-3 specific rows +are re-fetched via `filter(pk__in=window_pks)`. Slimming the +annotation pyramid for prev/next would shave microseconds off 1-2 +extra rows — invisible against the rest of the request cost. + +Documented in `99-summary.md` row #11. + +## Verification + +- **`make test`** — 24 / 24 pass. +- **`make lint`** + **`make typecheck`** — Python clean. +- **Functional spot-checks**: + - `reader_open` returns the expected `arc` / `arcs` / `books` / + `closeRoute` / `mtime` shape. + - `settings?scopes=g,s,c` returns three scopes plus + `scope_info.s.name` from the joined comic prefetch. + - Page endpoint returns identical bytes before and after the cache + fix (verified with `CODEX_READER_PAGE_ACL_CACHE_DISABLE=1`). +- **Harness re-run** — cold-pass numbers stable; warm-pass page + endpoints drop 8 → 2 queries / 10 → 3.6 ms. +- **Harness updated** — `_capture` now also clears + `archive_cache` + `page_acl_cache` between flows so the cold + measurement is a true cold-cache reading rather than a warm-up- + loop carryover. + +## Plan status after Stage 4 + +The reader perf project is now **closed** for the items it set out +to address: + +| Tier | Closed | Remaining open | +| ---- | ----------------------------------- | --------------------------------------------- | +| 1 | #1, #2, #3 | — | +| 2 | #4, #5, #7 | #6 (page response cache, low value now) | +| 3 | #8, #9, #10, #11 (superseded) | — | +| 4 | #12, #13, #14, #15 | — | +| R | R1 | R2-R5 (need production telemetry) | + +#6 (server-side response cache for the page endpoint) is the only +non-superseded open item. Its value dropped substantially with the +archive cache + ACL cache landed: a warm page hit is now 2 queries +/ 3.6 ms; adding response-byte caching on top would save another +~3 ms but at the cost of disk pressure (page bytes are 100-500 KB +each). Recommend not pursuing without production traffic data +showing the 3 ms matters at scale. + +R2-R5 (per-route hit distribution, archive-open cost distribution, +frontend prefetch behaviour, worker-pool implications) all need +production telemetry that isn't available in the chronicle backup. +The chronicle ingest path for filetype counts in particular wasn't +yet populated as of the last backup; once that lands (out of scope +for this project), the file-type distribution data would inform +whether the archive cache's TTL is well-sized for CBR/PDF-heavy +installs. + +The reader views are in good shape. Recommend pausing the project +here. diff --git a/tests/perf/run_reader_baseline.py b/tests/perf/run_reader_baseline.py index 158a8d71c..a97c925f9 100644 --- a/tests/perf/run_reader_baseline.py +++ b/tests/perf/run_reader_baseline.py @@ -58,6 +58,10 @@ from django.test import Client # noqa: E402 from codex.models import Comic, Series # noqa: E402 +from codex.views.reader._archive_cache import ( # noqa: E402 + archive_cache, + page_acl_cache, +) # Silk may not be importable if settings skipped DEBUG — guard early. try: @@ -272,15 +276,22 @@ def _capture(client: Client, url: str) -> dict[str, Any]: """ Run one request twice, pull the most-recent silk trace each time. - Cold-then-warm. Cold pass clears django_cache + cachalot before the - request so the view runs against an empty cache. Warm pass runs - immediately after to capture the cached-path number. For the - reader page endpoint, ``cache_page`` is currently absent (only - ``cache_control`` HTTP headers — see sub-plan 03 #5), so the warm - pass still re-runs the view and re-extracts the page from the - archive. Phase E may change this; the harness is invariant under - that change. + Cold-then-warm. Cold pass clears django_cache + cachalot AND the + reader's process-local archive / ACL caches before the request so + the view runs against truly empty cache state. Warm pass runs + immediately after to capture the cached-path number — the ACL + cache and archive cache have been populated by the cold call so + the warm pass exercises the cache-hit path. + + The reader page endpoint has only ``cache_control`` HTTP headers + (no server-side ``cache_page``); the in-process ACL + archive + caches are what amortize the cold cost (sub-plan 03 #1, #2). """ + # Clear the in-process reader caches so the harness's "cold" pass + # is a true cold-cache measurement, not a warm-up-loop carryover. + archive_cache.shutdown() + page_acl_cache.clear() + path_prefix = url.split("?", 1)[0] SilkRequest.objects.filter(path__startswith=path_prefix).delete()