From b7ee9caa7b960b2b1811f598625ba1eebf88005a Mon Sep 17 00:00:00 2001 From: Jason Little Date: Sat, 29 Jul 2023 17:13:48 -0500 Subject: [PATCH 1/3] Add metrics tracking for eviction based on time to ResponseCache --- synapse/util/caches/response_cache.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/synapse/util/caches/response_cache.py b/synapse/util/caches/response_cache.py index 340e5e914533..40f583930981 100644 --- a/synapse/util/caches/response_cache.py +++ b/synapse/util/caches/response_cache.py @@ -36,7 +36,7 @@ ) from synapse.util import Clock from synapse.util.async_helpers import AbstractObservableDeferred, ObservableDeferred -from synapse.util.caches import register_cache +from synapse.util.caches import EvictionReason, register_cache logger = logging.getLogger(__name__) @@ -167,7 +167,7 @@ def on_complete(r: RV) -> RV: # the should_cache bit, we leave it in the cache for now and schedule # its removal later. if self.timeout_sec and context.should_cache: - self.clock.call_later(self.timeout_sec, self.unset, key) + self.clock.call_later(self.timeout_sec, self.entry_timeout, key) else: # otherwise, remove the result immediately. self.unset(key) @@ -187,6 +187,16 @@ def unset(self, key: KV) -> None: """ self._result_cache.pop(key, None) + def evict_because(self, key: KV, reason: EvictionReason) -> None: + """Basically the same as unset, but update reason why evicting for metrics""" + self._metrics.inc_evictions(reason) + self.unset(key) + + def entry_timeout(self, key: KV) -> None: + """For the call_later to remove from the cache""" + logger.debug(f"Expiring from [{self._name}] - {key}") + self.evict_because(key, EvictionReason.time) + async def wrap( self, key: KV, From 61ccf6cc1dfdf55a746716117b41abd9a36a9c30 Mon Sep 17 00:00:00 2001 From: Jason Little Date: Sat, 29 Jul 2023 18:03:17 -0500 Subject: [PATCH 2/3] changelog --- changelog.d/16028.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/16028.misc diff --git a/changelog.d/16028.misc b/changelog.d/16028.misc new file mode 100644 index 000000000000..3a1e9fef0979 --- /dev/null +++ b/changelog.d/16028.misc @@ -0,0 +1 @@ +Collect additional metrics from `ResponseCache` for eviction. From 59563ddddad7f1bb4c59cf321e4334e496f20236 Mon Sep 17 00:00:00 2001 From: Jason Little Date: Tue, 1 Aug 2023 03:55:21 -0500 Subject: [PATCH 3/3] Address review feedback 1. Change entry_timeout to _entry_timeout 2. Inline popping the cache item instead of using unset 3. Inline evict_because into _entry_timeout 4. Add EvictionReason.invalidate metrics counting to unset, so it is counted too --- synapse/util/caches/response_cache.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/synapse/util/caches/response_cache.py b/synapse/util/caches/response_cache.py index 40f583930981..0cb46700a9ab 100644 --- a/synapse/util/caches/response_cache.py +++ b/synapse/util/caches/response_cache.py @@ -167,7 +167,7 @@ def on_complete(r: RV) -> RV: # the should_cache bit, we leave it in the cache for now and schedule # its removal later. if self.timeout_sec and context.should_cache: - self.clock.call_later(self.timeout_sec, self.entry_timeout, key) + self.clock.call_later(self.timeout_sec, self._entry_timeout, key) else: # otherwise, remove the result immediately. self.unset(key) @@ -185,17 +185,13 @@ def unset(self, key: KV) -> None: Args: key: key used to remove the cached value """ + self._metrics.inc_evictions(EvictionReason.invalidation) self._result_cache.pop(key, None) - def evict_because(self, key: KV, reason: EvictionReason) -> None: - """Basically the same as unset, but update reason why evicting for metrics""" - self._metrics.inc_evictions(reason) - self.unset(key) - - def entry_timeout(self, key: KV) -> None: + def _entry_timeout(self, key: KV) -> None: """For the call_later to remove from the cache""" - logger.debug(f"Expiring from [{self._name}] - {key}") - self.evict_because(key, EvictionReason.time) + self._metrics.inc_evictions(EvictionReason.time) + self._result_cache.pop(key, None) async def wrap( self,