From 8d3940eab9e18b643cc7c955430bbbe95a7c6455 Mon Sep 17 00:00:00 2001 From: Denis Ledoux Date: Mon, 12 Dec 2022 13:14:24 +0000 Subject: [PATCH] [IMP] base: view validation performance, batch action and group exist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The main goal is to reduce the processing time it takes for views to be validated during installation. In order to mimic the behavior of the view creations during installation, and to analyze the performance, I used the below code in the shell: ```py env.cr.rollback() domain = [('model', '=', 'res.config.settings')] views = self.env['ir.ui.view'].search_read(domain, [ 'id', 'active', 'arch', 'inherit_id', 'key', 'mode', 'model', 'priority', 'type', 'xml_id' ], order="id ASC") for view in views: if view['inherit_id']: view['inherit_id'] = self.env['ir.ui.view'].browse(view['inherit_id'][0]).xml_id self.env['ir.ui.view'].browse(reversed([view['id'] for view in views])).unlink() with odoo.tools.profiler.Profiler(): for view in views: new_view = self.env['ir.ui.view'].create({ key: self.env.ref(value).id if key in ['inherit_id'] and value else value for key, value in view.items() if key not in ['id', 'xml_id'] }) if view['xml_id']: module, name = view['xml_id'].split('.') self.env['ir.model.data'].create({ 'module': module, 'name': name, 'model': 'ir.ui.view', 'res_id': new_view.id, }) env.cr.rollback() ``` Using the above, and analyzing the speedscope profile, we can see the lines ```py action = self.env['ir.actions.actions'].browse(action_id).exists() ``` from `_validate_tag_button`, and ```py if not self.env['ir.model.data']._xmlid_to_res_id(group.strip(), raise_if_not_found=False): ``` from `_validate_attrs` take a significant time. Batching them the actions and groups existence (as recommended as further improvement in the comment which is deleted by this revision), as well as a more efficient algorithm for `_get_node_groups`, reduce the processing time of the views validation. On my computer, the above code creating all the views for `res.config.settings`, which is an extreme case because the view has 100 inherited views, allows to reduce the validation procesing time from 9.42s to 8.18s. I include in the pull request the speedscope profiles screenshots. Regarding the algorithm for `_get_node_groups` itself: ```py In [0]: %timeit tuple(tuple(n.get('groups').split(',')) for n in node.xpath('ancestor-or-self::*[@groups]')) 10.1 µs ± 91 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each) In [1]: %timeit groups = [tuple(n.get('groups').split(',')) for n in chain([node], node.iterancestors()) if n.get('groups')];groups.reverse();tuple(groups) 3.46 µs ± 29.2 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each) ``` closes odoo/odoo#107742 Signed-off-by: Denis Ledoux (dle) --- odoo/addons/base/models/ir_ui_view.py | 70 ++++++++++++++++----------- odoo/addons/base/tests/test_views.py | 51 ++++++++++++++++++- 2 files changed, 92 insertions(+), 29 deletions(-) diff --git a/odoo/addons/base/models/ir_ui_view.py b/odoo/addons/base/models/ir_ui_view.py index 7bccafe400a..48d051a24c2 100644 --- a/odoo/addons/base/models/ir_ui_view.py +++ b/odoo/addons/base/models/ir_ui_view.py @@ -14,6 +14,8 @@ import time import uuid import warnings +from itertools import chain + from lxml import etree from lxml.etree import LxmlError from lxml.builder import E @@ -1606,28 +1608,7 @@ actual arch. msg = "%s on %s has parameters and cannot be called from a button" self._log_view_warning(msg % (name, name_manager.model._name), node) elif type_ == 'action': - # logic mimics /web/action/load behaviour - action = False - try: - action_id = int(name) - except ValueError: - model, action_id = self.env['ir.model.data']._xmlid_to_res_model_res_id(name, raise_if_not_found=False) - if not action_id: - msg = _("Invalid xmlid %(xmlid)s for button of type action.", xmlid=name) - self._raise_view_error(msg, node) - if not issubclass(self.pool[model], self.pool['ir.actions.actions']): - msg = _( - "%(xmlid)s is of type %(xmlid_model)s, expected a subclass of ir.actions.actions", - xmlid=name, xmlid_model=model, - ) - self._raise_view_error(msg, node) - action = self.env['ir.actions.actions'].browse(action_id).exists() - if not action: - msg = _( - "Action %(action_reference)s (id: %(action_id)s) does not exist for button of type action.", - action_reference=name, action_id=action_id, - ) - self._raise_view_error(msg, node) + name_manager.must_exist_action(name, node) name_manager.has_action(name) @@ -1791,11 +1772,7 @@ actual arch. elif attr == 'groups': for group in expr.replace('!', '').split(','): - # further improvement: add all groups to name_manager in - # order to batch check them at the end - if not self.env['ir.model.data']._xmlid_to_res_id(group.strip(), raise_if_not_found=False): - msg = "The group %r defined in view does not exist!" - self._log_view_warning(msg % group, node) + name_manager.must_exist_group(group.strip(), node) elif attr in ('col', 'colspan'): # col check is mainly there for the tag 'group', but previous @@ -2846,6 +2823,8 @@ class NameManager: self.mandatory_fields = collections.defaultdict(dict) # {field_name: {'groups': 'use}} self.mandatory_parent_fields = collections.defaultdict(dict) # {field_name: {'groups': use}} self.mandatory_names = dict() # {name: use} + self.must_exist_actions = {} + self.must_exist_groups = {} self.parent = parent self.children = [] if self.parent: @@ -2888,8 +2867,14 @@ class NameManager: def must_have_name(self, name, use): self.mandatory_names[name] = use + def must_exist_action(self, action_id, node): + self.must_exist_actions[action_id] = node + + def must_exist_group(self, name, node): + self.must_exist_groups[name] = node + def _get_node_groups(self, node): - return tuple(tuple(n.get('groups').split(',')) for n in node.xpath('ancestor-or-self::*[@groups]')) + return tuple(tuple(n.get('groups').split(',')) for n in chain([node], node.iterancestors()) if n.get('groups')) def check(self, view): # context for translations below @@ -2908,6 +2893,35 @@ class NameManager: message = _("Field `%(name)s` does not exist", name=name) view._raise_view_error(message) + for name, node in self.must_exist_actions.items(): + # logic mimics /web/action/load behaviour + action = False + try: + action_id = int(name) + except ValueError: + model, action_id = view.env['ir.model.data']._xmlid_to_res_model_res_id(name, raise_if_not_found=False) + if not action_id: + msg = _("Invalid xmlid %(xmlid)s for button of type action.", xmlid=name) + view._raise_view_error(msg, node) + if not issubclass(view.pool[model], view.pool['ir.actions.actions']): + msg = _( + "%(xmlid)s is of type %(xmlid_model)s, expected a subclass of ir.actions.actions", + xmlid=name, xmlid_model=model, + ) + view._raise_view_error(msg, node) + action = view.env['ir.actions.actions'].browse(action_id).exists() + if not action: + msg = _( + "Action %(action_reference)s (id: %(action_id)s) does not exist for button of type action.", + action_reference=name, action_id=action_id, + ) + view._raise_view_error(msg, node) + + for name, node in self.must_exist_groups.items(): + if not view.env['ir.model.data']._xmlid_to_res_id(name, raise_if_not_found=False): + msg = _("The group %(name)r defined in view does not exist!", name=name) + view._log_view_warning(msg, node) + for name, groups_uses in self.mandatory_fields.items(): use = next(iter(groups_uses.values())) if name == 'id': # always available diff --git a/odoo/addons/base/tests/test_views.py b/odoo/addons/base/tests/test_views.py index 92aaeb2da7e..4ee19f5d3b6 100644 --- a/odoo/addons/base/tests/test_views.py +++ b/odoo/addons/base/tests/test_views.py @@ -14,7 +14,7 @@ from psycopg2.extras import Json from odoo.exceptions import AccessError, ValidationError from odoo.tests import common -from odoo.tools import mute_logger, view_validation +from odoo.tools import get_cache_key_counter, mute_logger, view_validation from odoo.addons.base.models.ir_ui_view import ( transfer_field_to_modifiers, transfer_node_to_modifiers, simplify_modifiers, ) @@ -315,6 +315,55 @@ class TestViewInheritance(ViewCase): # 3: _combine: arch_db self.view_ids['A'].get_combined_arch() + def test_view_validate_button_action_query_count(self): + _, _, counter = get_cache_key_counter(self.env['ir.model.data']._xmlid_lookup, 'base.action_ui_view') + hit, miss = counter.hit, counter.miss + + with self.assertQueryCount(8): + base_view = self.assertValid(""" +
+
+
+ + + """) + self.assertEqual(counter.hit, hit) + self.assertEqual(counter.miss, miss + 2) + + with self.assertQueryCount(9): + self.assertValid(""" + + """, inherit_id=base_view.id) + self.assertEqual(counter.hit, hit) + self.assertEqual(counter.miss, miss + 4) + + def test_view_validate_attrs_groups_query_count(self): + _, _, counter = get_cache_key_counter(self.env['ir.model.data']._xmlid_lookup, 'base.group_system') + hit, miss = counter.hit, counter.miss + + with self.assertQueryCount(5): + base_view = self.assertValid(""" +
+ + + + + """) + self.assertEqual(counter.hit, hit) + self.assertEqual(counter.miss, miss + 1) + + with self.assertQueryCount(6): + self.assertValid(""" + + + + """, inherit_id=base_view.id) + self.assertEqual(counter.hit, hit) + self.assertEqual(counter.miss, miss + 2) + class TestApplyInheritanceSpecs(ViewCase): """ Applies a sequence of inheritance specification nodes to a base