[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 <rco@odoo.com>

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({

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)

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",

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:

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)