Skip to content

Conversation

jafin
Copy link
Contributor

@jafin jafin commented May 2, 2025

Benchmarking Project Setup:

Refactoring:

  • Refactored TempSheetBuffer to replace individual row and column variables with a CellPosition struct, simplifying cell position management and improving code readability. (ClosedXML.Report/Excel/TempSheetBuffer.cs, ClosedXML.Report/Excel/TempSheetBuffer.csL13-R135)

  • Updated SubtotalSummaryFunc to use modern C# features such as target-typed new() and pattern matching, and replaced verbose property definitions with expression-bodied members. (ClosedXML.Report/Excel/SubtotalSummaryFunc.cs, [1] [2] [3]

  • Track LastCellUsed for small perf gain.

Motivations

We have code that generates spreadsheets, some are taking an increasing amount of time and memory to create. A lot of the performance is in the underlying ClosedXml, not so much reporting lib, but these changes add a slight drop in raw and even slighter performance boost in general.

Benchmark this PR

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
ReportGeneration 2.867 s 0.0552 s 0.0737 s 131000 45000 5000 2.04 GB
ReportGeneration 2.753 s 0.0534 s 0.0500 s 131000 45000 5000 2.04 GB

Benchmark Existing code

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
ReportGeneration 2.940 s 0.0545 s 0.0968 s 134000 45000 5000 2.08 GB
ReportGeneration 2.902 s 0.0570 s 0.0679 s 134000 44000 5000 2.08 GB

Significant amount of ram allocation still! ⚠️

Flamegraph shows a lot of time is spent in the underlying ClosedXML lib, one hot path for us is in conditional formatting application. I'll see if I can review that in the ClosedXML project.

image

Test changes

I amended one expected test, it was failing I Think the change is justified.
Here is the existing test expectation

image

Here is the amended: note the formatting on the Item 6 cell. All the other cells follow this pattern, so I think it made sense that this cell also should be rendered the same.
image

I actually think all the cells should have the border formatting, but that is for another day.

jafin added 3 commits May 2, 2025 11:38
…lloc drop. Refactor row/column into CellPosition

Update expected style in tlist7_horizontal_images Guage test
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.

1 participant