From 6f9d377e834857ce6b37a8525bfe9f9d1785539b Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Sun, 16 Mar 2025 11:56:06 +0530 Subject: [PATCH 1/2] test: rewrite test_request_cache --- frappe/tests/test_caching.py | 47 ++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/frappe/tests/test_caching.py b/frappe/tests/test_caching.py index de5ce55fab..71fe6ca84e 100644 --- a/frappe/tests/test_caching.py +++ b/frappe/tests/test_caching.py @@ -38,16 +38,21 @@ def ping_with_ttl() -> str: class TestCachingUtils(IntegrationTestCase): def test_request_cache(self): retval = [] - acceptable_args = [ - [1, 2, 3, 4], + hashable_values = [ range(10), - {"abc": "test-key"}, frappe.get_last_doc("DocType"), + True, + None, + ] + + unhashable_values = [ + [1, 2, 3, 4], + {"abc": "test-key"}, frappe._dict(), ] def same_output_received(): - return all([x for x in set(retval) if x == retval[0]]) + return len(set(retval)) == 1 # ensure that external service was called only once # thereby return value of request_specific_api is cached @@ -55,27 +60,33 @@ class TestCachingUtils(IntegrationTestCase): external_service.assert_called_once() self.assertTrue(same_output_received()) - # ensure that cache differentiates between int & float - # types. Giving different return values for both + # hash() function does not differentiate between int & float + # Giving same values for both retval.append(request_specific_api(120.0, 23)) - self.assertTrue(external_service.call_count, 2) - - # ensure that function is executed when call isn't - # already cached - retval.clear() - for _ in range(10): - request_specific_api(120, 13) - self.assertTrue(external_service.call_count, 3) + external_service.assert_called_once() self.assertTrue(same_output_received()) - # ensure key generation capacity for different types + # ensure that function is executed when call isn't already cached retval.clear() - for arg in acceptable_args: + retval.extend(request_specific_api(120, 13) for _ in range(10)) + self.assertEqual(external_service.call_count, 2) + self.assertTrue(same_output_received()) + + # ensure single call if key is hashable + for arg in hashable_values: external_service.call_count = 0 for _ in range(2): request_specific_api(arg, 13) - self.assertTrue(external_service.call_count, 1) - self.assertTrue(same_output_received()) + + self.assertEqual(external_service.call_count, 1) + + # multiple calls if key cannot be generated + for arg in unhashable_values: + external_service.call_count = 0 + for _ in range(2): + request_specific_api(arg, 13) + + self.assertEqual(external_service.call_count, 2) class TestSiteCache(FrappeAPITestCase): From fdc7f69212d82de2fe009f0d055a6690d1d5292a Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Sun, 16 Mar 2025 12:35:37 +0530 Subject: [PATCH 2/2] perf: faster retrieval from site cache --- frappe/utils/caching.py | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/frappe/utils/caching.py b/frappe/utils/caching.py index bd6fa13bdc..f2f1f9aecd 100644 --- a/frappe/utils/caching.py +++ b/frappe/utils/caching.py @@ -11,13 +11,16 @@ from types import NoneType import frappe _SITE_CACHE = defaultdict(dict) +_KWD_MARK = object() # sentinel for separating args from kwargs -def __generate_request_cache_key(args: tuple, kwargs: dict) -> int: +def __generate_request_cache_key(args: tuple, kwargs: dict) -> tuple: """Generate a key for the cache.""" + if not kwargs: - return hash(args) - return hash((args, frozenset(kwargs.items()))) + return args + + return (args, _KWD_MARK, frozenset(kwargs.items())) def request_cache(func: Callable) -> Callable: @@ -60,7 +63,11 @@ def request_cache(func: Callable) -> Callable: try: return _cache[func][args_key] + except TypeError: + # args_key is not hashable + return func(*args, **kwargs) except KeyError: + # cache miss return_val = func(*args, **kwargs) _cache[func][args_key] = return_val return return_val @@ -102,11 +109,9 @@ def site_cache(ttl: int | None = None, maxsize: int | None = None) -> Callable: """ def time_cache_wrapper(func: Callable | None = None) -> Callable: - func_key = f"{func.__module__}.{func.__name__}" - def clear_cache(): """Clear cache for this function for all sites if not specified.""" - _SITE_CACHE[func_key].clear() + _SITE_CACHE[func].clear() func.clear_cache = clear_cache @@ -123,7 +128,7 @@ def site_cache(ttl: int | None = None, maxsize: int | None = None) -> Callable: if not site: return func(*args, **kwargs) - arguments_key = f"{site}::{__generate_request_cache_key(args, kwargs)}" + arguments_key = (site, __generate_request_cache_key(args, kwargs)) if hasattr(func, "ttl") and time.monotonic() >= func.expiration: func.clear_cache() @@ -134,10 +139,12 @@ def site_cache(ttl: int | None = None, maxsize: int | None = None) -> Callable: # 2. Other thread can pop the exact elemement we are reading if maxsize is hit. # NOTE: Keep a local reference to dictionary of interest so it doesn't get swapped - function_cache = _SITE_CACHE[func_key] + function_cache = _SITE_CACHE[func] try: return function_cache[arguments_key] + + # not handling TypeError here, expecting arguments_key to be hashable except (KeyError, RuntimeError): # NOTE: This is just a cache miss or dictionary was modified while reading it pass @@ -180,7 +187,7 @@ def redis_cache(ttl: int | None = 3600, user: str | bool | None = None, shared: @wraps(func) def redis_cache_wrapper(*args, **kwargs): - func_call_key = func_key + "::" + str(__generate_request_cache_key(args, kwargs)) + func_call_key = f"{func_key}::{hash(__generate_request_cache_key(args, kwargs))}" cached_val = frappe.cache.get_value(func_call_key, user=user, shared=shared) if cached_val is not None: return cached_val