Skip to content
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

Jump to bottom

[IMP] BottomBarStatistics: faster computation of stats #3924

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[IMP] BottomBarStatistics: faster computation of stats #3924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
77734b6
[IMP] BottomBarStatistics: Memoize cell filtering
rrahir Mar 26, 2024
82afe4f
[IMP] BottomBarStatistics: Lazify statistic evaluation
rrahir Mar 26, 2024
a7dc365
[IMP] BottomBarStatistics: limit redundant getter calls
rrahir Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 18 additions & 6 deletions src/components/bottom_bar_statistic/aggregate_statistics_store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
import { sum } from "../../functions/helper_math";
import { average, countAny, countNumbers, max, min } from "../../functions/helper_statistical";
import { lazy, memoize } from "../../helpers";
import { Get } from "../../store_engine";
import { SpreadsheetStore } from "../../stores";
import { _t } from "../../translation";
import {
CellValueType,
Command,
EvaluatedCell,
Lazy,
Locale,
invalidateEvaluationCommands,
} from "../../types";

export interface StatisticFnResults {
[name: string]: number | undefined;
[name: string]: Lazy<number> | undefined;
}

interface SelectionStatisticFunction {
Expand Down Expand Up @@ -105,12 +107,17 @@ export class AggregateStatisticsStore extends SpreadsheetStore {
const sheetId = getters.getActiveSheetId();
const cells = new Set<EvaluatedCell>();

const isColHidden = memoize((col: number) => getters.isColHidden(sheetId, col));
const isRowHidden = memoize((row: number) => getters.isRowHidden(sheetId, row));
Comment on lines +110 to +111
Copy link
Collaborator

Choose a reason for hiding this comment

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

does it really makes a significant difference ?
I believe the benchmark in the commit message is not the correct one. It's suspiciously the same as the previous commit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's around 40ms for each improvement, I left out the commas afterwards, I guess I can make it again if you want ...

Copy link
Contributor

Choose a reason for hiding this comment

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

And if this getter really cause performance issue (which I'm not sure how since it really does nothing), shouldn't it be memoized inside the plugin rather than here ? That way every call to this getter in other plugin/components is faster

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know what to say. Don't believe me? then try it for yourself.
The reason I haven't set it everywhere right now is because I don't want to have to invalidate the cache all the time and it's pretty much the only place where it's heavily used.

Copy link
Collaborator Author

@rrahir rrahir Mar 29, 2024 •
edited

Choose a reason for hiding this comment

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

Sorry I'm a moron and can't make proper commit messages apparently. The gain was around 30° ms. I did another round of benchmark and will update the values accordingly

Copy link
Collaborator

Choose a reason for hiding this comment

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

After the 2 first improvements, the time is mostly spent in isColFolder and isRowFolder (of header_grouping.ts).
I think it would be wise to store on this plugin the list of cols and rows that are folded by sheet, so these getters becomes immediate.
image

const zones = getters.getSelectedZones();
for (const zone of zones) {
for (let col = zone.left; col <= zone.right; col++) {
if (isColHidden(col)) {
continue; // Skip hidden columns
}
for (let row = zone.top; row <= zone.bottom; row++) {
if (getters.isRowHidden(sheetId, row) || getters.isColHidden(sheetId, col)) {
continue; // Skip hidden cells
if (isRowHidden(row)) {
continue; // Skip hidden rows
}

const evaluatedCell = getters.getEvaluatedCell({ sheetId, col, row });
Expand All @@ -123,16 +130,21 @@ export class AggregateStatisticsStore extends SpreadsheetStore {
const locale = getters.getLocale();
let statisticFnResults: StatisticFnResults = {};
const cellsArray = [...cells];

const getCells = memoize((typeStr: string) => {
const types = typeStr.split(",");
return cellsArray.filter((c) => types.includes(c.type));
});
for (let fn of selectionStatisticFunctions) {
// We don't want to display statistical information when there is no interest:
// We set the statistical result to undefined if the data handled by the selection
// does not match the data handled by the function.
// Ex: if there are only texts in the selection, we prefer that the SUM result
// be displayed as undefined rather than 0.
let fnResult: number | undefined = undefined;
const evaluatedCells = cellsArray.filter((c) => fn.types.includes(c.type));
let fnResult: Lazy<number> | undefined = undefined;
const evaluatedCells = getCells(fn.types.sort().join(","));
if (evaluatedCells.length) {
fnResult = fn.compute(evaluatedCells, locale);
fnResult = lazy(() => fn.compute(evaluatedCells, locale));
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could even make the cell iteration and filter part of the lazy computation

    const fnResult: Lazy<number| undefined> = lazy(() => {
        const evaluatedCells = getCells(fn.types.join(","));
        if (evaluatedCells.length) {
          return fn.compute(evaluatedCells, locale);
        }
        return undefined;
      });

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no we cannot. We check the presence of fnResult in the final object to know if we should display the statistics button or not. So we'd end up evaluating everything all the time

}
statisticFnResults[fn.name] = fnResult;
}
Expand Down
2 changes: 1 addition & 1 deletion src/components/bottom_bar_statistic/bottom_bar_statistic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,6 @@ export class BottomBarStatistic extends Component<Props, SpreadsheetChildEnv> {
private getComposedFnName(fnName: string): string {
const locale = this.env.model.getters.getLocale();
const fnValue = this.store.statisticFnResults[fnName];
return fnName + ": " + (fnValue !== undefined ? formatValue(fnValue, { locale }) : "__");
return fnName + ": " + (fnValue !== undefined ? formatValue(fnValue(), { locale }) : "__");
}
}
36 changes: 18 additions & 18 deletions tests/bottom_bar/aggregate_statistics_store.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,15 @@ describe("Aggregate statistic functions", () => {

setSelection(model, ["A1:A2"]);
let statisticFnResults = store.statisticFnResults;
expect(statisticFnResults["Count"]).toBe(2);
expect(statisticFnResults["Count"]?.()).toBe(2);

// expand selection with the range A3:A2
addCellToSelection(model, "A3");
setAnchorCorner(model, "A2");

// A2 is now present in two selection
statisticFnResults = store.statisticFnResults;
expect(statisticFnResults["Count"]).toBe(3);
expect(statisticFnResults["Count"]?.()).toBe(3);
});

test("statistic function should not include hidden rows/columns in calculations", () => {
Expand All @@ -43,11 +43,11 @@ describe("Aggregate statistic functions", () => {

setSelection(model, ["A1:A4"]);
let statisticFnResults = store.statisticFnResults;
expect(statisticFnResults["Sum"]).toBe(6);
expect(statisticFnResults["Sum"]?.()).toBe(6);

hideRows(model, [1, 2]);
statisticFnResults = store.statisticFnResults;
expect(statisticFnResults["Sum"]).toBe(1);
expect(statisticFnResults["Sum"]?.()).toBe(1);
});

describe("return undefined if the types handled by the function are not present among the types of the selected cells", () => {
Expand All @@ -68,7 +68,7 @@ describe("Aggregate statistic functions", () => {
selectCell(model, "A1");
setAnchorCorner(model, "A7");
let statisticFnResults = store.statisticFnResults;
expect(statisticFnResults["Sum"]).toBe(66);
expect(statisticFnResults["Sum"]?.()).toBe(66);

selectCell(model, "A7");
statisticFnResults = store.statisticFnResults;
Expand All @@ -80,7 +80,7 @@ describe("Aggregate statistic functions", () => {
selectCell(model, "A1");
setAnchorCorner(model, "A7");
let statisticFnResults = store.statisticFnResults;
expect(statisticFnResults["Avg"]).toBe(22);
expect(statisticFnResults["Avg"]?.()).toBe(22);

selectCell(model, "A7");
statisticFnResults = store.statisticFnResults;
Expand All @@ -92,7 +92,7 @@ describe("Aggregate statistic functions", () => {
selectCell(model, "A1");
setAnchorCorner(model, "A7");
let statisticFnResults = store.statisticFnResults;
expect(statisticFnResults["Min"]).toBe(0);
expect(statisticFnResults["Min"]?.()).toBe(0);

selectCell(model, "A7");
statisticFnResults = store.statisticFnResults;
Expand All @@ -104,7 +104,7 @@ describe("Aggregate statistic functions", () => {
selectCell(model, "A1");
setAnchorCorner(model, "A7");
let statisticFnResults = store.statisticFnResults;
expect(statisticFnResults["Max"]).toBe(42);
expect(statisticFnResults["Max"]?.()).toBe(42);

selectCell(model, "A7");
statisticFnResults = store.statisticFnResults;
Expand All @@ -116,7 +116,7 @@ describe("Aggregate statistic functions", () => {
selectCell(model, "A1");
setAnchorCorner(model, "A7");
let statisticFnResults = store.statisticFnResults;
expect(statisticFnResults["Count"]).toBe(6);
expect(statisticFnResults["Count"]?.()).toBe(6);

selectCell(model, "A7");
statisticFnResults = store.statisticFnResults;
Expand All @@ -126,29 +126,29 @@ describe("Aggregate statistic functions", () => {
test('return the "Count numbers" value on all types of interpreted cells except on cells interpreted as empty', () => {
selectCell(model, "A1");
let statisticFnResults = store.statisticFnResults;
expect(statisticFnResults["Count Numbers"]).toBe(1);
expect(statisticFnResults["Count Numbers"]?.()).toBe(1);

selectCell(model, "A2");
statisticFnResults = store.statisticFnResults;
expect(statisticFnResults["Count Numbers"]).toBe(1);
expect(statisticFnResults["Count Numbers"]?.()).toBe(1);

selectCell(model, "A3");
statisticFnResults = store.statisticFnResults;
expect(statisticFnResults["Count Numbers"]).toBe(0);
expect(statisticFnResults["Count Numbers"]?.()).toBe(0);

selectCell(model, "A4");
statisticFnResults = store.statisticFnResults;
expect(statisticFnResults["Count Numbers"]).toBe(0);
expect(statisticFnResults["Count Numbers"]?.()).toBe(0);

selectCell(model, "A5");
statisticFnResults = store.statisticFnResults;
expect(statisticFnResults["Count Numbers"]).toBe(0);
expect(statisticFnResults["Count Numbers"]?.()).toBe(0);

// select the range A6:A7
selectCell(model, "A6");
setAnchorCorner(model, "A7");
statisticFnResults = store.statisticFnResults;
expect(statisticFnResults["Count"]).toBe(1);
expect(statisticFnResults["Count"]?.()).toBe(1);
});
});

Expand Down Expand Up @@ -186,12 +186,12 @@ describe("Aggregate statistic functions", () => {
createSheet(model, { sheetId: sId2 });
setCellContent(model, "A2", "4", sId2);
setCellContent(model, "A3", "4", sId2);
expect(store.statisticFnResults["Count"]).toBe(3);
expect(store.statisticFnResults["Count"]?.()).toBe(3);
activateSheet(model, sId2);
selectAll(model);
expect(store.statisticFnResults["Count"]).toBe(2);
expect(store.statisticFnResults["Count"]?.()).toBe(2);
activateSheet(model, sId1);
selectAll(model);
expect(store.statisticFnResults["Count"]).toBe(3);
expect(store.statisticFnResults["Count"]?.()).toBe(3);
});
});