Skip to content

Perf: use Stream.CopyTo and zero-copy MemoryPackagePart reads#1743

Open
ken-swyfft wants to merge 2 commits intonissl-lab:masterfrom
swyfft-insurance:perf/ks/20260321_streamhelper-buffer-memorypart-copy
Open

Perf: use Stream.CopyTo and zero-copy MemoryPackagePart reads#1743
ken-swyfft wants to merge 2 commits intonissl-lab:masterfrom
swyfft-insurance:perf/ks/20260321_streamhelper-buffer-memorypart-copy

Conversation

@ken-swyfft
Copy link
Contributor

Summary

  • StreamHelper.CopyStream: Replaced manual 1KB buffer loop with Stream.CopyTo() which uses an 80KB internal buffer — available since .NET 4.0
  • MemoryPackagePart.GetInputStreamImpl: Eliminated full-stream copy by wrapping the existing MemoryStream buffer in a read-only view via GetBuffer() (zero-copy)

Benchmark results (test-performance.xlsx, 17MB, 5 sheets)

Benchmark Before After Change
Write 2,843 ms / 103,176 KB 2,752 ms / 73,263 KB ~3% faster / 29% less memory
Load (full) 14,386 ms 13,932 ms ~3% faster
Load (lazy) 1,738 ms 1,804 ms within noise

The Write benchmark shows the most significant improvement: 29% reduction in memory allocations (~30MB less per operation) from eliminating redundant MemoryStream copies during package writes.

Test plan

  • All 4,616 tests pass on net8.0 (main + ooxml + openxml4net)
  • All 4,619 tests pass on net472 (main + ooxml + openxml4net)
  • BenchmarkDotNet comparison against master baseline

🤖 Generated with Claude Code

…Part copy

Replace manual 1KB buffer loop in StreamHelper.CopyStream with Stream.CopyTo()
(80KB internal buffer). Eliminate redundant full-stream copy in
MemoryPackagePart.GetInputStreamImpl by wrapping the existing buffer in a
read-only MemoryStream (zero-copy). Benchmarked: 29% reduction in memory
allocations during Write operations (103MB → 73MB on test-performance.xlsx).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Contributor Author

@ken-swyfft ken-swyfft left a comment

Choose a reason for hiding this comment

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

Code Review

Overall: Approve. Both changes are correct and safe across all target frameworks.

StreamHelper.CopyStreamStream.CopyTo()

Straightforward replacement. Internal buffer goes from 1KB to 80KB (the CopyTo default) — strictly better for the two remaining call sites (thumbnail operations). Dead totalRead variable correctly eliminated.

MemoryPackagePart.GetInputStreamImpl — zero-copy buffer view

The change from a full defensive copy to a read-only MemoryStream view over GetBuffer() is safe. I verified:

  1. data is always constructed via the parameterless new MemoryStream() (resizable), so GetBuffer() won't throw UnauthorizedAccessException.
  2. The returned stream is writable: false, preventing accidental mutation.
  3. All callers of GetInputStream() only read from the stream (XML parsing, IOUtils.ToByteArray, IOUtils.Copy, Read loops).
  4. MemoryPackagePartOutputStream always replaces data with new MemoryStream(), so outstanding views referencing the old buffer remain valid and are not corrupted.

Minor suggestion

Consider adding a brief comment on the GetBuffer() line explaining the safety invariants for future maintainers, e.g.:

// Return a read-only, non-copying view over the internal buffer.
// Safe because callers only read, and GetOutputStreamImpl() replaces data entirely.
return new MemoryStream(data.GetBuffer(), 0, (int)data.Length, writable: false);

This makes the preconditions explicit so a future contributor doesn't accidentally break them (e.g., by constructing data from a byte[], which would make GetBuffer() throw).

Also noted

  • ZipPartMarshaller.Marshall (lines 54-67) has a similar manual buffer-copy loop (8KB buffer) that could benefit from Stream.CopyTo as a follow-up.
  • Pre-existing potential null-ref in MemoryPackagePart.Load() if called before GetOutputStreamImpl() — appears to be dead code, not introduced by this PR.

@tonyqus tonyqus added this to the NPOI 2.8.0 milestone Mar 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants