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

[FW] 16.0 perf 3588401 lul #3209

Closed
wants to merge 3 commits into from

Conversation

fw-bot
Copy link
Collaborator

@fw-bot fw-bot commented Nov 17, 2023

Description:

description of this task, what is implemented and why it is implemented that way.

Task: : 3588401

review checklist

  • feature is organized in plugin, or UI components
  • support of duplicate sheet (deep copy)
  • in model/core: ranges are Range object, and can be adapted (adaptRanges)
  • in model/UI: ranges are strings (to show the user)
  • undo-able commands (uses this.history.update)
  • multiuser-able commands (has inverse commands and transformations where needed)
  • new/updated/removed commands are documented
  • exportable in excel
  • translations (_t("qmsdf %s", abc))
  • unit tested
  • clean commented code
  • track breaking changes
  • doc is rebuild (npm run doc)
  • status is correct in Odoo

Forward-Port-Of: #3147

@robodoo
Copy link
Collaborator

robodoo commented Nov 17, 2023

@fw-bot
Copy link
Collaborator Author

fw-bot commented Nov 17, 2023

@LucasLefevre @rrahir cherrypicking of pull request #3147 failed.

stderr:

14:20:05.379625 git.c:463               trace: built-in: git cherry-pick d515f25de4abbed647baeed5898ffb4ce0e01a99
error: Cherry-picking is not possible because you have unmerged files.
hint: Fix them up in the work tree, and then use 'git add/rm <file>'
hint: as appropriate to mark resolution and make a commit.
fatal: cherry-pick failed
----------
status:

Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?).

In the former case, you may want to edit this PR message as well.

More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port

This commit caches string normalization used in lookup functions.
`normalizeString` is called for every element of lookup ranges. Those
ranges can be large.

In the production spreadsheet mentionned on the ticket, it was taking more
than 13% of the loading time. Now it's 0.0%

`normalizeString` was called 19M times while it has only 664 different
input values.

opw-3588401
Task: 3588401
With this commit, the range arrays are created with their final size.

On the ticket spreadsheet, `range` self-time goes from ~21.4% to ~17.4%

opw-3588401
When a function references a range `=SUM(A1:B10)`, a new 2d array is rebuilt
with the cell values for every range in every functions.

However, for LOOKUP functions, it's very likely to have many cells with a
LOOKUP function looking for a match in the exact same range.

In the customer spreadsheet, `range` self-time goes from ~15% to almost nothing
(0.2%). There's another spreadsheet where many cells references the same range
with 100k cells. Loading the spreadsheet was taking 26s before and 7s after.

opw-3588401
Task: 3588401
@LucasLefevre LucasLefevre force-pushed the saas-16.1-16.0-perf-3588401-lul-K1Kb-fw branch from d154353 to 00e88e9 Compare November 17, 2023 13:28
@LucasLefevre
Copy link
Collaborator

fw-bot r+

@fw-bot
Copy link
Collaborator Author

fw-bot commented Nov 17, 2023

@LucasLefevre I can only do this on unmodified forward-port PRs, ask robodoo.

@LucasLefevre
Copy link
Collaborator

robodoo r+

robodoo pushed a commit that referenced this pull request Nov 17, 2023
This commit caches string normalization used in lookup functions.
`normalizeString` is called for every element of lookup ranges. Those
ranges can be large.

In the production spreadsheet mentionned on the ticket, it was taking more
than 13% of the loading time. Now it's 0.0%

`normalizeString` was called 19M times while it has only 664 different
input values.

opw-3588401

Task: 3588401
Part-of: #3209
robodoo pushed a commit that referenced this pull request Nov 17, 2023
With this commit, the range arrays are created with their final size.

On the ticket spreadsheet, `range` self-time goes from ~21.4% to ~17.4%

opw-3588401

Part-of: #3209
robodoo pushed a commit that referenced this pull request Nov 17, 2023
When a function references a range `=SUM(A1:B10)`, a new 2d array is rebuilt
with the cell values for every range in every functions.

However, for LOOKUP functions, it's very likely to have many cells with a
LOOKUP function looking for a match in the exact same range.

In the customer spreadsheet, `range` self-time goes from ~15% to almost nothing
(0.2%). There's another spreadsheet where many cells references the same range
with 100k cells. Loading the spreadsheet was taking 26s before and 7s after.

opw-3588401

closes #3209

Task: 3588401
Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
@robodoo robodoo closed this Nov 17, 2023
@fw-bot fw-bot mentioned this pull request Nov 17, 2023
14 tasks
@fw-bot fw-bot deleted the saas-16.1-16.0-perf-3588401-lul-K1Kb-fw branch December 1, 2023 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants