Reset property tree indices when layer is removed from layer tree

Reset property tree indices when layer is removed from layer tree

An orphaned layer has no relation to property trees, so it is an error
for it to have property tree indices.

BUG=489725
Committed: https://crrev.com/692444f78ce635ca7a5dc4e5e9cb925c64500fad
Cr-Commit-Position: refs/heads/master@{#330739}

[Ian Vollick, 2015-05-19 19:59:19]

vollick@chromium.org changed reviewers:
+ ajuma@chromium.org

[ajuma, 2015-05-19 20:06:23]

lgtm

https://codereview.chromium.org/1139573004/diff/1/cc/trees/layer_tree_host_co...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1139573004/diff/1/cc/trees/layer_tree_host_co...
cc/trees/layer_tree_host_common_unittest.cc:9617:
ExecuteCalculateDrawProperties(root.get());
ExecuteCalculateDrawPropertiesWithPropertyTrees (same thing below).

https://codereview.chromium.org/1139573004/diff/1/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/1139573004/diff/1/cc/trees/property_tree.h#ne...
cc/trees/property_tree.h:145: CHECK(i < static_cast<int>(nodes_.size()));
I think it's a good idea to land these CHECKs to help track down places where
we're accessing non-existent nodes, but we should probably revert them before
the next branch point (unless their perf impact is negligible). I'd suggest
adding a TODO.

[Ian Vollick, 2015-05-19 21:11:48]

https://codereview.chromium.org/1139573004/diff/1/cc/trees/layer_tree_host_co...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1139573004/diff/1/cc/trees/layer_tree_host_co...
cc/trees/layer_tree_host_common_unittest.cc:9617:
ExecuteCalculateDrawProperties(root.get());
On 2015/05/19 20:06:23, ajuma wrote:
> ExecuteCalculateDrawPropertiesWithPropertyTrees (same thing below).

Done.

https://codereview.chromium.org/1139573004/diff/1/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/1139573004/diff/1/cc/trees/property_tree.h#ne...
cc/trees/property_tree.h:145: CHECK(i < static_cast<int>(nodes_.size()));
On 2015/05/19 20:06:23, ajuma wrote:
> I think it's a good idea to land these CHECKs to help track down places where
> we're accessing non-existent nodes, but we should probably revert them before
> the next branch point (unless their perf impact is negligible). I'd suggest
> adding a TODO.

Done.

[Ian Vollick, 2015-05-19 21:13:47]

The CQ bit was checked by vollick@chromium.org

[Ian Vollick, 2015-05-19 21:13:48]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1139573004/#ps20001
(title: "rebase/address review")

[commit-bot: I haz the power, 2015-05-19 21:14:32]

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

[commit-bot: I haz the power, 2015-05-19 23:13:51]

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

[commit-bot: I haz the power, 2015-05-19 23:13:52]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Ian Vollick, 2015-05-20 00:29:43]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2015-05-20 00:31:03]

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

[Ian Vollick, 2015-05-20 01:56:53]

On 2015/05/20 at 00:31:03, commit-bot wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1139573004/20001

The linux_android_rel_gn failure seems real, sadly.

[Ian Vollick, 2015-05-20 01:56:59]

The CQ bit was unchecked by vollick@chromium.org

[Ian Vollick, 2015-05-20 13:33:28]

On 2015/05/20 at 01:56:53, vollick wrote:
> On 2015/05/20 at 00:31:03, commit-bot wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1139573004/20001
> 
> The linux_android_rel_gn failure seems real, sadly.

The problem was that you can't just reset the indices for the root layer of a
subtree when it's detached, you have to invalidate the whole subtree. Latest
patch set does this.

Ali, PTAL.

[ajuma, 2015-05-20 13:54:03]

https://codereview.chromium.org/1139573004/diff/60001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1139573004/diff/60001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:258: InvalidatePropertyTreeIndices();
I'm a bit worried about the perf impact of this. Is the idea that we expect the
tree to be shallow, so O(height of the tree) isn't too much work? (This will
also make tree destruction O(n^2) in the worst case, since we disconnect layers
from their parents starting with the root's children and then proceeding
downwards, but maybe that's not too serious since it only happens once in the
life of the tree.)

An alternative would be to store a property tree version number, and then we'd
have a quick way of checking if our tree indices are stale.

WDYT?

[Ian Vollick, 2015-05-20 13:58:01]

On 2015/05/20 at 13:54:03, ajuma wrote:
> https://codereview.chromium.org/1139573004/diff/60001/cc/layers/layer.cc
> File cc/layers/layer.cc (right):
> 
>
https://codereview.chromium.org/1139573004/diff/60001/cc/layers/layer.cc#newc...
> cc/layers/layer.cc:258: InvalidatePropertyTreeIndices();
> I'm a bit worried about the perf impact of this. Is the idea that we expect
the tree to be shallow, so O(height of the tree) isn't too much work? (This will
also make tree destruction O(n^2) in the worst case, since we disconnect layers
from their parents starting with the root's children and then proceeding
downwards, but maybe that's not too serious since it only happens once in the
life of the tree.)
> 
> An alternative would be to store a property tree version number, and then we'd
have a quick way of checking if our tree indices are stale.
> 
> WDYT?

Yeah, you're right -- I think sequence numbers would be better here, too. I'll
put together a patch that does this.

[Ian Vollick, 2015-05-20 15:18:20]

On 2015/05/20 at 13:58:01, vollick wrote:
> On 2015/05/20 at 13:54:03, ajuma wrote:
> > https://codereview.chromium.org/1139573004/diff/60001/cc/layers/layer.cc
> > File cc/layers/layer.cc (right):
> > 
> >
https://codereview.chromium.org/1139573004/diff/60001/cc/layers/layer.cc#newc...
> > cc/layers/layer.cc:258: InvalidatePropertyTreeIndices();
> > I'm a bit worried about the perf impact of this. Is the idea that we expect
the tree to be shallow, so O(height of the tree) isn't too much work? (This will
also make tree destruction O(n^2) in the worst case, since we disconnect layers
from their parents starting with the root's children and then proceeding
downwards, but maybe that's not too serious since it only happens once in the
life of the tree.)
> > 
> > An alternative would be to store a property tree version number, and then
we'd have a quick way of checking if our tree indices are stale.
> > 
> > WDYT?
> 
> Yeah, you're right -- I think sequence numbers would be better here, too. I'll
put together a patch that does this.

Done.

[ajuma, 2015-05-20 15:28:59]

On 2015/05/20 15:18:20, vollick wrote:
> On 2015/05/20 at 13:58:01, vollick wrote:
> > Yeah, you're right -- I think sequence numbers would be better here, too.
I'll
> put together a patch that does this.
> 
> Done.

Thanks! lgtm.

[Ian Vollick, 2015-05-20 15:34:51]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2015-05-20 15:35:02]

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

[commit-bot: I haz the power, 2015-05-20 15:39:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-20 15:40:15]

Patchset 5 (id:??) landed as
https://crrev.com/692444f78ce635ca7a5dc4e5e9cb925c64500fad
Cr-Commit-Position: refs/heads/master@{#330739}