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 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
Copy link
Member

jkotas commented Dec 2, 2020

Related to #44771

{
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.

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

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.

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

@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants