Index and Range updates

[tarekgh]

This is the change reflect what has been decided in the design meetings. you can look at the notes here https://github.com/dotnet/apireviews/tree/master/2019/System.Index
Also the change reflect the list of the types uses Index and Range according to the spec https://github.com/dotnet/designs/blob/master/accepted/system-range/system-range.md#companion-apis

[tarekgh]

CC @jkotas @stephentoub @agocke @jaredpar @terrajobst @joshfree @AndyAyersMS

[tarekgh]

I'll submit the corefx PR part soon

[tarekgh]

@AndyAyersMS could you please look at these change specifically the one in Spans? I have some changes in the span indexer which take Range. Thanks.

[AndyAyersMS]

Left you some suggestions about re-expressing some of the C# to help the jit.

Any time you access a Range's Start or End more than once you should copy it to a local Index and refer to that instead. The jit can't do this for you today -- though maybe soon with #22255?

Even with carefully crafted C# there will still be perf issues with Range because it ends up being passed by value on x64 and its fields can't be promoted/enregistered by the jit. Details in #22079.

And perf issues with even locally constructed and obviously from-start or from-end Indexes because the JIT doesn't see the NOT (~) operator very often. See #22130.

[jkotas]

For covariant arrays, this implementation can throw ArrayTypeMismatchException and/or return array of a different type that what it started with. Is this what we want?

[tarekgh]

No, looks this is wrong implementation then. what should I use to ensure returning the exact array type?

[stephentoub]

Maybe something like:

Type elementType = array.GetType().GetElementType();
Span<T> source = array.AsSpan(range);

if (elementType.IsValueType)
{
    return source.ToArray();
}
else
{
    T[] newArray = (T[])Array.CreateInstance(elementType, source.Length);
    source.CopyTo(newArray);
    return newArray;
}

I realize I've already shared my opinion on this, but I'll just note here again how I think it's unfortunate we're exposing this. Such a perf trap.

int[] array = …;
Span<int> span = array[1..3];

is going to invoke all that above goo. It'll be even worse for all those folks that love using var everywhere:

var data = GetData();
Span<int> span = data[1..3];

Fast or slow? Who knows.

[jkotas]

I realize I've already shared my opinion on this, but I'll just note here again how I think it's unfortunate we're exposing this

+100

I believe that we would get much better feedback about the design if we did not add these API first and wait for feedback on why there are actually needed and what they are going to use for.

Mentioned it in dotnet/designs#48 (comment) , but got no response.

[tarekgh]

I believe I have addressed all feedback here, anyone has more comments?

[tarekgh]

@stephentoub @jkotas do you have any more feedback? or you are ok to merge?

[jkotas]

If we are going to add the super-slow indexers, I would like to see a plan for how we are going to collect feedback about their usability.

#22331 (comment)

[tarekgh]

If we are going to add the super-slow indexers, I would like to see a plan for how we are going to collect feedback about their usability.

I follow up with that with different parties. I want to ask if your request should block this PR?

[terrajobst]

If we are going to add the super-slow indexers, I would like to see a plan for how we are going to collect feedback about their usability.

My intention was to blog about the BCL feature in the preview 3 timeline, calling out the overall design points with an explicit ask for feedback with the intent to break the API post preview 3 if that's what's needed to address the feedback.

[baulig]

Do you guys plan on also updating the version in corefx (https://github.com/dotnet/corefx/blob/master/src/Common/src/CoreLib/System/Index.cs)?

I'm currently porting the Index and Range types to Mono and am unsure which version of them I should use.

[tarekgh]

yes we have the PR dotnet/corefx#35003 but just waiting the PR porting coreclr get opened.