-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Performance regression: DateTime.UtcNow and DateTimeOffset.UtcNow 2.5x slower #13091
Comments
I guess it's a cost of dotnet/coreclr#21420 (leap seconds support) |
It is. You can see this pretty clearly by just setting s_systemSupportsLeapSeconds to false. On my machine it ends up being true:
and when I set it to false:
I think that probably makes this "by design", but @tarekgh should comment. |
Might be worth seeing whether we can reduce the cost though, if that wasn't already done at the time. |
Yes this is expected because of the leap seconds support.
I already looked at that before and what we have the best we can have now till we get more new Windows APIs which may help. |
Fair enough. |
@stephentoub also reminded me about, there is a way to disable the leap seconds handling on the machine. That should give the perf back if anyone really sensitive to that.
|
@tarekgh maybe these two calls can be merged into a single internal call? https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/DateTime.Windows.cs#L21-L22 |
@EgorBo I don't think this will make much difference. The second call is inlined anyway. |
I moved this to future milestone for now. feel free to bring it back if you feel otherwise. |
Also, maybe it makes sense to introduce a |
That is will not be good because if the system support leap seconds and we disable the handling in the framework, you will start seeing time difference between what we report and what the system report (when calling Now functionality). the framework has to comply with what the system is doing. |
Just to make sure I confirm that OSes other than Windows are not affected: System.Tests.Perf_DateTime.GetUtcNow
System.Tests.Perf_DateTimeOffset.GetUtcNow
|
@tarekgh I am currently profiling all the issues that I've reported in the last few weeks and I want to get a better understanding of how we've implemented leap second support. Please excuse me if my questions are dummy ;) In the profiles for 3.0 the most expensive method is In 2.2 this method was not used. From the callstack and the code I can see that:
My questions:
public static DateTime UtcNow
{
get
{
long ticks = 0;
ticks = GetSystemTimeAsFileTime();
DateTime result = new DateTime(((ulong)(ticks + FileTimeOffset)) | KindUtc);
int second = result.Second;
if (second > 59)
{
result = result.AddSeconds(-second + 59);
}
return result;
}
} |
Here are some clarifications may help explaining why we did what that. there are 2 conditions we need to do when reporting the time through UtcNow:
DateTime in general encapsulate the time as ticks which are the number of the 100 nanoseconds with the notion the minutes are always 60 seconds (0 to 59). i.e. we never have second Windows introduced the leap seconds support, which means when any Windows system enable the leap seconds, the reported time will include the leap seconds. in other word, the minutes can have 61 seconds when we encounter a leap second. That means when we read the time from system, we must map that to our DateTime which require the minutes always 60 seconds. Before we did the leap seconds support in .NET, what we used to do is we just call GetSystemTimePreciseAsFileTime which return the time 100 nanoseconds that we just use directly in DateTime (as the ticks) and there is not any conversion needed. When you run on a system support leap seconds and there are already some leap seconds reported in this system, you will start see some shift in the reported .NET time because we converted the ticks came from GetSystemTimePreciseAsFileTime assuming every minute is 60 seconds while this is not the case anymore. To make .NET work with systems have leap seconds enabled, we had 2 options:
We have chosen the second solution because users cared more about the precision. you can see that on the issue which required us to use GetSystemTimePreciseAsFileTime instead of using GetSystemTimeAsFileTime which we use in the .NET desktop. Here are the answers for your questions:
First, RtlGetSystemTimePrecise is undocumented API which we cannot use. but even if we can use it will not help much because this will give use the time as ticks anyway and we'll still need to convert it. so, this is not buying us anything.
Yes, that is possible, but we'll lose the precision which we have tried to not do that in the first place.
No, this condition must be done anyway either in native or manager code, so I don't believe this will make any difference. Also, in the code you are suggesting if (second > 59) This condition is always false because DateTime never reprot second number greater than 59. We are asking Windows to expose some API to us something like GetSystemTimePrecise() and return the system time directly with the high precision. That is what I mentioned in my previous comments |
@tarekgh thanks for the explanation! |
…ap second performance regression impact, related to #25728
…e impact (#26046) * make few methods used by DateTime.UtcNow inlinable to minimize the leap second performance regression impact, related to #25728 * apply suggested micro-optimizations
@adamsitnik @DrewScoggins Heads up, as soon as your perf infrastructure takes eed3c76 you'll see the
If you see any change at all (good or bad) on non-Win10 platforms, let us know so that we can double-check that we didn't inadvertently change these code paths. If you see a regression on Win10, let us know because it probably means we have a problem in our caching logic. (A cache miss in this logic is very expensive, but they should happen so infrequently that they shouldn't show up at all in benchmarks.) |
How expensive? |
Best guess, probably 6x - 7x the cost of a cache hit? |
Ok, so relatively very expensive rather than absolutely very expensive. Cool. |
DateTime.UtcNow
andDateTimeOffset.UtcNow
are 2.5x slower compared to 2.2Repro
/cc @danmosemsft
The text was updated successfully, but these errors were encountered: