2D System.Memory-like primitives (Span2D<T>, Memory2D<T>) #3353

Sergio0694 · 2020-06-17T14:57:46Z

"If I have a 3D array, I want to take one of the slices of the 2D array out of it as a reference...?"

And this is the answer 🚀

PR Type

What kind of change does this PR introduce?

Feature

What is the current behavior?

There is no proper way to pass around a 2D memory area in a format that's easy to handle and somewhat similar to how the other System.Memory APIs work (ie. Span<T>, Memory<T>). This is regardless of whether or not the wrapped memory region is contiguous in memory or not.

What is the new behavior?

This PR adds a number of new types and extensions to solve this issue, specifically:

Span2D<T>
ReadOnlySpan2D<T>
Memory2D<T>
ReadOnlyMemory2D<T>

And a series of new extensions for 3D arrays as well.
Note that all these 2D types can be used with arrays of any type, eg. they can both be used to wrap a 2D memory region from a 2D array, a 2D slice for a 3D array, or even just to represent a 1D array as a 2D memory region to make it easier to properly access elements by their spatial coordinates.

NOTE: as mentioned above, the wrapped 2D area does not need to be contiguous! See comments in Span2D<T> to see how this is handled. Having this feature gives users much more freedom and flexibility when working with arbitrary arrays (or unmanaged memory too).

API surface (summary)

namespace Microsoft.Toolkit.HighPerformance.Enumerables
{
    public readonly ref struct RefEnumerable<T>
    {
        public readonly Enumerator GetEnumerator();
        public readonly void Clear();
        public readonly void CopyTo(RefEnumerable<T> destination);
        public readonly bool TryCopyTo(RefEnumerable<T> destination);
        public readonly void CopyTo(Span<T> destination);
        public readonly bool TryCopyTo(Span<T> destination);
        public readonly void Fill(T value);
        public readonly T[] ToArray();

        public ref struct Enumerator
        {
            public readonly ref T Current { get; }

            public bool MoveNext();
        }
    }

    public readonly ref struct ReadOnlyRefEnumerable<T>
    {
        public readonly Enumerator GetEnumerator();
        public readonly void CopyTo(RefEnumerable<T> destination);
        public readonly bool TryCopyTo(RefEnumerable<T> destination);
        public readonly void CopyTo(Span<T> destination);
        public readonly bool TryCopyTo(Span<T> destination);
        public readonly T[] ToArray();

        public static implicit operator ReadOnlyRefEnumerable<T>(RefEnumerable<T> enumerable);

        public ref struct Enumerator
        {
            public readonly ref readonly T Current { get; }

            public bool MoveNext();
        }
    }
}

namespace Microsoft.Toolkit.HighPerformance.Extensions
{
    public static class ArrayExtensions
    {
        public static bool IsCovariant<T>(this T[] array);

        public static RefEnumerable<T> GetRow<T>(this T[,] array, int row);
        public static RefEnumerable<T> GetColumn<T>(this T[,] array, int column);
        public static Span<T> GetRowSpan<T>(this T[,] array, int row);
        public static Memory<T> GetRowMemory<T>(this T[,] array, int row);
        public static Memory<T> AsMemory<T>(this T[,]? array);
        public static Span2D<T> AsSpan2D<T>(this T[,]? array);
        public static Span2D<T> AsSpan2D<T>(this T[,]? array, int row, int column, int height, int width);
        public static Memory2D<T> AsMemory2D<T>(this T[,]? array);
        public static Memory2D<T> AsMemory2D<T>(this T[,]? array, int row, int column, int height, int width);
        public static bool IsCovariant<T>(this T[,] array);

        public static ref T DangerousGetReference<T>(this T[,,] array);
        public static ref T DangerousGetReferenceAt<T>(this T[,,] array, int i, int j, int k);
        public static Span<T> AsSpan<T>(this T[,,]? array);
        public static Memory<T> AsMemory<T>(this T[,,]? array);
        public static Span<T> AsSpan<T>(this T[,,] array, int depth);
        public static Memory<T> AsMemory<T>(this T[,,] array, int depth);
        public static Span2D<T> AsSpan2D<T>(this T[,,] array, int depth);
        public static Memory2D<T> AsMemory2D<T>(this T[,,] array, int depth);
        public static int Count<T>(this T[,,] array, T value) where T : IEquatable<T>;
        public static int GetDjb2HashCode<T>(this T[,,] array);
        public static bool IsCovariant<T>(this T[,,] array);
    }

    public static class MemoryExtensions
    {
        public static Memory2D<T> AsMemory2D<T>(this Memory<T> memory, int height, int width);
        public static Memory2D<T> AsMemory2D<T>(this Memory<T> memory, int offset, int height, int width, int pitch);
    }

    public static class ReadOnlyMemoryExtensions
    {
        public static ReadOnlyMemory2D<T> AsMemory2D<T>(this ReadOnlyMemory<T> memory, int height, int width);
        public static ReadOnlyMemory2D<T> AsMemory2D<T>(this ReadOnlyMemory<T> memory, int offset, int height, int width, int pitch);
    }

    public static class ReadOnlySpanExtensions
    {
        public static ReadOnlySpan2D<T> AsMemory2D<T>(this ReadOnlySpan<T> span, int height, int width);
        public static ReadOnlySpan2D<T> AsMemory2D<T>(this ReadOnlySpan<T> span, int offset, int height, int width, int pitch);
        public static void CopyTo<T>(this ReadOnlySpan<T> span, RefEnumerable<T> destination);
        public static bool TryCopyTo<T>(this ReadOnlySpan<T> span, RefEnumerable<T> destination);
    }

    public static class SpanExtensions
    {
        public static Span2D<T> AsMemory2D<T>(this Span<T> span, int height, int width);
        public static Span2D<T> AsMemory2D<T>(this Span<T> span, int offset, int height, int width, int pitch);
        public static void CopyTo<T>(this Span<T> span, RefEnumerable<T> destination);
        public static bool TryCopyTo<T>(this Span<T> span, RefEnumerable<T> destination);
    }
}

namespace Microsoft.Toolkit.HighPerformance.Helpers
{
    public static partial class ParallelHelper
    {
        public static void ForEach<TItem, TAction>(Memory2D<TItem> memory) where TAction : struct, IRefAction<TItem>;
        public static void ForEach<TItem, TAction>(Memory2D<TItem> memory, int minimumActionsPerThread) where TAction : struct, IRefAction<TItem>;
        public static void ForEach<TItem, TAction>(Memory2D<TItem> memory, in TAction action) where TAction : struct, IRefAction<TItem>;
        public static void ForEach<TItem, TAction>(Memory2D<TItem> memory, in TAction action, int minimumActionsPerThread) where TAction : struct, IRefAction<TItem>;
        public static void ForEach<TItem, TAction>(ReadOnlyMemory2D<TItem> memory) where TAction : struct, IInAction<TItem>;
        public static void ForEach<TItem, TAction>(ReadOnlyMemory2D<TItem> memory, int minimumActionsPerThread) where TAction : struct, IInAction<TItem>;
        public static void ForEach<TItem, TAction>(ReadOnlyMemory2D<TItem> memory, in TAction action) where TAction : struct, IInAction<TItem>;
        public static void ForEach<TItem, TAction>(ReadOnlyMemory2D<TItem> memory, in TAction action, int minimumActionsPerThread) where TAction : struct, IInAction<TItem>;
    }
}

namespace Microsoft.Toolkit.HighPerformance.Memory
{
    public readonly struct Memory2D<T> : IEquatable<Memory2D<T>>
    {
        public Memory2D(T[] array, int height, int width);
        public Memory2D(T[] array, int offset, int height, int width, int pitch);
        public Memory2D(T[,]? array);
        public Memory2D(T[,]? array, int row, int column, int height, int width);
        public Memory2D(T[,,] array, int depth);
        public Memory2D(T[,,] array, int depth, int row, int column, int height, int width);
        public Memory2D(MemoryManager<T> memoryManager, int height, int width);
        public Memory2D(MemoryManager<T> memoryManager, int offset, int height, int width, int pitch);

        public static Memory2D<T> DangerousCreate(object instance, ref T value, int height, int width, int pitch);

        public static Memory2D<T> Empty { get; }

        public bool IsEmpty { get; }
        public nint Length { get; }
        public int Height { get; }
        public int Width { get; }
        public Span2D<T> Span { get; }
        public Memory2D<T> this[Range rows, Range columns] { get; }

        public Memory2D<T> Slice(int row, int column, int height, int width);
        public void CopyTo(Memory<T> destination);
        public bool TryCopyTo(Memory<T> destination);
        public void CopyTo(Memory2D<T> destination);
        public bool TryCopyTo(Memory2D<T> destination);
        public MemoryHandle Pin();
        public bool TryGetMemory(out Memory<T> memory);
        public T[,] ToArray();
        public override bool Equals(object? obj);
        public bool Equals(Memory2D<T> other);
        public override int GetHashCode();
        public override string ToString();

        public static implicit operator Memory2D<T>(T[,]? array) => new Memory2D<T>(array);
    }

    public readonly struct ReadOnlyMemory2D<T> : IEquatable<ReadOnlyMemory2D<T>>
    {
        public ReadOnlyMemory2D(string text, int height, int width);
        public ReadOnlyMemory2D(string text, int offset, int height, int width, int pitch);
        public ReadOnlyMemory2D(T[] array, int height, int width);
        public ReadOnlyMemory2D(T[] array, int offset, int height, int width, int pitch);
        public ReadOnlyMemory2D(T[,]? array);
        public ReadOnlyMemory2D(T[,]? array, int row, int column, int height, int width);
        public ReadOnlyMemory2D(T[,,] array, int depth);
        public ReadOnlyMemory2D(T[,,] array, int depth, int row, int column, int height, int width);
        public ReadOnlyMemory2D(MemoryManager<T> memoryManager, int height, int width);
        public ReadOnlyMemory2D(MemoryManager<T> memoryManager, int offset, int height, int width, int pitch);

        public static ReadOnlyMemory2D<T> DangerousCreate(object instance, ref T value, int height, int width, int pitch);

        public static ReadOnlyMemory2D<T> Empty  { get; }
        public bool IsEmpty { get; }
        public nint Length { get; }
        public int Height { get; }
        public int Width { get; }
        public ReadOnlySpan2D<T> Span { get; }
        public ReadOnlyMemory2D<T> this[Range rows, Range columns] { get; }

        public ReadOnlyMemory2D<T> Slice(int row, int column, int height, int width);
        public void CopyTo(Memory<T> destination);
        public bool TryCopyTo(Memory<T> destination);
        public void CopyTo(Memory2D<T> destination);
        public bool TryCopyTo(Memory2D<T> destination);
        public MemoryHandle Pin();
        public bool TryGetMemory(out ReadOnlyMemory<T> memory);
        public T[,] ToArray();
        public override bool Equals(object? obj);
        public bool Equals(ReadOnlyMemory2D<T> other);
        public override int GetHashCode();
        public override string ToString();

        public static implicit operator ReadOnlyMemory2D<T>(T[,]? array);
        public static implicit operator ReadOnlyMemory2D<T>(Memory2D<T> memory);
    }

    public readonly ref struct Span2D<T>
    {
        public Span2D(void* pointer, int height, int width, int pitch);
        public Span2D(T[] array, int height, int width);
        public Span2D(T[] array, int offset, int height, int width, int pitch);
        public Span2D(T[,]? array);
        public Span2D(T[,]? array, int row, int column, int height, int width);
        public Span2D(T[,,] array, int depth);
        public Span2D(T[,,] array, int depth, int row, int column, int height, int width);

        public static Span2D<T> DangerousCreate(ref T value, int height, int width, int pitch);

        public static Span2D<T> Empty { get; }
        public bool IsEmpty { get; }
        public nint Length { get; }
        public int Height { get; }
        public int Width { get; }
        public ref T this[int row, int column] { get; }
        public ref T this[Index row, Index column] { get; }
        public Span2D<T> this[Range rows, Range columns] { get; }

        public void Clear();
        public void CopyTo(Span<T> destination);
        public void CopyTo(Span2D<T> destination);
        public bool TryCopyTo(Span<T> destination);
        public bool TryCopyTo(Span2D<T> destination);
        public void Fill(T value);
        public ref T GetPinnableReference();
        public ref T DangerousGetReference();
        public ref T DangerousGetReferenceAt(int i, int j);
        public Span2D<T> Slice(int row, int column, int height, int width);
        public Span<T> GetRowSpan(int row);
        public bool TryGetSpan(out Span<T> span);
        public T[,] ToArray();
        public override bool Equals(object? obj);
        public override int GetHashCode();
        public override string ToString();

        public static bool operator ==(Span2D<T> left, Span2D<T> right);
        public static bool operator !=(Span2D<T> left, Span2D<T> right);
        public static implicit operator Span2D<T>(T[,]? array);

        public RefEnumerable<T> GetRow(int row);
        public RefEnumerable<T> GetColumn(int column);
        public Enumerator GetEnumerator();

        public ref struct Enumerator
        {
            public bool MoveNext();
            public readonly ref T Current { get; }
        }
    }

    public readonly ref struct ReadOnlySpan2D<T>
    {
        public ReadOnlySpan2D(void* pointer, int height, int width, int pitch);
        public ReadOnlySpan2D(T[] array, int height, int width);
        public ReadOnlySpan2D(T[] array, int offset, int height, int width, int pitch);
        public ReadOnlySpan2D(T[,]? array);
        public ReadOnlySpan2D(T[,]? array, int row, int column, int height, int width);
        public ReadOnlySpan2D(T[,,] array, int depth);
        public ReadOnlySpan2D(T[,,] array, int depth, int row, int column, int height, int width);

        public static ReadOnlySpan2D<T> DangerousCreate(in T value, int height, int width, int pitch);

        public static ReadOnlySpan2D<T> Empty { get; }
        public bool IsEmpty { get; }
        public nint Length { get; }
        public int Height { get; }
        public int Width { get; }
        public ref readonly T this[int row, int column] { get; }
        public ref readonly T this[Index row, Index column] { get; }
        public ReadOnlySpan2D<T> this[Range rows, Range columns] { get; }

        public void CopyTo(Span<T> destination);
        public void CopyTo(Span2D<T> destination);
        public bool TryCopyTo(Span<T> destination);
        public bool TryCopyTo(Span2D<T> destination);
        public ref T GetPinnableReference();
        public ref T DangerousGetReference();
        public ref T DangerousGetReferenceAt(int i, int j);
        public ReadOnlySpan2D<T> Slice(int row, int column, int width, int height);
        public ReadOnlySpan<T> GetRowSpan(int row);
        public bool TryGetSpan(out ReadOnlySpan<T> span);
        public T[,] ToArray();
        public override bool Equals(object? obj);
        public override int GetHashCode();
        public override string ToString();

        public static bool operator ==(ReadOnlySpan2D<T> left, ReadOnlySpan2D<T> right);
        public static bool operator !=(ReadOnlySpan2D<T> left, ReadOnlySpan2D<T> right);
        public static implicit operator ReadOnlySpan2D<T>(T[,]? array);
        public static implicit operator ReadOnlySpan2D<T>(Span2D<T> span);

        public ReadOnlyRefEnumerable<T> GetRow(int row);
        public ReadOnlyRefEnumerable<T> GetColumn(int column);
        public Enumerator GetEnumerator();

        public ref struct Enumerator
        {
            public bool MoveNext();
            public readonly ref readonly T Current { get; }
        }
    }
}

These two types are also mirrored as ReadOnlyMemory2D<T> and ReadOnlySpan2D<T>.

PR Checklist

Please check if your PR fulfills the following requirements:

Tested code with current supported SDKs
Pull Request has been submitted to the documentation repository instructions. Link: #376
~~Sample in sample app has been added / updated (for bug fixes / features)~~
- ~~Icon has been created (if new sample) following the Thumbnail Style Guide and templates~~
Tests for the changes have been added (for bug fixes / features) (if applicable)
Header has been added to all new source files (run build/UpdateHeaders.bat)
Contains NO breaking changes

Additional notes

The breaking change is the removal of the Array2DRowEnumerable<T> and Array2DColumnEnumerable<T> types, which were used in the GetRow and GetColumn APIs. This approach had two problems:

There were two completely different iterator types, one per API
The GetRow API was a breaking change when upgrading, as the return type was just Span<T> there.

The solution, as discussed with @john-h-k and @tannergooding, was to introduce a new, shared RefEnumerable<T> type that can be used to traverse both rows and columns, and to use that one for both APIs. Next to them, a new API GetRowSpan was also added only on supported runtimes (so not a breaking change), which is equivalent to GetRow but returning a Span<T> value.

ghost · 2020-06-17T14:57:49Z

Thanks Sergio0694 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

Microsoft.Toolkit.HighPerformance/Memory/Memory2D{T}.cs

Microsoft.Toolkit.HighPerformance/Memory/Span2D{T}.cs

Microsoft.Toolkit.HighPerformance/Enumerables/ReadOnlyRefEnumerable{T}.cs

Microsoft.Toolkit.HighPerformance/Extensions/ArrayExtensions.1D.cs

Microsoft.Toolkit.HighPerformance/Helpers/ParallelHelper.ForEach.IInAction2D.cs

Microsoft.Toolkit.HighPerformance/Helpers/ParallelHelper.ForEach.IRefAction2D.cs

ghost · 2020-06-19T21:41:36Z

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

This reverts commit 78a0266. Done to avoid conflicts, since these two enumerable types are already being overhauled in CommunityToolkit#3353 anyway.

michael-hawker

Going to wait until #3351 gets merged.

Microsoft.Toolkit.HighPerformance/Enumerables/ReadOnlySpanEnumerable{T}.cs

HurricanKai

I looked at most of this code, and while I'm not 100% sure, I think it all looks good to me.
I've got 2 usability issues though:

why are the width/height indexes called i, j? why not x, y?
Why do Equals & GetHashCode just throw?

Microsoft.Toolkit.HighPerformance/Memory/Span2D{T}.cs

Sergio0694 · 2020-09-10T15:47:39Z

Hey @HurricanKai, thanks for the review! 😊

As for your questions:

I called them i and j mostly out of habit, I suppose it would make sense to call them y and x, yes (not x and y though, the first order is the vertical coordinate)
Equals and GetHashCode throw to mirror the behavior of the Span<T> types in the BCL, which do the same (you can see the code in CoreCLR here). Like with those types, we're throwing here and instead implementing the == and != operators. I believe the intent here is that == is just used to indicate whether two memory regions map to the same area, while using GetHashCode and Equals could be confusing since they would not actually check the contents, but just the internal reference. So two different areas with the same contents would not be reported as being the same. Granted, that's the same for eg. arrays, but... I'm really just following their example here to make things consistent with the BCL 😄

michael-hawker · 2020-09-25T20:49:17Z

@Sergio0694 this has some conflicts to resolve now?

Sergio0694 · 2020-09-25T21:20:30Z

@michael-hawker Yup, was just waiting on #3378 to be merged first since that'll edit some of the same files anyway, so that here I'll just have to resolve conflicts once when pulling changes from master from both the two other PRs for this package 😄

michael-hawker · 2020-10-08T20:18:06Z

@Sergio0694 #3378 is merged now, can you resolve conflicts and clean-up this PR so we can re-review and get it merged as well? Thanks! 🙂

michael-hawker

Few small comments on the unit tests, looking good!

michael-hawker · 2020-11-03T17:52:10Z

Microsoft.Toolkit.HighPerformance/Extensions/HashCodeExtensions.cs

@@ -23,12 +23,7 @@ public static class HashCodeExtensions
        /// <param name="span">The input <see cref="ReadOnlySpan{T}"/> instance.</param>
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static void Add<T>(ref this HashCode hashCode, ReadOnlySpan<T> span)
-#if SPAN_RUNTIME_SUPPORT


This has changed now I guess from the runtime side?

I've removed that restriction because I've now added polyfills for the RuntimeHelpers.IsReferenceOrContainsReferences<T>() API on .NET Standard 2.0 and below (I needed this for other code in the Memory2D<T> and Span2D<T> types, so I used it here too while I was at it), I use that here:

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/2745962d560c4cf96b55074bc8babd0da7c6b0b3/Microsoft.Toolkit.HighPerformance/Helpers/HashCode%7BT%7D.cs#L62-L65

Now that older targets can test this too at runtime, we no longer have to add that extra type constraint, which was basically just a precaution due to the lack of this API at runtime. You can see how we switch the polyfill on here:

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/2745962d560c4cf96b55074bc8babd0da7c6b0b3/Microsoft.Toolkit.HighPerformance/Helpers/HashCode%7BT%7D.cs#L12-L16

So now on all targets the API will just automatically pick the right implementation for each input type T 😄

michael-hawker · 2020-11-03T17:56:45Z

UnitTests/UnitTests.HighPerformance.Shared/Extensions/Test_ArrayExtensions.2D.cs

+
+            Assert.ThrowsException<ArgumentOutOfRangeException>(() => array.GetColumn(-1));
+
+            Assert.ThrowsException<ArgumentOutOfRangeException>(() => array.GetColumn(20));


We should also be testing the 'off-by-1' index of '3', eh? (here and where-ever)

Right! I had added those tests when getting rows/columns from spans, but not from 2D arrays directly!
Fixed in 7a65a29, for both GetRow and GetColumn in this file 😄

michael-hawker · 2020-11-03T18:00:20Z

UnitTests/UnitTests.HighPerformance.Shared/Extensions/Test_ReadOnlySpanExtensions.cs

+
+            // Try to copy to a valid row and an invalid column (too short for the source span)
+            bool shouldBeTrue = values.TryCopyTo(array1.GetRow(2));
+            bool shouldBeFalse = values.TryCopyTo(array1.GetColumn(1));


We're not checking here that no mutation occurred, eh? As the line 281 above would have copied the same values? (I mean we know it failed as '20' is still in the 3rd row, but may be better to use a different column like '3' here to be safe to guard against this case too.)

Good catch! Also added one extra check for the entire target array for good measure 😄
Done in 6bb9e5a, including the same changes for the comment below.

UnitTests/UnitTests.HighPerformance.Shared/Extensions/Test_SpanExtensions.cs

michael-hawker · 2020-11-03T21:25:46Z

Microsoft.Toolkit.HighPerformance/Helpers/ParallelHelper.ForEach.IRefAction.cs

@@ -135,8 +135,7 @@ public static partial class ParallelHelper
            /// Processes the batch of actions at a specified index
            /// </summary>
            /// <param name="i">The index of the batch to process</param>
-            [MethodImpl(MethodImplOptions.AggressiveInlining)]


Expected for the inlining to disappear?

Yeah this was just a leftover, it's not there in the other implementations either. This method is just called once per thread anyway and does all the job. It's the actual delegates within each invocation of this method that need to be inlined, as they're invoked in a tight loop, not this one. As in, this loop here:

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/2745962d560c4cf96b55074bc8babd0da7c6b0b3/Microsoft.Toolkit.HighPerformance/Helpers/ParallelHelper.ForEach.IRefAction.cs#L147-L152

michael-hawker · 2020-11-03T21:27:24Z

Microsoft.Toolkit.HighPerformance/Helpers/ParallelHelper.ForEach.IRefAction2D.cs

+            // of CPU cores (which is an int), and the height of each batch is necessarily
+            // smaller than or equal than int.MaxValue, as it can't be greater than the
+            // number of total batches, which again is capped at the number of CPU cores.
+            nint


So if this is native to the platform, how do we unit test the code for both possible sizes?

michael-hawker · 2020-11-03T21:29:20Z

Microsoft.Toolkit.HighPerformance/Microsoft.Toolkit.HighPerformance.csproj

@@ -2,7 +2,7 @@

  <PropertyGroup>
    <TargetFrameworks>netstandard1.4;netstandard2.0;netstandard2.1;netcoreapp2.1;netcoreapp3.1</TargetFrameworks>
-    <LangVersion>8.0</LangVersion>
+    <LangVersion>9.0</LangVersion>


Does this need anything new for our build pipeline/tooling requirements for folks?

Commenting here about the same comments from the stream, for future reference. Right now this works on the latest versions of VS stable (not the preview preview), because it uses the same Roslyn codebase as VS Preview, just from an older branch. As such, some C# 9 features are buggy/outdated (eg. function pointers), but since here we're really only using the new nint type, that bit in particular mostly works. I only had to add an extra explicit cast here to ensure the correct codegen due to a small compiler bug that's fixed in VS Preview, for instance:

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/7a65a29ef8c0570c7c66a20cf2bb45d72a3066b2/Microsoft.Toolkit.HighPerformance/Helpers/Internals/SpanHelper.Count.cs#L277-L280

As in, this PR should work fine when compiled with VS stable, but especially due to all the other C# 9 changes coming in other PRs, as well as the .NET 5 support itself, we should definitely update the CI build of VS to the new release as soon as it drops hopefully next week when .NET 5 is announced at .NET Conf 😄

michael-hawker · 2020-11-03T21:32:32Z

UnitTests/UnitTests.HighPerformance.Shared/Extensions/Test_ReadOnlySpanExtensions.cs

+
+            // Copy a span to a target row and column with valid lengths
+            values.CopyTo(array1.GetRow(0));
+            values.Slice(0, 4).CopyTo(array1.GetColumn(1));


Should we check the intermediary results? I've noticed these tests are chaining quite a few operations together, so it may be better to help find problems in a particular part if we're validating the first part before moving on? (Or split them as separate tests?)

I think I addressed this in 6bb9e5a, let me know if that's alright 😄

michael-hawker · 2020-11-03T21:35:39Z

UnitTests/UnitTests.HighPerformance.Shared/Memory/Test_Memory2D{T}.cs

+
+            // A couple of tests for invalid parameters, ie. layers out of range
+            Assert.ThrowsException<ArgumentOutOfRangeException>(() => new Memory2D<int>(array, -1));
+            Assert.ThrowsException<ArgumentOutOfRangeException>(() => new Memory2D<int>(array, 20));


same here for off-by-one

Added a few new tests for this and ReadOnlyMemory2D<T> versions in 7b04c7d 👍

ghost · 2020-11-03T21:45:41Z

Hello @michael-hawker!

Because this pull request has the auto merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Microsoft.Toolkit.HighPerformance/Memory/Span2D{T}.cs

Rosuavio

Other than cleaning up some no longer needed SuppressMessageAttributes, It looks good.

Rosuavio · 2020-11-05T00:14:46Z

UnitTests/UnitTests.HighPerformance.Shared/Extensions/Test_ArrayExtensions.2D.cs

        [SuppressMessage("StyleCop.CSharp.NamingRules", "SA1312", Justification = "Dummy loop variable")]
        [SuppressMessage("StyleCop.CSharp.LayoutRules", "SA1501", Justification = "Empty test loop")]


There a few SuppressMessageAttributes left on methods that are no longer needed. It would be good to remove them from methods that no longer need it.

Oh, good catch, forgot about those! 😄
Fixed in 7e67a9c, and also fixed some other leftover warnings in the tests.

Sergio0694 · 2020-11-05T00:22:55Z

Removing the auto merge tag now that the PR is approved as I was waiting on one last piece of feedback from Tanner on a possible minor tweak to do to some constructors in the new types, so that if that's needed I could just do it here instead of opening a whole other PR just to fix that afterwards? cc. @michael-hawker

## Fixup for #3353   ## PR Type What kind of change does this PR introduce?  - Bugfix        ## Overview  `ReadOnlySpan2D<T>.Slice` has the incorrect ordering for the `height` and `width` parameters, which is inverted with respect to those in `Span2D<T>.Slice`. This is an oversight from a refactor in the original PR, and as a result it also causes the `ReadOnlySpan2D<T>.this[Range, Range]` indexer to fail for `ReadOnlySpan2D<T>`. This PR fixes both issues and adds a new series of tests for the two indexers as well. We probably didn't notice this before since those indexers are not available on UWP 🤔 > **NOTE:** this PR technically introduces a breaking change (swapped parameters in the `.Slice` method, but as that's an issue and not intended, and also because they're in the right order in `Span2D<T>`, it's more of a fix than an actual breaking change. ## PR Checklist Please check if your PR fulfills the following requirements: - [X] Tested code with current [supported SDKs](../readme.md#supported) - [ ] ~~Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: ~~ - [ ] ~~Sample in sample app has been added / updated (for bug fixes / features)~~ - [ ] ~~Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)~~ - [X] New major technical changes in the toolkit have or will be added to the [Wiki](https://github.com/windows-toolkit/WindowsCommunityToolkit/wiki) e.g. build changes, source generators, testing infrastructure, sample creation changes, etc... - [X] Tests for the changes have been added (for bug fixes / features) (if applicable) - [X] Header has been added to all new source files (run *build/UpdateHeaders.bat*) - [ ] Contains **NO** breaking changes

ghost assigned michael-hawker Jun 17, 2020

john-h-k reviewed Jun 17, 2020

View reviewed changes

Sergio0694 marked this pull request as ready for review June 19, 2020 10:36

john-h-k suggested changes Jun 19, 2020

View reviewed changes

ghost added needs author feedback 📝 needs attention 👋 and removed needs author feedback 📝 labels Jun 19, 2020

Sergio0694 added feature 💡 high-performance 🚂 Issues/PRs for the Microsoft.Toolkit.HighPerformance package labels Jul 7, 2020

This was referenced Aug 16, 2020

Restructure sections to separate UWP and .NET Standard MicrosoftDocs/WindowsCommunityToolkitDocs#303

Open

[Future] 7.0 Release Plan #3062

Closed

Memory2D<T>/Span2D<T> docs MicrosoftDocs/WindowsCommunityToolkitDocs#376

Merged

michael-hawker reviewed Sep 8, 2020

View reviewed changes

Microsoft.Toolkit.HighPerformance/Enumerables/ReadOnlySpanEnumerable{T}.cs Outdated Show resolved Hide resolved

HurricanKai reviewed Sep 10, 2020

View reviewed changes

Microsoft.Toolkit.HighPerformance/Memory/Span2D{T}.cs Outdated Show resolved Hide resolved

Sergio0694 mentioned this pull request Sep 24, 2020

Deprecated 2D array extensions from Microsoft.Toolkit #3444

Merged

7 tasks

ghost removed the needs attention 👋 label Sep 25, 2020

Sergio0694 added 7 commits October 8, 2020 22:57

Added new constructors for Ref<T> and ReadOnlyRef<T>

2b80061

Added initial version of the Span2D<T> type

b89a83c

Minor code tweaks to Span2D<T>

d6980bc

Added initial version of the Memory2D<T> type

eb1bb6e

Added missing Memory2D<T> header

f11b77d

Improved Span2D<T> layout (removed 4 bytes)

64a293c

Minor code refactoring

4de84df

Sergio0694 and others added 7 commits October 31, 2020 17:13

Removed unnecessary covariance array checks

2626c7d

Added comments to unit tests

8a70ace

Fixed bugs in unit tests

151f38a

Fixed bug in ReadOnlyRefEnumerable<T> implicit cast

20bd65f

Minor code refactoring

f3e7e00

Minor code tweaks for clarity

bc6f70b

Merge branch 'master' into feature/array3d-apis

2745962

michael-hawker approved these changes Nov 3, 2020

View reviewed changes

michael-hawker added this to the 7.0 milestone Nov 3, 2020

michael-hawker added the auto merge ⚡ label Nov 3, 2020

DaZombieKiller reviewed Nov 3, 2020

View reviewed changes

Microsoft.Toolkit.HighPerformance/Memory/Span2D{T}.cs Outdated Show resolved Hide resolved

Sergio0694 added 6 commits November 3, 2020 23:08

Added tests for off-by-one Get[Row|Column] for 2D arrays

7a65a29

Improved some unit tests

6bb9e5a

Added more tests for invalid Memory2D<T> constructors

7b04c7d

Fixed some typos

7738d34

Fixed XML docs for internal helpers

cd21086

Updated package description with new types

9c90f66

Rosuavio approved these changes Nov 5, 2020

View reviewed changes

Sergio0694 removed the auto merge ⚡ label Nov 5, 2020

Fixed StyleCop warnings in HighPerformance tests

7e67a9c

michael-hawker added the auto merge ⚡ label Nov 5, 2020

michael-hawker merged commit fdee563 into CommunityToolkit:master Nov 5, 2020

Sergio0694 mentioned this pull request Apr 20, 2021

Fixed ReadOnlySpan2D<T>.Slice parameters order #3951

Merged

8 tasks

Sergio0694 mentioned this pull request May 26, 2021

[Feature, HighPerformance] Public length and indexer properties for RefEnumerable<T> #4043

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2D System.Memory-like primitives (Span2D<T>, Memory2D<T>) #3353

2D System.Memory-like primitives (Span2D<T>, Memory2D<T>) #3353

Sergio0694 commented Jun 17, 2020 •

edited

Loading

ghost commented Jun 17, 2020

ghost commented Jun 19, 2020

michael-hawker left a comment

HurricanKai left a comment

Sergio0694 commented Sep 10, 2020

michael-hawker commented Sep 25, 2020

Sergio0694 commented Sep 25, 2020

michael-hawker commented Oct 8, 2020

michael-hawker left a comment

michael-hawker Nov 3, 2020

Sergio0694 Nov 3, 2020

michael-hawker Nov 3, 2020

Sergio0694 Nov 3, 2020

michael-hawker Nov 3, 2020

Sergio0694 Nov 3, 2020

michael-hawker Nov 3, 2020

Sergio0694 Nov 3, 2020

michael-hawker Nov 3, 2020

michael-hawker Nov 3, 2020

Sergio0694 Nov 3, 2020

michael-hawker Nov 3, 2020

Sergio0694 Nov 3, 2020

michael-hawker Nov 3, 2020

Sergio0694 Nov 3, 2020

ghost commented Nov 3, 2020

Rosuavio left a comment

Rosuavio Nov 5, 2020

Sergio0694 Nov 5, 2020

Sergio0694 commented Nov 5, 2020


		Assert.ThrowsException<ArgumentOutOfRangeException>(() => array.GetColumn(-1));

		Assert.ThrowsException<ArgumentOutOfRangeException>(() => array.GetColumn(20));

		[SuppressMessage("StyleCop.CSharp.NamingRules", "SA1312", Justification = "Dummy loop variable")]
		[SuppressMessage("StyleCop.CSharp.LayoutRules", "SA1501", Justification = "Empty test loop")]

2D System.Memory-like primitives (Span2D<T>, Memory2D<T>) #3353

2D System.Memory-like primitives (Span2D<T>, Memory2D<T>) #3353

Conversation

Sergio0694 commented Jun 17, 2020 • edited Loading

PR Type

What is the current behavior?

What is the new behavior?

API surface (summary)

PR Checklist

Additional notes

ghost commented Jun 17, 2020

ghost commented Jun 19, 2020

michael-hawker left a comment

Choose a reason for hiding this comment

HurricanKai left a comment

Choose a reason for hiding this comment

Sergio0694 commented Sep 10, 2020

michael-hawker commented Sep 25, 2020

Sergio0694 commented Sep 25, 2020

michael-hawker commented Oct 8, 2020

michael-hawker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 3, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Rosuavio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 commented Nov 5, 2020

Sergio0694 commented Jun 17, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.