Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor DependencyGraph.getArrayVerticesRelatedToRanges method to be more memory-effective #1333

Merged
merged 5 commits into from
Dec 15, 2023

Conversation

sequba
Copy link
Contributor

@sequba sequba commented Nov 8, 2023

Context

Originally fixed by @BrianHung in #1321:

By incrementing adding vertices to a set instead of creating arrays and then flattening them at the end, we save on memory.

How did you test your changes?

Types of changes

  • Breaking change (a fix or a feature because of which an existing functionality doesn't work as expected anymore)
  • New feature or improvement (a non-breaking change that adds functionality)
  • Bug fix (a non-breaking change that fixes an issue)
  • Additional language file, or a change to an existing language file (translations)
  • Change to the documentation

Related issues:

  1. Fixes [Bug]: Maximum call stack size exceeded when adding a row to a huge spreadsheet with formulas #1332

Checklist:

  • I have reviewed the guidelines about Contributing to HyperFormula and I confirm that my code follows the code style of this project.
  • I have signed the Contributor License Agreement.
  • My change is compliant with the OpenDocument standard.
  • My change is compatible with Microsoft Excel.
  • My change is compatible with Google Sheets.
  • I described my changes in the CHANGELOG.md file.
  • My changes require a documentation update.
  • My changes require a migration guide.

BrianHung and others added 2 commits November 8, 2023 14:10
* fix stackoverflow from spread operation

---------

Co-authored-by: Kuba Sekowski <jakub.sekowski@handsontable.com>
@sequba sequba requested a review from evanSe November 8, 2023 13:35
Copy link

github-actions bot commented Nov 8, 2023

Performance comparison of head (5ebfffa) vs base (4230e82)

                                     testName |   base |   head |  change
-------------------------------------------------------------------------
                                      Sheet A | 546.77 | 547.38 |  +0.11%
                                      Sheet B | 180.56 | 184.53 |  +2.20%
                                      Sheet T | 157.09 | 161.02 |  +2.50%
                                Column ranges | 558.19 | 562.51 |  +0.77%
Sheet A:  change value, add/remove row/column |     44 |     42 |  -4.55%
 Sheet B: change value, add/remove row/column |    299 |    366 | +22.41%
                   Column ranges - add column |    167 |    157 |  -5.99%
                Column ranges - without batch |    519 |    528 |  +1.73%
                        Column ranges - batch |     98 |     92 |  -6.12%

Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Merging #1333 (a84b9c2) into develop (4230e82) will increase coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head a84b9c2 differs from pull request most recent head 5ebfffa. Consider uploading reports for the commit 5ebfffa to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1333      +/-   ##
===========================================
+ Coverage    97.28%   97.34%   +0.06%     
===========================================
  Files          169      169              
  Lines        14425    14413      -12     
  Branches      3081     3016      -65     
===========================================
- Hits         14033    14031       -2     
+ Misses         387      382       -5     
+ Partials         5        0       -5     
Files Coverage Δ
src/DependencyGraph/DependencyGraph.ts 99.55% <100.00%> (+1.46%) ⬆️

... and 4 files with indirect coverage changes

@sequba sequba merged commit c74bb21 into develop Dec 15, 2023
21 checks passed
@sequba sequba deleted the feature/issue-1332 branch December 15, 2023 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Maximum call stack size exceeded when adding a row to a huge spreadsheet with formulas
3 participants