From df8399f5d3961096cc01100b36dd2c113ea24ce8 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Sat, 17 Sep 2022 10:58:40 +0530 Subject: [PATCH 1/7] perf: initialise field map when initialising meta --- frappe/model/meta.py | 81 +++++++++++++++++++++++--------------------- 1 file changed, 43 insertions(+), 38 deletions(-) diff --git a/frappe/model/meta.py b/frappe/model/meta.py index b06e174ef2..075c829ffe 100644 --- a/frappe/model/meta.py +++ b/frappe/model/meta.py @@ -40,6 +40,20 @@ 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: @@ -86,7 +100,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 +108,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 +141,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 +250,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 +278,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 +342,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 +352,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 +452,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) From 6c6a969d3a79ce8626c13a941b450acc1dde3f69 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Sat, 17 Sep 2022 11:33:47 +0530 Subject: [PATCH 2/7] perf: simpler, faster meta cache --- frappe/__init__.py | 1 - frappe/cache_manager.py | 3 --- frappe/core/doctype/doctype/doctype.py | 4 ---- frappe/model/meta.py | 15 +++------------ 4 files changed, 3 insertions(+), 20 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index a1e661dff0..6e8ee9344b 100644 --- a/frappe/__init__.py +++ b/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() diff --git a/frappe/cache_manager.py b/frappe/cache_manager.py index 942adfa190..0ef63a539d 100644 --- a/frappe/cache_manager.py +++ b/frappe/cache_manager.py @@ -116,9 +116,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) diff --git a/frappe/core/doctype/doctype/doctype.py b/frappe/core/doctype/doctype/doctype.py index 41d16b8a86..acc5c4871d 100644 --- a/frappe/core/doctype/doctype/doctype.py +++ b/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): diff --git a/frappe/model/meta.py b/frappe/model/meta.py index 075c829ffe..096cf29643 100644 --- a/frappe/model/meta.py +++ b/frappe/model/meta.py @@ -56,19 +56,10 @@ DEFAULT_FIELD_LABELS = { 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) + return frappe.cache().hget("meta", doctype, lambda: Meta(doctype)) def load_meta(doctype): From b529c27cb558dae8a6ba5c9d1f35c7e518d61d6d Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Sat, 17 Sep 2022 12:10:21 +0530 Subject: [PATCH 3/7] fix: ensure error is thrown --- frappe/model/meta.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/frappe/model/meta.py b/frappe/model/meta.py index 096cf29643..4f03279de9 100644 --- a/frappe/model/meta.py +++ b/frappe/model/meta.py @@ -59,7 +59,12 @@ def get_meta(doctype, cached=True) -> "Meta": if not cached: return Meta(doctype) - return frappe.cache().hget("meta", doctype, lambda: Meta(doctype)) + if meta := frappe.cache().hget("meta", doctype): + return meta + + meta = Meta(doctype) + frappe.cache().hset("meta", doctype, meta) + return meta def load_meta(doctype): From abd018773b5eee9cfdd2ebdfa619aa9200586186 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 18 Sep 2022 00:41:31 +0530 Subject: [PATCH 4/7] 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. --- frappe/__init__.py | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 6e8ee9344b..5eb8dd94e5 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -1063,21 +1063,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: @@ -1150,7 +1138,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 From bcb5fe91aa260cd4fbd3b1b930a650e653807c3d Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 18 Sep 2022 01:05:29 +0530 Subject: [PATCH 5/7] 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. --- frappe/__init__.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 5eb8dd94e5..e5174f022c 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -1089,17 +1089,20 @@ def get_cached_doc(*args, **kwargs) -> "Document": if not key: key = get_document_cache_key(doc.doctype, doc.name) - local.document_cache[key] = doc + _set_document_in_cache(key, doc) + return doc + + +def _set_document_in_cache(key: str, doc: "Document") -> None: # Avoid setting in local.cache since we're already using local.document_cache above # Try pickling the doc object as-is first, else fallback to doc.as_dict() + local.document_cache[key] = doc try: cache().hset("document_cache", key, doc, cache_locally=False) except Exception: cache().hset("document_cache", key, doc.as_dict(), cache_locally=False) - return doc - def can_cache_doc(args) -> str | None: """ @@ -1174,13 +1177,10 @@ def get_doc(*args, **kwargs) -> "Document": doc = frappe.model.document.get_doc(*args, **kwargs) - # Replace cache + # Replace cache if stale one exists 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()) + _set_document_in_cache(key, doc) return doc From c28ddcc3e67cf0e3141eae27df3a1415b461800f Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 18 Sep 2022 01:32:46 +0530 Subject: [PATCH 6/7] test: basic perf test for rps on get_list --- frappe/tests/test_perf.py | 39 +++++++++++++++++++++++++++++++++++++++ frappe/tests/utils.py | 2 +- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/frappe/tests/test_perf.py b/frappe/tests/test_perf.py index 6e23fb9856..5ce5d2dd5d 100644 --- a/frappe/tests/test_perf.py +++ b/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"] diff --git a/frappe/tests/utils.py b/frappe/tests/utils.py index b83a84ff9a..a56fe28d87 100644 --- a/frappe/tests/utils.py +++ b/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) From 40f6a348400259d5095a4db3e279984664b96fec Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 19 Sep 2022 16:14:08 +0530 Subject: [PATCH 7/7] build: pin testing-library/dom temporarily refer https://github.com/testing-library/dom-testing-library/issues/1169 --- frappe/commands/utils.py | 1 + 1 file changed, 1 insertion(+) diff --git a/frappe/commands/utils.py b/frappe/commands/utils.py index c7b331b253..07061444b0 100644 --- a/frappe/commands/utils.py +++ b/frappe/commands/utils.py @@ -895,6 +895,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", ] )