From a4f062eb79490f70cf0b598719ba86157c85fdd9 Mon Sep 17 00:00:00 2001 From: "Kai A. Hiller" Date: Thu, 17 Oct 2019 19:34:07 +0200 Subject: [PATCH 1/7] Fix LruCache callback deduplication Signed-off-by: Kai A. Hiller --- changelog.d/6213.bugfix | 1 + synapse/util/caches/descriptors.py | 25 ++++++++++++++++--------- 2 files changed, 17 insertions(+), 9 deletions(-) create mode 100644 changelog.d/6213.bugfix diff --git a/changelog.d/6213.bugfix b/changelog.d/6213.bugfix new file mode 100644 index 000000000000..072264fba395 --- /dev/null +++ b/changelog.d/6213.bugfix @@ -0,0 +1 @@ +Fix LruCache callback deduplication. diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index 5ac2530a6a72..fcea2f68df93 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -17,7 +17,6 @@ import inspect import logging import threading -from collections import namedtuple from typing import Any, cast from six import itervalues @@ -625,14 +624,22 @@ def errback(f): return wrapped -class _CacheContext(namedtuple("_CacheContext", ("cache", "key"))): - # We rely on _CacheContext implementing __eq__ and __hash__ sensibly, - # which namedtuple does for us (i.e. two _CacheContext are the same if - # their caches and keys match). This is important in particular to - # dedupe when we add callbacks to lru cache nodes, otherwise the number - # of callbacks would grow. - def invalidate(self): - self.cache.invalidate(self.key) +class _CacheContext: + # We make sure identical _CacheContext objects share the same invalidate + # function. This is important in particular to dedupe when we add callbacks + # to lru cache nodes, otherwise the number of callbacks would grow. + _invalidate_funcs = {} + + def __init__(self, cache, cache_key): + key = (cache, cache_key) + + def invalidate(): + cache.invalidate(cache_key) + + if key not in self._invalidate_funcs: + self._invalidate_funcs[key] = invalidate + + self.invalidate = self._invalidate_funcs[key] def cached( From 2d8e7a83ff2c2e1df9d87317127939bcae88fac3 Mon Sep 17 00:00:00 2001 From: "Kai A. Hiller" Date: Thu, 17 Oct 2019 21:14:36 +0200 Subject: [PATCH 2/7] Make use of dict.setdefault() for better readability Signed-off-by: Kai A. Hiller --- synapse/util/caches/descriptors.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index fcea2f68df93..8fb82dd9c730 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -636,10 +636,7 @@ def __init__(self, cache, cache_key): def invalidate(): cache.invalidate(cache_key) - if key not in self._invalidate_funcs: - self._invalidate_funcs[key] = invalidate - - self.invalidate = self._invalidate_funcs[key] + self.invalidate = self._invalidate_funcs.setdefault(key, invalidate) def cached( From 87d139231e3cba33d0cd19feaa458bbc16ab3a09 Mon Sep 17 00:00:00 2001 From: "Kai A. Hiller" Date: Thu, 24 Oct 2019 19:03:50 +0200 Subject: [PATCH 3/7] Use WeakValueDictionary in _CacheContext to allow GC Signed-off-by: Kai A. Hiller --- synapse/util/caches/descriptors.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index 8fb82dd9c730..15aebef229e9 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -18,6 +18,7 @@ import logging import threading from typing import Any, cast +from weakref import WeakValueDictionary from six import itervalues @@ -628,7 +629,7 @@ class _CacheContext: # We make sure identical _CacheContext objects share the same invalidate # function. This is important in particular to dedupe when we add callbacks # to lru cache nodes, otherwise the number of callbacks would grow. - _invalidate_funcs = {} + _invalidate_funcs = WeakValueDictionary() def __init__(self, cache, cache_key): key = (cache, cache_key) From 8d7211d683d8ef527f5a43595d51b76ce3ec2fa2 Mon Sep 17 00:00:00 2001 From: "Kai A. Hiller" Date: Thu, 31 Oct 2019 20:05:04 +0100 Subject: [PATCH 4/7] Change _CacheContext to not create identical objects Signed-off-by: Kai A. Hiller --- synapse/util/caches/descriptors.py | 35 ++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index 15aebef229e9..bddadd62901a 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -430,7 +430,7 @@ def _wrapped(*args, **kwargs): # Add our own `cache_context` to argument list if the wrapped function # has asked for one if self.add_cache_context: - kwargs["cache_context"] = _CacheContext(cache, cache_key) + kwargs["cache_context"] = _CacheContext.get_instance(cache, cache_key) try: cached_result_d = cache.get(cache_key, callback=invalidate_callback) @@ -626,18 +626,35 @@ def errback(f): class _CacheContext: - # We make sure identical _CacheContext objects share the same invalidate - # function. This is important in particular to dedupe when we add callbacks - # to lru cache nodes, otherwise the number of callbacks would grow. - _invalidate_funcs = WeakValueDictionary() + """Holds cache information from the cached function higher in the calling order. + + Can be used to invalidate the higher level cache entry if something changes + on a lower level. + """ + + _cache_context_objects = WeakValueDictionary() def __init__(self, cache, cache_key): - key = (cache, cache_key) + self._cache = cache + self._cache_key = cache_key + + def invalidate(self): + """Invalidates the cache entry referred to by the context.""" + self._cache.invalidate(self._cache_key) + + @classmethod + def get_instance(cls, cache, cache_key): + """Returns an instance constructed with the given arguments. - def invalidate(): - cache.invalidate(cache_key) + A new instance is only created if none already exists. + """ - self.invalidate = self._invalidate_funcs.setdefault(key, invalidate) + # We make sure there are no identical _CacheContext instances. This is + # important in particular to dedupe when we add callbacks to lru cache + # nodes, otherwise the number of callbacks would grow. + return cls._cache_context_objects.setdefault( + (cache, cache_key), cls(cache, cache_key) + ) def cached( From 30ac43034f0c7b2a0086c5f9b74d576a2cd1c43f Mon Sep 17 00:00:00 2001 From: "Kai A. Hiller" Date: Wed, 6 Nov 2019 17:48:09 +0100 Subject: [PATCH 5/7] Add type hints to _CacheContext --- synapse/util/caches/descriptors.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index 8d4db70987ef..63e9c282fdf8 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -13,11 +13,13 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +from __future__ import annotations + import functools import inspect import logging import threading -from typing import Any, cast +from typing import Any, Tuple, Union, cast from weakref import WeakValueDictionary from six import itervalues @@ -38,6 +40,8 @@ logger = logging.getLogger(__name__) +CacheKey = Union[Tuple, Any] + class _CachedFunction(Protocol): invalidate = None # type: Any @@ -631,18 +635,20 @@ class _CacheContext: on a lower level. """ - _cache_context_objects = WeakValueDictionary() + _cache_context_objects: WeakValueDictionary[ + Tuple[Cache, CacheKey], _CacheContext + ] = WeakValueDictionary() - def __init__(self, cache, cache_key): + def __init__(self, cache: Cache, cache_key: CacheKey) -> None: self._cache = cache self._cache_key = cache_key - def invalidate(self): + def invalidate(self) -> None: """Invalidates the cache entry referred to by the context.""" self._cache.invalidate(self._cache_key) @classmethod - def get_instance(cls, cache, cache_key): + def get_instance(cls, cache: Cache, cache_key: CacheKey) -> _CacheContext: """Returns an instance constructed with the given arguments. A new instance is only created if none already exists. From 94be60f9d91095857db2319d5ad2fb5de77d1aba Mon Sep 17 00:00:00 2001 From: "Kai A. Hiller" Date: Wed, 6 Nov 2019 18:34:58 +0100 Subject: [PATCH 6/7] _CacheContext: Restore compatibility with lower Python versions --- synapse/util/caches/descriptors.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index 63e9c282fdf8..0254498f8043 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -13,8 +13,6 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -from __future__ import annotations - import functools import inspect import logging @@ -635,9 +633,8 @@ class _CacheContext: on a lower level. """ - _cache_context_objects: WeakValueDictionary[ - Tuple[Cache, CacheKey], _CacheContext - ] = WeakValueDictionary() + # WeakValueDictionary is not a generic at runtime, so we need the quotes (Py <3.7) + _cache_context_objects: "WeakValueDictionary[Tuple[Cache, CacheKey], _CacheContext]" = WeakValueDictionary() def __init__(self, cache: Cache, cache_key: CacheKey) -> None: self._cache = cache @@ -648,7 +645,7 @@ def invalidate(self) -> None: self._cache.invalidate(self._cache_key) @classmethod - def get_instance(cls, cache: Cache, cache_key: CacheKey) -> _CacheContext: + def get_instance(cls, cache: Cache, cache_key: CacheKey) -> "_CacheContext": """Returns an instance constructed with the given arguments. A new instance is only created if none already exists. From 735ee87f89b3e5db756c3a4d5dc1bb03db9d4e58 Mon Sep 17 00:00:00 2001 From: "Kai A. Hiller" Date: Wed, 6 Nov 2019 19:12:01 +0100 Subject: [PATCH 7/7] _CacheContext: Restore compatibility with even lower Python versions (3.5) --- synapse/util/caches/descriptors.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index 0254498f8043..84f5ae22c374 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -633,19 +633,20 @@ class _CacheContext: on a lower level. """ - # WeakValueDictionary is not a generic at runtime, so we need the quotes (Py <3.7) - _cache_context_objects: "WeakValueDictionary[Tuple[Cache, CacheKey], _CacheContext]" = WeakValueDictionary() + _cache_context_objects = ( + WeakValueDictionary() + ) # type: WeakValueDictionary[Tuple[Cache, CacheKey], _CacheContext] - def __init__(self, cache: Cache, cache_key: CacheKey) -> None: + def __init__(self, cache, cache_key): # type: (Cache, CacheKey) -> None self._cache = cache self._cache_key = cache_key - def invalidate(self) -> None: + def invalidate(self): # type: () -> None """Invalidates the cache entry referred to by the context.""" self._cache.invalidate(self._cache_key) @classmethod - def get_instance(cls, cache: Cache, cache_key: CacheKey) -> "_CacheContext": + def get_instance(cls, cache, cache_key): # type: (Cache, CacheKey) -> _CacheContext """Returns an instance constructed with the given arguments. A new instance is only created if none already exists.