[Prototype] Alternative shape: derive ReadOnlyMemoryStream / WritableMemoryStream from Stream - #129913
[Prototype] Alternative shape: derive ReadOnlyMemoryStream / WritableMemoryStream from Stream#129913ViveliDuCh wants to merge 1 commit into
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-io |
There was a problem hiding this comment.
Pull request overview
This PR prototypes an alternative implementation of ReadOnlyMemoryStream and WritableMemoryStream by deriving them from Stream (with local backing state) instead of MemoryStream, and reverts the private protected field promotions on MemoryStream that were previously added to support a MemoryStream-based wrapper shape.
Changes:
ReadOnlyMemoryStream/WritableMemoryStream: base type changed toStream, with added local state (_position,_lengthwhere applicable,_isOpen, cached read task) and explicitStreamoverrides (seek/position/length/flush/etc.).MemoryStream: revertsprivate protectedfield visibility back toprivate.ref/System.Runtime.cs: updates the public API contract to match the new base type and member declarations.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/libraries/System.Runtime/ref/System.Runtime.cs | Updates the public contract to reflect ReadOnlyMemoryStream / WritableMemoryStream : Stream and their members. |
| src/libraries/System.Private.CoreLib/src/System/IO/ReadOnlyMemoryStream.cs | Re-implements ReadOnlyMemoryStream as a Stream-derived type with its own position/open-state and required overrides. |
| src/libraries/System.Private.CoreLib/src/System/IO/WritableMemoryStream.cs | Re-implements WritableMemoryStream as a Stream-derived type with its own length/position/open-state and required overrides. |
| src/libraries/System.Private.CoreLib/src/System/IO/MemoryStream.cs | Reverts promoted field visibility (private protected → private). |
| set | ||
| { | ||
| ArgumentOutOfRangeException.ThrowIfNegative(value); | ||
| ArgumentOutOfRangeException.ThrowIfGreaterThan(value, int.MaxValue); | ||
| EnsureNotClosed(); | ||
| _position = (int)value; | ||
| } |
| { | ||
| EnsureNotClosed(); | ||
|
|
||
| ArgumentOutOfRangeException.ThrowIfGreaterThan(offset, int.MaxValue); |
| /// <inheritdoc/> | ||
| public override bool TryGetBuffer(out ArraySegment<byte> buffer) | ||
| public override void SetLength(long value) => | ||
| throw new NotSupportedException(SR.NotSupported_UnwritableStream); |
| /// <inheritdoc/> | ||
| public override void Write(byte[] buffer, int offset, int count) | ||
| { | ||
| ValidateBufferArguments(buffer, offset, count); | ||
| throw new NotSupportedException(SR.NotSupported_UnwritableStream); | ||
| } |
| public override void Write(ReadOnlySpan<byte> buffer) => | ||
| throw new NotSupportedException(SR.NotSupported_UnwritableStream); | ||
|
|
||
| /// <inheritdoc/> | ||
| public override void WriteByte(byte value) => | ||
| throw new NotSupportedException(SR.NotSupported_UnwritableStream); |
| set | ||
| { | ||
| ArgumentOutOfRangeException.ThrowIfNegative(value); | ||
| ArgumentOutOfRangeException.ThrowIfGreaterThan(value, int.MaxValue); | ||
| EnsureNotClosed(); | ||
| _position = (int)value; | ||
| } |
| { | ||
| EnsureNotClosed(); | ||
|
|
||
| ArgumentOutOfRangeException.ThrowIfGreaterThan(offset, int.MaxValue); |
| @@ -238,8 +313,9 @@ public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationTo | |||
| /// <inheritdoc/> | |||
| public override void SetLength(long value) => throw new NotSupportedException(SR.NotSupported_MemStreamNotExpandable); | |||
| public sealed partial class ReadOnlyMemoryStream : System.IO.Stream | ||
| { |
|
|
||
| /// <summary>Always throws; the underlying buffer is not exposed.</summary> | ||
| /// <exception cref="UnauthorizedAccessException">Always thrown.</exception> | ||
| public byte[] GetBuffer() => |
There was a problem hiding this comment.
The methods that expose raw buffer as a byte array should most likely be deleted if we want to inherit from Stream. They will just always keep throwing and pollute the API surface. Instead we should consider introducing a method that returns ReadOnlyMemory<byte>
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "8f9d4d428d91d9b6ad2ce33eb1712327d986a6a6",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "eb60eb48998e0f79aee7217ef8faf9ba98cdb30a",
"last_reviewed_commit": "8f9d4d428d91d9b6ad2ce33eb1712327d986a6a6",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "eb60eb48998e0f79aee7217ef8faf9ba98cdb30a",
"last_recorded_worker_run_id": "29677707556",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "8f9d4d428d91d9b6ad2ce33eb1712327d986a6a6",
"review_id": 4730526216
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: The PR is explicitly an exploratory prototype — the author states up front it is not proposed for merge — that makes the alternative : Stream shape (vs. the shipped : MemoryStream shape from #126669) concrete and measurable. As a prototype the motivation is clear and legitimate; as a merge candidate the motivation is intentionally absent, and the linked #22838 is api-needs-work.
Approach: Re-parenting both wrappers on Stream and giving each its own backing state (_memory, _position, _isOpen, _length, _lastReadTask) is a reasonable way to demonstrate the ~40 B/instance savings, and it also reverts the private protected field promotions in MemoryStream that only existed to support the shipped shape. The trade-offs (loss of is MemoryStream fast-path participation, naming-vs-base-type mismatch) are well documented. The implementation faithfully reproduces MemoryStream semantics for the surface it keeps.
Summary: ref/System.Runtime.cs: : MemoryStream → : Stream) and demotes Capacity/GetBuffer/TryGetBuffer/ToArray/WriteTo from override to non-override, which is a binary/source breaking change with no api-approved issue — it must not merge to main in this form. The author already acknowledges this, and existing reviews (Copilot + adamsitnik) have covered the API-approval gate, disposal-ordering/param-name nits, and the buffer-method design question. Beyond confirming those, I add one new observation about WritableMemoryStream.SetLength behavior for a human to weigh if this shape is ever pursued.
Detailed Findings
❌ API Approval — Public base-type change with no approved API
The ref changes re-parent ReadOnlyMemoryStream and WritableMemoryStream from System.IO.MemoryStream to System.IO.Stream, and change several members from override to newly-declared instance members (Capacity, GetBuffer, TryGetBuffer, ToArray, WriteTo). Changing a shipping type's base class and dropping overrides is a breaking change to the public contract (x is MemoryStream, virtual dispatch, binary compatibility). Per the blocking API-approval procedure, new/changed public surface requires a linked api-approved issue; #22838 is api-needs-work and the PR links no approved proposal. This is already flagged by Copilot on the ref file; I concur and it is the dominant blocker for any merge. The author explicitly scopes this as a non-merge prototype, so this is a gate note rather than a request to change the prototype.
⚠️ Behavioral change — WritableMemoryStream.SetLength now always throws (new observation)
See the inline comment on WritableMemoryStream.cs. Under the shipped : MemoryStream shape the type inherits SetLength, which for a fixed-capacity buffer supports truncating/extending the logical length within Capacity; the prototype makes it unconditionally throw NotSupportedException, removing that capability. This is a semantic regression distinct from the disposal-ordering nit already noted. Worth a human decision on whether the fixed-buffer contract should retain bounded SetLength.
⚠️ Dead/always-throwing buffer surface (already raised by adamsitnik)
GetBuffer (always throws UnauthorizedAccessException) and TryGetBuffer (always false) on a Stream base add public members that never succeed. adamsitnik already suggested deleting them and instead exposing a ReadOnlyMemory<byte> accessor. I concur — as raised, no new inline needed.
✅ Disposal-ordering / param-name nits already covered
Copilot already flagged that Position/Seek/SetLength/Write* validate or throw before EnsureNotClosed() and omit nameof(...) param names in a few spots, diverging from MemoryStream diagnostics. These are valid; I do not duplicate them. If the prototype graduates, they should be reconciled against the MemoryStream conformance contract.
✅ Core read/seek/write semantics faithfully reproduced
The Read/ReadByte/CopyTo/CopyToAsync/Write/WriteByte/EnsureCapacity logic mirrors MemoryStream's fixed-buffer behavior (including zero-fill of the gap when _position > _length, IOException(SR.IO_SeekBeforeBegin) on negative seek target, and cancellation handling). The reverted private protected → private field changes in MemoryStream are internally consistent since no in-hierarchy consumer remains under this shape. The PR reports full conformance suites passing, which is consistent with the code read.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 116.1 AIC · ⌖ 11.5 AIC · ⊞ 10K
| @@ -238,8 +313,9 @@ public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationTo | |||
| /// <inheritdoc/> | |||
| public override void SetLength(long value) => throw new NotSupportedException(SR.NotSupported_MemStreamNotExpandable); | |||
There was a problem hiding this comment.
: MemoryStream shape, WritableMemoryStream inherits SetLength, which for a fixed (non-expandable) buffer is allowed to shrink/grow the logical length up to Capacity. Making SetLength unconditionally throw NotSupportedException removes that capability entirely, so callers can no longer truncate the writable region without discarding and re-wrapping. If this shape were ever pursued, consider supporting SetLength(value) for 0 <= value <= Capacity (adjusting _length and clamping _position) to preserve the fixed-buffer MemoryStream contract, rather than throwing for all values. (This is separate from the already-noted EnsureNotClosed() ordering nit.)
Follow-up on #126669. Shows what
ReadOnlyMemoryStream/WritableMemoryStreamwould look like derived fromStreaminstead ofMemoryStream.Context of the trade-off (recap of the prior discussion)
Streambase.is MemoryStreamfor fast paths; deriving fromMemoryStreamlets the new wrappers participate in those optimizations and keeps naming consistency.MemoryStreamfor the initial shape, withprivate protectedfield promotions onMemoryStreamso the wrappers can share state — the shape that shipped.This PR is the other side of that fork, in case it's ever useful to compare.
What this prototype does
MemoryStream.csprivate protectedfield promotions added in #126669 — no consumer in the hierarchy under this shape.ReadOnlyMemoryStream.cs: MemoryStream→: Stream. Adds local backing state andStreamoverrides (CanRead/CanSeek/CanWrite/Length/Position/Seek/Flush/FlushAsync/SetLength).Capacity,GetBuffer,TryGetBuffer,ToArray,WriteToloseoverride;Capacitysetter dropped (only threw).Write/Write(ROSpan)/WriteBytethrowNotSupportedException.WritableMemoryStream.cs_lengthand the writable surface.ref/System.Runtime.csTrade-offs
For a
StreambaseMemoryStream's buffer-management state (_buffer,_origin,_capacity,_expandable,_exposable), so the inherited fields are dead weight on every instance.MemoryStreambase also brings inherited surface (WriteTo(byte[]), expandable-capacity semantics,GetBuffer/TryGetBufferover an internalbyte[], etc.) that doesn't really apply to a fixedMemory<byte>wrapper — under the current shape these have to be overridden to throw or no-op.Against a
Streambaseis MemoryStreamfast paths used by several ecosystem consumers (mono, EFCore, MSBuild and friends).…MemoryStream, which is a soft signal to users (and to the framework design "self-documenting names" guidance) that they areMemoryStreams. Renaming is on the table in principle, but if the names stay then a non-MemoryStreambase is a mismatch worth flagging.Testing
WritableMemoryStreamTestsReadOnlyMemoryStreamTestsWritableMemoryStreamConformanceTestsReadOnlyMemoryStreamConformanceTestsSystem.IO.TestsSystem.IO.UnmanagedMemoryStream.TestsSeekkeepsMemoryStream'sIOException(SR.IO_SeekBeforeBegin)on negative target to preserve the existing conformance contract.Note
This PR description was drafted with the assistance of GitHub Copilot.