Make use of CopyBlock for non-overlapping spans #17063

shiftylogic · 2017-03-13T21:01:17Z

Fix for #16840.

For non-overlapping spans, this makes copy about 9-10x faster. For overlapping spans, we still take the slow paths.

[Edit] 9-10x for sufficiently large blocks (> 2 KB). Though a 256 byte block was still about 4x faster. A 16 byte block was only marginally faster.

shiftylogic · 2017-03-13T21:01:36Z

@KrzysztofCwalina @davidfowl @ahsonkhan

KrzysztofCwalina · 2017-03-13T21:44:44Z

src/System.Memory/src/System/Span.cs

                    {
-                        Unsafe.Add<T>(ref dst, i) = Unsafe.Add<T>(ref src, i);
+                        Unsafe.CopyBlock<T>(ref dst, ref src, (uint)length);


Does BlockCopy work for overlapping ranges where the source is after (address wise) the destination?

There is no Unsafe.CopyBlock<T>. I do not see how this can ever work. Note that this implementation is only for slow span, and so you have to go out of your way to build and test it.

@jkotas This is my first touch of CoreFx and I learned something new today. I was clearly not building what I thought I was building. Apparently, the file I was changing is only included in builds on particular platforms that I wasn't hitting until I submitted this.

I am testing a new commit that addresses the real issue right now. I will update this PR soon.

KrzysztofCwalina · 2017-03-13T21:45:03Z

src/System.Memory/src/System/Span.cs

+                        // Source address greater than or equal to destination address. Can do normal copy.
+                        for (int i = 0; i < length; i++)
+                        {
+                            Unsafe.Add<T>(ref dst, i) = Unsafe.Add<T>(ref src, i);


Should this be unrolled?

I was initially just fixing the perf for non-overlapping blocks. If you want this unrolled, I can add that as another commit.

Either a different commit or a different PR, but I think we should try to make the gap between overlapping and non-overlapping as small as possible. Otherwise it creates annoying performance traps.

The slower path for overlapping exists in the fast Span as well. Fixing it for fast Span should be higher priority than for the slow Span.

KrzysztofCwalina · 2017-03-13T21:46:56Z

Do we have a benchmark for this checked into coreclr? so that the JIT team can work on optimizing code gen for it?

cc: @ahsonkhan

jkotas · 2017-03-13T23:09:11Z

src/System.Memory/src/System/Span.cs

-                        Unsafe.CopyBlock<T>(ref dst, ref src, (uint)length);
+                        void* pSrc = Unsafe.AsPointer<T>(ref src);
+                        void* pDst = Unsafe.AsPointer<T>(ref dst);
+                        Unsafe.CopyBlockUnaligned(pDst, pSrc, (uint)(length * Unsafe.SizeOf<T>()));


length * Unsafe.SizeOf<T>() can be more than 4GB. It is not ok to cast it to uint.

Unsafe.CopyBlock only takes a uint in the 3rd argument.

Should I add a check and fail on overflow or is there a bug in CopyBlock?

You should call CopyBlock multiple times as necessary.

CopyBlock is basically just exposing cpblk instruction as an API. The cpblk instruction takes uint size unfortunately.

jkotas · 2017-03-13T23:09:37Z

src/System.Memory/src/System/Span.cs

-                        Unsafe.CopyBlock<T>(ref dst, ref src, (uint)length);
+                        void* pSrc = Unsafe.AsPointer<T>(ref src);
+                        void* pDst = Unsafe.AsPointer<T>(ref dst);
+                        Unsafe.CopyBlockUnaligned(pDst, pSrc, (uint)(length * Unsafe.SizeOf<T>()));


CopyBlockUnaligned only works for elements that do not contain GC pointers.

@jkotas Is it okay to call CopyBlock with an unaligned buffer? Or do we need to ensure it is aligned?

Calling CopyBlockUnaligned on unaligned buffer is fine.

@jkotas Is it okay to call CopyBlock with an unaligned buffer? Or do we need to ensure it is aligned?

Calling CopyBlockUnaligned on unaligned buffer is fine.

CopyBlock or CopyBlockUnaligned?

CopyBlockUnaligned. It will do the same thing as CopyBlock on x86 and x64 because of these platforms do not require alignment.

ahsonkhan · 2017-03-14T00:01:36Z

Do we have a benchmark for this checked into coreclr? so that the JIT team can work on optimizing code gen for it?

@KrzysztofCwalina, there is a benchmark for Span.CopyTo, however it only tests fast span.
https://github.com/dotnet/coreclr/blob/master/tests/src/JIT/Performance/CodeQuality/Span/SpanBench.cs#L597
Issue: https://github.com/dotnet/coreclr/issues/10098

KrzysztofCwalina · 2017-03-14T00:04:22Z

@ahsonkhan, thanks. Fast span only benchmarks is good enough. I don't think we want to spend much more resources optimizing JIT for the slow span

ahsonkhan · 2017-03-14T00:07:11Z

src/System.Memory/src/System/Span.cs

-                    // Source address greater than or equal to destination address. Can do normal copy.
-                    for (int i = 0; i < length; i++)
+                    IntPtr tailDiff = Unsafe.ByteOffset<T>(ref Unsafe.Add<T>(ref dst, destLength), ref src);
+                    bool isOverlapped = (sizeof(IntPtr) == sizeof(int)) ? tailDiff.ToInt32() < 0 : tailDiff.ToInt64() < 0;


@jkotas, these sizeof checks won't be necessary once we have support for native int, correct?

davidfowl · 2017-03-14T06:11:40Z

src/System.Memory/src/System/Span.cs

@@ -386,6 +382,25 @@ public bool TryCopyTo(Span<T> destination)
            return false;
        }

+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private unsafe void CopyBlocks(ref T dst, ref T src, int length)


Is it worth having a fast path for when length < uint.MaxValue?

This is slow span only and this implementation is already 9x faster than what we had. I could add that optimization if we really want it.

It's not that important 9x is good 😄. That can be up for grabs

jkotas · 2017-03-14T06:26:45Z

src/System.Memory/src/System/Span.cs

-                    while (i-- != 0)
+                    IntPtr tailDiff = Unsafe.ByteOffset<T>(ref Unsafe.Add<T>(ref src, length), ref dst);
+                    bool isOverlapped = (sizeof(IntPtr) == sizeof(int)) ? tailDiff.ToInt32() < 0 : tailDiff.ToInt64() < 0;
+                    if (!isOverlapped)


You also need to check for SpanHelpers.IsReferenceOrContainsReferences() and use the slower path if it returns true.

shiftylogic · 2017-03-14T15:56:20Z

@dotnet-bot test Innerloop OSX10.12 Debug x64 Build and Test

shiftylogic · 2017-03-14T19:01:50Z

@jkotas @KrzysztofCwalina Any more comments?

ahsonkhan · 2017-03-14T19:28:51Z

src/System.Memory/src/System/Span.cs

                    }
+
+                    return true;
                }


Should we refactor out the if(srcGreaterThanDst) block and if(dstGreaterThanSrc) block into helper methods? They are very similar.

ahsonkhan · 2017-03-14T19:30:08Z

src/System.Memory/src/System/Span.cs

-                        Unsafe.Add<T>(ref dst, i) = Unsafe.Add<T>(ref src, i);
+                        // Source address less than destination address. Must do backward copy.
+                        int i = length;
+                        while (i-- != 0)


We have for loop above vs while loop here. Is there a reason for this inconsistency?

I never changed that part. Not sure if there is a performance reason it was done.

Yes, the loop above walks memory forward. This one walks memory backwards. Walking memory forward is faster on some hardware as the CPU prefetches memory in the former case and not the latter.

@KrzysztofCwalina I think the question was about for vs while? Is there a reason for was used with one loop but while with the backwards loop?

Ah, got it. In general, I think it's better to use a for loop as it makes it more likely that bounds check optimizations will kick in. Otherwise, the optimizer might not realize that the while loop variable is actually the Length.

But, maybe our optimizer is better than I realize? @russellhadley?

… path

ahsonkhan · 2017-03-14T20:16:52Z

src/System.Memory/src/System/Span.cs

+            bool isOverlapped = (sizeof(IntPtr) == sizeof(int)) ? tailDiff.ToInt32() < 0 : tailDiff.ToInt64() < 0;
+            if (!isOverlapped && !SpanHelpers.IsReferenceOrContainsReferences<T>())
+            {
+                CopyBlocks(ref dst, ref src, srcLength);


Question: Why can't we call CopyBlocks if SpanHelpers.IsReferenceOrContainsReferences()?

@jkotas Also curious

Why can't we call CopyBlocks if SpanHelpers.IsReferenceOrContainsReferences

Because of CopyBlocks does not have the correct GC tracking (does not update cards for generational GC).

If you would like to see the details, take a look at the optimized Span Copy implementation in CoreCLR that updates the cards in bulk (dotnet/coreclr#9999).

ahsonkhan · 2017-03-14T20:17:24Z

src/System.Memory/src/System/Span.cs


-                bool srcGreaterThanDst = (sizeof(IntPtr) == sizeof(int)) ? srcMinusDst.ToInt32() >= 0 : srcMinusDst.ToInt64() >= 0;
-                if (srcGreaterThanDst)
+                if (!TryFastCopy(ref dst, destLength, ref src, length))


Any significant perf change because of this?

It is about the same logic as before but actually slightly more simple. In my tests, there isn't anything noticeably different than the first version.

shiftylogic · 2017-03-14T20:22:27Z

FYI, the code diff here looks a lot dirtier than it actually is. In Beyond Compare, it is much easier to consume. I now just add 2 new private methods that do the "fast" version of copy checking and work. Then I wrapped the original code in a fast attempt, but with a slow fallback.

KrzysztofCwalina · 2017-03-14T20:40:22Z

src/System.Memory/src/System/Span.cs

+                        // Source address greater than or equal to destination address. Can do normal copy.
+                        for (int i = 0; i < length; i++)
+                        {
+                            Unsafe.Add<T>(ref dst, i) = Unsafe.Add<T>(ref src, i);


It would be great to have an issue opened to unroll the loop later.

KrzysztofCwalina · 2017-03-14T20:46:39Z

src/System.Memory/src/System/Span.cs

+        private unsafe bool TryFastCopy(ref T dst, int dstLength, ref T src, int srcLength)
+        {
+            IntPtr srcMinusDst = Unsafe.ByteOffset<T>(ref dst, ref src);
+            bool srcGreaterThanDst = (sizeof(IntPtr) == sizeof(int)) ? srcMinusDst.ToInt32() >= 0 : srcMinusDst.ToInt64() >= 0;


Why not simply call ToInt64, i.e. don't we properly preserve the sign when we convert 32-bit IntPtr to Int64?

I was not certain of this either. The original code did this. I assumed there was a reason. @jkotas?

Why not simply call ToInt64

There are other places that check for sizeof(IntPtr) == sizeof(int) in the Span implementation. It is done to avoid performance hit from using long on 32-bit platforms. Workaround for lack of native int type in C#...

KrzysztofCwalina · 2017-03-14T20:50:49Z

src/System.Memory/src/System/Span.cs

+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private unsafe bool TryFastCopy(ref T dst, int dstLength, ref T src, int srcLength)


It would be good to add more comments in the method (like you did above) to describe all the pointer arithmetics and checks.

KrzysztofCwalina · 2017-03-14T20:53:57Z

src/System.Memory/src/System/Span.cs

+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private unsafe void CopyBlocks(ref T dst, ref T src, int length)
+        {


It might be worth adding Debug.Assert(length>=0);
And in general, would be good to have more debug assertions in code that deals with pointer arithmetic. It's difficult to assert the correctness something without seeing calling methods.

done. I also added an early exit for zero length span at the top level public method.

jkotas · 2017-03-14T22:03:54Z

src/System.Memory/src/System/Span.cs

+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private unsafe bool TryFastCopy(ref T dst, int dstLength, ref T src, int srcLength)


What's the point of splitting this into multiple methods? Are they even going to get inlined? (Note that AggressiveInlining does not guarantee inlining - the JIT often bails if the method is too complex.)

It was for readability, but I'm fine with merging them back together.

KrzysztofCwalina · 2017-03-15T17:17:55Z

looks good.

ahsonkhan · 2017-03-25T02:12:57Z

Part of dotnet/corefxlab#1314

Make use of CopyBlock for non-overlapping spans

df2bfc8

shiftylogic added the area-System.Memory label Mar 13, 2017

dnfclas added the cla-already-signed label Mar 13, 2017

karelz assigned shiftylogic Mar 13, 2017

sizeof(IntPtr) needs unsafe

0dff1c8

KrzysztofCwalina reviewed Mar 13, 2017

View reviewed changes

Making call to ref byte version of CopyBlock

15d7217

jkotas reviewed Mar 13, 2017

View reviewed changes

ahsonkhan reviewed Mar 14, 2017

View reviewed changes

Block copy that handles buffers larger than 4GB

3836687

davidfowl reviewed Mar 14, 2017

View reviewed changes

jkotas reviewed Mar 14, 2017

View reviewed changes

Skip fast path for reference types and types containing references

01303dc

ahsonkhan reviewed Mar 14, 2017

View reviewed changes

Consolidating code and simplifying the logic for Span::TryCopyTo fast…

8a9d8ad

… path

ahsonkhan reviewed Mar 14, 2017

View reviewed changes

KrzysztofCwalina reviewed Mar 14, 2017

View reviewed changes

FastCopy comments, asserts, and a fast exit for zero length source.

1a5672a

jkotas reviewed Mar 14, 2017

View reviewed changes

Merging methods back into a single method for trycopyto.

2ccd0d5

jkotas approved these changes Mar 15, 2017

View reviewed changes

shiftylogic merged commit 1014d2f into dotnet:master Mar 15, 2017

karelz modified the milestone: 2.0.0 Mar 18, 2017

Make use of CopyBlock for non-overlapping spans #17063

Make use of CopyBlock for non-overlapping spans #17063

Conversation

shiftylogic commented Mar 13, 2017 • edited Loading

shiftylogic commented Mar 13, 2017

Choose a reason for hiding this comment

jkotas Mar 13, 2017 • edited Loading

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

jkotas Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

KrzysztofCwalina commented Mar 13, 2017

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

shiftylogic Mar 13, 2017 • edited Loading

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

ahsonkhan commented Mar 14, 2017

KrzysztofCwalina commented Mar 14, 2017

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

shiftylogic commented Mar 14, 2017

shiftylogic commented Mar 14, 2017 • edited Loading

ahsonkhan Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina Mar 14, 2017 • edited Loading

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

jkotas Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiftylogic commented Mar 14, 2017

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

KrzysztofCwalina commented Mar 15, 2017

ahsonkhan commented Mar 25, 2017

shiftylogic commented Mar 13, 2017 •

edited

Loading

jkotas Mar 13, 2017 •

edited

Loading

jkotas Mar 13, 2017 •

edited

Loading

shiftylogic Mar 13, 2017 •

edited

Loading

shiftylogic commented Mar 14, 2017 •

edited

Loading

ahsonkhan Mar 14, 2017 •

edited

Loading

KrzysztofCwalina Mar 14, 2017 •

edited

Loading

jkotas Mar 15, 2017 •

edited

Loading