From a1b2a69de960c07143f52e59f5a002b8e9b5896c Mon Sep 17 00:00:00 2001 From: Xavier-Do Date: Mon, 28 Feb 2022 14:44:22 +0000 Subject: [PATCH 1/6] [IMP] speedscope: add a constant_time option The profiler can be useful to debug random behaviour, but the frame duration becomes irrelevant in this case. A new option allows to make all sample last exactly one second, making the duration correspond to the number of frame, especially useful for sql and query count debugging. task-2796579 Part-of: odoo/odoo#85525 --- odoo/tools/speedscope.py | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/odoo/tools/speedscope.py b/odoo/tools/speedscope.py index c0ffa03f732..d4b0bf2b7c1 100644 --- a/odoo/tools/speedscope.py +++ b/odoo/tools/speedscope.py @@ -93,6 +93,7 @@ class Speedscope: if sql: self.add_output([key], hide_gaps=True, display_name=f'{key} (no gap)') self.add_output([key], continuous=False, complete=False, display_name=f'{key} (density)') + else: self.add_output([key], display_name=key) return self @@ -144,7 +145,8 @@ class Speedscope: stack_ids.append(self.get_frame_id(frame)) return stack_ids - def process(self, entries, continuous=True, hide_gaps=False, use_context=True): + def process(self, entries, continuous=True, hide_gaps=False, use_context=True, constant_time=False): + # constant_time parameters is mainly usefull to hide temporality when focussing on sql determinism entry_end = previous_end = None if not entries: return [] @@ -157,25 +159,29 @@ class Speedscope: if last_entry['stack']: entries.append({'stack': [], 'start': last_entry['start'] + last_entry.get('time', 0)}) - for entry in entries: - previous_end = entry_end - - if hide_gaps and previous_end: - entry_start = previous_end + for index, entry in enumerate(entries): + if constant_time: + entry_start = close_time = index else: - entry_start = entry['start'] - frames_start + previous_end = entry_end - if previous_end and previous_end > entry_start: - # skip entry if entry starts after another entry end/ - continue + if hide_gaps and previous_end: + entry_start = previous_end + else: + entry_start = entry['start'] - frames_start - if previous_end: - close_time = min(entry_start, previous_end) - else: - close_time = entry_start + if previous_end and previous_end > entry_start: + # skip entry if entry starts after another entry end + continue + + if previous_end: + close_time = min(entry_start, previous_end) + else: + close_time = entry_start + + entry_time = entry.get('time') + entry_end = None if entry_time is None else entry_start + entry_time - entry_time = entry.get('time') - entry_end = None if entry_time is None else entry_start + entry_time entry_stack_ids = self.stack_to_ids( entry['stack'] or [], use_context and entry.get('exec_context'), From 6c25b81eb0e2a3ba7f925438b740e7000b6b396b Mon Sep 17 00:00:00 2001 From: Xavier-Do Date: Mon, 28 Feb 2022 14:50:11 +0000 Subject: [PATCH 2/6] [FIX] crm: make TestLeadAssignPerf deterministic The performances tests in TestLeadAssignPerf have a margin for queries because of some randomness in query counts. Those margins avoid random failures but will hide small increments, making the query count fail randomly in future build. This margin also makes the update of query counts difficult. This commit tries to identify the source of this randomness and proposes some fixes. Sources of randomness ===================== Comparing different executions, the query can differ on two point: 1. During the flush, while updating the lead 1.1 the written user_id can vary (~1 additional query) 1.2 the written convert_date can changes (~4 additional queries) 1.3 the written date_last_stage_update can change (~1 additional query) 2. during _handle_salesmen_assignment 2.1 write/_message_auto_subscribe can change? (~1 additional query) the point 1.2 and 1.3 can be easily reproduced, adding sleep, especially a 0.1 sleep in convert_opportunity Writing different values for date will lead to multiple execute when flushing the records: the orm cannot group records with different values Proposed fixes ============== A. Use `cr.now` instead of `fields.datetime.now` Using the transaction time will avoid randomness linked to the change of second during the transaction. This will fix 1.2 and 1.3 B. Sorting members The members comes from a o2m and the order is not deterministic. During the test with a subset of lead (10 instead of 100), the two last members ('Martin Sales Manager' and 'Orteil Sales Own') can come in different orders, leading to different user_id set on lead. Instead of 4 different users, the lead where sometimes dispatch on 5 different users with this setup. (5 queries in flush() instead of 4) This is fixed by adding a complete order (adding id) on crm.team.members This will solve 1.1 C. The leads are also ordered by id if they have the same probability, since it shouldn't hurt and make the search more deterministic. 2.1 was fixed by B or C or is not fixed, not sure anymore. task-2796579 Part-of: odoo/odoo#85525 --- addons/crm/models/crm_lead.py | 6 +++--- addons/crm/models/crm_team_member.py | 2 +- addons/crm/tests/test_crm_lead_assignment.py | 4 +++- addons/sales_team/models/crm_team_member.py | 2 +- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/addons/crm/models/crm_lead.py b/addons/crm/models/crm_lead.py index 0afa382d710..cf57d385ce2 100644 --- a/addons/crm/models/crm_lead.py +++ b/addons/crm/models/crm_lead.py @@ -321,7 +321,7 @@ class Lead(models.Model): @api.depends('stage_id') def _compute_date_last_stage_update(self): for lead in self: - lead.date_last_stage_update = fields.Datetime.now() + lead.date_last_stage_update = self.env.cr.now() @api.depends('create_date', 'date_open') def _compute_day_open(self): @@ -1526,8 +1526,8 @@ class Lead(models.Model): new_team_id = team_id if team_id else self.team_id.id upd_values = { 'type': 'opportunity', - 'date_open': fields.Datetime.now(), - 'date_conversion': fields.Datetime.now(), + 'date_open': self.env.cr.now(), + 'date_conversion': self.env.cr.now(), } if customer != self.partner_id: upd_values['partner_id'] = customer.id if customer else False diff --git a/addons/crm/models/crm_team_member.py b/addons/crm/models/crm_team_member.py index 75dcf662701..bf6f44ae37f 100644 --- a/addons/crm/models/crm_team_member.py +++ b/addons/crm/models/crm_team_member.py @@ -138,7 +138,7 @@ class TeamMember(models.Model): ['&', '&', ('user_id', '=', False), ('date_open', '=', False), ('team_id', '=', member.crm_team_id.id)] ]) - leads = self.env["crm.lead"].search(lead_domain, order='probability DESC', limit=lead_limit) + leads = self.env["crm.lead"].search(lead_domain, order='probability DESC, id', limit=lead_limit) to_assign = member._get_assignment_quota(work_days=work_days) members_data[member.id] = { diff --git a/addons/crm/tests/test_crm_lead_assignment.py b/addons/crm/tests/test_crm_lead_assignment.py index e528b5a87cb..31cfc7875e8 100644 --- a/addons/crm/tests/test_crm_lead_assignment.py +++ b/addons/crm/tests/test_crm_lead_assignment.py @@ -29,7 +29,9 @@ class TestLeadAssignCommon(TestLeadConvertCommon): # don't mess with existing leads, unlink those assigned to users used here to make tests # repeatable (archive is not sufficient because of lost leads) - cls.env['crm.lead'].with_context(active_test=False).search(['|', ('team_id', '=', False), ('user_id', 'in', cls.sales_teams.member_ids.ids)]).unlink() + + with mute_logger('odoo.models.unlink'): + cls.env['crm.lead'].with_context(active_test=False).search(['|', ('team_id', '=', False), ('user_id', 'in', cls.sales_teams.member_ids.ids)]).unlink() cls.bundle_size = 50 cls.env['ir.config_parameter'].set_param('crm.assignment.commit.bundle', '%s' % cls.bundle_size) cls.env['ir.config_parameter'].set_param('crm.assignment.delay', '0') diff --git a/addons/sales_team/models/crm_team_member.py b/addons/sales_team/models/crm_team_member.py index 9547f327d36..9ba61aad5d1 100644 --- a/addons/sales_team/models/crm_team_member.py +++ b/addons/sales_team/models/crm_team_member.py @@ -9,7 +9,7 @@ class CrmTeamMember(models.Model): _inherit = ['mail.thread'] _description = 'Sales Team Member' _rec_name = 'user_id' - _order = 'create_date ASC' + _order = 'create_date ASC, id' _check_company_auto = True crm_team_id = fields.Many2one( From 44b635200e03ae231ed4fd29962ca6fdb98d2ee1 Mon Sep 17 00:00:00 2001 From: Xavier-Do Date: Wed, 2 Mar 2022 09:00:39 +0000 Subject: [PATCH 3/6] [FIX] crm: adapt test_lead_convert_batch_internals task-2796579 Part-of: odoo/odoo#85525 --- addons/crm/models/crm_lead.py | 4 ++-- addons/crm/tests/test_crm_lead_convert.py | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/addons/crm/models/crm_lead.py b/addons/crm/models/crm_lead.py index cf57d385ce2..2a4f460a4b5 100644 --- a/addons/crm/models/crm_lead.py +++ b/addons/crm/models/crm_lead.py @@ -316,7 +316,7 @@ class Lead(models.Model): @api.depends('user_id') def _compute_date_open(self): for lead in self: - lead.date_open = fields.Datetime.now() if lead.user_id else False + lead.date_open = self.env.cr.now() if lead.user_id else False @api.depends('stage_id') def _compute_date_last_stage_update(self): @@ -852,7 +852,7 @@ class Lead(models.Model): context.setdefault('default_team_id', self.team_id.id) # Set date_open to today if it is an opp default = default or {} - default['date_open'] = fields.Datetime.now() if self.type == 'opportunity' else False + default['date_open'] = self.env.cr.now() if self.type == 'opportunity' else False # Do not assign to an archived user if not self.user_id.active: default['user_id'] = False diff --git a/addons/crm/tests/test_crm_lead_convert.py b/addons/crm/tests/test_crm_lead_convert.py index 52f41b83877..e544f5a8611 100644 --- a/addons/crm/tests/test_crm_lead_convert.py +++ b/addons/crm/tests/test_crm_lead_convert.py @@ -492,8 +492,7 @@ class TestLeadConvertBatch(crm_common.TestLeadConvertMassCommon): @users('user_sales_manager') def test_lead_convert_batch_internals(self): """ Test internals of convert wizard, working in batch mode """ - date = Datetime.from_string('2020-01-20 16:00:00') - self.crm_lead_dt_mock.now.return_value = date + date = self.env.cr.now() lead_w_partner = self.lead_w_partner lead_w_contact = self.lead_w_contact From 4a5dcaae94238e38b262b11e1dbbf0888a2e89ca Mon Sep 17 00:00:00 2001 From: Xavier-Do Date: Wed, 2 Mar 2022 11:25:50 +0000 Subject: [PATCH 4/6] [FIX] crm: adapt query counts task-2796579 Part-of: odoo/odoo#85525 --- addons/crm/tests/test_performances.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/addons/crm/tests/test_performances.py b/addons/crm/tests/test_performances.py index a0342faa2ee..4b7ba29d856 100644 --- a/addons/crm/tests/test_performances.py +++ b/addons/crm/tests/test_performances.py @@ -48,7 +48,7 @@ class TestLeadAssignPerf(TestLeadAssignCommon): leads.flush() with self.with_user('user_sales_manager'): - with self.assertQueryCount(user_sales_manager=1283): # 1277-1283 generally - crm only: 1204 + with self.assertQueryCount(user_sales_manager=1279): self.env['crm.team'].browse(self.sales_teams.ids)._action_assign_leads(work_days=2) # teams assign @@ -92,7 +92,7 @@ class TestLeadAssignPerf(TestLeadAssignCommon): leads.flush() with self.with_user('user_sales_manager'): - with self.assertQueryCount(user_sales_manager=589): # 584-585 generally, sometimes 589 + with self.assertQueryCount(user_sales_manager=584): self.env['crm.team'].browse(self.sales_teams.ids)._action_assign_leads(work_days=2) # teams assign @@ -174,7 +174,7 @@ class TestLeadAssignPerf(TestLeadAssignCommon): leads.flush() with self.with_user('user_sales_manager'): - with self.assertQueryCount(user_sales_manager=6502): # 6494-6502 generally + with self.assertQueryCount(user_sales_manager=6487): self.env['crm.team'].browse(sales_teams.ids)._action_assign_leads(work_days=30) # teams assign From ee4157a3968f5e9ba2ea113a7a77fe4d23934d74 Mon Sep 17 00:00:00 2001 From: Xavier-Do Date: Mon, 7 Mar 2022 07:36:17 +0000 Subject: [PATCH 5/6] [FIX] crm: consistent community and enterprise query count It looks like in enterprise, the value of has_group is already in cache This will lead to an additional query when checking mail.message creation access rights in community. This commit warmup the user _has_group ormcache to avoid an inconsistency between community and enterprise Stack of the additional query: ``` sql('SELECT 1 FROM res_groups_users_rel WHERE uid=%s AND gid IN\n ... (SELECT res_id FROM ir_model_data WHERE module=%s AND name=%s)') (SELECT 1 FROM res_groups_users_rel WHERE uid=29 AND gid IN (SELECT res_id FROM ir_model_data WHERE module='base' AND name='group_user')) execute (odoo/addons/base/models/res_users.py (self._cr.execute("""SELECT 1 FROM res_groups_users_rel WHERE uid=%s AND gid IN):847) _has_group (odoo/tools/cache.py (value = d[key] = self.method(*args, **kwargs)):90) lookup (called at ():2) _has_group (odoo/addons/base/models/res_users.py (return self.with_user(uid)._has_group(group_ext_id)):832) has_group (addons/mail/models/mail_message.py (if not self.env['res.users'].has_group('base.group_user'):):380) check_access_rule (odoo/models.py (records.check_access_rule('create')):4321) _create (odoo/models.py (records = self._create(data_list)):4087) create (odoo/api.py (return create(self, arg)):410) _model_create_multi (called at ():2) create (odoo/addons/base/models/ir_fields.py (recs = super().create(vals_list)):613) create (odoo/api.py (return create(self, arg)):410) _model_create_multi (called at ():2) create (addons/mail/models/mail_message.py (messages = super(Message, self).create(values_list)):599) create (odoo/api.py (return create(self, arg)):410) _model_create_multi (called at ():2) create (addons/mail/models/mail_thread.py (return self.env['mail.message'].create(create_values_list)):2132) _message_create (addons/mail/models/mail_thread.py (new_message = self._message_create(msg_values)):1874) message_post (addons/rating/models/mail_thread.py (message = super(MailThread, self).message_post(**kwargs)):14) message_post (addons/mail/wizard/mail_compose_message.py (ActiveModel.browse(res_id).message_post(**post_params)):321) _action_send_mail (addons/mail/models/mail_thread.py (return composer._action_send_mail(auto_commit=auto_commit)):1964) message_post_with_template (addons/mail/models/mail_thread.py (return record.message_post_with_template(False, **kwargs)):1929) _message_compose_with_view (addons/mail/models/mail_thread.py (self._message_compose_with_view(views_or_xmlid, **kwargs)):1933) message_post_with_view (addons/crm/models/crm_lead.py (opportunities_head.message_post_with_view():1336) _merge_opportunity (addons/crm/models/crm_team.py (merged = lead_duplicates._merge_opportunity(user_id=False, team_id=False, auto_unlink=False, max_length=0)):578) _allocate_leads_deduplicate (addons/crm/models/crm_team.py (assign_res = team._allocate_leads_deduplicate(candidate_lead, duplicates_cache=duplicates_lead_cache)):507) _allocate_leads (addons/crm/models/crm_team.py (teams_data = self._allocate_leads(work_days=work_days)):304) _action_assign_leads test_assign_perf_duplicates (odoo/tools/misc.py (return func(*args, **kwargs)):792) ``` task-2796579 Part-of: odoo/odoo#85525 --- addons/crm/tests/test_performances.py | 1 + 1 file changed, 1 insertion(+) diff --git a/addons/crm/tests/test_performances.py b/addons/crm/tests/test_performances.py index 4b7ba29d856..dd4b58b8227 100644 --- a/addons/crm/tests/test_performances.py +++ b/addons/crm/tests/test_performances.py @@ -48,6 +48,7 @@ class TestLeadAssignPerf(TestLeadAssignCommon): leads.flush() with self.with_user('user_sales_manager'): + self.env['res.users'].has_group('base.group_user') # warmup the cache to avoid inconsistency between community an enterprise with self.assertQueryCount(user_sales_manager=1279): self.env['crm.team'].browse(self.sales_teams.ids)._action_assign_leads(work_days=2) From f50162162cb1c10950326a2eeb950b7eca60fae9 Mon Sep 17 00:00:00 2001 From: Xavier-Do Date: Tue, 22 Mar 2022 07:13:11 +0000 Subject: [PATCH 6/6] [FIX] crm: used cr date for date_closed This is mainly here to avoid future randomness on tests and have a coherent value with previous commits Part-of: odoo/odoo#85525 --- addons/event/models/event_registration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/addons/event/models/event_registration.py b/addons/event/models/event_registration.py index 7a48123bcb5..db2c76af728 100644 --- a/addons/event/models/event_registration.py +++ b/addons/event/models/event_registration.py @@ -89,7 +89,7 @@ class EventRegistration(models.Model): for registration in self: if not registration.date_closed: if registration.state == 'done': - registration.date_closed = fields.Datetime.now() + registration.date_closed = self.env.cr.now() else: registration.date_closed = False