From 444dbf2001815aef49241f00a90cffe52690445e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Voet=20=28ryv=29?= Date: Thu, 2 Mar 2023 15:12:36 +0000 Subject: [PATCH] [FIX] core: avoid quadratic complexity for partial compute methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For a compute store field with new records (e.g. during an 'onchange'), the compute method can be called multiple times on the same records without changing the dependencies. Moreover, it can lead to have N² / 2 complexity for a trivial compute on N records. With partial (where we don't always change the value) compute method: ``` @api.depends('reward') def _compute_has_been_rewarded(self): for rec in self: if rec.reward: rec.has_been_rewarded = 'Yes' ``` If every `reward` of `self` (N records) is `False`, when the ORM needs to recompute `has_been_rewarded` of `self`: the compute will be batched, but only the first record in the batch will be set (to `False`) each time (due to the current fallback - "fallback to null value if compute gives nothing"). This means that we will call the compute method N times, and the compute itself will loop on an average of N/2 records (the prefetch set decreasing at each step). Fix this quadratic behavior by setting the cache to `False` for every record not set during the compute method (instead of just the current record). closes odoo/odoo#142162 X-original-commit: 1604ee983aadc0cbee0cd50cbea2b09572905b04 Signed-off-by: Raphael Collet Signed-off-by: Rémy Voet (ryv) --- .../test_new_api/models/test_new_api.py | 7 +++++++ .../test_new_api/tests/test_new_fields.py | 19 +++++++++++++++++++ odoo/fields.py | 18 +++++++++++------- 3 files changed, 37 insertions(+), 7 deletions(-) diff --git a/odoo/addons/test_new_api/models/test_new_api.py b/odoo/addons/test_new_api/models/test_new_api.py index fd081a49c2a..42bfb114eec 100644 --- a/odoo/addons/test_new_api/models/test_new_api.py +++ b/odoo/addons/test_new_api/models/test_new_api.py @@ -562,6 +562,13 @@ class OrderLine(models.Model): short_field_name = fields.Integer(index=True) very_very_very_very_very_long_field_name_1 = fields.Integer(index=True) very_very_very_very_very_long_field_name_2 = fields.Integer(index=True) + has_been_rewarded = fields.Char(compute='_compute_has_been_rewarded', store=True) + + @api.depends('reward') + def _compute_has_been_rewarded(self): + for rec in self: + if rec.reward: + rec.has_been_rewarded = 'Yes' def unlink(self): # also delete associated reward lines 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 be233a3cbe2..211d3b874a7 100644 --- a/odoo/addons/test_new_api/tests/test_new_fields.py +++ b/odoo/addons/test_new_api/tests/test_new_fields.py @@ -9,6 +9,7 @@ from collections import OrderedDict from datetime import date, datetime, time import io from PIL import Image +from unittest.mock import patch import psycopg2 from odoo import models, fields, Command @@ -4360,6 +4361,24 @@ class TestComputeQueries(common.TransactionCase): self.assertEqual(records.mapped('value1'), [10, 0, 0, 0]) self.assertEqual(records.mapped('value2'), [0, 12, 0, 0]) + def test_partial_compute_batching(self): + """ Create several 'new' records and check that the partial compute + method is called only once. + """ + order = self.env['test_new_api.order'].new({ + 'line_ids': [Command.create({'reward': False})] * 100, + }) + + OrderLine = self.env.registry['test_new_api.order.line'] + with patch.object( + OrderLine, + '_compute_has_been_rewarded', + side_effect=OrderLine._compute_has_been_rewarded, + autospec=True, + ) as patch_compute: + order.line_ids.mapped('has_been_rewarded') + self.assertEqual(patch_compute.call_count, 1) + class test_shared_cache(TransactionCaseWithUserDemo): def test_shared_cache_computed_field(self): diff --git a/odoo/fields.py b/odoo/fields.py index 49077d05161..661a986c246 100644 --- a/odoo/fields.py +++ b/odoo/fields.py @@ -1205,14 +1205,18 @@ class Field(MetaField('DummyField', (object,), {})): self.compute_value(recs) except (AccessError, MissingError): self.compute_value(record) - try: - value = env.cache.get(record, self) - except CacheMiss: + recs = record + + missing_recs_ids = tuple(env.cache.get_missing_ids(recs, self)) + if missing_recs_ids: + missing_recs = record.browse(missing_recs_ids) if self.readonly and not self.store: - raise ValueError(f"Compute method failed to assign {record}.{self.name}") from None - # fallback to null value if compute gives nothing - value = self.convert_to_cache(False, record, validate=False) - env.cache.set(record, self, value) + raise ValueError(f"Compute method failed to assign {missing_recs}.{self.name}") + # fallback to null value if compute gives nothing, do it for every unset record + false_value = self.convert_to_cache(False, record, validate=False) + env.cache.update(missing_recs, self, itertools.repeat(false_value)) + + value = env.cache.get(record, self) elif self.type == 'many2one' and self.delegate and not record.id: # parent record of a new record: new record, with the same