dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.17k stars 4.72k forks source link

Make DateTIme.Now as efficient as DateTime.UtcNow #24277

Open custarddog opened 6 years ago

custarddog commented 6 years ago

@vancem commented on Tue Sep 05 2017

I thought we fixed this but I just checked and we have not, so I am logging an issue.

DateTime.Now is over 10X SLOWER than DateTime.UtcNow.
I ran the the trivial program below

            var start = DateTime.Now;
            while ((DateTime.Now-start).TotalSeconds < 5)
            {

            }

while profiling with PerfView to get the following data

Name Inc % Inc
||    |+ system.private.corelib!System.DateTime.get_Now() 95.4 4,832
||    | + system.private.corelib!TimeZoneInfo.GetDateTimeNowUtcOffsetFromUtc 88.1 4,462
||    | + system.private.corelib!System.DateTime.get_UtcNow() 5.8 293

which show that only 6% of time is spent actually fetching the time, and 95% of the time was figuring out what time zone we are in.

This mapping from Utc time to local time could be cached and all the inefficiency removed.

This is important because people DO use DateTIme.Now to measure the durations of fast things (in fact we improved DateTime.UtcNow so that it has a accuracy of < 1msec). Currently we tell people to avoid DateTime.Now (use DateTime.UtcNow), to avoid this inefficiency, but we could easily make DateTIme.Now almost the same cost

This is only a small amount of work, we should just do it.

@danmosemsft , @karelz


@mikedn commented on Tue Sep 05 2017

Currently we tell people to avoid DateTime.Now (use DateTime.UtcNow), to avoid this inefficiency

I don't know about others but I avoid DateTime.Now not because it's inefficient but because it is plain wrong to use it to measure durations. Or more generally, to use it for anything that doesn't involve displaying the local time to the user...

Let's keep DateTime.Now slow to encourage devs to do the right thing 😁


@karelz commented on Tue Sep 05 2017

Marking as up-for-grabs. Moderate cost (as it requires perf measurements before and after the change)


@danmosemsft commented on Tue Sep 05 2017

mapping from Utc time to local time could be cached

Of course, daylight savings could begin one second from now so it presumably can't be trivially cached. @tarekgh is there a way that DateTime can be told when the local time offset changes, instead of asking each call? I'm sure there has been thinking about this in the past.


@tarekgh commented on Tue Sep 05 2017

is there a way that DateTime can be told when the local time offset changes

No, we don't have a way to know that. We looked before, and the way you can know that (on Windows) is either you have a Window and listen to some window message or you use WinRT API. Obviously, both are not a good solution for us.

Also, it is difficult to know if we are passing the daylight saving at any moment to know we need to adjust the calculation (or the cache).

We already have some optimization here: http://source.dot.net/#System.Private.CoreLib/src/System/TimeZoneInfo.Win32.cs,50

but we still doing the calculations here: http://source.dot.net/#System.Private.CoreLib/src/System/TimeZoneInfo.Win32.cs,402

because of we cannot know if we crossed the daylight or not.

by the way, the optimization we have there has a bug. that can tell when trying to optimize, you can run into some other problems.


@vancem commented on Tue Sep 05 2017

On the caching issue: It seems to me that when you calculate daylight savings time, you can also know when it will change (at worst you probe say 4 hours into the future and see if it is different, since you know that daylight savings time only happens 2 a year, if you get the same number for now and 4 hours from now, you can know that for the next 4 hours it will not change). The next time you want Now you can then just check if the time is in the 'known' region and if so use the cached DST value.

@mikedn - yes, arguably DateTime.Now is bad for computing deltas (since any time you cross DST, you get a weird value). However giving bad perf does not actually change behavior, it just makes their code that much worse. There are also correct usages (when you just want a timestamp). The bottom line is that if people use it at all frequently (and we have evidence that they do), it is worth optimizing.


@tarekgh commented on Tue Sep 05 2017

@vancem yes, in general, I agree there is a way we can optimize. we just want to be careful to not affect the functionality and not have a complex solution.

To mention, instead of probing for some time (4 hours in your example) we can just calculate the exact time we are going to switch the daylight when we create the cache. and then we always check against this time to know if we need to update the cache or just use the cached offset.


@danmosemsft commented on Tue Sep 05 2017

If the machine time zone changes, do we need to listen to that?


@tarekgh commented on Tue Sep 05 2017

If the machine time zone changes, do we need to listen to that?

Same answer:

No, we don't have a way to know that. We looked before, and the way you can know that (on Windows) is either you have a Window and listen to some window message or you use WinRT API. Obviously, both are not a good solution for us.

Today we ask the apps to listen to the TZ changes on the machine and then call us to clear the cache.


@danmosemsft commented on Wed Nov 29 2017

Moving to CoreFX to track more easily.

danmoseley commented 6 years ago

We should do some investigation to see whether we can do this without making DateTime.Now less reliable. As @tarekgh points out, it already relies on cached data, is there more that can be safely cached?

tarekgh commented 6 years ago

@vancem idea mentioned above should work to make it more performant. Just need to be careful we'll not regress any functionality.

PeterSmithRedmond commented 6 years ago

I just checked the documentation which says: The Now property is frequently used to measure performance. However, because of its low resolution, it is not suitable for use as a benchmarking tool. A better alternative is to use the Stopwatch class.

Certainly there's not even a hint that this API is slower than the UtcNow value. Those of us who have been programming for a while remember that the old real-time clock chips on IBM PCs only returned "local" time and never UTC time, and consequently the .Now would be expected to be faster than UtcNow because the localtime would not have to look up the local time zone.

(https://docs.microsoft.com/en-us/dotnet/api/system.datetime.now?view=netcore-2.0)

tarekgh commented 6 years ago

the old real-time clock chips on IBM PCs only returned "local" time and never UTC time,

This will make me wonder how the chip knows when to start/end the daylight saving?

Clockwork-Muse commented 6 years ago

Currently we tell people to avoid DateTime.Now (use DateTime.UtcNow), to avoid this inefficiency, but we could easily make DateTIme.Now almost the same cost.

No, currently we tell them to avoid DateTime.Now because it's almost certainly the wrong thing. Any efficiencies are side effects.

I just checked the documentation which says: The Now property is frequently used to measure performance. However, because of its low resolution, it is not suitable for use as a benchmarking tool. A better alternative is to use the Stopwatch class.

Except resolution is not speed. That particular gem is likely from before we switched to using a more precise method in corefx (not sure about status on the full framework).

Those of us who have been programming for a while remember that the old real-time clock chips on IBM PCs only returned "local" time and never UTC time, and consequently the .Now would be expected to be faster than UtcNow because the localtime would not have to look up the local time zone.

...which is a strange thing to have to worry about, I think, since you don't want to rely on any 'local time' shenanigans.

This will make me wonder how the chip knows when to start/end the daylight?

In many cases they probably didn't. If it was a consumer machine you might be expected to set the clock yourself, like it was another appliance.
Which is why I tend to think the "Set the server timezone to UTC" became such a thing: to avoid such a hassle. If you're writing your software correctly, the timezone of the box should be a nonissue.

GrabYourPitchforks commented 6 years ago

I agree with @Clockwork-Muse. What's the motivation for this work item? Of the vanishingly small number of people who profile their applications and see that DateTime.Now is a bottleneck, the easy solution for them is to change the code to instead call DateTime.UtcNow, which is almost certainly the right thing for them to have done from the start anyway.

Edit: I saw the comments above about when developers just want a timestamp, such as for logging purposes. I've worked on and profiled real-world services, including services that write to logs frequently. DateTime.Now never registered so much as a blip on those profiles.

Clockwork-Muse commented 6 years ago

...of course, part of the overall problem is that, really, you don't want to be logging in UTC either (which still has a timezone attached - namely, UTC). You want an "absolute" stamp, which is an Instant (which, granted, for historical and readability/roundtripping reasons, is usually serialized in UTC).
Because if I have a DateTime (or whatever) with a UTC zone.... did you get the 'local' zone of the machine or user (and is the fact you have a zone important), or are you trying to represent an invariant/"absolute" stamp, and the zone can be ignored? This is something of a fine distinction, however, and not something the general C# ecosystem pays attention to right now.

MV10 commented 6 years ago

@Clockwork-Muse UTC is not a timezone, UTC is a time standard. UTC is the time without any timezone applied. Timezones are relative to UTC, so using UTC shouldn't preclude also carrying timezone information, which is why everyone should use DateTimeOffset. GMT is the zero-offset timezone. It's a fine distinction but the ability to separately track a timezone offset (hence the name) is one of the reasons DateTimeOffset doesn't have the problems that DateTime has dragged around for all these years for compatibility reasons. Somebody blogged about this on MSDN here in some detail years ago, it's worth reading.

I actually came here from a search while trying to figure out whether DateTimeOffset uses the same high-res source as DateTime.UtcNow has been changed to use. I wasn't going to post but now that I have -- does anyone know? I'm doing high-frequency logging and would like the highest resolution timestamps possible. My logs use DateTimeOffset.UtcNow but it isn't clear where that's sourced.

svick commented 6 years ago

@MV10

I actually came here from a search while trying to figure out whether DateTimeOffset uses the same high-res source as DateTime.UtcNow has been changed to use. I wasn't going to post but now that I have -- does anyone know?

You can look at the source. On both .Net Core and .Net Framework it's:

static DateTimeOffset UtcNow
{
    get
    {
        return new DateTimeOffset(DateTime.UtcNow);
    }
}

This means DateTimeOffset.UtcNow has the same resolution as DateTime.UtcNow.

MV10 commented 6 years ago

Some days I'm still not used to the source actually being available. 😀

Clockwork-Muse commented 6 years ago

@MV10 - As it's commonly referenced, it's both. Yes, formally it's only a time standard, but if we translate to/from ticks then it acts as a zero-offset time zone as well.

And GMT is in the same boat, essentially. And it would only be one of the zero offset time zones.

DateTimes primary problem is DateTimeKind. If that had never been added, we'd have a nice local-only type. And even though DateTimeOffset avoided that, it still has issues: Most people really want a DateTimeZoned, because only having an offset loses some important information.

danmoseley commented 6 years ago

Of the vanishingly small number of people who profile their applications and see that DateTime.Now is a bottleneck, the easy solution for them is to change the code to instead call DateTime.UtcNow, which is almost certainly the right thing for them to have done from the start anyway.

Generally speaking of course we can't necessarily assume that the owner of the app profiles their code, or even can control the code (it might be in a library). It would also be nice if they didn't have to because they didn't have a perf issue in the first place. That isn't to say necessarily this particular API is worth optimizing, but I've seen it in enough profiles I've done to have a pretty good guess that it's worthwhile to optimize, if it is technically reasonable to do so. I think this is essentially what @vancem is saying in the top post.

Clockwork-Muse commented 6 years ago

@danmosemsft - So we're speeding up an API, the use of which (for this scenario) can lead to wrong results occasionally? If DST happens, they're going to get some strange reports...

danmoseley commented 6 years ago

@Clockwork-Muse not necessarily - if they're using it for logging, it could be a good enough timestamp, especially if it's for human use.

Clockwork-Muse commented 6 years ago

....my naive assumption is that the logging output is going to outweigh the amount of time (heh) used to deal with the local time. Perhaps @GrabYourPitchforks has more info, given his previous comment about that use case.

Most conversation on here has been about timing loops, which would be the biggest problem.

MV10 commented 6 years ago

Many non-bare-bones loggers write with an async batch strategy so log output overhead isn't necessarily tied to the rate at which log entries are generated. I've recently assumed the care-and-feeding of the Serilog SQL sink and we have some users batching many thousands of rows. Even with UtcNow it isn't uncommon to have many, many rows with identical timestamps. (We also see the UTC-loses-timezone-data confusion on a regular basis over there since we use DateTimeOffset.)

Clockwork-Muse commented 6 years ago

@MV10 - In logging, the timezone is completely irrelevant; what's relevant is something the equivalent of Java's Instant (which isn't really in the UTC "timezone", per se). No database really has an equivalent type, though - the best you can do on SQL Server is DateTimeOffset (as you appear to know), because otherwise the local/session time zone might be taken into account on retrieval.

Of course, that's just a symptom of a wider problem - when it comes to date/time, the database is out to get you, some more than others. But that's a blog post I need to write later.

MV10 commented 6 years ago

@Clockwork-Muse That's the confusion I referred to: people think UTC is a timezone, and when they set the flag to convert to UTC, they freak out because the timezone information goes away. It's an "education opportunity" I guess.

It seems to me the Java-style Instant epoch-offset would be subject to the same accuracy/drift risks, no? Though I can certainly see why it should perform better.

Clockwork-Muse commented 6 years ago

@MV10 - ...wait, what accuracy/drift risks are you talking about? All modern OSs have a "(fraction of) seconds since epoch" function, which is what Java uses (and C# as well). Note that such a count is completely ignorant of this thing called "UTC" and all actual timezones, it just refers to a specific point in time.

Now, when you format Java's Instant, by default it outputs UTC, since that's readable and commonly understood, but the underlying concept is still the numeric count. Which, again, is not actually UTC itself, because UTC is defined in terms of years, months, etc. You could instead output the count itself - I believe Windows log files do, normally (although Windows uses a different epoch), as well as several internet protocols. Since we have this epoch offset, it's trivial to use it to get the point in time of the event recorded in any given timezone (in Java, via instant.atZone(someTimeZone)). At no point is UTC itself involved (although certainly the then-offset must be applied).

This is the critical thing most programmers fail to actually realize, I think: UTC is almost never relevant to your problem domain.

.....aaaannnnd I'm getting off topic. If anybody wants to prod me about this, I'm on gitter, I guess.

GrabYourPitchforks commented 6 years ago

@danmosemsft I suppose my experience has been different than yours. In the profiles I've seen where DateTime.Now shows up in the hot path, it's always a microbenchmark or an application that's not doing anything terribly interesting. (I remember seeing this show up in Razor "hello world" pages in ASP.NET full framework a few years ago, but at that point we were just digging around in the dirt for a few dozen easy RPS here and there.)

If you have evidence that real-world applications running real-world workloads are actually running into this, I'll defer to you on that topic.

danmoseley commented 6 years ago

@grabyourpitchforks no I don't have anything useful. Maybe @vancem does based on the topmost comment.

GrabYourPitchforks commented 3 years ago

This issue has gone 2 years without activity. Closing as stale. If further discussion is warranted please reopen. Thanks!

tarekgh commented 3 years ago

Reopening as we can look at optimizing the perf for this API.