Delete DateTime FCalls and switch to fully managed implementation #46690
Conversation
|
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. |
|
Now that we have 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 on |
| if (s_systemSupportsLeapSeconds) | ||
| { | ||
| FullSystemTime time; | ||
| GetSystemTimeWithLeapSecondsHandling(&time); | ||
|
|
||
| if (Interop.Kernel32.FileTimeToSystemTime(&fileTime, &time.systemTime) != Interop.BOOL.FALSE) |
There was a problem hiding this comment.
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.
| @@ -111,12 +155,14 @@ internal FullSystemTime(long ticks) | |||
| } | |||
| } | |||
|
|
|||
| #if !CORECLR | |||
| internal static readonly bool s_systemSupportsPreciseSystemTime = SystemSupportsPreciseSystemTime(); | |||
| private static unsafe readonly delegate* unmanaged[SuppressGCTransition]<long*, void> s_pfnGetSystemTimeAsFileTime = GetGetSystemTimeAsFileTimeFnPtr(); | |||
There was a problem hiding this comment.
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.
| 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 (int i = 0; i < 10; i++) |
There was a problem hiding this comment.
This retry loop wasn't present previously, was it? You're just adding it proactively as you expect it could be an issue?
There was a problem hiding this comment.
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.
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. |
|
what effect on monovm? |
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.
MonoVM has been on the managed implementation already. This change is basically switching CoreCLR to the implementation that MonoVM has been using. #46690 (comment) should improve the performance for MonoVM a bit. The improvement is likely in the noise range. |
ghost
locked as resolved and limited conversation to collaborators
No description provided.