Add Memory Overrides to Streams #47125
Merged
Conversation
This file contains 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
-Implemented memory-based WriteAsync in DeflateStream.CopyToStream class. This required implementing a memory-based overload of System.IO.Inflater.SetInput(). Previously, Inflater used a GCHandle to pin the array that was passed into SetInput. I converted it to use a MemoryHandle, and changed the array-based overload of SetInput to delegate to the new Memory-based overload.
stephentoub
reviewed
Jan 19, 2021
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/DeflateStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs
Outdated
Show resolved
Hide resolved
In particular for any that are controversial or non-trivial, one per PR is good. For the rest, you can batch them up. |
- Memory overrides for System.Net.RequestStream - Memory overrides for System.Net.NetworkStreamWrapper
WriteAsync is implemented in terms of Write, so I went ahead and implemented Write(ReadOnlySpan<byte>). For some reason, AsSpan() isn't available in this file.
This was referenced Jan 21, 2021
stephentoub
reviewed
Jan 21, 2021
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/DeflateStream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs
Outdated
Show resolved
Hide resolved
NewellClark
changed the title
carlossanlop
approved these changes
Mar 4, 2021
There was a problem hiding this comment.
Triage: Reviewed with @adamsitnik and @jozkee and it looks good.
Suggestions have been addressed and CI is passing.
adamsitnik
approved these changes
Mar 5, 2021
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/DeflateStream.cs
Outdated
Show resolved
Hide resolved
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
In the process of implementing Override Stream ReadAsync/WriteAsync, I ran the analyzer against dotnet/runtime and found 7 violations. This is one of them.
I've added the required memory-based overrides, and refactored it so the array-based overrides are implemented in terms of the memory-based overrides. In order to do this, I had to add an overload of
Inflater.SetBufferthat accepts memory. Same thing here; I refactored the array-basedSetInputto delegate to the new memory-based overload.I've also fixed
RequestStream,ChunkedMemoryStream, andNetworkStreamWrapper, which were all trivial fixes.Should I do a separate PR for every violation I fix, or fix them all in one PR? Some of the fixes are non-trivial, and would require adding overloads to multiple dependencies used by the violating stream, which is why I figured a separate PR would be in order for each one.
Edit: to clarify, this is the PR I'm using for all the streams that are trivial to update. I'll be doing separate PRs for the streams that are not trivial to update.