Bug 60936 – Add currentTickCount() to get monotonically increasing time

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebCore Misc.

[James Simonsen]

We need a way to measure time without being affected by changes to the user's clock. Tick count should provide a monotonically increasing time that can be used to measure duration. We'll use this for Navigation Timing and it will also be useful for timing animations.

[James Simonsen]

Created attachment 93719 [details]
Patch

[James Simonsen]

This is just a primitive initial implementation. Each platform will need to implement this currentTickCount() correctly.

[James Robinson]

Comment on attachment 93719 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93719&action=review

> Source/WebCore/platform/chromium/SystemTimeChromium.cpp:48
> +    uint64_t tickCount = static_cast<uint64_t>(currentTime() * 1000000.0);

why is this currentTime() * 1000000.0 when the wtf/ version is currentTimeMS() * 1000.0?

nit: i'm bad at counting zeros so I like to write this sort of expression as 1000 * 1000

[James Simonsen]

Created attachment 93720 [details]
Patch

[James Simonsen]

(In reply to comment #3)
> (From update of attachment 93719 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93719&action=review
> 
> > Source/WebCore/platform/chromium/SystemTimeChromium.cpp:48
> > +    uint64_t tickCount = static_cast<uint64_t>(currentTime() * 1000000.0);
> 
> why is this currentTime() * 1000000.0 when the wtf/ version is currentTimeMS() * 1000.0?

It means an extra #include. I changed it so they match. (Note that the Chromium version will be replaced shortly anyway.)

[James Robinson]

+cc some folks for comment on the API

Monotonically non-decreasing isn't the most interesting property - uniformly increasing is.  I think this is a step in the right direction and assume that we'll want to implement this with the appropriate system APIs (mach_absolute_time(), QueryPerformanceCounter(), etc).  I'm not sure "tick count" is the correct thing to call this - in chromium we call this TimeTicks, although in practice I always want to use this sort of API to measure intervals or durations.

[David Levin]

Comment on attachment 93720 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93720&action=review

> Source/JavaScriptCore/wtf/CurrentTime.cpp:302
> +        return lastTickCount;

1. This is clearly wrong since lastTickCount isn't set.
2. I find it sad that we have two copies of this code.
3. How does this work in the presence of multiple threads?

Ditto for the other instance of this code.

[Alexey Proskuryakov]

The name "tick count" is quite opaque. Are you intentionally abstracting away how long a tick takes? Why is it not 1/60th of a second?

> Monotonically non-decreasing isn't the most interesting property - uniformly increasing is.

I wonder if uniformly increasing time across sleeps can be implemented on a sufficiently large amount of platforms.

[Dean Jackson]

I'm more interested in the duration of the document. I wonder why you describe the API as from an arbitrary point? Also, like JamesR, I expect this to be always increasing, not not-decreasing. It seems that in the current patch the tick would stop for an hour if the user's real clock moved backwards an hour.

Also, you never assign lastTickCount = tickCount. (Maybe you meant to do that in both return cases, in which case the tick would only stop once).

[James Simonsen]

(In reply to comment #7)
> (From update of attachment 93720 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93720&action=review
> 
> > Source/JavaScriptCore/wtf/CurrentTime.cpp:302
> > +        return lastTickCount;
> 
> 1. This is clearly wrong since lastTickCount isn't set.

Yep, that was stupid. :( Fixed.

> 2. I find it sad that we have two copies of this code.

It's only temporary. I expect each platform to implement it correctly. Chromium is special because it doesn't compile CurrentTime.cpp.

> 3. How does this work in the presence of multiple threads?

It should work as well as currentTime() works now. There are similar blocks of code elsewhere in this file.

(In reply to comment #8)
> The name "tick count" is quite opaque. Are you intentionally abstracting away how long a tick takes? Why is it not 1/60th of a second?

No, I'm not trying to abstract it away. I only wanted a name that makes it clear it's different than currentTime() so they're not mixed up. The choice of units is somewhat arbitrary. I chose microseconds since that's what Chromium uses. Really, I'd be happy with anything that has at least millisecond resolution. Does anyone else have a preference?

(In reply to comment #9)
> I'm more interested in the duration of the document. I wonder why you describe the API as from an arbitrary point?

That's definitely what I intend to use it for. We can grab a reference tick count when the document is created and then get deltas from that.

The purpose of the API is to measure duration, so the absolute value returned shouldn't be important. That's why I call it arbitrary. The platform can choose whatever point in time is convenient.

> Also, like JamesR, I expect this to be always increasing, not not-decreasing. It seems that in the current patch the tick would stop for an hour if the user's real clock moved backwards an hour.

Yeah, my hope is that nobody ever uses this implementation. Ideally, each platform will implement a true always increasing clock.

Perhaps I shouldn't even provide this clearly hacky default implementation? Should I instead put this behind an ENABLE and force platforms to implement it in order to use features that require it? Or should we try to implement it correctly on all platforms in this patch?

[James Simonsen]

Created attachment 93797 [details]
Patch

[James Simonsen]

Created attachment 94709 [details]
Patch

[James Robinson]

Comment on attachment 94709 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94709&action=review

This seems like a good start to me - there's a good implementation here for PLATFORM(MAC) and a reasonable fallback for other platforms, so we can start depending on it now and then improve the implementations on each port as we go.  Does that sound good to everyone?

> Source/JavaScriptCore/wtf/CurrentTime.cpp:310
> +    return mach_absolute_time() / 1000 * timebaseInfo.numer / timebaseInfo.denom;

nit: the order of operations/evaluation here is hard to follow, could you reorder or add some parens?

[James Simonsen]

Created attachment 94735 [details]
Patch

[Alexey Proskuryakov]

*** Bug 61456 has been marked as a duplicate of this bug. ***

[Yong Li]

This seems the same issue of https://bugs.webkit.org/show_bug.cgi?id=37743

Can we merge them into one?

[Darin Adler]

*** This bug has been marked as a duplicate of bug 37743 ***

[Eric Seidel (no email)]

Comment on attachment 94735 [details]
Patch

Cleared review? from attachment 94735 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).