Forward Span-based MemoryExtensions overloads to ReadOnlySpan overloads

[stephentoub]

Any reason not to do this? I'm not sure why it was done this way in the first place... seems like there must have been a reason?
cc: @GrabYourPitchforks

[ghost]

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar, @tannergooding, @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Any reason not to do this? I'm not sure why it was done this way in the first place... seems like there must have been a reason?
cc: @GrabYourPitchforks

Author:	stephentoub
Assignees:	-
Labels:	size-reduction
Milestone:	6.0.0

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Any reason not to do this? I'm not sure why it was done this way in the first place... seems like there must have been a reason?
cc: @GrabYourPitchforks

Author:	stephentoub
Assignees:	-
Labels:	area-System.Memory, size-reduction
Milestone:	6.0.0

[GrabYourPitchforks]

I want to say it was a limitation of the JIT, where passing around complex structs (Span<T> or ReadOnlySpan<T>) across ABI was resulting in some weird unoptimized codegen. But not sure if this is still the case. Will run a quick experiment.

[GrabYourPitchforks]

Both these methods produce identical codegen in .NET 6.

using BenchmarkDotNet.Attributes;
using System;
using System.Runtime.CompilerServices;

namespace ConsoleAppBenchmark
{
    [DisassemblyDiagnoser]
    public class SpanFwdRunner
    {
        private readonly int[] _ints = new int[] { 1, 2, 3 };

        [Benchmark(Baseline = true)]
        [Arguments(2)]
        public bool Control(int value)
        {
            Span<int> span = _ints;
            return span.Contains(value);
        }

        [Benchmark(Baseline = false)]
        [Arguments(2)]
        public bool Variable(int value)
        {
            Span<int> span = _ints;
            return span.ContainsEx(value);
        }
    }

    internal static class Extensions
    {
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static bool ContainsEx<T>(this Span<T> span, T value) where T : IEquatable<T>
        {
            return ((ReadOnlySpan<T>)span).Contains(value);
        }
    }
}

@AndyAyersMS Do you know if my theory about a previous JIT not optimizing this scenario correctly is viable? If something changed between earlier versions of .NET and vCurrent that means we can simplify this code, I'm all for Steve's proposed changes.

[GrabYourPitchforks]

Let's wait for the JIT team to chime in, but LGTM!

[SingleAccretion]

One thing this change could risk is pushing some methods over the "512 max tracked locals" limit. I wrote a sample that demonstrates this: asm diff. While the codegen is the same (yay!), the number of locals used is considerably higher with the cast.

C# code

private static bool ContainsSpan(int[] source, int a, int b, int c)
{
    if (source.AsSpan().Contains(a))
    {
        return true;
    }
    if (source.AsSpan().Contains(b))
    {
        return true;
    }
    if (source.AsSpan().Contains(c))
    {
        return true;
    }

    return false;
}

private static bool ContainsReadOnlySpan(int[] source, int a, int b, int c)
{
    if (((ReadOnlySpan<int>)source.AsSpan()).Contains(a))
    {
        return true;
    }
    if (((ReadOnlySpan<int>)source.AsSpan()).Contains(b))
    {
        return true;
    }
    if (((ReadOnlySpan<int>)source.AsSpan()).Contains(c))
    {
        return true;
    }

    return false;
}

We have tooling that compares ASM between different Jit's. It could be interesting to leverage that for diffing the CoreLib (and/or the shared framework too) with & without this change. Unfortunately I do not have the time for doing it right now, but a note if someone else is interested: do not forget to eliminate Debug.Fails from the equation (Checked CoreLib is built with DEBUG defined).

[AndyAyersMS]

Not sure.

Perhaps on it was x64/SysV ABI, or arm64, where we might have been spilling the spans to stack and back? Or local copies of pass through structs blocking tail calls?

Much of that should be mitigated by now.

[jkotas]

The extra forwarding layer will also regress the case where these methods are used over reference types (the generics inlining problem).

[jkotas]

One thing this change could risk is pushing some methods over the "512 max tracked locals" limit.

Yep, the JIT budgets tend to get consumed very fast for code that works with Spans due to extensive use of aggressive inlining, so calling method may not actually be that large to hit this limit.

[stephentoub]

I'm not sure how to evaluate the tradeoffs here. This is a fair amount of non-trivial code duplication; just from a maintenance perspective I'd like to get rid of it, but then also arguably unnecessary binary size (e.g. a default Blazor wasm app currently uses both the Span and ReadOnlySpan overloads of IndexOf(span, T) and StartsWith(span, ROS<T>). Maybe it's a drop in the bucket, but it's a shame that limitations elsewhere in the system force this to even be a performance tradeoff.

@jkotas, @AndyAyersMS, would you prefer to see this closed then?

[jkotas]

int IndexOfAny<T>(this Span<T> span, ReadOnlySpan<T> values) has the largest amount of duplication from all the methods touched in this PR. I think it is reasonable to eliminate the duplication for this method, but I would keep the duplication for the rest.

[AndyAyersMS]

"512 max tracked locals" limit.

We may want to revaluate this limit. Last time we looked (~5 years ago) there were very few methods that came near. But perhaps things have changed.

the generics inlining problem

I know David was poking around here a while back -- at some point we should try and fix this.

[saucecontrol]

Last time we looked (~5 years ago) there were very few methods that came near. But perhaps things have changed.

Don't forget the examples in #11905 ;)