Skip to content

This closes #2355, improve chart and style XML element compatibility to prevent generate corrupted workbook - #2343

Merged
xuri merged 3 commits into
qax-os:masterfrom
1436761858:saveas-stream-temp-files
Aug 1, 2026
Merged

This closes #2355, improve chart and style XML element compatibility to prevent generate corrupted workbook#2343
xuri merged 3 commits into
qax-os:masterfrom
1436761858:saveas-stream-temp-files

Conversation

@1436761858

Copy link
Copy Markdown
Contributor

Related Issue

#2342

Description

This change updates the save path for temp-file-backed workbook parts.

Currently, when writeToZip writes entries from f.tempFiles, it calls f.readBytes(path). If the entry is backed by a temp file, readBytes opens the temp file, reads the entire content into memory with io.ReadAll, stores it back into f.Pkg, and then writes the resulting byte slice to the ZIP writer.

This change avoids materializing the whole temp file in heap. Instead, writeToZip opens the temp file with f.readTemp(path) and streams it directly to the ZIP writer with io.Copy.

The saved workbook content is intended to be functionally equivalent:

Before:
temp file -> []byte in memory -> ZIP writer

After:
temp file -> ZIP writer

The same temp-file-backed entry is written to the same ZIP path with the same bytes. The change only affects how the bytes are transported during save, not the generated workbook content or workbook structure.
ZIP64 handling is preserved by using the byte count returned by io.Copy, which is equivalent to the previous Write byte count for this purpose.

A new test, TestWriteToZipStreamsTempFiles, was added to cover the temp-file-backed save path.
The test verifies that:
A temp-file-backed entry is still written into the generated ZIP output.
The written ZIP entry content matches the original temp file content.
The temp-file-backed content is not loaded back into f.Pkg during save.
The third assertion is important because readBytes(path) stores temp file content into f.Pkg after reading it. Verifying that f.Pkg does not contain the entry after save helps ensure this path remains streaming and does not regress back to full in-memory loading.

@xuri xuri added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 24, 2026
@xuri

xuri commented Jun 30, 2026

Copy link
Copy Markdown
Member

Thanks for your PR. Any benchmark data on the performance impact of the changes? Specifically, how much memory is saved, and any speed impact? I suggest test with excelize-benchmark for performance related changes.

@xuri

xuri commented Jun 30, 2026

Copy link
Copy Markdown
Member

The following benchmark report compares memory usage before and after the changes in this PR. The benchmarks were conducted by generating a workbook generated by xuri/excelize-benchmark containing 512K cells.

Environment:
Windows 11 Enterprise, 13th Gen Intel® Core™ i9-13900 (2.00 GHz), 64GB DDR5 RAM, 500GB SSD, Go version go1.25.5 windows/amd64

Workbook generated using the following command:

./v2.11.0-master -func=StreamWriter -rows=102400 -cols=50 -n=6

Base version 09f0656

Func: StreamWriter_r102400xc50.xlsx  RSS = 182 MB  Alloc = 134 MB  TotalAlloc = 1470 MB  Sys = 189 MB  NumGC = 71  Cost = 3.1179301s

This PR: #2343

Func: StreamWriter_r102400xc50.xlsx  RSS = 180 MB  Alloc = 134 MB  TotalAlloc = 1470 MB  Sys = 181 MB  NumGC = 71  Cost = 3.0502737s

@xuri xuri added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 1, 2026
@xuri xuri added this to v2.11.1 Aug 1, 2026
@xuri xuri moved this to Improve the Compatibility in v2.11.1 Aug 1, 2026
@xuri xuri changed the title Stream temp files when saving workbook This closes #2355, improve chart and style XML element compatibility to prevent generate corrupted workbook Aug 1, 2026

@xuri xuri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

According the benchmark, there are no significant memory reduced, so I not suggest accept this changes. I've made some changes based on your branch to fix another issue instead of close this directly.

@codecov

codecov Bot commented Aug 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.62%. Comparing base (ae2113b) to head (c451032).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2343   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files          32       32           
  Lines       26979    26979           
=======================================
  Hits        26877    26877           
  Misses         53       53           
  Partials       49       49           
Flag Coverage Δ
unittests 99.62% <100.00%> (ø)

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

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@xuri
xuri merged commit ad2cb3e into qax-os:master Aug 1, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

Status: Improve the Compatibility

Development

Successfully merging this pull request may close these issues.

AddChart: <c:marker> and <c:invertIfNegative> serialized after <c:dLbls>, out of the CT_LineSer/CT_BarSer sequence (Excel repairs on open)

2 participants