Skip to content

Commit

Permalink
[FIX] *: Compute max/min on huge arrays
Browse files Browse the repository at this point in the history
This is an extension of PR #1217. Navigators (or rather javascript
engines) have a limit to the number of function arguments [1].

We sometimes have to use Math.max and Math.min functions (which are
subject to this limitation) on every cell/column/row which means that
we are functionally limited.

E.g. If we take the limitation of Chrome, we could only process 117k
rows, which is not such a huge number if we consider the situation of a
user importing their data.

This revision introduces wrappers `largeMax` and `largeMin` which take
an  1-D array (a single argument) and find the maximum (resp. minimum)
present in that list. For arrays with les than a 100K cells, the wrapper
will still call Math.max as its implementation is most of the time
faster.

[1]: Experimentally, that limit is 117k on Google Chrome and 501k on
Firefox.

closes #3952

Task: 3802691
X-original-commit: c2a7220
Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
  • Loading branch information
rrahir committed Apr 1, 2024
1 parent 06db896 commit c2f6da2
Show file tree
Hide file tree
Showing 15 changed files with 103 additions and 41 deletions.
33 changes: 20 additions & 13 deletions src/actions/menu_items_actions.ts
Expand Up @@ -4,7 +4,14 @@ import {
getSmartChartDefinition,
} from "../helpers/figures/charts";
import { centerFigurePosition, getMaxFigureSize } from "../helpers/figures/figure/figure";
import { getZoneArea, isConsecutive, isEqual, numberToLetters } from "../helpers/index";
import {
getZoneArea,
isConsecutive,
isEqual,
largeMax,
largeMin,
numberToLetters,
} from "../helpers/index";
import { interactivePaste, interactivePasteFromOS } from "../helpers/ui/paste_interactive";
import { _t } from "../translation";
import { ClipboardMIMEType, ClipboardPasteOptions } from "../types/clipboard";
Expand Down Expand Up @@ -86,8 +93,8 @@ export const DELETE_CONTENT_ROWS_NAME = (env: SpreadsheetChildEnv) => {
let last: number;
const activesRows = env.model.getters.getActiveRows();
if (activesRows.size !== 0) {
first = Math.min(...activesRows);
last = Math.max(...activesRows);
first = largeMin([...activesRows]);
last = largeMax([...activesRows]);
} else {
const zone = env.model.getters.getSelectedZones()[0];
first = zone.top;
Expand Down Expand Up @@ -118,8 +125,8 @@ export const DELETE_CONTENT_COLUMNS_NAME = (env: SpreadsheetChildEnv) => {
let last: number;
const activeCols = env.model.getters.getActiveCols();
if (activeCols.size !== 0) {
first = Math.min(...activeCols);
last = Math.max(...activeCols);
first = largeMin([...activeCols]);
last = largeMax([...activeCols]);
} else {
const zone = env.model.getters.getSelectedZones()[0];
first = zone.left;
Expand Down Expand Up @@ -150,8 +157,8 @@ export const REMOVE_ROWS_NAME = (env: SpreadsheetChildEnv) => {
let last: number;
const activesRows = env.model.getters.getActiveRows();
if (activesRows.size !== 0) {
first = Math.min(...activesRows);
last = Math.max(...activesRows);
first = largeMin([...activesRows]);
last = largeMax([...activesRows]);
} else {
const zone = env.model.getters.getSelectedZones()[0];
first = zone.top;
Expand Down Expand Up @@ -207,8 +214,8 @@ export const REMOVE_COLUMNS_NAME = (env: SpreadsheetChildEnv) => {
let last: number;
const activeCols = env.model.getters.getActiveCols();
if (activeCols.size !== 0) {
first = Math.min(...activeCols);
last = Math.max(...activeCols);
first = largeMin([...activeCols]);
last = largeMax([...activeCols]);
} else {
const zone = env.model.getters.getSelectedZones()[0];
first = zone.left;
Expand Down Expand Up @@ -252,7 +259,7 @@ export const INSERT_ROWS_BEFORE_ACTION = (env: SpreadsheetChildEnv) => {
let row: number;
let quantity: number;
if (activeRows.size) {
row = Math.min(...activeRows);
row = largeMin([...activeRows]);
quantity = activeRows.size;
} else {
const zone = env.model.getters.getSelectedZones()[0];
Expand All @@ -273,7 +280,7 @@ export const INSERT_ROWS_AFTER_ACTION = (env: SpreadsheetChildEnv) => {
let row: number;
let quantity: number;
if (activeRows.size) {
row = Math.max(...activeRows);
row = largeMax([...activeRows]);
quantity = activeRows.size;
} else {
const zone = env.model.getters.getSelectedZones()[0];
Expand All @@ -294,7 +301,7 @@ export const INSERT_COLUMNS_BEFORE_ACTION = (env: SpreadsheetChildEnv) => {
let column: number;
let quantity: number;
if (activeCols.size) {
column = Math.min(...activeCols);
column = largeMin([...activeCols]);
quantity = activeCols.size;
} else {
const zone = env.model.getters.getSelectedZones()[0];
Expand All @@ -315,7 +322,7 @@ export const INSERT_COLUMNS_AFTER_ACTION = (env: SpreadsheetChildEnv) => {
let column: number;
let quantity: number;
if (activeCols.size) {
column = Math.max(...activeCols);
column = largeMax([...activeCols]);
quantity = activeCols.size;
} else {
const zone = env.model.getters.getSelectedZones()[0];
Expand Down
4 changes: 2 additions & 2 deletions src/helpers/chart_date.ts
@@ -1,6 +1,6 @@
import type { TimeScaleOptions } from "chart.js";
import { DeepPartial } from "chart.js/dist/types/utils";
import { parseDateTime } from ".";
import { largeMax, largeMin, parseDateTime } from ".";
import { Alias, Format, Locale } from "../types";

// -----------------------------------------------------------------------------
Expand Down Expand Up @@ -126,7 +126,7 @@ function getBestTimeUnitForScale(
return undefined;
}
const labelsTimestamps = labelDates.map((date) => date!.getTime());
const period = Math.max(...labelsTimestamps) - Math.min(...labelsTimestamps);
const period = largeMax(labelsTimestamps) - largeMin(labelsTimestamps);

const minUnit = getFormatMinDisplayUnit(format);

Expand Down
3 changes: 2 additions & 1 deletion src/helpers/clipboard/clipboard_os_state.ts
Expand Up @@ -8,6 +8,7 @@ import {
Getters,
Zone,
} from "../../types";
import { largeMax } from "../misc";
import { zoneToDimension } from "../zones";
import { ClipboardCellsAbstractState } from "./clipboard_abstract_cell_state";

Expand Down Expand Up @@ -78,7 +79,7 @@ export class ClipboardOsState extends ClipboardCellsAbstractState {

private getPasteZone(target: Zone[]): Zone {
const height = this.values.length;
const width = Math.max(...this.values.map((a) => a.length));
const width = largeMax(this.values.map((a) => a.length));
const { left: activeCol, top: activeRow } = target[0];
return {
top: activeRow,
Expand Down
3 changes: 2 additions & 1 deletion src/helpers/figures/charts/pie_chart.ts
Expand Up @@ -31,6 +31,7 @@ import { PieChartDefinition, PieChartRuntime } from "../../../types/chart/pie_ch
import { Validator } from "../../../types/validator";
import { toXlsxHexColor } from "../../../xlsx/helpers/colors";
import { formatValue } from "../../format";
import { largeMax } from "../../misc";
import { createRange } from "../../range";
import { AbstractChart } from "./abstract_chart";
import {
Expand Down Expand Up @@ -237,7 +238,7 @@ function getPieConfiguration(

function getPieColors(colors: ChartColors, dataSetsValues: DatasetValues[]): Color[] {
const pieColors: Color[] = [];
const maxLength = Math.max(...dataSetsValues.map((ds) => ds.data.length));
const maxLength = largeMax(dataSetsValues.map((ds) => ds.data.length));
for (let i = 0; i <= maxLength; i++) {
pieColors.push(colors.next());
}
Expand Down
34 changes: 33 additions & 1 deletion src/helpers/misc.ts
Expand Up @@ -260,7 +260,7 @@ export function getItemId<T>(item: T, itemsDic: { [id: number]: T }) {

// Generate new Id if the item didn't exist in the dictionary
const ids = Object.keys(itemsDic);
const maxId = ids.length === 0 ? 0 : Math.max(...ids.map((id) => parseInt(id, 10)));
const maxId = ids.length === 0 ? 0 : largeMax(ids.map((id) => parseInt(id, 10)));

itemsDic[maxId + 1] = item;
return maxId + 1;
Expand Down Expand Up @@ -497,3 +497,35 @@ export function isNumberBetween(value: number, min: number, max: number): boolea
}
return value >= min && value <= max;
}

/**
* Alternative to Math.max that works with large arrays.
* Typically useful for arrays bigger than 100k elements.
*/
export function largeMax(array: number[]) {
let len = array.length;

if (len < 100_000) return Math.max(...array);

let max: number = -Infinity;
while (len--) {
max = array[len] > max ? array[len] : max;
}
return max;
}

/**
* Alternative to Math.min that works with large arrays.
* Typically useful for arrays bigger than 100k elements.
*/
export function largeMin(array: number[]) {
let len = array.length;

if (len < 100_000) return Math.min(...array);

let min: number = +Infinity;
while (len--) {
min = array[len] < min ? array[len] : min;
}
return min;
}
3 changes: 1 addition & 2 deletions src/plugins/core/header_size.ts
@@ -1,9 +1,8 @@
import { DEFAULT_CELL_WIDTH } from "../../constants";
import { DEFAULT_CELL_HEIGHT, DEFAULT_CELL_WIDTH } from "../../constants";
import { deepCopy, getAddHeaderStartIndex, range, removeIndexesFromArray } from "../../helpers";
import { Command, ExcelWorkbookData, WorkbookData } from "../../types";
import { Dimension, HeaderIndex, Pixel, UID } from "../../types/misc";
import { CorePlugin } from "../core_plugin";
import { DEFAULT_CELL_HEIGHT } from "./../../constants";

interface HeaderSizeState {
sizes: Record<UID, Record<Dimension, Array<Pixel | undefined>>>;
Expand Down
11 changes: 9 additions & 2 deletions src/plugins/core/header_visibility.ts
@@ -1,4 +1,11 @@
import { deepCopy, getAddHeaderStartIndex, includesAll, range } from "../../helpers";
import {
deepCopy,
getAddHeaderStartIndex,
includesAll,
largeMax,
largeMin,
range,
} from "../../helpers";
import { Command, CommandResult, ExcelWorkbookData, WorkbookData } from "../../types";
import { ConsecutiveIndexes, Dimension, HeaderIndex, UID } from "../../types/misc";
import { CorePlugin } from "../core_plugin";
Expand Down Expand Up @@ -32,7 +39,7 @@ export class HeaderVisibilityPlugin extends CorePlugin {
const hiddenElements = new Set((hiddenGroup || []).flat().concat(cmd.elements));
if (hiddenElements.size >= elements) {
return CommandResult.TooManyHiddenElements;
} else if (Math.min(...cmd.elements) < 0 || Math.max(...cmd.elements) > elements) {
} else if (largeMin(cmd.elements) < 0 || largeMax(cmd.elements) > elements) {
return CommandResult.InvalidHeaderIndex;
} else {
return CommandResult.Success;
Expand Down
6 changes: 4 additions & 2 deletions src/plugins/core/range.ts
Expand Up @@ -5,6 +5,8 @@ import {
groupConsecutive,
isZoneInside,
isZoneValid,
largeMax,
largeMin,
numberToLetters,
RangeImpl,
rangeReference,
Expand Down Expand Up @@ -74,8 +76,8 @@ export class RangeAdapter implements CommandHandler<CoreCommand> {
let newRange = range;
let changeType: ChangeType = "NONE";
for (let group of groups) {
const min = Math.min(...group);
const max = Math.max(...group);
const min = largeMin(group);
const max = largeMax(group);
if (range.zone[start] <= min && min <= range.zone[end]) {
const toRemove = Math.min(range.zone[end], max) - min + 1;
changeType = "RESIZE";
Expand Down
6 changes: 4 additions & 2 deletions src/plugins/core/sheet.ts
Expand Up @@ -8,6 +8,8 @@ import {
isDefined,
isZoneInside,
isZoneValid,
largeMax,
largeMin,
range,
toCartesian,
} from "../../helpers/index";
Expand Down Expand Up @@ -127,8 +129,8 @@ export class SheetPlugin extends CorePlugin<SheetState> implements SheetState {
}
return CommandResult.Success;
case "REMOVE_COLUMNS_ROWS": {
const min = Math.min(...cmd.elements);
const max = Math.max(...cmd.elements);
const min = largeMin(cmd.elements);
const max = largeMax(cmd.elements);
if (min < 0 || !this.doesHeaderExist(cmd.sheetId, cmd.dimension, max)) {
return CommandResult.InvalidHeaderIndex;
} else if (
Expand Down
8 changes: 4 additions & 4 deletions src/plugins/ui_core_views/evaluation_conditional_format.ts
Expand Up @@ -2,7 +2,7 @@ import { LINK_COLOR } from "../../constants";
import { compile } from "../../formulas";
import { parseLiteral } from "../../helpers/cells";
import { colorNumberString, percentile } from "../../helpers/index";
import { clip, lazy } from "../../helpers/misc";
import { clip, largeMax, largeMin, lazy } from "../../helpers/misc";
import { _t } from "../../translation";
import {
CellIsRule,
Expand Down Expand Up @@ -180,13 +180,13 @@ export class EvaluationConditionalFormatPlugin extends UIPlugin {
.map((cell) => cell.value);
switch (threshold.type) {
case "value":
const result = functionName === "max" ? Math.max(...rangeValues) : Math.min(...rangeValues);
const result = functionName === "max" ? largeMax(rangeValues) : largeMin(rangeValues);
return result;
case "number":
return Number(threshold.value);
case "percentage":
const min = Math.min(...rangeValues);
const max = Math.max(...rangeValues);
const min = largeMin(rangeValues);
const max = largeMax(rangeValues);
const delta = max - min;
return min + (delta * Number(threshold.value)) / 100;
case "percentile":
Expand Down
8 changes: 5 additions & 3 deletions src/plugins/ui_feature/automatic_sum.ts
Expand Up @@ -3,6 +3,8 @@ import {
isDateTimeFormat,
isInside,
isOneDimensional,
largeMax,
largeMin,
positions,
range,
union,
Expand Down Expand Up @@ -189,15 +191,15 @@ export class AutomaticSumPlugin extends UIPlugin {
const invalidCells = cellPositions.filter(
(position) => cells[position] && !cells[position].isAutoSummable
);
const maxValidPosition = Math.max(...invalidCells);
const maxValidPosition = largeMax(invalidCells);
const numberSequences = groupConsecutive(
cellPositions.filter((position) => this.isNumber(cells[position]))
);
const firstSequence = numberSequences[0] || [];
if (Math.max(...firstSequence) < maxValidPosition) {
if (largeMax(firstSequence) < maxValidPosition) {
return Infinity;
}
return Math.min(...firstSequence);
return largeMin(firstSequence);
}

private shouldFindData(sheetId: UID, zone: Zone): boolean {
Expand Down
5 changes: 3 additions & 2 deletions src/plugins/ui_feature/renderer.ts
Expand Up @@ -29,6 +29,7 @@ import {
drawDecoratedText,
getZonesCols,
getZonesRows,
largeMin,
numberToLetters,
overlap,
positionToZone,
Expand Down Expand Up @@ -72,7 +73,7 @@ export class RendererPlugin extends UIPlugin {
* column of the current viewport
*/
getColDimensionsInViewport(sheetId: UID, col: HeaderIndex): HeaderDimensions {
const left = Math.min(...this.getters.getSheetViewVisibleCols());
const left = largeMin(this.getters.getSheetViewVisibleCols());
const start = this.getters.getColRowOffsetInViewport("COL", left, col);
const size = this.getters.getColSize(sheetId, col);
const isColHidden = this.getters.isColHidden(sheetId, col);
Expand All @@ -88,7 +89,7 @@ export class RendererPlugin extends UIPlugin {
* of the current viewport
*/
getRowDimensionsInViewport(sheetId: UID, row: HeaderIndex): HeaderDimensions {
const top = Math.min(...this.getters.getSheetViewVisibleRows());
const top = largeMin(this.getters.getSheetViewVisibleRows());
const start = this.getters.getColRowOffsetInViewport("ROW", top, row);
const size = this.getters.getRowSize(sheetId, row);
const isRowHidden = this.getters.isRowHidden(sheetId, row);
Expand Down
3 changes: 2 additions & 1 deletion src/plugins/ui_feature/ui_sheet.ts
Expand Up @@ -2,6 +2,7 @@ import { GRID_ICON_MARGIN, ICON_EDGE_LENGTH, PADDING_AUTORESIZE_HORIZONTAL } fro
import {
computeIconWidth,
computeTextWidth,
largeMax,
positions,
splitTextToWidth,
} from "../../helpers/index";
Expand Down Expand Up @@ -133,7 +134,7 @@ export class SheetUIPlugin extends UIPlugin {
private getColMaxWidth(sheetId: UID, index: HeaderIndex): number {
const cellsPositions = positions(this.getters.getColsZone(sheetId, index, index));
const sizes = cellsPositions.map((position) => this.getCellWidth({ sheetId, ...position }));
return Math.max(0, ...sizes);
return Math.max(0, largeMax(sizes));
}

/**
Expand Down
11 changes: 9 additions & 2 deletions src/xlsx/conversion/sheet_conversion.ts
@@ -1,4 +1,11 @@
import { buildSheetLink, markdownLink, splitReference, toCartesian, toXC } from "../../helpers";
import {
buildSheetLink,
largeMax,
markdownLink,
splitReference,
toCartesian,
toXC,
} from "../../helpers";
import { CellData, HeaderData, SheetData } from "../../types";
import { XLSXCell, XLSXHyperLink, XLSXImportData, XLSXWorksheet } from "../../types/xlsx";
import {
Expand Down Expand Up @@ -193,7 +200,7 @@ function getSheetDims(sheet: XLSXWorksheet): number[] {
const dims = [0, 0];

for (let row of sheet.rows) {
dims[0] = Math.max(dims[0], ...row.cells.map((cell) => toCartesian(cell.xc).col));
dims[0] = Math.max(dims[0], largeMax(row.cells.map((cell) => toCartesian(cell.xc).col)));
dims[1] = Math.max(dims[1], row.index);
}

Expand Down

0 comments on commit c2f6da2

Please sign in to comment.