handsontable / hyperformula

HyperFormula is an open-source headless spreadsheet for business web apps. It comes with over 400 formulas, CRUD operations, undo-redo, clipboard support, and sorting.
https://hyperformula.handsontable.com/
Other
1.97k stars 108 forks source link

fix stackoverflow from spread operation #1321

Closed BrianHung closed 11 months ago

BrianHung commented 11 months ago

Context

Ran into a stack overflow error when trying to build a sheet with 65536 (2^16) rows.

How did you test your changes?

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

Types of changes

Related issues:

Checklist:

sequba commented 11 months ago

@BrianHung, thank you for your contribution. I'll look into it soon.

Did you test how much memory does this way save? How many rows are you able to add after this change?

BrianHung commented 11 months ago

thank you for your contribution. I'll look into it soon.

Thanks! Not sure if it's the most efficient way so change it if you find something better.

Did you test how much memory does this way save? How many rows are you able to add after this change?

I modified the hyperformula benchmark suite to run with 2^16 rows since I wanted to see how fast it would take to initialize a spreadsheet of that size. This was on a 32 gb macbook. Don't know how much memory it saved but it allowed me to run the benchmark without crashing into OOM.

sequba commented 11 months ago

@BrianHung could you share the code that produced the OOM error?

BrianHung commented 11 months ago

@sequba

I modified cruds-benchmark.ts to use 2^16 rows. Unsure if this OOM was system specific since I only tested it only my macbook with 32gb of ram.

  batch(result,
    () => benchmarkCruds('Sheet A:  change value, add/remove row/column', sheetAGenerator(66536), (engine: HyperFormula) => {
      engine.setCellContents(adr('A1'), '123')
      engine.addRows(0, [5000, 1])
      engine.removeRows(0, [8000, 1])
      engine.addColumns(0, [0, 1])
      engine.removeColumns(0, [0, 1])
    }, [
      {address: 'E7000', value: -1.17344394901827e+23},
    ], {
      engineConfig: {
        maxRows: 131072
      }
    }),

    () => benchmarkCruds('Sheet B: change value, add/remove row/column', sheetBGenerator(66536), (engine: HyperFormula) => {
      engine.setCellContents(adr('A1'), '123')
      engine.addRows(0, [2000, 1])
      engine.removeRows(0, [3000, 1])
      engine.addColumns(0, [0, 1])
      engine.removeColumns(0, [0, 1])
    }, [
      {address: 'E50', value: 1347},
      {address: 'E2002', value: 2001122},
    ], {
      engineConfig: {
        maxRows: 131072
      }
    }),
sequba commented 11 months ago

Thank you @BrianHung. I'm merging this PR

codecov[bot] commented 11 months ago

Codecov Report

Merging #1321 (64f540e) into feature/issue-1332 (a008c1d) will decrease coverage by 0.03%. The diff coverage is 100.00%.

:exclamation: Current head 64f540e differs from pull request most recent head 60533c9. Consider uploading reports for the commit 60533c9 to get more accurate results

Impacted file tree graph

@@                  Coverage Diff                   @@
##           feature/issue-1332    #1321      +/-   ##
======================================================
- Coverage               97.27%   97.25%   -0.03%     
======================================================
  Files                     169      169              
  Lines                   14422    14409      -13     
  Branches                 3018     3092      +74     
======================================================
- Hits                    14029    14013      -16     
+ Misses                    393      391       -2     
- Partials                    0        5       +5     
Files Coverage Δ
src/DependencyGraph/DependencyGraph.ts 97.94% <100.00%> (+<0.01%) :arrow_up:

... and 9 files with indirect coverage changes

AMBudnik commented 9 months ago

Hi @BrianHung

We just closed the linked issue https://github.com/handsontable/hyperformula/issues/1332 and released it under HyperFormula v2.6.1