Time goes backwards sometimes |
||||||
Issue description
TL;DR base::TimeTicks is broken on iOS - it can go backwards, which does not match the API contract.
So the Metrics code uses base::TimeTicks for these uptime measures, which Chromium documents as:
// TimeTicks and ThreadTicks represent an abstract time that is most of the time
// incrementing, for use in measuring time durations. Internally, they are
// represented in microseconds. They can not be converted to a human-readable
// time, but are guaranteed not to decrease (unlike the Time class). Note that
// TimeTicks may "stand still" (e.g., if the computer is suspended), and
// ThreadTicks will "stand still" whenever the thread has been de-scheduled by
// the operating system.
So, it's supposed to never go backwards. However, looking at the code in time_mac.cc, here's the iOS implementation (which is actually different than the Mac implementation):
int64_t ComputeCurrentTicks() {
#if defined(OS_IOS)
// On iOS mach_absolute_time stops while the device is sleeping. Instead use
// now - KERN_BOOTTIME to get a time difference that is not impacted by clock
// changes. KERN_BOOTTIME will be updated by the system whenever the system
// clock change.
struct timeval boottime;
int mib[2] = {CTL_KERN, KERN_BOOTTIME};
size_t size = sizeof(boottime);
int kr = sysctl(mib, arraysize(mib), &boottime, &size, NULL, 0);
DCHECK_EQ(KERN_SUCCESS, kr);
base::TimeDelta time_difference = base::Time::Now() -
(base::Time::FromTimeT(boottime.tv_sec) +
base::TimeDelta::FromMicroseconds(boottime.tv_usec));
return time_difference.InMicroseconds();
If I'm reading this correctly, even though it uses the KERN_BOTTIME thing, it actually subtracts this from Time::Now(), which is implemented thus:
Time Time::Now() {
return FromCFAbsoluteTime(CFAbsoluteTimeGetCurrent());
}
But, CFAbsoluteTimeGetCurrent() - which doesn't guarantee monotonic increasing times, from the docs:
"Repeated calls to this function do not guarantee monotonically increasing results. "
https://developer.apple.com/library/prerelease/ios/documentation/CoreFoundation/Reference/CFTimeUtils/index.html#//apple_ref/c/func/CFAbsoluteTimeGetCurrent
,
Apr 20 2016
What if we go back to the Mac codepath for the normal base API and implement the current technique specifically for the Omaha checks?
,
Apr 21 2016
Because the base API is used for more things than just omaha. Making a special case here might lead to worse issues in the future. I don't know but I can imagine things like network retry, or sync schedule using the same API… Let's back pedal a bit, can you tell us what the impact of this is? What is the largest negative number you've seen? Is it a small issue, or is it something we should really fix now or else?
,
Apr 21 2016
My point is the base API is documented explicitly as:
"// Represents monotonically non-decreasing clock time.
class BASE_EXPORT TimeTicks : public time_internal::TimeBase<TimeTicks> {"
You're using the argument that some code might rely on the some assumption about the API that is not explicitly documented and so we should break the assumption that is explicitly documented:
"// Note that TimeTicks may "stand still" (e.g., if the computer is suspended),".
,
Apr 21 2016
(Woops, lost a paragraph in my previous message.)
My point is the base API is documented explicitly as:
"// Represents monotonically non-decreasing clock time.
class BASE_EXPORT TimeTicks : public time_internal::TimeBase<TimeTicks> {"
You're using the argument that some code might rely on some assumption about the API that is not guaranteed by the API (time increments when device is suspended) and so we should break the assumption that is supposed to be guaranteed by the API (it doesn't go backwards).
Note that the fact that TimeTicks may stand still when computer is suspended is explicitly documented:
"// Note that TimeTicks may "stand still" (e.g., if the computer is suspended),".
If there are things that would like a time API that increments when computer goes to sleep and that don't care about time going backwards, then we should add such an API and make things that need this (e.g. Omaha) use that API.
,
Apr 21 2016
In terms of impact on UMA, it is not currently breaking UMA in the sense that our pipelines don't rely on this - but it wastes our time when debugging it. However, probably a more important effect is for histograms where TimeTicks is used to record time. Here, TimeTicks incrementing when the device is sleeping is actually likely detrimental - as it would record huge times if the timing window intersected with the device sleeping. And it would record times of 0 when time would go backwards w/ the current implementation. So, it would make timing histograms more noisy on iOS.
,
Apr 27 2017
,
Oct 12 2017
,
Oct 12
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 15
Based on comment 6, this does not sound like a serious issue (if it is an issue at all). |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by noyau@chromium.org
, Apr 20 2016