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] Various easy performance boosts #2374

Closed
wants to merge 5 commits into from

Conversation

fw-bot
Copy link
Collaborator

@fw-bot fw-bot commented Apr 20, 2023

[IMP] header_visibility: improve perfs of findVisibleHeader

The way the getter findVisibleHeader was implemented as that it received
an argument with all the indexes it need to check to find a visible header.

This means that if we want to get the first visible col of a sheet,
we have to first generate an array with all the indexes of the cols of the
sheet, then looping on this array to find a visible col. This was very
wasteful as :

  1. we created an array that needed to be garbage collected after
  2. the array contains all the indexes, but most of the time we only
    loop on the first few

Changed the arguments of findVisibleHeader to receive from and to
arguments.

Benchmark :
on a sheet 26 cols x 10.000 rows filled with numbers, try to delete the last
column

Before : ~2050ms. Most time taken by :

  • Garbage Collection : ~737ms
  • range() : ~729ms

After: ~542ms. Most time taken by :

  • Garbage Collection : ~124ms
  • getRowTallestCellSize() : ~105ms

[IMP] sheet: move cells methods performance

The methods moveCellOn*() in sheet.ts did an useless parseInt() to
loop through the indexes of an array, slightly decreasing the performance.

For when we really have to parse a string to an int, Number() is also
slightly faster than parseInt().

This saves 20-40ms on a 10.000 rows sheet with 26 columns, when deleting the
first column.

[IMP] formats: performance of splitNumber()

To compute the formatted value of a number, we need to split it into
its integer digits and decimal digits. This was done by the function
splitNumber(), which was using Intl.NumberFormat, which handles
everything for us, but is quite slow.

Now convert the number to string using Number.toString(), and handle
the max number of decimal and the rounding by hand. We will still use
Intl.NumberFormat for numbers too big/small, for which Number.toString()
returns an exponential notation.

Rough benchmark :
Sheet 26 cols x 10.000 rows, using a CF on the whole sheet to force the
evaluation of the lazy values.

w/ sheet filled with integers:
old: splitNumber ~299ms
new: splitNumber ~9.5ms

w/ sheet filled with floats:
old: splitNumber ~555ms
new: splitNumber ~30ms

[IMP] model: store handler arrays

Currently the model create a new array every time this.handlers or
this.coreHandlers are accessed. That means that
we create new arrays every time we dispatch a command, leading to
quite a bit of garbage collection.

When deleting the first column of a 26 x 1000 sheet filled with numbers,
initializing the arrays saves ~100ms in garbage collection.

Odoo task ID : 3272878

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 (_lt("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: #2373
Forward-Port-Of: #2340

The way the getter `findVisibleHeader` was implemented as that it received
an argument with all the indexes it need to check to find a visible header.

This means that if we want to get the first visible col of a sheet,
we have to first generate an array with all the indexes of the cols of the
sheet, then looping on this array to find a visible col. This was very
wasteful as :

1) we created an array that needed to be garbage collected after
2) the array contains all the indexes, but most of the time we only
loop on the first few

Changed the arguments of `findVisibleHeader` to receive `from` and `to`
arguments.

Benchmark :
on a sheet 26 cols x 10.000 rows filled with numbers, try to delete the last
column
-----------
Before : ~2050ms. Most time taken by :
    - Garbage Collection : ~737ms
    - range() : ~729ms

After: ~542ms. Most time taken by :
    - Garbage Collection : ~124ms
    - getRowTallestCellSize() : ~105ms

Task: 3272878
X-original-commit: 420f451
The methods `moveCellOn*()` in `sheet.ts` did an useless `parseInt()` to
loop through the indexes of an array, slightly decreasing the performance.

For when we really have to parse a string to an int, Number() is also
slightly faster than parseInt().

This saves 20-40ms on a 10.000 rows sheet with 26 columns, when deleting the
first column.

Task: 3272878
X-original-commit: dd322bb
To compute the formatted value of a number, we need to split it into
its integer digits and decimal digits. This was done by the function
`splitNumber()`, which was using Intl.NumberFormat, which handles
everything for us, but is quite slow.

Now convert the number to string using Number.toString(), and handle
the max number of decimal and the rounding by hand. We will still use
Intl.NumberFormat for numbers too big/small, for which Number.toString()
returns an exponential notation.

Rough benchmark :
Sheet 26 cols x 10.000 rows, using a CF on the whole sheet to force the
evaluation of the lazy values.
----------------------

w/ sheet filled with integers:
old: splitNumber ~299ms
new: splitNumber ~9.5ms

w/ sheet filled with floats:

Old: splitNumber ~555ms
New: splitNumber ~30ms
Task: 3272878
X-original-commit: a02ed72
Currently the model create a new array every time `this.handlers` or
`this.coreHandlers` are accessed. That means that
we create new arrays every time we dispatch a command, leading to
quite a bit of garbage collection.

When deleting the first column of a 26 x 1000 sheet filled with numbers,
initializing the arrays saves ~100ms in garbage collection.

Task: 3272878
X-original-commit: 1387b0f
@robodoo
Copy link
Collaborator

robodoo commented Apr 20, 2023

@fw-bot
Copy link
Collaborator Author

fw-bot commented Apr 20, 2023

This PR targets saas-16.2 and is part of the forward-port chain. Further PRs will be created up to master.

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

@fw-bot
Copy link
Collaborator Author

fw-bot commented Apr 20, 2023

@hokolomopo @rrahir the next pull request (#2375) is in conflict. You can merge the chain up to here by saying

@fw-bot r+

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

@rrahir
Copy link
Collaborator

rrahir commented Apr 20, 2023

@fw-bot r+

robodoo pushed a commit that referenced this pull request Apr 20, 2023
The way the getter `findVisibleHeader` was implemented as that it received
an argument with all the indexes it need to check to find a visible header.

This means that if we want to get the first visible col of a sheet,
we have to first generate an array with all the indexes of the cols of the
sheet, then looping on this array to find a visible col. This was very
wasteful as :

1) we created an array that needed to be garbage collected after
2) the array contains all the indexes, but most of the time we only
loop on the first few

Changed the arguments of `findVisibleHeader` to receive `from` and `to`
arguments.

Benchmark :
on a sheet 26 cols x 10.000 rows filled with numbers, try to delete the last
column
-----------
Before : ~2050ms. Most time taken by :
    - Garbage Collection : ~737ms
    - range() : ~729ms

After: ~542ms. Most time taken by :
    - Garbage Collection : ~124ms
    - getRowTallestCellSize() : ~105ms

Task: 3272878
X-original-commit: 420f451
Part-of: #2374
robodoo pushed a commit that referenced this pull request Apr 20, 2023
The methods `moveCellOn*()` in `sheet.ts` did an useless `parseInt()` to
loop through the indexes of an array, slightly decreasing the performance.

For when we really have to parse a string to an int, Number() is also
slightly faster than parseInt().

This saves 20-40ms on a 10.000 rows sheet with 26 columns, when deleting the
first column.

Task: 3272878
X-original-commit: dd322bb
Part-of: #2374
robodoo pushed a commit that referenced this pull request Apr 20, 2023
To compute the formatted value of a number, we need to split it into
its integer digits and decimal digits. This was done by the function
`splitNumber()`, which was using Intl.NumberFormat, which handles
everything for us, but is quite slow.

Now convert the number to string using Number.toString(), and handle
the max number of decimal and the rounding by hand. We will still use
Intl.NumberFormat for numbers too big/small, for which Number.toString()
returns an exponential notation.

Rough benchmark :
Sheet 26 cols x 10.000 rows, using a CF on the whole sheet to force the
evaluation of the lazy values.
----------------------

w/ sheet filled with integers:
old: splitNumber ~299ms
new: splitNumber ~9.5ms

w/ sheet filled with floats:

Old: splitNumber ~555ms
New: splitNumber ~30ms
Task: 3272878
X-original-commit: a02ed72
Part-of: #2374
robodoo pushed a commit that referenced this pull request Apr 20, 2023
Currently the model create a new array every time `this.handlers` or
`this.coreHandlers` are accessed. That means that
we create new arrays every time we dispatch a command, leading to
quite a bit of garbage collection.

When deleting the first column of a 26 x 1000 sheet filled with numbers,
initializing the arrays saves ~100ms in garbage collection.

Task: 3272878
X-original-commit: 1387b0f
Part-of: #2374
@robodoo robodoo temporarily deployed to merge April 20, 2023 07:57 Inactive
@robodoo robodoo closed this Apr 20, 2023
@robodoo robodoo closed this in 5e2c0b2 Apr 20, 2023
@fw-bot fw-bot deleted the saas-16.2-16.0-random-perfs-adrm-eQJB-fw branch May 4, 2023 08:46
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.

4 participants