Bug 62162

Summary: [Chromium] Implement monotonicallyIncreasingClock()
Product: WebKit Reporter: James Simonsen <simonjam>
Component: JavaScriptCoreAssignee: James Simonsen <simonjam>
Status: RESOLVED FIXED    
Severity: Normal CC: fishd, jamesr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 37743    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

Description James Simonsen 2011-06-06 15:53:24 PDT
Plumb TimeTicks through to implement monotonicallyIncreasingClock().
Comment 1 James Simonsen 2011-06-09 14:13:18 PDT
Created attachment 96641 [details]
Patch
Comment 2 Darin Fisher (:fishd, Google) 2011-06-10 12:29:21 PDT
Comment on attachment 96641 [details]
Patch

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

> Source/WebKit/chromium/public/WebKitClient.h:270
> +    virtual double monotonicallyIncreasingTime() { return 0; }

i'm curious about using a double here.  it seems like this clock is
useful for computing time deltas (which will often be on the order of
milliseconds).  as the magnitude of this clock increases, won't the
precision of the difference between two nearby values decrease?
wouldn't it be better to use an int64_t in other words?
Comment 3 Darin Fisher (:fishd, Google) 2011-06-10 12:34:03 PDT
i think what i'm really trying to say is that it is important for the origin (0-point) of the clock to be well within the 53-bit precision of a double.  making the origin be the epoch accomplishes this, but the definition of the clock doesn't make that clear.  if you used int64_t, then you would be force to make that clear.

maybe a double is used to avoid wrap-around issues, but instead of wrap-around issues, you have loss of precision issues, which are also problematic.  using int64_t, and saying that the clock has an origin of process startup time, would probably be much clearer and less problematic.
Comment 4 James Simonsen 2011-06-10 13:51:32 PDT
(In reply to comment #3)
> i think what i'm really trying to say is that it is important for the origin (0-point) of the clock to be well within the 53-bit precision of a double.  making the origin be the epoch accomplishes this, but the definition of the clock doesn't make that clear.  if you used int64_t, then you would be force to make that clear.
> 
> maybe a double is used to avoid wrap-around issues, but instead of wrap-around issues, you have loss of precision issues, which are also problematic.  using int64_t, and saying that the clock has an origin of process startup time, would probably be much clearer and less problematic.

(This discussion might be better on the parent bug.)

I expect all platforms will do what you've described, although maybe 0 ends up being boot time instead of process start. The main reason I avoided specifying 0 is that I didn't want everyone to have to do a bunch of math to normalize behavior across platforms when we really don't care about that. I want people to use whatever millisecond resolution monotonic clock is convenient. In practice, I expect 0 won't be any farther away than the epoch, so we should have plenty of precision to handle milliseconds.

I also have a slight desire that the epoch not be used so that it's harder to confuse monotonic times with currentTime() when debugging. I did initially implement this with uint64_t for this reason, although that's far from foolproof.

Can you think of some other way to spec these requirements? 

I felt (other) Darin's comment about using doubles was more for style and consistency reasons. I didn't think wrapping was a concern.
Comment 5 Darin Fisher (:fishd, Google) 2011-06-10 22:38:25 PDT
I think you should at least spell out the requirements for the embedder, who has to implement this new clock.

I understand the stylistic reasons for preferring double.  I do wonder if it wouldn't lead to mistakes, in which we confuse this clock for the wall clock because the types are the same :-(
Comment 6 James Simonsen 2011-06-13 17:43:36 PDT
Created attachment 97046 [details]
Patch
Comment 7 WebKit Review Bot 2011-06-13 18:14:09 PDT
Comment on attachment 97046 [details]
Patch

Clearing flags on attachment: 97046

Committed r88745: <http://trac.webkit.org/changeset/88745>
Comment 8 WebKit Review Bot 2011-06-13 18:14:14 PDT
All reviewed patches have been landed.  Closing bug.