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

Tracking: Revisit APIs to use Span<T> #19444

Closed
karelz opened this issue Nov 22, 2016 · 15 comments
Closed

Tracking: Revisit APIs to use Span<T> #19444

karelz opened this issue Nov 22, 2016 · 15 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-Meta blocked Issue/PR is blocked on something - see comments
Milestone

Comments

@karelz
Copy link
Member

karelz commented Nov 22, 2016

We just need to wait for Span<T> to mature a bit.

@tarekgh
Copy link
Member

tarekgh commented Nov 22, 2016

@karelz why this not under System.Runtime area?

@karelz
Copy link
Member Author

karelz commented Nov 22, 2016

I expected it to be cross-cutting. It will also grow. I don't have strong preference between Meta vs. Runtime in this case.

@jamesqo
Copy link
Contributor

jamesqo commented Nov 22, 2016

@karelz If you have int.Parse, then you need to add Parse apis for all the other primitive types too, plus some others. e.g. sbyte.Parse byte.Parse short.Parse ... ulong.Parse, DateTime.Parse, Guid.Parse, etc.

@jamesqo
Copy link
Contributor

jamesqo commented Nov 23, 2016

@karelz, I think it may also be valuable offering an API for converting an ImmutableArray<T> to a ReadOnlySpan<T> via an implicit conversion, e.g.:

public struct ImmutableArray<T>
{
    public static implicit operator ReadOnlySpan<T>(ImmutableArray<T> array);
}

ImmutableArray<T> as it is it kind of hard to work with, because most of the framework methods accept plain arrays. That leads to scenarios like this, where to write an ImmutableArray to a Stream, instead of calling Write directly, you have to loop through each byte of it & call WriteByte (super slow), or call ToArray and make a copy (allocates). It seems like it would be beneficial to convert an ImmutableArray to a ReadOnlySpan as more & more methods start accepting spans.

cc @AArnott @KrzysztofCwalina

@karelz
Copy link
Member Author

karelz commented Nov 23, 2016

Updated above ...

@AArnott
Copy link
Contributor

AArnott commented Nov 25, 2016

I wonder when the right time to make ImmutableArray<T> depend on the new API. Adding such a dependency will limit who can upgrade to the latest System.Collections.Immutables. Which versions of .NET Framework define ReadOnlySpan<T>?

@jamesqo
Copy link
Contributor

jamesqo commented Nov 25, 2016

cc: @jkotas @terrajobst

@jkotas
Copy link
Member

jkotas commented Nov 25, 2016

Which versions of .NET Framework define ReadOnlySpan<T>?

System.Memory for .NET Framework will be separate nuget package for foreseeable future.

ImmutableArray<T> as it is it kind of hard to work with, because most of the framework methods accept plain arrays.

Right. The current solution to this problem is this hack: https://github.com/dotnet/corefx/blob/master/src/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/ImmutableByteArrayInterop.cs#L83 that is getting replicated around. There was a discussion about adding official method to get to the underlying storage to avoid this hack, but the purist view of not having such method in the public surface prevailed. I believe this discussion was before open source, so it is not on GitHub. (disclaimer: I was for adding the method).

ReadOnlySpan<T> is going to have a similar method to get unprotected access to the underlying storage for interop. It is called DangerousGetPinnableReference currently, but this name may not be final. So once there is a public method to convert from ImmutableArray<T> to ReadOnlySpan<T>, there will be a way to get to the unprotected access to the underlying storage via public APIs and the purist argument will have much less merit. Thus, we can as well revisit adding the method to get to the underlying array for ImmutableArray<T>. It will address the current ImmutableArray<T> interop issue, without hacks and Span<T> dependency.

@jamesqo
Copy link
Contributor

jamesqo commented Nov 25, 2016

@jkotas Thanks for the synopsis. Very interesting to know the background on all of this.

@geoffkizer
Copy link
Contributor

Stream.Read/Write[Async]
Socket.Send/Receive[Async]

@jamesqo
Copy link
Contributor

jamesqo commented Mar 20, 2017

Did we also forget about the IO APIs? https://github.com/dotnet/corefx/issues/6740#issuecomment-253602220

@karelz
Copy link
Member Author

karelz commented Mar 20, 2017

Top post updated with both @geoffkizer's and @jamesqo's links.

@Drawaes
Copy link
Contributor

Drawaes commented Apr 20, 2017

What about Crypto API's?
dotnet/corefx#18693

@karelz
Copy link
Member Author

karelz commented Apr 21, 2017

Top post updated, thanks @Drawaes!

@stephentoub
Copy link
Member

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-Meta blocked Issue/PR is blocked on something - see comments
Projects
None yet
Development

No branches or pull requests

9 participants