From 266b03ac0db8e1b8f8dd479e789c7e31f5427138 Mon Sep 17 00:00:00 2001 From: Henri Hulski Date: Wed, 30 Sep 2026 18:39:38 +0200 Subject: [PATCH] refactor: switch to functools.lru_cache --- pyproject.toml | 2 +- reg/cache.py | 48 +++------ reg/tests/test_cache.py | 195 +++++++++++++++++++++++++++++++++++++ reg/tests/test_registry.py | 60 +++++++++--- 4 files changed, 253 insertions(+), 52 deletions(-) create mode 100644 reg/tests/test_cache.py diff --git a/pyproject.toml b/pyproject.toml index 84300d3..be923d1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -26,7 +26,6 @@ classifiers = [ "Development Status :: 5 - Production/Stable", ] requires-python = ">= 3.10" -dependencies = ["repoze.lru"] [project.urls] Documentation = "https://reg.readthedocs.io" @@ -84,6 +83,7 @@ python_version = "3.10" strict = true files = ["."] warn_unreachable = true +warn_unused_configs = true warn_unused_ignores = true [tool.pyright] diff --git a/reg/cache.py b/reg/cache.py index a89c108..cd3e313 100644 --- a/reg/cache.py +++ b/reg/cache.py @@ -1,9 +1,8 @@ from __future__ import annotations +from functools import lru_cache from typing import TYPE_CHECKING, Any, Generic -from repoze.lru import lru_cache # type: ignore[import-untyped] - if TYPE_CHECKING: from collections.abc import Callable, Sequence from typing_extensions import TypeVar @@ -49,27 +48,13 @@ class DictCachingKeyLookup(Generic[_ValueT]): def __init__(self, key_lookup: KeyLookup[_ValueT]) -> None: self.key_lookup = key_lookup - self.component = Cache(key_lookup.component).__getitem__ # type: ignore - self.fallback = Cache(key_lookup.fallback).__getitem__ # type: ignore + self.component = Cache(key_lookup.component).__getitem__ + self.fallback = Cache(key_lookup.fallback).__getitem__ def _all(key: Sequence[Any]) -> list[_ValueT]: return list(key_lookup.all(key)) - self.all = Cache(_all).__getitem__ # type: ignore - - if TYPE_CHECKING: - # NOTE: For pyright's sake we declare these callable instance attributes - # as methods, even though they're not, since pyright does not seem - # to be able to match protocols against them. mypy can deal with - # it just fine - def component(self, key: Sequence[Any], /) -> _ValueT | None: - raise NotImplementedError - - def fallback(self, key: Sequence[Any], /) -> _ValueT | None: - raise NotImplementedError - - def all(self, key: Sequence[Any], /) -> list[_ValueT]: - raise NotImplementedError + self.all = Cache(_all).__getitem__ class LruCachingKeyLookup(Generic[_ValueT]): @@ -100,20 +85,11 @@ def __init__( fallback_cache_size: int, ) -> None: self.key_lookup = key_lookup - self.component = lru_cache(component_cache_size)(key_lookup.component) # type: ignore - self.fallback = lru_cache(fallback_cache_size)(key_lookup.fallback) # type: ignore - self.all = lru_cache(all_cache_size)(lambda key: list(key_lookup.all(key))) # type: ignore - - if TYPE_CHECKING: - # NOTE: For pyright's sake we declare these callable instance attributes - # as methods, even though they're not, since pyright does not seem - # to be able to match protocols against them. mypy can deal with - # it just fine - def component(self, key: Sequence[Any], /) -> _ValueT | None: - raise NotImplementedError - - def fallback(self, key: Sequence[Any], /) -> _ValueT | None: - raise NotImplementedError - - def all(self, key: Sequence[Any], /) -> list[_ValueT]: - raise NotImplementedError + self.component = lru_cache(component_cache_size)(key_lookup.component) + self.fallback = lru_cache(fallback_cache_size)(key_lookup.fallback) + + @lru_cache(all_cache_size) + def _all(key: Sequence[Any]) -> list[_ValueT]: + return list(key_lookup.all(key)) + + self.all = _all diff --git a/reg/tests/test_cache.py b/reg/tests/test_cache.py new file mode 100644 index 0000000..1272e09 --- /dev/null +++ b/reg/tests/test_cache.py @@ -0,0 +1,195 @@ +"""Tests for the caching key lookups in reg.cache. + +These tests treat the caches as black boxes: a fake key lookup records every +call it receives, so we can assert how often the underlying functions were +really invoked. Nothing here depends on the cache implementation, only on +caching behavior. + +The fake returns ``str`` values, so the lookups under test are typed as +``LruCachingKeyLookup[str]`` / ``DictCachingKeyLookup[str]``. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Any + +from reg.cache import DictCachingKeyLookup, LruCachingKeyLookup + +if TYPE_CHECKING: + from collections.abc import Iterator, Sequence + + +class FakeKeyLookup: + """Records every call so tests can see what reached the underlying lookup.""" + + def __init__(self) -> None: + self.calls: list[tuple[str, Sequence[Any]]] = [] + + def component(self, key: Sequence[Any]) -> str | None: + self.calls.append(("component", key)) + return f"component:{key}" + + def fallback(self, key: Sequence[Any]) -> str | None: + self.calls.append(("fallback", key)) + return None # None results must be cached too + + def all(self, key: Sequence[Any]) -> Iterator[str]: + self.calls.append(("all", key)) + return iter([f"all:{key}"]) # an iterator; the cache must turn it into a list + + +def make( + component: int = 10, all: int = 10, fallback: int = 10 +) -> tuple[FakeKeyLookup, LruCachingKeyLookup[str]]: + kl = FakeKeyLookup() + return kl, LruCachingKeyLookup(kl, component, all, fallback) + + +# --- LruCachingKeyLookup: component --------------------------------------- + + +def test_component_result_is_returned() -> None: + _kl, cached = make() + assert cached.component(("a",)) == "component:('a',)" + + +def test_component_is_cached() -> None: + kl, cached = make() + cached.component(("a",)) + cached.component(("a",)) + assert kl.calls == [("component", ("a",))] + + +def test_component_different_keys_are_cached_separately() -> None: + kl, cached = make() + cached.component(("a",)) + cached.component(("b",)) + cached.component(("a",)) + cached.component(("b",)) + assert kl.calls == [("component", ("a",)), ("component", ("b",))] + + +# --- LruCachingKeyLookup: fallback ---------------------------------------- + + +def test_fallback_is_cached() -> None: + kl, cached = make() + cached.fallback(("a",)) + cached.fallback(("a",)) + assert kl.calls == [("fallback", ("a",))] + + +def test_none_result_is_cached() -> None: + kl, cached = make() + assert cached.fallback(("a",)) is None + assert cached.fallback(("a",)) is None + assert len(kl.calls) == 1 + + +# --- LruCachingKeyLookup: all --------------------------------------------- + + +def test_all_returns_a_list() -> None: + _kl, cached = make() + result = cached.all(("a",)) + assert isinstance(result, list) + assert result == ["all:('a',)"] + + +def test_all_is_cached() -> None: + kl, cached = make() + first = cached.all(("a",)) + second = cached.all(("a",)) + assert len(kl.calls) == 1 + assert first == second + + +# --- LruCachingKeyLookup: independence ------------------------------------ + + +def test_same_key_is_cached_per_method() -> None: + kl, cached = make() + cached.component(("a",)) + cached.fallback(("a",)) + cached.all(("a",)) + cached.component(("a",)) + cached.fallback(("a",)) + cached.all(("a",)) + assert kl.calls == [ + ("component", ("a",)), + ("fallback", ("a",)), + ("all", ("a",)), + ] + + +def test_cache_sizes_are_independent() -> None: + kl, cached = make(component=1, fallback=3) + cached.component(("a",)) + cached.component(("b",)) # evicts "a" from the component cache only + for k in ("a", "b", "c"): + cached.fallback((k,)) + kl.calls.clear() + + cached.component(("a",)) + for k in ("a", "b", "c"): + cached.fallback((k,)) + assert kl.calls == [("component", ("a",))] + + +# --- LruCachingKeyLookup: eviction ---------------------------------------- + + +def test_component_least_recently_used_is_evicted() -> None: + kl, cached = make(component=2) + cached.component(("a",)) + cached.component(("b",)) + cached.component(("c",)) # evicts "a" + kl.calls.clear() + + cached.component(("b",)) # still cached + cached.component(("c",)) # still cached + assert kl.calls == [] + + cached.component(("a",)) # was evicted, recomputed + assert kl.calls == [("component", ("a",))] + + +def test_component_recent_use_protects_from_eviction() -> None: + kl, cached = make(component=2) + cached.component(("a",)) + cached.component(("b",)) + cached.component(("a",)) # "b" is now the least recently used + cached.component(("c",)) # evicts "b" + kl.calls.clear() + + cached.component(("a",)) + assert kl.calls == [] + cached.component(("b",)) + assert kl.calls == [("component", ("b",))] + + +# --- DictCachingKeyLookup (unbounded) ------------------------------------- + + +def test_dict_component_is_cached() -> None: + kl = FakeKeyLookup() + cached: DictCachingKeyLookup[str] = DictCachingKeyLookup(kl) + assert cached.component(("a",)) == "component:('a',)" + cached.component(("a",)) + assert kl.calls == [("component", ("a",))] + + +def test_dict_fallback_caches_none() -> None: + kl = FakeKeyLookup() + cached: DictCachingKeyLookup[str] = DictCachingKeyLookup(kl) + assert cached.fallback(("a",)) is None + assert cached.fallback(("a",)) is None + assert kl.calls == [("fallback", ("a",))] + + +def test_dict_all_returns_list_and_is_cached() -> None: + kl = FakeKeyLookup() + cached: DictCachingKeyLookup[str] = DictCachingKeyLookup(kl) + assert cached.all(("a",)) == ["all:('a',)"] + cached.all(("a",)) + assert kl.calls == [("all", ("a",))] diff --git a/reg/tests/test_registry.py b/reg/tests/test_registry.py index 1e0e2ef..ba9fb0f 100644 --- a/reg/tests/test_registry.py +++ b/reg/tests/test_registry.py @@ -1,5 +1,6 @@ from __future__ import annotations +from collections import Counter from typing import TYPE_CHECKING, Any, ParamSpec, TypeVar import pytest @@ -14,8 +15,9 @@ from ..types import DispatchCall - _T = TypeVar("_T") - _P = ParamSpec("_P") + +_T = TypeVar("_T") +_P = ParamSpec("_P") def register_value( @@ -282,6 +284,29 @@ class Bar: assert view(FooSub(), Request("dummy", "GET")) == "Name fallback" +class SpyKeyLookup: + """Wraps a key lookup and counts the calls that reach it.""" + + def __init__(self, key_lookup: Any) -> None: + self.key_lookup = key_lookup + self.calls: Counter[tuple[str, Any]] = Counter() + + def component(self, key: Any) -> Any: + self.calls[("component", key)] += 1 + return self.key_lookup.component(key) + + def fallback(self, key: Any) -> Any: + self.calls[("fallback", key)] += 1 + return self.key_lookup.fallback(key) + + def all(self, key: Any) -> Any: + self.calls[("all", key)] += 1 + return self.key_lookup.all(key) + + +spies: list[SpyKeyLookup] = [] + + def test_lru_caching_registry() -> None: class Foo: pass @@ -313,7 +338,9 @@ def request_method_fallback(self: Any, request: Request) -> str: return "Request method fallback" def get_caching_key_lookup(r: PredicateRegistry) -> LruCachingKeyLookup: - return LruCachingKeyLookup(r, 100, 100, 100) + spy = SpyKeyLookup(r) + spies.append(spy) + return LruCachingKeyLookup(spy, 100, 100, 100) @dispatch( match_instance("model", get_model, model_fallback), @@ -337,6 +364,9 @@ def foo_edit(self: Foo, request: Request) -> str: register_value(view, (Foo, "", "POST"), foo_post) register_value(view, (Foo, "edit", "POST"), foo_edit) + assert len(spies) == 1 + spy = spies[0] + assert view(Foo(), Request("", "GET")) == "foo default" assert view(FooSub(), Request("", "GET")) == "foo default" assert view(FooSub(), Request("edit", "POST")) == "foo edit" @@ -346,25 +376,25 @@ def foo_edit(self: Foo, request: Request) -> str: "GET", ) - # use a bit of inside knowledge to check the cache is filled - assert view.key_lookup.component.__closure__ is not None - component_cache = view.key_lookup.component.__closure__[0].cell_contents - assert component_cache.get(((Foo, "", "GET"),)) is not None - assert component_cache.get(((FooSub, "", "GET"),)) is not None - assert component_cache.get(((FooSub, "edit", "POST"),)) is not None + # each key has reached the real registry exactly once + assert spy.calls[("component", (Foo, "", "GET"))] == 1 + assert spy.calls[("component", (FooSub, "", "GET"))] == 1 + assert spy.calls[("component", (FooSub, "edit", "POST"))] == 1 # now let's do this again. this time things come from the component cache + before = spy.calls.copy() assert view(Foo(), Request("", "GET")) == "foo default" assert view(FooSub(), Request("", "GET")) == "foo default" assert view(FooSub(), Request("edit", "POST")) == "foo edit" + assert spy.calls == before # nothing new reached the registry - assert view.key_lookup.all.__closure__ is not None - all_cache = view.key_lookup.all.__closure__[0].cell_contents # prime and check the all cache assert view.by_args(Foo(), Request("", "GET")).all_matches == [foo_default] - assert all_cache.get(((Foo, "", "GET"),)) is not None + assert spy.calls[("all", (Foo, "", "GET"))] == 1 # should be coming from cache now + before = spy.calls.copy() assert view.by_args(Foo(), Request("", "GET")).all_matches == [foo_default] + assert spy.calls == before class Bar: pass @@ -375,12 +405,12 @@ class Bar: assert view(FooSub(), Request("dummy", "GET")) == "Name fallback" # fallbacks get cached too - assert view.key_lookup.fallback.__closure__ is not None - fallback_cache = view.key_lookup.fallback.__closure__[0].cell_contents - assert fallback_cache.get(((Bar, "", "GET"),)) is model_fallback + assert spy.calls[("fallback", (Bar, "", "GET"))] == 1 # these come from the fallback cache now + before = spy.calls.copy() assert view(Bar(), Request("", "GET")) == "Model fallback" assert view(Foo(), Request("dummy", "GET")) == "Name fallback" assert view(Foo(), Request("", "PUT")) == "Request method fallback" assert view(FooSub(), Request("dummy", "GET")) == "Name fallback" + assert spy.calls == before