Bug 60936

Created attachment 93719 [details] Patch

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

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

Created attachment 93720 [details] Patch

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

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

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.

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.

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

(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?

Created attachment 93797 [details] Patch

Created attachment 94709 [details] Patch

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?

Created attachment 94735 [details] Patch

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

This seems the same issue of https://bugs.webkit.org/show_bug.cgi?id=37743 Can we merge them into one?

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

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