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

Optimize System.DateTime #45479

Merged
merged 5 commits into from
Feb 2, 2021
Merged

Optimize System.DateTime #45479

merged 5 commits into from
Feb 2, 2021

Conversation

pentp
Copy link
Contributor

@pentp pentp commented Dec 2, 2020

Makes DateTime.UtcNow about 5% faster (over 90% time is spent in OS calls now).
Also changed most of DateTime calculations to use unsigned integers.

@Dotnet-GitSync-Bot
Copy link
Collaborator

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.

@jkotas jkotas added area-System.Runtime tenet-performance Performance related issue labels Dec 2, 2020
@jkotas
Copy link
Member

jkotas commented Dec 2, 2020

Related to #44771

jkotas
jkotas reviewed Dec 2, 2020
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/DateTime.cs
{
throw new InvalidCastException(SR.Format(SR.InvalidCast_FromTo, "DateTime", "Double"));
}
bool IConvertible.ToBoolean(IFormatProvider? provider) => throw InvalidCast(nameof(Boolean));
Copy link
Member

Choose a reason for hiding this comment

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

I do not think it is worth optimizing these using ThrowHelper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't an optimization but just reducing code size/duplication.

@tarekgh
Copy link
Member

tarekgh commented Dec 2, 2020

@pentp I am off properly will not be able to go deeper in this review. I am seeing you are touching the leap seconds handling in the code and I am not sure if the changes would affect that. did you test with a leap seconds on in any system?

@tarekgh
Copy link
Member

tarekgh commented Dec 2, 2020

@jkotas would be good to not merge this till it is carefully reviewed and tested with leap seconds.

@jkotas
Copy link
Member

jkotas commented Dec 2, 2020

@jkotas would be good to not merge this till it is carefully reviewed and tested with leap seconds.

Agree. I won't merge this before you and @GrabYourPitchforks sign off on this.

@runfoapp runfoapp bot mentioned this pull request Dec 2, 2020
Closed
@ViktorHofer
Copy link
Member

// Auto-generated message

69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts.

To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others.

@jkotas jkotas mentioned this pull request Dec 19, 2020
Closed
@jkotas jkotas mentioned this pull request Jan 7, 2021
Merged
@jkotas
Copy link
Member

jkotas commented Jan 8, 2021

@pentp I have deleted the DateTime FCalls in #46690 that will make this delta smaller. Could you please resolve the conflict?

@tarekgh
Copy link
Member

tarekgh commented Jan 18, 2021

Hi @pentp, do you have a chance to resolve the merge conflict?

@pentp
Copy link
Contributor Author

pentp commented Jan 19, 2021

Yes, I'll rebase and fix the conflicts.
I've thought about how to write tests for the leap second paths also and now that the native part has been removed it should be possible to use function pointers in checked builds for TzSpecificLocalTimeToSystemTime/SystemTimeToFileTime (like s_pfnGetSystemTimeAsFileTime) and then write tests that intercept these calls so that IsValidTimeWithLeapSeconds can return a predetermined value. Alternatively, the entire IsValidTimeWithLeapSeconds call could be intercepted (perf less important as it should be a rare call). Does that sound like something worth investigating?

@tarekgh
Copy link
Member

tarekgh commented Jan 19, 2021

@pentp

Yes, I'll rebase and fix the conflicts.

Thanks a lot.

I've thought about how to write tests for the leap second paths also and now that the native part has been removed it should be possible to use function pointers in checked builds for TzSpecificLocalTimeToSystemTime/SystemTimeToFileTime (like s_pfnGetSystemTimeAsFileTime) and then write tests that intercept these calls so that IsValidTimeWithLeapSeconds can return a predetermined value. Alternatively, the entire IsValidTimeWithLeapSeconds call could be intercepted (perf less important as it should be a rare call). Does that sound like something worth investigating?

I didn't closely review your original changes so I am not clear why need to intercept the calls to IsValidTimeWithLeapSeconds? is it part of the optimization we are trying to do here? or this is just to do some validation? If this is not related to the optimization, then I would say let's focus to get the optimization first and have some perf numbers how much we are gaining.

@tarekgh
Copy link
Member

tarekgh commented Jan 24, 2021

@pentp sorry to ping you again. Would you be able to get this soon? I am asking because we are trying to close long opened PR's. Thanks!

@pentp
Copy link
Contributor Author

pentp commented Jan 27, 2021

Rebased and resolved conflicts. Test failures seem to be #47374.

@tarekgh
Copy link
Member

tarekgh commented Jan 27, 2021

Thanks @pentp. I'll take a look soon.

@tarekgh
Copy link
Member

tarekgh commented Jan 28, 2021

@pentp thanks a lot. The changes LGTM. As it looks this changes will give us some perf gain. do we have some perf numbers for that. (at least in the basic scenarios with DateTime)?

@pentp
Copy link
Contributor Author

pentp commented Jan 28, 2021 •
edited
Loading

For DateTime.UtcNow it's about 6% faster (x64 asm size from 625 to 413), DateTime.Now is about 10% faster, TryParseExact for round-trip format O is about 4% faster (uses DatetTime.TryCreate).
The other methods I haven't measured properly, but they all have reduced asm code size.

@mattjohnsonpint
Copy link
Contributor

I got through about half of this so far, and will review further tomorrow.

This was referenced Jan 28, 2021
Closed
Closed
@tarekgh
Copy link
Member

tarekgh commented Feb 1, 2021

@pentp did you have time to address the remaining comments? I think we should be good to go after that.

@runfoapp runfoapp bot mentioned this pull request Feb 2, 2021
Closed
@tarekgh
Copy link
Member

tarekgh commented Feb 2, 2021

@pentp thanks for getting this done. this is amazing.

@tarekgh tarekgh merged commit e6a5339 into dotnet:master Feb 2, 2021
@tarekgh
Copy link
Member

tarekgh commented Feb 2, 2021

#45318

@pentp pentp deleted the DateTime branch February 3, 2021 02:51
@ghost ghost locked as resolved and limited conversation to collaborators Mar 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@EgorBo EgorBo EgorBo left review comments

@mattjohnsonpint mattjohnsonpint mattjohnsonpint left review comments

@jkotas jkotas jkotas left review comments

@tarekgh tarekgh tarekgh approved these changes

@GrabYourPitchforks GrabYourPitchforks Awaiting requested review from GrabYourPitchforks

Assignees
No one assigned
Labels
area-System.DateTime tenet-performance Performance related issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@pentp @Dotnet-GitSync-Bot @jkotas @tarekgh @ViktorHofer @mattjohnsonpint @EgorBo @jeffhandley