[16.0] Various easy performance boosts #2340

hokolomopo · 2023-04-13T11:55:44Z

[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 :

we created an array that needed to be garbage collected after
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

robodoo · 2023-04-13T11:56:06Z

Pull request status dashboard.

LucasLefevre

nice 👍

src/helpers/misc.ts

src/plugins/core/sheet.ts

rrahir

Nice imp!

Just a note, you don't have to change it right now but for future commits, it would be nice to add the trail Task: {tasknumber} instead of Odoo taskto help us out when we merge everything in Odoo ;-)

src/helpers/format.ts

hokolomopo · 2023-04-13T15:28:18Z

Fixed a bug in splitNumber(), I didn't handle correctly the case where we round up and the last digit is a "9". Added a test. Unfortunately, it makes the function more complex :/ I'd really like to use simply Number.toFixed(), but it has some strange illogical behavour...

TODO for tomorrow: also check if it's worth changing range() to an iterator, to avoid allocating arrays where we don't have to.
And TODO : change Odoo task id=> task id

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

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

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

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

rrahir · 2023-04-19T14:50:24Z

src/migrations/data.ts

@@ -46,7 +46,7 @@ export function load(data?: any, verboseImport?: boolean): WorkbookData {
      }
    }
  }
-  data = JSON.parse(JSON.stringify(data));
+  data = deepCopy(data);


not imported in the file :o

Task: 3272878

rrahir · 2023-04-19T15:02:50Z

@robodoo r+

LucasLefevre · 2023-04-19T15:06:49Z

robodoo rebase-ff 😛

robodoo · 2023-04-19T15:07:16Z

Merge method set to rebase and fast-forward.

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 Part-of: #2340

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 Part-of: #2340

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 Part-of: #2340

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 Part-of: #2340

closes #2340 Task: 3272878 Signed-off-by: Rémi Rahir (rar) <[email protected]>

LucasLefevre reviewed Apr 13, 2023

View reviewed changes

src/helpers/misc.ts Outdated Show resolved Hide resolved

src/helpers/misc.ts Outdated Show resolved Hide resolved

src/plugins/core/sheet.ts Show resolved Hide resolved

LucasLefevre mentioned this pull request Apr 13, 2023

[REF] format: don't use formatter for integers #1585

Closed

14 tasks

rrahir reviewed Apr 13, 2023

View reviewed changes

src/helpers/format.ts Show resolved Hide resolved

src/helpers/format.ts Outdated Show resolved Hide resolved

hokolomopo force-pushed the 16.0-random-perfs-adrm branch 2 times, most recently from 7e4e07b to 209c13a Compare April 13, 2023 15:26

hokolomopo force-pushed the 16.0-random-perfs-adrm branch 3 times, most recently from 341c82f to f75aedc Compare April 14, 2023 13:52

hokolomopo added 4 commits April 17, 2023 09:49

hokolomopo force-pushed the 16.0-random-perfs-adrm branch from f75aedc to a5169ac Compare April 17, 2023 07:55

rrahir reviewed Apr 19, 2023

View reviewed changes

[IMP] *: use deepCopy instead of JSON.parse(JSON.stringify)

b16ef59

Task: 3272878

rrahir force-pushed the 16.0-random-perfs-adrm branch from 2990e65 to b16ef59 Compare April 19, 2023 14:52

robodoo pushed a commit that referenced this pull request Apr 19, 2023

[IMP] *: use deepCopy instead of JSON.parse(JSON.stringify)

300596b

closes #2340 Task: 3272878 Signed-off-by: Rémi Rahir (rar) <[email protected]>

robodoo temporarily deployed to merge April 19, 2023 15:14 Inactive

robodoo closed this Apr 19, 2023

This was referenced Apr 19, 2023

[FW][16.0] Various easy performance boosts #2373

Closed

[FW][16.0] Various easy performance boosts #2374

Closed

fw-bot mentioned this pull request Apr 20, 2023

[FW][16.0] Various easy performance boosts #2375

Closed

14 tasks

fw-bot deleted the 16.0-random-perfs-adrm branch May 3, 2023 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16.0] Various easy performance boosts #2340

[16.0] Various easy performance boosts #2340

hokolomopo commented Apr 13, 2023 •

edited

Loading

robodoo commented Apr 13, 2023

LucasLefevre left a comment

rrahir left a comment

hokolomopo commented Apr 13, 2023 •

edited

Loading

rrahir Apr 19, 2023

rrahir commented Apr 19, 2023

LucasLefevre commented Apr 19, 2023

robodoo commented Apr 19, 2023

[16.0] Various easy performance boosts #2340

[16.0] Various easy performance boosts #2340

Conversation

hokolomopo commented Apr 13, 2023 • edited Loading

[IMP] header_visibility: improve perfs of findVisibleHeader

[IMP] sheet: move cells methods performance

[IMP] formats: performance of splitNumber()

[IMP] model: store handler arrays

review checklist

robodoo commented Apr 13, 2023

LucasLefevre left a comment

Choose a reason for hiding this comment

rrahir left a comment

Choose a reason for hiding this comment

hokolomopo commented Apr 13, 2023 • edited Loading

rrahir Apr 19, 2023

Choose a reason for hiding this comment

rrahir commented Apr 19, 2023

LucasLefevre commented Apr 19, 2023

robodoo commented Apr 19, 2023

hokolomopo commented Apr 13, 2023 •

edited

Loading

[IMP] header_visibility: improve perfs of `findVisibleHeader`

[IMP] formats: performance of `splitNumber()`

hokolomopo commented Apr 13, 2023 •

edited

Loading