From a430341a931196a9d7cf2cf424ad8b82dc2f2a96 Mon Sep 17 00:00:00 2001 From: splitsec2 <35583321+splitsec2@users.noreply.github.com> Date: Thu, 22 Oct 2026 18:24:57 -0600 Subject: [PATCH] perf: cut repeated work on search and cover paths (#1544) I went through what Shelfmark does per search and per cover and measured the pieces that showed up. Four of them are cheap to fix, so this is one PR. Shelfmark runs on a single gevent worker, so any of these blocks every other request while it runs, and I'm assuming a lot of people run it on small hardware (a Pi, say). Cover cache. Every cache hit rewrote the whole `cache_index.json` just to bump the entry's last-access time. On my box that is 3.7 ms of CPU per hit at 500 entries and about 100 ms at 15,000 (2.2 MB written each time), and a results page asks for around 40 covers. The access time now stays in memory and the index is written at most every 30 seconds, or whenever something else saves it. A crash inside that window loses some access times, which only changes eviction order. Settings reload. `config.refresh()` rebuilt the cache by reading and parsing a tab's JSON file once per field, so about 250 parses per refresh, 100 ms of CPU, and the metadata search, providers and config routes each trigger it. It now reads each file once per rebuild (several tabs share `settings.json`, so the memo is keyed on the file path). Library match. When the library check is on, each search result is compared against every library entry, and the title was split into words again for every entry. The words are now computed once per result. I measured 33 ms per result at 5,000 library entries and 106 ms at 20,000, which is a second or more per page of 40 results. The matching itself is unchanged. Cover URL probe. `is_covers_cache_enabled()` creates and deletes a file in the config directory to check it is writable, and it runs for every cover, queue task and history row. The answer is remembered for 60 seconds. The probe is 0.045 ms on its own, but it is called a few hundred times per request in the worst case. Each change has a test that fails without it (call counts on the index saves, file opens during a refresh, tokenisation calls, and probe calls). Ruff, basedpyright on the changed files and the non-e2e suite pass. The timings are from a dev machine with an empty config, so the real numbers depend on cache and library size. --- shelfmark/config/env.py | 27 +++++++++++++++-- shelfmark/core/config.py | 7 +++-- shelfmark/core/image_cache.py | 13 ++++++-- shelfmark/core/library_index.py | 6 ++-- shelfmark/core/settings_registry.py | 34 +++++++++++++++++++++ shelfmark/core/text_match.py | 17 ++++++++++- tests/config/test_docker_volumes.py | 17 +++++++++++ tests/conftest.py | 10 +++++++ tests/core/test_image_cache.py | 37 +++++++++++++++++++++++ tests/core/test_library_index.py | 18 +++++++++++ tests/core/test_settings_reload.py | 46 +++++++++++++++++++++++++++++ tests/core/test_text_match.py | 8 +++++ 12 files changed, 230 insertions(+), 10 deletions(-) create mode 100644 tests/core/test_settings_reload.py diff --git a/shelfmark/config/env.py b/shelfmark/config/env.py index b01a31d4..df88e55d 100644 --- a/shelfmark/config/env.py +++ b/shelfmark/config/env.py @@ -4,6 +4,7 @@ import json import os import shutil import tempfile +import time from pathlib import Path LOG_LEVELS = ("DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL") @@ -118,14 +119,36 @@ def _is_config_dir_writable() -> bool: return True +# The probe above creates and deletes a file, and cover URLs are rewritten per result, +# per queue task and per history row, so it is remembered for a short time. +_WRITABLE_PROBE_TTL_SECONDS = 60.0 +_writable_probe: tuple[float, bool] | None = None + + +def _config_dir_writable_cached() -> bool: + global _writable_probe + now = time.monotonic() + if _writable_probe is not None and now - _writable_probe[0] < _WRITABLE_PROBE_TTL_SECONDS: + return _writable_probe[1] + writable = _is_config_dir_writable() + _writable_probe = (now, writable) + return writable + + +def reset_config_dir_writable_cache() -> None: + """Forget the remembered probe (tests change the directory under it).""" + global _writable_probe + _writable_probe = None + + def is_covers_cache_enabled() -> bool: """Check if cover caching is enabled (requires setting + writable config dir).""" from shelfmark.core.config import config setting_enabled = config.get("COVERS_CACHE_ENABLED", True) if isinstance(setting_enabled, str): - return string_to_bool(setting_enabled) and _is_config_dir_writable() - return bool(setting_enabled) and _is_config_dir_writable() + return string_to_bool(setting_enabled) and _config_dir_writable_cached() + return bool(setting_enabled) and _config_dir_writable_cached() # ============================================================================= diff --git a/shelfmark/core/config.py b/shelfmark/core/config.py index 7b4c4104..5aa960df 100644 --- a/shelfmark/core/config.py +++ b/shelfmark/core/config.py @@ -124,9 +124,10 @@ class Config: self._field_map.clear() self._cache.clear() - for key, (field, tab_name) in registry.get_settings_field_map().items(): - self._field_map[key] = (field, tab_name) - self._cache[key] = registry.get_setting_value(field, tab_name) + with registry.config_files_read_once(): + for key, (field, tab_name) in registry.get_settings_field_map().items(): + self._field_map[key] = (field, tab_name) + self._cache[key] = registry.get_setting_value(field, tab_name) self._loaded = True diff --git a/shelfmark/core/image_cache.py b/shelfmark/core/image_cache.py index 77af1a66..1245e1ee 100644 --- a/shelfmark/core/image_cache.py +++ b/shelfmark/core/image_cache.py @@ -47,6 +47,11 @@ NEGATIVE_CACHE_TTL = 3600 # Short enough to retry soon, long enough to prevent spam during one page view TRANSIENT_CACHE_TTL = 60 +# A hit only moves the entry's last-access time, which orders LRU eviction. The whole +# index is rewritten to disk, so that is done at most this often instead of per hit. +# A crash inside the window loses some access times, nothing else. +ACCESS_FLUSH_SECONDS = 30 + _MIN_WEBP_HEADER_LENGTH = 12 HTTP_NOT_FOUND = HTTPStatus.NOT_FOUND @@ -102,6 +107,7 @@ class ImageCacheService: self.index_path = cache_dir / "cache_index.json" self._lock = threading.RLock() self._index: dict[str, dict[str, Any]] = {} + self._last_saved = time.monotonic() # Stats tracking self._hits = 0 @@ -195,6 +201,7 @@ class ImageCacheService: with temp_path.open("w") as f: json.dump(self._index, f) temp_path.rename(self.index_path) + self._last_saved = time.monotonic() except OSError: pass @@ -325,9 +332,11 @@ class ImageCacheService: with image_path.open("rb") as f: data = f.read() - # Update accessed time + # Update accessed time. Written out with the next save, or here once + # enough time has passed, not once per hit. entry["accessed_at"] = time.time() - self._save_index() + if time.monotonic() - self._last_saved >= ACCESS_FLUSH_SECONDS: + self._save_index() result = data, content_type except OSError: diff --git a/shelfmark/core/library_index.py b/shelfmark/core/library_index.py index 3e91a980..970ee9ab 100644 --- a/shelfmark/core/library_index.py +++ b/shelfmark/core/library_index.py @@ -25,7 +25,8 @@ from shelfmark.core.text_match import ( author_surname, extra_work_tokens, isbn_variants, - title_tokens_match, + significant_tokens, + significant_tokens_match, ) if TYPE_CHECKING: @@ -128,8 +129,9 @@ def match_entries(book: BookMetadata, entries: list[LibraryEntry]) -> str | None title = book.search_title or book.title surname = author_surname(book.search_author or (book.authors[0] if book.authors else "")) collection: str | None = None + title_toks = significant_tokens(title) for entry in entries: - if not title_tokens_match(title, set(entry.tokens)): + if not significant_tokens_match(title_toks, entry.tokens): continue if surname is not None and surname not in entry.tokens: continue diff --git a/shelfmark/core/settings_registry.py b/shelfmark/core/settings_registry.py index 2081d809..6ea6d605 100644 --- a/shelfmark/core/settings_registry.py +++ b/shelfmark/core/settings_registry.py @@ -2,6 +2,8 @@ import json import os +from contextlib import contextmanager +from contextvars import ContextVar from dataclasses import dataclass, field from pathlib import Path from threading import Lock @@ -419,8 +421,40 @@ def _ensure_config_dir(tab_name: str) -> None: config_path.parent.mkdir(parents=True, exist_ok=True) +# Set while the whole settings cache is being rebuilt. Every field of a tab used to +# re-read and re-parse that tab's file, so a rebuild parsed each file once per field. +_config_file_memo: ContextVar[dict[str, dict[str, Any]] | None] = ContextVar( + "config_file_memo", default=None +) + + +@contextmanager +def config_files_read_once() -> Iterator[None]: + """Read each tab's config file at most once inside the block. + + Only for a read-only pass such as rebuilding the settings cache; a save inside the + block would not be seen by reads after it. + """ + token = _config_file_memo.set({}) + try: + yield + finally: + _config_file_memo.reset(token) + + def load_config_file(tab_name: str) -> dict[str, Any]: """Load a settings tab config file, returning an empty dict on failure.""" + memo = _config_file_memo.get() + if memo is None: + return _read_config_file(tab_name) + config_path = _get_config_file_path(tab_name) + key = str(config_path) # several tabs share settings.json + if key not in memo: + memo[key] = _read_config_file(tab_name) + return memo[key] + + +def _read_config_file(tab_name: str) -> dict[str, Any]: config_path = _get_config_file_path(tab_name) if not config_path.exists(): diff --git a/shelfmark/core/text_match.py b/shelfmark/core/text_match.py index 4c66fa07..375c36bb 100644 --- a/shelfmark/core/text_match.py +++ b/shelfmark/core/text_match.py @@ -7,6 +7,10 @@ library matches titles, authors and ISBNs the same way. from __future__ import annotations import re +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from collections.abc import Collection DEFAULT_TITLE_MATCH_THRESHOLD = 0.85 @@ -57,7 +61,18 @@ def title_tokens_match( threshold: float = DEFAULT_TITLE_MATCH_THRESHOLD, ) -> bool: """True when enough significant title tokens appear in ``haystack_tokens``.""" - title_toks = significant_tokens(title) + return significant_tokens_match(significant_tokens(title), haystack_tokens, threshold) + + +def significant_tokens_match( + title_toks: list[str], + haystack_tokens: Collection[str], + threshold: float = DEFAULT_TITLE_MATCH_THRESHOLD, +) -> bool: + """``title_tokens_match`` for a title already run through ``significant_tokens``. + + Matching one title against many entries should tokenise it once, not per entry. + """ if not title_toks: return False present = sum(1 for tok in title_toks if tok in haystack_tokens) diff --git a/tests/config/test_docker_volumes.py b/tests/config/test_docker_volumes.py index dd96148a..31154fd8 100644 --- a/tests/config/test_docker_volumes.py +++ b/tests/config/test_docker_volumes.py @@ -104,3 +104,20 @@ def test_save_config_file_returns_false_when_config_dir_is_not_writable(tmp_path assert save_config_file("downloads", {"key": "value"}) is False finally: config_dir.chmod(stat.S_IRWXU) + + +def test_covers_cache_probe_is_remembered_for_a_short_time(monkeypatch): + from shelfmark.config import env + + probes: list[int] = [] + monkeypatch.setattr(env, "_is_config_dir_writable", lambda: (probes.append(1), True)[1]) + clock = [1000.0] + monkeypatch.setattr(env.time, "monotonic", lambda: clock[0]) + + for _ in range(5): + assert env._config_dir_writable_cached() is True + assert probes == [1] + + clock[0] += env._WRITABLE_PROBE_TTL_SECONDS + 1 + assert env._config_dir_writable_cached() is True + assert probes == [1, 1] diff --git a/tests/conftest.py b/tests/conftest.py index de13e702..60cc8952 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -29,6 +29,16 @@ sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) import pytest +@pytest.fixture(autouse=True) +def _reset_config_dir_writable_cache(): + """The writable-config-dir probe is remembered; each test starts without it.""" + from shelfmark.config.env import reset_config_dir_writable_cache + + reset_config_dir_writable_cache() + yield + reset_config_dir_writable_cache() + + @pytest.fixture(autouse=True) def _clear_torrent_fetch_cache(): """Keep the shared torrent fetch cache from leaking between tests.""" diff --git a/tests/core/test_image_cache.py b/tests/core/test_image_cache.py index e0a3fd98..7bd4e087 100644 --- a/tests/core/test_image_cache.py +++ b/tests/core/test_image_cache.py @@ -84,3 +84,40 @@ def test_cover_cache_key_depends_on_the_url_only() -> None: assert key != cover_cache_key("https://example.com/cover.jpg?size=large") assert key.isalnum() and key == key.lower() assert "/" not in key and "." not in key + + +def test_cache_hits_do_not_rewrite_the_index_each_time(tmp_path, monkeypatch) -> None: + """A hit only moves the last-access time, so the index is flushed on a timer.""" + cache = ImageCacheService(tmp_path) + assert cache.put("cover-a", b"\xff\xd8\xff" + b"0" * 64, "image/jpeg") + saves: list[int] = [] + real_save = cache._save_index + monkeypatch.setattr(cache, "_save_index", lambda: (saves.append(1), real_save())[1]) + + before = cache._index["cover-a"]["accessed_at"] + for _ in range(5): + assert cache.get("cover-a") is not None + + assert saves == [] + assert cache._index["cover-a"]["accessed_at"] >= before + + +def test_cache_hit_flushes_the_index_once_the_window_has_passed(tmp_path, monkeypatch) -> None: + from shelfmark.core import image_cache + + cache = ImageCacheService(tmp_path) + assert cache.put("cover-a", b"\xff\xd8\xff" + b"0" * 64, "image/jpeg") + saves: list[int] = [] + real_save = cache._save_index + monkeypatch.setattr(cache, "_save_index", lambda: (saves.append(1), real_save())[1]) + + now = cache._last_saved + monkeypatch.setattr( + image_cache.time, "monotonic", lambda: now + image_cache.ACCESS_FLUSH_SECONDS + ) + assert cache.get("cover-a") is not None + assert saves == [1] + + # The flush restarts the window. + assert cache.get("cover-a") is not None + assert saves == [1] diff --git a/tests/core/test_library_index.py b/tests/core/test_library_index.py index 08d55d91..1197a484 100644 --- a/tests/core/test_library_index.py +++ b/tests/core/test_library_index.py @@ -466,3 +466,21 @@ def test_an_exact_shelf_title_outranks_a_collection_holding_the_same_book() -> N book = _book(title="Dungeon Crawler Carl", authors=["Matt Dinniman"]) assert library_index.match_entries(book, [collection, single]) == "owned" + + +def test_matching_tokenises_the_title_once_for_all_entries(monkeypatch: pytest.MonkeyPatch) -> None: + """The title does not change between entries, so it is split into words once.""" + from shelfmark.core import text_match + + calls: list[str | None] = [] + real = text_match.significant_tokens + + def counting(text: str | None) -> list[str]: + calls.append(text) + return real(text) + + monkeypatch.setattr(text_match, "significant_tokens", counting) + entries = [_entry("unrelated", f"word{i}", "someone") for i in range(50)] + + assert library_index.match_entries(_book(), entries) is None + assert len(calls) <= 2 diff --git a/tests/core/test_settings_reload.py b/tests/core/test_settings_reload.py new file mode 100644 index 00000000..89b8d4cb --- /dev/null +++ b/tests/core/test_settings_reload.py @@ -0,0 +1,46 @@ +"""Rebuilding the settings cache parses each tab's config file once.""" + +from __future__ import annotations + +import json +from pathlib import Path + +from shelfmark.core import settings_registry +from shelfmark.core.config import config as app_config + + +def test_refresh_reads_each_config_file_once(tmp_path, monkeypatch): + plugins = tmp_path / "plugins" + plugins.mkdir() + (tmp_path / "settings.json").write_text(json.dumps({"SEARCH_MODE": "universal"})) + (plugins / "downloads.json").write_text(json.dumps({"MAX_CONCURRENT_DOWNLOADS": 3})) + (plugins / "hardcover.json").write_text(json.dumps({"HARDCOVER_ENABLED": False})) + + opened: dict[str, int] = {} + real_open = Path.open + + def counting_open(self, *args, **kwargs): + if self.suffix == ".json" and self.parent in (tmp_path, plugins): + opened[self.name] = opened.get(self.name, 0) + 1 + return real_open(self, *args, **kwargs) + + with monkeypatch.context() as patched: + patched.setattr(settings_registry, "_get_config_dir", lambda: tmp_path) + patched.setattr(Path, "open", counting_open) + app_config.refresh(force=True) + + app_config.refresh(force=True) + assert opened, "no config file was read" + assert max(opened.values()) == 1, opened + + +def test_config_files_are_read_fresh_outside_the_block(tmp_path, monkeypatch): + monkeypatch.setattr(settings_registry, "_get_config_dir", lambda: tmp_path) + (tmp_path / "settings.json").write_text(json.dumps({"A": 1})) + + with settings_registry.config_files_read_once(): + assert settings_registry.load_config_file("general") == {"A": 1} + (tmp_path / "settings.json").write_text(json.dumps({"A": 2})) + assert settings_registry.load_config_file("general") == {"A": 1} + + assert settings_registry.load_config_file("general") == {"A": 2} diff --git a/tests/core/test_text_match.py b/tests/core/test_text_match.py index cf2fcd34..559b218d 100644 --- a/tests/core/test_text_match.py +++ b/tests/core/test_text_match.py @@ -75,3 +75,11 @@ def test_title_tokens_match_without_significant_tokens_is_false(title): ) def test_normalize_isbn(value, expected): assert text_match.normalize_isbn(value) == expected + + +def test_significant_tokens_match_agrees_with_title_tokens_match(): + haystack = {"alpha", "beta", "gamma"} + for title in ("Alpha Beta", "alpha delta", "the of", ""): + assert text_match.significant_tokens_match( + text_match.significant_tokens(title), haystack + ) is text_match.title_tokens_match(title, haystack)