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

Add benchmark for adding images to sheet #367

Merged
merged 2 commits into from
Mar 23, 2019
Merged

Add benchmark for adding images to sheet #367

merged 2 commits into from
Mar 23, 2019

Conversation

mlh758
Copy link
Contributor

@mlh758 mlh758 commented Mar 22, 2019

PR Details

Benchmark image adding

Description

This should help track performance regressions in future changes. I didn't want to start code to
reduce image duplication until I had a performance baseline.

Current output looks like this:
BenchmarkAddPictureFromBytes-8 10000 2205294 ns/op 373361 B/op 20358 allocs/op

Related Issue

#359

Motivation and Context

This is a first step towards future changes to this functionality.

How Has This Been Tested

This is a test :)

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.

This should help track performance regressions in future changes.
@mlh758
Copy link
Contributor Author

mlh758 commented Mar 22, 2019

I thought the allocation count was kind of high, it looks like almost all the memory consumption is in trimSheetName

(pprof) top
Showing nodes accounting for 3562.68MB, 97.65% of 3648.40MB total
Dropped 222 nodes (cum <= 18.24MB)
Showing top 10 nodes out of 31
      flat  flat%   sum%        cum   cum%
 3419.08MB 93.71% 93.71%  3419.08MB 93.71%  github.com/360EntSecGroup-Skylar/excelize.trimSheetName
   42.63MB  1.17% 94.88%    42.63MB  1.17%  image/png.(*decoder).parsePLTE
   42.31MB  1.16% 96.04%    55.76MB  1.53%  compress/flate.NewWriter
   41.15MB  1.13% 97.17%    41.15MB  1.13%  bufio.NewReaderSize
    7.51MB  0.21% 97.38%    52.14MB  1.43%  image/png.DecodeConfig

That's probably a good target for some tinkering.

@codecov-io
Copy link

codecov-io commented Mar 22, 2019

Codecov Report

Merging #367 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #367   +/-   ##
=======================================
  Coverage   97.91%   97.91%           
=======================================
  Files          21       21           
  Lines        4074     4074           
=======================================
  Hits         3989     3989           
  Misses         48       48           
  Partials       37       37

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 677a22d...b7164bd. Read the comment docs.

@mlh758
Copy link
Contributor Author

mlh758 commented Mar 22, 2019

I just added another commit to this PR. trimSheetName now checks if it has any work to do before jumping in. It also pre-allocates the rune slice's capacity and breaks at a rune count of 31 instead of checking the rune count afterwards and trimming it back down. Here are the new benchmarks and profiles on my machine:

BenchmarkAddPictureFromBytes-8 10000 1170422 ns/op 12141 B/op 296 allocs/op

(pprof) top
Showing nodes accounting for 206.98MB, 79.95% of 258.88MB total
Dropped 112 nodes (cum <= 1.29MB)
Showing top 10 nodes out of 157
      flat  flat%   sum%        cum   cum%
   51.12MB 19.75% 19.75%    59.77MB 23.09%  compress/flate.NewWriter
   43.13MB 16.66% 36.41%    43.13MB 16.66%  image/png.(*decoder).parsePLTE
   41.16MB 15.90% 52.31%    41.16MB 15.90%  bufio.NewReaderSize
   22.92MB  8.86% 61.16%    22.92MB  8.86%  bytes.makeSlice
   11.04MB  4.27% 65.43%    11.04MB  4.27%  bytes.Replace

I tested this by adding a sheet with some ?:/ characters in the name and that was too long and made sure I still got a valid document.

@xuri xuri merged commit 2874d75 into qax-os:master Mar 23, 2019
@xuri xuri added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 6, 2020
nullfy pushed a commit to nullfy/excelize that referenced this pull request Oct 23, 2020
* Add benchmark for adding images to sheet

This should help track performance regressions in future changes.

* Only transform sheet name if necessary
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