Reuse property trees

Reuse property trees

This patch adds a needs_rebuild boolean to PropertyTrees
and resets it each time the trees are rebuilt. Certain
layer operations do not require a rebuild and can instead
mutate an existing property tree node (e.g., updating a
scroll offset). Special care is taken in Layer to avoid
setting needs_rebuild in these cases.

BUG=470272
Committed: https://crrev.com/5057e1ee8ec082969674af4518082e9845417229
Cr-Commit-Position: refs/heads/master@{#325694}

[Ian Vollick, 2015-04-15 18:50:22]

Patchset #2 (id:20001) has been deleted

[Ian Vollick, 2015-04-15 18:50:28]

Patchset #2 (id:40001) has been deleted

[Ian Vollick, 2015-04-15 18:50:35]

Patchset #2 (id:60001) has been deleted

[Ian Vollick, 2015-04-15 18:50:42]

Patchset #2 (id:80001) has been deleted

[Ian Vollick, 2015-04-15 18:50:51]

Patchset #1 (id:1) has been deleted

[Ian Vollick, 2015-04-15 18:58:17]

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

[ajuma, 2015-04-16 14:52:14]

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:184: layer_tree_host_->property_trees()->needs_rebuild =
true;
LayerTreeHost::SetNeedsFullTreeSync will trigger a rebuild, so this line
shouldn't be needed.

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:369: SetNeedsCommit();
In what situations will changing bounds change whether a clip node is needed?

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:625: SetNeedsCommit();
Can setting position change whether a transform node is needed? Or is the
rebuild needed only because offset_to_transform_parent values might change?

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:651: transform_node->data.needs_local_transform_update =
true;
Could changing a transform's value affect whether this or other layers need a
clip node (by changing the result of the axis alignment check in
RequiresClipNode)? Could it change the offset_to_transform_parent for other
layers (e.g. for fixed position layers and for layer with scroll parents, where
we the offset_to_transform_parent calculation involves computing the transform
from one node to another)?

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:657: SetNeedsCommit();
As a future optimization (not in this CL), we could probably add more checks to
avoid always re-building here (e.g. if the transform is just a 2d translation,
we don't need to rebuild).

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:790: LayerTreeHost* host) {
Will this ever get called with a |host| that's not layer_tree_host_?

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.h#newc...
cc/layers/layer.h:550: void SetNeedsCommitNoRebuild();
Please add a comment here that explains how this is different from plain
SetNeedsCommit.

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

https://codereview.chromium.org/1088773003/diff/140001/cc/trees/property_tree...
cc/trees/property_tree.h:136: return i > -1 && i < static_cast<int>(size()) ?
&nodes_[i] : nullptr;
Do we need this check because we're winding up with stale ids somewhere? Or
could this be a DCHECK?

[ajuma, 2015-04-16 14:55:05]

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:657: SetNeedsCommit();
On 2015/04/16 14:52:14, ajuma wrote:
> As a future optimization (not in this CL), we could probably add more checks
to
> avoid always re-building here (e.g. if the transform is just a 2d translation,
> we don't need to rebuild).

Actually, I take this back; a 2d translation would still mean we need to update
offset_to_transform_parent values.

[enne (OOO), 2015-04-16 21:49:32]

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

[enne (OOO), 2015-04-16 21:49:33]

I like the direction here in general.  It looks like ajuma has picked over where
updates need to happen and where they don't, so I haven't looked as closely at
that.

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:184: layer_tree_host_->property_trees()->needs_rebuild =
true;
On 2015/04/16 at 14:52:14, ajuma wrote:
> LayerTreeHost::SetNeedsFullTreeSync will trigger a rebuild, so this line
shouldn't be needed.

That's kind of subtle to skip it.  I think I'd prefer to have it here.

https://codereview.chromium.org/1088773003/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/1088773003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common.cc:2559: CHECK(draw_opacities_match)
This seems like it should get done in a separate patch?

[Ian Vollick, 2015-04-17 14:56:22]

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:369: SetNeedsCommit();
On 2015/04/16 14:52:14, ajuma wrote:
> In what situations will changing bounds change whether a clip node is needed?

None! I've gotten rid of the early out and we now always call
SetNeedsCommitNoRebuild().

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:625: SetNeedsCommit();
On 2015/04/16 14:52:13, ajuma wrote:
> Can setting position change whether a transform node is needed? Or is the
> rebuild needed only because offset_to_transform_parent values might change?

The latter, yeah.

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:651: transform_node->data.needs_local_transform_update =
true;
On 2015/04/16 14:52:14, ajuma wrote:
> Could changing a transform's value affect whether this or other layers need a
> clip node (by changing the result of the axis alignment check in
> RequiresClipNode)? Could it change the offset_to_transform_parent for other
> layers (e.g. for fixed position layers and for layer with scroll parents,
where
> we the offset_to_transform_parent calculation involves computing the transform
> from one node to another)?

Yeah, it could. I've updated this code to check for changes to axis alignment
(and added a test).

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:657: SetNeedsCommit();
On 2015/04/16 14:55:04, ajuma wrote:
> On 2015/04/16 14:52:14, ajuma wrote:
> > As a future optimization (not in this CL), we could probably add more checks
> to
> > avoid always re-building here (e.g. if the transform is just a 2d
translation,
> > we don't need to rebuild).
> 
> Actually, I take this back; a 2d translation would still mean we need to
update
> offset_to_transform_parent values.

We've talked about the offset_to_transform_parent stuff offline. I think we'll
still trigger rebuilds in cases where it could be affected.

https://codereview.chromium.org/1088773003/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:790: LayerTreeHost* host) {
On 2015/04/16 14:52:14, ajuma wrote:
> Will this ever get called with a |host| that's not layer_tree_host_?

Nope. Removed.

https://codereview.chromium.org/1088773003/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/1088773003/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common.cc:2559: CHECK(draw_opacities_match)
On 2015/04/16 21:49:33, enne wrote:
> This seems like it should get done in a separate patch?

Done.

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

https://codereview.chromium.org/1088773003/diff/140001/cc/trees/property_tree...
cc/trees/property_tree.h:136: return i > -1 && i < static_cast<int>(size()) ?
&nodes_[i] : nullptr;
On 2015/04/16 14:52:14, ajuma wrote:
> Do we need this check because we're winding up with stale ids somewhere? Or
> could this be a DCHECK?

This was due to a bad bug! What was happening was that in the unit tests, we'd
get fresh (i.e. empty) property trees each time we ran CDP because each
CalcDrawPropertiesInputsForTesting would have a new batch. I've updated the unit
tests to reuse the LTH's property trees where applicable (and removed this
unnecessary stuff).

[ajuma, 2015-04-17 15:15:52]

Thanks, lgtm!

[Ian Vollick, 2015-04-17 18:13:47]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2015-04-17 18:14:02]

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

[commit-bot: I haz the power, 2015-04-17 19:12:41]

Committed patchset #5 (id:180001)

[commit-bot: I haz the power, 2015-04-17 19:13:42]

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

[enne (OOO), 2015-04-17 20:04:22]

https://codereview.chromium.org/1088773003/diff/180001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/1088773003/diff/180001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common.cc:2559: CHECK(draw_opacities_match)
On 2015/04/17 14:56:22, vollick wrote:
> On 2015/04/16 21:49:33, enne wrote:
> > This seems like it should get done in a separate patch?
> 
> Done.

<_<