From 95a2949afc69ad87841f55395edf687a659ebdf0 Mon Sep 17 00:00:00 2001 From: "Adrien Minne (adrm)" Date: Wed, 12 Apr 2023 12:51:34 +0000 Subject: [PATCH] [IMP] header_visibility: improve perfs of `findVisibleHeader` The way the getter `findVisibleHeader` was implemented as that it received an argument with all the indexes it need to check to find a visible header. This means that if we want to get the first visible col of a sheet, we have to first generate an array with all the indexes of the cols of the sheet, then looping on this array to find a visible col. This was very wasteful as : 1) we created an array that needed to be garbage collected after 2) the array contains all the indexes, but most of the time we only loop on the first few Changed the arguments of `findVisibleHeader` to receive `from` and `to` arguments. Benchmark : on a sheet 26 cols x 10.000 rows filled with numbers, try to delete the last column ----------- Before : ~2050ms. Most time taken by : - Garbage Collection : ~737ms - range() : ~729ms After: ~542ms. Most time taken by : - Garbage Collection : ~124ms - getRowTallestCellSize() : ~105ms Task: 3272878 Part-of: odoo/o-spreadsheet#2340 --- src/components/grid/grid.ts | 8 ++-- src/plugins/ui/filter_evaluation.ts | 10 ++--- src/plugins/ui/header_visibility_ui.ts | 44 +++++++++++++++---- .../selection_stream_processor.ts | 8 ++-- 4 files changed, 48 insertions(+), 22 deletions(-) diff --git a/src/components/grid/grid.ts b/src/components/grid/grid.ts index 5fe88896f4..63d6a92cab 100644 --- a/src/components/grid/grid.ts +++ b/src/components/grid/grid.ts @@ -5,7 +5,7 @@ import { HEADER_WIDTH, SCROLLBAR_WIDTH, } from "../../constants"; -import { isInside, range } from "../../helpers/index"; +import { isInside } from "../../helpers/index"; import { interactiveCut } from "../../helpers/ui/cut_interactive"; import { interactivePaste, interactivePasteFromOS } from "../../helpers/ui/paste_interactive"; import { ComposerSelection } from "../../plugins/ui/edition"; @@ -224,12 +224,14 @@ export class Grid extends Component { const col = this.env.model.getters.findVisibleHeader( sheetId, "COL", - range(0, this.env.model.getters.getNumberCols(sheetId)).reverse() + this.env.model.getters.getNumberCols(sheetId) - 1, + 0 )!; const row = this.env.model.getters.findVisibleHeader( sheetId, "ROW", - range(0, this.env.model.getters.getNumberRows(sheetId)).reverse() + this.env.model.getters.getNumberRows(sheetId) - 1, + 0 )!; this.env.model.selection.selectCell(col, row); }, diff --git a/src/plugins/ui/filter_evaluation.ts b/src/plugins/ui/filter_evaluation.ts index 8170204902..eb0f1a5dc2 100644 --- a/src/plugins/ui/filter_evaluation.ts +++ b/src/plugins/ui/filter_evaluation.ts @@ -166,13 +166,11 @@ export class FilterEvaluationPlugin extends UIPlugin { } private intersectZoneWithViewport(sheetId: UID, zone: Zone) { - const colsRange = range(zone.left, zone.right + 1); - const rowsRange = range(zone.top, zone.bottom + 1); return { - left: this.getters.findVisibleHeader(sheetId, "COL", colsRange), - right: this.getters.findVisibleHeader(sheetId, "COL", colsRange.reverse()), - top: this.getters.findVisibleHeader(sheetId, "ROW", rowsRange), - bottom: this.getters.findVisibleHeader(sheetId, "ROW", rowsRange.reverse()), + left: this.getters.findVisibleHeader(sheetId, "COL", zone.left, zone.right), + right: this.getters.findVisibleHeader(sheetId, "COL", zone.right, zone.left), + top: this.getters.findVisibleHeader(sheetId, "ROW", zone.top, zone.bottom), + bottom: this.getters.findVisibleHeader(sheetId, "ROW", zone.bottom, zone.top), }; } diff --git a/src/plugins/ui/header_visibility_ui.ts b/src/plugins/ui/header_visibility_ui.ts index be11e93fa6..4e5967492d 100644 --- a/src/plugins/ui/header_visibility_ui.ts +++ b/src/plugins/ui/header_visibility_ui.ts @@ -1,4 +1,3 @@ -import { range } from "../../helpers"; import { Dimension, ExcelWorkbookData, HeaderIndex, Position, UID } from "../../types"; import { UIPlugin } from "../ui_plugin"; @@ -31,17 +30,44 @@ export class HeaderVisibilityUIPlugin extends UIPlugin { getNextVisibleCellPosition(sheetId: UID, col: number, row: number): Position { return { - col: this.findVisibleHeader(sheetId, "COL", range(col, this.getters.getNumberCols(sheetId)))!, - row: this.findVisibleHeader(sheetId, "ROW", range(row, this.getters.getNumberRows(sheetId)))!, + col: this.findVisibleHeader(sheetId, "COL", col, this.getters.getNumberCols(sheetId) - 1)!, + row: this.findVisibleHeader(sheetId, "ROW", row, this.getters.getNumberRows(sheetId) - 1)!, }; } - findVisibleHeader(sheetId: UID, dimension: Dimension, indexes: number[]): number | undefined { - return indexes.find( - (index) => - this.getters.doesHeaderExist(sheetId, dimension, index) && - !this.isHeaderHidden(sheetId, dimension, index) - ); + /** + * Find the first visible header in the range [`from` => `to`]. + * + * Both `from` and `to` are inclusive. + */ + findVisibleHeader( + sheetId: UID, + dimension: Dimension, + from: number, + to: number + ): number | undefined { + if (from <= to) { + for (let i = from; i <= to; i++) { + if ( + this.getters.doesHeaderExist(sheetId, dimension, i) && + !this.isHeaderHidden(sheetId, dimension, i) + ) { + return i; + } + } + } + if (from > to) { + for (let i = from; i >= to; i--) { + if ( + this.getters.doesHeaderExist(sheetId, dimension, i) && + !this.isHeaderHidden(sheetId, dimension, i) + ) { + return i; + } + } + } + + return undefined; } findLastVisibleColRowIndex( diff --git a/src/selection_stream/selection_stream_processor.ts b/src/selection_stream/selection_stream_processor.ts index c82d6a40a3..b9e715fdc9 100644 --- a/src/selection_stream/selection_stream_processor.ts +++ b/src/selection_stream/selection_stream_processor.ts @@ -421,8 +421,8 @@ export class SelectionStreamProcessor } const { left, right, top, bottom } = zone; const sheetId = this.getters.getActiveSheetId(); - const refCol = this.getters.findVisibleHeader(sheetId, "COL", range(left, right + 1)); - const refRow = this.getters.findVisibleHeader(sheetId, "ROW", range(top, bottom + 1)); + const refCol = this.getters.findVisibleHeader(sheetId, "COL", left, right); + const refRow = this.getters.findVisibleHeader(sheetId, "ROW", top, bottom); if (refRow === undefined || refCol === undefined) { return CommandResult.SelectionOutOfBound; } @@ -530,10 +530,10 @@ export class SelectionStreamProcessor return { col: this.getters.isColHidden(sheetId, anchorCol) - ? this.getters.findVisibleHeader(sheetId, "COL", range(left, right + 1)) || anchorCol + ? this.getters.findVisibleHeader(sheetId, "COL", left, right) || anchorCol : anchorCol, row: this.getters.isRowHidden(sheetId, anchorRow) - ? this.getters.findVisibleHeader(sheetId, "ROW", range(top, bottom + 1)) || anchorRow + ? this.getters.findVisibleHeader(sheetId, "ROW", top, bottom) || anchorRow : anchorRow, }; }