Add Stream wrappers for memory and text-based types#126669
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces new standardized Stream wrapper types across CoreLib and System.Memory, aligning with the approved API shape from #82801 and updating a few internal call sites to use the new wrappers.
Changes:
- Added new public sealed stream wrappers:
StringStream,ReadOnlyMemoryStream,WritableMemoryStream(CoreLib) andReadOnlySequenceStream(System.Memory). - Updated reference assemblies and project files to expose/compile the new APIs.
- Added conformance + targeted behavioral tests, and updated select internal consumers to use
StringStream.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/IO/StringStream.cs | Implements the new StringStream API (read-only, non-seekable, on-the-fly encoding). |
| src/libraries/System.Private.CoreLib/src/System/IO/ReadOnlyMemoryStream.cs | Implements seekable read-only stream over ReadOnlyMemory<byte>. |
| src/libraries/System.Private.CoreLib/src/System/IO/WritableMemoryStream.cs | Implements seekable fixed-capacity read/write stream over Memory<byte>. |
| src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems | Wires new CoreLib stream wrapper source files into the build. |
| src/libraries/System.Private.CoreLib/src/Resources/Strings.resx | Adds a new resource string entry (currently appears unused). |
| src/libraries/System.Runtime/ref/System.Runtime.cs | Updates public API surface to include the new stream wrapper types (and a small ref signature normalization change). |
| src/libraries/System.Runtime/tests/System.IO.Tests/System.IO.Tests.csproj | Includes the new test files in the System.IO test project. |
| src/libraries/System.Runtime/tests/System.IO.Tests/StringStream/StringStreamConformanceTests.cs | Adds conformance coverage for StringStream (string + ReadOnlyMemory<char> overloads). |
| src/libraries/System.Runtime/tests/System.IO.Tests/StringStream/StringStreamTests_String.cs | Adds targeted StringStream(string, Encoding) behavioral tests. |
| src/libraries/System.Runtime/tests/System.IO.Tests/StringStream/StringStreamTests_Memory.cs | Adds targeted StringStream(ReadOnlyMemory<char>, Encoding) behavioral tests. |
| src/libraries/System.Runtime/tests/System.IO.Tests/ReadOnlyMemoryStream/ReadOnlyMemoryStreamConformanceTests.cs | Adds conformance coverage for ReadOnlyMemoryStream. |
| src/libraries/System.Runtime/tests/System.IO.Tests/ReadOnlyMemoryStream/ReadOnlyMemoryStreamTests.cs | Adds targeted ReadOnlyMemoryStream behavioral tests. |
| src/libraries/System.Runtime/tests/System.IO.Tests/WritableMemoryStream/WritableMemoryStreamConformanceTests.cs | Adds conformance coverage for WritableMemoryStream (with some overridden tests). |
| src/libraries/System.Runtime/tests/System.IO.Tests/WritableMemoryStream/WritableMemoryStreamTests.cs | Adds targeted WritableMemoryStream behavioral tests. |
| src/libraries/System.Memory/src/System/Buffers/ReadOnlySequenceStream.cs | Implements ReadOnlySequenceStream over ReadOnlySequence<byte>. |
| src/libraries/System.Memory/src/System.Memory.csproj | Includes the new ReadOnlySequenceStream source file in System.Memory. |
| src/libraries/System.Memory/src/Resources/Strings.resx | Adds SR strings needed for stream-like exception messages in System.Memory. |
| src/libraries/System.Memory/ref/System.Memory.cs | Adds ReadOnlySequenceStream to the System.Memory ref surface. |
| src/libraries/System.Memory/tests/System.Memory.Tests.csproj | Adds tests for ReadOnlySequenceStream and references StreamConformanceTests. |
| src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceStreamTests.cs | Adds targeted behavioral tests for multi-segment ReadOnlySequenceStream scenarios. |
| src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceStream.ConformanceTests.cs | Adds conformance coverage for ReadOnlySequenceStream. |
| src/libraries/System.Private.Xml/src/System/Xml/Resolvers/XmlPreloadedResolver.cs | Switches internal string-to-stream conversion to StringStream. |
| src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/Json/JsonXmlDataContract.cs | Switches internal string-to-stream conversion to StringStream. |
Comments suppressed due to low confidence (1)
src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/Json/JsonXmlDataContract.cs:38
- Variable is still named 'memoryStream' but the implementation is now StringStream (not a MemoryStream). Rename the local to avoid implying the stream is seekable/in-memory-buffered (e.g., 'xmlContentStream').
Stream memoryStream = new StringStream(xmlContent, Encoding.UTF8);
object? xmlValue;
XmlDictionaryReaderQuotas? quotas = ((JsonReaderDelegator)jsonReader).ReaderQuotas;
if (quotas == null)
{
xmlValue = dataContractSerializer.ReadObject(memoryStream);
27761ff to
b43698b
Compare
b43698b to
95a2989
Compare
95a2989 to
5e3f98c
Compare
…onvert edge case. Flush strategy update.
5e3f98c to
fec7a59
Compare
| Task<int> task1 = stream.ReadAsync(buffer1, 0, 5); | ||
| Task<int> task2 = stream.ReadAsync(buffer2, 0, 5); | ||
| Task<int> task3 = stream.ReadAsync(buffer3, 0, 5); | ||
|
|
||
| await task1; | ||
| await task2; | ||
| await task3; | ||
|
|
||
| Assert.Same(task1, task2); | ||
| Assert.Same(task2, task3); | ||
|
|
There was a problem hiding this comment.
Intentional — validates the Task caching optimization built into the stream implementation.
There was a problem hiding this comment.
You may want to reconsider this one, I don't see the Task caching optimization built into ReadOnlySequenceStream.
There was a problem hiding this comment.
Good catch, thanks. Moved CachedCompletedInt32Task to Common/src/ as shared source (since the struct is internal to CoreLib and inaccessible from System.Memory) and wired it into ReadOnlySequenceStream.ReadAsync(byte[], ...) via a _lastReadTask field, matching the established MemoryStream/BufferedStream pattern.
…eam length/seek semantics, ReadOnlySequenceStream safety, and consolidate tests
adamsitnik
left a comment
There was a problem hiding this comment.
I really like the simplicity of the design and implementation.
Overall it LGTM, I found mostly some nits related to tests. I will need to perform another round for StringStream.
Thank you @ViveliDuCh !
|
Note AI-generated content (Copilot) Base Class Benchmark:
|
|
Note AI-generated content (Copilot) Base Class Benchmark:
|
…s, and conformance coverage
d8e4b7a to
8e4a441
Compare
| public class ReadOnlySequenceStreamTests | ||
| { | ||
| [Fact] | ||
| public void ReadZeroBytesReturnsZero() |
There was a problem hiding this comment.
This seems like a duplicate of
There was a problem hiding this comment.
Can you try to ask copilot to cross-reference all your tests, so they are not duplicating the ones from StreamConformanceTests. That's how I found these.
| } | ||
|
|
||
| [Fact] | ||
| public void SeekingBeyondEmptyBufferIsAllowed() |
There was a problem hiding this comment.
We should maybe instead update Seek_PastEnd_ReadReturns0 to assert Position is updated when seeking past stream.
| Task<int> task1 = stream.ReadAsync(buffer1, 0, 5); | ||
| Task<int> task2 = stream.ReadAsync(buffer2, 0, 5); | ||
| Task<int> task3 = stream.ReadAsync(buffer3, 0, 5); | ||
|
|
||
| await task1; | ||
| await task2; | ||
| await task3; | ||
|
|
||
| Assert.Same(task1, task2); | ||
| Assert.Same(task2, task3); | ||
|
|
There was a problem hiding this comment.
You may want to reconsider this one, I don't see the Task caching optimization built into ReadOnlySequenceStream.
| } | ||
|
|
||
| [Fact] | ||
| public async Task ReadAsyncArrayBackedMemoryUsesFastPath() |
There was a problem hiding this comment.
This isn't testing any fast path I believe and there's nothing special about it. I suggest removing it.
| { | ||
| return 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
Have you considered adding a single-shot Encoding.GetBytes fast path? It is ~34% faster than using Encoder.Convert and avoids some allocations: https://gist.github.com/jozkee/ec72a16575edac7fcbde864cde61cd76.
Results:
| Method | Length | Mean | Error | StdDev | Ratio | Gen0 | Allocated | Alloc Ratio |
|---|---|---|---|---|---|---|---|---|
| Baseline | 16 | 40.10 ns | 6.150 ns | 0.337 ns | 1.00 | 0.0100 | 168 B | 1.00 |
| OneShot | 16 | 26.60 ns | 2.302 ns | 0.126 ns | 0.66 | 0.0076 | 128 B | 0.76 |
| Baseline | 1024 | 57.02 ns | 3.955 ns | 0.217 ns | 1.00 | 0.0100 | 168 B | 1.00 |
| OneShot | 1024 | 42.24 ns | 8.008 ns | 0.439 ns | 0.74 | 0.0076 | 128 B | 0.76 |
| // cannot hold a single complete encoded character. | ||
| if (buffer.Length < _encoding.GetMaxByteCount(1)) | ||
| { | ||
| _pendingBytes ??= new byte[_encoding.GetMaxByteCount(2)]; |
There was a problem hiding this comment.
This could benefit a bit from GC.AllocateUninitializedArray, as done in
|
|
||
| _text = text.AsMemory(); | ||
| _encoding = encoding; | ||
| _encoder = encoding.GetEncoder(); |
There was a problem hiding this comment.
If we do #126669 (comment), we should lazy-init this.
| _disposed = true; | ||
| base.Dispose(disposing); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should CopyTo[Async] be overridden?
Fixes #82801
Introduces standardized stream wrappers for text and memory types as public sealed classes (
StringStream,ReadOnlyMemoryStream,WritableMemoryStream, andReadOnlySequenceStream) as approved in API review (video).What's included
StringStream(System.IO, CoreLib): non-seekable, read-only stream that encodesstringorReadOnlyMemory<char>on-the-fly. Encoding is required (no default). Never emits BOMs.ReadOnlyMemoryStream(System.IO, CoreLib): seekable, read-only stream overReadOnlyMemory<byte>WritableMemoryStream(System.IO, CoreLib): seekable, read-write stream overMemory<byte>with fixed capacityReadOnlySequenceStream(System.Buffers, System.Memory): seekable, read-only stream overReadOnlySequence<byte>System.RuntimeandSystem.MemoryReadOnlyMemoryContent,XmlPreloadedResolver,JsonXmlDataContract) to use the new typesImplementation notes
public sealedwith direct constructors: the review moved away from factory methods onStreamto avoid derived-type IntelliSense noiseStringStreamencodes directly into the caller's buffer inRead()rather than using an internal byte bufferReadOnlyMemoryStreamandWritableMemoryStreamderive fromMemoryStream.TryGetBufferreturns false andGetBufferthrows on the memory stream types (nopubliclyVisiblesupport for now)Limitations
ReadOnlyMemoryStreaminCommon/cannot be fully replaced:System.Memory.Datamulti-targetsnetstandard2.0where the new public type doesn't exist.System.Net.Http(net11.0 only) can adopt it directly.Follow-up work
IBufferWriter<byte>→Streamwrapper (separate proposal per #100434)