Bug 94719 – [Chromium] Unnecessary delay when starting to update resources with an inactive vsync timer.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Reveman]

We're calling CCDelayBasedTimeSource::nextTickTime() before the timer is activated and nextTickTime() then returns 0.

[David Reveman]

Created attachment 159953 [details]
Patch

Simple fix. It might be more appropriate to re-factor CCScheduler to not call nextTickTime() on an inactive timer

[Nat Duca]

Comment on attachment 159953 [details]
Patch

I like this. @brianderson?

[David Reveman]

Increasing priority as this bug is causing animations driven of requestAnimationFrame to run at half of the nominal frame rate.

[James Robinson]

This seems OK.  The complexity here is getting scary.  Is there any way we could have caught this regression more directly?

I think Brian should take a look but we should land ASAP to get coverage back on the perf bots downstream.

[Brian Anderson]

I'm fine with getting this in, but would like to change it eventually, since it's lying about what the next tick time actually is.  Also, the behavior for nextTickTime becomes intertwined with specific use cases of CCScheduler, which is only okay for now because CCDelayBasedTimeSource isn't used in many places.

Note: There is a comment in CCDelayBasedTimeSource.h indicating nextTickTime will return 0 if not active.  That comment should be changed to reflect the new behavior.

[Brian Anderson]

Comment on attachment 159953 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:107
> +    return monotonicTimeNow() + m_currentParameters.interval;

jbates brought up a good point that this fake time will not be vsync aligned.  Could this cause the texture uploading to run long and prevent us from going active again ASAP?

[David Reveman]

(In reply to comment #6)
> (From update of attachment 159953 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159953&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:107
> > +    return monotonicTimeNow() + m_currentParameters.interval;
> 
> jbates brought up a good point that this fake time will not be vsync aligned.  Could this cause the texture uploading to run long and prevent us from going active again ASAP?

It looks to me as if tickTarget always becomes "now + interval" when the timer goes from inactive to active state. Maybe I'm missing something?

[David Reveman]

Created attachment 159996 [details]
Patch

Move comment to header

[David Reveman]

(In reply to comment #5)
> I'm fine with getting this in, but would like to change it eventually, since it's lying about what the next tick time actually is.  Also, the behavior for nextTickTime becomes intertwined with specific use cases of CCScheduler, which is only okay for now because CCDelayBasedTimeSource isn't used in many places.

Would changing CCScheduler so that it always activates the timer before calling nextTickTime() be enough?

> 
> Note: There is a comment in CCDelayBasedTimeSource.h indicating nextTickTime will return 0 if not active.  That comment should be changed to reflect the new behavior.

Fixed in latest patch.

[Brian Anderson]

> It looks to me as if tickTarget always becomes "now + interval" when the timer goes from inactive to active state. Maybe I'm missing something?

There is some additional logic in CCDelayBasedTimeSource::postNextTickTask that aligns "now" to the nearest vsync when the timer is activated.

> Would changing CCScheduler so that it always activates the timer before calling nextTickTime() be enough?

That would prevent us from lying and would also automatically align nextTickTime to a vsync.  We would end up with ticks that are no-ops, but the overhead should be negligible.  Sounds like a good trade off to me.

[Nat Duca]

Recapping verbal discussion, how about reusing the lastTickTime of the DBTS?

[David Reveman]

Created attachment 160016 [details]
Patch

Add nextTickTimeIfActivated and use postNextTickTask logic determine tick target when inactive

[WebKit Review Bot]

Comment on attachment 160016 [details]
Patch

Attachment 160016 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13561421

[David Reveman]

Created attachment 160021 [details]
Patch

Fix unit test and ASSERT in postNextTickTask

[Brian Anderson]

Comment on attachment 160021 [details]
Patch

New version looks good.  It returns a vsync aligned timestamp, the function name doesn't lie about what it's doing, and it doesn't spuriously activate ticking.

[WebKit Review Bot]

Comment on attachment 160021 [details]
Patch

Clearing flags on attachment: 160021

Committed r126431: <http://trac.webkit.org/changeset/126431>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.