Vauxoo / addons-vauxoo

All our modules related to developments that solves generic issues on Odoo, or that solve internal problems on Odoo Core, if something is here, maybe it is solving an issue in your company, try it and report what you see.
http://www.vauxoo.com
193 stars 288 forks source link

8.0 controller report xls performance imp hbto #1204

Closed hbto closed 7 years ago

hbto commented 7 years ago

Memoizing frequently asked method.

ormcache as it is only applicable to orm methods as stated here

moylop260 commented 7 years ago

What about use lru cache of python? like as https://github.com/Vauxoo/cfdilib/pull/49

hbto commented 7 years ago

@moylop260 Yes we have got rid of the previous implementation and we have deployed the standard implementation of python.

What about use lru cache of python? like as Vauxoo/cfdilib#49

Regards

moylop260 commented 7 years ago

The method get_odoo_style if you use a hashable objects (like as string) in the parameter received of the method you could cache it too and the method should tranform this string to html object and could be faster. (Slow the first time but faster the second ones)

moylop260 commented 7 years ago

The method _get_due_days could be improvement with the following patch:

+       today = datetime.now()
        for record in self:
-            today = datetime.now()
             date_due = datetime.strptime(record.date_due, '%Y-%m-%d')
             record.due_days = (today - date_due).days
moylop260 commented 7 years ago

The method _get_invoice_by_partner looks like as:

invoices = set(inv.search(...))

You don't need a set for a search if you want get unique records because sql return unique record for SELECT ... FROM ... WHERE ...

moylop260 commented 7 years ago

For the method _get_invoice_by_partner you are using [aml.id for aml in invoice.payment_ids] but you could use just invoice.payment_ids.ids

moylop260 commented 7 years ago

For the method _get_invoice_by_partner you are using to get date_due:

            date_due = [amx_brw.date_maturity
                        for amx_brw in invoice.move_id.line_id
                        if amx_brw.account_id.type in (
                            'receivable', 'payable')]
            date_due = date_due and min(date_due)
            date_due = date_due or invoice.date_due or invoice.date_invoice

I mean, you get all date_maturity of all records to use just one (the minimum one). This consume too much resources.

You should use search with limit and order to faster date_due process.

           date_maturity = aml.search_read(
                [('account_id.type', 'in', ['receivable', 'payable']),
                 ('date_maturity', '!=', None),
                 ('move_id', '=', invoice.move_id.id)], ['date_maturity'],
                limit=1, order='date_maturity ASC')
            date_due = (date_maturity[0]['date_maturity'] if date_maturity
                        else invoice.date_due)  or invoice.date_invoice

This will be faster because you don't have many small queries to get record by record the value.

moylop260 commented 7 years ago

You have a global get_amount plus a get_amount by line

Both are using the same operations but one is cumulative and other one is just by line. What about use just line._get_amount and assign a cumulative sum to parent?

Something like as:

_name = 'account.aging.wizard.partner'

def _get_amount(self):
    line.span01 = 10
    line.parent_id.span01 += line.span01

With this feature you won't need to compute the same value 2 times to totalize

moylop260 commented 7 years ago

For the following logic:

values = [line.value for line in record.lines]
count = 0
for value in values:
   count += value

You should use read_group like as @josemoralesp recommend us. I mean,

count = record.read_group([('id', 'in', record.lines.ids)], ['value'], [])  # Use sum(value) of sql
count = count[0]['value'] if count else 0

By each line.value you are executing a small query too many times to database and this is slower. Better use a big query with the result.

E.g. with this logic:

moylop260 commented 7 years ago

FYI I have created the following patches using read_group in order to have a better practical example of use them.

https://gist.github.com/moylop260/9e0211ee02cd440101d733f0e7f337c3

Please, test and fix before to use them directly

hbto commented 7 years ago

The method get_odoo_style if you use a hashable objects (like as string) in the parameter received of the method you could cache it too and the method should tranform this string to html object and could be faster. (Slow the first time but faster the second ones)

@moylop260 uno de los parámetros no es hashable

hbto commented 7 years ago

The method _get_due_days could be improvement with the following patch:

  • today = datetime.now() for record in self:
  • today = datetime.now() date_due = datetime.strptime(record.date_due, '%Y-%m-%d') record.due_days = (today - date_due).days

@moylop260 Taking into account

hbto commented 7 years ago

@moylop260 Thanks for you suggestions they will be taken into account.

Thanks so much.

dsabrinarg commented 7 years ago

@moylop260 technically this LGTY?, the functional tests 👍

josemoralesp commented 7 years ago

Hello @hbto @moylop260 .

These would be my changes for _get_amount method using read_group:

from collections import defaultdict
@api.depends('document_ids', 'document_ids.residual',
                'document_ids.payment', 'document_ids.total')
def _get_amount(self):
    field_sum = ['payment', 'total', 'aawp_id']
    field_spans = ['span01', 'span02', 'span03', 'span04', 'span05']
    for record in self:
        res = defaultdict(float)
        direction = record.aaw_id.direction == 'past'
        spans = [record.aaw_id.period_length * x * (direction and 1 or -1)
                    for x in range(5)]
        vals = record.document_ids.read_group(
            [('aawp_id', '=', record.id)], field_sum, ['aawp_id'])

        vals = vals[0] if vals else {}

        res['payment'] += vals[0].get('payment', 0)
        res['total'] += vals[0].get('total', 0)

        vals = record.document_ids.read_group(
            [('aawp_id', '=', record.id),
             ('due_days', '<=' if direction else '>', 0)],
            ['residual', 'aawp_id'], ['aawp_id'])

        if vals:
            res['not_due'] += vals[0].get('residual', 0)
        else:
            record._get_amount_span(
                field_spans, res, doc, spans, direction)
        record.update(res)

I have used two new things here:

This method defaultdict allows set an object by default, then when you try to get an unexisting key, this return the object. This is useful to avoid using extra loops in our code to fill values in a dict.

Another method used was read_group. This method is useful when you want to get sum of numeric values in many records(The same behavior of use a sql with "sum" and "group by"). In our case we use it to get the sum of three fields in the whole documents related with the wizard.

In sql this would be something like this:

SELECT
   aawp_id,
   sum(residual),
   sum(payment)
FROM
   account_aging_wizard_document
WHERE
   aawp_id = our_current_id
GROUP BY
   aawp_id

I didn't test the python method above. But it's only an idea(how it could be done). Let me know if you need my help for something. I want to apologize if I said something extremely obvious for you, but I would want to leave the message to be reused in other cases for anyone in the team

moylop260 commented 7 years ago

@hbto I found the following OCA module to create xls reports:

You can see a implementation from:

Maybe this module could help us to improve performance.

hbto commented 7 years ago

I found the following OCA module to create xls reports

@moylop260 our way of implementing a report in XLS is less cumbersome than theirs.

We don't have to deal with fields nor definitions of any kind whatsoever.

Actually, controller_report_xls is cleaner to deploy

Did you see how many lines with xlwt they had to write to implement that module? It could be that one glance is not enough to judge an amazing judge.

Meanwhile using controller_report_xls is less intrusive, you have to declare one context and the report will come in xls, nothing else, (behind the job you have to do to get it done).

Regards.

moy commented 7 years ago

I guess you meant @moylop260, not @moy ;-).

moylop260 commented 7 years ago

@hbto Please, check my last commit

dsabrinarg commented 7 years ago

@josemoralesp merge please as soon as the CI ends, @moylop260 approves this already, the functional test say that is taking only 2 minutes, great job team @hbto

josemoralesp commented 7 years ago

Roger that