Corefxlab: [Pre-release] Productize System.Span and ReadOnlySpan

[ahsonkhan]

Pri 0 - _Must complete for release_

• [x] Finalize API shape of SpanExtensionsLab and merge with SpanExtensions - #1292
• [x] Add tests to increase code coverage - https://github.com/dotnet/corefx/issues/17078
• [ ] Add stress tests - #1313 / https://github.com/dotnet/corefx/issues/28227
• [x] Remove System.Slices - #1295
• [x] Consider changing CopyTo methods to copy as much as possible and return int - https://github.com/dotnet/corefx/issues/16888
• [x] Should we rename Span to Memory and Memory to something else - https://github.com/dotnet/corefx/issues/16997
• [x] Improve Span.StartsWith(ReadOnlySpan|ReadOnlySpan) performance + investigate API that takes index - #1294
• [x] Span/ReadonlySpan TryCopy/CopyTo are very slow (slow span) - https://github.com/dotnet/corefx/issues/16840
• [x] System.Memory.Tests crashing on Windows outer loop runs - https://github.com/dotnet/corefx/issues/16492
• [x] Fix array variance checks in slow Span - https://github.com/dotnet/corefx/issues/14533
• [x] Remove System.Buffers.Experimental dependency from Pipelines.

Pri 1 - _Nice to have_

• [x] Debugger proxies don't work on Spans - https://github.com/dotnet/corefx/issues/14376
• [x] Test: System.SpanTests.ReadOnlySpanTests/IndexOverflow failed with "System.OutOfMemoryException" - https://github.com/dotnet/corefx/issues/16593
• [x] Span: DangerousCreate should avoid argument validation - https://github.com/dotnet/corefx/issues/14486

Pri 2 - _Nice to have_

• [x] Fix Spanbench benchmark tests for fast span to be more reliable - https://github.com/dotnet/coreclr/issues/10099
• [ ] Add slow span benchmarks to benchview - https://github.com/dotnet/coreclr/issues/10098
• [x] Add IndexOfAny overload - #1293

---

Future - _Not part of this release_
~- [ ] Add a way to efficiently compare memory - https://github.com/dotnet/corefx/issues/16878~
~- [ ] Span == null throws ArgumentNullException - https://github.com/dotnet/corefx/issues/16730~
~- [ ] Span: Add BinarySearch(...) extension methods for ReadOnlySpan (and Span) - https://github.com/dotnet/corefx/issues/15818~
~- [ ] Proposal: Add Sort(...) extension methods for Span - https://github.com/dotnet/corefx/issues/15329~
~- [ ] Slicing pattern (range) - #1306~

~- [ ] Finish design for Memory and friends - #1296~ This issue tracks System.Memory.dll APIs only
~- [ ] Design BytesReader - #1297~ This issue tracks System.Memory.dll APIs only
~- [ ] Implement Memory final design - #1298~ This issue tracks System.Memory.dll APIs only

[ahsonkhan]

Issue tracking the work items for Span
Feel free to add/tweak work items or adjust priorities.

cc @KrzysztofCwalina, @shiftylogic, @jkotas, @davidfowl, @joshfree

[jkotas]

dotnet/corefx#14533 should be P0. It is a real bug, not just a test issue.

[KrzysztofCwalina]

dotnet/corefx#14376 should be pri 1. I would do it rather than dotnet/corefx#14486. Debugging span code is so painful without this fix. I moved it.

[KrzysztofCwalina]

Also, some of the Future items are probably not actually future, e.g. #1298.

But, I would just move the Memory (and friends) issues to a separate location. This issue should track System.Memory.dll APIs only.

[joshfree]

Thanks for organizing this @ahsonkhan.

[ahsonkhan]

But, I would just move the Memory (and friends) issues to a separate location. This issue should track System.Memory.dll APIs only.

Updated.

[benaadams]

Should Span Fill & Clear be moved to extension methods in corefx from operations on Span in coreclr?

They don't particularly need access to anything only internal to the struct and then could be specialized with Vectors if that was better.

[ahsonkhan]

then could be specialized with Vectors if that was better.

@benaadams, can you expand on that? Why would Fill/Clear being extension method allow for use of vectors?

[benaadams]

Why would Fill/Clear being extension method allow for use of vectors?

They could be in corefx and you'd have multiple methods much like IndexOf so

public static void Fill(this Span<T> span, T value)
{
    // General implementation
}

public static void Fill(this Span<byte> span, byte value)
{
    // add handling for non-vector lengths
    var vector = new Vector<byte>(value);
    ref var dest = ref span.DangerousGetPinnableReference();
    var length = span.Length;

    for (var offset = 0; offset < length; offset += Vector<byte>.Count)
    {
        Unsafe.As<byte, Vector<byte>>(ref Unsafe.Add(ref dest, offset)) = vector;
    }
}

Clear() being .Fill(0)

[ahsonkhan]

They could be in corefx and you'd have multiple methods much like IndexOf so

They are in corefx though: https://github.com/dotnet/corefx/blob/master/src/System.Memory/src/System/Span.cs#L234

Does Fill have to be an extension method for it to have byte/other overloads?

Also, isn't this always going to be faster than vectorization for the T=byte case?
https://github.com/dotnet/corefx/blob/master/src/System.Memory/src/System/Span.cs#L241-L253

[benaadams]

They are instance methods on the generic type; so both in coreclr and corefx for fast and slow span respectively. Instance methods always take precedence over extension methods.

Also wouldn't make sense to have Fill(byte|char|int|double value) methods on a T type that could contain references.

[nietras]

Does Fill have to be an extension method for it to have byte/other overloads?

The type specific "optimizations" do not have to be made as overloads, but could be simple if (typeof(T) == typeof(char)) etc. The pattern of using overloads seems unnecessary to me, since the JIT can elide these checks. Why have overloads when (at least some of) these are more of an implementation/performance detail than an API thing?

Anyway, I think @benaadams has a valid point that making Clear/Fill be extension methods opens these up for more involved code using more of the available API surface in .NET such as Vector<T>, it also means, I think, we only need to have single implementation of these and not one for both fast and slow Span.

On the other hand there are some drawbacks too. Fast Span member methods can divert to native code if needed etc. Slow Span can use knowledge of memory type managed, pinned/native and so forth.

I had the same thoughts when proposing Clear (https://github.com/dotnet/corefx/issues/13915) / Fill (https://github.com/dotnet/corefx/issues/14189) but thought member methods were a more direct way and had the benefit of being much easier to find e.g. F12 on type. Compared to the less than ideal current static methods for Array.Clear and similar. Given that extension methods are much easier to discover and use in VS these points seem less important and perhaps the other points about code reuse and possible perf improvements more important.

[benaadams]

The type specific "optimizations" do not have to be made as overloads, but could be simple if (typeof(T) == typeof(char)) etc. The pattern of using overloads seems unnecessary to me, since the JIT can elide these checks. Why have overloads when (at least some of) these are more of an implementation/performance detail than an API thing?

Because they are baked into the generic type. For Vector it makes sense as its only a Vector of what the JIT supports.

However it allows no scope for user defined overloads for their specific type; e.g.

public static void Fill(this Span<MyType> span, MyType value)

[ahsonkhan]

@benadams, regarding a comment you made in another thread related to this:

Extensions for the methods like clear allow specialization; so a Clear method could be defined differently. Whether its to use vectors; or if for a custom user struct all the values should be set to -1 rather than zero it would allow that;

Are these statements true:

• If we have Fill instance method on Span<T>, custom Span<MyType>.Fill extension method will not get called.
• If we have Fill extension method on Span<T>, custom Span<MyType>.Fill extension method will get called.

[benaadams]

(edit) had it wrong before; didn't make method public

Yes those statements are true

class Program
{
    static void Main(string[] args)
    {
        var value = new MyType<byte>();

        value.Output();
    }
}

public class MyType<T>
{
    public void Output()
    {
        Console.WriteLine("Generic instance method called");
    }
}

public static class MyTypeExtensions
{
    public static void Output(this MyType<byte> myType)
    {
        Console.WriteLine("MyType<byte> Extension method called");
    }
}

Outputs

Generic instance method called

[benaadams]

So it only makes sense to be an instance method over extension method if it gets a benefit from having internal access; which these methods don't need since they use the public method .DangerousGetPinnableReference()

[KrzysztofCwalina]

@ahsonkhan, we have a similar issue in corefx, don't we? If yes, can you move undone items to corefx and close this issue?
Thanks.

[KrzysztofCwalina]

@ahsonkhan, I am closing this one. I assume all the items are done or moved to corefx.