From ea18b8c3146f44096830044ada106898c59adfce Mon Sep 17 00:00:00 2001 From: AJ Slater Date: Thu, 23 Apr 2026 17:27:27 -0700 Subject: [PATCH] =?UTF-8?q?Browser=20views=20perf:=20Stage=202=20=E2=80=94?= =?UTF-8?q?=20triple=20COUNT=20+=20page=20mtime=20cache?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## PR 2a — Eliminate triple COUNT on the paginate path Each browse request ran three COUNT queries per section (groups & books): 1. The outer grouped COUNT in `_get_common_queryset`. 2. Paginator's internal COUNT triggered by `paginator.page()`. 3. An explicit `.count()` on the paginated slice. The outer COUNT is needed (sizing the paginator). The other two are redundant — the page row count is bounded by `per_page` and derivable from `end_index - start_index + 1`. - Shadow `Paginator.count` (a `@cached_property`) with the pre-computed total to skip Paginator's internal COUNT. - Derive page row count arithmetically from `Page.start_index()` / `end_index()`. - Pass `book_count` through `paginate()` alongside `group_count`; drop `book_qs.count()` on the opds2 path. - `_paginate_section` returns `(qs, count)` directly. Short-circuits on `total_count == 0` (avoids Paginator instantiation on empty sections) and preserves the EmptyPage warning branch. ## PR 2b — Short-TTL page mtime cache `BrowserView._get_page_mtime()` calls `get_group_mtime(page_mtime=True)` on every browse request. The query is a filtered Max aggregate that cachalot caches — but any write to Comic / Bookmark invalidates it, so bookmark-heavy usage forces recomputation. Cold-path silk traces show this aggregate at ~26ms on flow_a — the second-slowest query in the request. Add a 5s TTL cache layer gated on page_mtime=True. Key includes user, model, group, pks, page, and a hash of filter-affecting params (filters, search, q, order_by, order_reverse). The polling MtimeView path (no page_mtime) is unaffected, so frontend change-detection stays live. ## Measurements tests/perf/run_baseline.py on the slimlib DB. Cold = full cache invalidation; warm = cachalot populated. | flow | stage1 cold | stage2 cold | |-------------------------|------------------------|------------------------| | flow_a root browse | 18 queries / 182.3 ms | 16 queries / 135.6 ms | | flow_b filtered search | 17 queries / 178.0 ms | 15 queries / 130.3 ms | | flow_c series metadata | 31 queries / 226.9 ms | 31 queries / 229.9 ms | flow_a / flow_b: -2 queries, ~26% cold wall-time reduction. flow_c unaffected (metadata doesn't traverse paginate). PR 2b's benefit is a dogpile guard after cachalot invalidation — doesn't show in the harness (cold = both caches empty; warm = cachalot wins first). Co-Authored-By: Claude Opus 4.7 --- codex/views/browser/browser.py | 2 +- codex/views/browser/group_mtime.py | 41 ++++++++++++++- codex/views/browser/paginate.py | 77 +++++++++++++++++++--------- tasks/browser-views-perf/stage2.json | 50 ++++++++++++++++++ 4 files changed, 144 insertions(+), 26 deletions(-) create mode 100644 tasks/browser-views-perf/stage2.json diff --git a/codex/views/browser/browser.py b/codex/views/browser/browser.py index 7587972ba..412ba11a9 100644 --- a/codex/views/browser/browser.py +++ b/codex/views/browser/browser.py @@ -199,7 +199,7 @@ def _get_group_and_books(self) -> tuple: num_pages = ceil((group_count + book_count) / BROWSER_MAX_OBJ_PER_PAGE) self.check_page_in_bounds(num_pages) group_qs, book_qs, page_group_count, page_book_count = self.paginate( - group_qs, book_qs, group_count + group_qs, book_qs, group_count, book_count ) # Annotate diff --git a/codex/views/browser/group_mtime.py b/codex/views/browser/group_mtime.py index ecd42067e..2d5279764 100644 --- a/codex/views/browser/group_mtime.py +++ b/codex/views/browser/group_mtime.py @@ -1,7 +1,10 @@ """Group Mtime Function.""" +import hashlib +import json from typing import TYPE_CHECKING +from django.core.cache import cache as _django_cache from django.db.models.aggregates import Aggregate, Max from django.db.models.functions import Greatest from django.db.utils import OperationalError @@ -15,6 +18,9 @@ from django.db.models import Q, Value _FTS5_PREFIX = "fts5: " +_PAGE_MTIME_TTL_SECONDS = 5 +_PAGE_MTIME_CACHE_MISS = object() +_PAGE_MTIME_NONE_SENTINEL = "none" class BrowserGroupMtimeView(BrowserFilterView): @@ -70,8 +76,37 @@ def get_max_bookmark_updated_at_aggregate( self._bmua_agg_cache[key] = aggregate return aggregate + def _page_mtime_cache_key(self, model) -> str: + """Stable key scoped to user + filter-affecting params.""" + user_id = self.request.user.pk if self.request.user.is_authenticated else 0 + group = self.kwargs.get("group", "r") + pks = tuple(self.kwargs.get("pks") or (0,)) + page = self.kwargs.get("page", 1) + filter_keys = ("filters", "search", "q", "order_by", "order_reverse") + params_data = {k: self.params.get(k) for k in filter_keys} + params_str = json.dumps(params_data, sort_keys=True, default=str) + params_hash = hashlib.blake2s(params_str.encode(), digest_size=8).hexdigest() + return ( + f"codex:page_mtime:{user_id}:{model.__name__}:" + f"{group}:{pks}:{page}:{params_hash}" + ) + def get_group_mtime(self, model, group=None, pks=None, *, page_mtime=False): - """Get a filtered mtime for browser pages and mtime checker.""" + """ + Get a filtered mtime for browser pages and mtime checker. + + When ``page_mtime`` is true this is called from the browse page + response — the query is a filtered Max aggregate that runs even on + cachalot misses (writes to Comic/Bookmark invalidate it). A short + TTL cache absorbs concurrent recomputes within that window; real + staleness is bounded by TTL + cachalot's signal-driven invalidation. + """ + cache_key = self._page_mtime_cache_key(model) if page_mtime else "" + if cache_key: + cached = _django_cache.get(cache_key, _PAGE_MTIME_CACHE_MISS) + if cached is not _PAGE_MTIME_CACHE_MISS: + return None if cached == _PAGE_MTIME_NONE_SENTINEL else cached + qs = self.get_filtered_queryset( model, group=group, @@ -101,4 +136,8 @@ def get_group_mtime(self, model, group=None, pks=None, *, page_mtime=False): mtime = None if mtime == NotImplemented: mtime = None + + if cache_key: + stored = _PAGE_MTIME_NONE_SENTINEL if mtime is None else mtime + _django_cache.set(cache_key, stored, _PAGE_MTIME_TTL_SECONDS) return mtime diff --git a/codex/views/browser/paginate.py b/codex/views/browser/paginate.py index a1e0628ea..d847ecd34 100644 --- a/codex/views/browser/paginate.py +++ b/codex/views/browser/paginate.py @@ -13,27 +13,52 @@ class BrowserPaginateView(BrowserPageInBoundsView): """Paginate Groups and Books.""" - def _paginate_section(self, qs: QuerySet, page: int) -> QuerySet: - """Paginate a group or Comic section.""" + def _paginate_section( + self, qs: QuerySet, page: int, total_count: int + ) -> tuple[QuerySet, int]: + """ + Paginate a group or Comic section. + + ``total_count`` is the pre-computed section total (already run by + the caller via one grouped COUNT). It's stuffed into + ``Paginator._count`` so Paginator skips its own COUNT query, and + used to derive the page row count arithmetically instead of + issuing another COUNT on the sliced queryset. + """ + if not total_count: + return qs.model.objects.none(), 0 orphans = 0 if self.model_group == "f" or self.params.get("search") else 5 paginator = Paginator(qs, BROWSER_MAX_OBJ_PER_PAGE, orphans=orphans) + # Shadow the @cached_property with the pre-computed total so + # Paginator.num_pages doesn't issue its own COUNT query. + paginator.count = total_count try: paginator_page = paginator.page(page) qs = paginator_page.object_list + count = paginator_page.end_index() - paginator_page.start_index() + 1 except EmptyPage: if self.model_group != "f": model_name = qs.model.__name__ if qs.model else "UnknownGroup" logger.warning(f"No {model_name}s on page {page}") qs = qs.model.objects.none() + count = 0 - return qs + return qs, count - def _paginate_groups(self, group_qs: QuerySet): + def _paginate_groups( + self, group_qs: QuerySet, group_count: int + ) -> tuple[QuerySet, int]: """Paginate the group object list before books.""" page = self.kwargs.get("page", 1) - return self._paginate_section(group_qs, page) + return self._paginate_section(group_qs, page, group_count) - def _paginate_books(self, book_qs, total_group_count, page_group_count) -> QuerySet: + def _paginate_books( + self, + book_qs: QuerySet, + book_count: int, + total_group_count: int, + page_group_count: int, + ) -> tuple[QuerySet, int]: """Paginate the book object list based on how many group/folders are showing.""" group_remainder = total_group_count % BROWSER_MAX_OBJ_PER_PAGE num_books_on_mixed_page = BROWSER_MAX_OBJ_PER_PAGE - group_remainder @@ -41,32 +66,36 @@ def _paginate_books(self, book_qs, total_group_count, page_group_count) -> Query # There are books after the groups on the same page # Add remainder books without the paginator page_book_qs = book_qs[:num_books_on_mixed_page] - else: - # There are books after the groups on a new page - book_offset = 0 if not group_remainder else num_books_on_mixed_page - page_book_qs = book_qs[book_offset:] + page_book_count = min(num_books_on_mixed_page, book_count) + return page_book_qs, page_book_count + + # There are books after the groups on a new page + book_offset = 0 if not group_remainder else num_books_on_mixed_page + page_book_qs = book_qs[book_offset:] - # Which book page are we on after groups? - page = self.kwargs.get("page", 1) - num_group_and_mixed_pages = ceil( - total_group_count / BROWSER_MAX_OBJ_PER_PAGE - ) - book_only_page = page - num_group_and_mixed_pages + # Which book page are we on after groups? + page = self.kwargs.get("page", 1) + num_group_and_mixed_pages = ceil(total_group_count / BROWSER_MAX_OBJ_PER_PAGE) + book_only_page = page - num_group_and_mixed_pages - page_book_qs = self._paginate_section(page_book_qs, book_only_page) - return page_book_qs + remaining_book_count = max(0, book_count - book_offset) + return self._paginate_section(page_book_qs, book_only_page, remaining_book_count) def paginate( - self, group_qs: QuerySet, book_qs: QuerySet, group_count: int + self, + group_qs: QuerySet, + book_qs: QuerySet, + group_count: int, + book_count: int, ) -> tuple[QuerySet, QuerySet, int, int]: """Paginate the queryset into a group and book object lists.""" if self.TARGET == "opds2": - self._opds_number_of_books = book_qs.count() + self._opds_number_of_books = book_count self._opds_number_of_groups = group_count - page_group_qs = self._paginate_groups(group_qs) - page_group_count = page_group_qs.count() - page_book_qs = self._paginate_books(book_qs, group_count, page_group_count) - page_book_count = page_book_qs.count() + page_group_qs, page_group_count = self._paginate_groups(group_qs, group_count) + page_book_qs, page_book_count = self._paginate_books( + book_qs, book_count, group_count, page_group_count + ) return page_group_qs, page_book_qs, page_group_count, page_book_count diff --git a/tasks/browser-views-perf/stage2.json b/tasks/browser-views-perf/stage2.json new file mode 100644 index 000000000..74f290c5c --- /dev/null +++ b/tasks/browser-views-perf/stage2.json @@ -0,0 +1,50 @@ +{ + "series_pk_used": 325, + "flows": [ + { + "name": "flow_a_root_browse", + "description": "Root browse, no filters, no search.", + "url": "/api/v3/r/0/1", + "cold": { + "status_code": 200, + "num_sql_queries": 16, + "time_taken_ms": 135.547 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 0, + "time_taken_ms": 2.0349999999999997 + } + }, + { + "name": "flow_b_filtered_search", + "description": "Root browse with a search term.", + "url": "/api/v3/r/0/1?q=man", + "cold": { + "status_code": 200, + "num_sql_queries": 15, + "time_taken_ms": 130.28500000000003 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 0, + "time_taken_ms": 1.5939999999999999 + } + }, + { + "name": "flow_c_series_metadata", + "description": "Metadata detail for the largest series.", + "url": "/api/v3/s/325/metadata", + "cold": { + "status_code": 200, + "num_sql_queries": 31, + "time_taken_ms": 229.85 + }, + "warm": { + "status_code": 200, + "num_sql_queries": 0, + "time_taken_ms": 1.729 + } + } + ] +} \ No newline at end of file