cc: Don't attempt main thread synchronization if it is slow.

cc: Don't attempt main thread synchronization if it is slow.

This patch:
1) Adds a critical_begin_main_frame_to_activate_is_fast flag.
2) Separates impl_latency_takes_priority_ into tree_priority_
   and a scroll_handler_state_.
3) Replaces impl_latency_takes_priority with
   ImplLatencyTakesPriority(), which is now a function of
   the above and is now true when the main thread is too slow.

There is also a small change to the latency recovery logic that
estimates the main thread response time depending on whether
the the BeginMainFrame is critical or not.

BUG=469953
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/42ef946ac829db450d131ad06a399a85f9fe207f
Cr-Commit-Position: refs/heads/master@{#360500}

[Sami, 2015-10-29 10:53:55]

skyostil@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2015-10-29 10:53:56]

I really like this idea, but have one thought about cost estimation.

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

https://codereview.chromium.org/1425973003/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:473:
state_machine_.SetMainThreadIsFast(bmf_to_activate_estimate < args.interval);
This part I'm a little unsure about. The delay of BeginMainFrames depends on
whether the scheduler is prioritizing them or not, so we should really only
measure the delay any time we're actually trying to prioritize it.

I wonder if we just did this when BeginFrame.on_critical_path is true? That'd
still be a somewhat circular dependency because we likely wouldn't set the flag
unless we think the main thread is fast.

Another option would be to only measure the time from when BeginMainFrame starts
to execute up to the activation, i.e., ignore the queueing delay. The scheduler
can often make the queueing delay disappear if critical_path is set.

[brianderson, 2015-10-29 17:56:52]

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

https://codereview.chromium.org/1425973003/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:473:
state_machine_.SetMainThreadIsFast(bmf_to_activate_estimate < args.interval);
On 2015/10/29 10:53:56, Sami wrote:
> This part I'm a little unsure about. The delay of BeginMainFrames depends on
> whether the scheduler is prioritizing them or not, so we should really only
> measure the delay any time we're actually trying to prioritize it.
> 
> I wonder if we just did this when BeginFrame.on_critical_path is true? That'd
> still be a somewhat circular dependency because we likely wouldn't set the
flag
> unless we think the main thread is fast.
> 
> Another option would be to only measure the time from when BeginMainFrame
starts
> to execute up to the activation, i.e., ignore the queueing delay. The
scheduler
> can often make the queueing delay disappear if critical_path is set.

What if we:
1) Make adjusted_args.on_critical_path = (scroll_affects_scroll_handler_ &&
smoothness_takes_priority), which should break the circular dependency.
2) Split CompositorTimingHistory::BeginMainFrameToCommit into 3: a) queueing
duration w/ on_critical_path set, b) queueing duration w/o on_critical_path set
c) BeginMainFrame starts to commit.

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

https://codereview.chromium.org/1425973003/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.cc:996: if (scroll_affects_scroll_handler_
&& main_thread_is_fast_)
Somewhat related to this patch, I wonder if we should have a
SynchronizationTakesPriority mode that sets the deadline to be the next
interval, where SynchronizationTakesPriority is equal to this condition and
would tell the TileManager to make the tree priorities more equal.

[Sami, 2015-10-29 18:37:04]

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

https://codereview.chromium.org/1425973003/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:473:
state_machine_.SetMainThreadIsFast(bmf_to_activate_estimate < args.interval);
On 2015/10/29 17:56:51, brianderson wrote:
> On 2015/10/29 10:53:56, Sami wrote:
> > This part I'm a little unsure about. The delay of BeginMainFrames depends on
> > whether the scheduler is prioritizing them or not, so we should really only
> > measure the delay any time we're actually trying to prioritize it.
> > 
> > I wonder if we just did this when BeginFrame.on_critical_path is true?
That'd
> > still be a somewhat circular dependency because we likely wouldn't set the
> flag
> > unless we think the main thread is fast.
> > 
> > Another option would be to only measure the time from when BeginMainFrame
> starts
> > to execute up to the activation, i.e., ignore the queueing delay. The
> scheduler
> > can often make the queueing delay disappear if critical_path is set.
> 
> What if we:
> 1) Make adjusted_args.on_critical_path = (scroll_affects_scroll_handler_ &&
> smoothness_takes_priority), which should break the circular dependency.
> 2) Split CompositorTimingHistory::BeginMainFrameToCommit into 3: a) queueing
> duration w/ on_critical_path set, b) queueing duration w/o on_critical_path
set
> c) BeginMainFrame starts to commit.

I like both ideas!

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

https://codereview.chromium.org/1425973003/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.cc:996: if (scroll_affects_scroll_handler_
&& main_thread_is_fast_)
On 2015/10/29 17:56:51, brianderson wrote:
> Somewhat related to this patch, I wonder if we should have a
> SynchronizationTakesPriority mode that sets the deadline to be the next
> interval, where SynchronizationTakesPriority is equal to this condition and
> would tell the TileManager to make the tree priorities more equal.

I think that describes the underlying thing pretty well. We'd probably want that
to kick in for all the existing things plus rAF and main thread animations.

[brianderson, 2015-10-31 01:11:05]

WIP patch to break down BeginMainFrame here:
https://codereview.chromium.org/1432463002

[brianderson, 2015-11-06 02:02:37]

brianderson@chromium.org changed reviewers:
+ mithro@mithis.com, sunnyps@chromium.org

[brianderson, 2015-11-06 02:02:37]

All existing tests pass now and I removed the circular dependency, but I still
need to add new tests. Ptal.

Depends on https://codereview.chromium.org/1415763008/ and
https://codereview.chromium.org/1432463002/

[Sami, 2015-11-06 11:54:43]

https://codereview.chromium.org/1425973003/diff/40001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/1425973003/diff/40001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:482: bmf_to_activate_estimate_if_critical <
args.interval);
Should we be comparing against the adjusted deadline instead?

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

https://codereview.chromium.org/1425973003/diff/40001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:102: void SetSmoothnessMode(bool
smoothness_takes_priority,
nit: some of the (test) call sites might read a bit better if this was split
into two setters. The naming is also a little confusing now since
|smoothness_takes_priority| only sometimes makes impl take priority :)

How about something like this:

SetImplLatencyIsImportant(...)
SetBestEffortMainSynchronizationEnabled(scroll_affects_scroll_handler)  // Later
we'd likely add rAF and CSS animation bits here.

Not totally happy with the naming but WDYT of the general idea?

[brianderson, 2015-11-12 20:22:04]

https://codereview.chromium.org/1425973003/diff/40001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/1425973003/diff/40001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:482: bmf_to_activate_estimate_if_critical <
args.interval);
On 2015/11/06 11:54:42, Sami (slow) wrote:
> Should we be comparing against the adjusted deadline instead?

Yeah. For the purposes of this patch, I think we should change it to the
adjusted deadline.

Using the interval would be good for a "prefers synchronization" mode in the
future, but not now.

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

https://codereview.chromium.org/1425973003/diff/40001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:102: void SetSmoothnessMode(bool
smoothness_takes_priority,
On 2015/11/06 11:54:42, Sami (slow) wrote:
> nit: some of the (test) call sites might read a bit better if this was split
> into two setters. The naming is also a little confusing now since
> |smoothness_takes_priority| only sometimes makes impl take priority :)
> 
> How about something like this:
> 
> SetImplLatencyIsImportant(...)
> SetBestEffortMainSynchronizationEnabled(scroll_affects_scroll_handler)  //
Later
> we'd likely add rAF and CSS animation bits here.
> 
> Not totally happy with the naming but WDYT of the general idea?

That sounds good. I might call the second one SetScrollAffectsScrollHandler, and
change the name again in the follow up patches.

[brianderson, 2015-11-17 22:57:04]

Ptal! New tests added for:
1) Main thread latency recovery
2) Impl thread latency recovery
3) ImplLatencyTakesPriority
4) on_critical_path

[brianderson, 2015-11-17 23:01:45]

https://codereview.chromium.org/1425973003/diff/80001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/1425973003/diff/80001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:221: void Scheduler::SetSmoothnessMode(TreePriority
tree_priority,
Note: I ended up keeping this a single function so we don't have to worry about
calling ProcessedScheduledActions twice. To make it easier to read, I replaced
the bools with enums.

[brianderson, 2015-11-17 23:10:49]

Description was changed from

==========
cc: Don't attempt main thread synchronization if it is slow.

This adds a main_thread_is_fast_ flag and separates
impl_latency_takes_priority_ into smoothness_takes_priority_
and scroll_affects_scroll_handler_ in SchedulerStateMachine.

impl_latency_takes_priority is replaced by
ImplLatencyTakesPriority(), which is a function the above and
is now true when the main thread is too slow.

BUG=469953
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Don't attempt main thread synchronization if it is slow.

This patch:
1) Adds a critical_begin_main_frame_to_activate_is_fast flag.
2) Separates impl_latency_takes_priority_ into tree_priority_
   and a scroll_handler_state_.
3) Replaces impl_latency_takes_priority with
   ImplLatencyTakesPriority(), which is now a function of
   the above and is now true when the main thread is too slow.

There is also a small change to the latency recovery logic that
estimates the main thread response time depending on whether
the the BeginMainFrame is critical or not.

BUG=469953
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Sami, 2015-11-18 19:46:05]

lgtm, thanks for adding the tests.

SmoothnessMode is a bit vague but I can't think of a better name right now :)

https://codereview.chromium.org/1425973003/diff/80001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/1425973003/diff/80001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:221: void Scheduler::SetSmoothnessMode(TreePriority
tree_priority,
On 2015/11/17 23:01:45, brianderson wrote:
> Note: I ended up keeping this a single function so we don't have to worry
about
> calling ProcessedScheduledActions twice. To make it easier to read, I replaced
> the bools with enums.

Ok, the enums do make the call sites easier to read.

https://codereview.chromium.org/1425973003/diff/80001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/1425973003/diff/80001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.h:29: enum ScrollHandlerState {
nit: could this be an enum class?

[brianderson, 2015-11-19 00:59:01]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-11-19 00:59:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1425973003/#ps100001
(title: "rebase; change names; use enum class")

[commit-bot: I haz the power, 2015-11-19 01:06:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425973003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425973003/100001

[commit-bot: I haz the power, 2015-11-19 02:36:47]

Committed patchset #6 (id:100001)

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

Patchset 6 (id:??) landed as
https://crrev.com/42ef946ac829db450d131ad06a399a85f9fe207f
Cr-Commit-Position: refs/heads/master@{#360500}