From 8a37d6d2786a35c4b6231aa029ce338790f1ecfe Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Fri, 23 Jun 2023 12:51:45 +0530 Subject: [PATCH] perf: reduce memory usage of background processes (#21467) * perf: defer translation.py imports This indirectly imports babel which isn't really required most of the time. * perf: defer gzip import * perf: move validate_and_sanitize_search_inputs This causes all sorts of indirect imports and increases memory usage * perf: defer requests module imports * perf: defer system settings import * perf: defer LOG_DOCTYPES import Causes many indirect imports * perf: defer update_site_config * perf: defer notifications import * perf: remove unused import * perf: defer safe exec import * test: memory usage overhead --- frappe/__init__.py | 18 ++++++++- frappe/boot.py | 5 ++- frappe/build.py | 8 ++-- frappe/cache_manager.py | 7 ++-- frappe/commands/redis_utils.py | 3 +- frappe/commands/site.py | 5 ++- frappe/core/doctype/rq_job/test_rq_job.py | 14 +++++++ .../system_settings/system_settings.py | 9 +++-- frappe/database/database.py | 1 - frappe/defaults.py | 1 - frappe/desk/form/meta.py | 3 +- frappe/desk/search.py | 20 ++-------- frappe/query_builder/utils.py | 37 ++++++++++--------- frappe/utils/__init__.py | 5 ++- frappe/utils/change_log.py | 2 +- frappe/utils/data.py | 8 ++++ frappe/utils/scheduler.py | 3 +- 17 files changed, 93 insertions(+), 56 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 0f85c8a11b..a4ac8111dc 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -2418,4 +2418,20 @@ def mock(type, size=1, locale="en"): return squashify(results) -from frappe.desk.search import validate_and_sanitize_search_inputs # noqa +def validate_and_sanitize_search_inputs(fn): + @functools.wraps(fn) + def wrapper(*args, **kwargs): + from frappe.desk.search import sanitize_searchfield + from frappe.utils import cint + + kwargs.update(dict(zip(fn.__code__.co_varnames, args))) + sanitize_searchfield(kwargs["searchfield"]) + kwargs["start"] = cint(kwargs["start"]) + kwargs["page_len"] = cint(kwargs["page_len"]) + + if kwargs["doctype"] and not db.exists("DocType", kwargs["doctype"]): + return [] + + return fn(**kwargs) + + return wrapper diff --git a/frappe/boot.py b/frappe/boot.py index 8881d25bd6..fb1bd3d7a2 100644 --- a/frappe/boot.py +++ b/frappe/boot.py @@ -21,7 +21,6 @@ from frappe.social.doctype.energy_point_log.energy_point_log import get_energy_p from frappe.social.doctype.energy_point_settings.energy_point_settings import ( is_energy_point_enabled, ) -from frappe.translate import get_lang_dict, get_messages_for_boot, get_translated_doctypes from frappe.utils import add_user_info, cstr, get_system_timezone from frappe.utils.change_log import get_versions from frappe.website.doctype.web_page_view.web_page_view import is_tracking_enabled @@ -29,6 +28,8 @@ from frappe.website.doctype.web_page_view.web_page_view import is_tracking_enabl def get_bootinfo(): """build and return boot info""" + from frappe.translate import get_lang_dict, get_translated_doctypes + frappe.set_user_lang(frappe.session.user) bootinfo = frappe._dict() hooks = frappe.get_hooks() @@ -257,6 +258,8 @@ def get_user_pages_or_reports(parent, cache=False): def load_translations(bootinfo): + from frappe.translate import get_messages_for_boot + bootinfo["lang"] = frappe.lang bootinfo["__messages"] = get_messages_for_boot() diff --git a/frappe/build.py b/frappe/build.py index 46301dadaf..66b3eabd5e 100644 --- a/frappe/build.py +++ b/frappe/build.py @@ -10,8 +10,6 @@ from urllib.parse import urlparse import click import psutil -from requests import head -from requests.exceptions import HTTPError from semantic_version import Version import frappe @@ -27,7 +25,7 @@ class AssetsNotDownloadedError(Exception): pass -class AssetsDontExistError(HTTPError): +class AssetsDontExistError(Exception): pass @@ -78,6 +76,8 @@ def build_missing_files(): def get_assets_link(frappe_head) -> str: + import requests + tag = getoutput( r"cd ../apps/frappe && git show-ref --tags -d | grep %s | sed -e 's,.*" r" refs/tags/,,' -e 's/\^{}//'" % frappe_head @@ -89,7 +89,7 @@ def get_assets_link(frappe_head) -> str: else: url = f"http://assets.frappeframework.com/{frappe_head}.tar.gz" - if not head(url): + if not requests.head(url): reference = f"Release {tag}" if tag else f"Commit {frappe_head}" raise AssetsDontExistError(f"Assets for {reference} don't exist") diff --git a/frappe/cache_manager.py b/frappe/cache_manager.py index 6ee88d9d37..f47478d871 100644 --- a/frappe/cache_manager.py +++ b/frappe/cache_manager.py @@ -1,10 +1,7 @@ # Copyright (c) 2018, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE -import json - import frappe -from frappe.desk.notifications import clear_notifications, delete_notification_count_for common_default_keys = ["__default", "__global"] @@ -79,6 +76,8 @@ doctype_cache_keys = ( def clear_user_cache(user=None): + from frappe.desk.notifications import clear_notifications + # this will automatically reload the global cache # so it is important to clear this first clear_notifications(user) @@ -128,6 +127,8 @@ def clear_doctype_cache(doctype=None): def _clear_doctype_cache_form_redis(doctype: str | None = None): + from frappe.desk.notifications import delete_notification_count_for + for key in ("is_table", "doctype_modules"): frappe.cache.delete_value(key) diff --git a/frappe/commands/redis_utils.py b/frappe/commands/redis_utils.py index 1c3292b7fa..07ad8715c4 100644 --- a/frappe/commands/redis_utils.py +++ b/frappe/commands/redis_utils.py @@ -3,7 +3,6 @@ import os import click import frappe -from frappe.installer import update_site_config from frappe.utils.redis_queue import RedisQueue @@ -23,6 +22,8 @@ def create_rq_users(set_admin_password=False, use_rq_auth=False): acl config file will be used by redis server while starting the server and app config is used by app while connecting to redis server. """ + from frappe.installer import update_site_config + acl_file_path = os.path.abspath("../config/redis_queue.acl") with frappe.init_site(): diff --git a/frappe/commands/site.py b/frappe/commands/site.py index 25c8c3159d..d606bb78cf 100644 --- a/frappe/commands/site.py +++ b/frappe/commands/site.py @@ -9,7 +9,6 @@ import click # imports - module imports import frappe from frappe.commands import get_site, pass_context -from frappe.core.doctype.log_settings.log_settings import LOG_DOCTYPES from frappe.exceptions import SiteNotSpecifiedError @@ -1199,11 +1198,12 @@ def build_search_index(context): @click.command("clear-log-table") -@click.option("--doctype", required=True, type=click.Choice(LOG_DOCTYPES), help="Log DocType") +@click.option("--doctype", required=True, type=str, help="Log DocType") @click.option("--days", type=int, help="Keep records for days") @click.option("--no-backup", is_flag=True, default=False, help="Do not backup the table") @pass_context def clear_log_table(context, doctype, days, no_backup): + """If any logtype table grows too large then clearing it with DELETE query is not feasible in reasonable time. This command copies recent data to new table and replaces current table with new smaller table. @@ -1211,6 +1211,7 @@ def clear_log_table(context, doctype, days, no_backup): ref: https://mariadb.com/kb/en/big-deletes/#deleting-more-than-half-a-table """ + from frappe.core.doctype.log_settings.log_settings import LOG_DOCTYPES from frappe.core.doctype.log_settings.log_settings import clear_log_table as clear_logs from frappe.utils.backups import scheduled_backup diff --git a/frappe/core/doctype/rq_job/test_rq_job.py b/frappe/core/doctype/rq_job/test_rq_job.py index 09a90f7445..c39717cfd8 100644 --- a/frappe/core/doctype/rq_job/test_rq_job.py +++ b/frappe/core/doctype/rq_job/test_rq_job.py @@ -124,6 +124,20 @@ class TestRQJob(FrappeTestCase): frappe.db.commit() self.assertIsNone(get_job_status(job_id)) + @timeout(20) + def test_memory_usage(self): + job = frappe.enqueue("frappe.utils.data._get_rss_memory_usage") + self.check_status(job, "finished") + + rss = job.latest_result().return_value + msg = """Memory usage of simple background job increased. Potential root cause can be a newly added python module import. Check and move them to approriate file/function to avoid loading the module by default.""" + + # If this starts failing analyze memory usage using memray or some equivalent tool to find + # offending imports/function calls. + # Refer this PR: https://github.com/frappe/frappe/pull/21467 + LAST_MEASURED_USAGE = 40 + self.assertLessEqual(rss, LAST_MEASURED_USAGE * 1.05, msg) + def test_func(fail=False, sleep=0): if fail: diff --git a/frappe/core/doctype/system_settings/system_settings.py b/frappe/core/doctype/system_settings/system_settings.py index 2fec4e87af..0c842f9c7d 100644 --- a/frappe/core/doctype/system_settings/system_settings.py +++ b/frappe/core/doctype/system_settings/system_settings.py @@ -5,14 +5,13 @@ import frappe from frappe import _ from frappe.model import no_value_fields from frappe.model.document import Document -from frappe.translate import set_default_language -from frappe.twofactor import toggle_two_factor_auth from frappe.utils import cint, today -from frappe.utils.momentjs import get_all_timezones class SystemSettings(Document): def validate(self): + from frappe.twofactor import toggle_two_factor_auth + enable_password_policy = cint(self.enable_password_policy) and True or False minimum_password_score = cint(getattr(self, "minimum_password_score", 0)) or 0 if enable_password_policy and minimum_password_score <= 0: @@ -71,6 +70,8 @@ class SystemSettings(Document): update_last_reset_password_date() def set_defaults(self): + from frappe.translate import set_default_language + for df in self.meta.get("fields"): if df.fieldtype not in no_value_fields and self.has_value_changed(df.fieldname): frappe.db.set_default(df.fieldname, self.get(df.fieldname)) @@ -92,6 +93,8 @@ def update_last_reset_password_date(): @frappe.whitelist() def load(): + from frappe.utils.momentjs import get_all_timezones + if not "System Manager" in frappe.get_roles(): frappe.throw(_("Not permitted"), frappe.PermissionError) diff --git a/frappe/database/database.py b/frappe/database/database.py index 1f2a1eeba9..a264f39d47 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -17,7 +17,6 @@ from pypika.terms import Criterion, NullValue import frappe import frappe.defaults -import frappe.model.meta from frappe import _ from frappe.database.utils import ( DefaultOrderBy, diff --git a/frappe/defaults.py b/frappe/defaults.py index 0b86e99efa..3bcfbec1ce 100644 --- a/frappe/defaults.py +++ b/frappe/defaults.py @@ -3,7 +3,6 @@ import frappe from frappe.cache_manager import clear_defaults_cache, common_default_keys -from frappe.desk.notifications import clear_notifications from frappe.query_builder import DocType # Note: DefaultValue records are identified by parent (e.g. __default, __global) diff --git a/frappe/desk/form/meta.py b/frappe/desk/form/meta.py index 3ad65f7b13..6c338dbbbc 100644 --- a/frappe/desk/form/meta.py +++ b/frappe/desk/form/meta.py @@ -9,7 +9,6 @@ from frappe.build import scrub_html_template from frappe.model.meta import Meta from frappe.model.utils import render_include from frappe.modules import get_module_path, load_doctype_module, scrub -from frappe.translate import extract_messages_from_code, make_dict_from_messages from frappe.utils import get_html_format from frappe.utils.data import get_link_to_form @@ -260,6 +259,8 @@ class FormMeta(Meta): self.set("__form_grid_templates", templates) def set_translations(self, lang): + from frappe.translate import extract_messages_from_code, make_dict_from_messages + self.set("__messages", frappe.get_lang_dict("doctype", self.name)) # set translations for grid templates diff --git a/frappe/desk/search.py b/frappe/desk/search.py index d347cc188c..c4c11558dd 100644 --- a/frappe/desk/search.py +++ b/frappe/desk/search.py @@ -6,7 +6,9 @@ import json import re import frappe -from frappe import _, is_whitelisted + +# Backward compatbility +from frappe import _, is_whitelisted, validate_and_sanitize_search_inputs from frappe.database.schema import SPECIAL_CHAR_PATTERN from frappe.permissions import has_permission from frappe.utils import cint, cstr, unique @@ -293,22 +295,6 @@ def relevance_sorter(key, query, as_dict): return (cstr(value).casefold().startswith(query.casefold()) is not True, value) -def validate_and_sanitize_search_inputs(fn): - @functools.wraps(fn) - def wrapper(*args, **kwargs): - kwargs.update(dict(zip(fn.__code__.co_varnames, args))) - sanitize_searchfield(kwargs["searchfield"]) - kwargs["start"] = cint(kwargs["start"]) - kwargs["page_len"] = cint(kwargs["page_len"]) - - if kwargs["doctype"] and not frappe.db.exists("DocType", kwargs["doctype"]): - return [] - - return fn(**kwargs) - - return wrapper - - @frappe.whitelist() def get_names_for_mentions(search_term): users_for_mentions = frappe.cache.get_value("users_for_mentions", get_users_for_mentions) diff --git a/frappe/query_builder/utils.py b/frappe/query_builder/utils.py index af2c871e83..f2651904fb 100644 --- a/frappe/query_builder/utils.py +++ b/frappe/query_builder/utils.py @@ -106,27 +106,28 @@ def patch_query_execute(): def prepare_query(query): import inspect - from frappe.utils.safe_exec import check_safe_sql_query - param_collector = NamedParameterWrapper() query = query.get_sql(param_wrapper=param_collector) - if frappe.flags.in_safe_exec and not check_safe_sql_query(query, throw=False): - callstack = inspect.stack() - if len(callstack) >= 3 and ".py" in callstack[2].filename: - # ignore any query builder methods called from python files - # assumption is that those functions are whitelisted already. + if frappe.flags.in_safe_exec: + from frappe.utils.safe_exec import check_safe_sql_query - # since query objects are patched everywhere any query.run() - # will have callstack like this: - # frame0: this function prepare_query() - # frame1: execute_query() - # frame2: frame that called `query.run()` - # - # if frame2 is server script is set as the filename - # it shouldn't be allowed. - pass - else: - raise frappe.PermissionError("Only SELECT SQL allowed in scripting") + if not check_safe_sql_query(query, throw=False): + callstack = inspect.stack() + if len(callstack) >= 3 and ".py" in callstack[2].filename: + # ignore any query builder methods called from python files + # assumption is that those functions are whitelisted already. + + # since query objects are patched everywhere any query.run() + # will have callstack like this: + # frame0: this function prepare_query() + # frame1: execute_query() + # frame2: frame that called `query.run()` + # + # if frame2 is server script is set as the filename + # it shouldn't be allowed. + pass + else: + raise frappe.PermissionError("Only SELECT SQL allowed in scripting") return query, param_collector.get_parameters() builder_class = frappe.qb._BuilderClasss diff --git a/frappe/utils/__init__.py b/frappe/utils/__init__.py index d6b8186a2f..6ffa011ed9 100644 --- a/frappe/utils/__init__.py +++ b/frappe/utils/__init__.py @@ -20,7 +20,6 @@ from collections.abc import ( ) from email.header import decode_header, make_header from email.utils import formataddr, parseaddr -from gzip import GzipFile from typing import Any, Callable, Literal from urllib.parse import quote, urlparse @@ -873,6 +872,8 @@ def gzip_compress(data, compresslevel=9): """Compress data in one shot and return the compressed string. Optional argument is the compression level, in range of 0-9. """ + from gzip import GzipFile + buf = io.BytesIO() with GzipFile(fileobj=buf, mode="wb", compresslevel=compresslevel) as f: f.write(data) @@ -883,6 +884,8 @@ def gzip_decompress(data): """Decompress a gzip compressed string in one shot. Return the decompressed string. """ + from gzip import GzipFile + with GzipFile(fileobj=io.BytesIO(data)) as f: return f.read() diff --git a/frappe/utils/change_log.py b/frappe/utils/change_log.py index 586024f931..3da9c5d757 100644 --- a/frappe/utils/change_log.py +++ b/frappe/utils/change_log.py @@ -5,7 +5,6 @@ import json import os import subprocess # nosec -import requests from semantic_version import Version import frappe @@ -231,6 +230,7 @@ def check_release_on_github(app: str): organization name, if the application exists, otherwise None. """ + import requests from giturlparse import parse from giturlparse.parser import ParserError diff --git a/frappe/utils/data.py b/frappe/utils/data.py index bf999825f4..eeabfad559 100644 --- a/frappe/utils/data.py +++ b/frappe/utils/data.py @@ -2234,3 +2234,11 @@ def add_trackers_to_url(url: str, source: str, campaign: str, medium: str = "ema url_parts[4] = urlencode(query) return urlunparse(url_parts) + + +# This is used in test to count memory overhead of default imports. +def _get_rss_memory_usage(): + import psutil + + rss = psutil.Process().memory_info().rss // (1024 * 1024) + return rss diff --git a/frappe/utils/scheduler.py b/frappe/utils/scheduler.py index 529a3c7bf7..903a8dd081 100755 --- a/frappe/utils/scheduler.py +++ b/frappe/utils/scheduler.py @@ -15,7 +15,6 @@ from typing import NoReturn # imports - module imports import frappe -from frappe.installer import update_site_config from frappe.utils import cint, get_datetime, get_sites, now_datetime from frappe.utils.background_jobs import get_jobs @@ -176,6 +175,8 @@ def _get_last_modified_timestamp(doctype): @frappe.whitelist() def activate_scheduler(): + from frappe.installer import update_site_config + frappe.only_for("Administrator") if frappe.local.conf.maintenance_mode: