Refactor some DateTime and TimeSpan formatting/parsing methods #101640

lilinus · 2024-04-27T12:53:05Z

Use FormattingHelpers.CountDigits.
Use TimeSpanParse.Pow10 in DateTimeFormat.
Change fraction variables from long to int.

dotnet-policy-service · 2024-04-27T12:53:29Z

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

src/libraries/System.Private.CoreLib/src/System/Globalization/TimeSpanParse.cs

This reverts commit 267d5e8.

src/libraries/System.Private.CoreLib/src/System/Globalization/TimeSpanParse.cs

danmoseley · 2024-04-28T02:45:04Z

Are these sufficiently covered in dotnet/performance? Might be worth running appropriate benchmarks there.

tarekgh · 2024-04-28T17:01:06Z

src/libraries/System.Private.CoreLib/src/System/Globalization/TimeSpanParse.cs

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


_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?

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.

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

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

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.

@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

@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);

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);

@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!

Comment added 👍

src/libraries/System.Private.CoreLib/src/System/Globalization/TimeSpanParse.cs

tarekgh

@lilinus thanks for helping with this change.

tarekgh · 2024-05-02T15:29:19Z

@lilinus could you please merge the latest change from main to this PR to be able to proceed?

lilinus · 2024-05-02T17:18:24Z

@tarekgh, done. Thanks for the feedback etc. I also tried benchmarking the changes but had trouble with antivirus stopping the runtime.

tarekgh · 2024-05-02T17:47:03Z

I also tried benchmarking the changes but had trouble with antivirus stopping the runtime.

Is it possible you can temporary disable the antivirus? Or add exclusion to the runtime and test folders to have the antivirus ignore them?

lilinus · 2024-05-02T20:50:25Z

Managed to get it working. Benchmarked some DateTime and TimeSpan parsing. Results are looking good IMO. Link to benchmarks: here

BenchmarkDotNet v0.13.13-nightly.20240311.145, Windows 10 (10.0.19044.3086/21H2/November2021Update)
AMD Ryzen 9 4900HS with Radeon Graphics, 1 CPU, 16 logical and 8 physical cores
.NET SDK 9.0.100-preview.3.24204.13
  [Host]     : .NET 9.0.0 (9.0.24.17209), X64 RyuJIT AVX2
  Job-GLLMPZ : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2
  Job-AQRKOX : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2

PowerPlanMode=00000000-0000-0000-0000-000000000000  IterationTime=250ms  MaxIterationCount=20
MinIterationCount=15  WarmupCount=1

| Method          | Job        | Toolchain                                                                                                | arg1                      | arg2                                  | Mean      | Error    | StdDev   | Median    | Min       | Max       | Ratio | RatioSD | Gen0   | Allocated | Alloc Ratio |
| TimeSpan_Parse  | Job-GLLMPZ | \dnmain\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe | 0.00.00.00.123            | d\\.hh\\.mm\\.ss\\.FFFFF              |  78.58 ns | 1.200 ns | 1.123 ns |  78.43 ns |  76.52 ns |  80.90 ns |  1.00 |    0.02 |      - |         - |          NA |
| TimeSpan_Parse  | Job-AQRKOX | \dnrt\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe   | 0.00.00.00.123            | d\\.hh\\.mm\\.ss\\.FFFFF              |  64.23 ns | 0.889 ns | 0.831 ns |  64.38 ns |  62.55 ns |  65.61 ns |  0.82 |    0.02 |      - |         - |          NA |
| TimeSpan_Parse  | Job-GLLMPZ | \dnmain\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe | 0.00.00.00.12345          | d\\.hh\\.mm\\.ss\\.fffff              |  72.58 ns | 0.264 ns | 0.234 ns |  72.57 ns |  72.30 ns |  73.05 ns |  1.00 |    0.00 |      - |         - |          NA |
| TimeSpan_Parse  | Job-AQRKOX | \dnrt\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe   | 0.00.00.00.12345          | d\\.hh\\.mm\\.ss\\.fffff              |  63.92 ns | 0.693 ns | 0.614 ns |  63.78 ns |  62.92 ns |  64.95 ns |  0.88 |    0.01 |      - |         - |          NA |
| DateTime_Format | Job-GLLMPZ | \dnmain\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe | 01/01/0001 00:00:12       | yyyy\\-MM\\-dd\\.HH\\.mm\\.ss\\.fff   | 213.02 ns | 0.465 ns | 0.388 ns | 212.95 ns | 212.32 ns | 213.69 ns |  1.00 |    0.00 | 0.0337 |      72 B |        1.00 |
| DateTime_Format | Job-AQRKOX | \dnrt\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe   | 01/01/0001 00:00:12       | yyyy\\-MM\\-dd\\.HH\\.mm\\.ss\\.fff   | 179.06 ns | 0.303 ns | 0.268 ns | 179.04 ns | 178.52 ns | 179.53 ns |  0.84 |    0.00 | 0.0343 |      72 B |        1.00 |
| DateTime_Format | Job-GLLMPZ | \dnmain\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe | 01/01/0001 00:00:12       | yyyy\\-MM\\-dd\\.HH\\.mm\\.ss\\.FFFFF | 146.01 ns | 0.579 ns | 0.542 ns | 145.84 ns | 145.43 ns | 147.21 ns |  1.00 |    0.01 | 0.0305 |      64 B |        1.00 |
| DateTime_Format | Job-AQRKOX | \dnrt\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe   | 01/01/0001 00:00:12       | yyyy\\-MM\\-dd\\.HH\\.mm\\.ss\\.FFFFF | 109.59 ns | 0.142 ns | 0.126 ns | 109.61 ns | 109.28 ns | 109.79 ns |  0.75 |    0.00 | 0.0304 |      64 B |        1.00 |
| TimeSpan_Format | Job-GLLMPZ | \dnmain\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe | 00:00:12.3456789          | d\\.hh\\.mm\\.ss\\.fffffff            | 177.14 ns | 0.506 ns | 0.473 ns | 177.02 ns | 176.46 ns | 178.08 ns |  1.00 |    0.00 | 0.0303 |      64 B |        1.00 |
| TimeSpan_Format | Job-AQRKOX | \dnrt\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe   | 00:00:12.3456789          | d\\.hh\\.mm\\.ss\\.fffffff            | 164.35 ns | 0.436 ns | 0.407 ns | 164.33 ns | 163.68 ns | 165.22 ns |  0.93 |    0.00 | 0.0301 |      64 B |        1.00 |
| TimeSpan_Format | Job-GLLMPZ | \dnmain\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe | 00:00:12.3456789          | d\\.hh\\.mm\\.ss\\.FFFFFFF            | 184.79 ns | 0.532 ns | 0.497 ns | 184.62 ns | 184.17 ns | 185.89 ns |  1.00 |    0.00 | 0.0300 |      64 B |        1.00 |
| TimeSpan_Format | Job-AQRKOX | \dnrt\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe   | 00:00:12.3456789          | d\\.hh\\.mm\\.ss\\.FFFFFFF            | 177.78 ns | 0.358 ns | 0.317 ns | 177.76 ns | 177.20 ns | 178.39 ns |  0.96 |    0.00 | 0.0305 |      64 B |        1.00 |
| DateTime_Parse  | Job-GLLMPZ | \dnmain\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe | 2024-05-02.21.40.42.123   | yyyy\\-MM\\-dd\\.HH\\.mm\\.ss\\.FFFFF | 225.67 ns | 0.378 ns | 0.335 ns | 225.61 ns | 225.21 ns | 226.24 ns |  1.00 |    0.00 |      - |         - |          NA |
| DateTime_Parse  | Job-AQRKOX | \dnrt\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe   | 2024-05-02.21.40.42.123   | yyyy\\-MM\\-dd\\.HH\\.mm\\.ss\\.FFFFF | 202.20 ns | 0.250 ns | 0.209 ns | 202.22 ns | 201.87 ns | 202.51 ns |  0.90 |    0.00 |      - |         - |          NA |
| DateTime_Parse  | Job-GLLMPZ | \dnmain\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe | 2024-05-02.21.40.42.12345 | yyyy\\-MM\\-dd\\.HH\\.mm\\.ss\\.fffff | 219.44 ns | 0.367 ns | 0.325 ns | 219.45 ns | 218.83 ns | 220.02 ns |  1.00 |    0.00 |      - |         - |          NA |
| DateTime_Parse  | Job-AQRKOX | \dnrt\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe   | 2024-05-02.21.40.42.12345 | yyyy\\-MM\\-dd\\.HH\\.mm\\.ss\\.fffff | 218.17 ns | 0.361 ns | 0.320 ns | 218.18 ns | 217.31 ns | 218.65 ns |  0.99 |    0.00 |      - |         - |          NA |

…alidateFraction

…action

src/libraries/System.Private.CoreLib/src/System/Globalization/TimeSpanParse.cs

…TimeSpanParse.cs Co-authored-by: xtqqczze <45661989+xtqqczze@users.noreply.github.com>

…t#101640) * Refactor some DateTime and TimeSpan formatting/parsing methods * Fix assertion in TimeSpanParse.Pow10 * Don't use Unsafe in TimeSpanParse.Pow10 * Revert changes to TimeSpanParse.Pow10 * Revert "Revert changes to TimeSpanParse.Pow10" This reverts commit 267d5e8. * Change method name to Pow10UpToMaxFractionDigits * Fix TimeSpanParse.TimeSpanToken.NormalizeAndValidateFraction * Address feedback in TimeSpanParse * Change from Math.Round to uint divison in TimeSpanParse.NormalizeAndValidateFraction * Comment for rounding division in TimeSpanParse.NormalizeAndValidateFraction * Update src/libraries/System.Private.CoreLib/src/System/Globalization/TimeSpanParse.cs Co-authored-by: xtqqczze <45661989+xtqqczze@users.noreply.github.com> --------- Co-authored-by: xtqqczze <45661989+xtqqczze@users.noreply.github.com>

lilinus added 2 commits April 27, 2024 14:40

Refactor some DateTime and TimeSpan formatting/parsing methods

1c1f006

Fix assertion in TimeSpanParse.Pow10

001ee0a

dotnet-issue-labeler bot added the area-System.Globalization label Apr 27, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 27, 2024

Merge branch 'main' into dt-ts-format-parse-helpers

7800524

EgorBo reviewed Apr 27, 2024

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/TimeSpanParse.cs Outdated Show resolved Hide resolved

lilinus added 3 commits April 27, 2024 15:25

Don't use Unsafe in TimeSpanParse.Pow10

decf099

Revert changes to TimeSpanParse.Pow10

267d5e8

Revert "Revert changes to TimeSpanParse.Pow10"

61863e3

This reverts commit 267d5e8.

tarekgh reviewed Apr 27, 2024

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/TimeSpanParse.cs Outdated Show resolved Hide resolved

tarekgh added this to the 9.0.0 milestone Apr 27, 2024

Change method name to Pow10UpToMaxFractionDigits

95857be

xtqqczze reviewed Apr 27, 2024

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/TimeSpanParse.cs Outdated Show resolved Hide resolved

Fix TimeSpanParse.TimeSpanToken.NormalizeAndValidateFraction

2fe27c1

tarekgh reviewed Apr 28, 2024

View reviewed changes

huoyaoyuan reviewed Apr 29, 2024

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/TimeSpanParse.cs Outdated Show resolved Hide resolved

Address feedback in TimeSpanParse

dd64b42

tarekgh marked this pull request as ready for review May 2, 2024 15:26

tarekgh approved these changes May 2, 2024

View reviewed changes

build-analysis bot mentioned this pull request May 2, 2024

System.Numerics.Tensors.Tests.SingleGenericTensorPrimitives.SpanScalarDestination_SpecialValues fails #101721

Closed

Merge branch 'main' into dt-ts-format-parse-helpers

5e171ed

lilinus and others added 2 commits May 3, 2024 10:51

Change from Math.Round to uint divison in TimeSpanParse.NormalizeAndV…

0c04b2f

…alidateFraction

Merge branch 'main' into dt-ts-format-parse-helpers

3cf992c

build-analysis bot mentioned this pull request May 3, 2024

Timeout in System.Net.Quic.Functional.Tests #86019

Open

lilinus and others added 2 commits May 5, 2024 12:00

Comment for rounding division in TimeSpanParse.NormalizeAndValidateFr…

e455150

…action

Merge branch 'main' into dt-ts-format-parse-helpers

4e074fd

xtqqczze reviewed May 5, 2024

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/TimeSpanParse.cs Outdated Show resolved Hide resolved

Update src/libraries/System.Private.CoreLib/src/System/Globalization/…

ef86e5a

…TimeSpanParse.cs Co-authored-by: xtqqczze <45661989+xtqqczze@users.noreply.github.com>

build-analysis bot mentioned this pull request May 6, 2024

Test failure in System.Numerics.Tensors.Tests.SingleGenericTensorPrimitives.SpanDestinationFunctions_SpecialValues #101731

Open

tarekgh merged commit c81eca8 into dotnet:main May 6, 2024
144 of 146 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor some DateTime and TimeSpan formatting/parsing methods #101640

Refactor some DateTime and TimeSpan formatting/parsing methods #101640

lilinus commented Apr 27, 2024 •

edited

dotnet-policy-service bot commented Apr 27, 2024

danmoseley commented Apr 28, 2024

tarekgh Apr 28, 2024

huoyaoyuan Apr 29, 2024

tarekgh Apr 29, 2024 •

edited

xtqqczze Apr 30, 2024 •

edited

EgorBo Apr 30, 2024

xtqqczze May 2, 2024

tarekgh May 2, 2024

lilinus May 3, 2024

tarekgh May 3, 2024

lilinus May 5, 2024

tarekgh left a comment

tarekgh commented May 2, 2024

lilinus commented May 2, 2024

tarekgh commented May 2, 2024

lilinus commented May 2, 2024

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

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

Refactor some DateTime and TimeSpan formatting/parsing methods #101640

Refactor some DateTime and TimeSpan formatting/parsing methods #101640

Conversation

lilinus commented Apr 27, 2024 • edited

dotnet-policy-service bot commented Apr 27, 2024

danmoseley commented Apr 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh Apr 29, 2024 • edited

Choose a reason for hiding this comment

xtqqczze Apr 30, 2024 • edited

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

tarekgh left a comment

Choose a reason for hiding this comment

tarekgh commented May 2, 2024

lilinus commented May 2, 2024

tarekgh commented May 2, 2024

lilinus commented May 2, 2024

lilinus commented Apr 27, 2024 •

edited

tarekgh Apr 29, 2024 •

edited

xtqqczze Apr 30, 2024 •

edited