NotificationsYou must be signed in to change notification settings
Fork5.2k
Star17.2k

Delete DateTime FCalls and switch to fully managed implementation#46690

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

jkotas merged 1 commit intodotnet:masterfromjkotas:datetime

Jan 8, 2021

Merged

Delete DateTime FCalls and switch to fully managed implementation#46690

jkotas merged 1 commit intodotnet:masterfromjkotas:datetime

Jan 8, 2021

Conversation

Copy link

Member

jkotas commentedJan 7, 2021

No description provided.

Delete DateTime FCalls and switch to fully managed implementation

4652370

Copy link

Collaborator

Dotnet-GitSync-Bot commentedJan 7, 2021

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 onearea label.

Copy link

MemberAuthor

Now that we haveSuppressGCTransition modifiers for function pointers thanks for@jkoritzinsky, we can get rid of the DateTime FCalls that existed for performance reasons and unify the code accross runtimes.

On Windows without leap time second support, this change makes DateTime.UtcNow about 5% faster. On Windows with lead time second support, the performance impact of this change is in the noise range.

This change includes the straightforward parts from#44771. One of my motivation for doing this sooner rather than later is to start getting real-world millage onSuppressGCTransition to shake out corner-case bugs in it if there are any.

jkotas requested review fromGrabYourPitchforks andtarekgh

January 7, 2021 18:17

jkotas added the area-System.Runtime label

Jan 7, 2021

jkotas commented

Jan 7, 2021

View reviewed changes

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

		FullSystemTimetime;
		GetSystemTimeWithLeapSecondsHandling(&time);

		if(Interop.Kernel32.FileTimeToSystemTime(&fileTime,&time.systemTime)!=Interop.BOOL.FALSE)

Copy link

MemberAuthor

jkotasJan 7, 2021

Choose a reason for hiding this comment

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

Some of the methods were split into multiple parts to make the FCalls work. I have undone the split since it is no longer needed.

stephentoub reviewed

Jan 7, 2021

View reviewed changes

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


		#if!CORECLR
		internalstaticreadonlybools_systemSupportsPreciseSystemTime=SystemSupportsPreciseSystemTime();
		privatestaticunsafereadonlydelegate* unmanaged[SuppressGCTransition]<long*,void>s_pfnGetSystemTimeAsFileTime=GetGetSystemTimeAsFileTimeFnPtr();

Copy link

Member

stephentoubJan 7, 2021

Choose a reason for hiding this comment

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

I admit when support was added for function pointers I was not expecting us to be able to make such extensive use of it. Cool.

stephentoub reviewed

Jan 7, 2021

View reviewed changes

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

		if(time->systemTime.Second>59)
		// Retry this check several times to reduce chance of false negatives due to thread being rescheduled
		// at wrong time.
		for(inti=0;i<10;i++)

Copy link

Member

stephentoubJan 7, 2021•
edited
Loading

Choose a reason for hiding this comment

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

This retry loop wasn't present previously, was it? You're just adding it proactively as you expect it could be an issue?

Copy link

MemberAuthor

jkotasJan 7, 2021

Choose a reason for hiding this comment

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

Yes, I am adding it proactively to make this check more robust. I think we are incorrectly falling back to imprecise UtcNow like one in million runtime startups without this loop. The loop will exit in first iteration on well-behaving systems.

stephentoub approved these changes

Jan 7, 2021

View reviewed changes

tarekgh approved these changes

Jan 7, 2021

View reviewed changes

Copy link

Member

tarekgh left a comment

Choose a reason for hiding this comment

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

This is looks great to me. Thanks@jkotas for cleaning it all up.

should we close#45479 now?

Copy link

MemberAuthor

jkotas commentedJan 7, 2021

should we close#45479 now?

#45479 has number of micro-optimizations that may be still valuable. I think we should keep it after resolving the conflict with this change.

Copy link

Member

tarekgh commentedJan 7, 2021

#45479 has number of micro-optimizations that may be still valuable. I think we should keep it after resolving the conflict with this change.

I am seeing the PR has very minor such micro optimization (converting some long to ulong). would it make sense just to added these here instead? it is not much and also I will expect the perf gain from these would be small to justify the change. no pushing here though.

Copy link

Contributor

srxqds commentedJan 7, 2021

what effect on monovm?

Copy link

MemberAuthor

jkotas commentedJan 8, 2021

converting some long to ulong

I have actually started doing that originally, but then the delta kept growing and the PR got harder to review. I prefer to take it smaller step at a time.