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

Performance regression: DateTime.UtcNow and DateTimeOffset.UtcNow 2.5x slower #13091

Closed
adamsitnik opened this issue Jul 16, 2019 · 20 comments · Fixed by #50263
Closed

Performance regression: DateTime.UtcNow and DateTimeOffset.UtcNow 2.5x slower #13091

adamsitnik opened this issue Jul 16, 2019 · 20 comments · Fixed by #50263
Labels
Milestone

Comments

@adamsitnik
Copy link
Member

DateTime.UtcNow and DateTimeOffset.UtcNow are 2.5x slower compared to 2.2

[Benchmark]
public DateTimeOffset GetUtcNow() => DateTimeOffset.UtcNow;

[Benchmark]
public DateTime GetUtcNow() => DateTime.UtcNow;

Repro

git clone https://github.com/dotnet/performance.git
cd performance
# if you don't have cli installed and want python script to download the latest cli for you
py .\scripts\benchmarks_ci.py -f netcoreapp2.2 netcoreapp3.0 --filter *GetUtcNow*
# if you do
dotnet run -p .\src\benchmarks\micro\MicroBenchmarks.csproj -c Release -f netcoreapp2.2 --filter *GetUtcNow* --runtimes netcoreapp2.2 netcoreapp3.0 --join
BenchmarkDotNet=v0.11.3.1003-nightly, OS=Windows 10.0.18362
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=3.0.100-preview8-013262
   [Host]     : .NET Core 2.2.6 (CoreCLR 4.6.27817.03, CoreFX 4.6.27818.02), 64bit RyuJIT
   Job-BYJCMJ : .NET Core 2.2.6 (CoreCLR 4.6.27817.03, CoreFX 4.6.27818.02), 64bit RyuJIT
   Job-JSSCYO : .NET Core 3.0.0-preview8-27916-02 (CoreCLR 4.700.19.36302, CoreFX 4.700.19.36514), 64bit RyuJIT
Type Method Toolchain Mean
Perf_DateTime GetUtcNow netcoreapp2.2 25.12 ns
Perf_DateTime GetUtcNow netcoreapp3.0 70.91 ns
Perf_DateTimeOffset GetUtcNow netcoreapp2.2 38.19 ns
Perf_DateTimeOffset GetUtcNow netcoreapp3.0 91.96 ns

/cc @danmosemsft

@EgorBo
Copy link
Member

EgorBo commented Jul 16, 2019

I guess it's a cost of dotnet/coreclr#21420 (leap seconds support)

@adamsitnik
Copy link
Member Author

/cc @billwert @DrewScoggins

@stephentoub
Copy link
Member

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:

Method Toolchain Mean Error StdDev Ratio
GetDateTimeOffsetUtcNow netcoreapp2.1 32.22 ns 0.1057 ns 0.0989 ns 1.00
GetDateTimeOffsetUtcNow netcoreapp3.0 71.98 ns 0.5065 ns 0.4490 ns 2.24

and when I set it to false:

Method Toolchain Mean Error StdDev Ratio
GetDateTimeOffsetUtcNow netcoreapp2.1 32.18 ns 0.0193 ns 0.0180 ns 1.00
GetDateTimeOffsetUtcNow netcoreapp3.0 31.91 ns 0.0373 ns 0.0349 ns 0.99

I think that probably makes this "by design", but @tarekgh should comment.

@danmoseley
Copy link
Member

Might be worth seeing whether we can reduce the cost though, if that wasn't already done at the time.

@stephentoub stephentoub transferred this issue from dotnet/corefx Jul 16, 2019
@tarekgh
Copy link
Member

tarekgh commented Jul 16, 2019

Yes this is expected because of the leap seconds support.

Might be worth seeing whether we can reduce the cost though, if that wasn't already done at the time.

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.

@danmoseley
Copy link
Member

Fair enough.

@tarekgh
Copy link
Member

tarekgh commented Jul 16, 2019

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

https://techcommunity.microsoft.com/t5/Networking-Blog/Leap-Seconds-for-the-IT-Pro-What-you-need-to-know/ba-p/339811


However, if you have a heterogenous time-sensitive environment you can revert to the prior operating system behavior and disable leap seconds across the board by adding the following registry key:

HKLM:\SYSTEM\CurrentControlSet\Control\LeapSecondInformation

 Type : "REG_DWORD"
Name : Enabled
Value : 0 Disables the system-wide setting
Value : 1 Enables the system-wide setting

@EgorBo
Copy link
Member

EgorBo commented Jul 16, 2019

@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

@tarekgh
Copy link
Member

tarekgh commented Jul 16, 2019

@tarekgh
Copy link
Member

tarekgh commented Jul 16, 2019

I moved this to future milestone for now. feel free to bring it back if you feel otherwise.

@EgorBo
Copy link
Member

EgorBo commented Jul 16, 2019

Also, maybe it makes sense to introduce a COMPlus_LeapSecondsSupport env variable to be able to turn that behavior off for a specific app?

@tarekgh
Copy link
Member

tarekgh commented Jul 16, 2019

Also, maybe it makes sense to introduce a COMPlus_LeapSecondsSupport env variable to be able to turn that behavior off for a specific app?

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.

@adamsitnik
Copy link
Member Author

adamsitnik commented Jul 23, 2019

Just to make sure I confirm that OSes other than Windows are not affected:

System.Tests.Perf_DateTime.GetUtcNow

conclusion Base Diff Base/Diff Modality Base Runtime Diff Runtime Arch Processor Name Operating System
Same 20.13 20.02 1.01 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-09 64bit Intel Xeon CPU E5-1650 v4 3.60GHz ubuntu 18.04
Slower 23.90 68.20 0.35 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-09 64bit Intel Xeon CPU E5-1650 v4 3.60GHz Windows 10.0.18362
Same 38.28 37.29 1.03 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-01 64bit Intel Xeon CPU E5-2673 v4 2.30GHz ubuntu 16.04
Same 39.22 38.84 1.01 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-01 64bit Intel Xeon CPU E5-2673 v4 2.30GHz ubuntu 18.04
Same 34.76 34.66 1.00 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-09 64bit Intel Core i7-5557U CPU 3.10GHz (Broadwell) macOS Mojave 10.14.5
Slower 25.93 77.56 0.33 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-09 64bit Intel Core i7-5557U CPU 3.10GHz (Broadwell) Windows 10.0.18362
Slower 21.10 63.59 0.33 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-09 64bit Intel Core i7-7700 CPU 3.60GHz (Kaby Lake) Windows 10.0.18362
Slower 34.39 81.64 0.42 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-09 64bit AMD Ryzen 7 1800X Windows 10.0.18362
Slower 33.98 136.04 0.25 several? .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-09 32bit Intel Xeon CPU E5-1650 v4 3.60GHz Windows 10.0.18362

System.Tests.Perf_DateTimeOffset.GetUtcNow

conclusion Base Diff Base/Diff Modality Base Runtime Diff Runtime Arch Processor Name Operating System
Same 40.62 38.68 1.05 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-09 64bit Intel Xeon CPU E5-1650 v4 3.60GHz ubuntu 18.04
Slower 36.06 79.45 0.45 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-09 64bit Intel Xeon CPU E5-1650 v4 3.60GHz Windows 10.0.18362
Same 69.96 69.97 1.00 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-01 64bit Intel Xeon CPU E5-2673 v4 2.30GHz ubuntu 16.04
Faster 73.01 68.54 1.07 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-01 64bit Intel Xeon CPU E5-2673 v4 2.30GHz ubuntu 18.04
Same 59.95 57.15 1.05 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-09 64bit Intel Core i7-5557U CPU 3.10GHz (Broadwell) macOS Mojave 10.14.5
Slower 40.69 92.05 0.44 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-09 64bit Intel Core i7-5557U CPU 3.10GHz (Broadwell) Windows 10.0.18362
Slower 31.87 74.86 0.43 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-09 64bit Intel Core i7-7700 CPU 3.60GHz (Kaby Lake) Windows 10.0.18362
Slower 47.93 97.43 0.49 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-09 64bit AMD Ryzen 7 1800X Windows 10.0.18362
Slower 66.75 158.34 0.42 .NET Core 2.2.6 .NET Core 3.0.0-preview8-27919-09 32bit Intel Xeon CPU E5-1650 v4 3.60GHz Windows 10.0.18362

@adamsitnik
Copy link
Member Author

@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 FileTimeToSystemTime which is a mapping from FileTime to SystemTime:

obraz

In 2.2 this method was not used.

obraz

From the callstack and the code I can see that:

  1. The FileTime is obtained from a call to kernelbase!GetSystemTimePreciseAsFileTime which internally calls ntdll!RtlGetSystemTimePrecise
  2. FileTime is mapped to SystemTime using kernelbase!FileTimeToSystemTime. This is very expensive
  3. If systemTime.wSecond > 59 then it's the leap second case and few important fields are set to some certain values (this is fast)

My questions:

  1. Why don't we just call ntdll!RtlGetSystemTimePrecise which presumably returns a SystemTime and would allow us to avoid the expensive mapping? Does the mapping itself contains the expensive leap second support logic?
  2. Would calling GetSystemTime instead getting the FileTime and mapping it to a SystemTime reduce the time it takes to get it? I can see that today it's a fallback when the mapping fails.
  3. Would it be possible to move the > 59 seconds check to managed code and move the edge case (user calls DateTime.UtcNow exactly when the leap second happens) from the hot path?
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;
    }
}

@tarekgh
Copy link
Member

tarekgh commented Jul 29, 2019

@adamsitnik

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:

  • The reported time must be precise (up to 100-nanoseconds)
  • The reported time must be exact with the underlying system.

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 60 at all. This design we cannot change because many apps today depend on that and they use ticks directly and always assuming the minute is 60 seconds when doing any manual conversion to time parts (hours, minutes, seconds...etc.). changing this design will be a major app compat concern and systems interacting with each other's will be no longer can exchange the time because one system can be enabled for leap seconds and other is not. we already discussed that with .NET ship room.

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:

  • Instead of calling GetSystemTimePreciseAsFileTime we can just call GetSystemTime which will report the time in (hours, minutes, seconds) and then we can convert it to the tick values that can be used in DateTime. obviously, the problem here is we are going to lose the precision. our max precision would be milliseconds (compared to current precision which is 100 nanoseconds)
  • Or we keep the precision, but we'll have some perf hit we take that we convert the reported system ticks to system time (hours, minutes, second) and we still keep the 100 nanoseconds fraction precision. we did that by using FileTimeToSystemTime.

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:

Why don't we just call ntdll!RtlGetSystemTimePrecise which returns a SystemTime and would allow us to avoid the expensive mapping? Does the mapping itself contains the expensive leap second support logic?

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.

Would calling GetSystemTime instead getting the FileTime and mapping it to a SystemTime reduce the time it takes to get it? I can see that today it's a fallback when the mapping fails.

Yes, that is possible, but we'll lose the precision which we have tried to not do that in the first place.

Would it be possible to move the > 59 seconds check to managed code and move the edge case (user calls DateTime.UtcNow exactly when the leap second happens) from the hot path?

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 till we get more new Windows APIs which may help..

@adamsitnik
Copy link
Member Author

@tarekgh thanks for the explanation!

adamsitnik referenced this issue in adamsitnik/coreclr Aug 7, 2019
…ap second performance regression impact, related to #25728
adamsitnik referenced this issue in dotnet/coreclr Aug 8, 2019
…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
@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 26, 2020
@joperezr joperezr removed the untriaged New issue has not been triaged by the area owner label Jul 2, 2020
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 25, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 27, 2021
@GrabYourPitchforks GrabYourPitchforks modified the milestones: Future, 6.0.0 Mar 27, 2021
@GrabYourPitchforks
Copy link
Member

@adamsitnik @DrewScoggins Heads up, as soon as your perf infrastructure takes eed3c76 you'll see the DateTime.UtcNow benchmarks change. Here's what you should expect:

  • On non-Windows platforms, no change
  • On Win8 and earlier, no change
  • On Win10, an improvement

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

@stephentoub
Copy link
Member

A cache miss in this logic is very expensive

How expensive?

@GrabYourPitchforks
Copy link
Member

How expensive?

Best guess, probably 6x - 7x the cost of a cache hit?

@stephentoub
Copy link
Member

Ok, so relatively very expensive rather than absolutely very expensive. Cool.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
10 participants