Scheduler: Switch from high to low latency mode if possible.

Scheduler: Switch from high to low latency mode if possible.

Detect if the renderer's impl thread is one frame behind the main thread (i.e. the main thread is in a high
latency mode). If, based on our estimates, the main thread can produce a new tree
before the impl thread's deadline, skip a BeginMainFrame to let the impl thread
catch up and operate in low latency mode from then on.

This feature is currently disabled by default.

BUG=309648
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233734

[Dominik Grewe, 2013-10-31 16:03:16]

PTAL. Feel free to suggest different names :)

[brianderson, 2013-11-01 00:22:59]

Can you add a few scheduler unit tests that:
1) Make sure a main frame is skipped if we are in a high latency mode and the
draw + activation estimates are fast enough to beat the deadline.
2) Make sure a main frame is not skipped if draw is slow.
3) Make sure a main frame is not skipped if activate is slow.

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

https://codereview.chromium.org/54913003/diff/1/cc/scheduler/scheduler.h#newc...
cc/scheduler/scheduler.h:136: bool CanOperateInLowLatencyMode() const;
CanCommitAndDrawBeforeDeadline?

https://codereview.chromium.org/54913003/diff/1/cc/scheduler/scheduler_state_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/54913003/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:939: if (active_tree_needs_first_draw_
|| has_pending_tree_ )
Can you split this condition into separate if statements and add a comment above
each condition explaining why we are in a high latency mode?

https://codereview.chromium.org/54913003/diff/1/cc/scheduler/scheduler_state_...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/54913003/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.h:150: // behind the main thread. We're
thus in high latency mode.
"the main thread is lagging behind the impl thread"

https://codereview.chromium.org/54913003/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.h:216: void SetSkipBeginMainFrame(bool
skip);
SkipNextBeginMainFrame()?

Actually, nevermind; this will allow us to change our mind immediately after a
long commit or activate.

https://codereview.chromium.org/54913003/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.h:297: bool skip_begin_main_frame_;
skip_begin_main_frame_to_reduce_latency_

[Dominik Grewe, 2013-11-01 17:09:13]

Some updates. Will add unit tests next.

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

https://codereview.chromium.org/54913003/diff/1/cc/scheduler/scheduler.h#newc...
cc/scheduler/scheduler.h:136: bool CanOperateInLowLatencyMode() const;
On 2013/11/01 00:22:59, Brian Anderson wrote:
> CanCommitAndDrawBeforeDeadline?

But we're drawing after the deadline, right? How about
CanPaintAndCommitBeforeDeadline?

https://codereview.chromium.org/54913003/diff/1/cc/scheduler/scheduler_state_...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/54913003/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.h:150: // behind the main thread. We're
thus in high latency mode.
On 2013/11/01 00:22:59, Brian Anderson wrote:
> "the main thread is lagging behind the impl thread"

But the main thread is one frame ahead, isn't it?

https://codereview.chromium.org/54913003/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.h:297: bool skip_begin_main_frame_;
On 2013/11/01 00:22:59, Brian Anderson wrote:
> skip_begin_main_frame_to_reduce_latency_

Done. Also changed the name of the corresponding setter method.

[brianderson, 2013-11-01 21:18:30]

https://codereview.chromium.org/54913003/diff/1/cc/scheduler/scheduler_state_...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/54913003/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.h:150: // behind the main thread. We're
thus in high latency mode.
On 2013/11/01 17:09:14, Dominik Grewe wrote:
> On 2013/11/01 00:22:59, Brian Anderson wrote:
> > "the main thread is lagging behind the impl thread"
> 
> But the main thread is one frame ahead, isn't it?

The main thread gets into a high latency mode by being too slow. In the high
latency mode, the current commit would have been initiated by the previous
BeginImplFrame, so the main thread is lagging.

https://codereview.chromium.org/54913003/diff/80001/cc/scheduler/scheduler_st...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/54913003/diff/80001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:932: bool
SchedulerStateMachine::InHighLatencyMode() const {
Maybe rename this to MainThreadIsInHighLatencyMode?

https://codereview.chromium.org/54913003/diff/80001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:935: if (begin_impl_frame_state_ !=
BEGIN_IMPL_FRAME_STATE_BEGIN_FRAME_STARTING)
Can you add the result of this method to the output of AsValue? It would be good
to make this method return a valid result regardless of the state we are in, in
which case I think you can change this if to read:

// If we just sent a BeginMainFrame and haven't hit the deadline yet, the main
// thread is in a low latency mode.
if (last_frame_number_begin_main_frame_sent_ == current_frame_number_ &&
    (begin_impl_frame_state_ == BEGIN_IMPL_FRAME_STATE_BEGIN_FRAME_STARTING ||
     begin_impl_frame_state_ == BEGIN_IMPL_FRAME_STATE_INSIDE_BEGIN_FRAME))
  return false;

https://codereview.chromium.org/54913003/diff/80001/cc/scheduler/scheduler_st...
cc/scheduler/scheduler_state_machine.cc:949: if (active_tree_needs_first_draw_)
To make this method valid regardless of the state we are in, I think you'd also
need to add the following if statement here:

if (begin_impl_frame_state == BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE) {
  // Even if there's a new active tree to draw at the deadline it may have
  // been triggered by a previous BeginImplFrame, in which case the main thread
  // is in a high latency mode.
  return active_tree_needs_first_draw_ &&
         last_frame_number_begin_main_frame_sent_ != current_frame_number_;
} else {
  // If the active tree needs it's first draw in any other state, we know
  // the main thread is in a high latency mode.
  return active_tree_needs_first_draw_;
}

[Dominik Grewe, 2013-11-04 14:10:41]

I've updated MainThreadIsInHighLatencyMode so it works no matter what state
we're in. PTAL.

Tests coming soon :)

[brianderson, 2013-11-05 00:24:17]

Looking forward to the tests.

https://codereview.chromium.org/54913003/diff/180001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/54913003/diff/180001/cc/scheduler/scheduler.h...
cc/scheduler/scheduler.h:136: bool CanPaintAndCommitBeforeDeadline() const;
Since commit happens before rasterization + activation, maybe call this
CanCommitAndActivateBeforeDeadline.

Activate is more general and includes rasterization + texture upload time.

Also note that, although the feature is called impl-side-painting, I think most
of the code refers to rasterization rather than painting.

https://codereview.chromium.org/54913003/diff/180001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/54913003/diff/180001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:963: last_frame_number_swap_performed_
== current_frame_number_) &&
Ah, good catch here.

https://codereview.chromium.org/54913003/diff/180001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:971: return false;
Hmm, this will never be reached. Maybe we can just remove the else block above
and put it's body at the end of the method.

[Dominik Grewe, 2013-11-05 12:24:19]

I've added some unit tests as requested. Please have a look to see if that's
what you were thinking about. Is there an easier way to get the main thread into
high latency mode (or somehow fake it)?

https://codereview.chromium.org/54913003/diff/180001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/54913003/diff/180001/cc/scheduler/scheduler.h...
cc/scheduler/scheduler.h:136: bool CanPaintAndCommitBeforeDeadline() const;
On 2013/11/05 00:24:17, Brian Anderson wrote:
> Since commit happens before rasterization + activation, maybe call this
> CanCommitAndActivateBeforeDeadline.
> 
> Activate is more general and includes rasterization + texture upload time.
> 
> Also note that, although the feature is called impl-side-painting, I think
most
> of the code refers to rasterization rather than painting.
> 

Sgtm.

https://codereview.chromium.org/54913003/diff/180001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/54913003/diff/180001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:971: return false;
On 2013/11/05 00:24:17, Brian Anderson wrote:
> Hmm, this will never be reached. Maybe we can just remove the else block above
> and put it's body at the end of the method.

Done.

[brianderson, 2013-11-06 02:16:54]

lgtm.

Thanks for the tests, they do exactly what I had in mind.

As a followup patch, can you add a command-line flag that will enable this
setting so we can more easily test the pros/cons?

[brianderson, 2013-11-06 02:38:06]

https://codereview.chromium.org/54913003/diff/290001/cc/scheduler/scheduler_u...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/54913003/diff/290001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:1131: scheduler->SetNeedsCommit();
Before you land this, can you check the expected value of
SchedulerStateMachine::MainThreadInHighLatencyMode after each of the calls on
the scheduler?

[Dominik Grewe, 2013-11-07 15:21:15]

I've added the checks but I had to add a method
Scheduler::MainThreadIsInHighLatencyMode(), otherwise it's a pain to get to the
state machine from the scheduler. Hope that's okay. There are already some
methods which seem to be only used by unit tests, e.g.
Scheduler::RedrawPending().

[commit-bot: I haz the power, 2013-11-07 22:29:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominikg@chromium.org/54913003/420001

[commit-bot: I haz the power, 2013-11-08 00:51:47]

Change committed as 233734