Skip to content

Commit

Permalink
[PERF] evaluation: don't evaluate needlessly
Browse files Browse the repository at this point in the history
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

Task: 4036899
  • Loading branch information
LucasLefevre committed Jul 5, 2024
1 parent b0225a4 commit 5b14256
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 2 deletions.
1 change: 1 addition & 0 deletions src/plugins/ui_core_views/cell_evaluation/evaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ export class Evaluator {
return this.handleError(e, cell);
} finally {
this.cellsBeingComputed.delete(cellId);
this.nextPositionsToUpdate.delete(positionId);
}
}

Expand Down
29 changes: 27 additions & 2 deletions tests/evaluation/evaluation_formula_array.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -637,8 +637,8 @@ describe("evaluate formulas that return an array", () => {
setCellContent(model, "C1", "=MFILL(2,1,B1+1)");
expect(getEvaluatedCell(model, "A1").value).toBe(31);
expect(getEvaluatedCell(model, "B1").value).toBe(31);
expect(getEvaluatedCell(model, "C1").value).toBe(32);
expect(getEvaluatedCell(model, "D1").value).toBe(32);
expect(getEvaluatedCell(model, "C1").value).toBe(30);
expect(getEvaluatedCell(model, "D1").value).toBe(30);
});

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

test("array formula depending on array formula result is evaluated once", () => {
const mockCompute = jest.fn().mockImplementation((values) => values);

functionRegistry.add("RANGE_IDENTITY", {
description: "returns the input. Like transpose(transpose(range))",
args: [arg("range (range<any>)", "")],
returns: ["RANGE<ANY>"],
compute: mockCompute,
});
new Model({
sheets: [
{
cells: {
A1: { content: "0" },
A2: { content: "1" },
B1: { content: "=RANGE_IDENTITY(A1:A2)" },
C1: { content: "=RANGE_IDENTITY(B1:B2)" },
D1: { content: "=RANGE_IDENTITY(C1:C2)" },
},
},
],
});
expect(mockCompute).toHaveBeenCalledTimes(3);
});

test("Spreaded formulas with range deps invalidate only once the dependencies of themselves", () => {
let c = 0;
functionRegistry.add("INCREMENTONEVAL", {
Expand Down

0 comments on commit 5b14256

Please sign in to comment.