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.

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

Optimize DateOnly properties and deconstruct #103352

Closed
wants to merge 1 commit into from

Conversation

lilinus
Copy link
Contributor

@lilinus lilinus commented Jun 12, 2024

Algorithms are modified copies of the respective DateTime properties (and GetDate method).

It is possible to make the code shared between DateTime and DateOnly. Could probably be done with an input "six hour count", but it feels a bit weird. Opinions?

Benchmark results (source):

| Method               | Job        | value      | Mean      | Error     | StdDev    | Median    | Min       | Max       | Ratio | RatioSD | Allocated | Alloc Ratio |
|--------------------- |----------- |----------- |----------:|----------:|----------:|----------:|----------:|----------:|------:|--------:|----------:|------------:|
| DateOnly_Year        | main       | 2024-06-12 | 0.8427 ns | 0.0283 ns | 0.0236 ns | 0.8522 ns | 0.7961 ns | 0.8689 ns |  1.00 |    0.04 |         - |          NA |
| DateOnly_Year        | pr         | 2024-06-12 | 0.2751 ns | 0.0086 ns | 0.0067 ns | 0.2786 ns | 0.2601 ns | 0.2794 ns |  0.33 |    0.01 |         - |          NA |
| DateOnly_Month       | main       | 2024-06-12 | 3.2173 ns | 0.0443 ns | 0.0392 ns | 3.2148 ns | 3.1718 ns | 3.2948 ns |  1.00 |    0.02 |         - |          NA |
| DateOnly_Month       | pr         | 2024-06-12 | 1.2608 ns | 0.0096 ns | 0.0090 ns | 1.2587 ns | 1.2493 ns | 1.2742 ns |  0.39 |    0.01 |         - |          NA |
| DateOnly_Day         | main       | 2024-06-12 | 2.9684 ns | 0.0451 ns | 0.0399 ns | 2.9505 ns | 2.9356 ns | 3.0554 ns |  1.00 |    0.02 |         - |          NA |
| DateOnly_Day         | pr         | 2024-06-12 | 1.1118 ns | 0.0086 ns | 0.0076 ns | 1.1123 ns | 1.0947 ns | 1.1211 ns |  0.37 |    0.01 |         - |          NA |
| DateOnly_DayOfYear   | main       | 2024-06-12 | 0.8781 ns | 0.0075 ns | 0.0063 ns | 0.8777 ns | 0.8680 ns | 0.8878 ns |  1.00 |    0.01 |         - |          NA |
| DateOnly_DayOfYear   | pr         | 2024-06-12 | 0.1715 ns | 0.0087 ns | 0.0081 ns | 0.1737 ns | 0.1586 ns | 0.1822 ns |  0.20 |    0.01 |         - |          NA |
| DateOnly_Deconstruct | main       | 2024-06-12 | 5.2509 ns | 0.0390 ns | 0.0364 ns | 5.2378 ns | 5.2140 ns | 5.3214 ns |  1.00 |    0.01 |         - |          NA |
| DateOnly_Deconstruct | pr         | 2024-06-12 | 4.6530 ns | 0.0267 ns | 0.0236 ns | 4.6589 ns | 4.6075 ns | 4.6817 ns |  0.89 |    0.01 |         - |          NA |

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 12, 2024
Copy link
Contributor

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

ushort daySinceMarch1 = (ushort)((uint)u2 / DateTime.EafDivider);
int n3 = 2141 * daySinceMarch1 + 197913;
// Return 1-based day-of-month
return (ushort)n3 / 2141 + 1;
Copy link
Member

@EgorBo EgorBo Jun 12, 2024

Choose a reason for hiding this comment

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

here and in above - don't you just effectively inline DateTime.Day/Month/Year by hands? Shouldn't it be JIT's resposobility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In effect it circumvents 64-bit multiplication, 64-bit bitwise-and, 64-bit division; and replaces it with a 32-bit shift.

Should we assume JIT takes care of that?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @EgorBo. I don't think we need to complicate the code just to do what the JIT can do. I am concerned about the code maintainability. Keeping it simple is easier to touch in the future.

@tarekgh
Copy link
Member

tarekgh commented Jun 12, 2024

I appreciate trying to optimize here but I have a concern the new code is more involving and will be more difficult to maintain. Is there any issue forcing to try optimizing such cases? I mean anyone complaint about the perf of the cases involved or this optimization affecting any hot path scenario.

@lilinus
Copy link
Contributor Author

lilinus commented Jun 12, 2024

I appreciate trying to optimize here but I have a concern the new code is more involving and will be more difficult to maintain. Is there any issue forcing to try optimizing such cases? I mean anyone complaint about the perf of the cases involved or this optimization affecting any hot path scenario.

Fair enough.

Working with financial applications; discrete dates (and numerics) is a big part of the values the calculations are done on, and where a significant CPU time is spent.

Thanks!

@lilinus lilinus closed this Jun 12, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.DateTime community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants