From 5ab587592de6bf5d890eb3ebc10c773a09b7ce2f Mon Sep 17 00:00:00 2001 From: marcin-kordas-hoc Date: Sat, 7 Mar 2026 14:52:46 +0000 Subject: [PATCH 01/15] Fix #1629: resolve memory leaks in LazilyTransformingAstService and UndoRedo - Add versionOffset compaction to LTAS so transformations array does not grow unboundedly; compact() fires automatically in recomputeIfDependencyGraphNeedsIt (before partialRun) after all consumers (FormulaVertex, ColumnIndex) are synchronized - Guard storeDataForVersion with early return when undoLimit === 0 to prevent oldData map growth when undo is disabled - Add forceApplyPostponedTransformations to ColumnIndex and ColumnSearchStrategy interface to synchronize all consumers before compaction - Coordinate compaction in Operations.forceApplyPostponedTransformations called from UndoRedo before irreversible undo operations --- src/HyperFormula.ts | 6 ++++++ src/LazilyTransformingAstService.ts | 32 +++++++++++++++++++++++------ src/Lookup/ColumnBinarySearch.ts | 3 +++ src/Lookup/ColumnIndex.ts | 18 ++++++++++++++++ src/Lookup/SearchStrategy.ts | 7 +++++++ src/Operations.ts | 2 ++ src/UndoRedo.ts | 12 +++++++++++ 7 files changed, 74 insertions(+), 6 deletions(-) diff --git a/src/HyperFormula.ts b/src/HyperFormula.ts index e7c4b5150c..e98356d404 100644 --- a/src/HyperFormula.ts +++ b/src/HyperFormula.ts @@ -4610,6 +4610,12 @@ export class HyperFormula implements TypedEmitter { const verticesToRecomputeFrom = this.dependencyGraph.verticesToRecompute() this.dependencyGraph.clearDirtyVertices() + if (this._lazilyTransformingAstService.needsCompaction()) { + this._dependencyGraph.forceApplyPostponedTransformations() + this._columnSearch.forceApplyPostponedTransformations() + this._lazilyTransformingAstService.compact() + } + if (verticesToRecomputeFrom.length > 0) { changes.addAll(this.evaluator.partialRun(verticesToRecomputeFrom)) } diff --git a/src/LazilyTransformingAstService.ts b/src/LazilyTransformingAstService.ts index 899f89d2ac..978be3399b 100644 --- a/src/LazilyTransformingAstService.ts +++ b/src/LazilyTransformingAstService.ts @@ -17,6 +17,7 @@ export class LazilyTransformingAstService { public undoRedo?: UndoRedo private transformations: FormulaTransformer[] = [] + private versionOffset: number = 0 private combinedTransformer?: CombinedTransformer constructor( @@ -25,7 +26,7 @@ export class LazilyTransformingAstService { } public version(): number { - return this.transformations.length + return this.versionOffset + this.transformations.length } public addTransformation(transformation: FormulaTransformer): number { @@ -53,8 +54,9 @@ export class LazilyTransformingAstService { public applyTransformations(ast: Ast, address: SimpleCellAddress, version: number): [Ast, SimpleCellAddress, number] { this.stats.start(StatType.TRANSFORM_ASTS_POSTPONED) - for (let v = version; v < this.transformations.length; v++) { - const transformation = this.transformations[v] + const currentVersion = this.version() + for (let v = Math.max(version, this.versionOffset); v < currentVersion; v++) { + const transformation = this.transformations[v - this.versionOffset] if (transformation.isIrreversible()) { this.undoRedo!.storeDataForVersion(v, address, this.parser!.computeHashFromAst(ast)) this.parser!.rememberNewAst(ast) @@ -67,15 +69,33 @@ export class LazilyTransformingAstService { const cachedAst = this.parser!.rememberNewAst(ast) this.stats.end(StatType.TRANSFORM_ASTS_POSTPONED) - return [cachedAst, address, this.transformations.length] + return [cachedAst, address, currentVersion] } public* getTransformationsFrom(version: number, filter?: (transformation: FormulaTransformer) => boolean): IterableIterator { - for (let v = version; v < this.transformations.length; v++) { - const transformation = this.transformations[v] + const currentVersion = this.version() + for (let v = Math.max(version, this.versionOffset); v < currentVersion; v++) { + const transformation = this.transformations[v - this.versionOffset] if (!filter || filter(transformation)) { yield transformation } } } + + /** + * Returns true if there are pending transformations that can be compacted. + */ + public needsCompaction(): boolean { + return this.transformations.length > 0 + } + + /** + * Compacts the transformations array by discarding all entries that have already + * been applied by every consumer. Safe to call only after all FormulaVertex and + * ColumnIndex consumers have been brought up to the current version. + */ + public compact(): void { + this.versionOffset += this.transformations.length + this.transformations = [] + } } diff --git a/src/Lookup/ColumnBinarySearch.ts b/src/Lookup/ColumnBinarySearch.ts index 458070687c..15d6c613f5 100644 --- a/src/Lookup/ColumnBinarySearch.ts +++ b/src/Lookup/ColumnBinarySearch.ts @@ -53,6 +53,9 @@ export class ColumnBinarySearch extends AdvancedFind implements ColumnSearchStra public removeValues(range: IterableIterator<[RawScalarValue, SimpleCellAddress]>): void { } + public forceApplyPostponedTransformations(): void { + } + /* * WARNING: Finding lower/upper bounds in unordered ranges is not supported. When ordering === 'none', assumes matchExactly === true */ diff --git a/src/Lookup/ColumnIndex.ts b/src/Lookup/ColumnIndex.ts index ba6a3c9201..2d0d17df2d 100644 --- a/src/Lookup/ColumnIndex.ts +++ b/src/Lookup/ColumnIndex.ts @@ -184,6 +184,24 @@ export class ColumnIndex implements ColumnSearchStrategy { this.index.delete(sheetId) } + /** + * Forces all ValueIndex entries to apply any pending lazy transformations, + * bringing every entry up to the current LTAS version. + * Must be called before compacting LazilyTransformingAstService. + */ + public forceApplyPostponedTransformations(): void { + for (const [sheet, sheetIndex] of this.index) { + sheetIndex.forEach((columnMap, col) => { + if (!columnMap) { + return + } + for (const value of columnMap.keys()) { + this.ensureRecentData(sheet, col, value) + } + }) + } + } + public getColumnMap(sheet: number, col: number): ColumnMap { if (!this.index.has(sheet)) { this.index.set(sheet, []) diff --git a/src/Lookup/SearchStrategy.ts b/src/Lookup/SearchStrategy.ts index a12a48fc6a..a44480037a 100644 --- a/src/Lookup/SearchStrategy.ts +++ b/src/Lookup/SearchStrategy.ts @@ -51,6 +51,13 @@ export interface ColumnSearchStrategy extends SearchStrategy { moveValues(range: IterableIterator<[RawScalarValue, SimpleCellAddress]>, toRight: number, toBottom: number, toSheet: number): void, removeValues(range: IterableIterator<[RawScalarValue, SimpleCellAddress]>): void, + + /** + * Forces all lazily-tracked ValueIndex entries to apply any pending transformations, + * bringing every entry's version up to the current LTAS version. + * Must be called before compacting LazilyTransformingAstService. + */ + forceApplyPostponedTransformations(): void, } export function buildColumnSearchStrategy(dependencyGraph: DependencyGraph, config: Config, statistics: Statistics): ColumnSearchStrategy { diff --git a/src/Operations.ts b/src/Operations.ts index 27f45c2d9c..094b27b82a 100644 --- a/src/Operations.ts +++ b/src/Operations.ts @@ -739,6 +739,8 @@ export class Operations { public forceApplyPostponedTransformations(): void { this.dependencyGraph.forceApplyPostponedTransformations() + this.columnSearch.forceApplyPostponedTransformations() + this.dependencyGraph.lazilyTransformingAstService.compact() } /** diff --git a/src/UndoRedo.ts b/src/UndoRedo.ts index 5413a79d59..345027179b 100644 --- a/src/UndoRedo.ts +++ b/src/UndoRedo.ts @@ -458,6 +458,9 @@ export class UndoRedo { } public storeDataForVersion(version: number, address: SimpleCellAddress, astHash: string) { + if (this.undoLimit === 0) { + return + } if (!this.oldData.has(version)) { this.oldData.set(version, []) } @@ -771,6 +774,15 @@ export class UndoRedo { this.operations.setColumnOrder(operation.sheetId, operation.columnMapping) } + /** + * Adds an entry to the undo stack, evicting the oldest entry when undoLimit is exceeded. + * + * Known limitation (follow-up for issue #1629): when an entry is evicted by the splice + * below, its corresponding `oldData` keys are never removed from the map. Fixing this + * properly requires each UndoEntry to record which LTAS version keys it references so + * they can be deleted on eviction. Until then, `oldData` has a minor leak when + * `undoLimit > 0` and many structural operations are performed. + */ private addUndoEntry(operation: UndoEntry) { this.undoStack.push(operation) this.undoStack.splice(0, Math.max(0, this.undoStack.length - this.undoLimit)) From cee787b7a7a9cf5094f4bc61895f5769a7cc34ee Mon Sep 17 00:00:00 2001 From: marcin-kordas-hoc Date: Mon, 9 Mar 2026 18:49:38 +0000 Subject: [PATCH 02/15] Fix oldData memory leak when undoLimit > 0 Clean up oldData map entries when undo/redo entries are permanently discarded (eviction from undo stack, clearRedoStack, clearUndoStack). Each UndoEntry now reports which LTAS version keys it references via getReferencedOldDataVersions(), enabling targeted cleanup on eviction. Resolves the secondary memory leak documented in #1629 follow-up: oldData.size now stays bounded by undoLimit instead of growing linearly with total structural operations. --- src/UndoRedo.ts | 66 +++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 58 insertions(+), 8 deletions(-) diff --git a/src/UndoRedo.ts b/src/UndoRedo.ts index 345027179b..4538c0edc8 100644 --- a/src/UndoRedo.ts +++ b/src/UndoRedo.ts @@ -22,12 +22,22 @@ export interface UndoEntry { doUndo(undoRedo: UndoRedo): void, doRedo(undoRedo: UndoRedo): void, + + /** + * Returns the LTAS version keys referenced by this entry's oldData storage. + * Used to clean up oldData when the entry is permanently evicted from the undo/redo stack. + */ + getReferencedOldDataVersions(): number[], } export abstract class BaseUndoEntry implements UndoEntry { abstract doUndo(undoRedo: UndoRedo): void abstract doRedo(undoRedo: UndoRedo): void + + public getReferencedOldDataVersions(): number[] { + return [] + } } export class RemoveRowsUndoEntry extends BaseUndoEntry { @@ -45,6 +55,10 @@ export class RemoveRowsUndoEntry extends BaseUndoEntry { public doRedo(undoRedo: UndoRedo): void { undoRedo.redoRemoveRows(this) } + + public getReferencedOldDataVersions(): number[] { + return this.rowsRemovals.map(r => r.version - 1) + } } export class MoveCellsUndoEntry extends BaseUndoEntry { @@ -67,6 +81,10 @@ export class MoveCellsUndoEntry extends BaseUndoEntry { public doRedo(undoRedo: UndoRedo): void { undoRedo.redoMoveCells(this) } + + public getReferencedOldDataVersions(): number[] { + return [this.version - 1] + } } export class AddRowsUndoEntry extends BaseUndoEntry { @@ -162,6 +180,10 @@ export class MoveRowsUndoEntry extends BaseUndoEntry { public doRedo(undoRedo: UndoRedo): void { undoRedo.redoMoveRows(this) } + + public getReferencedOldDataVersions(): number[] { + return [this.version - 1] + } } export class MoveColumnsUndoEntry extends BaseUndoEntry { @@ -187,6 +209,10 @@ export class MoveColumnsUndoEntry extends BaseUndoEntry { public doRedo(undoRedo: UndoRedo): void { undoRedo.redoMoveColumns(this) } + + public getReferencedOldDataVersions(): number[] { + return [this.version - 1] + } } export class AddColumnsUndoEntry extends BaseUndoEntry { @@ -220,6 +246,10 @@ export class RemoveColumnsUndoEntry extends BaseUndoEntry { public doRedo(undoRedo: UndoRedo): void { undoRedo.redoRemoveColumns(this) } + + public getReferencedOldDataVersions(): number[] { + return this.columnsRemovals.map(r => r.version - 1) + } } export class AddSheetUndoEntry extends BaseUndoEntry { @@ -286,6 +316,10 @@ export class RenameSheetUndoEntry extends BaseUndoEntry { public doRedo(undoRedo: UndoRedo): void { undoRedo.redoRenameSheet(this) } + + public getReferencedOldDataVersions(): number[] { + return this.version !== undefined ? [this.version - 1] : [] + } } export class ClearSheetUndoEntry extends BaseUndoEntry { @@ -421,6 +455,10 @@ export class BatchUndoEntry extends BaseUndoEntry { public doRedo(undoRedo: UndoRedo): void { undoRedo.redoBatch(this) } + + public getReferencedOldDataVersions(): number[] { + return this.operations.flatMap(op => op.getReferencedOldDataVersions()) + } } export class UndoRedo { @@ -469,10 +507,12 @@ export class UndoRedo { } public clearRedoStack() { + this.cleanupOldDataForEntries(this.redoStack) this.redoStack = [] } public clearUndoStack() { + this.cleanupOldDataForEntries(this.undoStack) this.undoStack = [] } @@ -775,17 +815,27 @@ export class UndoRedo { } /** - * Adds an entry to the undo stack, evicting the oldest entry when undoLimit is exceeded. - * - * Known limitation (follow-up for issue #1629): when an entry is evicted by the splice - * below, its corresponding `oldData` keys are never removed from the map. Fixing this - * properly requires each UndoEntry to record which LTAS version keys it references so - * they can be deleted on eviction. Until then, `oldData` has a minor leak when - * `undoLimit > 0` and many structural operations are performed. + * Adds an entry to the undo stack, evicting the oldest entries when undoLimit is exceeded. + * Evicted entries have their oldData keys cleaned up to prevent memory leaks. */ private addUndoEntry(operation: UndoEntry) { this.undoStack.push(operation) - this.undoStack.splice(0, Math.max(0, this.undoStack.length - this.undoLimit)) + const evictCount = Math.max(0, this.undoStack.length - this.undoLimit) + if (evictCount > 0) { + const evicted = this.undoStack.splice(0, evictCount) + this.cleanupOldDataForEntries(evicted) + } + } + + /** + * Removes oldData entries referenced by permanently discarded undo/redo entries. + */ + private cleanupOldDataForEntries(entries: UndoEntry[]) { + for (const entry of entries) { + for (const version of entry.getReferencedOldDataVersions()) { + this.oldData.delete(version) + } + } } private undoEntry(operation: UndoEntry) { From 106767af489c57c6962bad81790a7ba03e7e917c Mon Sep 17 00:00:00 2001 From: marcin-kordas-hoc Date: Mon, 16 Mar 2026 11:26:17 +0000 Subject: [PATCH 03/15] Optimize compaction: add threshold and fix orphaned oldData race condition - Add COMPACTION_THRESHOLD=50 to LazilyTransformingAstService to defer compaction until enough transformations accumulate, preserving lazy evaluation for small operation batches - Fix race condition: when compaction forces lazy formula evaluation, storeDataForVersion() may insert oldData entries for already-evicted undo entries. cleanupOrphanedOldData() removes these orphans after each compaction cycle - Without threshold, every structural op triggered full vertex traversal, effectively defeating lazy transformations (~18x slower for 2.5k formulas) --- src/HyperFormula.ts | 1 + src/LazilyTransformingAstService.ts | 7 +++++-- src/UndoRedo.ts | 17 +++++++++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/HyperFormula.ts b/src/HyperFormula.ts index e98356d404..f6e8d496cc 100644 --- a/src/HyperFormula.ts +++ b/src/HyperFormula.ts @@ -4614,6 +4614,7 @@ export class HyperFormula implements TypedEmitter { this._dependencyGraph.forceApplyPostponedTransformations() this._columnSearch.forceApplyPostponedTransformations() this._lazilyTransformingAstService.compact() + this._lazilyTransformingAstService.undoRedo?.cleanupOrphanedOldData() } if (verticesToRecomputeFrom.length > 0) { diff --git a/src/LazilyTransformingAstService.ts b/src/LazilyTransformingAstService.ts index 978be3399b..1e63981dcd 100644 --- a/src/LazilyTransformingAstService.ts +++ b/src/LazilyTransformingAstService.ts @@ -16,6 +16,8 @@ export class LazilyTransformingAstService { public parser?: ParserWithCaching public undoRedo?: UndoRedo + private static readonly COMPACTION_THRESHOLD = 50 + private transformations: FormulaTransformer[] = [] private versionOffset: number = 0 private combinedTransformer?: CombinedTransformer @@ -83,10 +85,11 @@ export class LazilyTransformingAstService { } /** - * Returns true if there are pending transformations that can be compacted. + * Returns true when enough transformations have accumulated to justify the cost + * of forcing all consumers (FormulaVertex, ColumnIndex) to apply pending changes. */ public needsCompaction(): boolean { - return this.transformations.length > 0 + return this.transformations.length >= LazilyTransformingAstService.COMPACTION_THRESHOLD } /** diff --git a/src/UndoRedo.ts b/src/UndoRedo.ts index 4538c0edc8..23e5aab76e 100644 --- a/src/UndoRedo.ts +++ b/src/UndoRedo.ts @@ -827,6 +827,23 @@ export class UndoRedo { } } + /** + * Removes oldData entries whose version keys are not referenced by any + * entry on the undo or redo stack. Called after compaction forces lazy + * formula evaluation, which may insert oldData for already-evicted entries. + */ + public cleanupOrphanedOldData() { + const referencedVersions = new Set([ + ...this.undoStack.flatMap(e => e.getReferencedOldDataVersions()), + ...this.redoStack.flatMap(e => e.getReferencedOldDataVersions()), + ]) + for (const version of this.oldData.keys()) { + if (!referencedVersions.has(version)) { + this.oldData.delete(version) + } + } + } + /** * Removes oldData entries referenced by permanently discarded undo/redo entries. */ From 350e42e075e4094fc3fa2dbeae248f7b6bbf44e7 Mon Sep 17 00:00:00 2001 From: marcin-kordas-hoc Date: Mon, 16 Mar 2026 15:00:37 +0000 Subject: [PATCH 04/15] Remove redundant compact() from Operations.forceApplyPostponedTransformations Undo path only needs force-apply to bring consumers up-to-date before inverse operations. Compaction is handled centrally in HyperFormula.recomputeIfDependencyGraphNeedsIt(), eliminating double traversal during undo and resolving the DRY duplication. --- src/Operations.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Operations.ts b/src/Operations.ts index 094b27b82a..50a7407b1f 100644 --- a/src/Operations.ts +++ b/src/Operations.ts @@ -740,7 +740,6 @@ export class Operations { public forceApplyPostponedTransformations(): void { this.dependencyGraph.forceApplyPostponedTransformations() this.columnSearch.forceApplyPostponedTransformations() - this.dependencyGraph.lazilyTransformingAstService.compact() } /** From 39fba38648e571e500c56d994a292b00ab87ce38 Mon Sep 17 00:00:00 2001 From: marcin-kordas-hoc Date: Tue, 17 Mar 2026 08:53:44 +0000 Subject: [PATCH 05/15] Fix code review issues: version guard, forceApply parity, JSDoc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Guard version > 0 in getReferencedOldDataVersions() for MoveCellsUndoEntry, MoveRowsUndoEntry, MoveColumnsUndoEntry — prevents returning [-1] when no transformation was added (e.g. moving empty range) - Add forceApplyPostponedTransformations() to undoMoveRows/undoMoveColumns, matching the pattern already present in undoRemoveRows, undoRemoveColumns, undoMoveCells, undoRemoveSheet, undoRenameSheet - Extend compact() JSDoc to document the cleanupOrphanedOldData() postcondition --- src/LazilyTransformingAstService.ts | 3 +++ src/UndoRedo.ts | 8 +++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/LazilyTransformingAstService.ts b/src/LazilyTransformingAstService.ts index 1e63981dcd..49212ac752 100644 --- a/src/LazilyTransformingAstService.ts +++ b/src/LazilyTransformingAstService.ts @@ -96,6 +96,9 @@ export class LazilyTransformingAstService { * Compacts the transformations array by discarding all entries that have already * been applied by every consumer. Safe to call only after all FormulaVertex and * ColumnIndex consumers have been brought up to the current version. + * After calling, UndoRedo.cleanupOrphanedOldData() must be invoked to remove + * oldData entries written during forceApplyPostponedTransformations for + * already-evicted undo entries. */ public compact(): void { this.versionOffset += this.transformations.length diff --git a/src/UndoRedo.ts b/src/UndoRedo.ts index 23e5aab76e..36d8d9f792 100644 --- a/src/UndoRedo.ts +++ b/src/UndoRedo.ts @@ -83,7 +83,7 @@ export class MoveCellsUndoEntry extends BaseUndoEntry { } public getReferencedOldDataVersions(): number[] { - return [this.version - 1] + return this.version > 0 ? [this.version - 1] : [] } } @@ -182,7 +182,7 @@ export class MoveRowsUndoEntry extends BaseUndoEntry { } public getReferencedOldDataVersions(): number[] { - return [this.version - 1] + return this.version > 0 ? [this.version - 1] : [] } } @@ -211,7 +211,7 @@ export class MoveColumnsUndoEntry extends BaseUndoEntry { } public getReferencedOldDataVersions(): number[] { - return [this.version - 1] + return this.version > 0 ? [this.version - 1] : [] } } @@ -608,12 +608,14 @@ export class UndoRedo { } public undoMoveRows(operation: MoveRowsUndoEntry) { + this.operations.forceApplyPostponedTransformations() const {sheet} = operation this.operations.moveRows(sheet, operation.undoStart, operation.numberOfRows, operation.undoEnd) this.restoreOldDataFromVersion(operation.version - 1) } public undoMoveColumns(operation: MoveColumnsUndoEntry) { + this.operations.forceApplyPostponedTransformations() const {sheet} = operation this.operations.moveColumns(sheet, operation.undoStart, operation.numberOfColumns, operation.undoEnd) this.restoreOldDataFromVersion(operation.version - 1) From ec97e2e3c83064090b16ba6f4d8f220b6ebb663a Mon Sep 17 00:00:00 2001 From: marcin-kordas-hoc Date: Wed, 18 Mar 2026 09:25:09 +0000 Subject: [PATCH 06/15] ci: retrigger CI with hyperformula-tests fix/1629 branch From 5628e25c1eca259b24c2a5beab40479a3f9ef4d7 Mon Sep 17 00:00:00 2001 From: marcin-kordas-hoc Date: Wed, 25 Mar 2026 00:12:25 +0000 Subject: [PATCH 07/15] Address sequba's code review: configurable compactionThreshold, JSDoc, helpers --- CHANGELOG.md | 2 ++ src/BuildEngineFactory.ts | 2 +- src/Config.ts | 6 +++++ src/ConfigParams.ts | 12 +++++++++ src/HyperFormula.ts | 21 +++++++++++----- src/LazilyTransformingAstService.ts | 38 ++++++++++++++++++++++++++--- src/Lookup/ColumnBinarySearch.ts | 7 ++++++ src/Lookup/ColumnIndex.ts | 2 +- src/Lookup/SearchStrategy.ts | 2 +- src/UndoRedo.ts | 30 ++++++++++++++++++++++- 10 files changed, 109 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e08b5fcba..10dfa11509 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Fixed +- Fixed a memory leak in `LazilyTransformingAstService` where the transformations array grew unboundedly, causing increasing memory usage over time. [#1629](https://github.com/handsontable/hyperformula/issues/1629) +- Fixed a memory leak in `UndoRedo` where `oldData` entries for evicted undo stack entries were never cleaned up, causing increasing memory usage over time. [#1629](https://github.com/handsontable/hyperformula/issues/1629) - Fixed the IRR function returning `#NUM!` error when the initial investment significantly exceeds the sum of returns. [#1628](https://github.com/handsontable/hyperformula/issues/1628) - Fixed the ADDRESS function ignoring `defaultValue` when arguments are syntactically empty (e.g., `=ADDRESS(2,3,,FALSE())`). [#1632](https://github.com/handsontable/hyperformula/issues/1632) diff --git a/src/BuildEngineFactory.ts b/src/BuildEngineFactory.ts index 1faff1e889..97f09a0d95 100644 --- a/src/BuildEngineFactory.ts +++ b/src/BuildEngineFactory.ts @@ -72,7 +72,7 @@ export class BuildEngineFactory { const namedExpressions = new NamedExpressions() const functionRegistry = new FunctionRegistry(config) - const lazilyTransformingAstService = new LazilyTransformingAstService(stats) + const lazilyTransformingAstService = new LazilyTransformingAstService(stats, config.compactionThreshold) const dependencyGraph = DependencyGraph.buildEmpty(lazilyTransformingAstService, config, functionRegistry, namedExpressions, stats) const columnSearch = buildColumnSearchStrategy(dependencyGraph, config, stats) const sheetMapping = dependencyGraph.sheetMapping diff --git a/src/Config.ts b/src/Config.ts index c345c97e95..f362e0fa00 100644 --- a/src/Config.ts +++ b/src/Config.ts @@ -62,6 +62,7 @@ export class Config implements ConfigParams, ParserConfig { timeFormats: ['hh:mm', 'hh:mm:ss.sss'], thousandSeparator: '', undoLimit: 20, + compactionThreshold: 50, useRegularExpressions: false, useWildcards: true, useColumnIndex: false, @@ -135,6 +136,8 @@ export class Config implements ConfigParams, ParserConfig { /** @inheritDoc */ public readonly undoLimit: number /** @inheritDoc */ + public readonly compactionThreshold: number + /** @inheritDoc */ public readonly context: unknown /** @@ -198,6 +201,7 @@ export class Config implements ConfigParams, ParserConfig { useArrayArithmetic, useStats, undoLimit, + compactionThreshold, useColumnIndex, useRegularExpressions, useWildcards, @@ -244,10 +248,12 @@ export class Config implements ConfigParams, ParserConfig { this.nullDate = configValueFromParamCheck(nullDate, instanceOfSimpleDate, 'IDate', 'nullDate') this.leapYear1900 = configValueFromParam(leapYear1900, 'boolean', 'leapYear1900') this.undoLimit = configValueFromParam(undoLimit, 'number', 'undoLimit') + this.compactionThreshold = configValueFromParam(compactionThreshold, 'number', 'compactionThreshold') this.useRegularExpressions = configValueFromParam(useRegularExpressions, 'boolean', 'useRegularExpressions') this.useWildcards = configValueFromParam(useWildcards, 'boolean', 'useWildcards') this.matchWholeCell = configValueFromParam(matchWholeCell, 'boolean', 'matchWholeCell') validateNumberToBeAtLeast(this.undoLimit, 'undoLimit', 0) + validateNumberToBeAtLeast(this.compactionThreshold, 'compactionThreshold', 1) this.maxRows = configValueFromParam(maxRows, 'number', 'maxRows') validateNumberToBeAtLeast(this.maxRows, 'maxRows', 1) this.maxColumns = configValueFromParam(maxColumns, 'number', 'maxColumns') diff --git a/src/ConfigParams.ts b/src/ConfigParams.ts index e036731c4f..2a8067beef 100644 --- a/src/ConfigParams.ts +++ b/src/ConfigParams.ts @@ -402,6 +402,18 @@ export interface ConfigParams { * @category Undo and Redo */ undoLimit: number, + /** + * Sets the number of accumulated formula transformations that triggers compaction + * of the LazilyTransformingAstService. When the number of pending transformations + * reaches this threshold, all formula vertices and column indexes are forced to + * apply their postponed transformations, and the transformation history is cleared. + * + * Lower values cause more frequent compaction (useful for testing), while higher + * values reduce overhead at the cost of more memory usage. + * @default 50 + * @category Engine + */ + compactionThreshold: number, /** * When set to `true`, criteria in functions (SUMIF, COUNTIF, ...) are allowed to use regular expressions. * @default false diff --git a/src/HyperFormula.ts b/src/HyperFormula.ts index f6e8d496cc..9194b7d32e 100644 --- a/src/HyperFormula.ts +++ b/src/HyperFormula.ts @@ -4604,18 +4604,27 @@ export class HyperFormula implements TypedEmitter { * * @fires [[valuesUpdated]] if recalculation was triggered by this change */ + /** + * When enough transformations have accumulated, forces all formula vertices and + * column index entries to apply pending lazy transformations, then compacts the + * transformation history and cleans up orphaned undo oldData entries. + */ + private compactLazyTransformationsIfNeeded(): void { + if (this._lazilyTransformingAstService.needsCompaction()) { + this._dependencyGraph.forceApplyPostponedTransformations() + this._columnSearch.forceApplyPostponedTransformations() + this._lazilyTransformingAstService.compact() + this._lazilyTransformingAstService.undoRedo?.cleanupOrphanedOldData() + } + } + private recomputeIfDependencyGraphNeedsIt(): ExportedChange[] { if (!this._evaluationSuspended) { const changes = this._crudOperations.getAndClearContentChanges() const verticesToRecomputeFrom = this.dependencyGraph.verticesToRecompute() this.dependencyGraph.clearDirtyVertices() - if (this._lazilyTransformingAstService.needsCompaction()) { - this._dependencyGraph.forceApplyPostponedTransformations() - this._columnSearch.forceApplyPostponedTransformations() - this._lazilyTransformingAstService.compact() - this._lazilyTransformingAstService.undoRedo?.cleanupOrphanedOldData() - } + this.compactLazyTransformationsIfNeeded() if (verticesToRecomputeFrom.length > 0) { changes.addAll(this.evaluator.partialRun(verticesToRecomputeFrom)) diff --git a/src/LazilyTransformingAstService.ts b/src/LazilyTransformingAstService.ts index 49212ac752..acd517577b 100644 --- a/src/LazilyTransformingAstService.ts +++ b/src/LazilyTransformingAstService.ts @@ -11,19 +11,51 @@ import {StatType} from './statistics' import {Statistics} from './statistics/Statistics' import {UndoRedo} from './UndoRedo' +/** + * Manages lazy application of formula AST transformations. + * + * ## Problem + * Structural operations (adding/removing rows/columns, moving cells, renaming sheets) + * require updating every formula that references the affected area. Applying these + * transformations eagerly to all formulas after every operation is expensive, especially + * for large spreadsheets with many formulas. + * + * ## Solution: Lazy Transformation + * Instead of transforming all formulas immediately, this service stores transformations + * in a queue. Each formula vertex (FormulaVertex) and column index entry (ValueIndex) + * tracks its own version number. When a consumer needs up-to-date data, it calls + * `applyTransformations()` with its current version and receives all transformations + * accumulated since that version. + * + * ## Compaction + * Over time, the transformations array grows unboundedly. To prevent this memory leak, + * the engine periodically triggers compaction when the number of accumulated + * transformations reaches the configurable `compactionThreshold`: + * + * 1. All FormulaVertex instances are forced to apply pending transformations + * (via `DependencyGraph.forceApplyPostponedTransformations()`). + * 2. All ColumnIndex entries are forced to apply pending transformations + * (via `ColumnSearchStrategy.forceApplyPostponedTransformations()`). + * 3. `compact()` is called, which advances `versionOffset` and clears the + * transformations array. + * 4. `UndoRedo.cleanupOrphanedOldData()` removes any oldData entries that were + * written during forced application but belong to already-evicted undo entries. + * + * The `versionOffset` ensures that version numbers remain globally consistent + * after compaction: `version() = versionOffset + transformations.length`. + */ export class LazilyTransformingAstService { public parser?: ParserWithCaching public undoRedo?: UndoRedo - private static readonly COMPACTION_THRESHOLD = 50 - private transformations: FormulaTransformer[] = [] private versionOffset: number = 0 private combinedTransformer?: CombinedTransformer constructor( private readonly stats: Statistics, + private readonly compactionThreshold: number, ) { } @@ -89,7 +121,7 @@ export class LazilyTransformingAstService { * of forcing all consumers (FormulaVertex, ColumnIndex) to apply pending changes. */ public needsCompaction(): boolean { - return this.transformations.length >= LazilyTransformingAstService.COMPACTION_THRESHOLD + return this.transformations.length >= this.compactionThreshold } /** diff --git a/src/Lookup/ColumnBinarySearch.ts b/src/Lookup/ColumnBinarySearch.ts index 15d6c613f5..cf034d11cc 100644 --- a/src/Lookup/ColumnBinarySearch.ts +++ b/src/Lookup/ColumnBinarySearch.ts @@ -53,6 +53,13 @@ export class ColumnBinarySearch extends AdvancedFind implements ColumnSearchStra public removeValues(range: IterableIterator<[RawScalarValue, SimpleCellAddress]>): void { } + /** + * No-op: ColumnBinarySearch reads cell values directly from the dependency graph + * on every lookup, so it has no cached data that could become stale. + * Unlike ColumnIndex, which maintains a separate value-to-address index that + * must be kept in sync with lazy transformations, binary search always operates + * on the current graph state. + */ public forceApplyPostponedTransformations(): void { } diff --git a/src/Lookup/ColumnIndex.ts b/src/Lookup/ColumnIndex.ts index 2d0d17df2d..d7c32e5894 100644 --- a/src/Lookup/ColumnIndex.ts +++ b/src/Lookup/ColumnIndex.ts @@ -186,7 +186,7 @@ export class ColumnIndex implements ColumnSearchStrategy { /** * Forces all ValueIndex entries to apply any pending lazy transformations, - * bringing every entry up to the current LTAS version. + * bringing every entry up to the current LazilyTransformingAstService version. * Must be called before compacting LazilyTransformingAstService. */ public forceApplyPostponedTransformations(): void { diff --git a/src/Lookup/SearchStrategy.ts b/src/Lookup/SearchStrategy.ts index a44480037a..2316e586c9 100644 --- a/src/Lookup/SearchStrategy.ts +++ b/src/Lookup/SearchStrategy.ts @@ -54,7 +54,7 @@ export interface ColumnSearchStrategy extends SearchStrategy { /** * Forces all lazily-tracked ValueIndex entries to apply any pending transformations, - * bringing every entry's version up to the current LTAS version. + * bringing every entry's version up to the current LazilyTransformingAstService version. * Must be called before compacting LazilyTransformingAstService. */ forceApplyPostponedTransformations(): void, diff --git a/src/UndoRedo.ts b/src/UndoRedo.ts index 36d8d9f792..cdd922c077 100644 --- a/src/UndoRedo.ts +++ b/src/UndoRedo.ts @@ -24,7 +24,7 @@ export interface UndoEntry { doRedo(undoRedo: UndoRedo): void, /** - * Returns the LTAS version keys referenced by this entry's oldData storage. + * Returns the LazilyTransformingAstService version keys referenced by this entry's oldData storage. * Used to clean up oldData when the entry is permanently evicted from the undo/redo stack. */ getReferencedOldDataVersions(): number[], @@ -461,6 +461,34 @@ export class BatchUndoEntry extends BaseUndoEntry { } } +/** + * Manages undo/redo stacks for all spreadsheet operations. + * + * ## oldData: Preserving Formula ASTs Across Irreversible Transformations + * + * Some structural operations (e.g., removing rows/columns, moving cells) destroy + * formula information that cannot be reconstructed from the transformation alone. + * For example, when a row is removed, formulas referencing that row are rewritten + * to `#REF!` — an irreversible change. + * + * To support undo of such operations, `oldData` stores snapshots of formula AST + * hashes keyed by the LazilyTransformingAstService version at which the irreversible + * transformation was applied. Each entry maps a version number to an array of + * `[cellAddress, astHash]` pairs that can be used to restore the original formula + * from the parser cache. + * + * ### Memory Management + * + * Without cleanup, `oldData` grows indefinitely because: + * 1. When undo entries are evicted (due to `undoLimit`), their referenced oldData + * keys are deleted via `cleanupOldDataForEntries()`. + * 2. When compaction forces lazy formula evaluation, it may write new oldData entries + * for versions that belong to already-evicted undo entries. These orphaned entries + * are cleaned up by `cleanupOrphanedOldData()`, which retains only versions + * still referenced by entries on the undo or redo stack. + * 3. When `undoLimit` is 0 (undo disabled), `storeDataForVersion()` short-circuits + * to avoid storing data that would never be used. + */ export class UndoRedo { public oldData: Map = new Map() private undoStack: UndoEntry[] = [] From 5e1a2172b76d384f16d5494fae6f608bcc3cca86 Mon Sep 17 00:00:00 2001 From: marcin-kordas-hoc Date: Wed, 25 Mar 2026 01:45:01 +0000 Subject: [PATCH 08/15] Address review: JSDoc, CHANGELOG Added entry for compactionThreshold --- CHANGELOG.md | 4 ++++ src/Operations.ts | 5 +++++ src/UndoRedo.ts | 4 ++++ 3 files changed, 13 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 10dfa11509..1e66fd0c9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ## [Unreleased] +### Added + +- Added `compactionThreshold` configuration option to control how often the engine compacts accumulated formula transformations. [#1629](https://github.com/handsontable/hyperformula/issues/1629) + ### Fixed - Fixed a memory leak in `LazilyTransformingAstService` where the transformations array grew unboundedly, causing increasing memory usage over time. [#1629](https://github.com/handsontable/hyperformula/issues/1629) diff --git a/src/Operations.ts b/src/Operations.ts index 50a7407b1f..6a0e5ccdd4 100644 --- a/src/Operations.ts +++ b/src/Operations.ts @@ -737,6 +737,11 @@ export class Operations { return changes } + /** + * Forces all formula vertices and column index entries to apply pending lazy + * transformations, bringing them up to the current LazilyTransformingAstService version. + * Called before undo of move operations and before compaction. + */ public forceApplyPostponedTransformations(): void { this.dependencyGraph.forceApplyPostponedTransformations() this.columnSearch.forceApplyPostponedTransformations() diff --git a/src/UndoRedo.ts b/src/UndoRedo.ts index cdd922c077..b7001b837e 100644 --- a/src/UndoRedo.ts +++ b/src/UndoRedo.ts @@ -35,6 +35,10 @@ export abstract class BaseUndoEntry implements UndoEntry { abstract doRedo(undoRedo: UndoRedo): void + /** + * Returns LazilyTransformingAstService version keys referenced by this entry's oldData. + * Default implementation returns empty — override in entries that store oldData. + */ public getReferencedOldDataVersions(): number[] { return [] } From bf615cc14e5597654dc6b0b458724223ac0f6d5e Mon Sep 17 00:00:00 2001 From: marcin-kordas-hoc Date: Wed, 25 Mar 2026 01:54:38 +0000 Subject: [PATCH 09/15] Address review: JSDoc, CHANGELOG Added entry, fix orphaned JSDoc --- src/HyperFormula.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/HyperFormula.ts b/src/HyperFormula.ts index 9194b7d32e..566957270a 100644 --- a/src/HyperFormula.ts +++ b/src/HyperFormula.ts @@ -4595,15 +4595,6 @@ export class HyperFormula implements TypedEmitter { this._functionRegistry = newEngine.functionRegistry } - /** - * Runs a recomputation starting from recently changed vertices. - * - * Returns [an array of cells whose values changed as a result of this operation](/guide/basic-operations.md#changes-array). - * - * Note that this method may trigger dependency graph recalculation. - * - * @fires [[valuesUpdated]] if recalculation was triggered by this change - */ /** * When enough transformations have accumulated, forces all formula vertices and * column index entries to apply pending lazy transformations, then compacts the @@ -4618,6 +4609,15 @@ export class HyperFormula implements TypedEmitter { } } + /** + * Runs a recomputation starting from recently changed vertices. + * + * Returns [an array of cells whose values changed as a result of this operation](/guide/basic-operations.md#changes-array). + * + * Note that this method may trigger dependency graph recalculation. + * + * @fires [[valuesUpdated]] if recalculation was triggered by this change + */ private recomputeIfDependencyGraphNeedsIt(): ExportedChange[] { if (!this._evaluationSuspended) { const changes = this._crudOperations.getAndClearContentChanges() From 28a2cb6b5550111147db1c48b090ddd6d65df7b6 Mon Sep 17 00:00:00 2001 From: marcin-kordas-hoc Date: Wed, 25 Mar 2026 02:04:21 +0000 Subject: [PATCH 10/15] fix: include batchUndoEntry in cleanupOrphanedOldData version collection --- src/UndoRedo.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/UndoRedo.ts b/src/UndoRedo.ts index b7001b837e..0a41f1f086 100644 --- a/src/UndoRedo.ts +++ b/src/UndoRedo.ts @@ -863,13 +863,15 @@ export class UndoRedo { /** * Removes oldData entries whose version keys are not referenced by any - * entry on the undo or redo stack. Called after compaction forces lazy - * formula evaluation, which may insert oldData for already-evicted entries. + * entry on the undo stack, redo stack, or in-progress batch. Called after + * compaction forces lazy formula evaluation, which may insert oldData for + * already-evicted entries. */ public cleanupOrphanedOldData() { const referencedVersions = new Set([ ...this.undoStack.flatMap(e => e.getReferencedOldDataVersions()), ...this.redoStack.flatMap(e => e.getReferencedOldDataVersions()), + ...(this.batchUndoEntry?.getReferencedOldDataVersions() ?? []), ]) for (const version of this.oldData.keys()) { if (!referencedVersions.has(version)) { From 958689be4854bfb3ee75f2507506a8b8119cdf42 Mon Sep 17 00:00:00 2001 From: marcin-kordas-hoc Date: Wed, 25 Mar 2026 02:16:09 +0000 Subject: [PATCH 11/15] fix: prevent cross-stack oldData deletion in cleanup methods --- src/UndoRedo.ts | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/UndoRedo.ts b/src/UndoRedo.ts index 0a41f1f086..f80107a0e6 100644 --- a/src/UndoRedo.ts +++ b/src/UndoRedo.ts @@ -539,12 +539,12 @@ export class UndoRedo { } public clearRedoStack() { - this.cleanupOldDataForEntries(this.redoStack) + this.cleanupOldDataForEntries(this.redoStack, this.undoStack) this.redoStack = [] } public clearUndoStack() { - this.cleanupOldDataForEntries(this.undoStack) + this.cleanupOldDataForEntries(this.undoStack, this.redoStack) this.undoStack = [] } @@ -857,7 +857,7 @@ export class UndoRedo { const evictCount = Math.max(0, this.undoStack.length - this.undoLimit) if (evictCount > 0) { const evicted = this.undoStack.splice(0, evictCount) - this.cleanupOldDataForEntries(evicted) + this.cleanupOldDataForEntries(evicted, [...this.undoStack, ...this.redoStack]) } } @@ -881,12 +881,20 @@ export class UndoRedo { } /** - * Removes oldData entries referenced by permanently discarded undo/redo entries. + * Removes oldData entries referenced by permanently discarded undo/redo entries, + * but only if not still referenced by entries remaining on the other stack or + * in-progress batch. */ - private cleanupOldDataForEntries(entries: UndoEntry[]) { - for (const entry of entries) { + private cleanupOldDataForEntries(discardedEntries: UndoEntry[], retainedEntries: UndoEntry[]) { + const retainedVersions = new Set([ + ...retainedEntries.flatMap(e => e.getReferencedOldDataVersions()), + ...(this.batchUndoEntry?.getReferencedOldDataVersions() ?? []), + ]) + for (const entry of discardedEntries) { for (const version of entry.getReferencedOldDataVersions()) { - this.oldData.delete(version) + if (!retainedVersions.has(version)) { + this.oldData.delete(version) + } } } } From 3381234ea271a99fc0b7bce6a260cdde66afbe19 Mon Sep 17 00:00:00 2001 From: marcin-kordas-hoc Date: Wed, 25 Mar 2026 02:19:30 +0000 Subject: [PATCH 12/15] refactor: extract collectReferencedOldDataVersions to eliminate cleanup duplication --- src/UndoRedo.ts | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/UndoRedo.ts b/src/UndoRedo.ts index f80107a0e6..c6b97e7b15 100644 --- a/src/UndoRedo.ts +++ b/src/UndoRedo.ts @@ -868,11 +868,7 @@ export class UndoRedo { * already-evicted entries. */ public cleanupOrphanedOldData() { - const referencedVersions = new Set([ - ...this.undoStack.flatMap(e => e.getReferencedOldDataVersions()), - ...this.redoStack.flatMap(e => e.getReferencedOldDataVersions()), - ...(this.batchUndoEntry?.getReferencedOldDataVersions() ?? []), - ]) + const referencedVersions = this.collectReferencedOldDataVersions(this.undoStack, this.redoStack) for (const version of this.oldData.keys()) { if (!referencedVersions.has(version)) { this.oldData.delete(version) @@ -886,10 +882,7 @@ export class UndoRedo { * in-progress batch. */ private cleanupOldDataForEntries(discardedEntries: UndoEntry[], retainedEntries: UndoEntry[]) { - const retainedVersions = new Set([ - ...retainedEntries.flatMap(e => e.getReferencedOldDataVersions()), - ...(this.batchUndoEntry?.getReferencedOldDataVersions() ?? []), - ]) + const retainedVersions = this.collectReferencedOldDataVersions(retainedEntries) for (const entry of discardedEntries) { for (const version of entry.getReferencedOldDataVersions()) { if (!retainedVersions.has(version)) { @@ -899,6 +892,14 @@ export class UndoRedo { } } + /** Collects all oldData version keys referenced by the given entry lists and in-progress batch. */ + private collectReferencedOldDataVersions(...entryLists: UndoEntry[][]): Set { + return new Set([ + ...entryLists.flatMap(list => list.flatMap(e => e.getReferencedOldDataVersions())), + ...(this.batchUndoEntry?.getReferencedOldDataVersions() ?? []), + ]) + } + private undoEntry(operation: UndoEntry) { operation.doUndo(this) } From fa8d1ec7190b5cc5e4e8e0aace30eebf3fee2b25 Mon Sep 17 00:00:00 2001 From: marcin-kordas-hoc Date: Wed, 25 Mar 2026 02:42:56 +0000 Subject: [PATCH 13/15] docs: add JSDoc to storeDataForVersion, clearRedoStack, clearUndoStack --- src/UndoRedo.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/UndoRedo.ts b/src/UndoRedo.ts index c6b97e7b15..f915c3d0d8 100644 --- a/src/UndoRedo.ts +++ b/src/UndoRedo.ts @@ -527,6 +527,10 @@ export class UndoRedo { this.batchUndoEntry = undefined } + /** + * Stores a formula AST hash snapshot for the given LazilyTransformingAstService version. + * Skipped when `undoLimit` is 0 (undo disabled) to avoid storing data that would never be used. + */ public storeDataForVersion(version: number, address: SimpleCellAddress, astHash: string) { if (this.undoLimit === 0) { return @@ -538,11 +542,13 @@ export class UndoRedo { currentOldData.push([address, astHash]) } + /** Clears the redo stack and removes oldData entries no longer referenced by any remaining entry. */ public clearRedoStack() { this.cleanupOldDataForEntries(this.redoStack, this.undoStack) this.redoStack = [] } + /** Clears the undo stack and removes oldData entries no longer referenced by any remaining entry. */ public clearUndoStack() { this.cleanupOldDataForEntries(this.undoStack, this.redoStack) this.undoStack = [] From 0933a14d75f892c305540f77b2b59af7a76d08a8 Mon Sep 17 00:00:00 2001 From: marcin-kordas-hoc Date: Wed, 25 Mar 2026 15:28:12 +0000 Subject: [PATCH 14/15] docs: clarify oldData Memory Management JSDoc structure --- src/UndoRedo.ts | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/UndoRedo.ts b/src/UndoRedo.ts index f915c3d0d8..bbe666aec9 100644 --- a/src/UndoRedo.ts +++ b/src/UndoRedo.ts @@ -483,15 +483,19 @@ export class BatchUndoEntry extends BaseUndoEntry { * * ### Memory Management * - * Without cleanup, `oldData` grows indefinitely because: - * 1. When undo entries are evicted (due to `undoLimit`), their referenced oldData - * keys are deleted via `cleanupOldDataForEntries()`. - * 2. When compaction forces lazy formula evaluation, it may write new oldData entries - * for versions that belong to already-evicted undo entries. These orphaned entries - * are cleaned up by `cleanupOrphanedOldData()`, which retains only versions - * still referenced by entries on the undo or redo stack. - * 3. When `undoLimit` is 0 (undo disabled), `storeDataForVersion()` short-circuits - * to avoid storing data that would never be used. + * Without cleanup, `oldData` grows indefinitely as undo entries are evicted but + * their oldData keys remain. Three mechanisms prevent this: + * + * 1. **Eviction cleanup**: When undo entries are evicted (due to `undoLimit`), + * `cleanupOldDataForEntries()` deletes their referenced oldData keys + * (unless still needed by entries on the other stack). + * 2. **Orphan cleanup**: Compaction may force lazy formula evaluation, which + * writes new oldData entries for already-evicted undo entries. After compaction, + * `cleanupOrphanedOldData()` removes any keys not referenced by entries on + * either stack or the in-progress batch. + * 3. **Short-circuit**: When `undoLimit` is 0 (undo disabled), + * `storeDataForVersion()` returns immediately to avoid storing data that + * would never be used. */ export class UndoRedo { public oldData: Map = new Map() From 8e0ebec601da0649750fa14c05adbc0201cb23f5 Mon Sep 17 00:00:00 2001 From: marcin-kordas-hoc Date: Wed, 25 Mar 2026 15:58:36 +0000 Subject: [PATCH 15/15] fix: consistent version-zero guards in getReferencedOldDataVersions --- src/UndoRedo.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/UndoRedo.ts b/src/UndoRedo.ts index bbe666aec9..87ab06439a 100644 --- a/src/UndoRedo.ts +++ b/src/UndoRedo.ts @@ -61,7 +61,7 @@ export class RemoveRowsUndoEntry extends BaseUndoEntry { } public getReferencedOldDataVersions(): number[] { - return this.rowsRemovals.map(r => r.version - 1) + return this.rowsRemovals.filter(r => r.version > 0).map(r => r.version - 1) } } @@ -252,7 +252,7 @@ export class RemoveColumnsUndoEntry extends BaseUndoEntry { } public getReferencedOldDataVersions(): number[] { - return this.columnsRemovals.map(r => r.version - 1) + return this.columnsRemovals.filter(r => r.version > 0).map(r => r.version - 1) } } @@ -322,7 +322,7 @@ export class RenameSheetUndoEntry extends BaseUndoEntry { } public getReferencedOldDataVersions(): number[] { - return this.version !== undefined ? [this.version - 1] : [] + return (this.version !== undefined && this.version > 0) ? [this.version - 1] : [] } }