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

optimize memory allocation #722

Merged
merged 7 commits into from
Nov 3, 2020
Merged

optimize memory allocation #722

merged 7 commits into from
Nov 3, 2020

Conversation

Theodoree
Copy link
Contributor

PR Details

1.align xlsxRow struct.
2.optimization workSheetWriter
3. add NewSheetWithRowNum

Description

Mainly to optimize memory allocation

Related Issue

l wish somebody can optimize xlsxC struct. it's too bigger.Maybe it can be optimized smaller.

Motivation and Context

sometime l use this package. memory will alloc 500m ~ 1000m. it's too expensive

How Has This Been Tested


go test -run=none -bench=BenchmarkNewSheetWithRowNum -benchmem -count 10

name                   old time/op    new time/op    delta
NewSheetWithRowNum1-8     667µs ± 5%     679µs ±15%     ~     (p=0.968 n=9+10)

name                   old alloc/op   new alloc/op   delta
NewSheetWithRowNum1-8     643kB ± 0%     588kB ± 0%   -8.46%  (p=0.000 n=10+10)

name                   old allocs/op  new allocs/op  delta
NewSheetWithRowNum1-8     10.3k ± 0%      9.2k ± 0%  -10.68%  (p=0.000 n=10+10)


go test -run=none -bench=BenchmarkFile_SaveAs -benchmem -count 10
name           old time/op    new time/op    delta
File_SaveAs-8     738µs ±13%     698µs ±10%     ~     (p=0.105 n=10+10)

name           old alloc/op   new alloc/op   delta
File_SaveAs-8     643kB ± 0%     588kB ± 0%   -8.46%  (p=0.000 n=10+10)

name           old allocs/op  new allocs/op  delta
File_SaveAs-8     10.3k ± 0%      9.2k ± 0%  -10.67%  (p=0.000 n=9+10)



Types of changes

  • [ x ] New feature (non-breaking change which adds functionality)
  • [ x ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

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

@codecov-io
Copy link

codecov-io commented Oct 29, 2020

Codecov Report

Merging #722 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #722   +/-   ##
=======================================
  Coverage   96.65%   96.65%           
=======================================
  Files          31       31           
  Lines        8553     8557    +4     
=======================================
+ Hits         8267     8271    +4     
  Misses        163      163           
  Partials      123      123           
Impacted Files Coverage Δ
xmlWorksheet.go 100.00% <ø> (ø)
lib.go 100.00% <100.00%> (ø)
sheet.go 97.28% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d470bb...a2b0cae. Read the comment docs.

@xuri xuri added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 29, 2020
xmlWorksheet.go Show resolved Hide resolved
sheet.go Outdated
@@ -64,6 +64,19 @@ func (f *File) NewSheet(name string) int {
return f.GetSheetIndex(name)
}

// NewSheetWithRowNum provides the function to create a new sheet by given a worksheet and a given number of rows.
Copy link
Member

Choose a reason for hiding this comment

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

This new API seems unnecessary, SetCellValue will preallocate cells for any new worksheets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometime will need this. Realloc cause huge GC pressure.
Example:
I need export a million row form db and write into excel, data is encrypt , need decrypt .
slice often realloc we cause huge GC pressure 、copy spending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have encountered this situation before.
l remember alloc 1-3G mem. it's too Impressive.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for my late reply. We can using streaming API NewStreamWriter to generate huge data spreadsheet, by evaluation and follow the principle of least availability, I don't purpose to add this API. If you have any questions, let me know.

@Theodoree
Copy link
Contributor Author

@xuri

@Theodoree
Copy link
Contributor Author

Theodoree commented Nov 3, 2020

@xuri use NewStreamWriter will take more Resources.

go test -run=none -bench=BenchmarkNewSheet -benchmem -count 10

name time/op
NewSheetWithRowNum-8 631µs ±12%
NewSheetWithOutRowNum-8 636µs ± 2%
NewSheetWithStreamWriter-8 1.07ms ± 3%

name alloc/op
NewSheetWithRowNum-8 508kB ± 0%
NewSheetWithOutRowNum-8 616kB ± 0%
NewSheetWithStreamWriter-8 880kB ± 4%

name allocs/op
NewSheetWithRowNum-8 9.18k ± 0%
NewSheetWithOutRowNum-8 9.20k ± 0%
NewSheetWithStreamWriter-8 12.6k ± 0%

@Theodoree
Copy link
Contributor Author

go test -run=none -bench=BenchmarkNewSheet -benchmem -count 10

name time/op
NewSheetWithRowNum-8 631µs ±12%
NewSheetWithOutRowNum-8 636µs ± 2%
NewSheetWithStreamWriter-8 1.07ms ± 3%

name alloc/op
NewSheetWithRowNum-8 508kB ± 0%
NewSheetWithOutRowNum-8 616kB ± 0%
NewSheetWithStreamWriter-8 880kB ± 4%

name allocs/op
NewSheetWithRowNum-8 9.18k ± 0%
NewSheetWithOutRowNum-8 9.20k ± 0%
NewSheetWithStreamWriter-8 12.6k ± 0%

@Theodoree Theodoree closed this Nov 3, 2020
@Theodoree Theodoree reopened this Nov 3, 2020
@Theodoree
Copy link
Contributor Author

l will Think about it, maybe I'll delete this api. NewSheetWithRowNum

sheet_test.go Outdated Show resolved Hide resolved
@xuri
Copy link
Member

xuri commented Nov 3, 2020

FYI, here is a graph that shows the performance comparison of normal API and streaming writing API. I'll public testing code later.

@Theodoree
Copy link
Contributor Author

Theodoree commented Nov 3, 2020

@xuri I have deleted that api. l was testing. l find NewStreamWriter will save more mem.but Execution time will be longer.

@xuri
Copy link
Member

xuri commented Nov 3, 2020

Usually, the streaming API will use less memory and faster than normal API, but affected by business scenario, you need trade-off by yourself. Try profiling your program by pprof to get more details.

@Theodoree
Copy link
Contributor Author

@xuri so.when can merge PR ?

@xuri xuri merged commit fcca8a3 into qax-os:master Nov 3, 2020
@xuri
Copy link
Member

xuri commented Nov 3, 2020

/lgtm Thanks @Theodoree

EugeneAndrosovPaser pushed a commit to ceearrashee/excelize that referenced this pull request Nov 14, 2020
* optimize marshal

* optimize mem alloc

* add benchmark testing

* add NewSheetWithRowNum testing

* sync struct fields order

* add BenchmarkNewSheetWithStreamWriter

* delete NewSheetWithRowNum and benchmark test
jenbonzhang pushed a commit to jenbonzhang/excelize that referenced this pull request Oct 22, 2023
* optimize marshal

* optimize mem alloc

* add benchmark testing

* add NewSheetWithRowNum testing

* sync struct fields order

* add BenchmarkNewSheetWithStreamWriter

* delete NewSheetWithRowNum and benchmark test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants