Remove use of Unsafe in UTF-8 formatters, plus performance improvements by GrabYourPitchforks · Pull Request #26289 · dotnet/corefx

GrabYourPitchforks · 2018-01-12T03:41:50Z

Fixes https://github.com/dotnet/corefx/issues/25648.

My first attempt to switch the implementations from unsafe code to safe buffer-based code was a bit naive and resulted in considerable performance degradation (double-digit percentage loss of throughput across many APIs). I've spent time reworking the implementations to to work around the performance loss, and in many cases the new implementations are faster than the originals.

For reviewers: I recommend looking at each commit in isolation, as each commit deals with a very specific formatter. It'll also be easier to see which helper routines in FormatterHelpers correlate with which implementations. This also allows individual commits to be backed out without affecting the rest of the PR if reviewers deem a particular commit as unwanted.

Also optimize code: on Win10 amd64 this results in +46% throughput (GUIDs formatted per second)

On Win10 amd64 this results in +82% throughput (bools formatted per second)

Does not significantly impact perf; measurements are within +/-5% on Win10 amd64 test box

Also optimize code: on Win10 amd64 this results in +25% to +35% throughput (DateTimes formatted per second) depending on format

Also add missing test case for DateTimeOffset formatter Also optimize code: on Win10 amd64 this results in +20% to +30% throughput (TimeSpans formatted per second) depending on magnitude of value

Also optimize code: on Win10 amd64 this results in +45% throughput (integers formatted per second) for signed integers formatted as D or N, +60% throughput for unsigned integers formatted as D or N

ghost · 2018-01-12T18:37:32Z

                    if (format.HasPrecision)
                        throw new NotSupportedException(SR.Argument_GWithPrecisionNotSupported); // With a precision, 'G' can produce exponential format, even for integers.
-                    return TryFormatUInt64D(value, format.Precision, buffer, out bytesWritten);
+                    return TryFormatUInt64D(value, format.Precision, buffer, false /* insertNegationSign */, out bytesWritten);


C# now allows name-tagging arguments without name-tagging every argument after it:

return TryFormatUInt64D(value, format.Precision, buffer, false /* insertNegationSign */, out bytesWritten);

==>

return TryFormatUInt64D(value, format.Precision, buffer, insertNegationSign: false, out bytesWritten);

Didn't know that - nifty! :)

This is now addressed.

ghost · 2018-01-12T19:44:42Z

What are the code coverage numbers after this change?

ghost · 2018-01-12T19:47:05Z

-            {
-                bytesWritten = 0;
-                return false;
+                    const uint FalsValueUppercase = ('F' << 24) + ('a' << 16) + ('l' << 8) + ('s' << 0);


Nit: "Fals" -> "False"

Nvm - I see the "e" doesn't fit into a uint :-)

jkotas · 2018-01-13T05:45:19Z

+                ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(T));
+            }
+#endif
+            return Span<byte>.DangerousCreate(null, ref Unsafe.As<T, byte>(ref value), Unsafe.SizeOf<T>());


This is not valid - it crash with slow Span.

It is not valid to pass null as first argument to DangerousCreate,

Previous closed issue about this https://github.com/dotnet/corefx/issues/26124

This provides approximately equal (within noise) performance to the original code which turned a ref Guid into a Span<byte>

GrabYourPitchforks · 2018-01-16T21:41:04Z

@atsushikan I don't have code coverage numbers available. I'm riding on top of the existing (quite comprehensive) unit tests for this functionality, plus I added a few extra test cases for situations where the original code produced incorrect results.

ghost · 2018-01-16T21:46:23Z

@GrabYourPitchforks - You can get code coverage results by doing this:

   cd <repo>\src\System.Memory
   msbuild /t:build /p:DebugType=pdbonly
   cd <repo>\src\System.Memory\tests

   msbuild -t:clean;build;test -p:Coverage=true
   start <repo>\bin\tests\coverage\index.htm

The coverage numbers for Utf8Formatting were at a 100% when I checked in the original stuff - it's probably regressed some since then but it'd be good to know at least that the stuff you touched is at 100%.

MattGal · 2018-01-16T22:10:47Z

@dotnet-bot test Linux x64 Release Build please
@dotnet-bot test OSX x64 Debug Build please
@dotnet-bot test UWP CoreCLR x64 Debug Build please
@dotnet-bot test Windows x64 Debug Build please
@dotnet-bot test Windows x86 Release Build please

GrabYourPitchforks · 2018-01-16T23:35:26Z

@atsushikan It has regressed slightly (89%), but those were changes separate from this PR. Everything looks good here from a code coverage perspective. :)

ahsonkhan · 2018-01-19T19:28:20Z

+                else if (symbol == 'l')
+                {
+                    const uint TrueValueLowercase = ('t' << 24) + ('r' << 16) + ('u' << 8) + ('e' << 0);
+                    if (!BinaryPrimitives.TryWriteUInt32BigEndian(buffer, TrueValueLowercase))


Would writing TryFormat using BinaryPrimitives still be necessary once TryCopyTo is optimized?
https://github.com/dotnet/coreclr/issues/15076

cc @GrabYourPitchforks

ahsonkhan · 2018-01-19T19:37:41Z

        {
-            for (int i = FractionDigits; i > digitCount; i--)
-                value /= 10;
+            // This is a faster implementation of Span<T>.Fill().


Why is this faster? Is it due to buffer.Length being too small in this case?

Wouldn't Fill using InitBlockUnaligned be just as fast, if not faster:
https://github.com/dotnet/corefx/blob/master/src/System.Memory/src/System/Span.cs#L219-L231

ahsonkhan · 2018-01-19T19:42:03Z

+using System.Runtime.CompilerServices;
+
 #if !netstandard
 using Internal.Runtime.CompilerServices;


Do we still need this using directive?

Removed it here: #26598

…ts (dotnet/corefx#26289) Remove use of Unsafe in UTF-8 formatters, plus performance improvements Commit migrated from dotnet/corefx@534af93

GrabYourPitchforks added 6 commits January 11, 2018 18:36

Remove unsafe code from Guid formatter

e30a8cc

Also optimize code: on Win10 amd64 this results in +46% throughput (GUIDs formatted per second)

Improve performance of Bool formatter

bb80287

On Win10 amd64 this results in +82% throughput (bools formatted per second)

Remove unsafe code from TryFormatUInt64X

5d862bd

Does not significantly impact perf; measurements are within +/-5% on Win10 amd64 test box

Remove unsafe code from Date formatter

9d1e0bb

Also optimize code: on Win10 amd64 this results in +25% to +35% throughput (DateTimes formatted per second) depending on format

Remove unsafe code from TimeSpan formatter

4f2ed41

Also add missing test case for DateTimeOffset formatter Also optimize code: on Win10 amd64 this results in +20% to +30% throughput (TimeSpans formatted per second) depending on magnitude of value

Remove unsafe code from Integer formatter

32355ae

Also optimize code: on Win10 amd64 this results in +45% throughput (integers formatted per second) for signed integers formatted as D or N, +60% throughput for unsigned integers formatted as D or N

ghost reviewed Jan 12, 2018

View reviewed changes

jkotas reviewed Jan 13, 2018

View reviewed changes

GrabYourPitchforks added 2 commits January 16, 2018 13:28

Remove generic blittable reinterpretation logic from Guid processing

0892617

This provides approximately equal (within noise) performance to the original code which turned a ref Guid into a Span<byte>

Address CR feedback / general cleanup

7504f67

karelz added the area-System.Memory label Jan 18, 2018

karelz assigned GrabYourPitchforks Jan 18, 2018

GrabYourPitchforks merged commit 534af93 into dotnet:master Jan 19, 2018

ahsonkhan reviewed Jan 19, 2018

View reviewed changes

karelz added this to the 2.1.0 milestone Jan 20, 2018

ahsonkhan mentioned this pull request Jan 26, 2018

System.Memory source cleanup and fix byteOffset check in tests #26598

Merged

GrabYourPitchforks deleted the levib/remove_unsafe_4 branch January 29, 2018 02:27

ahsonkhan mentioned this pull request Jan 31, 2020

Investigate ways to optimize Span.Fill and Span.Clear for small buffers dotnet/runtime#24806

Closed

Uh oh!

Conversation

GrabYourPitchforks commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GrabYourPitchforks commented Jan 16, 2018

Uh oh!

ghost commented Jan 16, 2018

Uh oh!

MattGal commented Jan 16, 2018

Uh oh!

GrabYourPitchforks commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

GrabYourPitchforks commented Jan 12, 2018 •

edited

Loading