Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: System.Buffers : BuffersExtensions.AsStream #100434

Open
mgravell opened this issue Mar 29, 2024 · 6 comments
Open

[API Proposal]: System.Buffers : BuffersExtensions.AsStream #100434

mgravell opened this issue Mar 29, 2024 · 6 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Buffers needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Milestone

Comments

@mgravell
Copy link
Member

mgravell commented Mar 29, 2024

Background and motivation

This is tangentially related to #100290 and the .NET 9 distributed caching epic; the proposed hybid-cache API will use ReadOnlySequence<byte> and IBufferWriter<byte> as the primary serializer APIs, but: not all serializers support these APIs (many do, note) - with Stream being the most common fallback.

API Proposal

  // assembly: System.Memory
  namespace System.Buffers;
  
  public static class BuffersExtensions // pre-existing
  {
+     public static Stream AsStream(this ReadOnlySequence<byte> value) => new ReadOnlySequenceStream(value);
+     public static Stream AsStream(this IBufferWriter<byte> value) => new BufferWriterStream(value);
  }
+ internal sealed class ReadOnlySequenceStream : Stream {...}
+ internal sealed class BufferWriterStream : Stream {...}

API Usage

ReadOnlySequence<byte> payload = ...
Customer obj = SomeRandomSerializer.Deserialize<Customer>(payload.AsStream());

and

Customer obj = ...
IBufferWriter<byte> target = ...
SomeRandomSerializer.Serialize<Customer>(target.AsStream(), obj);

The implementations would be internal, but:

  • ReadOnlySequenceStream has a CanRead: true, CanWrite: false implementation that supports seek etc; no additional buffer copies, just a few "where are we" counters, using SequencePosition iteration and a ReadOnlyMemory<byte> snapshot of the current segment
  • BufferWriterStream has a CanRead: false, CanWrite: true implementation that does not support seek; position and length are read-only and report the bytes written so far; no double-buffering - it is assumed (as a fundamental part of IBufferWriter<byte>) that the underlying IBufferWriter<byte> already does some internal work there when responding to GetSpan/GetMemory (such that they are affordable), so this would be duplicated effort and an additional mem-copy
  • in both cases, all APIs are synchronous, with the async APIs implemented as async-over-sync as efficiently as possible
  • no need for fancy recycling, since we don't have any local buffers etc

Alternative Designs

The alternative is to use MemoryStream, which involves multiple additional copy operations and additional byte[] buffers.

Risks

None seen

Additional

I am happy to contribute the implementation effort.

@mgravell mgravell added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 29, 2024
Copy link
Contributor

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

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Mar 29, 2024
@mgravell mgravell changed the title [API Proposal]: System.Buffers : BufferExtensions.AsStream [API Proposal]: System.Buffers : BuffersExtensions.AsStream Mar 29, 2024
@stephentoub
Copy link
Member

The ReadOnlySequence<T> side of this is #27156. @jozkee, where did we land on a design for these extra streams, e.g. AsStream vs dedicated publicly named streams vs etc. Implementation is straightforward, so if we have an agreed upon design, we can make quick forward progress here.

@halter73 halter73 added api-ready-for-review API is ready for review, it is NOT ready for implementation api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 1, 2024
@jozkee
Copy link
Member

jozkee commented Apr 1, 2024

#82801 (comment)

For [ReadOnly]Memory<byte> and ReadOnlySequence<byte>, we want to have static methods that return the Stream wrapping them. If we want to have them as extension methods in Stream we would need the static extensions language feature to have AsStream(this ReadOnlySequence<byte>), or we move ReadOnlySequence down to S.P.CoreLib.

I discarded having Memory<byte> support directly in MemoryStream based on #84103.

@mgravell
Copy link
Member Author

mgravell commented Apr 1, 2024

Re corelib: that's why I proposed the existing BuffersExtensions as the root (or something in the same package/namespace) - it should have all the right refs.

@PaulusParssinen
Copy link
Contributor

Noting that we now have System.IO.Pipelines available in .NET 9 shared runtime meaning that there are now two types implementing IBufferWriter<byte> in the BCL; PipeWriter and ArrayBufferWriter<byte>.

When I use S.IO.Pipelines I often find myself having to pull CommunityToolkit.HighPerformance in for its AsStream(this IBufferWriter<byte> writer) fallback.

@jozkee
Copy link
Member

jozkee commented Apr 2, 2024

Considering that static extensions is in the backlog dotnet/csharplang#192, I think we should consider this for 9 while keeping #82801 as future.

@tannergooding tannergooding added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Jun 24, 2024
@stephentoub stephentoub added this to the Future milestone Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Buffers needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
None yet
Development

No branches or pull requests

6 participants