From c2f6da21963ffa5aa23ec1385f4d9b370ec88bab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Rahir=20=28rar=29?= Date: Mon, 25 Mar 2024 10:46:18 +0000 Subject: [PATCH] [FIX] *: Compute max/min on huge arrays MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 odoo/o-spreadsheet#3952 Task: 3802691 X-original-commit: c2a7220c4ad737d1e509ebd87566f0e181c3ef62 Signed-off-by: Lucas Lefèvre (lul) Signed-off-by: Rémi Rahir (rar) --- src/actions/menu_items_actions.ts | 33 +++++++++++------- src/helpers/chart_date.ts | 4 +-- src/helpers/clipboard/clipboard_os_state.ts | 3 +- src/helpers/figures/charts/pie_chart.ts | 3 +- src/helpers/misc.ts | 34 ++++++++++++++++++- src/plugins/core/header_size.ts | 3 +- src/plugins/core/header_visibility.ts | 11 ++++-- src/plugins/core/range.ts | 6 ++-- src/plugins/core/sheet.ts | 6 ++-- .../evaluation_conditional_format.ts | 8 ++--- src/plugins/ui_feature/automatic_sum.ts | 8 +++-- src/plugins/ui_feature/renderer.ts | 5 +-- src/plugins/ui_feature/ui_sheet.ts | 3 +- src/xlsx/conversion/sheet_conversion.ts | 11 ++++-- src/xlsx/functions/charts.ts | 6 ++-- 15 files changed, 103 insertions(+), 41 deletions(-) diff --git a/src/actions/menu_items_actions.ts b/src/actions/menu_items_actions.ts index a0d9fa05b..24e5733b4 100644 --- a/src/actions/menu_items_actions.ts +++ b/src/actions/menu_items_actions.ts @@ -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"; @@ -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; @@ -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; @@ -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; @@ -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; @@ -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]; @@ -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]; @@ -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]; @@ -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]; diff --git a/src/helpers/chart_date.ts b/src/helpers/chart_date.ts index 69b5cbce7..a6cc36eb4 100644 --- a/src/helpers/chart_date.ts +++ b/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"; // ----------------------------------------------------------------------------- @@ -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); diff --git a/src/helpers/clipboard/clipboard_os_state.ts b/src/helpers/clipboard/clipboard_os_state.ts index d14641af5..07c05b6b3 100644 --- a/src/helpers/clipboard/clipboard_os_state.ts +++ b/src/helpers/clipboard/clipboard_os_state.ts @@ -8,6 +8,7 @@ import { Getters, Zone, } from "../../types"; +import { largeMax } from "../misc"; import { zoneToDimension } from "../zones"; import { ClipboardCellsAbstractState } from "./clipboard_abstract_cell_state"; @@ -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, diff --git a/src/helpers/figures/charts/pie_chart.ts b/src/helpers/figures/charts/pie_chart.ts index 5f7572410..f40a78f1e 100644 --- a/src/helpers/figures/charts/pie_chart.ts +++ b/src/helpers/figures/charts/pie_chart.ts @@ -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 { @@ -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()); } diff --git a/src/helpers/misc.ts b/src/helpers/misc.ts index 9702eaf96..d179d9be2 100644 --- a/src/helpers/misc.ts +++ b/src/helpers/misc.ts @@ -260,7 +260,7 @@ export function getItemId(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; @@ -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; +} diff --git a/src/plugins/core/header_size.ts b/src/plugins/core/header_size.ts index bfa23afc2..a89fe8844 100644 --- a/src/plugins/core/header_size.ts +++ b/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>>; diff --git a/src/plugins/core/header_visibility.ts b/src/plugins/core/header_visibility.ts index 066fe3870..3e805a9a2 100644 --- a/src/plugins/core/header_visibility.ts +++ b/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"; @@ -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; diff --git a/src/plugins/core/range.ts b/src/plugins/core/range.ts index ea41c191b..1904b31bf 100644 --- a/src/plugins/core/range.ts +++ b/src/plugins/core/range.ts @@ -5,6 +5,8 @@ import { groupConsecutive, isZoneInside, isZoneValid, + largeMax, + largeMin, numberToLetters, RangeImpl, rangeReference, @@ -74,8 +76,8 @@ export class RangeAdapter implements CommandHandler { 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"; diff --git a/src/plugins/core/sheet.ts b/src/plugins/core/sheet.ts index 037547160..b4fcb4122 100644 --- a/src/plugins/core/sheet.ts +++ b/src/plugins/core/sheet.ts @@ -8,6 +8,8 @@ import { isDefined, isZoneInside, isZoneValid, + largeMax, + largeMin, range, toCartesian, } from "../../helpers/index"; @@ -127,8 +129,8 @@ export class SheetPlugin extends CorePlugin 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 ( diff --git a/src/plugins/ui_core_views/evaluation_conditional_format.ts b/src/plugins/ui_core_views/evaluation_conditional_format.ts index 85695212b..7e52664fe 100644 --- a/src/plugins/ui_core_views/evaluation_conditional_format.ts +++ b/src/plugins/ui_core_views/evaluation_conditional_format.ts @@ -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, @@ -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": diff --git a/src/plugins/ui_feature/automatic_sum.ts b/src/plugins/ui_feature/automatic_sum.ts index 861354cfc..ddb2adfb8 100644 --- a/src/plugins/ui_feature/automatic_sum.ts +++ b/src/plugins/ui_feature/automatic_sum.ts @@ -3,6 +3,8 @@ import { isDateTimeFormat, isInside, isOneDimensional, + largeMax, + largeMin, positions, range, union, @@ -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 { diff --git a/src/plugins/ui_feature/renderer.ts b/src/plugins/ui_feature/renderer.ts index a3ac75ec2..aea8305ea 100644 --- a/src/plugins/ui_feature/renderer.ts +++ b/src/plugins/ui_feature/renderer.ts @@ -29,6 +29,7 @@ import { drawDecoratedText, getZonesCols, getZonesRows, + largeMin, numberToLetters, overlap, positionToZone, @@ -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); @@ -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); diff --git a/src/plugins/ui_feature/ui_sheet.ts b/src/plugins/ui_feature/ui_sheet.ts index 4844b9229..64313ba90 100644 --- a/src/plugins/ui_feature/ui_sheet.ts +++ b/src/plugins/ui_feature/ui_sheet.ts @@ -2,6 +2,7 @@ import { GRID_ICON_MARGIN, ICON_EDGE_LENGTH, PADDING_AUTORESIZE_HORIZONTAL } fro import { computeIconWidth, computeTextWidth, + largeMax, positions, splitTextToWidth, } from "../../helpers/index"; @@ -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)); } /** diff --git a/src/xlsx/conversion/sheet_conversion.ts b/src/xlsx/conversion/sheet_conversion.ts index 8119d986e..1eca072b6 100644 --- a/src/xlsx/conversion/sheet_conversion.ts +++ b/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 { @@ -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); } diff --git a/src/xlsx/functions/charts.ts b/src/xlsx/functions/charts.ts index cdcb2dd76..b8c773818 100644 --- a/src/xlsx/functions/charts.ts +++ b/src/xlsx/functions/charts.ts @@ -1,4 +1,4 @@ -import { range } from "../../helpers"; +import { largeMax, range } from "../../helpers"; import { ChartColors } from "../../helpers/figures/charts"; import { Color, ExcelWorkbookData, FigureData } from "../../types"; import { ExcelChartDefinition } from "../../types/chart/chart"; @@ -310,8 +310,8 @@ function addDoughnutChart( ) { const colors = new ChartColors(); - const maxLength = Math.max( - ...chart.dataSets.map((ds) => getRangeSize(ds.range, chartSheetIndex, data)) + const maxLength = largeMax( + chart.dataSets.map((ds) => getRangeSize(ds.range, chartSheetIndex, data)) ); const doughnutColors: string[] = range(0, maxLength).map(() => toXlsxHexColor(colors.next()));