-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Use GetSystemTimePreciseAsFileTime if available in DateTime.UtcNow #9736
Conversation
Can it be marked with port to desktop potential? Don't know what the procedure is on it |
|
||
::GetSystemTimeAsFileTime((FILETIME*)×tamp); | ||
INT64 timestamp; | ||
g_pfnGetSystemTimeAsFileTime((FILETIME*)×tamp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nit - please replace the tab by spaces here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
FCIMPL0(INT64, SystemNative::__GetSystemTimeAsFileTime) | ||
{ | ||
FCALL_CONTRACT; | ||
|
||
INT64 timestamp; | ||
if (!g_fGetSystemTimeAsFileTimeInitialized) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will save a few cycles to get rid of this volatile check and move the initialization out into separate method. You can do something like this:
extern pfnGetSystemTimeAsFileTime g_pfnGetSystemTimeAsFileTime;
void WINAPI InitializeGetSystemTimeAsFileTime(LPFILETIME lpSystemTimeAsFileTime)
{
HMODULE hKernel32 = WszLoadLibrary(W("kernel32.dll"));
if (hKernel32 != NULL)
{
g_pfnGetSystemTimeAsFileTime = (pfnGetSystemTimeAsFileTime)GetProcAddress(hKernel32, "GetSystemTimePreciseAsFileTime");
}
else
{
g_pfnGetSystemTimeAsFileTime = &::GetSystemTimeAsFileTime;
}
g_pfnGetSystemTimeAsFileTime(lpSystemTimeAsFileTime);
}
pfnGetSystemTimeAsFileTime g_pfnGetSystemTimeAsFileTime = InitializeGetSystemTimeAsFileTime(LPFILETIME lpSystemTimeAsFileTime);
The hot path can then be just g_pfnGetSystemTimeAsFileTime((FILETIME*)×tamp);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to this general approach
@@ -41,14 +41,35 @@ typedef BOOL (*pfnGetPhoneVersion)(LPOSVERSIONINFO lpVersionInformation); | |||
pfnGetPhoneVersion g_pfnGetPhoneVersion = NULL; | |||
#endif | |||
|
|||
typedef void(WINAPI *pfnGetSystemTimeAsFileTime)(LPFILETIME lpSystemTimeAsFileTime); | |||
pfnGetSystemTimeAsFileTime g_pfnGetSystemTimeAsFileTime = NULL; | |||
Volatile<bool> g_fGetSystemTimeAsFileTimeInitialized = false; | |||
|
|||
FCIMPL0(INT64, SystemNative::__GetSystemTimeAsFileTime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing that should save a few cycles is to change the signature of this method to: void GetSystemTimeAsFileTime(INT64* pTimestamp)
.
It will make the signature of the underlying windows API match exactly the signature of the FCall. It means that the compiler can compile the FCall to just jmp [g_pfnGetSystemTimeAsFileTime]
vs. method with a frame that it is today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meaning change the API signature up through to how its called from managed? Makes sense, but I'd like to leave that for a separate change, and leave this one just as the precision change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meaning change the API signature up through to how its called from managed?
Right.
this one just as the precision change.
ok
// Try to use GetSystemTimePreciseAsFileTime if it's available (Win8+). | ||
// Otherwise fall back to GetSystemTimeAsFileTime. | ||
pfnGetSystemTimeAsFileTime func = NULL; | ||
HMODULE hKernel32 = WszLoadLibrary(W("kernel32.dll")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be under #ifndef FEATURE_PAL
so that we are not trying to load kernel32.dll on Unix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janvorli noted this to me offline as well. Fixed.
Marked https://github.com/dotnet/corefx/issues/15739 as netfx-port-consider |
a851bd8
to
141ce2b
Compare
LGTM, I would like to document the precision of DateTime.NowUtc to the degree we can. We should update the documentation comment of DateTime.NowUtc to indicate the precision you can expect (which I think is ~1us+ on both Win8+ and Linux but may be as little as 16msec on older OSes. |
@dotnet-bot test OSX x64 Checked Build and Test please |
@dotnet-bot test Ubuntu x64 Checked Build and Test please (https://github.com/dotnet/coreclr/issues/9750) |
+1 Thanks! |
What about non-windows implementations? |
In Issue #5061 @mj1856 collected some useful data about what happens on Linux which I copy here (but the original has links. .
It was not completely clear what the granularity is of what we currently do however. We also don't have a clear idea of the cost of the extra precision. However by arguments I have already made, as long as it is not super-bad, we should make the granularity on Linux at least 1msec, and preferably more like 1usec (probably by using clock_gettime(CLOCK_REALTIME). |
I'll take a look at it later today and submit a PR if needed. |
Not only is it slower that way (as stephentoub pointed out), but I also challenge the "more precise" statement. |
@fuerstrainier, you're saying you have that problem with GetSystemTimePreciseAsFileTime but not with GetSystemTimeAsFileTime? |
@stephentoub yes that is correct. The windows clock (which is read by GetSystemTimeAsFileTime) is updated by a proper external clock and the tick counter (additionally) used by GetSystemTimePreciseAsFileTime is updated by the local machines tick rate, which will tick faster or slower than our reference clock. If it's faster it could even overtake the windows clock updates... |
Thanks, @fuerstrainier. |
@fuerstrainier - Is there a basis for your claim that it is ticking faster or slower than your reference clock? The considerable documentation on this subject would suggest otherwise. The GetSystemTimePreciseAsFileTime docs state:
The Windows Server 2016 Accurate Time docs state:
Perhaps you were thinking of
So, if you feel otherwise, I'd really like to understand why. Thanks. |
@mj1856 Thanks for your reply. I have to amend my original statement a bit, since it basically depends on the implementation of the timeservice you are using. If it follows a "stepping" approach (using the SetSystemTime operation), you can get into troubles due to different clock speeds, but if it just "skews" the windows clock (using the SetSystemTimeAdjustment), we have no issue with time continuity. Since I learned today that using the stepping approach is not a good idea anyway, it's not a deal for us anymore. Cheers. |
Makes DateTime.UtcNow much more precise, at the expense of being ~2x slower than it was previously.
For example, prior to the change, this code:
produced results like:
After the change, it produces results like:
Fixes https://github.com/dotnet/coreclr/issues/5061
Fixes https://github.com/dotnet/corefx/issues/16405
Closes https://github.com/dotnet/corefx/issues/15739
cc: @vancem, @jkotas