Optimize DateTime.Year/.DayOfYear properties; fix comments

[SergeiPavlov]

It is continuation of #72712

After we split .GetDatePart() method into separate properties (thank @danmoseley for the idea) we can calculate DateTime.Year & DateTime.DayOfYear more efficiently.
No need for shift to 3/1/0000 base date, used for month/day calculation.

|                                   Method |     Mean | Error | Allocated
|----------------------------------------- |---------:|------:|----------:
|                            DateTime.Year | 4.057 ns |    NA |         -
|   'July 2022 optimization DateTime.Year' | 2.816 ns |    NA |         -
| 'August 2022 optimization DateTime.Year' | 2.192 ns |    NA |         -

|                                        Method |     Mean | Error | Allocated |
|---------------------------------------------- |---------:|------:|----------:|
|                            DateTime.DayOfYear | 3.819 ns |    NA |         - |
|   'July 2022 optimization DateTime.DayOfYear' | 4.026 ns |    NA |         - |
|       'Shift optimization DateTime.DayOfYear' | 2.602 ns |    NA |         - |        // int % 1461 >> 2    expression
|    'UInt Div optimization DateTime.DayOfYear' | 1.698 ns |    NA |         - |        // uint % 1461 / 4     expression
|  'UInt Shift optimization DateTime.DayOfYear' | 1.692 ns |    NA |         - |        // uint % 1461 >> 2    expression
| 'Mul 2939745 optimization DateTime.DayOfYear' | 1.701 ns |    NA |         - |        // This PR: ...  * 2939745 / 11758980 expression 

It will further improve DateTime.Now, because its implementation uses .Year.

Apparently 'July 2022 optimization DateTime.DayOfYear' was an anti-optimization. It used IsLeapYear() with unnecessary year range checks. I missed its benchmarking while checked only .Year, .Month, .Day, .GetDate()

Also:

• corrected comments and var names after previous commit.
• added named constants EafMultiplier, EafDivider used over EAF calculations

Thanks again to Cassio Neri & Lorenz Schneider for their brilliant work!

[dotnet-issue-labeler]

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]

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

Issue Details

---

It is continuation of #72712

After we split .GetDatePart() method into separate properties (thank @danmoseley for the idea) we can calculate DateTime.Year & DateTime.DayOfYear more efficiently.
No need for shift to 3/1/0000 base date, used for month/day calculation.

|                                   Method |     Mean | Error | Allocated
|----------------------------------------- |---------:|------:|----------:
|                            DateTime.Year | 4.057 ns |    NA |         -
|   'July 2022 optimization DateTime.Year' | 2.816 ns |    NA |         -
| 'August 2022 optimization DateTime.Year' | 2.192 ns |    NA |         -

|                                        Method |     Mean | Error | Allocated |
|---------------------------------------------- |---------:|------:|----------:|
|                            DateTime.DayOfYear | 4.211 ns |    NA |         - |
|   'July 2022 optimization DateTime.DayOfYear' | 4.196 ns |    NA |         - |
| 'August 2022 optimization DateTime.DayOfYear' | 2.778 ns |    NA |         - |

It will further improve DateTime.Now, because its implementation uses .Year.

Also corrected comments and var names after previous commit.

Thanks again to Cassio Neri & Lorenz Schneider for their brilliant work!

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

[cassioneri]

Some remarks and suggestions.

1. Divisions on uint are, usually, cheaper than on int. Hence, when we know operands are positive numbers, it's better to use uint. For instance, 3 versions of n / 4 are shown here. We can see that the assembly for the uint version (function g) has fewer operations than the the one for int (function f). In addition, the compiler can understand that n / 4 (for uint n) is the same as n >> 2 (compare g and h). So this manual optimisation (which obfuscates code) is not needed.

2. Example 3.12 in my paper with Lorenz says that
   n % 1461 = 2939745 * n % 2^32 / 2939745, ∀n ∈ [0, 28825529[.
   Hence,
   n % 1461 / 4 = 2939745 * n % 2^32 / 11758980, ∀n ∈ [0, 28825529[.
   But, if 2939745 and n are uint, then % 2^32 above is a no-op. All this can be confirmed by this test.

3. Using the above, three versions of day_of_year can be seen here. Version 1 is yours. Version 2 has the same logic but uses only uint operands and we can see it needs fewer operations than version 1. Finally, version 3 uses the result of point 2.

My C implementation of version 3 seems to work fine (*) as can be seen here. Please, test and measure (and possibly fix) the C# implementation.

Perhaps, your PR #72712 can also be improved by taking point 1 above into consideration.

(*) Unless I got wrong the epoch that .NET uses. I assumed it's 01/01/0001.

[SergeiPavlov]

test and measure (and possibly fix) the C# implementation.

I confirm perfrormance gain when using unsigned arithmetic and mul/div optimization instead of %:
Here is result of benchmarking:

|                                        Method |     Mean | Error | Allocated |
|---------------------------------------------- |---------:|------:|----------:|
|                            DateTime.DayOfYear | 3.819 ns |    NA |         - |
|   'July 2022 optimization DateTime.DayOfYear' | 4.026 ns |    NA |         - |
|       'Shift optimization DateTime.DayOfYear' | 2.602 ns |    NA |         - |
|    'UInt Div optimization DateTime.DayOfYear' | 1.698 ns |    NA |         - |
|  'UInt Shift optimization DateTime.DayOfYear' | 1.692 ns |    NA |         - |
| 'Mul 2939745 optimization DateTime.DayOfYear' | 1.701 ns |    NA |         - |

Last 3 implementations are equal to the accuracy of the measurement.
Pros for uint implementation:

• Most simple code

Pros for uint & Shift implementation:

• On unoptimizing compiler it results to lesser DIV instructions

Pros for your MUL/DIV suggestion:

• consistency with other methods from previous PR.

I have changed .DayOfYear to latter.

(*) Unless I got wrong the epoch that .NET uses. I assumed it's 01/01/0001.

You are right. Epoch date for DateTime type is 01/01/0001. And its range limited by years 1..9999. So all operations can be unsigned.

Perhaps, your PR #72712 can also be improved by taking point 1 above into consideration.

Its look like all division operations are unsigned in that PR (excluding those replaced in this PR). I followed this rule (point 1.).

Thank you. I really appreciate your help to improve the code!

[cassioneri]

Good stuff.

[danmoseley]

Great follow up! We should merge this in the coming week so it is in .NET 7.

(There is no error value in your benchmark table - are you using Benchmark .NET and somehow it is only running one iteration?)

Would you consider adding some randomized test cases for DayOfYear, as mentioned in the other PR? I am on vacation and can't look at the moment but IIRC it was the one that did not have such coverage.

[SergeiPavlov]

(There is no error value in your benchmark table - are you using Benchmark .NET and somehow it is only running one iteration?)

Yes, I used Benchmark.NET with following pattern:

[SimpleJob(launchCount: 1, warmupCount: 1, targetCount: 1, invocationCount: 1  )]
public partial class DateTimeBenchmark
{

const int N = 87_500_000;
static int r;                                 // to suppress discarding calculations by compiler optimizer

[Benchmark(OperationsPerInvoke = N, Description = "August 2022 optimization DateTime.Year")]
public void New_Optimized_Year()
{
    for (var n = N * MyDateTime.TicksPerHour; n > 0;) {
        var dt = new MyDateTime(n -= MyDateTime.TicksPerHour);
        r += dt.New_Optimized_Year;
    }
}

}

Internal loop is to cover full range of DateTime values.
Benchmark.Net divides time of single invocation by N

Would you consider adding some randomized test cases for DayOfYear, as mentioned in the other PR? I am on vacation and can't look at the moment but IIRC it was the one that did not have such coverage.

I will look what can be improved in tests.

[cassioneri]

Would you consider adding some randomized test cases for DayOfYear, as mentioned in the other PR?

Instead of randomized dates, what about a kind of exhaustive test? It can loop through values of ticks (one per day for all possible dates), create a DateTime d out of it and compare d.Year and d.DayOfYear with their expected values. (See this draft -- forgive my C# since I'm not fluent.) I believe such test would be quickly enough to run -- like the similar C code here.

The above is not exhaustive because it only considers one tick per day (ignoring intraday ticks). However, the only think it misses to test is the conversion from ticks to ticks-per-day performed behind the scenes byDateTime. But this conversion is just an integer division which I doubt can go wrong 😉. That's why I say "kind of exhaustive".

[SergeiPavlov]

Instead of randomized dates, what about a kind of exhaustive test?

I have to learn test conventions of dotnet project, but may be exhaustive test is very expensive, because millions tests run on every commit into repo.

Added quick random DayOfYear test
https://github.com/dotnet/runtime/pull/73277/files#diff-7288aaa1cd68794ddf468800e08d8c2d7a606fb6237a8a49f0910fffb97afa48R815

[danmoseley]

For the test, we use a fixed seed for reproducibility. Then of course we will need to test more then one value. Eg use N random values or perhaps better to cover the epoch with random strides?

With respect to exhaustive a test like this probably should not take longer than say 50ms? Depending on other tests running in parallel, it may not end up making the whole library run for any longer.

[SergeiPavlov]

For the test, we use a fixed seed for reproducibility. Then of course we will need to test more then one value. Eg use N random values or perhaps better to cover the epoch with random strides?

With respect to exhaustive a test like this probably should not take longer than say 50ms? Depending on other tests running in parallel, it may not end up making the whole library run for any longer.

Changed DayOfYear_Random test to use seeded Random with 1000 iterations

Added DayOfYear_Exhaustive which covers entire DateTime range with 6-hour step.
Executes < 0.1 s

[danmoseley]

Looks good if the tests pass!

[danmoseley]

If you feel like it you could follow up with a PR into dotnet/performance that protects the improvements.

[SergeiPavlov]

Looks good if the tests pass!

The tests cancel by timeout. I see it is 200 ms.
Probably exhaustive test works slower in multithread environment.
I can try to reduce its step from 6 hours to 1 day.

[danmoseley]

I am not aware of a general test timeout mechanism beyond the one for the test library as a while (which is very large, something like an hour I believe)

Occasionally the test machines are exceptionally slow. I have seen tests that take 0.5 sec or less locally seem to take 20 sec. This can particularly happen on constrained hardware such as phone and simulators. They should not fail in such a case so where a test needs a timeout we often use 30 seconds so long as the test is typically much faster.

[danmoseley]

Failures are unrelated, I will have to open an issue later. Merging.

Thanks!

[danmoseley]

Are you interested in offering any other contributions @SergeiPavlov ? We have many marked up for grabs, or perhaps there is some other area where you are a potential optimization that interests you.

[SergeiPavlov]

Are you interested in offering any other contributions @SergeiPavlov ? We have many marked up for grabs, or perhaps there is some other area where you are a potential optimization that interests you.

Usually I contribute into areas which can impact performance of applications I develop as my main job.
DateTime impacts almost everything, that is why I researched how to improve it.

Of course I'd be glad to improve other places when I face with them.