cc : Compute subtree has copy requests before property tree building

cc : Compute subtree has copy requests before property tree building

To determine if we can skip creating effect nodes for hidden subtrees
during property tree building, we need to know if the subtree has any
copy requests. So, moving this computation to happen before property
tree building. After this CL, we can stop creating effect nodes for 
hidden subtrees and also stop pushing these layers to compositor thread 
at commit. 

BUG=595843
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2822303003
Cr-Commit-Position: refs/heads/master@{#466745}
Committed: https://chromium.googlesource.com/chromium/src/+/ebc9428960482240e8a1114b7b795cabf42a6b82

[jaydasika, 2017-04-19 00:17:10]

Description was changed from

==========
cc : Compute subtree has copy requests before property tree building

To determine if we can skip creating effect nodes for hidden subtrees
during property tree building, we need to know if the subtree has any
copy requests. So, moving this computation to happen before property
tree building.

BUG=595843
==========

to

==========
cc : Compute subtree has copy requests before property tree building

To determine if we can skip creating effect nodes for hidden subtrees
during property tree building, we need to know if the subtree has any
copy requests. So, moving this computation to happen before property
tree building.

BUG=595843
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[jaydasika, 2017-04-19 02:20:33]

jaydasika@chromium.org changed reviewers:
+ weiliangc@chromium.org

[jaydasika, 2017-04-19 02:20:34]

PTAL

https://codereview.chromium.org/2822303003/diff/20001/cc/trees/effect_node.h
File cc/trees/effect_node.h (left):

https://codereview.chromium.org/2822303003/diff/20001/cc/trees/effect_node.h#...
cc/trees/effect_node.h:61: int num_copy_requests_in_subtree;
We need not track the number. The only thing we are interested is whether there
are copy requests.

[jaydasika, 2017-04-19 02:20:43]

The CQ bit was checked by jaydasika@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 02:21:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 03:29:16]

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

[commit-bot: I haz the power, 2017-04-19 03:29:16]

Dry run: This issue passed the CQ dry run.

[weiliangc, 2017-04-19 17:58:48]

https://codereview.chromium.org/2822303003/diff/20001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2822303003/diff/20001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:461: if (effect_node->has_render_surface &&
effect_node->subtree_has_copy_request)
If subtree_has_copy_request is only useful here, why not just update the value
right before calling LayerShouldBeSkipped in FindLayersThatNeedUpdates? That way
we don't need to care about keep track of it during property tree builder?

https://codereview.chromium.org/2822303003/diff/20001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2822303003/diff/20001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:1356:
root_layer->layer_tree_host()->SetHasCopyRequest(false);
Since we always collect the copy requests with property tree builder, this is
equivalent to effect tree's copy_requests_ not empty right?

[jaydasika, 2017-04-19 18:15:15]

https://codereview.chromium.org/2822303003/diff/20001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2822303003/diff/20001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:461: if (effect_node->has_render_surface &&
effect_node->subtree_has_copy_request)
On 2017/04/19 17:58:48, weiliangc wrote:
> If subtree_has_copy_request is only useful here, why not just update the value
> right before calling LayerShouldBeSkipped in FindLayersThatNeedUpdates? That
way
> we don't need to care about keep track of it during property tree builder?

I want to stop creating effect nodes for handling SetHideLayerAndSubtree
(https://bugs.chromium.org/p/chromium/issues/detail?id=595843#c5). To do that,
we will need to know subtree_has_copy_request during property tree building as
we have to create effects nodes in case copy requests exist. So, this CL is to
make that possible.

https://codereview.chromium.org/2822303003/diff/20001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2822303003/diff/20001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:1356:
root_layer->layer_tree_host()->SetHasCopyRequest(false);
On 2017/04/19 17:58:48, weiliangc wrote:
> Since we always collect the copy requests with property tree builder, this is
> equivalent to effect tree's copy_requests_ not empty right?

No, they are not equivalent. copy requests are added to layers. As soon as any
layer has copy request, we set LTH::has_copy_request (at this point we haven't
build property trees). If this bool is true, we go and update
Layer::subtree_has_copy_request just before PT building. So, this bool is an
optimization to make sure we don't unnecessarily walk the layer tree when there
are no copy requests. During PT building, copy requests are moved to effect tree
and this bool is no longer required. But, we have to make sure this bool is set
to false before the next PT build, otherwise we will walk layer tree and update
Layer::subtree_has_copy_request even if there are no new copy requests.

[weiliangc, 2017-04-19 21:21:54]

https://codereview.chromium.org/2822303003/diff/20001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2822303003/diff/20001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:461: if (effect_node->has_render_surface &&
effect_node->subtree_has_copy_request)
On 2017/04/19 at 18:15:14, jaydasika wrote:
> On 2017/04/19 17:58:48, weiliangc wrote:
> > If subtree_has_copy_request is only useful here, why not just update the
value
> > right before calling LayerShouldBeSkipped in FindLayersThatNeedUpdates? That
way
> > we don't need to care about keep track of it during property tree builder?
> 
> I want to stop creating effect nodes for handling SetHideLayerAndSubtree
(https://bugs.chromium.org/p/chromium/issues/detail?id=595843#c5). To do that,
we will need to know subtree_has_copy_request during property tree building as
we have to create effects nodes in case copy requests exist. So, this CL is to
make that possible.

If we want to avoid sending those layers over, could we ignore them while
building layer list/property trees? and is it possible to not have
subtree_has_copy_request value in the node in the future?

[jaydasika, 2017-04-19 21:31:36]

https://codereview.chromium.org/2822303003/diff/20001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2822303003/diff/20001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:461: if (effect_node->has_render_surface &&
effect_node->subtree_has_copy_request)
On 2017/04/19 21:21:54, weiliangc wrote:
> On 2017/04/19 at 18:15:14, jaydasika wrote:
> > On 2017/04/19 17:58:48, weiliangc wrote:
> > > If subtree_has_copy_request is only useful here, why not just update the
> value
> > > right before calling LayerShouldBeSkipped in FindLayersThatNeedUpdates?
That
> way
> > > we don't need to care about keep track of it during property tree builder?
> > 
> > I want to stop creating effect nodes for handling SetHideLayerAndSubtree
> (https://bugs.chromium.org/p/chromium/issues/detail?id=595843#c5). To do that,
> we will need to know subtree_has_copy_request during property tree building as
> we have to create effects nodes in case copy requests exist. So, this CL is to
> make that possible.
> 
> If we want to avoid sending those layers over, could we ignore them while
> building layer list/property trees? and is it possible to not have
> subtree_has_copy_request value in the node in the future?

Yes, that's the plan I have in mind : skip these layers while property tree
building and don't send them at commit so LayerImpl::layer_list won't have them.
I think it should be possible to delete effect node's subtree_has_copy_request.

[jaydasika, 2017-04-21 03:38:36]

ping

[weiliangc, 2017-04-24 18:33:57]

LGTM. Could you update the bug description with your plans? Thanks.

[jaydasika, 2017-04-24 19:17:29]

Description was changed from

==========
cc : Compute subtree has copy requests before property tree building

To determine if we can skip creating effect nodes for hidden subtrees
during property tree building, we need to know if the subtree has any
copy requests. So, moving this computation to happen before property
tree building.

BUG=595843
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc : Compute subtree has copy requests before property tree building

To determine if we can skip creating effect nodes for hidden subtrees
during property tree building, we need to know if the subtree has any
copy requests. So, moving this computation to happen before property
tree building. After this CL, we can stop creating effect nodes for hidden
subtrees and also stop pushing these layers to compositor thread at commit. 

BUG=595843
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[jaydasika, 2017-04-24 19:17:40]

Description was changed from

==========
cc : Compute subtree has copy requests before property tree building

To determine if we can skip creating effect nodes for hidden subtrees
during property tree building, we need to know if the subtree has any
copy requests. So, moving this computation to happen before property
tree building. After this CL, we can stop creating effect nodes for hidden
subtrees and also stop pushing these layers to compositor thread at commit. 

BUG=595843
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc : Compute subtree has copy requests before property tree building

To determine if we can skip creating effect nodes for hidden subtrees
during property tree building, we need to know if the subtree has any
copy requests. So, moving this computation to happen before property
tree building. After this CL, we can stop creating effect nodes for 
hidden subtrees and also stop pushing these layers to compositor thread 
at commit. 

BUG=595843
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[jaydasika, 2017-04-24 19:17:49]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2017-04-24 19:18:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 20:34:04]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493061469727660,
"parent_rev": "c15ac888ad840b483c5abf18863fb48f090386e4", "commit_rev":
"ebc9428960482240e8a1114b7b795cabf42a6b82"}

[commit-bot: I haz the power, 2017-04-24 20:34:19]

Description was changed from

==========
cc : Compute subtree has copy requests before property tree building

To determine if we can skip creating effect nodes for hidden subtrees
during property tree building, we need to know if the subtree has any
copy requests. So, moving this computation to happen before property
tree building. After this CL, we can stop creating effect nodes for 
hidden subtrees and also stop pushing these layers to compositor thread 
at commit. 

BUG=595843
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc : Compute subtree has copy requests before property tree building

To determine if we can skip creating effect nodes for hidden subtrees
during property tree building, we need to know if the subtree has any
copy requests. So, moving this computation to happen before property
tree building. After this CL, we can stop creating effect nodes for 
hidden subtrees and also stop pushing these layers to compositor thread 
at commit. 

BUG=595843
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2822303003
Cr-Commit-Position: refs/heads/master@{#466745}
Committed:
https://chromium.googlesource.com/chromium/src/+/ebc9428960482240e8a1114b7b79...
==========

[commit-bot: I haz the power, 2017-04-24 20:34:23]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/ebc9428960482240e8a1114b7b79...