cc: Throttle tile priority updates to once a frame.

cc: Throttle tile priority updates to once a frame.

The tile priority updates happen when calcDrawProperties is called. Now we
make sure we early out except in the following cases.

On the pending tree, we calculate tile priorities:
- Immediately after a commit, so that we can start raster jobs appropriately.
- On an animation tick if it is not the same impl frame as the commit occured
in.

On the active tree, we calculate tile priorities:
- During draw, if it is not in the same impl frame as the tree activation
occured in. In that frame, we already have priorities from the pending tree.

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

[danakj, 2013-01-25 04:51:29]

After discussing this out, it seems the most important thing is calling this
once a frame only. So this patch does that.

Then, it is more accurate because it only computes on the active tree during
draw, which is at the end of the current frame, so it has all property updates
for the frame.

On the pending tree, it will be accurate except in the first frame, where it
computes priorities immediately on commit to get raster jobs started. Then it
skips it in the first animate so we only compute priorities once a frame in the
pending tree.

[qinmin, 2013-01-25 05:07:53]

https://codereview.chromium.org/12045086/diff/1004/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12045086/diff/1004/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:203: current_source_frame_number >
last_source_frame_number_;
i think it is possible for current_source_frame_number to overflow, so this
statement will not be true when that happens

[danakj, 2013-01-25 05:09:01]

https://codereview.chromium.org/12045086/diff/1004/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12045086/diff/1004/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:203: current_source_frame_number >
last_source_frame_number_;
On 2013/01/25 05:07:53, qinmin wrote:
> i think it is possible for current_source_frame_number to overflow, so this
> statement will not be true when that happens

ya you're right, i already switched it to != in the last patch. good catch tho!

[enne (OOO), 2013-01-25 18:17:52]

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:72: layer_impl->last_impl_frame_number_ = 0;
I don't understand this.  I think you'd want to set this to be
this->last_impl_frame_number_ so that if you activate the pending tree, you
don't update priorities again on it as the active tree.  Also, do you need to
set layer_impl->last_source_frame_number_?

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:206:
layerTreeImpl()->frame_rate_counter()->currentFrameNumber();
Can you expose this on LayerTreeImpl like source_frame_number is?

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:214: // In active tree, this is only called during
draw. We update priorities:
How is ensureRSLL handled?

[danakj, 2013-01-25 18:25:47]

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:72: layer_impl->last_impl_frame_number_ = 0;
On 2013/01/25 18:17:52, enne wrote:
> I don't understand this.  I think you'd want to set this to be
> this->last_impl_frame_number_ so that if you activate the pending tree, you
> don't update priorities again on it as the active tree.  Also, do you need to
> set layer_impl->last_source_frame_number_?

This is resetting the value on the target tree. This way if you get into
updateTilePriorities() it will say "hey my last impl frame number is 0" and
reset it to "current frame" and then say "well i've already updated this frame
so don't update".

My understanding was that this is called on the pending tree's layer, so
layer_impl here is the active layer. Similar to commit. Am I mistaking that?

last_source_frame_number doesn't need to be set because it only increments on
commit. This needs to be reset to indicate a commit/sync happened. the
source_frame_number changing indicates this already.

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:206:
layerTreeImpl()->frame_rate_counter()->currentFrameNumber();
On 2013/01/25 18:17:52, enne wrote:
> Can you expose this on LayerTreeImpl like source_frame_number is?

Yep.

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:214: // In active tree, this is only called during
draw. We update priorities:
On 2013/01/25 18:17:52, enne wrote:
> How is ensureRSLL handled?

ensureRSLL calls UDP(ACTIVE_TREE) which does not do tile prioritization.

UDP(ACTIVE_TREE_FOR_DRAW) does do tile prioritization. So, we don't get here as
claimed on 214.

[enne (OOO), 2013-01-25 18:51:26]

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:72: layer_impl->last_impl_frame_number_ = 0;
On 2013/01/25 18:25:47, danakj wrote:
> On 2013/01/25 18:17:52, enne wrote:
> > I don't understand this.  I think you'd want to set this to be
> > this->last_impl_frame_number_ so that if you activate the pending tree, you
> > don't update priorities again on it as the active tree.  Also, do you need
to
> > set layer_impl->last_source_frame_number_?
> 
> This is resetting the value on the target tree. This way if you get into
> updateTilePriorities() it will say "hey my last impl frame number is 0" and
> reset it to "current frame" and then say "well i've already updated this frame
> so don't update".
> 
> My understanding was that this is called on the pending tree's layer, so
> layer_impl here is the active layer. Similar to commit. Am I mistaking that?
> 
> last_source_frame_number doesn't need to be set because it only increments on
> commit. This needs to be reset to indicate a commit/sync happened. the
> source_frame_number changing indicates this already.

You're right that this is called on the pending tree's layer and layer_impl is
the active layer.

Just to walk through the logic, if you activate a tree of source frame 7, the
values on the active layer are going to be last_impl_frame_number_=0,
last_source_frame_number_=6.  activeTree()->source_frame_number() will be 7. 
This active layer will then update tile priorities on draw because the source
frame numbers are different?

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:214: // In active tree, this is only called during
draw. We update priorities:
On 2013/01/25 18:25:47, danakj wrote:
> On 2013/01/25 18:17:52, enne wrote:
> > How is ensureRSLL handled?
> 
> ensureRSLL calls UDP(ACTIVE_TREE) which does not do tile prioritization.
> 
> UDP(ACTIVE_TREE_FOR_DRAW) does do tile prioritization. So, we don't get here
as
> claimed on 214.

Aha, thanks.

[danakj, 2013-01-25 18:56:29]

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:72: layer_impl->last_impl_frame_number_ = 0;
On 2013/01/25 18:51:26, enne wrote:
> On 2013/01/25 18:25:47, danakj wrote:
> > On 2013/01/25 18:17:52, enne wrote:
> > > I don't understand this.  I think you'd want to set this to be
> > > this->last_impl_frame_number_ so that if you activate the pending tree,
you
> > > don't update priorities again on it as the active tree.  Also, do you need
> to
> > > set layer_impl->last_source_frame_number_?
> > 
> > This is resetting the value on the target tree. This way if you get into
> > updateTilePriorities() it will say "hey my last impl frame number is 0" and
> > reset it to "current frame" and then say "well i've already updated this
frame
> > so don't update".
> > 
> > My understanding was that this is called on the pending tree's layer, so
> > layer_impl here is the active layer. Similar to commit. Am I mistaking that?
> > 
> > last_source_frame_number doesn't need to be set because it only increments
on
> > commit. This needs to be reset to indicate a commit/sync happened. the
> > source_frame_number changing indicates this already.
> 
> You're right that this is called on the pending tree's layer and layer_impl is
> the active layer.
> 
> Just to walk through the logic, if you activate a tree of source frame 7, the
> values on the active layer are going to be last_impl_frame_number_=0,
> last_source_frame_number_=6.  activeTree()->source_frame_number() will be 7. 
> This active layer will then update tile priorities on draw because the source
> frame numbers are different?

The goal is that when you activate the tree, it won't update priorities on the
first draw, because it already has priorities from the pending tree. That was
what we had planned yesterday at least.

I didn't think that the source_frame_number would change but you're right. So
I'll make it only use the source_frame_number on the pending tree. Active tree
will make its decision only on the impl_frame_number.

[enne (OOO), 2013-01-25 19:00:26]

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:72: layer_impl->last_impl_frame_number_ = 0;
On 2013/01/25 18:56:30, danakj wrote:
> On 2013/01/25 18:51:26, enne wrote:
> > On 2013/01/25 18:25:47, danakj wrote:
> > > On 2013/01/25 18:17:52, enne wrote:
> > > > I don't understand this.  I think you'd want to set this to be
> > > > this->last_impl_frame_number_ so that if you activate the pending tree,
> you
> > > > don't update priorities again on it as the active tree.  Also, do you
need
> > to
> > > > set layer_impl->last_source_frame_number_?
> > > 
> > > This is resetting the value on the target tree. This way if you get into
> > > updateTilePriorities() it will say "hey my last impl frame number is 0"
and
> > > reset it to "current frame" and then say "well i've already updated this
> frame
> > > so don't update".
> > > 
> > > My understanding was that this is called on the pending tree's layer, so
> > > layer_impl here is the active layer. Similar to commit. Am I mistaking
that?
> > > 
> > > last_source_frame_number doesn't need to be set because it only increments
> on
> > > commit. This needs to be reset to indicate a commit/sync happened. the
> > > source_frame_number changing indicates this already.
> > 
> > You're right that this is called on the pending tree's layer and layer_impl
is
> > the active layer.
> > 
> > Just to walk through the logic, if you activate a tree of source frame 7,
the
> > values on the active layer are going to be last_impl_frame_number_=0,
> > last_source_frame_number_=6.  activeTree()->source_frame_number() will be 7.

> > This active layer will then update tile priorities on draw because the
source
> > frame numbers are different?
> 
> The goal is that when you activate the tree, it won't update priorities on the
> first draw, because it already has priorities from the pending tree. That was
> what we had planned yesterday at least.
> 
> I didn't think that the source_frame_number would change but you're right. So
> I'll make it only use the source_frame_number on the pending tree. Active tree
> will make its decision only on the impl_frame_number.

I think if you just sync the last source frame number over during activation
pushPropertiesTo, then it would work as desired.

(You might even be able to just sync the impl frame number as well and drop the
special 0 case?)

[epenner, 2013-01-25 20:29:09]

https://codereview.chromium.org/12045086/diff/7001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12045086/diff/7001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:196:
pendingTree()->UpdateDrawProperties(LayerTreeImpl::UPDATE_PENDING_TREE);
Wouldn't it be easier to just call pendingTree()->CalculateTilePriorities()
after UpdateDrawProperties()?

If it's for performance reasons do we have any evidence it saves us time to save
one loop over the layers?

[danakj, 2013-01-25 20:34:11]

On Fri, Jan 25, 2013 at 3:29 PM, <epenner@chromium.org> wrote:

>
> https://codereview.chromium.**org/12045086/diff/7001/cc/**
>
layer_tree_host_impl.cc<https://codereview.chromium.org/12045086/diff/7001/cc/layer_tree_host_impl.cc>
> File cc/layer_tree_host_impl.cc (right):
>
> https://codereview.chromium.**org/12045086/diff/7001/cc/**
>
layer_tree_host_impl.cc#**newcode196<https://codereview.chromium.org/12045086/diff/7001/cc/layer_tree_host_impl.cc#newcode196>
> cc/layer_tree_host_impl.cc:**196:
> pendingTree()->**UpdateDrawProperties(**LayerTreeImpl::UPDATE_PENDING_**
> TREE);
> Wouldn't it be easier to just call
> pendingTree()->**CalculateTilePriorities() after UpdateDrawProperties()?
>
> If it's for performance reasons do we have any evidence it saves us time
> to save one loop over the layers?
>

I'll run both thru perftests.


>
>
https://codereview.chromium.**org/12045086/<https://codereview.chromium.org/1...
>

[danakj, 2013-01-25 20:43:25]

This patch as is:
*RESULT 10_10_layer_tree: frames= 2670.67 runs/s

Moving into a separate tree walk:
*RESULT 10_10_layer_tree: frames= 2643.33 runs/s

I think we should avoid the tree walk.

On Fri, Jan 25, 2013 at 3:33 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Fri, Jan 25, 2013 at 3:29 PM, <epenner@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/12045086/diff/7001/cc/**
>>
layer_tree_host_impl.cc<https://codereview.chromium.org/12045086/diff/7001/cc/layer_tree_host_impl.cc>
>> File cc/layer_tree_host_impl.cc (right):
>>
>> https://codereview.chromium.**org/12045086/diff/7001/cc/**
>>
layer_tree_host_impl.cc#**newcode196<https://codereview.chromium.org/12045086/diff/7001/cc/layer_tree_host_impl.cc#newcode196>
>> cc/layer_tree_host_impl.cc:**196:
>> pendingTree()->**UpdateDrawProperties(**LayerTreeImpl::UPDATE_PENDING_**
>> TREE);
>> Wouldn't it be easier to just call
>> pendingTree()->**CalculateTilePriorities() after UpdateDrawProperties()?
>>
>> If it's for performance reasons do we have any evidence it saves us time
>> to save one loop over the layers?
>>
>
> I'll run both thru perftests.
>
>
>>
>>
https://codereview.chromium.**org/12045086/<https://codereview.chromium.org/1...
>>
>
>

[danakj, 2013-01-25 20:45:25]

PTAL. Reviewing the logic.

On commit, the source frame number changes. So no early out, pending tree will
prioritize.

After commit, it will be in the same impl frame and avoid prioritizing pending
tree until 2nd animate.

On tree activate, we push over the last source and impl frames. This avoids
prioritize on draw if the pending tree prioritized within the same impl frame.

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12045086/diff/7001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:72: layer_impl->last_impl_frame_number_ = 0;
On 2013/01/25 19:00:26, enne wrote:
> On 2013/01/25 18:56:30, danakj wrote:
> > On 2013/01/25 18:51:26, enne wrote:
> > > On 2013/01/25 18:25:47, danakj wrote:
> > > > On 2013/01/25 18:17:52, enne wrote:
> > > > > I don't understand this.  I think you'd want to set this to be
> > > > > this->last_impl_frame_number_ so that if you activate the pending
tree,
> > you
> > > > > don't update priorities again on it as the active tree.  Also, do you
> need
> > > to
> > > > > set layer_impl->last_source_frame_number_?
> > > > 
> > > > This is resetting the value on the target tree. This way if you get into
> > > > updateTilePriorities() it will say "hey my last impl frame number is 0"
> and
> > > > reset it to "current frame" and then say "well i've already updated this
> > frame
> > > > so don't update".
> > > > 
> > > > My understanding was that this is called on the pending tree's layer, so
> > > > layer_impl here is the active layer. Similar to commit. Am I mistaking
> that?
> > > > 
> > > > last_source_frame_number doesn't need to be set because it only
increments
> > on
> > > > commit. This needs to be reset to indicate a commit/sync happened. the
> > > > source_frame_number changing indicates this already.
> > > 
> > > You're right that this is called on the pending tree's layer and
layer_impl
> is
> > > the active layer.
> > > 
> > > Just to walk through the logic, if you activate a tree of source frame 7,
> the
> > > values on the active layer are going to be last_impl_frame_number_=0,
> > > last_source_frame_number_=6.  activeTree()->source_frame_number() will be
7.
> 
> > > This active layer will then update tile priorities on draw because the
> source
> > > frame numbers are different?
> > 
> > The goal is that when you activate the tree, it won't update priorities on
the
> > first draw, because it already has priorities from the pending tree. That
was
> > what we had planned yesterday at least.
> > 
> > I didn't think that the source_frame_number would change but you're right.
So
> > I'll make it only use the source_frame_number on the pending tree. Active
tree
> > will make its decision only on the impl_frame_number.
> 
> I think if you just sync the last source frame number over during activation
> pushPropertiesTo, then it would work as desired.
> 
> (You might even be able to just sync the impl frame number as well and drop
the
> special 0 case?)

Yes, I agree. Good observation. Special case is gone.

[enne (OOO), 2013-01-25 20:59:10]

https://codereview.chromium.org/12045086/diff/14001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/12045086/diff/14001/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:133:
LayerTreeHostCommon::callFunctionForSubtree<UpdateTilePrioritiesForLayer>(
I think this might be unneeded.

You should only need to update tile priorities after commit (to initialize) or
when draw properties have changed.  (This patch throttles back how many times we
do it from the second condition.) However, if there's no need to update draw
properties, then there should be equally no need to update tile priorities.

[danakj, 2013-01-25 21:18:15]

https://codereview.chromium.org/12045086/diff/14001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/12045086/diff/14001/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:133:
LayerTreeHostCommon::callFunctionForSubtree<UpdateTilePrioritiesForLayer>(
On 2013/01/25 20:59:10, enne wrote:
> I think this might be unneeded.
> 
> You should only need to update tile priorities after commit (to initialize) or
> when draw properties have changed.  (This patch throttles back how many times
we
> do it from the second condition.) However, if there's no need to update draw
> properties, then there should be equally no need to update tile priorities.

The idea is that if you update draw props on scroll (ensureRSLL) but don't
update tile prio at the time, you do it when you draw.

[enne (OOO), 2013-01-25 22:27:58]

\okayguy{lgtm}

I wish there were a less complicated way to do this, but I can't think of
anything better.

[commit-bot: I haz the power, 2013-01-26 18:53:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12045086/22001

[commit-bot: I haz the power, 2013-01-27 05:41:20]

Change committed as 179079