Instanced buffer size#1354
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements configurable stream-copy buffer sizes during extraction and writing, exposing a new BufferSize option on ExtractionOptions and WriterOptions (and propagating it through readers/writers and docs) to help performance on slow/high-latency destinations (e.g., SMB network shares).
Changes:
- Add
BufferSizeto extraction/writer options (and interfaces) with defaults based onConstants.BufferSize. - Thread buffer size through reader/writer copy paths (sync/async) and add tests validating propagation.
- Update docs/examples and bump
csharpiertool version.
Review findings (changes needed):
Stream.TransferToinsrc/SharpCompress/Utility.cschanged signature from(destination, maxLength)to(destination, maxLength, bufferSize)without keeping the old overload or making the new parameter optional. If this is a public API (it appears to be, since tests call it cross-assembly), this is a breaking change for existing consumers. Consider restoring the old overload and delegating to the new one, and/or making thebufferSizeparameter optional (while still keeping the old overload if binary compatibility matters).tests/SharpCompress.Test/OptionsUsabilityTests.csis now entirely wrapped in#if !LEGACY_DOTNET, which removes all existing tests in that file from legacy target runs—not just the newly added buffer-size tests. If only some new code requires the guard, consider scoping the preprocessor directive to only the new tests/helpers so prior coverage isn’t lost.src/SharpCompress/Readers/IReaderExtensions.cs/IAsyncReaderExtensions.csmoved fromreader.WriteEntryTo(fs)to manually opening the entry stream and copying. This bypassesAbstractReader.WriteEntryTo’s internal “already wrote this entry” guard/state (_wroteCurrentEntry) visible in the diff, which is a behavioral change. Consider adding a buffer-size-aware entry write path that preserves the reader’s write semantics/state tracking, rather than duplicating the copy logic in extensions.docs/API.mdstates “If it is not set, SharpCompress falls back toConstants.BufferSize”, butWriterOptions.BufferSizeis a non-nullableintwith a default initializer—so it is always set. Consider rewording to “Defaults toConstants.BufferSize”.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/SharpCompress.Test/UtilityTests.cs | Updates TransferTo tests for new signature. |
| tests/SharpCompress.Test/OptionsUsabilityTests.cs | Adds buffer-size propagation tests; introduces #if !LEGACY_DOTNET guard for entire file. |
| src/SharpCompress/Writers/Zip/ZipWriterOptions.cs | Adds BufferSize to zip writer options and copy constructors. |
| src/SharpCompress/Writers/Zip/ZipWriter.cs | Uses WriterOptions.BufferSize for entry stream copy. |
| src/SharpCompress/Writers/WriterOptionsExtensions.cs | Adds fluent WithBufferSize helper. |
| src/SharpCompress/Writers/WriterOptions.cs | Adds BufferSize to common writer options. |
| src/SharpCompress/Writers/Tar/TarWriterOptions.cs | Adds BufferSize to tar writer options and copy constructors. |
| src/SharpCompress/Writers/Tar/TarWriter.cs | Uses configured buffer size when transferring entry data. |
| src/SharpCompress/Writers/Tar/TarWriter.Async.cs | Async transfer uses configured buffer size. |
| src/SharpCompress/Writers/SevenZip/SevenZipWriterOptions.cs | Adds BufferSize to 7z writer options and copy constructors. |
| src/SharpCompress/Writers/GZip/GZipWriterOptions.cs | Adds BufferSize to gzip writer options and copy constructors. |
| src/SharpCompress/Writers/GZip/GZipWriter.cs | Uses WriterOptions.BufferSize for entry stream copy. |
| src/SharpCompress/Utility.cs | Changes TransferTo signature and applies buffer size to CopyTo. |
| src/SharpCompress/Utility.Async.cs | Adds optional bufferSize parameter to TransferToAsync. |
| src/SharpCompress/Readers/IReaderExtensions.cs | Copies entry streams using extraction buffer size (new helper methods). |
| src/SharpCompress/Readers/IAsyncReaderExtensions.cs | Async copy path uses extraction buffer size (new helper methods). |
| src/SharpCompress/Readers/AbstractReader.cs | Plumbs buffer size into internal copy and WriteEntryTo. |
| src/SharpCompress/Readers/AbstractReader.Async.cs | Async copy now uses Options.BufferSize; refactors some internal methods. |
| src/SharpCompress/Common/Options/IWriterOptions.cs | Adds BufferSize to writer options interface. |
| src/SharpCompress/Common/Options/IExtractionOptions.cs | Adds BufferSize to extraction options interface. |
| src/SharpCompress/Common/ExtractionOptions.cs | Adds BufferSize with default to extraction options. |
| src/SharpCompress/Common/Constants.cs | Notes TODO about remaining non-extraction usages. |
| src/SharpCompress/Archives/IArchiveEntryExtensions.cs | Adds buffer-size-aware internal overloads; uses extraction buffer size for file extraction. |
| docs/USAGE.md | Updates extraction example to show BufferSize. |
| docs/API.md | Updates writer/extraction examples and describes BufferSize. |
| .config/dotnet-tools.json | Bumps csharpier tool version. |
…adam/instance-buffer-size
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.
Addresses #1351
This pull request introduces configurable buffer sizes for extraction and writing operations, allowing users to control the memory usage and performance of archive operations. The changes add a
BufferSizeproperty to bothExtractionOptionsandWriterOptions, update the relevant APIs and extension methods to use these buffer sizes, and update documentation and usage examples accordingly.Configurable Buffer Size for Extraction and Writing:
BufferSizeproperty toExtractionOptionsandWriterOptions(and their interfaces) to allow users to specify the buffer size used during stream copy operations. Default is set toConstants.BufferSize(81920 bytes). [1] [2] [3]IArchiveEntryExtensions,IReaderExtensions, andIAsyncReaderExtensionsto use the buffer size from the provided options, falling back to the default if not specified. This includes both synchronous and asynchronous extraction methods. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Documentation and Usage Updates:
API.md,USAGE.md) to show how to set the buffer size inExtractionOptionsandWriterOptions, and explained the effect of the buffer size on extraction and writing. [1] [2] [3] [4] [5]Other Notable Changes:
csharpiertool version in.config/dotnet-tools.jsonfrom 1.2.6 to 1.3.0.These changes give users more control over performance and resource usage during archive extraction and writing, and make the API more flexible and transparent regarding buffer management.