From 111df3a8d7fab30e03dce18221812422e0cc0a34 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@frappe.io>
Date: Fri, 3 Jun 2022 15:28:20 +0530
Subject: [PATCH] perf: improve document caching

As per current implementation whenever `get_doc` is called, document is
cached. However, this cache is only ever used by `get_cached_doc`. Going
through the codebase of both FF/ERPNext you'll find that `get_doc` is
used a lot more than `get_cached_doc`. So in many places, all this
caching overhead is unnecessary.

This change removes implicit caching from get_doc and replaces it with
cache-replacement instead. i.e. cache is only updated if it exists but
not created from get_doc.

Pros:
- faster `get_doc`
- lower memory usage on Redis
- Reduces chances of OOM by blowing up worker's memory as old docs can't
  be GCed until
- Correctness i.e. caching only what gets used from cache.

Con:
- After this change. First call to `get_cached_doc` will always be a cache miss. DUH.
---
 frappe/__init__.py            | 19 ++++++++++++++-----
 frappe/utils/redis_wrapper.py |  9 +++++++++
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/frappe/__init__.py b/frappe/__init__.py
index 11acdc55f4..ea578b4813 100644
--- a/frappe/__init__.py
+++ b/frappe/__init__.py
@@ -1023,7 +1023,7 @@ def get_cached_doc(*args, **kwargs):
 		return doc
 
 	if key := can_cache_doc(args):
-		# local cache
+		# local cache - has "ready" `Document` objects
 		if doc := local.document_cache.get(key):
 			return _respond(doc)
 
@@ -1031,9 +1031,16 @@ def get_cached_doc(*args, **kwargs):
 		if doc := cache().hget("document_cache", key):
 			return _respond(doc, True)
 
-	# database
+	# Not found in local/redis, fetch from DB and store in cache
 	doc = get_doc(*args, **kwargs)
 
+	# Store in redis cache
+	key = get_document_cache_key(doc.doctype, doc.name)
+
+	local.document_cache[key] = doc
+	# Avoid setting in local.cache since there's separate cache
+	cache().hset("document_cache", key, doc.as_dict(), cache_locally=False)
+
 	return doc
 
 
@@ -1104,10 +1111,12 @@ def get_doc(*args, **kwargs):
 
 	doc = frappe.model.document.get_doc(*args, **kwargs)
 
-	# set in cache
+	# Replace cache
 	if key := can_cache_doc(args):
-		local.document_cache[key] = doc
-		cache().hset("document_cache", key, doc.as_dict())
+		if key in local.document_cache:
+			local.document_cache[key] = doc
+		if cache().hexists("document_cache", key):
+			cache().hset("document_cache", key, doc.as_dict())
 
 	return doc
 
diff --git a/frappe/utils/redis_wrapper.py b/frappe/utils/redis_wrapper.py
index c06161f270..3f364821d1 100644
--- a/frappe/utils/redis_wrapper.py
+++ b/frappe/utils/redis_wrapper.py
@@ -169,6 +169,15 @@ class RedisWrapper(redis.Redis):
 		except redis.exceptions.ConnectionError:
 			pass
 
+	def hexists(self, name: str, key: str, shared: bool = False) -> bool:
+		if key is None:
+			return False
+		_name = self.make_key(name, shared=shared)
+		try:
+			return super(RedisWrapper, self).hexists(_name, key)
+		except redis.exceptions.ConnectionError:
+			return False
+
 	def hgetall(self, name):
 		value = super(RedisWrapper, self).hgetall(self.make_key(name))
 		return {key: pickle.loads(value) for key, value in value.items()}