cc: Remove frame queuing from the scheduler.

cc: Remove frame queuing from the scheduler.

CC scheduler has a frame queuing mechanism called "retro frames". This
has been responsible for a lot of complexity and hard to fix bugs. The
original intent for adding retro frames was to allow the scheduler to
handle multiple frames in flight but that goal doesn't seem feasible or
even desirable any more. This CL removes the retro frames queue and
instead makes the Scheduler end the previous frame when it receives a
BeginFrame message.

One surprising behavior was that SyntheticBFS MISSED frames would be
queued as retro frames and this would convert the synchronous begin
frame call (inside Scheduler::ProcessScheduledActions) to an
asynchronous retro frame PostTask. To work around this the Scheduler
keeps track of a single CancelableClosure that's used for MISSED frames.

The above behavior was also causing the BeginFramesNotFromClient tests
to work even though there was an extra MISSED frame in the queue. This
is more elegantly solved in another way by using frame number to advance
the task runner instead of just running pending tasks.

Lastly SchedulerStateMachine is modified so that it's possible to end
the previous frame and still have the same behavior as before in the
commit to active tree (browser compositor) mode.

R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org
BUG=602485, 644992
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78
Cr-Commit-Position: refs/heads/master@{#417764}

[sunnyps, 2016-09-09 02:25:23]

Description was changed from

==========
cc: Remove frame queuing from the scheduler.

CC scheduler has a frame queuing mechanism called "retro frames". This
has been responsible for a lot of complexity and hard to fix bugs. The
original intent for adding retro frames was to allow the scheduler to
handle multiple frames in flight but that goal doesn't seem feasible or
even desirable any more. This CL removes the retro frames queue and
instead makes the Scheduler end the previous frame when it receives a
BeginFrame message.

One surprising behavior was that SyntheticBFS MISSED frames would be
queued as retro frames and this would convert the synchronous begin
frame call (inside Scheduler::ProcessScheduledActions) to an
asynchronous retro frame PostTask. To work around this the Scheduler
keeps track of a single CancelableClosure that's used for MISSED frames.

The above behavior was also causing the BeginFramesNotFromClient tests
to work even though there was an extra MISSED frame in the queue. This
is more elegantly solved in another way by using frame number to advance
the task runner instead of just running pending tasks.

Lastly SchedulerStateMachine is modified so that it's possible to end
the previous frame and still have the same behavior as before in the
commit to active tree (browser compositor) mode.

R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org
BUG=602485, 644992
==========

to

==========
cc: Remove frame queuing from the scheduler.

CC scheduler has a frame queuing mechanism called "retro frames". This
has been responsible for a lot of complexity and hard to fix bugs. The
original intent for adding retro frames was to allow the scheduler to
handle multiple frames in flight but that goal doesn't seem feasible or
even desirable any more. This CL removes the retro frames queue and
instead makes the Scheduler end the previous frame when it receives a
BeginFrame message.

One surprising behavior was that SyntheticBFS MISSED frames would be
queued as retro frames and this would convert the synchronous begin
frame call (inside Scheduler::ProcessScheduledActions) to an
asynchronous retro frame PostTask. To work around this the Scheduler
keeps track of a single CancelableClosure that's used for MISSED frames.

The above behavior was also causing the BeginFramesNotFromClient tests
to work even though there was an extra MISSED frame in the queue. This
is more elegantly solved in another way by using frame number to advance
the task runner instead of just running pending tasks.

Lastly SchedulerStateMachine is modified so that it's possible to end
the previous frame and still have the same behavior as before in the
commit to active tree (browser compositor) mode.

R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org
BUG=602485, 644992
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[sunnyps, 2016-09-09 02:25:34]

The CQ bit was checked by sunnyps@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 02:25:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunnyps, 2016-09-09 02:26:06]

PTAL

[commit-bot: I haz the power, 2016-09-09 03:28:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 03:28:46]

Dry run: This issue passed the CQ dry run.

[brianderson, 2016-09-09 07:35:58]

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (left):

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler.cc#o...
cc/scheduler/scheduler.cc:727: state->SetBoolean("begin_retro_frame_task",
Add a trace for the new missed task?

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:343:
SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE) {
Is it possible to reach this point in all begin_impl_frame_states? If not, maybe
add DCHECKs?

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler_stat...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine_unittest.cc:1130: // No draw because we
haven't received NotifyReadyToDraw yet.
Thanks for fixing this test. How was it drawing before without the
NotifyReadyToDraw!?

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler_unit...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:388:
task_runner_->RunTasksWhile(client_->FrameHasNotAdvancedCallback());
Nice. So much easier to read.

[enne (OOO), 2016-09-09 17:40:27]

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:304: &Scheduler::BeginImplFrameWithDeadline,
base::Unretained(this), args));
Why args and not adjusted_args? (Just trying to understand.)

[sunnyps, 2016-09-09 21:21:54]

Addressed all comments. PTAL.

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (left):

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler.cc#o...
cc/scheduler/scheduler.cc:727: state->SetBoolean("begin_retro_frame_task",
On 2016/09/09 07:35:58, brianderson wrote:
> Add a trace for the new missed task?

Done.

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:304: &Scheduler::BeginImplFrameWithDeadline,
base::Unretained(this), args));
On 2016/09/09 17:40:27, enne wrote:
> Why args and not adjusted_args? (Just trying to understand.)

This was a mistake. Thanks for catching this!

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler_stat...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine_unittest.cc:1130: // No draw because we
haven't received NotifyReadyToDraw yet.
On 2016/09/09 07:35:58, brianderson wrote:
> Thanks for fixing this test. How was it drawing before without the
> NotifyReadyToDraw!?

The scheduler doesn't post a deadline until we get the NotifyReadyToDraw. The
state machine is oblivious about this. After removing retro frames it's possible
for the deadline to run if a new frame comes in.

I plan to simplify the scheduler deadline logic in a follow-up CL.

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler_unit...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:388:
task_runner_->RunTasksWhile(client_->FrameHasNotAdvancedCallback());
On 2016/09/09 07:35:58, brianderson wrote:
> Nice. So much easier to read.

This will also make it easier to simplify the deadline logic (the tests depend
on the deadline to advance frames) so that we don't have a deadline if there are
no impl-side animations/scrolls.

[enne (OOO), 2016-09-09 21:56:27]

lgtm % brianderson

Glad to finally see retro frames go away, and excited to see deadline logic
cleaned up too.  :)

[brianderson, 2016-09-09 22:25:37]

lgtm

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:343:
SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE) {
On 2016/09/09 07:35:58, brianderson wrote:
> Is it possible to reach this point in all begin_impl_frame_states? If not,
maybe
> add DCHECKs?

I double checked and I don't think we can get here in
BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE. The if statement implies that we might
be able to though.

I think it would be clearer to DCHECK that we are not in
BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE, then change the if statement to only
check for BEGIN_IMPL_FRAME_STATE_INSIDE_BEGIN_FRAME.

In any case, this is mostly a nit and i'll leave it up to you.

[sunnyps, 2016-09-09 22:35:16]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2016-09-09 22:35:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianderson@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2323063004/#ps40001
(title: "nit")

[sunnyps, 2016-09-09 22:35:42]

Thanks!

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2323063004/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:343:
SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE) {
On 2016/09/09 22:25:37, brianderson wrote:
> On 2016/09/09 07:35:58, brianderson wrote:
> > Is it possible to reach this point in all begin_impl_frame_states? If not,
> maybe
> > add DCHECKs?
> 
> I double checked and I don't think we can get here in
> BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE. The if statement implies that we might
> be able to though.
> 
> I think it would be clearer to DCHECK that we are not in
> BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE, then change the if statement to only
> check for BEGIN_IMPL_FRAME_STATE_INSIDE_BEGIN_FRAME.
> 
> In any case, this is mostly a nit and i'll leave it up to you.

Sorry, I missed this in my last patch. Done now.

[commit-bot: I haz the power, 2016-09-09 22:35:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 23:34:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-09 23:38:57]

Description was changed from

==========
cc: Remove frame queuing from the scheduler.

CC scheduler has a frame queuing mechanism called "retro frames". This
has been responsible for a lot of complexity and hard to fix bugs. The
original intent for adding retro frames was to allow the scheduler to
handle multiple frames in flight but that goal doesn't seem feasible or
even desirable any more. This CL removes the retro frames queue and
instead makes the Scheduler end the previous frame when it receives a
BeginFrame message.

One surprising behavior was that SyntheticBFS MISSED frames would be
queued as retro frames and this would convert the synchronous begin
frame call (inside Scheduler::ProcessScheduledActions) to an
asynchronous retro frame PostTask. To work around this the Scheduler
keeps track of a single CancelableClosure that's used for MISSED frames.

The above behavior was also causing the BeginFramesNotFromClient tests
to work even though there was an extra MISSED frame in the queue. This
is more elegantly solved in another way by using frame number to advance
the task runner instead of just running pending tasks.

Lastly SchedulerStateMachine is modified so that it's possible to end
the previous frame and still have the same behavior as before in the
commit to active tree (browser compositor) mode.

R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org
BUG=602485, 644992
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Remove frame queuing from the scheduler.

CC scheduler has a frame queuing mechanism called "retro frames". This
has been responsible for a lot of complexity and hard to fix bugs. The
original intent for adding retro frames was to allow the scheduler to
handle multiple frames in flight but that goal doesn't seem feasible or
even desirable any more. This CL removes the retro frames queue and
instead makes the Scheduler end the previous frame when it receives a
BeginFrame message.

One surprising behavior was that SyntheticBFS MISSED frames would be
queued as retro frames and this would convert the synchronous begin
frame call (inside Scheduler::ProcessScheduledActions) to an
asynchronous retro frame PostTask. To work around this the Scheduler
keeps track of a single CancelableClosure that's used for MISSED frames.

The above behavior was also causing the BeginFramesNotFromClient tests
to work even though there was an extra MISSED frame in the queue. This
is more elegantly solved in another way by using frame number to advance
the task runner instead of just running pending tasks.

Lastly SchedulerStateMachine is modified so that it's possible to end
the previous frame and still have the same behavior as before in the
commit to active tree (browser compositor) mode.

R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org
BUG=602485, 644992
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78
Cr-Commit-Position: refs/heads/master@{#417764}
==========

[commit-bot: I haz the power, 2016-09-09 23:38:58]

Patchset 3 (id:??) landed as
https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78
Cr-Commit-Position: refs/heads/master@{#417764}

[Sam McNally, 2016-09-12 07:46:47]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2336493002/ by sammc@chromium.org.

The reason for reverting is: Broke ChromeScreenshotGrabberTest.TakeScreenshot on
 Linux ChromiumOS Tests (dbg)(1):
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....