cc: Do not use current frame number in state machine decisions.

cc: Do not use current frame number in state machine decisions.

This allows the scheduler to evolve to a state where it's able to
perform multiple actions of the same type in a given frame. One example
of this is webview where we can stop receiving BeginFrames but still
have to draw any number of times the system asks us to.

BUG=439275
Committed: https://crrev.com/55ee356251c5393fec8bb3b81dfe800896abfb42
Cr-Commit-Position: refs/heads/master@{#321625}

[sunnyps, 2015-03-19 22:19:26]

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

[sunnyps, 2015-03-19 22:19:33]

PTAL

This was split out of my webview CL is based on top of crrev.com/1020993002

[brianderson, 2015-03-19 23:31:43]

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

https://codereview.chromium.org/1002493006/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.cc:565: animate_funnel_ = false;
Neat way of getting rid of did_commit_after_animating_.

https://codereview.chromium.org/1002493006/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.cc:645: request_swap_funnel_ = true;
For all the places where we set the funnels to true, should we DCHECK that they
were previously false?

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

https://codereview.chromium.org/1002493006/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.h:308: bool animate_funnel_;
animate_funnel_ can be reset in the middle of a frame, so I see why you don't
call these animated_this_frame or something like that.

It's a little weird to call these funnels when they are bools, but I guess it
groups them conceptually with the prepare_tiles_funnel_, which is nice.

Not sure where I'm going with this. I can't come up with a better name than
funnel.

https://codereview.chromium.org/1002493006/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.h:316: int prepare_tiles_funnel_;
nit: Space after this line to separate all the funnels from the other variables?

[sunnyps, 2015-03-20 01:13:44]

PTAL

Added DCHECKs and addressed nits. Also added a "funnel: " prefix to tracing so
that funnels show up together.

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

https://codereview.chromium.org/1002493006/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.cc:565: animate_funnel_ = false;
On 2015/03/19 23:31:43, brianderson wrote:
> Neat way of getting rid of did_commit_after_animating_.

Acknowledged.

https://codereview.chromium.org/1002493006/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.cc:645: request_swap_funnel_ = true;
On 2015/03/19 23:31:43, brianderson wrote:
> For all the places where we set the funnels to true, should we DCHECK that
they
> were previously false?

Done.

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

https://codereview.chromium.org/1002493006/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.h:308: bool animate_funnel_;
On 2015/03/19 23:31:43, brianderson wrote:
> animate_funnel_ can be reset in the middle of a frame, so I see why you don't
> call these animated_this_frame or something like that.
> 
> It's a little weird to call these funnels when they are bools, but I guess it
> groups them conceptually with the prepare_tiles_funnel_, which is nice.
> 
> Not sure where I'm going with this. I can't come up with a better name than
> funnel.

We can change the names later - I wasn't able to come up anything better.

https://codereview.chromium.org/1002493006/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.h:316: int prepare_tiles_funnel_;
On 2015/03/19 23:31:43, brianderson wrote:
> nit: Space after this line to separate all the funnels from the other
variables?

Done.

[mithro-old, 2015-03-20 01:34:20]

Hi Sunny,

I think the funnel idea is pretty reasonable with the current structure.
However, I worry about the fact that with bools the "state reset" in
AdvanceFrame makes debugging harder, there is no way to see that Swap hasn't
happened for like 10 frames now. I would like to try and keep tracing
information about the last frame / time an action was performed -- maybe these
are just informational now?

Tim 'mithro' Ansell

[sunnyps, 2015-03-20 01:39:58]

On 2015/03/20 01:34:20, mithro wrote:
> Hi Sunny,
> 
> I think the funnel idea is pretty reasonable with the current structure.
> However, I worry about the fact that with bools the "state reset" in
> AdvanceFrame makes debugging harder, there is no way to see that Swap hasn't
> happened for like 10 frames now. I would like to try and keep tracing
> information about the last frame / time an action was performed -- maybe these
> are just informational now?
> 
> Tim 'mithro' Ansell

The big reason to do this is to allow multiple actions of the same type per
frame. Checking frame numbers can't support that. BTW the frame numbers haven't
been removed in this CL - they're kept precisely for the purpose of tracing.

[mithro-old, 2015-03-20 01:46:41]

On 2015/03/20 01:39:58, sunnyps wrote:
> On 2015/03/20 01:34:20, mithro wrote:
> > Hi Sunny,
> > 
> > I think the funnel idea is pretty reasonable with the current structure.
> > However, I worry about the fact that with bools the "state reset" in
> > AdvanceFrame makes debugging harder, there is no way to see that Swap hasn't
> > happened for like 10 frames now. I would like to try and keep tracing
> > information about the last frame / time an action was performed -- maybe
these
> > are just informational now?
> > 
> > Tim 'mithro' Ansell
> 
> The big reason to do this is to allow multiple actions of the same type per
> frame. Checking frame numbers can't support that.

Yes, understood that.

> BTW the frame numbers haven't been removed in this CL - they're kept precisely
for the purpose of tracing.

I think I need more sleep - should have seen that from the CL.

I was wondering if we should use a set of two integers here - a "XXX_requested"
and "XXX_serviced". This would allow us to track how many times we actually end
up doing each task per frame. AdvanceFrame could then log some information. It
could also be good to DCHECK some sanity conditions like ANIMATE only happened
at most twice per frame?

Tim

[sunnyps, 2015-03-20 04:35:58]

On 2015/03/20 01:46:41, mithro wrote:
> On 2015/03/20 01:39:58, sunnyps wrote:
> > On 2015/03/20 01:34:20, mithro wrote:
> > > Hi Sunny,
> > > 
> > > I think the funnel idea is pretty reasonable with the current structure.
> > > However, I worry about the fact that with bools the "state reset" in
> > > AdvanceFrame makes debugging harder, there is no way to see that Swap
hasn't
> > > happened for like 10 frames now. I would like to try and keep tracing
> > > information about the last frame / time an action was performed -- maybe
> these
> > > are just informational now?
> > > 
> > > Tim 'mithro' Ansell
> > 
> > The big reason to do this is to allow multiple actions of the same type per
> > frame. Checking frame numbers can't support that.
> 
> Yes, understood that.
> 
> > BTW the frame numbers haven't been removed in this CL - they're kept
precisely
> for the purpose of tracing.
> 
> I think I need more sleep - should have seen that from the CL.
> 
> I was wondering if we should use a set of two integers here - a
"XXX_requested"
> and "XXX_serviced". This would allow us to track how many times we actually
end
> up doing each task per frame. AdvanceFrame could then log some information. It
> could also be good to DCHECK some sanity conditions like ANIMATE only happened
> at most twice per frame?

Sure, keeping a count of the number of times an action was performed
("XXX_serviced") seems reasonable for DCHECKs, not so much for tracing because
we can already see what actions are performed in traces. Keeping track of the
number of times an action was requested ("XXX_requested") might not be very
useful - e.g. LTHI calls SetNeedsRedraw and SetNeedsAnimate quite liberally (and
that is fine).

> 
> Tim

[brianderson, 2015-03-20 19:49:37]

lgtm

[sunnyps, 2015-03-20 20:14:08]

Thanks, marking for commit.

[sunnyps, 2015-03-20 20:14:17]

The CQ bit was checked by sunnyps@chromium.org

[commit-bot: I haz the power, 2015-03-20 20:15:26]

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

[commit-bot: I haz the power, 2015-03-20 20:48:57]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-20 20:49:42]

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