Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[POC][IMP] mail, *: post creation messages in batch. #61237

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Feyensv
Copy link
Contributor

@Feyensv Feyensv commented Nov 3, 2020

Even if a creation subtype is specified on the model.

project.task records population is unexpectedly slow.
Performance analysis has shown a major bottleneck in mail, in the mail.thread create override.

  • Followers insertion mail_followers._insert_followers (8%` of the population time)
  • message_post calls (45% of the population time)
    When a specific mail subtype is given for creation on the model (_creation_subtype method), the messages are not logged
    in batch but posted one by one through message_post.

This PR aims to improve the project population performance, mainly by targeting mail related performance problems.

  1. update message_post to allow posting identical messages in batch, on multiple threads (and its overrides).
    And use it in mail.thread.create to post messages in batches, even when specific mail creation subtype are specified, posting
    one batch by mail subtype specified (as some models may use different subtypes for different records of same model).
  2. clean/adapt/improve methods related to message_post related methods (message_post_after_hook, ...) and their overrides:
  • avoid useless operations if possible
  • cleaner and more consistent API & docstrings

Perf gains:

  • message_post: 45% --> 25%
  • creation_subtype: 2% -> 0%

Population Time:
~100s --> ~60s

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@Feyensv Feyensv requested review from a team as code owners November 3, 2020 10:22
@Feyensv Feyensv changed the title [IMP] mail, *: post creation messages in batch. [POC][IMP] mail, *: post creation messages in batch. Nov 3, 2020
@seb-odoo
Copy link
Contributor

seb-odoo commented Nov 3, 2020

Promising change for performance, let me know when it's getting ready for review.

@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses ☑ ci/runbot and removed ☐ ci/runbot labels Nov 3, 2020
@tde-banana-odoo
Copy link
Contributor

I still wonder if it would not be better to first clean some low level methods in mail thread and re-order the way we post and notify before trying to post in batch ... especially that as import does not go through any thread code your performance improvements are mainly useful for populate which is not a production tool.

@C3POdoo C3POdoo added the RD research & development, internal work label Nov 3, 2020
@Feyensv
Copy link
Contributor Author

Feyensv commented Nov 3, 2020

Promising change for performance, let me know when it's getting ready for review.

IMO it's ready (the error in the enterprise build is strange, I launched a rebuild), but we will have to convince tde ^^.

I still wonder if it would not be better to first clean some low level methods in mail thread and re-order the way we post and notify before trying to post in batch ... especially that as import does not go through any thread code your performance improvements are mainly useful for populate which is not a production tool.

IMO, any gain is good to take... populate may be a side tool, but I hope it's gonna be used more in the future (I use it a lot already to test some ORM stuff on large recordsets).
Furthermore, I believe there should be some places where tasks (or any other model with a creation_subtype) are created in batch and the gain could be appreciated there.

@tde-banana-odoo
Copy link
Contributor

Heeee, why closing ?

@Feyensv
Copy link
Contributor Author

Feyensv commented Feb 22, 2022

Why not?
Part of it was merged already (insert_followers perf improvement) and the remaining part might as well be done again from scratch if we really want to resurrect the idea.

@tde-banana-odoo
Copy link
Contributor

Snif.

@Feyensv Feyensv reopened this Feb 22, 2022
@C3POdoo C3POdoo requested a review from a team February 22, 2022 13:56
@Feyensv Feyensv force-pushed the master-populate-perfs-vfe branch 6 times, most recently from d281620 to a5ed8a4 Compare February 28, 2022 10:45
for identical messages posted in multiple threads.
Performance gain of up to 25% of the time in the case of models with
specific mail subtypes (e.g. project task or any model specifying _creation_subtype() method).
When creating projects tasks, _creation_subtype is called once by record
created.

As ref always trigger a database query to verify the record still
exists, by using the cached value instead, we avoid n-1 queries (gaining
between 1 & 2 % of the creation time).
@rdeodoo rdeodoo removed the request for review from a team July 29, 2022 18:33
@Feyensv Feyensv removed the request for review from a team October 25, 2022 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI 🤖 Robodoo has seen passing statuses RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants