Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor some DateTime and TimeSpan formatting/parsing methods #101640

Merged
merged 15 commits into from May 6, 2024
Merged

Refactor some DateTime and TimeSpan formatting/parsing methods #101640

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1c1f006
Refactor some DateTime and TimeSpan formatting/parsing methods
lilinus Apr 27, 2024
001ee0a
Fix assertion in TimeSpanParse.Pow10
lilinus Apr 27, 2024
7800524
Merge branch 'main' into dt-ts-format-parse-helpers
lilinus Apr 27, 2024
decf099
Don't use Unsafe in TimeSpanParse.Pow10
lilinus Apr 27, 2024
267d5e8
Revert changes to TimeSpanParse.Pow10
lilinus Apr 27, 2024
61863e3
Revert "Revert changes to TimeSpanParse.Pow10"
lilinus Apr 27, 2024
95857be
Change method name to Pow10UpToMaxFractionDigits
lilinus Apr 27, 2024
2fe27c1
Fix TimeSpanParse.TimeSpanToken.NormalizeAndValidateFraction
lilinus Apr 28, 2024
dd64b42
Address feedback in TimeSpanParse
lilinus May 2, 2024
5e171ed
Merge branch 'main' into dt-ts-format-parse-helpers
lilinus May 2, 2024
0c04b2f
Change from Math.Round to uint divison in TimeSpanParse.NormalizeAndV…
lilinus May 3, 2024
3cf992c
Merge branch 'main' into dt-ts-format-parse-helpers
lilinus May 3, 2024
e455150
Comment for rounding division in TimeSpanParse.NormalizeAndValidateFr…
lilinus May 5, 2024
4e074fd
Merge branch 'main' into dt-ts-format-parse-helpers
lilinus May 5, 2024
ef86e5a
Update src/libraries/System.Private.CoreLib/src/System/Globalization/…
lilinus May 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeFormat.cs
View file
Open in desktop
Expand Up @@ -484,11 +484,11 @@ private static bool IsUseGenitiveForm(ReadOnlySpan<char> format, int index, int
tokenLen = ParseRepeatPattern(format, i, ch);
if (tokenLen <= MaxSecondsFractionDigits)
{
long fraction = (dateTime.Ticks % Calendar.TicksPerSecond);
fraction /= (long)Math.Pow(10, 7 - tokenLen);
int fraction = (int)(dateTime.Ticks % Calendar.TicksPerSecond);
fraction /= TimeSpanParse.Pow10UpToMaxFractionDigits(MaxSecondsFractionDigits - tokenLen);
if (ch == 'f')
{
FormatFraction(ref result, (int)fraction, fixedNumberFormats[tokenLen - 1]);
FormatFraction(ref result, fraction, fixedNumberFormats[tokenLen - 1]);
}
else
{
Expand All @@ -507,7 +507,7 @@ private static bool IsUseGenitiveForm(ReadOnlySpan<char> format, int index, int
}
if (effectiveDigits > 0)
{
FormatFraction(ref result, (int)fraction, fixedNumberFormats[effectiveDigits - 1]);
FormatFraction(ref result, fraction, fixedNumberFormats[effectiveDigits - 1]);
}
else
{
Expand Down
3 changes: 2 additions & 1 deletion src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeParse.cs
View file
Open in desktop
Expand Up @@ -3179,6 +3179,7 @@ internal static bool ParseDigits(ref __DTString str, int minDigitLen, int maxDig

private static bool ParseFractionExact(ref __DTString str, int maxDigitLen, scoped ref double result)
{
Debug.Assert(maxDigitLen <= DateTimeFormat.MaxSecondsFractionDigits);
if (!str.GetNextDigit())
{
str.Index--;
Expand All @@ -3197,7 +3198,7 @@ private static bool ParseFractionExact(ref __DTString str, int maxDigitLen, scop
result = result * 10 + str.GetDigit();
}

result /= TimeSpanParse.Pow10(digitLen);
result /= TimeSpanParse.Pow10UpToMaxFractionDigits(digitLen);
return digitLen == maxDigitLen;
}

Expand Down
10 changes: 5 additions & 5 deletions src/libraries/System.Private.CoreLib/src/System/Globalization/TimeSpanFormat.cs
View file
Open in desktop
Expand Up @@ -311,7 +311,7 @@ internal enum StandardFormat
int seconds = (int)(time / TimeSpan.TicksPerSecond % 60);
int fraction = (int)(time % TimeSpan.TicksPerSecond);

long tmp;
int tmp;
int i = 0;
int tokenLen;

Expand Down Expand Up @@ -356,8 +356,8 @@ internal enum StandardFormat
}

tmp = fraction;
tmp /= TimeSpanParse.Pow10(DateTimeFormat.MaxSecondsFractionDigits - tokenLen);
DateTimeFormat.FormatFraction(ref result, (int)tmp, DateTimeFormat.fixedNumberFormats[tokenLen - 1]);
tmp /= TimeSpanParse.Pow10UpToMaxFractionDigits(DateTimeFormat.MaxSecondsFractionDigits - tokenLen);
DateTimeFormat.FormatFraction(ref result, tmp, DateTimeFormat.fixedNumberFormats[tokenLen - 1]);
break;
case 'F':
//
Expand All @@ -370,7 +370,7 @@ internal enum StandardFormat
}

tmp = fraction;
tmp /= TimeSpanParse.Pow10(DateTimeFormat.MaxSecondsFractionDigits - tokenLen);
tmp /= TimeSpanParse.Pow10UpToMaxFractionDigits(DateTimeFormat.MaxSecondsFractionDigits - tokenLen);
int effectiveDigits = tokenLen;
while (effectiveDigits > 0)
{
Expand All @@ -386,7 +386,7 @@ internal enum StandardFormat
}
if (effectiveDigits > 0)
{
DateTimeFormat.FormatFraction(ref result, (int)tmp, DateTimeFormat.fixedNumberFormats[effectiveDigits - 1]);
DateTimeFormat.FormatFraction(ref result, tmp, DateTimeFormat.fixedNumberFormats[effectiveDigits - 1]);
}
break;
case 'd':
Expand Down
43 changes: 27 additions & 16 deletions src/libraries/System.Private.CoreLib/src/System/Globalization/TimeSpanParse.cs
View file
Open in desktop
Expand Up @@ -47,6 +47,7 @@
//
////////////////////////////////////////////////////////////////////////////

using System.Buffers.Text;
using System.Diagnostics;
using System.Text;

Expand Down Expand Up @@ -114,7 +115,7 @@ public bool NormalizeAndValidateFraction()
if (_zeroes == 0 && _num > MaxFraction)
return false;

int totalDigitsCount = ((int)Math.Floor(Math.Log10(_num))) + 1 + _zeroes;
int totalDigitsCount = FormattingHelpers.CountDigits((uint)_num) + _zeroes;

if (totalDigitsCount == MaxFractionDigits)
{
Expand All @@ -132,7 +133,7 @@ public bool NormalizeAndValidateFraction()
// .000001 normalize to 10 ticks
// .1 normalize to 1,000,000 ticks

_num *= (int)Pow10(MaxFractionDigits - totalDigitsCount);
_num *= Pow10UpToMaxFractionDigits(MaxFractionDigits - totalDigitsCount);
return true;
}

Expand All @@ -143,7 +144,17 @@ public bool NormalizeAndValidateFraction()
// .099999999 normalize to 1,000,000 ticks

Debug.Assert(_zeroes > 0); // Already validated that in the condition _zeroes == 0 && _num > MaxFraction
_num = (int)Math.Round((double)_num / Pow10(totalDigitsCount - MaxFractionDigits), MidpointRounding.AwayFromZero);

if (_zeroes > MaxFractionDigits)
{
// If there are 8 leading zeroes, it rounds to zero
_num = 0;
return true;
}

Debug.Assert(totalDigitsCount - MaxFractionDigits <= MaxFractionDigits);
uint power = (uint)Pow10UpToMaxFractionDigits(totalDigitsCount - MaxFractionDigits);
_num = (int)(((uint)_num + power / 2) / power);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_num = (int)(((uint)_num + power / 2) / power);

I am not sure if this dividing by 2 and dividing by power will be more performant than just calling Math.Round?
@tannergooding any opinion on that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

    const int MaxFractionDigits = 7;

    [Params(123456789)]
    public int Num { get; set; }

    [Params(10)]
    public int TotalDigitsCount { get; set; }

    internal static int Pow10UpToMaxFractionDigits(int pow)
    {
        ReadOnlySpan<int> powersOfTen =
        [
            1,
            10,
            100,
            1000,
            10000,
            100000,
            1000000,
            10000000,
        ];
        Debug.Assert(powersOfTen.Length == MaxFractionDigits + 1);
        return powersOfTen[pow];
    }

    [Benchmark]
    public int MathRound() => (int)Math.Round((double)Num / Pow10UpToMaxFractionDigits(TotalDigitsCount - MaxFractionDigits), MidpointRounding.AwayFromZero);

    [Benchmark]
    public int IntegerRound()
    {
        uint power = (uint)Pow10UpToMaxFractionDigits(TotalDigitsCount - MaxFractionDigits);
        return (int)(((uint)Num + power / 2) / power);
    }
Method Num TotalDigitsCount Mean Error StdDev
MathRound 123456789 10 0.8892 ns 0.0068 ns 0.0061 ns
IntegerRound 123456789 10 1.2103 ns 0.0044 ns 0.0034 ns

Interesting...I suppose floating point math could be much slower than integer.

Copy link
Member

@tarekgh tarekgh Apr 29, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest reverting the code back to use Math.Round. It is even more readable and clear the intention of what the code does.

Copy link
Contributor

@xtqqczze xtqqczze Apr 30, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Results on arm64 with the span bounds check removed:

BenchmarkDotNet v0.13.12, macOS Sonoma 14.4.1 (23E224) [Darwin 23.4.0]
Apple M2, 1 CPU, 8 logical and 8 physical cores
.NET SDK 8.0.204
[Host] : .NET 8.0.4 (8.0.424.16909), Arm64 RyuJIT AdvSIMD
DefaultJob : .NET 8.0.4 (8.0.424.16909), Arm64 RyuJIT AdvSIMD

Method Num TotalDigitsCount Mean Error StdDev
MathRound 123456789 10 0.4477 ns 0.0105 ns 0.0093 ns
IntegerRound 123456789 10 0.1372 ns 0.0037 ns 0.0032 ns

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Results on arm64 with the span bounds check removed:

Can you provide a public API benchmark that shows any noticeable perf difference? IMO, It doesn't make sense to benchmark internals.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EgorBo Not sure about real-world benchmarks, but with the suggested change code size (on X64) is 172 bytes vs is 271 bytes.

https://csharp.godbolt.org/z/YcMKW5fza

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lilinus is it possible you run your perf tests again mentioned in #101640 (comment) with @xtqqczze suggestion to get some idea about the perf of this change? Thanks!

I am talking about the change

uint power = (uint)Pow10UpToMaxFractionDigits(totalDigitsCount - MaxFractionDigits);
_num = (int)(((uint)_num + power / 2) / power);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like uint division is slightly faster. I assume you want me to push back to that change so I will do it.

| Method         | Job        | Toolchain     | arg                      | Mean     | Error   | StdDev  | Median   | Min      | Max      | Ratio | Allocated | Alloc Ratio |
|--------------- |----------- |---------------|------------------------- |---------:|--------:|--------:|---------:|---------:|---------:|------:|----------:|------------:|
| TimeSpan_Parse | Job-PGUVPC | main          | 00:00:00.000000001234567 | 215.0 ns | 0.90 ns | 0.84 ns | 215.0 ns | 213.3 ns | 216.7 ns |  1.00 |         - |          NA |
| TimeSpan_Parse | Job-AKPZDD | uint division | 00:00:00.000000001234567 | 154.5 ns | 0.43 ns | 0.36 ns | 154.4 ns | 154.1 ns | 155.4 ns |  0.72 |         - |          NA |
| TimeSpan_Parse | Job-RERBPQ | Mth.Round     | 00:00:00.000000001234567 | 155.8 ns | 0.40 ns | 0.35 ns | 155.7 ns | 155.1 ns | 156.4 ns |  0.72 |         - |          NA |
| TimeSpan_Parse | Job-PGUVPC | main          | 00:00:00.00001234567     | 156.1 ns | 0.48 ns | 0.43 ns | 155.8 ns | 155.6 ns | 157.0 ns |  1.00 |         - |          NA |
| TimeSpan_Parse | Job-AKPZDD | uint division | 00:00:00.00001234567     | 147.1 ns | 0.72 ns | 0.68 ns | 146.7 ns | 146.3 ns | 148.4 ns |  0.94 |         - |          NA |
| TimeSpan_Parse | Job-RERBPQ | Math.Round    | 00:00:00.00001234567     | 151.3 ns | 0.53 ns | 0.50 ns | 151.4 ns | 150.2 ns | 151.9 ns |  0.97 |         - |          NA |

Benchmark source:

public static IEnumerable<object> Values()
{
	yield return "00:00:00.00001234567";
	yield return "00:00:00.000000001234567";
}
[Benchmark, ArgumentsSource(nameof(Values))]
public TimeSpan TimeSpan_Parse(string arg) => TimeSpan.Parse(arg, CultureInfo.InvariantCulture);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lilinus looks you already did :-) I am fine with that as tt provides us with a smaller code size and a slightly faster performance as well.

One last ask, please add a comment above that line telling what is does to be easier for anyone reading the code understand what the code is doing.

@xtqqczze thanks for the suggestion!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added 👍

Debug.Assert(_num < MaxFraction);

return true;
Expand Down Expand Up @@ -563,20 +574,20 @@ internal bool SetBadFormatSpecifierFailure(char? formatSpecifierCharacter = null
}
}

internal static long Pow10(int pow)
internal static int Pow10UpToMaxFractionDigits(int pow)
{
return pow switch
{
0 => 1,
1 => 10,
2 => 100,
3 => 1000,
4 => 10000,
5 => 100000,
6 => 1000000,
7 => 10000000,
_ => (long)Math.Pow(10, pow),
};
ReadOnlySpan<int> powersOfTen = [
tarekgh marked this conversation as resolved.
Show resolved Hide resolved
1,
10,
100,
1000,
10000,
100000,
1000000,
10000000,
];
Debug.Assert(powersOfTen.Length == MaxFractionDigits + 1);
return powersOfTen[pow];
}

private static bool TryTimeToTicks(bool positive, TimeSpanToken days, TimeSpanToken hours, TimeSpanToken minutes, TimeSpanToken seconds, TimeSpanToken fraction, out long result)
Expand Down