Bug 93316 – [chromium] add highResolutionMonotonicallyIncreasingTime to Platform API

• Component: Platform
• Status: RESOLVED INVALID

[John Bates]

[chromium] add highResolutionMonotonicallyIncreasingTime to Platform API

[John Bates]

Created attachment 156819 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

Comment on attachment 156819 [details]
Patch

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

> Source/Platform/chromium/public/Platform.h:303
> +    virtual double highResolutionMonotonicallyIncreasingTime() { return 0; }

do we know if this clock actually monotonically increasing?

[John Bates]

(In reply to comment #3)
> (From update of attachment 156819 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156819&action=review
> 
> > Source/Platform/chromium/public/Platform.h:303
> > +    virtual double highResolutionMonotonicallyIncreasingTime() { return 0; }
> 
> do we know if this clock actually monotonically increasing?

It is unless the system has a specific Windows XP / AMD driver bug (discussed here: http://support.microsoft.com/kb/895980).

Maybe the name should indicate the risk of using it?

[James Robinson]

I think we should put a warning somewhere or add some code to detect+correct for this case within the implementation.

[John Bates]

Created attachment 157037 [details]
Patch

[John Bates]

Comment on attachment 157037 [details]
Patch

(In reply to comment #5)
> I think we should put a warning somewhere or add some code to detect+correct for this case within the implementation.

I just checked our chromium implementation and it turns out we already check for the buggy AMD family. When the faulty class is detected we fall back on the regular time (same as monotonicallyIncreasingTime).

In the latest patch I updated the comment to indicate the fallback behavior.

[John Bates]

Created attachment 157060 [details]
Patch

[John Bates]

Comment on attachment 157060 [details]
Patch

Updated the CCDelayBasedTimeSource to use the new API.

[James Robinson]

Comment on attachment 157060 [details]
Patch

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

> Source/Platform/chromium/public/Platform.h:313
> +    virtual double highResolutionMonotonicallyIncreasingTime() { return 0; }

Is this call expected to be more expensive than monotonicallyIncreasingTime()? Should we warn callers?

> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:31
>  #include <wtf/CurrentTime.h>

go ahead and delete this

> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:161
> +    return WebKit::Platform::current()->highResolutionMonotonicallyIncreasingTime();

I'm a little nervous about this differing from monotonicallyIncreasingTime() and want Nat to weigh in on the implications.  One thing in particular that's different is that PostDelayedTask uses the monotonicallyIncreasingTime() clock, so if you have a Timer or a delayed task with a delay of X milliseconds you are promised that when that task runs monotonicallyIncreasingTime() will have increased by at least X.  With this being a different clock, is that behavior still guaranteed or not?  If not, is all of our timing logic OK with that?

[Nat Duca]

Is this really necessary?

[Nat Duca]

Oh, I get it. The issue is that monotonicallyIncreasingTime isn't backed by TimeTicks::HighresNow, right?

I guess the devils advocate in me wonders whether we can't just redirect to highres all the time?

[John Bates]

So after discussing, it seems that the timebase should be converted from QPC time-space to TimeTicks::Now time-space. Theoretically, we can just do:

now_based_timebase = qpc_timebase + TimeTicks::Now() - TimeTicks::HighResNow();

The one issue is when Now() granularity is low (16ms), which is the case when any of the following are true:
- we haven't scheduled a task at less than 16ms granularity.
- we are running on battery power.

So... In any case, this patch is probably not necessary, because either way we still have to deal with (or fix) the PostTask timer resolution.

[John Bates]

Marking invalid for now.