cc: Do not ignore layers without valid priorities during eviction.

cc: Do not ignore layers without valid priorities during eviction.

This is achieved by moving eviction priority validity checking from
layer tree host impl to tiling eviction tile iterators.

Committed: https://crrev.com/85c0bffeb20b8b0d97bcfc093848b3215e3be4ee
Cr-Commit-Position: refs/heads/master@{#303880}

[USE eero AT chromium.org, 2014-10-30 17:51:34]

e.hakkinen@samsung.com changed reviewers:
+ vmpstr@chromium.org

[USE eero AT chromium.org, 2014-10-30 17:52:17]

PTAL.
This is the first part of https://codereview.chromium.org/674103004/

[vmpstr, 2014-10-31 15:35:49]

Can you add a test that demonstrates the problem? Something like a layer which
layer becomes 0 opacity (thus not in rsll). 

In general, I want to avoid passing twin knowledge to layer/tiling iterators.
They should independently return tiles that they think should be evicted. I
think the only thing we need is to ensure that tiles returned from the iterators
without valid tile priorities have low priorities (EVENTUALLY).

https://codereview.chromium.org/690063002/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/690063002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1588: const PictureLayerImpl* twin_layer,
layer itself has access to the twin, let's just get it there so that we don't
have to pass extra parameters.

https://codereview.chromium.org/690063002/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/690063002/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:749: if (use_tile_priority_rects) {
How about we add a client_->GetMaxBinForTilePriority() (or GetMaxTilePriority())
or something along those lines, which would return TilePriority::EVENTUALLY for
layers without valid tile priorities, and use that when calculating tile
priorities. I think that's a bit more explicit.

https://codereview.chromium.org/690063002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/690063002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1277: if (need_valid_tile_priorities &&
!layer->HasValidTilePriorities())
I think this is accepting too many layers. layer->HasValidTilePriorities would
return false in two situations: layer is recycled, or layer is not in render
surface layer list (rsll). If the layer is recycled we still want to skip it,
since it can't have unshared tiles (or at least if it does then that's a
separate bug). We only want to capture layers that are on either active or
pending tree but are not in rsll.

[USE eero AT chromium.org, 2014-11-04 17:47:57]

https://codereview.chromium.org/690063002/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/690063002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1588: const PictureLayerImpl* twin_layer,
On 2014/10/31 15:35:49, vmpstr wrote:
> layer itself has access to the twin, let's just get it there so that we don't
> have to pass extra parameters.

Done.

https://codereview.chromium.org/690063002/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/690063002/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:749: if (use_tile_priority_rects) {
On 2014/10/31 15:35:49, vmpstr wrote:
> How about we add a client_->GetMaxBinForTilePriority() (or
GetMaxTilePriority())
> or something along those lines, which would return TilePriority::EVENTUALLY
for
> layers without valid tile priorities, and use that when calculating tile
> priorities. I think that's a bit more explicit.

Done.

https://codereview.chromium.org/690063002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/690063002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1277: if (need_valid_tile_priorities &&
!layer->HasValidTilePriorities())
On 2014/10/31 15:35:49, vmpstr wrote:
> I think this is accepting too many layers. layer->HasValidTilePriorities would
> return false in two situations: layer is recycled, or layer is not in render
> surface layer list (rsll).

Yes, you are right. Fixed.

[USE eero AT chromium.org, 2014-11-04 17:49:10]

On 2014/10/31 15:35:49, vmpstr wrote:
> Can you add a test that demonstrates the problem? Something like a layer which
> layer becomes 0 opacity (thus not in rsll). 

Will do.

[vmpstr, 2014-11-04 18:15:16]

This is looking very good. Let's just make that test and I think this is good to
go.

https://codereview.chromium.org/690063002/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/690063002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1292: DCHECK(!twin_layer ||
twin_layer->GetTree() == PENDING_TREE);
while here, can you change this to DCHECK_IMPLIES(twin_layer,
twin_layer->GetTree() == PENDING_TREE)?

https://codereview.chromium.org/690063002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1313:
GetPictureLayerImplPairs(&picture_layer_pairs_, true);
false here?

[USE eero AT chromium.org, 2014-11-06 15:23:07]

https://codereview.chromium.org/690063002/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/690063002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1292: DCHECK(!twin_layer ||
twin_layer->GetTree() == PENDING_TREE);
On 2014/11/04 18:15:16, vmpstr wrote:
> while here, can you change this to DCHECK_IMPLIES(twin_layer,
> twin_layer->GetTree() == PENDING_TREE)?

Done.

https://codereview.chromium.org/690063002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1313:
GetPictureLayerImplPairs(&picture_layer_pairs_, true);
Yes.

[USE eero AT chromium.org, 2014-11-06 15:26:13]

On 2014/11/04 18:15:16, vmpstr wrote:
> This is looking very good. Let's just make that test and I think this is good
to
> go.

PTAL if the unit test checks what you want.

[vmpstr, 2014-11-08 00:18:43]

Thanks for the test, that's exactly what I was looking for.

https://codereview.chromium.org/690063002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/690063002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1601: // that layers that don't have valid tile
priorities have lowest priorities so
This TODO can go away.

https://codereview.chromium.org/690063002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1603: DCHECK(layer_->tilings_);
I'm a bit worried that if we're adding layers without valid tile priorities now,
this DCHECK might not be true anymore (for layers that never got a chance to
create tilings/tiles). Perhaps to make this more robust, we should just
explicitly check if layer has tilings and if it doesn't, early out of the ctor.

[USE eero AT chromium.org, 2014-11-10 09:53:08]

PTAL

https://codereview.chromium.org/690063002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/690063002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1601: // that layers that don't have valid tile
priorities have lowest priorities so
On 2014/11/08 00:18:42, vmpstr wrote:
> This TODO can go away. 

Done.

https://codereview.chromium.org/690063002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1603: DCHECK(layer_->tilings_);
On 2014/11/08 00:18:42, vmpstr wrote:
> I'm a bit worried that if we're adding layers without valid tile priorities
now,
> this DCHECK might not be true anymore (for layers that never got a chance to
> create tilings/tiles). Perhaps to make this more robust, we should just
> explicitly check if layer has tilings and if it doesn't, early out of the
ctor.

Done. I have not hit that case, but that does not mean that it is impossible.

[vmpstr, 2014-11-10 17:33:59]

lgtm, thanks

[USE eero AT chromium.org, 2014-11-11 09:14:11]

The CQ bit was checked by e.hakkinen@samsung.com

[commit-bot: I haz the power, 2014-11-11 09:14:36]

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

[commit-bot: I haz the power, 2014-11-11 10:11:05]

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

[commit-bot: I haz the power, 2014-11-11 10:11:06]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[USE eero AT chromium.org, 2014-11-11 16:26:01]

The CQ bit was checked by e.hakkinen@samsung.com

[commit-bot: I haz the power, 2014-11-11 16:27:16]

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

[USE eero AT chromium.org, 2014-11-11 17:11:33]

The CQ bit was unchecked by e.hakkinen@samsung.com

[USE eero AT chromium.org, 2014-11-11 17:12:50]

https://codereview.chromium.org/690063002/diff/60001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/690063002/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1599: current_tiling_(CurrentTilingRange().start
- 1u) {
CurrentTilingRange must not be called if the layer does not have a tiling set.

[USE eero AT chromium.org, 2014-11-12 11:05:30]

Do I need a new approval?

With the previous patch set, some browser tests crashed, and I am able to
reproduce that locally.
With the latest patch set, all browser tests which succeed without patch succeed
also with patch (at least locally).
There are still some browser tests which time out or fail with and without patch
when I run when locally.

[vmpstr, 2014-11-12 17:46:30]

On 2014/11/12 11:05:30, e_hakkinen wrote:
> Do I need a new approval?
> 
> With the previous patch set, some browser tests crashed, and I am able to
> reproduce that locally.
> With the latest patch set, all browser tests which succeed without patch
succeed
> also with patch (at least locally).
> There are still some browser tests which time out or fail with and without
patch
> when I run when locally.

Oh sorry, I did look at this and it seemed fine. Still lgtm.

[vmpstr, 2014-11-12 17:46:35]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-11-12 17:47:39]

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

[commit-bot: I haz the power, 2014-11-12 17:54:15]

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

[commit-bot: I haz the power, 2014-11-12 17:54:16]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vmpstr, 2014-11-12 18:09:05]

On 2014/11/12 17:54:16, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_gn_dbg on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

I think you need to rebase. After the rebase, feel free to put it in cq.

[USE eero AT chromium.org, 2014-11-12 18:45:04]

The CQ bit was checked by e.hakkinen@samsung.com

[commit-bot: I haz the power, 2014-11-12 18:45:18]

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

[commit-bot: I haz the power, 2014-11-12 20:02:40]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-12 20:03:22]

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