Runtime: API Proposal: Span<T> AsReadOnlySpan(this T[]) extension method

[jkotas]

dotnet/runtime#20736 added extension methods that make it easy to create Span from array using fluent pattern. This proposal adds similar methods for ReadOnlySpan.

c# public static class SpanExtensions { public static ReadOnlySpan<T> AsReadOnlySpan<T>(this T[] array); public static ReadOnlySpan<T> AsReadOnlySpan<T>(this ArraySegment<T> segment); }

Note that creating ReadOnlySpan directly using these APIs is not equivalent to creating Span and than casting it to ReadOnlySpan. The later incurs covariance array check that adds unnecessary performance overhead and throws for covariant array. An instance of this problem was fixed in dotnet/corefx#18419.

[jkotas]

cc @KrzysztofCwalina @shiftylogic @ahsonkhan

[ahsonkhan]

The later incurs covariance array check that adds unnecessary performance overhead

I am curious to know how big the overhead cost is. Regardless, I agree with the proposal.

[ahsonkhan]

@jkotas, when do we change from api-needs-work to api-ready-for-review?

[jkotas]

Typically, the area owner (it includes you for area-System.Memory) flips it when he/she believes that the proposal is actionable. Check https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md.

[benaadams]

I am curious to know how big the overhead cost is.

Looks a bit weird though

var segment = new ArraySegment<int>(a, 1, 2);
var roSpan = (ReadOnlySpan<int>)segment.AsSpan();

Much better as

var segment = new ArraySegment<int>(a, 1, 2);
var roSpan = segment.AsReadOnlySpan();

[benaadams]

covariance 😢

[ahsonkhan]

Should the "ReadOnly" be a suffix on AsSpan instead (AsSpanReadOnly)? I realize that the type is called ReadOnlySpan, but I am just thinking if both AsSpan and AsReadOnlySpan should appear next to each other in IntelliSense.

[jkotas]

Both AsReadOnlySpan and AsSpanReadOnly will show up in IntelliSense very close to each other.

[terrajobst]

The later incurs covariance array check that adds unnecessary performance overhead and throws for covariant array.

Makes perfect sense.

[ahsonkhan]

The later incurs covariance array check that adds unnecessary performance overhead.

I did not notice any significant performance overhead from constructing a Span<T> constructor vs ReadOnlySpan<T> constructor. Granted, I testing for T = byte, int, and string only.

@jkotas, how can I measure this covariance array check performance overhead? What type of Span/ReadOnlySpan would I have to construct?

[jkotas]

static int DoStuff(object[] a)
{
    int counter = 0;
    for (int i = 0; i < a.Length; i++)
        counter += (new Span<object>(a, i, 1)[0] != null) ? 1 : -2;
    return counter;
}

static void Main()
{
    object[] a = new object[1000];
    for (int i = 0; i < 1000000; i++)    
        DoStuff(a);
}

If you change Span to ReadOnlySpan that does not have the covariance check, you should see it run ~9x faster.

[jswolf19]

Is this something a first-time contributor could start with?

[jkotas]

@jswolf19 Yes, this is good one for a first-time contributor.

cc @karelz

[jswolf19]

I'd be happy to take it on, then. I'm guessing the AsSpan functions/tests are a good template to work from?

[jkotas]

I'd be happy to take it on, then.

The issue is yours. Thanks!

I'm guessing the AsSpan functions/tests are a good template to work from?

Yes.

[karelz]

@jswolf19 welcome on board! I sent you collaborator invite, please ping me here when you accept - we can then assign the issue to you (GitHub limitation). Assigning to myself in the meantime.

Also check our contributor guide docs (under construction) - https://github.com/dotnet/corefx/wiki/New-contributor-Docs#contributing-guide ... feel free to improve them / ask questions in our gitter room (one is dedicated to the new-contributor docs / support).
Thanks!

[jswolf19]

@karelz I accepted. Looking forward to working with you all ^^

[jswolf19]

I've started work and noticed the API public static ReadOnlySpan<char> AsSpan(this string text).
I wonder if this naming might become confusing with the addition of the AsReadOnlySpan APIs.

[ahsonkhan]

I've started work and noticed the API public static ReadOnlySpan<char> AsSpan(this string text).
I wonder if this naming might become confusing with the addition of the AsReadOnlySpan APIs.

Good point. One option could be to rename AsSpan(this string text) to AsReadOnlySpan.

However, since the proposed APIs are extension methods on specific types, if you have a string, you won't see any AsReadOnlySpan extension method for it in IntelliSense, so it may not necessarily cause confusion.

With the additions from this API proposal, we get:
C# public static ReadOnlySpan<char> AsSpan(this string text); public static Span<T> AsSpan<T>(this T[] array); public static Span<T> AsSpan<T>(this ArraySegment<T> arraySegment); public static ReadOnlySpan<T> AsReadOnlySpan<T>(this T[] array); public static ReadOnlySpan<T> AsReadOnlySpan<T>(this ArraySegment<T> segment);

byte[] byteArray;
byteArray. [Options are AsSpan and AsReadOnlySpan]

string str;
str. [Only AsSpan is visible, which will return ReadOnlySpan<char> since string is immutable]

[jswolf19]

I agree from a discoverability standpoint that AsSpan is the more intuitive name, as making the mental leap from string → immutable → ReadOnlySpan instead of Span while likely not hard for someone who would likely be using Span is not immediate in my mind.

From a readability standpoint, though I imagine
var span = str.AsReadOnlySpan();
might be preferable to
var span = str.AsSpan();
for mentally resolving type, especially in a context like source control where IntelliSense may not be available.
However, I also imagine that, for many use cases, differentiating Span from ReadOnlySpan may not be necessary (except of course when it is ^^). Just thought I'd bring it up.

[ektrah]

+1 for changing public static ReadOnlySpan<char> AsSpan(this string text); to public static ReadOnlySpan<char> AsReadOnlySpan(this string text); if public static ReadOnlySpan<T> AsReadOnlySpan<T>(this T[] array); is added. It's confusing if AsSpan on strings is inconsistent with every other use of AsSpan and if there is a AsReadOnlySpan method which can be used in every case except for strings.

[benaadams]

If the string extension method was to change

ReadOnlySpan<char> AsSpan(this string text) -> ReadOnlySpan<char> AsReadOnlySpan(this string text)

At a guess it would need to change before Escrow? Else it couldn't change due to back compat?

Can an expedited decision be made? /cc @jkotas @karelz @terrajobst

[jkotas]

At a guess it would need to change before Escrow? Else it couldn't change due to back compat?

System.Memory is not shipping a stable package for 2.0.

[benaadams]

K, raised as separate issue https://github.com/dotnet/corefx/issues/20242