cc: Cleanup DelayBasedTimeSource code.

cc: Cleanup DelayBasedTimeSource code.

BUG=463198
R=brianderson,mithro
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2452de2def483ca4011194e185bc8ed3a1b49bb3
Cr-Commit-Position: refs/heads/master@{#337734}

[sunnyps, 2015-06-23 01:35:44]

PTAL

Requires https://codereview.chromium.org/1205483004/ to go in first.

[brianderson, 2015-06-23 01:47:56]

lgtm. I think you can remove my DisplayScheduler hack that posts missed
BeginFrames from the delegate now.

[mithro-old, 2015-06-23 02:45:39]

On 2015/06/23 01:47:56, brianderson wrote:
> lgtm. I think you can remove my DisplayScheduler hack that posts missed
> BeginFrames from the delegate now.

Not LGTM for the moment.

Need to take a closer look but I think this could result in
BeginFrameArgs.frame_time going backwards due to a task already been posted then
posting a missed one?

[mithro-old, 2015-06-23 02:57:20]

Looking at the delay_based_time_source.cc, when SetActive(true) is called it
posts a DelayedTask which will end up calling OnTimerTick which sends a
BeginFrameArgs with the tick time. With the new PostTask for the MISSED, there
is a (very small) possibility this delayed task could be called before the
missed one and hence end up with time going backwards (as the missed value is
always strictly less than the delay task's value).

The only way this would be safe is if the TaskRunner *guaranteed* running all
PostTask's before PostDelayedTask's - which I'm not sure is the case?

[sunnyps, 2015-06-23 18:37:45]

On 2015/06/23 02:57:20, mithro wrote:
> Looking at the delay_based_time_source.cc, when SetActive(true) is called it
> posts a DelayedTask which will end up calling OnTimerTick which sends a
> BeginFrameArgs with the tick time. With the new PostTask for the MISSED, there
> is a (very small) possibility this delayed task could be called before the
> missed one and hence end up with time going backwards (as the missed value is
> always strictly less than the delay task's value).
> 
> The only way this would be safe is if the TaskRunner *guaranteed* running all
> PostTask's before PostDelayedTask's - which I'm not sure is the case?

This is guaranteed by SequencedTaskRunner. Here's the relevant excerpt:

// The guarantees of SequencedTaskRunner are as follows:
//
//   - Given two tasks T2 and T1, T2 will start after T1 starts if:
//
//       * T2 is posted after T1; and
//       * T2 has equal or higher delay than T1; and
//       * T2 is non-nestable or T1 is nestable.

PostTasks are tasks with delay 0 so they necessarily run before PostDelayedTasks
with non-zero delay. That leaves exactly one case which is problematic - if
Now() == next_tick_target then it's possible for us to get the MISSED frame
after the regular frame.

I think we can specifically check for that in SetActive. This also suggests that
we should have some sort of threshold for posting MISSED frames i.e. if
(next_tick_target - Now()) < interval / kMissedFrameThreshold then we should not
send a MISSED frame.

brianderson@ any thoughts on the threshold?

[sunnyps, 2015-06-23 20:21:58]

Added a check to ensure that we don't send the MISSED frame if we're going to
send a NORMAL frame.

PTAL

[brianderson, 2015-06-23 21:00:17]

On 2015/06/23 18:37:45, sunnyps wrote:
> On 2015/06/23 02:57:20, mithro wrote:
> > Looking at the delay_based_time_source.cc, when SetActive(true) is called it
> > posts a DelayedTask which will end up calling OnTimerTick which sends a
> > BeginFrameArgs with the tick time. With the new PostTask for the MISSED,
there
> > is a (very small) possibility this delayed task could be called before the
> > missed one and hence end up with time going backwards (as the missed value
is
> > always strictly less than the delay task's value).
> > 
> > The only way this would be safe is if the TaskRunner *guaranteed* running
all
> > PostTask's before PostDelayedTask's - which I'm not sure is the case?
> 
> This is guaranteed by SequencedTaskRunner. Here's the relevant excerpt:
> 
> // The guarantees of SequencedTaskRunner are as follows:
> //
> //   - Given two tasks T2 and T1, T2 will start after T1 starts if:
> //
> //       * T2 is posted after T1; and
> //       * T2 has equal or higher delay than T1; and
> //       * T2 is non-nestable or T1 is nestable.
> 
> PostTasks are tasks with delay 0 so they necessarily run before
PostDelayedTasks
> with non-zero delay. That leaves exactly one case which is problematic - if
> Now() == next_tick_target then it's possible for us to get the MISSED frame
> after the regular frame.

What happens if T2 is posted before T1 though? DBTS will post the next tick
before we get to post the missed frame.

Maybe we can add a TimeSourceClient::OnMissedTimerTick() that the DBTS posts
before the next tick?

> 
> I think we can specifically check for that in SetActive. This also suggests
that
> we should have some sort of threshold for posting MISSED frames i.e. if
> (next_tick_target - Now()) < interval / kMissedFrameThreshold then we should
not
> send a MISSED frame.
> 
> brianderson@ any thoughts on the threshold?

Depends on the use case. If there are no impl-side updates, we don't want to
insert unnecessary waits for the next BeginFrame. If there are, we might want to
wait for more user input if we are close to the next BeginFrame.

We have better things to do than try to be 100% correct in that case so a hard
coded threshold would be okay. Don't we already discard BeginFrames that are
passed their deadline though?

[brianderson, 2015-06-23 21:00:29]

https://codereview.chromium.org/1200113003/diff/20001/cc/scheduler/delay_base...
File cc/scheduler/delay_based_time_source.cc (left):

https://codereview.chromium.org/1200113003/diff/20001/cc/scheduler/delay_base...
cc/scheduler/delay_based_time_source.cc:233: if (now <= new_tick_target)
I forget why this was needed. Could small jitter in the timebase cause this
condition to hit?

https://codereview.chromium.org/1200113003/diff/20001/cc/scheduler/delay_base...
File cc/scheduler/delay_based_time_source.cc (right):

https://codereview.chromium.org/1200113003/diff/20001/cc/scheduler/delay_base...
cc/scheduler/delay_based_time_source.cc:234: DCHECK(new_tick_target >= now);
DCHECK_GE

[sunnyps, 2015-06-26 00:45:28]

PTAL

This version makes DelayBasedTimeSource responsible for handling both missed
frames and normal frames - they are different signals to the client. I
considered the following alternative solutions:

1. Strip out the missed time logic from SetActive and make the SyntheticBFS post
a missed task before it calls SetActive. This makes it hard to update the
last_tick_time_ variable in DBTS - we want it to track both missed and normal
ticks.
2. Add a new TickType enum in DelayBasedTimeSource that SyntheticBFS looks at
when calling OnBeginFrame. This went out of hand really quickly.

Also cleaned up the code in DBTS e.g. unnecessary virtual methods (leftovers
from TimeSource), renaming the variables, etc.

Some tests fail right now:
    DelayBasedTimeSourceTest.TestDeactivateAndReactivateAfterNextTickTime
    SyntheticBeginFrameSourceTest.BasicOperation
    SyntheticBeginFrameSourceTest.SetNeedsBeginFramesCallsCausesOnBeginFrame
    SyntheticBeginFrameSourceTest.VSyncChanges

But the test failures seem to be because the tests don't expect a missed frame
showing up. I also discovered that the missed begin frame stuff is not well
tested so we'll need to add some tests for that.

I expect some more cleanups to be done - e.g. SyntheticBFS doesn't need a task
runner now.

https://codereview.chromium.org/1200113003/diff/20001/cc/scheduler/delay_base...
File cc/scheduler/delay_based_time_source.cc (left):

https://codereview.chromium.org/1200113003/diff/20001/cc/scheduler/delay_base...
cc/scheduler/delay_based_time_source.cc:233: if (now <= new_tick_target)
On 2015/06/23 21:00:29, brianderson wrote:
> I forget why this was needed. Could small jitter in the timebase cause this
> condition to hit?

This condition should always be true. See the DCHECK above.

[brianderson, 2015-06-26 19:24:35]

Approach sgtm. Thanks for cleaning up the DBTS code as well.

https://codereview.chromium.org/1200113003/diff/40001/cc/scheduler/delay_base...
File cc/scheduler/delay_based_time_source.cc (right):

https://codereview.chromium.org/1200113003/diff/40001/cc/scheduler/delay_base...
cc/scheduler/delay_based_time_source.cc:79:
base::Bind(&DelayBasedTimeSource::OnMissedTick,
Can you bind to a Closure (in the constructor and in ResetTickTask) and reuse
that closure here rather than dynamically binding?

https://codereview.chromium.org/1200113003/diff/40001/cc/scheduler/delay_base...
cc/scheduler/delay_based_time_source.cc:96: base::TimeTicks
DelayBasedTimeSource::MissedTickTime() const {
Is this needed? Looks like Client::OnMissedTick can look at LastTickTime to get
the same info.

https://codereview.chromium.org/1200113003/diff/40001/cc/scheduler/delay_base...
cc/scheduler/delay_based_time_source.cc:101: return next_tick_time_;
Probably doesn't matter for the current user of this class, but should this
theoretically return the missed_tick_time_ if an OnMissedTick is pending?

https://codereview.chromium.org/1200113003/diff/40001/cc/scheduler/delay_base...
cc/scheduler/delay_based_time_source.cc:104: void
DelayBasedTimeSource::OnMissedTick() {
Should missed_tick_time_ be reset somewhere in this method?

https://codereview.chromium.org/1200113003/diff/40001/cc/scheduler/delay_base...
cc/scheduler/delay_based_time_source.cc:253:
base::Bind(&DelayBasedTimeSource::OnTimerTick,
Ditto regarding dynamic bind.

[mithro-old, 2015-06-30 08:11:52]

I agree with brianderson that it would be cleaner to use closures rather than
the current method, but I don't really think it is worth the effort to fix it.

I'm a little worried that the changes to DelayBasedTimeSource might affect it
behaviour, but if the tests pass, then I'm happy.

Otherwise a LGTM to cancel my other N-O-T LGTM.

Tim 'mithro' Ansell

https://codereview.chromium.org/1200113003/diff/40001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/1200113003/diff/40001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:248: void
SyntheticBeginFrameSource::OnMissedTick() {
I feel like these callbacks should just take a time value?

https://codereview.chromium.org/1200113003/diff/40001/cc/scheduler/delay_base...
File cc/scheduler/delay_based_time_source.cc (right):

https://codereview.chromium.org/1200113003/diff/40001/cc/scheduler/delay_base...
cc/scheduler/delay_based_time_source.cc:64: weak_factory_.InvalidateWeakPtrs();
It's really weird that this works by invalidating weak pointers...

https://codereview.chromium.org/1200113003/diff/40001/cc/scheduler/delay_base...
File cc/scheduler/delay_based_time_source.h (left):

https://codereview.chromium.org/1200113003/diff/40001/cc/scheduler/delay_base...
cc/scheduler/delay_based_time_source.h:87: // reflect the actual time we expect
OnTimerFired to be called.
Is this still preserved? I assume there was a test for it?

https://codereview.chromium.org/1200113003/diff/40001/cc/scheduler/delay_base...
File cc/scheduler/delay_based_time_source.h (right):

https://codereview.chromium.org/1200113003/diff/40001/cc/scheduler/delay_base...
cc/scheduler/delay_based_time_source.h:25: virtual void OnTimerTick() = 0;
I feel like these callbacks should just take a time value?

[sunnyps, 2015-07-02 01:01:14]

PTAL.

I removed the separate PostTask for the missed begin frame from this CL. I found
that it wasn't crucial for removing retro frames. I have kept the cleanup of
DBTS in this CL. So this CL is purely a cleanup CL at this point.

[sunnyps, 2015-07-02 01:01:38]

On 2015/07/02 at 01:01:14, sunnyps wrote:
> PTAL.
> 
> I removed the separate PostTask for the missed begin frame from this CL. I
found that it wasn't crucial for removing retro frames. I have kept the cleanup
of DBTS in this CL. So this CL is purely a cleanup CL at this point.

Also addressed the comments about reusing a closure.

[mithro-old, 2015-07-06 11:29:13]

LGTM.

https://codereview.chromium.org/1200113003/diff/80001/cc/scheduler/delay_base...
File cc/scheduler/delay_based_time_source.cc (right):

https://codereview.chromium.org/1200113003/diff/80001/cc/scheduler/delay_base...
cc/scheduler/delay_based_time_source.cc:59: return base::TimeTicks();
nit: I think it would be good to have a new line between 59 and 60?

https://codereview.chromium.org/1200113003/diff/80001/cc/scheduler/delay_base...
File cc/scheduler/delay_based_time_source.h (right):

https://codereview.chromium.org/1200113003/diff/80001/cc/scheduler/delay_base...
cc/scheduler/delay_based_time_source.h:51: // Returns the time for the the last
missed tick.
nit: the the

https://codereview.chromium.org/1200113003/diff/80001/cc/scheduler/delay_base...
cc/scheduler/delay_based_time_source.h:80: 
nit: Should some of these actually be private?

[brianderson, 2015-07-06 23:36:24]

lgtm

[sunnyps, 2015-07-07 23:21:59]

On 2015/07/06 11:29:13, mithro wrote:
> LGTM.
> 
>
https://codereview.chromium.org/1200113003/diff/80001/cc/scheduler/delay_base...
> File cc/scheduler/delay_based_time_source.cc (right):
> 
>
https://codereview.chromium.org/1200113003/diff/80001/cc/scheduler/delay_base...
> cc/scheduler/delay_based_time_source.cc:59: return base::TimeTicks();
> nit: I think it would be good to have a new line between 59 and 60?

Done.

> 
>
https://codereview.chromium.org/1200113003/diff/80001/cc/scheduler/delay_base...
> File cc/scheduler/delay_based_time_source.h (right):
> 
>
https://codereview.chromium.org/1200113003/diff/80001/cc/scheduler/delay_base...
> cc/scheduler/delay_based_time_source.h:51: // Returns the time for the the
last
> missed tick.
> nit: the the

Done.

> 
>
https://codereview.chromium.org/1200113003/diff/80001/cc/scheduler/delay_base...
> cc/scheduler/delay_based_time_source.h:80: 
> nit: Should some of these actually be private?

Done.

[sunnyps, 2015-07-07 23:22:11]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-07-07 23:22:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianderson@chromium.org, mithro@mithis.com
Link to the patchset: https://codereview.chromium.org/1200113003/#ps120001
(title: "mithro's review")

[commit-bot: I haz the power, 2015-07-07 23:22:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1200113003/120001

[commit-bot: I haz the power, 2015-07-08 02:09:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-08 02:11:11]

Patchset 7 (id:??) landed as
https://crrev.com/2452de2def483ca4011194e185bc8ed3a1b49bb3
Cr-Commit-Position: refs/heads/master@{#337734}