[IMP] account,hr_expense: allow ORM to prefetch attachments #117074

william-andre · 2023-03-29T19:54:50Z

Instead of having to search record by record thanks to the domain returned by _get_attachment_domains, we can use the ORM to fetch all the records at once.

Benchmark

Speedup of 5x-6x (93ms to 16ms for 100 invoices)

Before

After

Setup

Here is my quick benchmark code:
Setup:

for i in range(100): self.env['account.move'].search([('move_type', '=', 'out_invoice')], limit=1).copy()
invoices = self.env['account.move'].search([('move_type', '=', 'out_invoice')])
invoices.button_draft()
invoices.action_post()
self.env['account.move.send'].create({ 
    'move_ids' : invoices, 
    'checkbox_send_mail': True, 
    'checkbox_download': True, 
}).action_send_and_print()
self.env.ref('account.ir_cron_account_move_send').method_direct_trigger()  # can't do this in a shell without another instance running because of wkhtmltopdf

Test:

import time
self.env.invalidate_all(); start = time.time(); invoices.line_ids.move_attachment_ids; print(time.time() - start)

robodoo · 2023-03-29T19:55:09Z

Pull request status dashboard.

rco-odoo

I just saw that you abuse the ORM's field attribute automatic for one accounting widget https://github.com/odoo/enterprise/pull/38991/files#diff-ee342c09ffb6b8de7f51c4a0ed66fee056e8975e7416d0f85ae0d2b6b1883dfdR204-R205. Please don't do that.

addons/account/models/account_bank_statement.py

rco-odoo · 2023-05-30T07:36:24Z

odoo/addons/base/models/ir_attachment.py

+    linked_attachment_ids = fields.One2many(
+        comodel_name='ir.attachment',
+        compute='_compute_linked_attachment_ids',
+        groups="base.group_user",
+        automatic=True,
+    )


Please explain the presence of groups and automatic. What is their added value?
Please also justify polluting the field space of all models for optimizing one model.
It really looks like you are tweaking the framework for the accounting modules' needs.

The groups is basically because of this (and other similar access rights checks); it would raise errors in some tests because the field would be displayed otherwise, without enough rights to do so.
https://github.com/odoo/odoo/blob/e4a09ce62e165b8a33797702f89695debee15968/odoo/addons/base/models/ir_attachment.py#L445-L446

I will push a version without the automatic, but as I understand it, it is there for the same reasons as for display_name

I have pushed a version without those attributes set, here is the result:
https://runbot.odoo.com/runbot/batch/1062568/build/41373652

rco-odoo · 2023-05-30T08:02:02Z

@william-andre adding a field on all models slows down the loading of the registry. That's why we removed the magic field __last_update in #105739.

Moreover, I just realized that your computed field has no dependency, and therefore has no cache consistency. That's not cool. I guess that's why you needed to add explicit cache invalidation in a test...

So why don't you simply add a method _get_attachments() on the base mixin model in ir.attachment ? This would possibly solve your problem without the downsides I mentioned above. The only thing an actual field adds is the cache for the data. I don't know whether this has an impact in your case...

odoo/addons/base/models/ir_attachment.py

rco-odoo · 2023-05-30T08:15:24Z

See https://github.com/odoo/enterprise/pull/38991#discussion_r1209891985.
I think that's the real source of the problem.

rco-odoo · 2023-06-02T09:42:50Z

odoo/addons/base/models/ir_attachment.py

+
+
+class LinkAttachmentMixin(models.AbstractModel):
+    _name = 'link.attachment.mixin'


The name is confusing: why "link" ? Simply follow conventions...

Suggested change

_name = 'link.attachment.mixin'

_name = 'ir.attachment.mixin'

ir.attachment.mixin doesn't mean anything to me 😕
Maybe ir.attachment.link? My thinking is that this mixin adds a link between ir.attachment and the model inheriting the mixin

rco-odoo · 2023-06-02T09:54:05Z

odoo/addons/base/models/ir_attachment.py

+    linked_attachment_ids = fields.One2many(
+        comodel_name='ir.attachment',
+        compute='_compute_linked_attachment_ids',
+        inverse='_inverse_linked_attachment_ids',
+    )


If you use the following definition, invalidation is automatic:

Suggested change

linked_attachment_ids = fields.One2many(

comodel_name='ir.attachment',

compute='_compute_linked_attachment_ids',

inverse='_inverse_linked_attachment_ids',

)

linked_attachment_ids = fields.One2many(

'ir.attachment', 'res_id',

domain=lambda self: [('res_model', '=', self._name)],

readonly=True, string="Attachments",

)

No need to compute, no need to add explicit invalidation in model ir.attachment: all those things are supported by the framework. This solution's performance should be very close to yours.

I recommend to not modify attachments through this field. There's a widget for that.

A lot of tests won't pass with your suggestion.

I recommend to not modify attachments through this field. There's a widget for that.

Which widget?

Please don't try to add magic on attachments. It is sufficiently complicated without adding yet another layer of issues (ACLs, res_model with chatter, ...) This seems like a good idea, but ends up like a bad idea to me.

It is simplifying all the places where attachments are being manipulated manually, avoiding to do manual work where one could do mistakes...

Also, I am not changing anything related to ACLs, everything is done using the ORM and public methods.

tde-banana-odoo

A bit like @rco-odoo , agree we could have a getter (or a field if you really want it), but don't allow updating / writing to those fields. I bet you are going to break a lot of cases in chatter / mailing, because res_model / res_id has a lot of complex use cases (linked to thread and not the message, because of document-related models, because you change the way ACLs will be managed on attachments, ...)

I don't see the added value of this PR compared to the fragility of current attachment model, that is worsened by this PR.

Cheers,

tde-banana-odoo · 2023-06-02T13:39:27Z

addons/mail/models/mail_activity.py

+                message_attachments = activity.linked_attachment_ids
+                if message_attachments:
+                    activity_message.attachment_ids = message_attachments
+                    activity_message.linked_attachment_ids += message_attachments


The new code is less clear than the old code. It was explicit and clea. This one just pill up some "attachment_ids" but we don't understand what it is doing.

I very strongly disagree, but it's your ownership I guess

This reads as simple as this, in only 4 lines

if there are attachments linked to the activity, link them to the activity message

Before it read like this, with code scattered with a span of 44 lines

get all the attachments linked to my record, group them by activity, if there are some for my activity, link them to the activity message

tde-banana-odoo · 2023-06-02T13:40:24Z

addons/mail/models/mail_message.py

@@ -65,6 +65,7 @@ class Message(models.Model):
    information.
    """
    _name = 'mail.message'
+    _inherit = ['link.attachment.mixin']


Actually I would prefer not having it on message. Attachments are generally linked to the thread, not the message.

tde-banana-odoo · 2023-06-02T13:41:51Z

odoo/addons/base/models/ir_attachment.py

+    linked_attachment_ids = fields.One2many(
+        comodel_name='ir.attachment',
+        compute='_compute_linked_attachment_ids',
+        inverse='_inverse_linked_attachment_ids',
+    )


Please don't try to add magic on attachments. It is sufficiently complicated without adding yet another layer of issues (ACLs, res_model with chatter, ...) This seems like a good idea, but ends up like a bad idea to me.

william-andre · 2023-06-13T14:36:34Z

@rco-odoo @tde-banana-odoo
Does it seem more logical now?
I basically removed the mixin and made the o2m fields work as I would expect them to work when they are related to a many2one_reference (see the new test class)
https://github.com/odoo/odoo/blob/096b6efd7bbbb5ddd9c89ce06bc4c6bf046992d2/odoo/addons/test_new_api/tests/test_new_fields.py#L3562-L3584

These would be failing in general, not only for the new fields linked_attachment_ids that I'm adding here, of course.

Currently, there is no invalidation whatsoever when writing on a one2many field that has a many2one_reference inverse. This can lead to weird behavior, such as * a record not being added to the field when updated through the ORM * the cache not being updated when changing the reference of the related field

… move line Before, we had to do one search per line instead of batching everything in one query per model. This leads to a linear time complexity to display the list view of Journal Entries.

This could have lead to incoherences with the security rules of the related attachment.

Instead of searching all the related attachments of the related model and then filtering them per record, use the ORM to already batch them per record. O(n^2) to O(n)

Instead of doing one search per expense to fetch the attchments, rely on the ORM to prefetch everything. Also create the attachments in batch when splitting an expense.

Everything works fine using a one2many field related to a many2one_reference out of the box. No need to complicate everything with a proxy field.

Instead of doing a manual search and grouping of the attachments, rely on the ORM to do the labour.

This reverts commit 6679232. Performances shouldn't be fixed by duplicating all the attachments.

pivi-odoo · 2024-02-26T15:48:50Z

Hi @william-andre,
I see that you are trying to revert the commit introduced in #151183 but I think this is a hasty decision. It's a bugfix (even if initially it started as a perf investigation). There were other tickets in parrallel showing this same issue (OPW-3432427). The patch was made with validation from the PO.
I invite you to read the discussion on the PR for more context on why we ended up doing this duplication. TLDR: the res_model on the attachments of the expense sheet were hr.expense, not hr.expense.sheet.

C3POdoo added the RD research & development, internal work label Mar 29, 2023

william-andre force-pushed the master-move-line-attachment-perf-wan branch from ba23285 to aee66a2 Compare March 31, 2023 12:06

william-andre force-pushed the master-move-line-attachment-perf-wan branch 10 times, most recently from 8bd3a13 to e4a09ce Compare May 25, 2023 21:19

william-andre marked this pull request as ready for review May 26, 2023 09:51

C3POdoo requested review from a team, VincentSchippefilt and ryv-odoo and removed request for a team May 26, 2023 09:53

seb-odoo removed the request for review from a team May 26, 2023 11:27

rco-odoo requested changes May 30, 2023

View reviewed changes

ryv-odoo reviewed May 30, 2023

View reviewed changes

odoo/addons/base/models/ir_attachment.py Outdated Show resolved Hide resolved

odoo/addons/base/models/ir_attachment.py Outdated Show resolved Hide resolved

william-andre force-pushed the master-move-line-attachment-perf-wan branch 4 times, most recently from a265d3e to e30aad0 Compare May 30, 2023 11:39

william-andre force-pushed the master-move-line-attachment-perf-wan branch from 6dd3aa6 to 8db2fe2 Compare June 1, 2023 12:54

rco-odoo requested changes Jun 2, 2023

View reviewed changes

tde-banana-odoo requested changes Jun 2, 2023

View reviewed changes

william-andre force-pushed the master-move-line-attachment-perf-wan branch 5 times, most recently from 995dfdd to b365545 Compare June 9, 2023 12:42

william-andre force-pushed the master-move-line-attachment-perf-wan branch 9 times, most recently from 65b0cd0 to 096b6ef Compare June 13, 2023 14:31

william-andre force-pushed the master-move-line-attachment-perf-wan branch 2 times, most recently from f9c6656 to 749e8a4 Compare June 14, 2023 11:14

william-andre added 7 commits February 26, 2024 15:26

[IMP] account,hr_expense: improve perfs to fetch the attachments of a…

2ccd1f2

… move line Before, we had to do one search per line instead of batching everything in one query per model. This leads to a linear time complexity to display the list view of Journal Entries.

[IMP] account: remove useless m2m relation table between attachments

2593507

This could have lead to incoherences with the security rules of the related attachment.

[FIX] crm: performance while searching for attachments

40be074

Instead of searching all the related attachments of the related model and then filtering them per record, use the ORM to already batch them per record. O(n^2) to O(n)

[FIX] hr_expense: improve performances while working with attachments

13ce019

Instead of doing one search per expense to fetch the attchments, rely on the ORM to prefetch everything. Also create the attachments in batch when splitting an expense.

[IMP] hr_holiday: remove proxy fields for attachment managing.

6e2e622

Everything works fine using a one2many field related to a many2one_reference out of the box. No need to complicate everything with a proxy field.

[REF] mail: manage attachments through ORM

4f2af4b

Instead of doing a manual search and grouping of the attachments, rely on the ORM to do the labour.

william-andre force-pushed the master-move-line-attachment-perf-wan branch from 749e8a4 to 4f2af4b Compare February 26, 2024 14:44

[REV] hr_expense: set main attachment on hr.expense.sheet"

28f5238

This reverts commit 6679232. Performances shouldn't be fixed by duplicating all the attachments.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IMP] account,hr_expense: allow ORM to prefetch attachments #117074

[IMP] account,hr_expense: allow ORM to prefetch attachments #117074

william-andre commented Mar 29, 2023 •

edited

robodoo commented Mar 29, 2023

rco-odoo left a comment

rco-odoo May 30, 2023

william-andre May 30, 2023

william-andre May 30, 2023

rco-odoo commented May 30, 2023

rco-odoo commented May 30, 2023

rco-odoo Jun 2, 2023

william-andre Jun 2, 2023

rco-odoo Jun 2, 2023

william-andre Jun 2, 2023

tde-banana-odoo Jun 2, 2023

william-andre Jun 2, 2023

william-andre Jun 2, 2023

tde-banana-odoo left a comment

tde-banana-odoo Jun 2, 2023

william-andre Jun 2, 2023

tde-banana-odoo Jun 2, 2023

tde-banana-odoo Jun 2, 2023

william-andre commented Jun 13, 2023

pivi-odoo commented Feb 26, 2024



		class LinkAttachmentMixin(models.AbstractModel):
		_name = 'link.attachment.mixin'

	_name = 'link.attachment.mixin'
	_name = 'ir.attachment.mixin'

[IMP] account,hr_expense: allow ORM to prefetch attachments #117074

Are you sure you want to change the base?

[IMP] account,hr_expense: allow ORM to prefetch attachments #117074

Conversation

william-andre commented Mar 29, 2023 • edited

Benchmark

Before

After

Setup

robodoo commented Mar 29, 2023

rco-odoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rco-odoo commented May 30, 2023

rco-odoo commented May 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tde-banana-odoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

william-andre commented Jun 13, 2023

pivi-odoo commented Feb 26, 2024

william-andre commented Mar 29, 2023 •

edited