From dfa34aa3feee109916a24bf9817f97cd1fbaa4a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Voet=20=28ryv=29?= Date: Wed, 31 Aug 2022 07:48:43 +0000 Subject: [PATCH] [IMP] core: reduce cost of `modified`. For non-stored computed fields, `_modified_triggers` will traverse the tree (at the cost of extra queries) only to know which record to invalidate in cache. But in most cases, these fields have no data in cache, so they can be ignored from the start, which allows us to prune entire subtrees from the merged tree. By example: With simple write on `show_operations` of one `stock.picking.type`, the `_modified_triggers` will fetch every ids (from database) of `stock.picking` and `stock.move` related to this `stock.picking.type` (For `stock.move`, it is because of the `show_operations = fields.Boolean(related='picking_id.picking_type_id.show_operations'`)) In that case, there isn't any data of `show_operations` (`stock.move`) in cache, then there are nothing to invalidate and the `_modified_triggers` cost is high (extra queries/processing) for nothing. Then we cut parts of the tree when we know that they won't invalidate anything (=> if the cache is empty for the field). Also refactor the way to merge trees to be more efficient. Performance improvements: - For all tests at-install done by the runbot, we gain -+ 3.5% of queries. - In the example above, we reduce the number of queries (potentially bottleneck queries in large DB) from 7 to 4. - In term of CPU (without counting time in SQL), the new version is -+ 20 % faster (on install of stock,purchase,mrp and with --test-tags=/stock,/purchase). closes odoo/odoo#76322 task-2780812 closes odoo/odoo#99274 Related: odoo/enterprise#30951 Signed-off-by: Raphael Collet --- .../tests/test_project_billing.py | 6 ++ .../test_new_api/tests/test_new_fields.py | 40 ++---------- .../test_new_api/tests/test_properties.py | 8 +-- odoo/api.py | 10 +++ odoo/models.py | 65 ++++++++++++++----- 5 files changed, 71 insertions(+), 58 deletions(-) diff --git a/addons/sale_timesheet/tests/test_project_billing.py b/addons/sale_timesheet/tests/test_project_billing.py index d3c210172a0..7765541c674 100644 --- a/addons/sale_timesheet/tests/test_project_billing.py +++ b/addons/sale_timesheet/tests/test_project_billing.py @@ -295,6 +295,9 @@ class TestProjectBilling(TestCommonSaleTimesheet): self.assertTrue(task.allow_billable, "Task in project 'task rate' should be billed at task rate") self.assertEqual(task.sale_line_id, self.so1_line_deliver_no_task, "The task should keep the same SOL since the partner_id has not changed when the project of the task has changed.") self.assertEqual(task.partner_id, self.partner_a, "Task created in a project billed on 'employee rate' should have the same customer when it has been created.") + # the `subtask.sale_line_id` is consider to be recompute, + # but the result differ after the write of display_project_id without depend on it + task.flush_model(["sale_line_id"]) # move subtask into task rate project subtask.write({ @@ -379,6 +382,9 @@ class TestProjectBilling(TestCommonSaleTimesheet): }) self.assertEqual(subtask.display_project_id, timesheet2.project_id, "The timesheet is in the subtask project") self.assertFalse(timesheet2.so_line, "The timesheet should not be linked to SOL as it's a non billable project") + # the `subtask.sale_line_id` is consider to be recompute, + # but the result differ after the write of project_id + task.flush_model(["sale_line_id"]) # move task and subtask into task rate project task.write({ diff --git a/odoo/addons/test_new_api/tests/test_new_fields.py b/odoo/addons/test_new_api/tests/test_new_fields.py index 7e64d8c0d99..3eb16dabbbb 100644 --- a/odoo/addons/test_new_api/tests/test_new_fields.py +++ b/odoo/addons/test_new_api/tests/test_new_fields.py @@ -4117,50 +4117,22 @@ class TestModifiedPerformance(common.TransactionCase): cls.env.invalidate_all() # Clean the cache def test_modified_trigger_related(self): - with self.assertQueries([""" - SELECT "test_new_api_modified_line".id - FROM "test_new_api_modified_line" - WHERE ("test_new_api_modified_line"."modified_id" IN %s) - ORDER BY "test_new_api_modified_line"."id" - """, - """ - SELECT "test_new_api_modified_line"."id" AS "id", "test_new_api_modified_line"."modified_id" AS "modified_id" - FROM "test_new_api_modified_line" - WHERE "test_new_api_modified_line".id IN %s - """], flush=False): - # Two queries to fetch the `line_ids` in order to invalidate the cache for `modified_name` - # (even if the cache is empty for this field) + with self.assertQueryCount(0, flush=False): + # No queries because `modified_name` has a empty cache self.modified_a.name = "Other" self.assertEqual(self.modified_line_a.modified_name, 'Other') # check def test_modified_trigger_no_store_compute(self): - with self.assertQueries([""" - SELECT "test_new_api_modified_line"."id" AS "id", "test_new_api_modified_line"."modified_id" AS "modified_id", - "test_new_api_modified_line"."quantity" AS "quantity", "test_new_api_modified_line"."price" AS "price", - "test_new_api_modified_line"."parent_id" AS "parent_id", "test_new_api_modified_line"."create_uid" AS "create_uid", - "test_new_api_modified_line"."create_date" AS "create_date", "test_new_api_modified_line"."write_uid" AS "write_uid", - "test_new_api_modified_line"."write_date" AS "write_date" - FROM "test_new_api_modified_line" - WHERE "test_new_api_modified_line".id IN %s - """], flush=False): - # One query to fetch `modified_id` in order to invalidate `total_quantity` + with self.assertQueryCount(0, flush=False): + # No queries because `total_quantity` has a empty cache self.modified_line_a.quantity = 8 self.assertEqual(self.modified_a.total_quantity, 18) def test_modified_trigger_recursive_empty_cache(self): - with self.assertQueries([""" - SELECT "test_new_api_modified_line"."id" AS "id", "test_new_api_modified_line"."modified_id" AS "modified_id", - "test_new_api_modified_line"."quantity" AS "quantity", "test_new_api_modified_line"."price" AS "price", - "test_new_api_modified_line"."parent_id" AS "parent_id", "test_new_api_modified_line"."create_uid" AS "create_uid", - "test_new_api_modified_line"."create_date" AS "create_date", "test_new_api_modified_line"."write_uid" AS "write_uid", - "test_new_api_modified_line"."write_date" AS "write_date" - FROM "test_new_api_modified_line" - WHERE "test_new_api_modified_line".id IN %s - """], flush=False): - # One query to fetch `parent_id` in order to invalidate `total_price` recursively - # but because no record has already recompute `total_price` no need to invalidate recursivly + with self.assertQueryCount(0, flush=False): + # No queries because `total_price` has a empty cache self.modified_line_a_child_child.price = 4 self.assertEqual(self.modified_line_a.total_price, 7) diff --git a/odoo/addons/test_new_api/tests/test_properties.py b/odoo/addons/test_new_api/tests/test_properties.py index ab552b9e2a5..2486e6d8ca1 100644 --- a/odoo/addons/test_new_api/tests/test_properties.py +++ b/odoo/addons/test_new_api/tests/test_properties.py @@ -905,12 +905,8 @@ class PropertiesCase(TransactionCase): # so the value in cache is not verified (see models.py@_read) self.message_1.attributes - expected = [ - 'SELECT "test_new_api_emailmessage".id FROM "test_new_api_emailmessage" WHERE ("test_new_api_emailmessage"."message" in %s) ORDER BY "test_new_api_emailmessage"."id"', - 'UPDATE "test_new_api_message" SET "attributes" = %s, "write_date" = %s, "write_uid" = %s WHERE id IN %s', - ] - - with self.assertQueryCount(2), self.assertQueries(expected): + expected = ['UPDATE "test_new_api_message" SET "attributes" = %s, "write_date" = %s, "write_uid" = %s WHERE id IN %s'] + with self.assertQueryCount(1), self.assertQueries(expected): self.message_1.attributes = [ { "name": "discussion_color_code", diff --git a/odoo/api.py b/odoo/api.py index 854d597bb26..1b2203ab72c 100644 --- a/odoo/api.py +++ b/odoo/api.py @@ -948,6 +948,16 @@ class Cache(object): """ Return whether ``record`` has a value for ``field``. """ return record.id in self._get_field_cache(record, field) + def contains_field(self, field): + """ Return whether ``field`` has a value for at least one record. """ + cache = self._data.get(field) + if not cache: + return False + # 'cache' keys are tuples if 'field' is context-dependent, record ids otherwise + if isinstance(next(iter(cache)), tuple): + return any(value for value in cache.values()) + return True + def get(self, record, field, default=NOTHING): """ Return the value of ``field`` for ``record``. """ try: diff --git a/odoo/models.py b/odoo/models.py index d073f6d486c..48c84673333 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -132,15 +132,33 @@ def fix_import_export_id_paths(fieldname): fixed_external_id = re.sub(r'([^/]):id', r'\1/id', fixed_db_id) return fixed_external_id.split('/') -def trigger_tree_merge(node1, node2): - """ Merge two trigger trees. """ - for key, val in node2.items(): - if key is None: - node1.setdefault(None, OrderedSet()) - node1[None].update(val) - else: - node1.setdefault(key, {}) - trigger_tree_merge(node1[key], node2[key]) +def merge_trigger_trees(trees: list, select=bool) -> dict: + """ Merge trigger trees list into a final tree. The function ``select`` is + called on every field to determine which fields should be kept in the tree + nodes. This enables to discard some fields from the tree nodes. + """ + result_tree = {} # the resulting tree + root_fields = OrderedSet() # the fields in the root node + subtrees_to_merge = defaultdict(list) # the subtrees to merge grouped by key + + for tree in trees: + for key, val in tree.items(): + if key is None: + root_fields.update(val) + else: + subtrees_to_merge[key].append(val) + + # the root node contains the collected fields for which select is true + root_node = [field for field in root_fields if select(field)] + if root_node: + result_tree[None] = root_node + + for key, subtrees in subtrees_to_merge.items(): + subtree = merge_trigger_trees(subtrees, select) + if subtree: + result_tree[key] = subtree + + return result_tree class MetaModel(api.Meta): @@ -6260,15 +6278,26 @@ class BaseModel(metaclass=MetaModel): # - mark H to recompute on inverse(X, records), # - mark I to recompute on inverse(W, inverse(X, records)), # - mark J to recompute on inverse(Y, records). - if len(fnames) == 1: - tree = self.pool.field_triggers.get(self._fields[next(iter(fnames))]) - else: - # merge dependency trees to evaluate all triggers at once - tree = {} - for fname in fnames: - node = self.pool.field_triggers.get(self._fields[fname]) - if node: - trigger_tree_merge(tree, node) + fields = [self._fields[fname] for fname in fnames] + field_triggers = self.pool.field_triggers + trees = [field_triggers[field] for field in fields if field in field_triggers] + + if not trees: + return + + cache = self.env.cache + + # Merge dependency trees to evaluate all triggers at once. + # For non-stored computed fields, `_modified_triggers` might traverse + # the tree (at the cost of extra queries) only to know which records to + # invalidate in cache. But in many cases, most of these fields have no + # data in cache, so they can be ignored from the start. This allows us + # to discard subtrees from the merged tree when they only contain such + # fields. + tree = merge_trigger_trees( + trees, + select=lambda field: (field.compute and field.store) or cache.contains_field(field), + ) if tree: # determine what to compute (through an iterator)