So long as this feature is only used by clients that are using PropertyTreeBuilder to build property trees and convert from a layer tree to a layer list (and that's what ui and android will be doing, at least for the near future), SetHideLayerAndSubtree doesn't seem to block the switch from using layer trees to using layer lists (since the skipping is implemented by changing effective opacity to 0, and the effect tree is aware of effective opacity, so the layers that would have been skipped by subtree skipping can still be skipped by checking each individual layer's effective opacity using the effect tree).

Putting it down here so that we remember this while refactoring/removing :

layer_is_drawn already accounts for effective_opacity and background filters. So I think lines 364-371 can be deleted if we just return !layer_is_drawn after the  backface visibility check (I  might me missing some edge cases) :
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/draw_property_utils.cc&l=353
  

The current implementation of this is creating a large number of property tree nodes on the CrOS browser compositor, where SetHideLayerAndSubtree is used a lot. Since the implementation makes effective opacity become 0, we get an effect node for each hidden subtree. Further, if there are at least 2 layers that draw content in the subtree, the effect node thinks it needs a render surface, so we currently also get a transform and effect node.

The result is that on a CrOS desktop build, with six windows open, we have ~250 effect nodes, ~120 transform nodes, and ~125 clip nodes. There are also a large number of layers (nearly 700), though that's not caused by property trees. Updating draw properties takes ~400 microseconds per frame on my z620.

We should try to stop creating effect nodes and pushing layers for things we know will never be shown (that is, for hidden things that have no descendant copy requests, we should skip creating property tree nodes and skip pushing the layers in that subtree to the compositor). A hacked-up version of this (that ignores the possibility of copy requests) speeds up the calculation of draw properties (on the same six window example) to about ~200 microseconds per frame.

Implementing a clean version of this might be easier once Blink no longer uses cc's PropertyTreeBuilder, since then we can change the Layer tree iterator to skip over the subtrees we want to skip (right now, PropertyTreeBuilder doesn't use a stacking order iterator to visit the Layer tree, since it needs to account for scroll children, which are a Blink-only concept), and use this iterator when building property trees and the layer list and get the skipping we want without adding any extra logic there.

Re #3 : Not just on Cr0s, we create a lot of effect nodes in general for handling SetHideLayerAndSubtree. I counted what triggers effect node creation on top 10k pages on Linux using cluster telemetry and found that ~75% of effect nodes are created for handling SetHideLayerAndSubtree.

As already suggested in #3, we should stop creating effect nodes (except when we have copy requests or opacity animation) and also skip pushing these layers at commit.

 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ebc9428960482240e8a1114b7b795cabf42a6b82

commit ebc9428960482240e8a1114b7b795cabf42a6b82
Author: jaydasika <jaydasika@chromium.org>
Date: Mon Apr 24 20:34:01 2017

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}

[modify] https://crrev.com/ebc9428960482240e8a1114b7b795cabf42a6b82/cc/layers/layer.cc
[modify] https://crrev.com/ebc9428960482240e8a1114b7b795cabf42a6b82/cc/layers/layer.h
[modify] https://crrev.com/ebc9428960482240e8a1114b7b795cabf42a6b82/cc/layers/layer_impl.cc
[modify] https://crrev.com/ebc9428960482240e8a1114b7b795cabf42a6b82/cc/layers/layer_impl.h
[modify] https://crrev.com/ebc9428960482240e8a1114b7b795cabf42a6b82/cc/layers/layer_impl_test_properties.cc
[modify] https://crrev.com/ebc9428960482240e8a1114b7b795cabf42a6b82/cc/layers/layer_impl_test_properties.h
[modify] https://crrev.com/ebc9428960482240e8a1114b7b795cabf42a6b82/cc/trees/draw_property_utils.cc
[modify] https://crrev.com/ebc9428960482240e8a1114b7b795cabf42a6b82/cc/trees/effect_node.cc
[modify] https://crrev.com/ebc9428960482240e8a1114b7b795cabf42a6b82/cc/trees/effect_node.h
[modify] https://crrev.com/ebc9428960482240e8a1114b7b795cabf42a6b82/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/ebc9428960482240e8a1114b7b795cabf42a6b82/cc/trees/layer_tree_host.h
[modify] https://crrev.com/ebc9428960482240e8a1114b7b795cabf42a6b82/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/ebc9428960482240e8a1114b7b795cabf42a6b82/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/ebc9428960482240e8a1114b7b795cabf42a6b82/cc/trees/property_tree.cc
[modify] https://crrev.com/ebc9428960482240e8a1114b7b795cabf42a6b82/cc/trees/property_tree_builder.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/10be211f21100f7e9a3069cdefff580ddd0ef544

commit 10be211f21100f7e9a3069cdefff580ddd0ef544
Author: jaydasika <jaydasika@chromium.org>
Date: Fri May 26 04:35:50 2017

cc : Store surface layer ids on LayerTreeHost and push them at commit

This CL stores surface layer ids on LayerTreeHost which is pushed onto
LayerTreeImpl on commit and activation. These ids are used make the
compositor frame metadata instead of LayerTreeImpl::surface_layers and
so this patch also deletes LayerTreeImpl::surface_layers.

This CL is required to stop pushing hidden subtrees at commit.
(See comments in crrev.com/2846653002)

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

Review-Url: https://codereview.chromium.org/2905533002
Cr-Commit-Position: refs/heads/master@{#474907}

[modify] https://crrev.com/10be211f21100f7e9a3069cdefff580ddd0ef544/cc/layers/surface_layer.cc
[modify] https://crrev.com/10be211f21100f7e9a3069cdefff580ddd0ef544/cc/layers/surface_layer.h
[modify] https://crrev.com/10be211f21100f7e9a3069cdefff580ddd0ef544/cc/layers/surface_layer_impl.cc
[modify] https://crrev.com/10be211f21100f7e9a3069cdefff580ddd0ef544/cc/layers/surface_layer_unittest.cc
[modify] https://crrev.com/10be211f21100f7e9a3069cdefff580ddd0ef544/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/10be211f21100f7e9a3069cdefff580ddd0ef544/cc/trees/layer_tree_host.h
[modify] https://crrev.com/10be211f21100f7e9a3069cdefff580ddd0ef544/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/10be211f21100f7e9a3069cdefff580ddd0ef544/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/10be211f21100f7e9a3069cdefff580ddd0ef544/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/10be211f21100f7e9a3069cdefff580ddd0ef544/cc/trees/layer_tree_impl.h

Re-assigning jaydasika's issues.  Chris could you please triage?

So I was in the neighborhood...