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

[REF] account: Huge improve performance to create account_move_lines for create_tax_cash_basis_entry method #31014

Closed
wants to merge 2 commits into
base: 11.0
from

Conversation

Projects
None yet
5 participants
@moylop260
Copy link
Contributor

moylop260 commented Feb 12, 2019

Fix #30934

I just get pyflame file and the SVG result is:

The following method

def create_tax_cash_basis_entry(self, percentage_before_rec):

is creating too much account.move.lines (for my case 876 lines) but the way is:

  1. Create account_move_line.create({..., 'move_id': ...})

  2. These create calls are nested from 2 levels for-loop:

    • for move in (self.debit_move_id.move_id, self.credit_move_id.move_id):
      #move_date is the max of the 2 reconciled items
      if move_date < move.date:
      move_date = move.date
      for line in move.line_ids:

The issue is that account.move and account.move.line models are triggering computed fields/methods for each new account.move.line added computing all lines again and again.

Running a process: O n^n

I mean,
account.move has a method sensible to account_move_line.debit to compute matched_percentage field:

  • @api.depends('line_ids.debit', 'line_ids.credit', 'line_ids.matched_debit_ids.amount', 'line_ids.matched_credit_ids.amount', 'line_ids.account_id.user_type_id.type')
    def _compute_matched_percentage(self):
    """Compute the percentage to apply for cash basis method. This value is relevant only for moves that
    involve journal items on receivable or payable accounts.
    """
    for move in self:
    total_amount = 0.0
    total_reconciled = 0.0
    for line in move.line_ids:
    if line.account_id.user_type_id.type in ('receivable', 'payable'):
    amount = abs(line.debit - line.credit)
    total_amount += amount
    for partial_line in (line.matched_debit_ids + line.matched_credit_ids):
    total_reconciled += partial_line.amount
    precision_currency = move.currency_id or move.company_id.currency_id
    if float_is_zero(total_amount, precision_rounding=precision_currency.rounding):
    move.matched_percentage = 1.0
    else:
    move.matched_percentage = total_reconciled / total_amount

Then for each line created this compute is executed for all lines (not just the last created one).

Similar case for:

@api.depends('debit', 'credit', 'move_id.matched_percentage', 'move_id.journal_id')
def _compute_cash_basis(self):
for move_line in self:
move_line_data = {}
if move_line.journal_id.type in ('sale', 'purchase'):
move_line_data['debit_cash_basis'] = move_line.debit * move_line.move_id.matched_percentage
move_line_data['credit_cash_basis'] = move_line.credit * move_line.move_id.matched_percentage
else:
move_line_data['debit_cash_basis'] = move_line.debit
move_line_data['credit_cash_basis'] = move_line.credit
move_line_data['balance_cash_basis'] = move_line.debit_cash_basis - move_line.credit_cash_basis
move_line.update(move_line_data)

For this case this compute method are called:
(876^876) * 2

This PR creates account_move_lines at final in order to trigger the compute just one global time.
I don't know if this is the better way to fix, but really it needs a fix.

OPW 1938216

@robodoo robodoo added the seen 🙂 label Feb 12, 2019

@moylop260

This comment has been minimized.

Copy link
Contributor Author

moylop260 commented Feb 12, 2019

@qdp-odoo What do you think?

@jev-odoo

This comment has been minimized.

Copy link
Contributor

jev-odoo commented Feb 12, 2019

@qdp-odoo I will test and compare with the version I've showed to you.

@hugho-ad

This comment has been minimized.

Copy link
Contributor

hugho-ad commented Feb 12, 2019

I tested it with the use of case reported at #30934
And the conciliation process took about 2 min on average.

👍

@robodoo robodoo removed the CI 🤖 label Feb 13, 2019

@hugho-ad hugho-ad force-pushed the vauxoo-dev:11.0-odoo-perf-cash-basis-moy branch 2 times, most recently to ee7de26 Feb 13, 2019

@robodoo robodoo added the CI 🤖 label Feb 13, 2019

@hugho-ad

This comment has been minimized.

Copy link
Contributor

hugho-ad commented Feb 13, 2019

The last commit added is attempting to speed up the reconciliation process by saving all lines to reconcile in a var to_reconcile_moves, after that, that var is used to reconcile each tuple of lines.

Also, self.env.norecompute is used to prevent computation and made that at the end of the cration of lines.

All changes above were made because of using 'new' function, the lines of the amount base were not getting the tax_ids, that field was kept empty.

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 14, 2019

@jev-odoo

This comment has been minimized.

Copy link
Contributor

jev-odoo commented Feb 14, 2019

What about that ?
Would you test it ?

def create_tax_cash_basis_entry(self, percentage_before_rec):
    self.ensure_one()
    move_date = self.debit_move_id.date
    newly_created_move = self.env['account.move']
    line_dict = []
    to_clear_aml_line_names = []
    to_clear_aml_line_ids = []
    for move in (self.debit_move_id.move_id, self.credit_move_id.move_id):
        # move_date is the max of the 2 reconciled items
        if move_date < move.date:
            move_date = move.date
        for line in move.line_ids:
            # TOCHECK: normal and cash basis taxes shoudn't be mixed together (on the same invoice line for example) as it will
            # create reporting issues. Not sure of the behavior to implement in that case, though.
            if not line.tax_exigible:
                percentage_before = percentage_before_rec[move.id]
                percentage_after = line._get_matched_percentage()[move.id]
                # amount is the current cash_basis amount minus the one before the reconciliation
                amount = line.balance * percentage_after - line.balance * percentage_before
                rounded_amt = self._get_amount_tax_cash_basis(amount, line)
                if float_is_zero(rounded_amt, precision_rounding=line.company_id.currency_id.rounding):
                    continue
                if line.tax_line_id and line.tax_line_id.tax_exigibility == 'on_payment':
                    # create cash basis entry for the tax line
                    line_dict.append((0, 0, {
                        'name': line.move_id.name,
                        'debit': abs(rounded_amt) if rounded_amt < 0 else 0.0,
                        'credit': rounded_amt if rounded_amt > 0 else 0.0,
                        'account_id': line.account_id.id,
                        'tax_exigible': True,
                        'amount_currency': line.amount_currency and line.currency_id.round(-line.amount_currency * amount / line.balance) or 0.0,
                        'currency_id': line.currency_id.id,
                        'move_id': newly_created_move.id,
                        'partner_id': line.partner_id.id, 
                        }))
                    if line.account_id.reconcile:
                        to_clear_aml_line_names.append(line.account_id.id)
                        to_clear_aml_line_ids.append(line.id)
                    # Group by cash basis account and tax
                    line_dict.append((0, 0, {
                        'name': line.name,
                        'debit': rounded_amt if rounded_amt > 0 else 0.0,
                        'credit': abs(rounded_amt) if rounded_amt < 0 else 0.0,
                        'account_id': line.tax_line_id.cash_basis_account.id,
                        'tax_line_id': line.tax_line_id.id,
                        'tax_exigible': True,
                        'amount_currency': line.amount_currency and line.currency_id.round(line.amount_currency * amount / line.balance) or 0.0,
                        'currency_id': line.currency_id.id,
                        'move_id': newly_created_move.id,
                        'partner_id': line.partner_id.id, 
                        }))
                if any([tax.tax_exigibility == 'on_payment' for tax in line.tax_ids]):
                    # create cash basis entry for the base
                    for tax in line.tax_ids:
                        account_id = self._get_tax_cash_basis_base_account(
                            line, tax)
                        line_dict.append((0, 0, {
                            'name': line.name,
                            'debit': rounded_amt > 0 and rounded_amt or 0.0,
                            'credit': rounded_amt < 0 and abs(rounded_amt) or 0.0,
                            'account_id': account_id.id,
                            'tax_exigible': True,
                            'tax_ids': [(6, 0, [tax.id])],
                            'move_id': newly_created_move.id,
                            'currency_id': line.currency_id.id,
                            'amount_currency': self.amount_currency and line.currency_id.round(line.amount_currency * amount / line.balance) or 0.0,
                            'partner_id': line.partner_id.id, 
                            }))
                        line_dict.append((0, 0, {
                            'name': line.name,
                            'credit': rounded_amt > 0 and rounded_amt or 0.0,
                            'debit': rounded_amt < 0 and abs(rounded_amt) or 0.0,
                            'account_id': account_id.id,
                            'tax_exigible': True,
                            'move_id': newly_created_move.id,
                            'currency_id': line.currency_id.id,
                            'amount_currency': self.amount_currency and line.currency_id.round(-line.amount_currency * amount / line.balance) or 0.0,
                            'partner_id': line.partner_id.id, 
                            }))
    if line_dict:
        self._create_tax_basis_move().write({'line_ids': line_dict})
        to_clear_aml = self.env['account.move.line'].search(
            ['|', ('id', 'in', to_clear_aml_line_ids), ('account_id', "in", to_clear_aml_line_names)])
        to_clear_aml.reconcile()
        if move_date > (self.company_id.period_lock_date or '0000-00-00') and newly_created_move.date != move_date:
            # The move date should be the maximum date between payment and invoice (in case
            # of payment in advance). However, we should make sure the move date is not
            # recorded before the period lock date as the tax statement for this period is
            # probably already sent to the estate.
            newly_created_move.write({'date': move_date})
        # post move
        newly_created_move.post()
@moylop260

This comment has been minimized.

Copy link
Contributor Author

moylop260 commented Feb 14, 2019

Hi @jev-odoo
For us, it is fine your proposal.

I mean, using dict is a good approach too.
Feel free of apply the change for this PR or a new one.

We have 5 customers with OPW hurry up for this issue to report taxes for government
Notice that we are improving create_tax_cash_basis_entry slow method but too for _reverse_move

reconcile and unreconcile are so slow process.

@hugho-ad

This comment has been minimized.

Copy link
Contributor

hugho-ad commented Feb 14, 2019

For the reconciliation process:

almost the same, what do you think @moylop260 ?

@jev-odoo

This comment has been minimized.

Copy link
Contributor

jev-odoo commented Feb 14, 2019

@hugho-ad @moylop260 , unfortunately, the resultant datas are not the same.
Yours are strictly the same as the old process.
Mine are not... account_move doesn't have name, are flagged 'draft' instead of 'done', ...
So yours is good.

@jev-odoo jev-odoo requested a review from qdp-odoo Feb 14, 2019

@moylop260

This comment has been minimized.

Copy link
Contributor Author

moylop260 commented Feb 15, 2019

@mart-e
May ask you to give a look at this PR, please? Thanks in advance

@moylop260

This comment has been minimized.

Copy link
Contributor Author

moylop260 commented Feb 15, 2019

@nim-odoo
Your feedback is welcome
Thanks in advance

@nim-odoo

This comment has been minimized.

Copy link
Contributor

nim-odoo commented Feb 15, 2019

@moylop260 @hugho-ad It seems fine, but could you please clean up the the commits with a clear message? From what I see, there should be 2 commits: one for _reverse_move, the other for create_tax_cash_basis_entry.

Thanks

@hugho-ad hugho-ad force-pushed the vauxoo-dev:11.0-odoo-perf-cash-basis-moy branch from 21907e1 Feb 15, 2019

@robodoo robodoo removed the CI 🤖 label Feb 15, 2019

@hugho-ad hugho-ad force-pushed the vauxoo-dev:11.0-odoo-perf-cash-basis-moy branch 2 times, most recently Feb 15, 2019

@robodoo robodoo added the CI 🤖 label Feb 15, 2019

@moylop260

This comment has been minimized.

Copy link
Contributor Author

moylop260 commented Feb 17, 2019

@nim-odoo
I'm working checking a full database using the patch and without using it
Could you let me finish the audit about before to merge, please?

hugho-ad added some commits Feb 13, 2019

[REF] account: Performance improve for create_tax_cash_basis_entry
create_tax_cash_basis_entry presents a critical performance issue when
reconciling moves with a lot of account_move_line.

The issue is that `account.move` and `account.move.line` models are triggering computed fields/methods for each new
`account.move.line` added, computing all lines again and again.

Making a factorial performance issue.

For each line the function try create cash basis entries for tax/base lines and this is combined with all the
fields compute that exists on the model so this introduce a performance problem

The issue is that `account.move` and `account.move.line` models are triggering
computed fields/methods for each new `account.move.line` created isolated,
computing all lines again and again.

e.g.
 _compute_matched_percentage at
https://github.com/odoo/odoo/blob/5e8289cfeaf52a1eac64bcc975b186f0b5f9594b/addons/account/models/account_move.py#L44-L62

Then for each line created this compute is executed for all lines (not just the last created one).

Now by using self.env.norecompute() and self.recompute() the problem was reduced.

Reconciling an 'account.move' with 1576 lines with a payment takes almost 4
minutes when before this changes, the conciliation process seemed not to never
end.

Fix #30934
[REF] account: Performance improve unreconcile process (_reverse_move):
_reverse_move presents a critical performance issue when
unreconcile moves with a lot of account_move_line.

The issue is that `account.move` and `account.move.line` models are triggering computed fields/methods for each new
`account.move.line` added, computing all lines again and again.

Making a factorial performance issue.

For each line the function try create cash basis entries for tax/base lines and this is combined with all the
fields compute that exists on the model so this introduce a performance problem

The issue is that `account.move` and `account.move.line` models are triggering
computed fields/methods for each new `account.move.line` created isolated,
computing all lines again and again.

e.g.
 _compute_matched_percentage at
https://github.com/odoo/odoo/blob/5e8289cfeaf52a1eac64bcc975b186f0b5f9594b/addons/account/models/account_move.py#L44-L62

Then for each line modified/created this compute is executed for all lines (not just the last one).

Now by using self.env.norecompute() and self.recompute() the problem was reduced.

Unreconciling an 'account.move' with 1576 lines with a payment takes almost 4
minutes when before this changes, the conciliation process seemed not to never
end.

Fix #30934

@moylop260 moylop260 force-pushed the vauxoo-dev:11.0-odoo-perf-cash-basis-moy branch to 52154f4 Feb 17, 2019

@robodoo robodoo removed the CI 🤖 label Feb 17, 2019

@moylop260

This comment has been minimized.

Copy link
Contributor Author

moylop260 commented Feb 17, 2019

Hi @nim-odoo
I just have created 2 identical databases and environment
Then for environment 1 I applied this patch for another one I used the current stable branch without changes.

Then, I confirm a big payment for both environments then I ran the following script for both ones:

SELECT concat_ws(',', balance, statement_id, partner_id, tax_exigible, amount_residual_currency, account_id, tax_line_id, debit_cash_basis, user_type_id, tax_base_amount,
    credit_cash_basis, reconciled, analytic_account_id, expected_pay_date, product_uom_id, index_based_currency_id, quantity, followup_line_id, blocked, payment_id,
    balance_cash_basis, company_id, product_id, debit, agreement_currency_id, agreement_currency_amount, date_maturity,
    amount_currency, followup_date, statement_line_id, invoice_id, credit, company_currency_id, journal_id, expense_id, internal_note, date, amount_residual, currency_id
) AS conc
FROM account_move_line
ORDER BY conc ASC

I just checked the diff and both outputs are identical.
I checked group by account.move.line by journal, reconciled, reconcile_id for both environments and all number are identical too.

LGTM 👍 for this PR.
Regards!

@moylop260

This comment has been minimized.

Copy link
Contributor Author

moylop260 commented Feb 17, 2019

Tip: You can hidden the spaces using these parameters:
https://github.com/odoo/odoo/pull/31014/files?utf8=%E2%9C%93&diff=unified&w=1

@robodoo robodoo added the CI 🤖 label Feb 17, 2019

@nim-odoo
Copy link
Contributor

nim-odoo left a comment

robodoo rebase-ff
robodoo r+

@robodoo robodoo added the r+ 👌 label Feb 20, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 20, 2019

Merge method set to rebase and fast-forward

pniederlag pushed a commit to pniederlag/odoo that referenced this pull request Feb 20, 2019

[REF] account: Performance improve unreconcile process (_reverse_move):
_reverse_move presents a critical performance issue when
unreconcile moves with a lot of account_move_line.

The issue is that `account.move` and `account.move.line` models are triggering computed fields/methods for each new
`account.move.line` added, computing all lines again and again.

Making a factorial performance issue.

For each line the function try create cash basis entries for tax/base lines and this is combined with all the
fields compute that exists on the model so this introduce a performance problem

The issue is that `account.move` and `account.move.line` models are triggering
computed fields/methods for each new `account.move.line` created isolated,
computing all lines again and again.

e.g.
 _compute_matched_percentage at
https://github.com/odoo/odoo/blob/5e8289cfeaf52a1eac64bcc975b186f0b5f9594b/addons/account/models/account_move.py#L44-L62

Then for each line modified/created this compute is executed for all lines (not just the last one).

Now by using self.env.norecompute() and self.recompute() the problem was reduced.

Unreconciling an 'account.move' with 1576 lines with a payment takes almost 4
minutes when before this changes, the conciliation process seemed not to never
end.

Fix odoo#30934

closes odoo#31014

@robodoo robodoo added merged 🎉 and removed merging 👷 labels Feb 20, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 20, 2019

Merged, thanks!

@robodoo robodoo closed this Feb 20, 2019

@luisg123v luisg123v deleted the vauxoo-dev:11.0-odoo-perf-cash-basis-moy branch Feb 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.