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.

Sign up for GitHub

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

Jump to bottom

Reduce memory consumption #1713

Merged
merged 3 commits into from
Nov 7, 2023
Merged

Reduce memory consumption #1713

merged 3 commits into from
Nov 7, 2023

Conversation

krstak
Copy link
Contributor

@krstak krstak commented Nov 6, 2023

PR Details

Reduce memory consumption

Description

By having a lot of rows and columns, memory consumption increases a lot. The reason is that we always allocate a new underlying array instead of re-using the existing one.

Related Issue

#1712

Motivation and Context

By having a huge amount of rows, the library consumes a lot of memory for converting it to the xlsx file. After this change, a lot of memory would be saved.

How Has This Been Tested

I tried to convert a huge amount of data (60000 rows) to the xlsx

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@xuri xuri added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 6, 2023
xuri
xuri requested changes Nov 6, 2023
View reviewed changes
sheet.go Show resolved Hide resolved
@xuri xuri added this to Performance in v2.8.1 Nov 6, 2023
@krstak krstak requested a review from xuri November 6, 2023 09:29
xuri
xuri requested changes Nov 6, 2023
View reviewed changes
sheet.go Outdated Show resolved Hide resolved
@krstak krstak requested a review from xuri November 6, 2023 09:37
xuri
xuri reviewed Nov 6, 2023
View reviewed changes
Copy link
Member

@xuri xuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your great work. Could you run a new pprof figure to show how much memory was reduced in the optimization?

@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2023 •
edited
Loading

Codecov Report

Merging #1713 (68bd59f) into master (fe639fa) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##           master    #1713   +/-   ##
=======================================
  Coverage   99.03%   99.03%           
=======================================
  Files          32       32           
  Lines       28100    28103    +3     
=======================================
+ Hits        27830    27833    +3     
  Misses        179      179           
  Partials       91       91
Flag Coverage Δ
unittests 99.03% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
sheet.go 100.00% <100.00%> (ø)

@xuri xuri linked an issue Nov 6, 2023 that may be closed by this pull request
Memory consumption by trimming the rows and cells #1712
Closed
@krstak
Copy link
Contributor Author

krstak commented Nov 6, 2023

Thanks for your great work. Could you run a new pprof figure to show how much memory was reduced in the optimization?

You are welcome.

I've already done it and pprof didn't show the problem anymore. It didn't show anything related to that problem. Since there are other potential memory consumption data issues, pprof pointed me to the new place that uses ~192MB. That place is in the function prepareSheetXML where we use append ws.SheetData.Row = append(ws.SheetData.Row, ...)

Since I am not 100% familiar with the code and it seems I would need to re-read a whole codebase to safely change that part, I didn't want to touch it.

mem_problem

@krstak krstak requested a review from xuri November 6, 2023 11:29
xuri
xuri approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@xuri xuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for your contribution.

@xuri xuri merged commit 6cc1a54 into qax-os:master Nov 7, 2023
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xuri xuri xuri approved these changes

Assignees
No one assigned
Labels
size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
No open projects
v2.8.1
Performance
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Memory consumption by trimming the rows and cells
3 participants
@krstak @codecov-commenter @xuri