Merge pull request #18164 from resilient-tech/perf-meta-field-map
perf: cache `Meta` directly and other improvements
This commit is contained in:
Ankush Menat
GitHub
commit
00a82fabd7
7 changed files with 102 additions and 81 deletions
|
|
@ -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()
|
||||
|
|
@ -1064,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:
|
||||
|
|
@ -1102,17 +1089,20 @@ def get_cached_doc(*args, **kwargs):
|
|||
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:
|
||||
"""
|
||||
|
|
@ -1151,7 +1141,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
|
||||
|
|
@ -1187,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
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
]
|
||||
)
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
|
|
|||
|
|
@ -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("meta", doctype):
|
||||
return meta
|
||||
|
||||
meta = Meta(doctype)
|
||||
frappe.cache().hset("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)
|
||||
|
|
|
|||
|
|
@ -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"]
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue