-
Notifications
You must be signed in to change notification settings - Fork 32
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.
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
[IMP] BottomBarStatistics: faster computation of stats #3924
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 { | ||
|
@@ -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)); | ||
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 }); | ||
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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;
}); There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.