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

Fix prefetch performance on computed fields (especially related/inherited fields) #18644

Merged
merged 4 commits into from Aug 10, 2017

Conversation

Projects
None yet
10 participants
@xmo-odoo
Collaborator

xmo-odoo commented Aug 2, 2017

When deciding to prefetch records (getting records from the cache with no value for the field being fetched), if the field was computed determine_value would just get all records, not limited by the normal
prefetch limit, for large recordsets this would generate gigantic prefetch lists for records we may not need at all.

Fix by applying the PREFETCH_MAX limit to records from the cache as is done in _prefetch_field.

Complementarily, when traversing related fields the prefetch environment would be lost and every record would get an empty prefetch environment, so the values would ultimately be read one by one.

Example: select (search) 1000 product.product records, access a related field (e.g. categ_id) in a loop, on the first iteration the system would first read 1000 templates, then it would read each categ_id individually, resulting in >1000 SQL queries rather than the ~2 we would expect.

Fixes #18511

[FIX] prefetch issues on computed/related fields
When deciding to prefetch records (getting records from the cache with
no value for the field being fetched), if the field was computed
determine_value would just get all records, not limited by the normal
prefetch limit, for large recordsets this would generate gigantic
prefetch lists for records we may not need at all.

Fix by applying the PREFETCH_MAX limit to records from the cache as is
done in _prefetch_field.

Complementarily, when traversing related fields the prefetch
environment would be lost and every record would get an empty prefetch
environment, so the values would ultimately be read one by one.

Example: select (search) 1000 product.product records, access a
related field (e.g. categ_id) in a loop, on the first iteration the
system would first read 1000 templates, then it would read each
categ_id individually, resulting in >1000 SQL queries rather than the
~2 we would expect.

Fixes #18511
Show outdated Hide outdated odoo/fields.py
@@ -565,7 +565,7 @@ def traverse_related(self, record):
""" Traverse the fields of the related field `self` except for the last
one, and return it as a pair `(last_record, last_field)`. """
for name in self.related[:-1]:
record = record[name][:1]
record = record[name][:1].with_prefetch(record._prefetch)

This comment has been minimized.

@odony

odony Aug 2, 2017

Contributor

👍 That subtle slice (for traversing x2m) had the unexpected side-effect of dropping the prefetch set.
This is consistent with the spirit of 8bc7d85 and the prefetch propagation for relationships (x2m and m2o)

@odony

odony Aug 2, 2017

Contributor

👍 That subtle slice (for traversing x2m) had the unexpected side-effect of dropping the prefetch set.
This is consistent with the spirit of 8bc7d85 and the prefetch propagation for relationships (x2m and m2o)

This comment has been minimized.

@rco-odoo

rco-odoo Aug 9, 2017

Member

Well done! I am glad that the prefetching API makes this fix simple and clear!

@rco-odoo

rco-odoo Aug 9, 2017

Member

Well done! I am glad that the prefetching API makes this fix simple and clear!

@odony

odony approved these changes Aug 2, 2017

Looks good, would be great to have feedback from people affected by #18511

@odony odony changed the title from Fix prefetch issue on computed fields (including related) to Fix prefetch performance on computed fields (especially related/inherited fields) Aug 2, 2017

@Yenthe666

This comment has been minimized.

Show comment
Hide comment
@Yenthe666

Yenthe666 Aug 3, 2017

Contributor

I'll do a test today - will give you feedback by tommorow.

Contributor

Yenthe666 commented Aug 3, 2017

I'll do a test today - will give you feedback by tommorow.

@rvalyi

This comment has been minimized.

Show comment
Hide comment
@rvalyi

rvalyi Aug 3, 2017

Contributor

does it affect v8 too?

Contributor

rvalyi commented Aug 3, 2017

does it affect v8 too?

@pedrobaeza

This comment has been minimized.

Show comment
Hide comment
@pedrobaeza
Contributor

pedrobaeza commented Aug 3, 2017

@C3POdoo C3POdoo added the RD label Aug 3, 2017

@Cedric-Pigeon

This comment has been minimized.

Show comment
Hide comment
@Cedric-Pigeon

Cedric-Pigeon Aug 3, 2017

Contributor

@xmo-odoo I confirm I still have an issue when you traverse several relations on the same recordset.

1/ Install a database with account module

2/ Run the following code through shell command:

# -*- coding: utf-8 -*-
import logging

_logger = logging.getLogger(__name__)
self = self  # noqa

nb_product = 5000

i = 0
taxe = self.env['account.tax'].search([], limit=1)
while i < nb_product:
    prod = self.env['product.template'].with_context(
        tracking_disable=True).create(
        {'name': 'product_%s' % i,
         'taxes_id': [(6, 0, [taxe.id])]})
    i += 1
    if i % 1000 == 0:
        self.env.cr.commit()
        _logger.info("%s Products Created" % i)

2/ Run this python code:

# -*- coding: utf-8 -*-
import logging

_logger = logging.getLogger(__name__)
self = self  # noqa


products = self.env['product.product'].sudo(self.env.ref(
    'base.user_demo')).search([])

for product in products:
    _logger.info('Taxe Variant %s' % product.taxes_id.mapped('name'))
    _logger.info('Category Variant %s' % product.categ_id.name)

Normally we should have 6 queries on the database but we got more than 5000.

Contributor

Cedric-Pigeon commented Aug 3, 2017

@xmo-odoo I confirm I still have an issue when you traverse several relations on the same recordset.

1/ Install a database with account module

2/ Run the following code through shell command:

# -*- coding: utf-8 -*-
import logging

_logger = logging.getLogger(__name__)
self = self  # noqa

nb_product = 5000

i = 0
taxe = self.env['account.tax'].search([], limit=1)
while i < nb_product:
    prod = self.env['product.template'].with_context(
        tracking_disable=True).create(
        {'name': 'product_%s' % i,
         'taxes_id': [(6, 0, [taxe.id])]})
    i += 1
    if i % 1000 == 0:
        self.env.cr.commit()
        _logger.info("%s Products Created" % i)

2/ Run this python code:

# -*- coding: utf-8 -*-
import logging

_logger = logging.getLogger(__name__)
self = self  # noqa


products = self.env['product.product'].sudo(self.env.ref(
    'base.user_demo')).search([])

for product in products:
    _logger.info('Taxe Variant %s' % product.taxes_id.mapped('name'))
    _logger.info('Category Variant %s' % product.categ_id.name)

Normally we should have 6 queries on the database but we got more than 5000.

@KangOl

This comment has been minimized.

Show comment
Hide comment
@KangOl

KangOl Aug 3, 2017

Contributor

You're right.
The patch in determine_value to limit to PREFETCH_MAX records also lost the prefetch environment.
The same error was done when filtering the x2many fields computed in sudo.

diff --git odoo/fields.py odoo/fields.py
index 5bcbbd2b9a5..a735eb7ee4b 100644
--- odoo/fields.py
+++ odoo/fields.py
@@ -980,6 +980,7 @@ class Field(object):
                 recs = record._in_cache_without(self)
                 if len(recs) > PREFETCH_MAX:
                     recs = recs[:PREFETCH_MAX] | record
+                recs = recs.with_prefetch(record._prefetch)
                 self.compute_value(recs)
 
         else:
@@ -2068,7 +2069,7 @@ class _RelationalMulti(_Relational):
             accessible = lambda target: target.id in target_ids
             # filter values to keep the accessible records only
             for record in records:
-                record[self.name] = record[self.name].filtered(accessible)
+                record[self.name] = record[self.name].filtered(accessible).with_prefetch(record._prefetch)
 
     def _setup_regular_base(self, model):
         super(_RelationalMulti, self)._setup_regular_base(model)
Contributor

KangOl commented Aug 3, 2017

You're right.
The patch in determine_value to limit to PREFETCH_MAX records also lost the prefetch environment.
The same error was done when filtering the x2many fields computed in sudo.

diff --git odoo/fields.py odoo/fields.py
index 5bcbbd2b9a5..a735eb7ee4b 100644
--- odoo/fields.py
+++ odoo/fields.py
@@ -980,6 +980,7 @@ class Field(object):
                 recs = record._in_cache_without(self)
                 if len(recs) > PREFETCH_MAX:
                     recs = recs[:PREFETCH_MAX] | record
+                recs = recs.with_prefetch(record._prefetch)
                 self.compute_value(recs)
 
         else:
@@ -2068,7 +2069,7 @@ class _RelationalMulti(_Relational):
             accessible = lambda target: target.id in target_ids
             # filter values to keep the accessible records only
             for record in records:
-                record[self.name] = record[self.name].filtered(accessible)
+                record[self.name] = record[self.name].filtered(accessible).with_prefetch(record._prefetch)
 
     def _setup_regular_base(self, model):
         super(_RelationalMulti, self)._setup_regular_base(model)
@xmo-odoo

This comment has been minimized.

Show comment
Hide comment
@xmo-odoo

xmo-odoo Aug 4, 2017

Collaborator

@KangOl you can edit the PR (add new commits) directly.

Collaborator

xmo-odoo commented Aug 4, 2017

@KangOl you can edit the PR (add new commits) directly.

xmo-odoo referenced this pull request in odoo-dev/odoo Aug 4, 2017

[FIX] prefetch issues on computed/related fields
When deciding to prefetch records (getting records from the cache with
no value for the field being fetched), if the field was computed
determine_value would just get all records, not limited by the normal
prefetch limit, for large recordsets this would generate gigantic
prefetch lists for records we may not need at all.

Fix by applying the PREFETCH_MAX limit to records from the cache as is
done in _prefetch_field.

Complementarily, when traversing related fields the prefetch
environment would be lost and every record would get an empty prefetch
environment, so the values would ultimately be read one by one.

Example: select (search) 1000 product.product records, access a
related field (e.g. categ_id) in a loop, on the first iteration the
system would first read 1000 templates, then it would read each
categ_id individually, resulting in >1000 SQL queries rather than the
~2 we would expect.

Fixes #18511
@lmignon

This comment has been minimized.

Show comment
Hide comment
@lmignon

lmignon Aug 4, 2017

Contributor

@KangOl
I made some tests and this new fix improves one step further the performance.
We now have 12 queries on product.template to get the 5000 products! (To be compared to the 10060 queries without the 2 fixes !!!) The performance boost is impressive. 🍾
I still have to do in deep tests on the large database of one of our client. We have a process to export the 50.000 products and it seems that we need to process the first 600 records to have all the records prefetched.

Contributor

lmignon commented Aug 4, 2017

@KangOl
I made some tests and this new fix improves one step further the performance.
We now have 12 queries on product.template to get the 5000 products! (To be compared to the 10060 queries without the 2 fixes !!!) The performance boost is impressive. 🍾
I still have to do in deep tests on the large database of one of our client. We have a process to export the 50.000 products and it seems that we need to process the first 600 records to have all the records prefetched.

@rco-odoo

Overall this is ok for me, except for one line (see my specific comment).

@@ -565,7 +565,7 @@ def traverse_related(self, record):
""" Traverse the fields of the related field `self` except for the last
one, and return it as a pair `(last_record, last_field)`. """
for name in self.related[:-1]:
record = record[name][:1]
record = record[name][:1].with_prefetch(record._prefetch)

This comment has been minimized.

@rco-odoo

rco-odoo Aug 9, 2017

Member

Well done! I am glad that the prefetching API makes this fix simple and clear!

@rco-odoo

rco-odoo Aug 9, 2017

Member

Well done! I am glad that the prefetching API makes this fix simple and clear!

Show outdated Hide outdated odoo/fields.py
Show outdated Hide outdated odoo/fields.py

@rco-odoo rco-odoo merged commit d140f0e into odoo:10.0 Aug 10, 2017

1 of 2 checks passed

ci/runbot runbot build 253517-18644-202c6b
Details
legal/cla Raphael Collet Odoo CLA signature check
Details

@rco-odoo rco-odoo deleted the odoo-dev:10.0-related-prefetch-xmo branch Aug 10, 2017

@Yenthe666

This comment has been minimized.

Show comment
Hide comment
@Yenthe666

Yenthe666 Aug 10, 2017

Contributor

Hi @rco-odoo and @xmo-odoo,

Thank you for the fix and merge! Will this be backported to 8.0 / 9.0 or will it only land in 10.0?
I for example could really use this for a customer in V9.

Regards,
Yenthe

Contributor

Yenthe666 commented Aug 10, 2017

Hi @rco-odoo and @xmo-odoo,

Thank you for the fix and merge! Will this be backported to 8.0 / 9.0 or will it only land in 10.0?
I for example could really use this for a customer in V9.

Regards,
Yenthe

@rco-odoo

This comment has been minimized.

Show comment
Hide comment
@rco-odoo

rco-odoo Aug 10, 2017

Member

@Yenthe666 the prefetching mechanism has been modified for 10.0, therefore the prefetching behavior is different in 8.0 / 9.0.
However, the second part may be backported: use the prefetching limit for computing fields.

Member

rco-odoo commented Aug 10, 2017

@Yenthe666 the prefetching mechanism has been modified for 10.0, therefore the prefetching behavior is different in 8.0 / 9.0.
However, the second part may be backported: use the prefetching limit for computing fields.

@lmignon

This comment has been minimized.

Show comment
Hide comment
@lmignon

lmignon Aug 10, 2017

Contributor

Thank you @xmo-odoo an @rco-odoo for the fix!

Contributor

lmignon commented Aug 10, 2017

Thank you @xmo-odoo an @rco-odoo for the fix!

mvaled added a commit to merchise-autrement/odoo that referenced this pull request Aug 15, 2017

Merge branch '10.0-544aa9b04a9' into merchise-develop-10.0
* 10.0-544aa9b04a9:
  [FIX] prefetch issues on computed/related fields (#18644)
  [FIX] website_sale: no-cache IE11 cached cart XHR
  [FIX] website_event: Website Event Error when you encode a 0 value in the registration
  [FIX] stock: recompute display name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment