Stream WriteTo directly to writer and fix ZIP64 local file header sizes (B4)#2324
Open
AdamDrewsTR wants to merge 8 commits into
Open
Stream WriteTo directly to writer and fix ZIP64 local file header sizes (B4)#2324AdamDrewsTR wants to merge 8 commits into
AdamDrewsTR wants to merge 8 commits into
Conversation
…able thresholds - Add bufio.Writer layer after temp file threshold is crossed, reducing disk write syscalls from per-Write to per-buffer-full (default 128 KiB) - Add scratch [24]byte field for strconv.Append* to avoid heap allocations in WriteInt, WriteUint, and WriteFloat helper methods - Track total bytes written for offset-based operations (WriteAt) - Add WriteAt method for updating data at specific offsets in both in-memory and temp file modes - Add CopyTo method with large read buffer to minimize Pread syscalls when copying data to ZIP writers - Add Bytes method for direct access to in-memory buffer contents - Add Reset method for clearing state without closing the temp file - Add StreamingChunkSize option to control when streaming spills to disk (0 = default 16 MiB, -1 = never spill, keeps all data in memory) - Add StreamingBufSize option to control bufio.Writer size aft- Add StreamingBufSize option to control bufio.Writer size aft- Add StreamingBufSize option to control bufio.Writer size aft- Add Streaminak memory usage during streaming writes
WriteTo streaming: - Stream ZIP archive directly to the destination writer instead of buffering the entire compressed output in a bytes.Buffer. For large workbooks this avoids 50-200 MB+ of peak memory allocation. - For encrypted files, use a temporary file instead of an in-memory buffer to reduce peak memory during the encrypt-then-write cycle. - Add countWriter to track bytes written for the int64 return value. - Use CopyTo instead of Reader+io.Copy in writeToZip for streaming worksheets, reducing syscalls via larger read buffers. ZIP64 local file header fix: - Set compressed and uncompressed size fields in the Local File Header to 0xFFFFFFFF for ZIP64 entries. The Go standard library only patches the Central Directory version to 45 for entries >4GB but leaves the LFH sizes at their original values. Excel strictly validates that ZIP64 LFH entries have sizes set to 0xFFFFFFFF with the actual sizes in the ZIP64 extended information extra field. Without this fix, workbooks containing sheets larger than 4GB a larger than 4GB a larger than 4GB a larger than 4GB a larger than 4GB s is non-empty, avoiding unnecessary scanning of the output buffer for most unnecessary scanning of the output buffbased fixup (used by encryption path).
Cover WriteInt, WriteUint, WriteFloat, Bytes, WriteAt, CopyTo, and Reset methods in both in-memory and temp-file (bio) code paths. Test NewStreamWriter with custom StreamingChunkSize (-1, positive) and StreamingBufSize options.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2324 +/- ##
==========================================
+ Coverage 99.60% 99.61% +0.01%
==========================================
Files 32 32
Lines 26791 26977 +186
==========================================
+ Hits 26685 26873 +188
+ Misses 55 54 -1
+ Partials 51 50 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
This will need to be rebased after #2321 is merged. |
Add tests for IO error scenarios when the underlying temp file is closed or broken: - TestBufferedWriterWriteAtFlushError: Flush fails in WriteAt - TestBufferedWriterCopyToFlushError: Flush fails in CopyTo - TestBufferedWriterCopyToSeekError: Seek fails in CopyTo - TestBufferedWriterSyncWriteToError: WriteTo fails in Sync This brings diff coverage from 95.56% to 100%.
Add Compression type with three levels: - CompressionDefault: standard deflate (existing behavior) - CompressionNone: store without compression (fastest, for memory- constrained environments like AWS Lambda) - CompressionBestSpeed: fastest deflate level (good middle ground) The configureZipCompression method applies the setting via RegisterCompressor on the stdlib *zip.Writer. It is a no-op for custom ZipWriter implementations or the default compression level.
Add benchmarks comparing Default, None, and BestSpeed compression: - BenchmarkCompressionLevels: 10000x50 sheet, reports output size - BenchmarkCompressionBySize: multiple sheet sizes across all levels
- Replace Seek+ReadAll with os.ReadFile in writeToWithEncryption - Extract fixZip64LFH with io.ReaderAt/io.WriterAt interfaces for testability - Use ReadAt-until-EOF loop instead of Stat+fileSize loop - Combine three WriteAt calls into single write for LFH patching - Add comprehensive tests for all error paths: - TestWriteToWithEncryptionZip64 (happy path with zip64 trigger) - TestWriteToWithEncryptionZip64Error (fd closed, writeZip64LFHFile fails) - TestWriteToWithEncryptionReadFileError (file removed, os.ReadFile fails) - TestFixZip64LFHReadAtError (mock ReadAt error) - TestFixZip64LFHWriteAtError (mock WriteAt error) - TestFixZip64LFHLargeFile (>1MB data, offset overlap)
…ct coverage - TestWriteToBufferErrors: covers writeToZip and zw.Close error paths - TestWriteToBufferWithPassword: covers WriteToBuffer encryption path - TestWriteCellInlineStringWithSpace: covers IS.T.Space.Value branch - TestBufferedWriterReaderFlushError: covers Reader() flush error path - TestBufferedWriterSyncCreateTempError: covers Sync() CreateTemp failure - TestGetRowValuesEOF: covers clean EOF path in getRowValues Project coverage now 99.75% (53 uncovered) vs base 99.74% (54 uncovered).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related changes: (1) stream
WriteTodirectly instead of buffering, and (2) fix ZIP64 Local File Header size fields for entries >4GB.Depends on: #2321 (buffered-writer enhancements — for
CopyTo)Bug Fix: ZIP64 LFH compressed/uncompressed sizes
Problem
Go's
archive/zipwrites the Central Directory entry withReaderVersion=45for entries >4GB, but the Local File Header retainsversion=20and the original (truncated) 32-bit size values. Excel strictly validates that:writeZip64LFH0xFFFFFFFFwhen ZIP64 extended info is present ❌ not fixed until nowWithout setting sizes to
0xFFFFFFFF, Excel sees a mismatch between the LFH size fields and the ZIP64 extra field, and reports the workbook as corrupt.Fix
After patching the version to 45, also set the compressed size (offset +18) and uncompressed size (offset +22) to
0xFFFFFFFFin the LFH for all ZIP64 entries. This tells readers to use the actual sizes from the ZIP64 extended information extra field that follows the filename in the LFH.Performance: Streaming WriteTo
Problem
The current
WriteTocallsWriteToBuffer(), which materializes the entire compressed ZIP archive in abytes.Buffer. For large workbooks (e.g., a 4GB+ sheet compresses to 50-200 MB), this means 50-200 MB of peak memory just for the output buffer, on top of the input data.Solution
WriteTonow streams the ZIP directly to the destinationio.Writerusing acountWriterwrapper (to trackint64bytes written). No intermediatebytes.Bufferis needed.For encrypted files (which require post-processing the entire ZIP), a temporary file is used instead of an in-memory buffer, accessed via the new
writeToWithEncryptionhelper.Additional changes
writeToZipusesCopyToinstead ofReader+io.Copyfor streaming worksheets, leveraging the larger read buffers from Enhance bufferedWriter with bufio.Writer, scratch space, and configurable thresholds (B1) #2321writeZip64LFHis only called whenzip64Entriesis non-empty (avoids scanning the entire output for files with no >4GB entries)writeZip64LFHFilefor file-based ZIP64 fixup (used by the encryption path)Compatibility
All existing tests pass including
TestZip64.