[PERF] evaluation: faster spreading invalidation #4203
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
LucasLefevre
force-pushed
the
master-faster-invalidate-spread-lul
branch
from
129c140
to
e3bf587
Compare
|
ok IMO |
LucasLefevre
force-pushed
the
master-faster-invalidate-spread-lul
branch
4 times, most recently
from
7dd07f8
to
11ec7e7
Compare
LucasLefevre
force-pushed
the
master-faster-invalidate-spread-lul
branch
from
11ec7e7
to
8005562
Compare
anhe-odoo
reviewed
May 14, 2024
There was a problem hiding this comment.
@laa-odoo @LucasLefevre : two small comments but else LGTM :) Nice improvement !
| const arrayFormulaPosition = this.getArrayFormulaSpreadingOn(position); | ||
| if (arrayFormulaPosition) { | ||
| // ignore the formula spreading on the position. Keep only the blocked ones | ||
| arrayFormulaPositions.delete(arrayFormulaPosition); |
There was a problem hiding this comment.
Maybe we can remove it directly from arrayFormulas instead of adding it in arrayFormulaPositions and then remove it from the same set ?
There was a problem hiding this comment.
Problem is that arrayFormulas is typed as Iterable<CellPosition> (which I think is a good think because it makes it decoupled from the internal implementation). So we can't just remove a position from it.
Since 0467161 `getCellsDependingOn` is much faster but has a higher fixed cost overhead because it instanciates a `BinaryGrid`. It is very fast to use but a bit costly to create (it creates a 2d array proporional to the size of sheets). If you do it 10k times, it starts to add up... In `invalidateSpreading`, `getCellsDependingOn` was called once for each child position (which can be a lot if the formula spreads on a lot of cells). With this commit, it's called only once, with all positions batched. For the same reason, `getArrayFormulasBlockedBy` is also batched in this commit. The speed improvement spent in `invalidateSpreading` is ~10x faster. Task: 3925565
LucasLefevre
force-pushed
the
master-faster-invalidate-spread-lul
branch
from
8005562
to
8f29eeb
Compare
|
@robodoo r+ |
robodoo
pushed a commit
that referenced
this pull request
May 14, 2024
Since 0467161 `getCellsDependingOn` is much faster but has a higher fixed cost overhead because it instanciates a `BinaryGrid`. It is very fast to use but a bit costly to create (it creates a 2d array proporional to the size of sheets). If you do it 10k times, it starts to add up... In `invalidateSpreading`, `getCellsDependingOn` was called once for each child position (which can be a lot if the formula spreads on a lot of cells). With this commit, it's called only once, with all positions batched. For the same reason, `getArrayFormulasBlockedBy` is also batched in this commit. The speed improvement spent in `invalidateSpreading` is ~10x faster. closes #4203 Task: 3925565 Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
14 tasks
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.
Description:
description of this task, what is implemented and why it is implemented that way.
Task: : 3925565
review checklist