Skip to content

Commit 8d22e86

Browse files
committed
[PERF] evaluation: don't evaluate needlessly
Let's say we have the following array formulas: in B1: =transpose(transpose(A1:A2)) in C1: =transpose(transpose(B1:B2)) in D1: =transpose(transpose(C1:C2)) Each columns depends on the array formula in the previous column. Initially, all cells are evaluated in the order B1,C1,D1 When B1 is evaluated, all cells that depends on its result (B1 and B2) are marked to be re-evaluated in another iteration. => C1 and D1 are added to `this.nextPositionsToUpdate` At the next iteration (which evaluated C1 and D1), when C1 is evaluated it goes again: D1 is added to `this.nextPositionsToUpdate` for a third iteration. Coming back to the initial iteration: even if C1 and D1 are in `this.nextPositionsToUpdate`, they are later computed in the *current* iteration (remember the initial iteration evaluates B1,C1,D1). It's useless to re-compute them since they are already computed (and no other result invalidated their result) The evaluation of the large array formula dataset goes from ~36s to ~750ms closes #4589 Task: 4036899 Signed-off-by: Vincent Schippefilt (vsc) <vsc@odoo.com>
1 parent 08d7226 commit 8d22e86

File tree

2 files changed

+28
-2
lines changed

2 files changed

+28
-2
lines changed

src/plugins/ui_core_views/cell_evaluation/evaluator.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,7 @@ export class Evaluator {
259259
return this.handleError(e, cell);
260260
} finally {
261261
this.cellsBeingComputed.delete(cellId);
262+
this.nextPositionsToUpdate.delete(positionId);
262263
}
263264
}
264265

tests/evaluation/evaluation_formula_array.test.ts

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -637,8 +637,8 @@ describe("evaluate formulas that return an array", () => {
637637
setCellContent(model, "C1", "=MFILL(2,1,B1+1)");
638638
expect(getEvaluatedCell(model, "A1").value).toBe(31);
639639
expect(getEvaluatedCell(model, "B1").value).toBe(31);
640-
expect(getEvaluatedCell(model, "C1").value).toBe(32);
641-
expect(getEvaluatedCell(model, "D1").value).toBe(32);
640+
expect(getEvaluatedCell(model, "C1").value).toBe(30);
641+
expect(getEvaluatedCell(model, "D1").value).toBe(30);
642642
});
643643

644644
test("Spreaded formulas with range deps Do not invalidate themselves on evaluation", () => {
@@ -665,6 +665,31 @@ describe("evaluate formulas that return an array", () => {
665665
expect(c).toEqual(3);
666666
});
667667

668+
test("array formula depending on array formula result is evaluated once", () => {
669+
const mockCompute = jest.fn().mockImplementation((values) => values);
670+
671+
functionRegistry.add("RANGE_IDENTITY", {
672+
description: "returns the input. Like transpose(transpose(range))",
673+
args: [arg("range (range<any>)", "")],
674+
returns: ["RANGE<ANY>"],
675+
compute: mockCompute,
676+
});
677+
new Model({
678+
sheets: [
679+
{
680+
cells: {
681+
A1: { content: "0" },
682+
A2: { content: "1" },
683+
B1: { content: "=RANGE_IDENTITY(A1:A2)" },
684+
C1: { content: "=RANGE_IDENTITY(B1:B2)" },
685+
D1: { content: "=RANGE_IDENTITY(C1:C2)" },
686+
},
687+
},
688+
],
689+
});
690+
expect(mockCompute).toHaveBeenCalledTimes(3);
691+
});
692+
668693
test("Spreaded formulas with range deps invalidate only once the dependencies of themselves", () => {
669694
let c = 0;
670695
functionRegistry.add("INCREMENTONEVAL", {

0 commit comments

Comments
 (0)