Primitive formatting enhancements for Time and Uuid #1462

shiftylogic · 2017-04-11T23:33:55Z

Optimized for Invariant UTF-8 and UTF-16.
Formats are matched with existing ToString(InvariantCulture).
Test coverage.
Performance improvements (2-7x depending on scenario).

…versions; code structure cleanup

ahsonkhan · 2017-04-12T00:14:33Z

src/System.Text.Primitives/System/Text/Formatting/Internal/FormattingHelpers.cs

+        public static int WriteFractionDigits(long value, int digitCount, ref byte buffer, int index)
+        {
+            for (var i = FractionDigits; i > digitCount; i--)
+                value = DivMod(value, 10, out long m);


We don't use out long m anywhere, should we have an overload that doesn't return an out parameter?

In most cases, that out param is used since that is the purpose of this function. However, this particular usage is actually leftover from when I was using a custom implementation of DivMod10 that was faster but obscure and only worked on positive integers up to 1 billion or so. I ended up reverting to the Math.DivRem implementation for easier maintenance.

I will switch this to a straight division since my optimized DivMod10 is gone now.

ahsonkhan · 2017-04-12T00:17:05Z

src/System.Text.Primitives/System/Text/Formatting/Internal/FormattingHelpers.cs

+        public static long DivMod(long numerator, long denominator, out long modulo)
+        {
+            long div = numerator / denominator;
+            modulo = numerator - (div * denominator);


Is there a perf difference between this and just using the % operator?

The % operator is really expensive. The compiler should do this for us (like the C++ compiler does), but it doesn't. This trick is actually coded into the Math library as a DivRem method, but I don't have access to it here. My testing shows this to be significantly faster.

ahsonkhan · 2017-04-12T00:39:44Z

src/System.Text.Primitives/System/Text/Formatting/Internal/FormattingHelpers.cs

+        #region Character counting helper methods
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public static int CountDigits(long n)


I tried this implementation and it is faster (esp for n < 10000000):

n = Math.Abs(n); int length = 1; while ((n /= 10) >= 1) { length++; } return length;

I saw different results on my dev machine. I might re-visit this. Though, if I change it, I will likely just unroll the loop completely.

ahsonkhan · 2017-04-12T00:43:44Z

src/System.Text.Primitives/System/Text/Formatting/Internal/FormattingHelpers.cs

+    // code must have already done all the necessary validation.
+    internal static class FormattingHelpers
+    {
+        private const int FractionDigits = 7;


Why is this 7? Can you explain what FractionDigits represents?

The spec for fractions on time formats is fixed at exactly 7 characters. I have no idea why this is, but it is part of most time specs. I'll add a comment to this.

Sounds good.

ahsonkhan · 2017-04-12T00:48:45Z

src/System.Text.Primitives/System/Text/Formatting/Internal/TimeFormatter.Utf16.cs

+            Unsafe.Add(ref utf16Bytes, 13) = Colon;
+
+            FormattingHelpers.WriteDigits(value.Minute, 2, ref utf16Bytes, 14);
+            Unsafe.Add(ref utf16Bytes, 16) = Colon;


Do we get 16 from adding 2 and 14 from the call to WriteDigits? If so, should this be a calculation stored in a variable?

digitCount = 2; index = 14; Unsafe.Add(ref utf16Bytes, digitCount + index) = Colon;

Extra variables means more IL instructions. These are constants and therefore can be optimized.

Alright. Maybe add a comment to make the relationship explicit then and explain what the constants are? I am just thinking to make it easier in terms of maintainability or code changes/refactoring.

ahsonkhan · 2017-04-12T00:53:10Z

src/System.Text.Primitives/System/Text/Formatting/Internal/TimeFormatter.Utf8.cs

+
+namespace System.Text
+{
+    internal static class InvariantUtf8TimeFormatter


Does it make sense to extract out the duplicate code between InvariantUtf16TimeFormatter and InvariantUtf8TimeFormatter into helpers?

They are fundamentally different enough that you can't merge them without using generics or some level of indirection since they operate using different sized characters and types. The point of separating them was to avoid indirection and abstraction.

As an example, for TryFormatG, aren't these parts of the methods exactly the same (aside from the difference that one takes a ref char while the other takes ref byte)? Could we extract that out into helpers?

https://github.com/shiftylogic/corefxlab/blob/9b46b6344f2feae9e4ffdff1fc074ec42203dad3/src/System.Text.Primitives/System/Text/Formatting/Internal/TimeFormatter.Utf8.cs#L76-L113

https://github.com/shiftylogic/corefxlab/blob/9b46b6344f2feae9e4ffdff1fc074ec42203dad3/src/System.Text.Primitives/System/Text/Formatting/Internal/TimeFormatter.Utf16.cs#L78-L115

Aside from that, the other difference is:
bytesWritten = charsNeeded * sizeof(char);
and
Span<char> dst = buffer.NonPortableCast<byte, char>();

One writes char, the other byte. Without generics, how would you make this common? If they were each operating on the same sized span and writing the same data, it would be trivial.

ahsonkhan · 2017-04-12T00:57:11Z

src/System.Text.Primitives/System/Text/Formatting/PrimitiveFormatter_time.cs

                case 'G':
-                    return TryFormatDateTimeFormatG(value.DateTime, buffer, out bytesWritten, encoder);
+                    return TryFormatDateTimeFormatG(value.DateTime, offset, buffer, out bytesWritten, encoder);


If format.Symbol == 'G' and format.IsDefault == false, offset = NullOffset. Do we want that?

I have tests for the "default" case and this is passing based on what DateTimeOffset.ToString returns. For some reason, the system DateTime.ToString "default" (though it is documented as 'G') actually does something slightly different than explicitly using 'G'. I don't claim to understand why, but these APIs now follow exactly what the system does in all the cases we support.

That seems odd, but I suppose we should match the DateTime.ToString behaviour.
I wonder if the source can provide some insight (I haven't looked deeply into this, but for reference):
http://referencesource.microsoft.com/#mscorlib/system/globalization/datetimeformat.cs,908

My implementations are derived from that code. That is how I found the difference. For some reason, they treat the default (i.e. no format specifier) as a modified 'G'.

ahsonkhan · 2017-04-12T01:02:02Z

tests/System.Text.Primitives.Tests/Formatting/TimeTests.cs

+            Assert.Equal(expected, actual);
+        }
+
+        static readonly Random Rnd = new Random();


Use a seed value for consistency between runs.

I thought about that, but I don't know if I like it. This will have better coverage over time, and in failure cases, the logs will have enough information to replicate the failure if we end up hitting a bad case.

I could be swayed, if this is something we MUST do.

I think it makes sense to keep test runs consistent. If the test does catch a bug, we want it to be deterministic and always fail rather than seeing intermittent failures which could cause confusion (scenario: seeing a red in CI, re-test assuming some CI infra issue, and it goes green). If we want higher coverage, should we be explicit in our test cases and brute force a large input set?

better coverage over time

I suppose that would be OK but imo being explicit in the edge cases/etc would be better.

ahsonkhan · 2017-04-12T01:03:40Z

src/System.Text.Primitives/System/Text/Formatting/Internal/UuidFormatter.Utf16.cs

+
+namespace System.Text
+{
+    internal static class InvariantUtf16UuidFormatter


Rename as GUID instead of UUID?

ahsonkhan · 2017-04-12T01:05:29Z

Otherwise, LGTM.

ahsonkhan · 2017-04-12T18:29:16Z

tests/System.Text.Primitives.Tests/Formatting/UuidTests.cs

+
+namespace System.Text.Primitives.Tests
+{
+    public class UuidTests


Rename as guid? Along with filename.

KrzysztofCwalina · 2017-04-14T18:48:12Z

src/System.Text.Primitives/System/Text/Formatting/PrimitiveFormatter.Guid.cs

+            else if (encoder.IsInvariantUtf16)
+                return InvariantUtf16GuidFormatter.TryFormat(value, buffer, out bytesWritten, format);
+            else
+                throw new NotImplementedException();


It would be good to have issues filed (or some other tracking mechanism) for all the NotImplementedExceptions.

This should already be covered by issue #57. There are variants of that issue for parsing and encoding as well.

Robert Anderson added 16 commits April 7, 2017 09:43

Primitive formatting optimizations

28f525f

Old implementations for reference; will remove later

0555f46

Invariant specific implementations for UTF-8 and UTF-16; removed old …

b22c72e

…versions; code structure cleanup

Adding faster formatting for TimeSpan; some cleanup

b18a0dd

Removing some dead code

16bd180

Setting System.Runtime.CompilerServices.Unsafe version correctly

1fd44fd

DateTimeOffset format for default needs offset

8627a7d

Fixing composite formatting tests after time / uuid formatting changes

19e0a82

Fixing a corner case in TimeSpan formatting for TimeSpan.MinValue

374fab0

Patching tests: we only support invariant culture right now.

5d8f629

Removing an unneeded project reference; Small tweak to a comment.

4f85594

Merge branch 'master' of github.com:dotnet/corefxlab into working/format

ed06824

Re-structuring test file layout

3ae92cb

Adding tests for primitive time and uuid formatting

32fa49e

Removing an unnecessary helper method.

25c27c3

Adding a missing file license header

9b46b63

dnfclas added the cla-already-signed label Apr 11, 2017

ahsonkhan reviewed Apr 12, 2017

View reviewed changes

Robert Anderson added 2 commits April 12, 2017 09:03

Added a few comments; small tweaks from CR.

fb065a4

Seeding the randomness of the time tests

8a79e80

ahsonkhan reviewed Apr 12, 2017

View reviewed changes

ahsonkhan approved these changes Apr 12, 2017

View reviewed changes

Simple rename (Uuid => Guid)

d8d6e95

shiftylogic merged commit 156e818 into dotnet:master Apr 12, 2017

KrzysztofCwalina reviewed Apr 14, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Primitive formatting enhancements for Time and Uuid #1462

Primitive formatting enhancements for Time and Uuid #1462

shiftylogic commented Apr 11, 2017

ahsonkhan Apr 12, 2017

shiftylogic Apr 12, 2017

ahsonkhan Apr 12, 2017

shiftylogic Apr 12, 2017

ahsonkhan Apr 12, 2017 •

edited

Loading

shiftylogic Apr 12, 2017

ahsonkhan Apr 12, 2017

shiftylogic Apr 12, 2017

ahsonkhan Apr 12, 2017

ahsonkhan Apr 12, 2017

shiftylogic Apr 12, 2017

ahsonkhan Apr 12, 2017

ahsonkhan Apr 12, 2017

shiftylogic Apr 12, 2017

ahsonkhan Apr 12, 2017 •

edited

Loading

shiftylogic Apr 12, 2017

ahsonkhan Apr 12, 2017

shiftylogic Apr 12, 2017

ahsonkhan Apr 12, 2017

shiftylogic Apr 12, 2017

ahsonkhan Apr 12, 2017

shiftylogic Apr 12, 2017

ahsonkhan Apr 12, 2017

ahsonkhan Apr 12, 2017

ahsonkhan commented Apr 12, 2017

ahsonkhan Apr 12, 2017

KrzysztofCwalina Apr 14, 2017

shiftylogic Apr 14, 2017

Primitive formatting enhancements for Time and Uuid #1462

Primitive formatting enhancements for Time and Uuid #1462

Conversation

shiftylogic commented Apr 11, 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

ahsonkhan Apr 12, 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

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

ahsonkhan Apr 12, 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

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

ahsonkhan commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Apr 12, 2017 •

edited

Loading

ahsonkhan Apr 12, 2017 •

edited

Loading