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]: Add CollectionsMarshal.AsMemory(List<T>) #90141

Closed
Neme12 opened this issue Aug 8, 2023 · 14 comments
Closed

[API Proposal]: Add CollectionsMarshal.AsMemory(List<T>) #90141

Neme12 opened this issue Aug 8, 2023 · 14 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Memory

Comments

@Neme12
Copy link

Neme12 commented Aug 8, 2023

Background and motivation

The existing CollectionsMarshal.AsSpan method is really useful in order to be able to get a span from a List<T>, however, in many scenarios, e.g. when calling async methods, what you really need is a Memory<T> instead. Since you can't get a Memory<T> from a Span<T>, you currently need to make a copy of the data. This could be avoided if there was an ability to get a Memory<T> directly from a List<T>.

API Proposal

 namespace System.Runtime.InteropServices;

 public static class CollectionsMarshal
 {
     // Existing method:
     public static Span<T> AsSpan<T>(List<T>? list)

     // New method:
+    public static Memory<T> AsMemory<T>(List<T>? list)
 }

and if #90138 is approved, then also:

 namespace System.Runtime.InteropServices;

 public static class ImmutableCollectionsMarshal
 {
     // Method proposed in #90138:
     public static Span<T> AsSpan<T>(ImmutableArray<T>.Builder? builder);

     // New method:
+    public static Span<T> AsMemory<T>(ImmutableArray<T>.Builder? builder);
 }

API Usage

async Task FooAsync(TextWriter writer)
{
    var list = new List<char>();
    // ...fill the collection
    await writer.WriteAsync(CollectionsMarshal.AsMemory(list));
}

Alternative Designs

No response

Risks

No response

@Neme12 Neme12 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 8, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 8, 2023
@ghost
Copy link

ghost commented Aug 8, 2023

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

Issue Details

Background and motivation

The existing CollectionsMarshal.AsSpan method is really useful in order to be able to get a span from a List<T>, however, in many scenarios, e.g. when calling async methods, what you really need is a Memory<T> instead. Since you can't get a Memory<T> from a Span<T>, you currently need to make a copy of the data. This could be avoided if there was an ability to get a Memory<T> directly from a List<T>.

API Proposal

 namespace System.Runtime.InteropServices;

 public static class CollectionsMarshal
 {
     // Existing method:
     public static Span<T> AsSpan<T>(List<T>? list)

     // New method:
+    public static Memory<T> AsMemory<T>(List<T>? list)
 }

API Usage

async Task FooAsync(TextWriter writer)
{
    var list = new List<char>();
    // ...fill the collection
    await writer.WriteAsync(CollectionsMarshal.AsMemory(list));
}

Alternative Designs

No response

Risks

No response

Author: Neme12
Assignees: -
Labels:

api-suggestion, area-System.Memory

Milestone: -

@Tornhoof
Copy link
Contributor

Tornhoof commented Aug 8, 2023

The AsMemory method was specifically excluded in the original API Review, see #27061 (comment) (and specifically the video).

@huoyaoyuan
Copy link
Member

See also #87210.

@adamsitnik adamsitnik closed this as not planned Won't fix, can't repro, duplicate, stale Aug 8, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 8, 2023
@Neme12
Copy link
Author

Neme12 commented Aug 8, 2023

Could this not be revisited? It's been 4 years since that API review.

@stephentoub
Copy link
Member

While AsSpan(List<T>) baked in the notion of List<T> being backed by contiguous memory, AsMemory(List<T>) would effectively bake in the notion of it being backed by a T[]. That's something we'd like to avoid doing right now. (Such an AsMemory could return a Memory backed by a MemoryManager, but that would then either require that AsMemory to start allocating or it would require the list to grow to be able to cache the MemoryManager reference, neither of which we'd want.)

@Neme12
Copy link
Author

Neme12 commented Aug 8, 2023

Why would it have to bake in the notion of it being a T[]? I don't understand. Memory<T> is an abstraction.

@Neme12
Copy link
Author

Neme12 commented Aug 8, 2023

I understand that it's possible to get the underlying array from a Memory<T>, but only via MemoryMarshal and anyone who does that and relies on it being an array could be expected to be broken IMO. That's akin to people using reflection to get the backing array.

@stephentoub
Copy link
Member

Because Memory<T> is backed by either T[] or a MemoryManager<T>, and we wouldn't use MemoryManager<T> for the reasons I mentioned.

@Neme12
Copy link
Author

Neme12 commented Aug 8, 2023

But that would be an implementation detail. The API would still return an abstraction.

@stephentoub
Copy link
Member

stephentoub commented Aug 8, 2023

Let's say List<T> changed to be backed by something other than T[]. What "implementation detail" would you then employ to keep this new AsMemory functional, fast, and allocation-free?

@Neme12
Copy link
Author

Neme12 commented Aug 8, 2023

I mean I wouldn't have the expectation that it has to be 100% allocation free. I would expect that the data wouldn't be copied so that I could use this with huge data sets, but a small allocation just for the memory manager seems fine IMO.

@stephentoub
Copy link
Member

I mean I wouldn't have the expectation that it has to be 100% allocation free

It's a key expectation in general for this kind of method, that it's effectively a cheap, allocation-free cast.

@DaZombieKiller
Copy link
Contributor

The use case presented in the OP seems like it would be better served by an IBufferWriter<T> implementation for List<T>. That's a whole can of worms of its own, though.

@Neme12
Copy link
Author

Neme12 commented Aug 8, 2023

I mean I wouldn't have the expectation that it has to be 100% allocation free

It's a key expectation in general for this kind of method, that it's effectively a cheap, allocation-free cast.

I guess we have different expectations. Whenever I'm using Memory<T>, I'm aware that there might be a small allocation there for the MemoryManager since Memory<T> can be backed by that.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Memory
Projects
None yet
Development

No branches or pull requests

6 participants