30% optimization of `DateTime.GetDate()`/`.Year/.Month/.Day/.DayOfYear` by 'Euclidean affine functions' #72712

SergeiPavlov · 2022-07-23T08:17:58Z

This optimization based on article
Cassio Neri, Lorenz Schneider - Euclidean Affine Functions and Applications to Calendar Algorithms - 2021

It avoids loops and minimizes branching in calendar calculations.

Benchmarks:

|                                                     Method |      Mean | Error |       Min |       Max | Allocated |
|----------------------------------------------------------- |----------:|------:|----------:|----------:|----------:|
|           'DateTime.GetDate(out year, out month, out day)' |  7.697 ns |    NA |  7.697 ns |  7.697 ns |         - |
| 'Optimized DateTime.GetDate(out year, out month, out day)' |  5.114 ns |    NA |  5.114 ns |  5.114 ns |         - |


|                     Method |     Mean | Error |      Min |      Max | Allocated |
|--------------------------- |---------:|------:|---------:|---------:|----------:|
|              DateTime.Year | 4.289 ns |    NA | 4.289 ns | 4.289 ns |         - |
|             DateTime.Month | 6.534 ns |    NA | 6.534 ns | 6.534 ns |         - |
|               DateTime.Day | 6.946 ns |    NA | 6.946 ns | 6.946 ns |         - |
|  'Optimized DateTime.Year' | 2.945 ns |    NA | 2.945 ns | 2.945 ns |         - |
| 'Optimized DateTime.Month' | 3.161 ns |    NA | 3.161 ns | 3.161 ns |         - |
|   'Optimized DateTime.Day' | 3.638 ns |    NA | 3.638 ns | 3.638 ns |         - |

It is difficult to explain values of magic constants of the algorithm in comments.
They are inferred algebraically.

Get rid of GetDatePart() function and inline its functionality directly into Year, Month, Day, DayOfYear properties

…ions'

dotnet-issue-labeler · 2022-07-23T08:18:07Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

ghost · 2022-07-23T09:01:40Z

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

Issue Details

This optimization based on article
Cassio Neri, Lorenz Schneiderhttps - Euclidean Affine Functions and Applications to Calendar Algorithms - 2021

It avoids loops and minimizes branching in calendar calculations.

Benchmarks:

|                                                     Method |      Mean | Error |       Min |       Max | Allocated |
|----------------------------------------------------------- |----------:|------:|----------:|----------:|----------:|
|           'DateTime.GetDate(out year, out month, out day)' |  7.697 ns |    NA |  7.697 ns |  7.697 ns |         - |
| 'Optimized DateTime.GetDate(out year, out month, out day)' |  5.114 ns |    NA |  5.114 ns |  5.114 ns |         - |
|            '(DateTime.Year, DateTime.Month, DateTime.Day)' | 15.147 ns |    NA | 15.147 ns | 15.147 ns |         - |
|  'Optimized (DateTime.Year, DateTime.Month, DateTime.Day)' |  9.687 ns |    NA |  9.687 ns |  9.687 ns |         - |

It is difficult to explain values of magic constants of the algorithm in comments.
The are inferred algebraically.

Author:	SergeiPavlov
Assignees:	-
Labels:	`area-System.Runtime`, `community-contribution`
Milestone:	-

Clockwork-Muse · 2022-07-23T15:58:08Z

src/libraries/System.Private.CoreLib/src/System/DateTime.cs

+            (uint y400, uint r1) = Math.DivRem(((uint)(UTicks / TicksPer6Hours) | 3U) + 1224, DaysPer400Years);
+            ulong u2 = (ulong)Math.BigMul(2939745, (int)r1 | 3);
+            ushort daySinceMarch1 = (ushort)((uint)u2 / 11758980);
+            var n3 = 2141 * daySinceMarch1 + 197913;


... Some additional comments, especially around the specific magic numbers, would be greatly appreciated.

These magic number have not explainable meaning. They just give right result.
To understand them it is neccessary to follow chain of propositions in the article.

Then you should at least be noting the source inside the code. If there is any sort of naming for those constants in the source, you should probably be pulling them out into named constants as well.

I did in the function comment https://github.com/dotnet/runtime/pull/72712/files#diff-5afffb30d8c00fa852b7df6e5bce6255be93acc1281d20b308ab57e8b70f567bR1356

Authors of the algorithm don't use any constant naming in the article and their reference implementation https://github.com/cassioneri/calendar/blob/master/calendar.hpp#L285

src/libraries/System.Private.CoreLib/src/System/DateTime.cs

danmoseley · 2022-07-24T03:35:25Z

Do we have test coverage here sufficient to give high confidence? If you can think of any other useful test cases this PR is a good time to add them.

danmoseley · 2022-07-24T03:48:00Z

Maybe @lemire is interested in this trick.

…ear constant

SergeiPavlov · 2022-07-24T04:38:30Z

Do we have test coverage here sufficient to give high confidence? If you can think of any other useful test cases this PR is a good time to add them.

I tested it for every possible date (range 1/1/0001 .. 12/31/9999). Results are identical to current implementation.
Of course it does not replace testing in dotnet workflow.

I see the existing DateTime tests have good enough coverage:
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime/tests/System/DateTimeTests.cs

lemire · 2022-07-24T04:45:54Z

@danmoseley I knew of the article, but I am happy to see this work come to dotnet.

danmoseley · 2022-07-28T03:02:38Z

I've compared the paper to the code changes here and I can reproduce the calculations, so the code makes sense to me. (Although, I see r2 = u2 % 2^32 / 2939745 / 4, is turning into (ushort)((uint)u2 / 11758980) .. clearly produces the right result, but I would think precedence would run the divisions one after the other, rather than multiplying .. what am I missing about their notation..)

I added some more random tests locally and they all pass, which should not be a surprise since you say you've tried all possible valid dates yourself.

Adding [MethodImpl(MethodImplOptions.AggressiveInlining)] to GetDatePart() makes properties Year, Month, Day, DayOfYear significantly more efficient. The compiler discards unrelated branches of GetDatePart().

Would it make for clearer code, albeit with a bit more duplication, to manually inline the appropriate parts inside each of these? Then it will be self evident that the unrelated branches are discarded.

Re my question about perf tests, I guess this covers it indirectly. Did you run this test before/after? But, it would probably be good to add benchmarks in this file for Year, Month, Day, DayOfYear individually, especially if you end up copy/pasting instead of inlining as suggested above. (If you add benchmarks, that would of course be a PR into dotnet/performance that we'd merge after code flows, but you could run them locally and report here.)
https://github.com/dotnet/performance/blob/e60e729d0008d8522b518b11c06b9803f6d674ac/src/benchmarks/micro/libraries/System.Runtime/Perf.DateTime.cs#L34

danmoseley · 2022-07-28T03:05:25Z

@tannergooding in case he is interested in the math..

danmoseley · 2022-07-28T14:49:37Z

I notice we are missing tests for DayOfYear that use randomized inputs. Year, Month, Day are indirectly covered by the ToString() tests with randomized inputs

runtime/src/libraries/System.Runtime/tests/System/DateTimeTests.cs

Line 2149 in cc7ccfd

public static IEnumerable<object[]> ToString_MatchesExpected_MemberData()

Those ToString() tests indirectly test Year, Month, Day getters.

danmoseley · 2022-07-28T16:51:05Z

@richlander when we're using an algorithm from a paper like this, ie., there's no particular software license, I assume credit in a comment is fine, and no entry in THIRD-PARTY-NOTICES.TXT is expected?

…ead of Common GetDatePart() function

SergeiPavlov · 2022-07-28T17:39:45Z

Would it make for clearer code, albeit with a bit more duplication, to manually inline the appropriate parts inside each of these? Then it will be self evident that the unrelated branches are discarded.

Inlined them

SergeiPavlov · 2022-07-28T17:49:38Z

Re my question about perf tests, I guess this covers it indirectly. Did you run this test before/after? But, it would probably be good to add benchmarks in this file for Year, Month, Day, DayOfYear individually, especially if you end up copy/pasting instead of inlining as suggested above. (If you add benchmarks, that would of course be a PR into dotnet/performance that we'd merge after code flows, but you could run them locally and report here.)

Locally I have a source file with both: original & new implementations. The first benchmarking result were before applying [MethodImpl(MethodImplOptions.AggressiveInlining)].

Added individual benchmarks for the properties. Results arу in the PR description. The most significant gain (50%) is for Month which was most heavy because of loop.

danmoseley

LGTM - I didn't recheck the math in detail, if the tests pass.

As mentioned, DayOfYear coverage is relatively poor, while the others have good indirect coverage, it would be nice to add some tests with randomized data.

Thanks for the nice improvement!

danmoseley

on latest changes

danmoseley · 2022-07-28T21:37:00Z

@richlander is going to check on licensing here. Marking do not merge meantime. The change is otherwise fine.

richlander · 2022-07-28T23:11:45Z

Right. I sent mail to the authors of the paper + @SergeiPavlov.

cassioneri · 2022-07-31T10:55:41Z

Lorenz and I are glad to see that our algorithm might go into .NET. We wish to thank, especially, @SergeiPavlov for implementing and creating the PR.

On the licensing question sent by email, Lorenz and I have discussed the matter and we agreed with your second bullet point which I quote:

Agree that the code was primarily informed by the paper and that it is licensed permissively such that we can land the PR.

It is clear that Sergei did implement the algorithm as explained in the paper. In terms of implementation, we consider his C# code substantially different from the GPL-licensed C++ code found in my repository.

For the reasons above, as far as Lorenz and I are concerned, we do not believe that GPL applies to Sergei's PR. As I said above, we would like to see the PR merged into the .NET's main branch for the benefit of .NET users. We would appreciate (but not require) if the release notes could mention our work.

danmoseley · 2022-07-31T15:07:15Z

Appreciation to @cassioneri for the licensing. I will follow up with an addition to the third party notices, no need to reset CI.

AOT and tvOS timed out, but previously passed, and vanishingly likely to be relevant here.

richlander · 2022-07-31T15:27:12Z

We will add in release notes and a blog post (including the release blog post). Thanks much!

@JonDouglas

IDisposable · 2022-08-01T23:30:10Z

I've compared the paper to the code changes here and I can reproduce the calculations, so the code makes sense to me. (Although, I see r2 = u2 % 2^32 / 2939745 / 4, is turning into (ushort)((uint)u2 / 11758980) .. clearly produces the right result, but I would think precedence would run the divisions one after the other, rather than multiplying .. what am I missing about their notation..)

Ignoring the % 2^32 part... the basic math rule at hand here is that X / Y / Z === X / (Y * Z). The easiest way to grok this is an example: 64 / 4 / 4 -> left-to-right apply first divisions (64 / 4) / 4 == 16 / 4 -> apply remaining division 16 / 4 == 4... now let's pre-apply the second division (which can be thought of as scaling up the divisor of the first one... so 64 / ( 4 * 4) == 64 / 16 and then apply the first division so 64 / 16 == 4... same answer... and if we KNOW the right-side divisors are constant there's no reason not to pre-multiply.

In pure algebra:
X / Y / Z == a --> given
((X / Y) / Z) * Z = a * Z --> move over the Z by multiplying both sides by Z
(X / Y) = a * Z --> which isolates X / Y on left
(X / Y) * Y = a * Z * Y --> move over the Y by multiplying both sides by Y
X = a * Z * Y --> which isolates X
X / (Z * Y) = a * Z * Y / (Z * Y) --> now solve for a by dividing both sides by Z * Y
X / (Y * Z) = a --> reduce and swap Y and Z for neatness because multiplication is commutative
cogito ergo algebra

lemire · 2022-08-01T23:45:40Z

+1

SergeiPavlov · 2022-08-02T00:47:00Z

((X / Y) / Z) * Z = a * Z --> move over the Z by multiplying both sides by Z
(X / Y) = a * Z --> which isolates X / Y on left

Hm, this implication does not work for integer division in general case.
Counter-example:

X = 21
Y = 3
Z = 2
a = X / Y / Z = 3

((21 / 3) / 2) * 2 = 3 * 2
but
(21 / 3) ≠ 3 * 2

IDisposable · 2022-08-02T02:12:27Z

Certainly there are truncation errors for integers where significance is lost, that's why I was talking about it in algebraic form :(
Perhaps I should have moved Y over first to better match the potential truncations. In that case, the early steps are

((Y * (X / Y)) / Z = a * Y --> move over the Y by multiplying both sides by Y
(X / Z) = a * Y --> which isolates X / Z on left

... and so on... but doing that puts you at risk of integer overflow if Y * X overflows (even though the subsequent division by Y cancels it out...) which is why I did the whole thing symbolically.

SergeiPavlov · 2022-08-02T02:24:00Z

The proof for integer division:

let X = YA+B; X/Y = A = ZQ+R
where Y = (B+M), 1 <= M
Z = (R+N), 1 <= N

then:
YZ = (B+M)(R+N) = BR+BN+MR+MN

X = YA + B = Y(ZQ+R) + B = YZQ + YR + B = (YZ)Q + (B+M)R + B = (YZ)Q + BR + MR + B

X/(YZ) = ((YZ)Q + BR + MR + B) / YZ
= Q + (BR+MR+B)/YZ
= Q + (BR+MR+B)/(BR+BN+RM+MN) = Q because BR+MR+B < BR+BN+RM+MN always

X/(YZ) = Q = X/Y/Z

Q.E.D.

SergeiPavlov · 2022-08-02T02:50:17Z

((Y * (X / Y)) / Z = a * Y --> move over the Y by multiplying both sides by Y
(X / Z) = a * Y --> which isolates X / Z on left

again (Y * (X / Y) is not always the same as X
Rational numbers algebra rules are not applicable for integer arithmetics.

cassioneri · 2022-08-02T17:15:46Z

FWIW:

I agree with Sergei's proof provided that Y > 0 and Z > 0. (Otherwise, here is a counter-example: X = 10, Y = -3, A = -3, B = 1, M = -4, that is, 1 <= M is false.)
For X < 0 or Z < 0, the result X/(YZ) = X/Y/Z is still valid in C and C++ (where / means truncation towards 0) and the proof only involves the result already proven for X > 0 and Z > 0 and playing with sign rules of multiplication/division.
I don't know the exactly specification of / in C# but I bet that for positive operands they are the same as in C and C++ and that's all that matters in our case.
Here we can see that some C compilers can figure out the optimization and replace u2 / 2939745 / 4 with u2 / 11758980. That's why we did not do it in the paper -- we trusted our compiler would do it.
Unfortunately, the C# compiler can't do the same. (See here that in f there are 2 consecutive shrs -- right shifts -- that could be collapsed into a single one but it's not.) Hence the optimization must be done manually. (As Sergei did.)

EgorBo · 2022-08-02T17:28:25Z

Perf improvements dotnet/perf-autofiling-issues#6998

Nice!

Improvements on Ubuntu-arm64 dotnet/perf-autofiling-issues#7150
win-arm64: dotnet/perf-autofiling-issues#7143

DrewScoggins · 2022-08-09T16:41:36Z

Also improvements on
win-x64: dotnet/perf-autofiling-issues#7174
ubuntu-x64: dotnet/perf-autofiling-issues#7183
win-x86: dotnet/perf-autofiling-issues#7195
win-amd64: dotnet/perf-autofiling-issues#7220
alpine-x64: dotnet/perf-autofiling-issues#8105, dotnet/perf-autofiling-issues#8106

kunalspathak · 2022-08-16T16:54:46Z

dotnet/perf-autofiling-issues#7279

danmoseley · 2022-08-16T18:09:51Z

Unfortunately, the C# compiler can't do the same. (See here that in f there are 2 consecutive shrs -- right shifts -- that could be collapsed into a single one but it's not.) Hence the optimization must be done manually. (As Sergei did.)

Opened an issue for this. Thanks everyone for explaining the math :)

Optimize DateTime.GetDate()/.GetDatePart() by 'Euclidean affine funct…

5cc9503

…ions'

ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 23, 2022

teo-tsirpanis added the area-System.Runtime label Jul 23, 2022

Code style

4293bfd

Clockwork-Muse reviewed Jul 23, 2022

View reviewed changes

MichalPetryka reviewed Jul 23, 2022

View reviewed changes

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

jkotas reviewed Jul 24, 2022

View reviewed changes

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

SergeiPavlov added 2 commits July 23, 2022 21:18

Refactoring: avoid switch and 'var' keyword; add March1BasedDayOfNewY…

163509c

…ear constant

Fix build

dfd20c6

SergeiPavlov added 2 commits July 23, 2022 22:33

Add [MethodImpl(MethodImplOptions.AggressiveInlining)] to GetDatePart()

06cdd8f

revert relevant comments from original code

e5e0ded

Inline calculations into properties: Year, Month, Day, DayOfYear inst…

7503120

…ead of Common GetDatePart() function

danmoseley approved these changes Jul 28, 2022

View reviewed changes

SergeiPavlov added 2 commits July 28, 2022 11:08

Fix indentation

8f48de6

align comment

7a5531e

danmoseley approved these changes Jul 28, 2022

View reviewed changes

danmoseley added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 28, 2022

SergeiPavlov changed the title ~~30% optimization of DateTime.GetDate()/.GetDatePart() by 'Euclidean affine functions'~~ 30% optimization of DateTime.GetDate()/.Year/.Month/.Day/.DayOfYear by 'Euclidean affine functions' Jul 29, 2022

danmoseley removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 31, 2022

danmoseley merged commit 85beaec into dotnet:main Jul 31, 2022

danmoseley mentioned this pull request Jul 31, 2022

Add TPN entry for Date/Time processing algorithm #73128

Merged

This was referenced Aug 3, 2022

Optimize DateTime.Year/.DayOfYear properties; fix comments #73277

Merged

Benchmarks for DateTime.Day/.Month/.Year/.DayOfYear dotnet/performance#2549

Merged

danmoseley mentioned this pull request Aug 16, 2022

Unnecessary shr emitted for repeated multiplication #74020

Open

SergeiPavlov deleted the optimize_DateTime_GetDate branch August 16, 2022 21:57

danmoseley mentioned this pull request Aug 20, 2022

System.Tests.Perf_DateTime.GetNow gets slower every summer due to daylight savings #67932

Open

ghost locked as resolved and limited conversation to collaborators Oct 2, 2022

jeffhandley added area-System.DateTime and removed area-System.Runtime labels Nov 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

30% optimization of `DateTime.GetDate()`/`.Year/.Month/.Day/.DayOfYear` by 'Euclidean affine functions' #72712

30% optimization of `DateTime.GetDate()`/`.Year/.Month/.Day/.DayOfYear` by 'Euclidean affine functions' #72712

SergeiPavlov commented Jul 23, 2022 •

edited

Loading

dotnet-issue-labeler bot commented Jul 23, 2022

ghost commented Jul 23, 2022

Clockwork-Muse Jul 23, 2022

SergeiPavlov Jul 23, 2022 •

edited

Loading

Clockwork-Muse Jul 23, 2022

SergeiPavlov Jul 23, 2022 •

edited

Loading

danmoseley commented Jul 24, 2022

danmoseley commented Jul 24, 2022

SergeiPavlov commented Jul 24, 2022 •

edited

Loading

lemire commented Jul 24, 2022

danmoseley commented Jul 28, 2022 •

edited

Loading

danmoseley commented Jul 28, 2022

danmoseley commented Jul 28, 2022

danmoseley commented Jul 28, 2022

SergeiPavlov commented Jul 28, 2022

SergeiPavlov commented Jul 28, 2022 •

edited

Loading

danmoseley left a comment

danmoseley left a comment

danmoseley commented Jul 28, 2022

richlander commented Jul 28, 2022

cassioneri commented Jul 31, 2022 •

edited

Loading

danmoseley commented Jul 31, 2022

richlander commented Jul 31, 2022

IDisposable commented Aug 1, 2022 •

edited

Loading

lemire commented Aug 1, 2022

SergeiPavlov commented Aug 2, 2022

IDisposable commented Aug 2, 2022 •

edited

Loading

SergeiPavlov commented Aug 2, 2022 •

edited

Loading

SergeiPavlov commented Aug 2, 2022

cassioneri commented Aug 2, 2022

EgorBo commented Aug 2, 2022 •

edited

Loading

DrewScoggins commented Aug 9, 2022 •

edited by AndyAyersMS

Loading

kunalspathak commented Aug 16, 2022

danmoseley commented Aug 16, 2022

30% optimization of DateTime.GetDate()/.Year/.Month/.Day/.DayOfYear by 'Euclidean affine functions' #72712

30% optimization of DateTime.GetDate()/.Year/.Month/.Day/.DayOfYear by 'Euclidean affine functions' #72712

Conversation

SergeiPavlov commented Jul 23, 2022 • edited Loading

dotnet-issue-labeler bot commented Jul 23, 2022

ghost commented Jul 23, 2022

Clockwork-Muse Jul 23, 2022

Choose a reason for hiding this comment

SergeiPavlov Jul 23, 2022 • edited Loading

Choose a reason for hiding this comment

Clockwork-Muse Jul 23, 2022

Choose a reason for hiding this comment

SergeiPavlov Jul 23, 2022 • edited Loading

Choose a reason for hiding this comment

danmoseley commented Jul 24, 2022

danmoseley commented Jul 24, 2022

SergeiPavlov commented Jul 24, 2022 • edited Loading

lemire commented Jul 24, 2022

danmoseley commented Jul 28, 2022 • edited Loading

danmoseley commented Jul 28, 2022

danmoseley commented Jul 28, 2022

danmoseley commented Jul 28, 2022

SergeiPavlov commented Jul 28, 2022

SergeiPavlov commented Jul 28, 2022 • edited Loading

danmoseley left a comment

Choose a reason for hiding this comment

danmoseley left a comment

Choose a reason for hiding this comment

danmoseley commented Jul 28, 2022

richlander commented Jul 28, 2022

cassioneri commented Jul 31, 2022 • edited Loading

danmoseley commented Jul 31, 2022

richlander commented Jul 31, 2022

IDisposable commented Aug 1, 2022 • edited Loading

lemire commented Aug 1, 2022

SergeiPavlov commented Aug 2, 2022

IDisposable commented Aug 2, 2022 • edited Loading

SergeiPavlov commented Aug 2, 2022 • edited Loading

SergeiPavlov commented Aug 2, 2022

cassioneri commented Aug 2, 2022

EgorBo commented Aug 2, 2022 • edited Loading

DrewScoggins commented Aug 9, 2022 • edited by AndyAyersMS Loading

kunalspathak commented Aug 16, 2022

danmoseley commented Aug 16, 2022

30% optimization of `DateTime.GetDate()`/`.Year/.Month/.Day/.DayOfYear` by 'Euclidean affine functions' #72712

30% optimization of `DateTime.GetDate()`/`.Year/.Month/.Day/.DayOfYear` by 'Euclidean affine functions' #72712

SergeiPavlov commented Jul 23, 2022 •

edited

Loading

SergeiPavlov Jul 23, 2022 •

edited

Loading

SergeiPavlov Jul 23, 2022 •

edited

Loading

SergeiPavlov commented Jul 24, 2022 •

edited

Loading

danmoseley commented Jul 28, 2022 •

edited

Loading

SergeiPavlov commented Jul 28, 2022 •

edited

Loading

cassioneri commented Jul 31, 2022 •

edited

Loading

IDisposable commented Aug 1, 2022 •

edited

Loading

IDisposable commented Aug 2, 2022 •

edited

Loading

SergeiPavlov commented Aug 2, 2022 •

edited

Loading

EgorBo commented Aug 2, 2022 •

edited

Loading

DrewScoggins commented Aug 9, 2022 •

edited by AndyAyersMS

Loading