Skip to content

Commit

Permalink
[IMP] header_visibility: improve perfs of findVisibleHeader
Browse files Browse the repository at this point in the history
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: #2340
  • Loading branch information
hokolomopo committed Apr 19, 2023
1 parent 2b4831d commit 95a2949
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 22 deletions.
8 changes: 5 additions & 3 deletions src/components/grid/grid.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -224,12 +224,14 @@ export class Grid extends Component<Props, SpreadsheetChildEnv> {
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);
},
Expand Down
10 changes: 4 additions & 6 deletions src/plugins/ui/filter_evaluation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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),
};
}

Expand Down
44 changes: 35 additions & 9 deletions src/plugins/ui/header_visibility_ui.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { range } from "../../helpers";
import { Dimension, ExcelWorkbookData, HeaderIndex, Position, UID } from "../../types";
import { UIPlugin } from "../ui_plugin";

Expand Down Expand Up @@ -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(
Expand Down
8 changes: 4 additions & 4 deletions src/selection_stream/selection_stream_processor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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,
};
}
Expand Down

0 comments on commit 95a2949

Please sign in to comment.