perf: cache `Meta` directly and other improvements (backport #18164) (#18181)

* perf: initialise field map when initialising meta

(cherry picked from commit df8399f5d3)

* perf: simpler, faster meta cache

(cherry picked from commit 6c6a969d3a)

* fix: ensure error is thrown

(cherry picked from commit b529c27cb5)

* refactor: drop _allow_dict support

This is not required anymore since we store full doc anyway. Also since
they share same cache key calling get_cached_value and get_cached_doc
can end up doing duplicate / double work.

(cherry picked from commit abd018773b)

* fix: pickle doc objects directly

This seems to be missed out and causes performance regression in IRL
usage when get_doc is called on cached doc already.

(cherry picked from commit bcb5fe91aa)

* test: basic perf test for rps on get_list

(cherry picked from commit c28ddcc3e6)

* build: pin testing-library/dom temporarily

refer https://github.com/testing-library/dom-testing-library/issues/1169

(cherry picked from commit 40f6a34840)

* chore: semantic changes to document caching code

* fix: drop Meta cache during update

Revert "fix: drop Meta cache during update" (#18186)

* Revert "fix: drop Meta cache during update (#18182)"

This reverts commit 656f6df257.

* fix: replace meta cache keys

Old keys stored different types of data `dict` changing key to indicate
change in type.

* perf: cache `FormMeta` directly (#18165)

* perf: cache `FormMeta` directly

* perf: check if `dt` is table, use `db.get_value` instead of `get_all`

Co-authored-by: Sagar Vora <sagar@resilient.tech>
Co-authored-by: Ankush Menat <ankush@frappe.io>

frappe/__init__.py

@@ -239,7 +239,6 @@ def init(site: str, sites_path: str = ".", new_site: bool = False) -> None:
 	local.jloader = None
 	local.cache = {}
 	local.document_cache = {}
-	local.meta_cache = {}
 	local.form_dict = _dict()
 	local.preload_assets = {"style": [], "script": []}
 	local.session = _dict()
@@ -1065,21 +1064,9 @@ def set_value(doctype, docname, fieldname, value=None):
 	return frappe.client.set_value(doctype, docname, fieldname, value)
 
 
-@overload
-def get_cached_doc(doctype, docname, _allow_dict=True) -> dict:
-	...
-
-
-@overload
 def get_cached_doc(*args, **kwargs) -> "Document":
-	...
-
-
-def get_cached_doc(*args, **kwargs):
-	allow_dict = kwargs.pop("_allow_dict", False)
-
 	def _respond(doc, from_redis=False):
-		if not allow_dict and isinstance(doc, dict):
+		if isinstance(doc, dict):
 			local.document_cache[key] = doc = get_doc(doc)
 
 		elif from_redis:
@@ -1103,6 +1090,12 @@ def get_cached_doc(*args, **kwargs):
 	if not key:
 		key = get_document_cache_key(doc.doctype, doc.name)
 
+	_set_document_in_cache(key, doc)
+
+	return doc
+
+
+def _set_document_in_cache(key: str, doc: "Document") -> None:
 	local.document_cache[key] = doc
 
 	# Avoid setting in local.cache since we're already using local.document_cache above
@@ -1112,8 +1105,6 @@ def get_cached_doc(*args, **kwargs):
 	except Exception:
 		cache().hset("document_cache", key, doc.as_dict(), cache_locally=False)
 
-	return doc
-
 
 def can_cache_doc(args) -> str | None:
 	"""
@@ -1152,7 +1143,7 @@ def get_cached_value(
 	doctype: str, name: str, fieldname: str = "name", as_dict: bool = False
 ) -> Any:
 	try:
-		doc = get_cached_doc(doctype, name, _allow_dict=True)
+		doc = get_cached_doc(doctype, name)
 	except DoesNotExistError:
 		clear_last_message()
 		return
@@ -1188,13 +1179,9 @@ def get_doc(*args, **kwargs) -> "Document":
 
 	doc = frappe.model.document.get_doc(*args, **kwargs)
 
-	# Replace cache
-	if key := can_cache_doc(args):
-		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())
+	# Replace cache if stale one exists
+	if (key := can_cache_doc(args)) and cache().hexists("document_cache", key):
+		_set_document_in_cache(key, doc)
 
 	return doc
 

frappe/cache_manager.py

@@ -59,8 +59,8 @@ user_cache_keys = (
 )
 
 doctype_cache_keys = (
-	"meta",
-	"form_meta",
+	"doctype_meta",
+	"doctype_form_meta",
 	"table_columns",
 	"last_modified",
 	"linked_doctypes",
@@ -117,9 +117,6 @@ def clear_doctype_cache(doctype=None):
 	clear_controller_cache(doctype)
 	cache = frappe.cache()
 
-	if getattr(frappe.local, "meta_cache") and (doctype in frappe.local.meta_cache):
-		del frappe.local.meta_cache[doctype]
-
 	for key in ("is_table", "doctype_modules", "document_cache"):
 		cache.delete_value(key)
 

frappe/commands/utils.py

@@ -881,6 +881,7 @@ def run_ui_tests(
 				"@4tw/cypress-drag-drop@^2",
 				"cypress-real-events",
 				"@testing-library/cypress@^8",
+				"@testing-library/dom@8.17.1",
 				"@cypress/code-coverage@^3",
 			]
 		)

frappe/core/doctype/doctype/doctype.py

@@ -414,10 +414,6 @@ class DocType(Document):
 		if not frappe.flags.in_install and hasattr(self, "before_update"):
 			self.sync_global_search()
 
-		# clear from local cache
-		if self.name in frappe.local.meta_cache:
-			del frappe.local.meta_cache[self.name]
-
 		clear_linked_doctype_cache()
 
 	def setup_autoincrement_and_sequence(self):

frappe/desk/form/load.py

@@ -64,11 +64,9 @@ def getdoctype(doctype, with_parent=False, cached_timestamp=None):
 	parent_dt = None
 
 	# with parent (called from report builder)
-	if with_parent:
-		parent_dt = frappe.model.meta.get_parent_dt(doctype)
-		if parent_dt:
-			docs = get_meta_bundle(parent_dt)
-			frappe.response["parent_dt"] = parent_dt
+	if with_parent and (parent_dt := frappe.model.meta.get_parent_dt(doctype)):
+		docs = get_meta_bundle(parent_dt)
+		frappe.response["parent_dt"] = parent_dt
 
 	if not docs:
 		docs = get_meta_bundle(doctype)

frappe/desk/form/meta.py

@@ -35,12 +35,10 @@ ASSET_KEYS = (
 def get_meta(doctype, cached=True):
 	# don't cache for developer mode as js files, templates may be edited
 	if cached and not frappe.conf.developer_mode:
-		meta = frappe.cache().hget("form_meta", doctype)
-		if meta:
-			meta = FormMeta(meta)
-		else:
+		meta = frappe.cache().hget("doctype_form_meta", doctype)
+		if not meta:
 			meta = FormMeta(doctype)
-			frappe.cache().hset("form_meta", doctype, meta.as_dict())
+			frappe.cache().hset("doctype_form_meta", doctype, meta)
 	else:
 		meta = FormMeta(doctype)
 

frappe/model/meta.py

@@ -40,21 +40,31 @@ from frappe.model.workflow import get_workflow_name
 from frappe.modules import load_doctype_module
 from frappe.utils import cast, cint, cstr
 
+DEFAULT_FIELD_LABELS = {
+	"name": lambda: _("ID"),
+	"creation": lambda: _("Created On"),
+	"docstatus": lambda: _("Document Status"),
+	"idx": lambda: _("Index"),
+	"modified": lambda: _("Last Updated On"),
+	"modified_by": lambda: _("Last Updated By"),
+	"owner": lambda: _("Created By"),
+	"_user_tags": lambda: _("Tags"),
+	"_liked_by": lambda: _("Liked By"),
+	"_comments": lambda: _("Comments"),
+	"_assign": lambda: _("Assigned To"),
+}
+
 
 def get_meta(doctype, cached=True) -> "Meta":
-	if cached:
-		if not frappe.local.meta_cache.get(doctype):
-			meta = frappe.cache().hget("meta", doctype)
-			if meta:
-				meta = Meta(meta)
-			else:
-				meta = Meta(doctype)
-				frappe.cache().hset("meta", doctype, meta.as_dict())
-			frappe.local.meta_cache[doctype] = meta
+	if not cached:
+		return Meta(doctype)
 
-		return frappe.local.meta_cache[doctype]
-	else:
-		return load_meta(doctype)
+	if meta := frappe.cache().hget("doctype_meta", doctype):
+		return meta
+
+	meta = Meta(doctype)
+	frappe.cache().hset("doctype_meta", doctype, meta)
+	return meta
 
 
 def load_meta(doctype):
@@ -86,7 +96,7 @@ def load_doctype_from_file(doctype):
 class Meta(Document):
 	_metaclass = True
 	default_fields = list(default_fields)[1:]
-	special_doctypes = (
+	special_doctypes = {
 		"DocField",
 		"DocPerm",
 		"DocType",
@@ -94,24 +104,25 @@ class Meta(Document):
 		"DocType Action",
 		"DocType Link",
 		"DocType State",
-	)
+	}
 	standard_set_once_fields = [
 		frappe._dict(fieldname="creation", fieldtype="Datetime"),
 		frappe._dict(fieldname="owner", fieldtype="Data"),
 	]
 
 	def __init__(self, doctype):
-		self._fields = {}
+		# from cache
 		if isinstance(doctype, dict):
 			super().__init__(doctype)
+			self.init_field_map()
+			return
 
-		elif isinstance(doctype, Document):
+		if isinstance(doctype, Document):
 			super().__init__(doctype.as_dict())
-			self.process()
-
 		else:
 			super().__init__("DocType", doctype)
-			self.process()
+
+		self.process()
 
 	def load_from_db(self):
 		try:
@@ -126,10 +137,12 @@ class Meta(Document):
 		# don't process for special doctypes
 		# prevent's circular dependency
 		if self.name in self.special_doctypes:
+			self.init_field_map()
 			return
 
 		self.add_custom_fields()
 		self.apply_property_setters()
+		self.init_field_map()
 		self.sort_fields()
 		self.get_valid_columns()
 		self.set_custom_permissions()
@@ -233,36 +246,24 @@ class Meta(Document):
 
 	def get_field(self, fieldname):
 		"""Return docfield from meta"""
-		if not self._fields:
-			for f in self.get("fields"):
-				self._fields[f.fieldname] = f
 
 		return self._fields.get(fieldname)
 
 	def has_field(self, fieldname):
 		"""Returns True if fieldname exists"""
-		return True if self.get_field(fieldname) else False
+
+		return fieldname in self._fields
 
 	def get_label(self, fieldname):
 		"""Get label of the given fieldname"""
-		df = self.get_field(fieldname)
-		if df:
-			label = df.label
-		else:
-			label = {
-				"name": _("ID"),
-				"creation": _("Created On"),
-				"docstatus": _("Document Status"),
-				"idx": _("Index"),
-				"modified": _("Last Updated On"),
-				"modified_by": _("Last Updated By"),
-				"owner": _("Created By"),
-				"_user_tags": _("Tags"),
-				"_liked_by": _("Liked By"),
-				"_comments": _("Comments"),
-				"_assign": _("Assigned To"),
-			}.get(fieldname) or _("No Label")
-		return label
+
+		if df := self.get_field(fieldname):
+			return df.label
+
+		if fieldname in DEFAULT_FIELD_LABELS:
+			return DEFAULT_FIELD_LABELS[fieldname]()
+
+		return _("No Label")
 
 	def get_options(self, fieldname):
 		return self.get_field(fieldname).options
@@ -273,12 +274,9 @@ class Meta(Document):
 		if df.fieldtype == "Link":
 			return df.options
 
-		elif df.fieldtype == "Dynamic Link":
+		if df.fieldtype == "Dynamic Link":
 			return self.get_options(df.options)
 
-		else:
-			return None
-
 	def get_search_fields(self):
 		search_fields = self.search_fields or "name"
 		search_fields = [d.strip() for d in search_fields.split(",")]
@@ -340,8 +338,9 @@ class Meta(Document):
 
 	def is_translatable(self, fieldname):
 		"""Return true of false given a field"""
-		field = self.get_field(fieldname)
-		return field and field.translatable
+
+		if field := self.get_field(fieldname):
+			return field.translatable
 
 	def get_workflow(self):
 		return get_workflow_name(self.name)
@@ -349,11 +348,10 @@ class Meta(Document):
 	def get_naming_series_options(self) -> list[str]:
 		"""Get list naming series options."""
 
-		field = self.get_field("naming_series")
-		if field:
+		if field := self.get_field("naming_series"):
 			options = field.options or ""
-
 			return options.split("\n")
+
 		return []
 
 	def add_custom_fields(self):
@@ -450,6 +448,9 @@ class Meta(Document):
 
 				self.set(fieldname, new_list)
 
+	def init_field_map(self):
+		self._fields = {field.fieldname: field for field in self.fields}
+
 	def sort_fields(self):
 		"""sort on basis of insert_after"""
 		custom_fields = sorted(self.get_custom_fields(), key=lambda df: df.idx)
@@ -666,10 +667,17 @@ def is_single(doctype):
 
 
 def get_parent_dt(dt):
-	parent_dt = frappe.get_all(
-		"DocField", "parent", dict(fieldtype=["in", frappe.model.table_fields], options=dt), limit=1
+	if not frappe.is_table(dt):
+		return ""
+
+	return (
+		frappe.db.get_value(
+			"DocField",
+			{"fieldtype": ("in", frappe.model.table_fields), "options": dt},
+			"parent",
+		)
+		or ""
 	)
-	return parent_dt and parent_dt[0].parent or ""
 
 
 def set_fieldname(field_id, fieldname):

frappe/tests/test_perf.py

@@ -16,14 +16,21 @@ query. This test can be written like this.
 >>> 		get_controller("User")
 
 """
+import time
 import unittest
 
+from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_fixed
+
 import frappe
+from frappe.frappeclient import FrappeClient
 from frappe.model.base_document import get_controller
+from frappe.query_builder.utils import db_type_is
+from frappe.tests.test_query_builder import run_only_if
 from frappe.tests.utils import FrappeTestCase
 from frappe.website.path_resolver import PathResolver
 
 
+@run_only_if(db_type_is.MARIADB)
 class TestPerformance(FrappeTestCase):
 	def reset_request_specific_caches(self):
 		# To simulate close to request level of handling
@@ -33,6 +40,8 @@ class TestPerformance(FrappeTestCase):
 		frappe.clear_cache()
 
 	def setUp(self) -> None:
+		self.HOST = frappe.utils.get_site_url(frappe.local.site)
+
 		self.reset_request_specific_caches()
 
 	def test_meta_caching(self):
@@ -55,6 +64,36 @@ class TestPerformance(FrappeTestCase):
 		with self.assertQueryCount(0):
 			doc.get_invalid_links()
 
+	@retry(
+		retry=retry_if_exception_type(AssertionError),
+		stop=stop_after_attempt(3),
+		wait=wait_fixed(0.5),
+		reraise=True,
+	)
+	def test_req_per_seconds_basic(self):
+		"""Ideally should be ran against gunicorn worker, though I have not seen any difference
+		when using werkzeug's run_simple for synchronous requests."""
+
+		EXPECTED_RPS = 55  # measured on GHA
+		FAILURE_THREASHOLD = 0.1
+
+		req_count = 1000
+		client = FrappeClient(self.HOST, "Administrator", self.ADMIN_PASSWORD)
+
+		start = time.perf_counter()
+		for _ in range(req_count):
+			client.get_list("ToDo", limit_page_length=1)
+		end = time.perf_counter()
+
+		rps = req_count / (end - start)
+
+		print(f"Completed {req_count} in {end - start} @ {rps} requests per seconds")
+		self.assertGreaterEqual(
+			rps,
+			EXPECTED_RPS * (1 - FAILURE_THREASHOLD),
+			f"Possible performance regression in basic /api/Resource list  requests",
+		)
+
 	@unittest.skip("Not implemented")
 	def test_homepage_resolver(self):
 		paths = ["/", "/app"]

frappe/tests/utils.py

@@ -26,9 +26,9 @@ class FrappeTestCase(unittest.TestCase):
 	@classmethod
 	def setUpClass(cls) -> None:
 		cls.TEST_SITE = getattr(frappe.local, "site", None) or cls.TEST_SITE
+		cls.ADMIN_PASSWORD = frappe.get_conf(cls.TEST_SITE).admin_password
 		# flush changes done so far to avoid flake
 		frappe.db.commit()
-		frappe.db.begin()
 		if cls.SHOW_TRANSACTION_COMMIT_WARNINGS:
 			frappe.db.add_before_commit(_commit_watcher)