Fix #1629: resolve memory leaks in LazilyTransformingAstService and UndoRedo#1638
Open
marcin-kordas-hoc wants to merge 15 commits intodevelopfrom
Open
Fix #1629: resolve memory leaks in LazilyTransformingAstService and UndoRedo#1638marcin-kordas-hoc wants to merge 15 commits intodevelopfrom
marcin-kordas-hoc wants to merge 15 commits intodevelopfrom
Conversation
Performance comparison of head (8e0ebec) vs base (266089d) |
|
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. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
sequba
requested changes
Mar 23, 2026
Contributor
sequba
left a comment
sequba
left a comment
There was a problem hiding this comment.
It seems that you came up with an elegant and efficient solution even though it was a complex issue. Good job! I need more dev-oriented in-code documentation so I and the future contributors understand the optimization mechanisms introduced with this PR.
Also, please add 2 changelog entries describing both fixes.
…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
marcin-kordas-hoc
force-pushed
the
fix/1629
branch
from
b7b4c7b to
fa8d1ec
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1638 +/- ##
===========================================
- Coverage 97.18% 97.18% -0.01%
===========================================
Files 172 172
Lines 14845 14912 +67
Branches 3264 3276 +12
===========================================
+ Hits 14427 14492 +65
- Misses 418 420 +2
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
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
2 participants
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 engine mutation paths (lazy transformation versioning/compaction, column index maintenance, and undo/redo bookkeeping), so regressions could affect formula updates or undo correctness under heavy structural edits.
Overview
Fixes two long-running memory leaks by adding threshold-based compaction to
LazilyTransformingAstService(newcompactionThresholdconfig,versionOffset, and compaction triggered during recomputation after forcing all formula vertices and column indexes to apply pending transformations).Refactors
UndoRedoto track and garbage-collectoldDataby having eachUndoEntryreport referenced LTAS versions, cleaning up on stack eviction/clear, skipping storage whenundoLimitis0, and pruning orphanedoldDataafter compaction; column-search strategies gainforceApplyPostponedTransformations()to support safe compaction.Written by Cursor Bugbot for commit 8e0ebec. This will update automatically on new commits. Configure here.