[FIX] xlsx: accelerate xlsx export #3949
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`pushElement` was returning an object but only one of its properties was used. After this revision, the helper only returns the id inside the list. It also alleviates the memory print. Task: 3733743
rrahir
force-pushed
the
15.0-fix-slow-xlsx-rar
branch
from
f628472
to
c934f6e
Compare
The borders are already normalized at export time. The code was denormalizing the renormalizing it without reason. Task: 3733743
JSON.stringify is way slower than deepEquals to compare objects and
assuming that the keys of the object are not ordered the same way, will
yield false negatives.
e.g.
```javascript
a = {'a':2,'b':4}
b = {'b': 4, 'a':2}
JSON.stringify(a) === JSON.stringify(b) // false
deepEquals(a,b) // true
```
Benchmark of `getXLSX`
======================
(averaged on 10 runs)
- On a 30k random text cells (to force heavy use of pushElement):
Before: 153 seconds
After: 10 seconds
- On RNG heavy sheet (patch tested in master):
Before: 351 seconds
After : 15 seconds
Task: 3733743
rrahir
force-pushed
the
15.0-fix-slow-xlsx-rar
branch
from
8ae108a
to
911d598
Compare
After a first test of strict equalilty, `deepEquals` will make type comparisons before yielding the strict quality again. In the context of `pushElement`, we know that we compare same-type objects, which means we only need to evaluate the strict equality for primitive types. Benchmark of `getXLSX` ====================== (averaged on 10 runs) - On a 30k random text cells (to force heavy use of pushElement): Before: 10 seconds After: 6.7 seconds - On RNG heavy sheet (patch tested in master): Before: 15 seconds After : 9.7 seconds Task: 3733743
rrahir
force-pushed
the
15.0-fix-slow-xlsx-rar
branch
from
911d598
to
ef5dc52
Compare
We would evaluate the strict equality of the two arguments twice in case of primitive (non-null) types but the outcome of that evaluation was already known.
anhe-odoo
requested changes
Apr 8, 2024
Comment on lines
+150
to
+151
| propertyList[propertyList.length] = property; | ||
| return propertyList.length - 1; |
There was a problem hiding this comment.
Can't we re-use len here ?
propertyList[len] = property;
return len;?
LucasLefevre
reviewed
Apr 8, 2024
Comment on lines
+8004
to
+8008
| <right style=\\"thin\\" color=\\"000000\\"/> | ||
| <top/> | ||
| <bottom/> | ||
| <diagonal/> | ||
| </border> |
There was a problem hiding this comment.
Do you know what causes theses changes in the snapshot ?
LucasLefevre
approved these changes
Apr 8, 2024
robodoo
pushed a commit
that referenced
this pull request
Apr 8, 2024
`pushElement` was returning an object but only one of its properties was used. After this revision, the helper only returns the id inside the list. It also alleviates the memory print. Task: 3733743 Part-of: #3949
robodoo
pushed a commit
that referenced
this pull request
Apr 8, 2024
The borders are already normalized at export time. The code was denormalizing the renormalizing it without reason. Task: 3733743 Part-of: #3949
robodoo
pushed a commit
that referenced
this pull request
Apr 8, 2024
JSON.stringify is way slower than deepEquals to compare objects and
assuming that the keys of the object are not ordered the same way, will
yield false negatives.
e.g.
```javascript
a = {'a':2,'b':4}
b = {'b': 4, 'a':2}
JSON.stringify(a) === JSON.stringify(b) // false
deepEquals(a,b) // true
```
Benchmark of `getXLSX`
======================
(averaged on 10 runs)
- On a 30k random text cells (to force heavy use of pushElement):
Before: 153 seconds
After: 10 seconds
- On RNG heavy sheet (patch tested in master):
Before: 351 seconds
After : 15 seconds
Task: 3733743
Part-of: #3949
robodoo
pushed a commit
that referenced
this pull request
Apr 8, 2024
After a first test of strict equalilty, `deepEquals` will make type comparisons before yielding the strict quality again. In the context of `pushElement`, we know that we compare same-type objects, which means we only need to evaluate the strict equality for primitive types. Benchmark of `getXLSX` ====================== (averaged on 10 runs) - On a 30k random text cells (to force heavy use of pushElement): Before: 10 seconds After: 6.7 seconds - On RNG heavy sheet (patch tested in master): Before: 15 seconds After : 9.7 seconds Task: 3733743 Part-of: #3949
robodoo
pushed a commit
that referenced
this pull request
Apr 8, 2024
We would evaluate the strict equality of the two arguments twice in case of primitive (non-null) types but the outcome of that evaluation was already known. closes #3949 Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
|
Merge method set to rebase and fast-forward. |
14 tasks
This was referenced Apr 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Pull request addresses the slowness of the
pushElementhelper.The two main improvments concern the iteration over the array elements and the comparison operations.
Benchmark of
getXLSXfunctionOn a 30k random string cells (heavy use of
pushElement) averaged on 10 runsWith the same patch applied to the mater branch on RNG's super sheet
Description:
description of this task, what is implemented and why it is implemented that way.
Task: : 3733743
review checklist