Fix #1629: resolve memory leaks in LazilyTransformingAstService and UndoRedo#1638
Open
marcin-kordas-hoc wants to merge 6 commits intodevelopfrom
Open
Fix #1629: resolve memory leaks in LazilyTransformingAstService and UndoRedo#1638marcin-kordas-hoc wants to merge 6 commits intodevelopfrom
marcin-kordas-hoc wants to merge 6 commits intodevelopfrom
Conversation
…ndoRedo - 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
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.
…ition - 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)
…rmations 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.
- 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
Performance comparison of head (30dca8d) vs base (4042b04) |
|
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit in Settings → Usage. Once credits are available, push a new commit or reopen this pull request to trigger a review. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1638 +/- ##
===========================================
- Coverage 97.18% 97.14% -0.04%
===========================================
Files 172 172
Lines 14836 14895 +59
Branches 3258 3268 +10
===========================================
+ Hits 14418 14470 +52
- Misses 418 425 +7
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
|
|
||
| public getReferencedOldDataVersions(): number[] { | ||
| return this.rowsRemovals.map(r => r.version - 1) | ||
| } |
There was a problem hiding this comment.
Inconsistent version-zero guards across undo entry types
Low Severity
RemoveRowsUndoEntry and RemoveColumnsUndoEntry return version - 1 without guarding against version being zero, while MoveCellsUndoEntry, MoveRowsUndoEntry, and MoveColumnsUndoEntry all guard with this.version > 0. Similarly, RenameSheetUndoEntry only checks !== undefined but not > 0. If a version were ever zero, the unguarded entries would return [-1], which is harmless today but inconsistent with the guarded entries. The seven getReferencedOldDataVersions() implementations across entry types use three different guard styles for the same invariant.
Additional Locations (1)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1629. Closes #1633. Closes #1634.
Two unbounded memory leaks in long-running HyperFormula instances:
LTAS.transformations[]grew linearly with every structural operation (addRows, removeRows, moveCells, etc.) and was never cleaned up. Fixed by introducing threshold-based compaction withversionOffset— once 50+ transformations accumulate, all consumers (FormulaVertex, ColumnIndex) are force-updated, then the array is released while the logical version remains monotonically increasing.UndoRedo.oldDatagrew linearly even when entries were evicted from the undo stack. Fixed by tracking which LTAS versions eachUndoEntryreferences (getReferencedOldDataVersions()), cleaning up on eviction/clear, guarding against writes whenundoLimit === 0, and running orphan cleanup after compaction to handle a race condition where lazy-apply re-inserts already-evicted keys.Changed files
LazilyTransformingAstService.tsversionOffset,compact(),needsCompaction()with threshold=50, offset-aware iterationUndoRedo.tsgetReferencedOldDataVersions()on interface + 7 subclasses, eviction cleanup,undoLimit===0guard,cleanupOrphanedOldData(),forceApplyparity inundoMoveRows/undoMoveColumnsHyperFormula.tsrecomputeIfDependencyGraphNeedsIt()ColumnIndex.tsforceApplyPostponedTransformations()— iterates all ValueIndex entriesColumnBinarySearch.tsforceApplyPostponedTransformations()SearchStrategy.tsColumnSearchStrategyinterfaceOperations.tscolumnSearch.forceApplyto undo path (no compact — centralized in HyperFormula.ts)What is NOT fixed here
ParserWithCaching) — unbounded growth tracked separately in [Bug]:ParserWithCachingcache can grow without limit #1635Test plan
hyperformula-tests— see companion PR in that repoNote
Medium Risk
Touches core recalculation/undo internals and introduces forced application/compaction logic; regressions could surface as incorrect formula updates or undo behavior, though changes are targeted at memory cleanup.
Overview
Fixes long-running memory growth by adding threshold-based compaction to
LazilyTransformingAstService: versions become offset-aware (versionOffset), transformation iteration is updated, andcompact()drops accumulated transformations once they’ve been applied.Hooks compaction into
HyperFormula.recomputeIfDependencyGraphNeedsIt()by forcing postponed transformations on both the dependency graph and column search (ColumnIndexaddsforceApplyPostponedTransformations(),ColumnBinarySearchimplements a no-op) before compacting.Prevents
UndoRedo.oldDatafrom leaking by tracking referenced LTAS versions perUndoEntry, deleting oldData on stack eviction/clear, skipping oldData writes whenundoLimit === 0, and addingcleanupOrphanedOldData()invoked after compaction to remove unreferenced versions.Written by Cursor Bugbot for commit 30dca8d. This will update automatically on new commits. Configure here.