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 List<T> AsSpan to CollectionsMarshal #27061

Closed
benaadams opened this issue Aug 4, 2018 · 19 comments · Fixed by dotnet/coreclr#26867
Closed

[Api Proposal] Add List<T> AsSpan to CollectionsMarshal #27061

benaadams opened this issue Aug 4, 2018 · 19 comments · Fixed by dotnet/coreclr#26867
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections
Milestone

Comments

@benaadams
Copy link
Member

Its not a "safe" operation, though it is a useful one to gain access to the prebuilt array; without first copying using .ToArray()

namespace System.Runtime.InteropServices
{
    public partial static class CollectionsMarshal
    {
        public Span<T> AsSpan(List<T> list);
        public Memory<T> AsMemory(List<T> list);
    }
}

/cc @jkotas

@benaadams benaadams changed the title Add List<T>.AsSpan to MemoryMarshal [Api Proposal] Add List<T>.AsSpan to MemoryMarshal Aug 4, 2018
@jkotas
Copy link
Member

jkotas commented Aug 4, 2018

I do not think this should be on MemoryMarshal. This should be on a new one, like CollectionsMarshal.

This should not be an extension method. No other methods on *Marshal are extension methods to make you do more typing.

The PowerCollections in corefxlab would be a good place to experiment with this: dotnet/corefxlab#2406

@benaadams benaadams changed the title [Api Proposal] Add List<T>.AsSpan to MemoryMarshal [Api Proposal] Add List<T>.AsSpan to CollectionsMarshal Aug 4, 2018
@benaadams
Copy link
Member Author

benaadams commented Aug 4, 2018

Updated

The PowerCollections in corefxlab would be a good place to experiment with this

Would then need to go via private reflection?

@danmoseley
Copy link
Member

@jkotas is reflection what you have in mind?

@benaadams curious, do you happen to have a motivating scenario?

@benaadams
Copy link
Member Author

curious, do you happen to have a motivating scenario?

List<T> is a very convenient datastructure for creating an array of an unknown size. However, to use it as an array you need to copy the entire contents again with .ToArray().

AsSpan/AsMemory would give the "array" correctly bounded (size of List rather than size of backing array); without the copy.

@jkotas
Copy link
Member

jkotas commented Aug 15, 2018

@jkotas is reflection what you have in mind?

I had in mind all the different list and array builders in "Common". They are all variants of List with different performance characteristics.

@benaadams
Copy link
Member Author

All internal though 😢

@morganbr
Copy link
Contributor

Would it make sense to propose a "ListBuilder"/"SpanBuilder" type then? It could have roughly the same surface area as List, along with an AsSpan/AsMemory method. Once those are called, mutation is no longer allowed.

@danmoseley
Copy link
Member

@ahsonkhan

@danmoseley
Copy link
Member

@safern this does not seem to me essential in order to ship 3.0. Please correct me if I'm wrong.

@Gnbrkm41
Copy link
Contributor

Gnbrkm41 commented Aug 2, 2019

any updates, just wondering? Sounds like a great API 😃

@terrajobst
Copy link
Member

@benaadams We understand that this a logical extension of the current implementation of the type. Could you list some compelling scenarios? We can always come up with potential scenarios.

@benaadams
Copy link
Member Author

Creating a Memory/Span (Array-like) of an unknown upfront size. List<T> is currently the main builder type; but getting an array from it .ToArray() causes a secondary allocation.

The alternative would be to expose an alternative builder type https://github.com/dotnet/corefx/issues/31597#issuecomment-413059736

@TylerBrinkley
Copy link
Contributor

FYI, it's currently being discussed here.

@Gnbrkm41
Copy link
Contributor

I missed this when I had a list and wanted to do something (been a while... can't remember what the exact situation was) which could be vectorised (since you need to take pointer of the underlying array). It didn't have to be exposed as a Span... but just my thought.

@benaadams benaadams changed the title [Api Proposal] Add List<T>.AsSpan to CollectionsMarshal [Api Proposal] Add List<T> AsSpan to CollectionsMarshal Aug 31, 2019
@terrajobst
Copy link
Member

terrajobst commented Sep 24, 2019

Video

Approved shape:

namespace System.Runtime.InteropServices
{
    public partial static class CollectionsMarshal
    {
        public static Span<T> AsSpan(List<T> list);
        public static ReadOnlySpan<T> AsReadOnlySpan(List<T> list);

        // We don't want this one:
        // public Memory<T> AsMemory(List<T> list);
    }
}

@ahsonkhan
Copy link
Member

Re-opening until API is exposed from the ref assembly.

@ahsonkhan ahsonkhan reopened this Sep 25, 2019
@eerhardt
Copy link
Member

The “approved shape” wasn’t completely implemented with dotnet/coreclr#26867. Should we update the “approved shape” above so it is in sync with the code?

@ahsonkhan
Copy link
Member

ahsonkhan commented Sep 26, 2019

Re-flagging as api-ready-for-review to address the AsReadOnlySpan change and also do a quick review on the nullable annotations change (see dotnet/coreclr#26903).

namespace System.Runtime.InteropServices
{
    public partial static class CollectionsMarshal
    {
        // Updated with nullable annotation
        public static Span<T> AsSpan(List<T>? list);
   
        // Since the AsSpan(...) overload is no longer changing the internal version,
        // we don't need this one anymore
        // public static ReadOnlySpan<T> AsReadOnlySpan(List<T> list);

        // We don't want this one:
        // public Memory<T> AsMemory(List<T> list);
    }
}

@terrajobst
Copy link
Member

terrajobst commented Oct 1, 2019

Video

We decided to not increment the version, which is why we removed the AsReadOnlySpan() overload.

namespace System.Runtime.InteropServices
{
    public partial static class CollectionsMarshal
    {
        public static Span<T> AsSpan(List<T>? list);
    }
}

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants