Skip to content

add check to see if we need to seek before hand#1160

Merged
adamhathcock merged 8 commits intoreleasefrom
adam/check-if-seek
Jan 26, 2026
Merged

add check to see if we need to seek before hand#1160
adamhathcock merged 8 commits intoreleasefrom
adam/check-if-seek

Conversation

@adamhathcock
Copy link
Owner

@adamhathcock adamhathcock commented Jan 26, 2026

Related to findings on #1105 by @julianxhokaxhiu

This pull request improves memory efficiency and stream handling in BufferedSubStream by using pooled buffers and optimizing seek operations. It also updates the test suite to better simulate forward-only stream scenarios.

Memory Management Improvements:

  • Switched _cache allocation in BufferedSubStream to use ArrayPool<byte>.Shared.Rent for buffer pooling, and ensured the buffer is returned to the pool in Dispose for reduced GC pressure. [1] [2]

Performance and Stream Handling Enhancements:

  • Updated RefillCache and RefillCacheAsync to avoid unnecessary seek operations by checking the current stream position before seeking, improving performance for sequential reads. [1] [2]

Testing Improvements:

  • Modified BufferReadAndSeekTest to wrap the memory stream with ForwardOnlyStream, better simulating streams that do not support seeking. [1] [2]

@adamhathcock adamhathcock marked this pull request as ready for review January 26, 2026 11:47
@adamhathcock adamhathcock requested a review from Copilot January 26, 2026 11:47
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request aims to improve performance and memory efficiency for buffered/forward-only stream scenarios (per issue #1105) by reducing unnecessary seek operations, pooling buffers, and adjusting tests to better represent non-seekable streams.

Changes:

  • Update BufferedSubStream to rent its internal cache from ArrayPool<byte>.Shared and return it on dispose.
  • Optimize cache refill logic to avoid unnecessary seeks when already at the correct position.
  • Update BufferReadAndSeekTest to use a forward-only stream wrapper.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
tests/SharpCompress.Test/Streams/SharpCompressStreamTest.cs Adjusts a test to wrap the base stream in ForwardOnlyStream to simulate non-seekable behavior.
src/SharpCompress/IO/SharpCompressStream.cs Removes an unused local variable in Seek.
src/SharpCompress/IO/BufferedSubStream.cs Introduces pooled buffer usage and conditional seeking in cache refill paths.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI commented Jan 26, 2026

@adamhathcock I've opened a new pull request, #1161, to work on those changes. Once the pull request is ready, I'll request review from you.

adamhathcock and others added 5 commits January 26, 2026 12:06
Co-authored-by: adamhathcock <527620+adamhathcock@users.noreply.github.com>
Co-authored-by: adamhathcock <527620+adamhathcock@users.noreply.github.com>
Co-authored-by: adamhathcock <527620+adamhathcock@users.noreply.github.com>
Fix ArrayPool corruption from double-disposal in BufferedSubStream
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

2 participants

Comments