Skip to content

[Obsolete-WIP] Alternative shape for ReadOnlyMemoryStream / WritableMemoryStream#129912

Closed
ViveliDuCh wants to merge 1 commit into
dotnet:mainfrom
ViveliDuCh:base-change-streamwrappers
Closed

[Obsolete-WIP] Alternative shape for ReadOnlyMemoryStream / WritableMemoryStream#129912
ViveliDuCh wants to merge 1 commit into
dotnet:mainfrom
ViveliDuCh:base-change-streamwrappers

Conversation

@ViveliDuCh

Copy link
Copy Markdown
Member

⚠️ Prototype / exploration — not proposing this as a merge or as a re-API-review item. Per discussion on #126669 (r3453266352) and #22838, I haven't seen a feature-blocking reason to revisit the base class so far, and neither approach appears to be wrong, though it could still be worth exploring further. Opening this so the alternative shape is concrete and measurable if it ever comes up again, e.g. if the allocation overhead becomes meaningful for a future hot path.

Follow-up on #126669. Shows what ReadOnlyMemoryStream / WritableMemoryStream would look like derived from Stream instead of MemoryStream.

Context of the trade-off (recap of the prior discussion)

  • Allocation: Jozkee measured ROMS 96 → 56 B, WMS 104 → 64 B (~40 B saved per instance) on a Stream base.
  • Ecosystem cost: Adam noted that several callers in the wider .NET ecosystem special-case is MemoryStream for fast paths; deriving from MemoryStream lets the new wrappers participate in those optimizations and keeps naming consistency.
  • Verdict on the prior PR: Stay on MemoryStream for the initial shape, with private protected field promotions on MemoryStream so 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

Before                              After (prototype)
+--------------------------+        +--------------------------+
| MemoryStream base (88 B) |        | Stream base (48 / 56 B)  |
| + _memory                |   →    | + _memory                |
+--------------------------+        | + _position              |
                                    | + _isOpen [+ _length]    |
                                    | + _lastReadTask          |
                                    +--------------------------+

ReadOnlyMemoryStream:  88 B → 48 B
WritableMemoryStream:  88 B → 56 B
File Change
MemoryStream.cs Reverts the private protected field promotions added in #126669 — no consumer in the hierarchy under this shape.
ReadOnlyMemoryStream.cs Base : MemoryStream: Stream. Adds local backing state and Stream overrides (CanRead/CanSeek/CanWrite/Length/Position/Seek/Flush/FlushAsync/SetLength). Capacity, GetBuffer, TryGetBuffer, ToArray, WriteTo lose override; Capacity setter dropped (only threw). Write/Write(ROSpan)/WriteByte throw NotSupportedException.
WritableMemoryStream.cs Same skeleton; keeps _length and the writable surface.
ref/System.Runtime.cs Mirrors the prototype shape.

Trade-offs

For a Stream base

  • Smaller instances (~40 B / -36–45 %).
  • The two wrappers don't use any of MemoryStream's buffer-management state (_buffer, _origin, _capacity, _expandable, _exposable), so the inherited fields are dead weight on every instance.
  • The MemoryStream base also brings inherited surface (WriteTo(byte[]), expandable-capacity semantics, GetBuffer/TryGetBuffer over an internal byte[], etc.) that doesn't really apply to a fixed Memory<byte> wrapper — under the current shape these have to be overridden to throw or no-op.

Against a Stream base

  • Drops out of is MemoryStream fast paths used by several ecosystem consumers (mono, EFCore, MSBuild and friends).
  • Loses naming consistency: the types are still called …MemoryStream, which is a soft signal to users (and to the framework design "self-documenting names" guidance) that they are MemoryStreams. Renaming is on the table in principle, but if the names stay then a non-MemoryStream base is a mismatch worth flagging.

Testing

Suite Result
WritableMemoryStreamTests 7/7 ✓
ReadOnlyMemoryStreamTests 3/3 ✓
WritableMemoryStreamConformanceTests 128/128 ✓
ReadOnlyMemoryStreamConformanceTests 128/128 ✓
Full System.IO.Tests 2360/2360 ✓
System.IO.UnmanagedMemoryStream.Tests 289/289 ✓

Seek keeps MemoryStream's IOException(SR.IO_SeekBeforeBegin) on negative target to preserve the existing conformance contract.

Note

This PR description was drafted with the assistance of GitHub Copilot.

Copilot AI review requested due to automatic review settings June 26, 2026 20:38
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR prototypes an alternative implementation shape for the public System.IO.ReadOnlyMemoryStream and System.IO.WritableMemoryStream types by deriving them directly from Stream (rather than MemoryStream), with the goal of reducing per-instance size and avoiding inherited MemoryStream buffer-management surface that doesn’t apply to fixed Memory<byte> / ReadOnlyMemory<byte> wrappers.

Changes:

  • Switch ReadOnlyMemoryStream / WritableMemoryStream from MemoryStream-derived to Stream-derived implementations, adding local state and Stream overrides.
  • Revert MemoryStream field visibility promotions (private protectedprivate) that were used to share state with the wrappers in the MemoryStream-derived shape.
  • Update the System.Runtime ref assembly to reflect the prototype’s public API shape.

Reviewed changes

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

File Description
src/libraries/System.Private.CoreLib/src/System/IO/MemoryStream.cs Reverts field visibility promotions that were only needed for the MemoryStream-derived wrapper hierarchy.
src/libraries/System.Private.CoreLib/src/System/IO/ReadOnlyMemoryStream.cs Implements ReadOnlyMemoryStream : Stream with local position/open state and relevant Stream overrides; keeps MemoryStream-like helper methods as non-overrides.
src/libraries/System.Private.CoreLib/src/System/IO/WritableMemoryStream.cs Implements WritableMemoryStream : Stream with local position/length/open state and relevant Stream overrides; keeps helper methods as non-overrides.
src/libraries/System.Runtime/ref/System.Runtime.cs Updates public contract (base type and member set) to match the prototype Stream-derived shape.

Comment on lines +11056 to +11060
public sealed partial class ReadOnlyMemoryStream : System.IO.Stream
{
public ReadOnlyMemoryStream(System.ReadOnlyMemory<byte> source) { }
public override int Capacity { get { throw null; } set { } }
public int Capacity { get { throw null; } }
public override bool CanRead { get { throw null; } }
Comment on lines +60 to +66
set
{
ArgumentOutOfRangeException.ThrowIfNegative(value);
ArgumentOutOfRangeException.ThrowIfGreaterThan(value, int.MaxValue);
EnsureNotClosed();
_position = (int)value;
}
Comment on lines +61 to +67
set
{
ArgumentOutOfRangeException.ThrowIfNegative(value);
ArgumentOutOfRangeException.ThrowIfGreaterThan(value, int.MaxValue);
EnsureNotClosed();
_position = (int)value;
}
@ViveliDuCh

Copy link
Copy Markdown
Member Author

Testing AI workflow, wrong targeting branch. Closing as duplicate of #129913 — superseded by the manually-opened PR against the correct upstream branch.

@ViveliDuCh ViveliDuCh closed this Jun 26, 2026
@ViveliDuCh ViveliDuCh changed the title [Prototype] Alternative shape: derive ReadOnlyMemoryStream / WritableMemoryStream from Stream [Old draft] Alternative shape: derive ReadOnlyMemoryStream / WritableMemoryStream from Stream Jun 26, 2026
@ViveliDuCh ViveliDuCh changed the title [Old draft] Alternative shape: derive ReadOnlyMemoryStream / WritableMemoryStream from Stream [Obsolete-WIP] Alternative shape for ReadOnlyMemoryStream / WritableMemoryStream Jun 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants