Merge pull request #29170 from ankush/perf/jenv

perf: persistent jenv, ~7x faster `/login`

frappe/core/doctype/navbar_settings/navbar_settings.py

@@ -24,8 +24,10 @@ class NavbarSettings(Document):
 
 
 def get_app_logo():
-	app_logo = frappe.get_website_settings("app_logo") or frappe.db.get_single_value(
-		"Navbar Settings", "app_logo", cache=True
+	app_logo = frappe.get_website_settings("app_logo") or frappe.get_cached_value(
+		"Navbar Settings",
+		"Navbar Settings",
+		"app_logo",
 	)
 
 	if not app_logo:

frappe/tests/test_safe_exec.py

@@ -2,6 +2,7 @@ import types
 
 import frappe
 from frappe.tests import IntegrationTestCase
+from frappe.utils.jinja import get_jenv
 from frappe.utils.safe_exec import ServerScriptNotEnabled, get_safe_globals, safe_exec
 
 
@@ -127,3 +128,17 @@ class TestSafeExec(IntegrationTestCase):
 class TestNoSafeExec(IntegrationTestCase):
 	def test_safe_exec_disabled_by_default(self):
 		self.assertRaises(ServerScriptNotEnabled, safe_exec, "pass")
+
+
+class TestJinjaGlobals(IntegrationTestCase):
+	def test_jenv_thread_safety(self):
+		first = get_jenv()
+		# reinit to create a new local ctx, this "simulates" two request running in two diff
+		# thread.
+		frappe.init(frappe.local.site, force=True)
+		second = get_jenv()
+		self.assertIsNot(first, second)
+		self.assertIsNot(first.globals, second.globals)
+		self.assertIsNot(first.filters, second.filters)
+		self.assertIsNot(first.globals["frappe"], second.globals["frappe"])
+		self.assertIsNot(first.globals["frappe"]["form_dict"], second.globals["frappe"]["form_dict"])

frappe/tests/test_utils.py

@@ -68,6 +68,7 @@ from frappe.utils.data import (
 	get_url_to_form,
 	get_year_ending,
 	getdate,
+	is_invalid_date_string,
 	now_datetime,
 	nowtime,
 	pretty_date,
@@ -669,11 +670,15 @@ class TestDateUtils(IntegrationTestCase):
 
 	@given(st.datetimes())
 	def test_get_datetime(self, original):
+		if is_invalid_date_string(str(original)):
+			return
 		parsed = get_datetime(str(original))
 		self.assertEqual(parsed, original)
 
 	@given(st.datetimes(timezones=st.timezones()))
 	def test_get_datetime_tz_aware(self, original):
+		if is_invalid_date_string(str(original)):
+			return
 		parsed = get_datetime(str(original))
 		self.assertEqual(parsed, original)
 

frappe/utils/jinja.py

@@ -6,35 +6,57 @@ from frappe.utils.caching import site_cache
 
 def get_jenv():
 	import frappe
+	from frappe.utils.safe_exec import get_safe_globals
 
-	if not getattr(frappe.local, "jenv", None):
-		from jinja2 import DebugUndefined
-		from jinja2.sandbox import SandboxedEnvironment
+	if jenv := getattr(frappe.local, "jenv", None):
+		return jenv
 
-		from frappe.utils.safe_exec import UNSAFE_ATTRIBUTES, get_safe_globals
+	default_jenv = _get_jenv()
+	jenv = default_jenv.overlay()
+	# XXX: This is safe to share between requests, the only reason why we are overlaying jenv is to
+	# reuse cache but still have request specific jenv object.
+	if not frappe._dev_server:
+		jenv.cache = default_jenv.cache
 
-		UNSAFE_ATTRIBUTES = UNSAFE_ATTRIBUTES - {"format", "format_map"}
+	# Note: Overlay by default is "linked", we need to copy everything we are updating.
+	jenv.globals = default_jenv.globals.copy()
+	jenv.filters = default_jenv.filters.copy()
 
-		class FrappeSandboxedEnvironment(SandboxedEnvironment):
-			def is_safe_attribute(self, obj, attr, *args, **kwargs):
-				if attr in UNSAFE_ATTRIBUTES:
-					return False
+	jenv.globals.update(get_safe_globals())
+	methods, filters = get_jinja_hooks()
+	jenv.globals.update(methods or {})
+	jenv.filters.update(filters or {})
 
-				return super().is_safe_attribute(obj, attr, *args, **kwargs)
+	frappe.local.jenv = jenv
 
-		# frappe will be loaded last, so app templates will get precedence
-		jenv = FrappeSandboxedEnvironment(loader=get_jloader(), undefined=DebugUndefined)
-		set_filters(jenv)
+	return jenv
 
-		jenv.globals.update(get_safe_globals())
 
-		methods, filters = get_jinja_hooks()
-		jenv.globals.update(methods or {})
-		jenv.filters.update(filters or {})
+@site_cache(ttl=10 * 60, maxsize=4)
+def _get_jenv():
+	# XXX: DO NOT use any thread/request specific data in this function!
+	# Some functionality like `get_safe_globals` appears safe but internally uses request local
+	# data.
 
-		frappe.local.jenv = jenv
+	from jinja2 import DebugUndefined
+	from jinja2.sandbox import SandboxedEnvironment
 
-	return frappe.local.jenv
+	from frappe.utils.safe_exec import UNSAFE_ATTRIBUTES
+
+	UNSAFE_ATTRIBUTES = UNSAFE_ATTRIBUTES - {"format", "format_map"}
+
+	class FrappeSandboxedEnvironment(SandboxedEnvironment):
+		def is_safe_attribute(self, obj, attr, *args, **kwargs):
+			if attr in UNSAFE_ATTRIBUTES:
+				return False
+
+			return super().is_safe_attribute(obj, attr, *args, **kwargs)
+
+	# frappe will be loaded last, so app templates will get precedence
+	jenv = FrappeSandboxedEnvironment(loader=get_jloader(), undefined=DebugUndefined, cache_size=32)
+	set_filters(jenv)
+
+	return jenv
 
 
 def get_template(path):
@@ -95,7 +117,7 @@ def render_template(template, context=None, is_path=None, safe_render=True):
 	try:
 		if is_path or guess_is_path(template):
 			is_path = True
-			compiled_template = compile_template(template)
+			compiled_template = get_template(template)
 		else:
 			jenv: SandboxedEnvironment = get_jenv()
 			if safe_render and ".__" in template:
@@ -146,12 +168,6 @@ def get_jloader():
 	return jloader
 
 
-@site_cache(ttl=10 * 60, maxsize=16)
-def compile_template(path):
-	jenv = get_jenv()
-	return jenv.get_template(path)
-
-
 @site_cache(ttl=10 * 60, maxsize=8)
 def _get_jloader():
 	from jinja2 import ChoiceLoader, PackageLoader, PrefixLoader