Skip to content

[FIX] Evaluation: Optimize invalidation of cell depending of spreads #6827

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: 18.0
Choose a base branch
from
+30 −1
Open

[FIX] Evaluation: Optimize invalidation of cell depending of spreads #6827

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion src/plugins/ui_core_views/cell_evaluation/evaluator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ export class Evaluator {
);
private blockedArrayFormulas = new PositionSet({});
private spreadingRelations = new SpreadingRelation();
private positionsToInvalidate: RTreeBoundingBox[] = [];

constructor(private readonly context: ModelConfig["custom"], getters: Getters) {
this.getters = getters;
Expand Down Expand Up @@ -304,6 +305,8 @@ export class Evaluator {
this.evaluatedCells.set(position, evaluatedCell);
}
}

this.invalidatePendingPositionsStack();
onIterationEndEvaluationRegistry.getAll().forEach((callback) => callback(this.getters));
}
if (currentIteration >= MAX_ITERATION) {
Expand Down Expand Up @@ -395,7 +398,7 @@ export class Evaluator {
// thanks to the isMatrix check above, we know that formulaReturn is MatrixFunctionReturn
this.spreadValues(formulaPosition, formulaReturn)
);
this.invalidatePositionsDependingOnSpread(formulaPosition.sheetId, resultZone);
this.positionsToInvalidate.push({ zone: resultZone, sheetId: formulaPosition.sheetId });
return createEvaluatedCell(
nullValueToZeroValue(formulaReturn[0][0]),
this.getters.getLocale(),
Expand All @@ -412,6 +415,32 @@ export class Evaluator {
this.nextPositionsToUpdate.addMany(invalidatedPositions);
}

private invalidatePendingPositionsStack() {
if (this.positionsToInvalidate.length > 0) {
// the result matrices are split in subzones to exclude the array formula position.
// This avoids to invalidate the topleft position of an array formula
// and create an infinite loop of self-invalidation
const boundingboxes = this.positionsToInvalidate
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const boundingboxes = this.positionsToInvalidate
const boundingBoxes = this.positionsToInvalidate

.map((range) =>
excludeTopLeft(range.zone).map((zone) => ({
sheetId: range.sheetId,
zone,
}))
)
.flat();
const invalidatedPositions = this.formulaDependencies().getCellsDependingOn(boundingboxes);
for (const range of this.positionsToInvalidate) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this removes the top-left cells from the positions to invalidate ... wasn't this already done in the excludeTopLeft above ? I'm a bit confused

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So are they ranges or positions ? I feel like we could improve the naming here

invalidatedPositions.delete({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is causing the issue @hokolomopo posted on the task chatter.
A1 should be recomputed (because A3 spilled on the zone it depends on), but this line removes it from this.nextPositionsToUpdate

sheetId: range.sheetId,
col: range.zone.left,
row: range.zone.top,
});
}
this.nextPositionsToUpdate.addMany(invalidatedPositions);
this.positionsToInvalidate = [];
}
}

private assertSheetHasEnoughSpaceToSpreadFormulaResult(
{ sheetId, col, row }: CellPosition,
matrixResult: Matrix<FunctionResultObject>
Expand Down