Only do full tree sync if tree is actually changed, otherwise just push properties

Only do full tree sync if tree is actually changed, otherwise just push properties

We only have to run the complete tree synchronization algorithm if the cc::Layer tree's
structure is actually changed, otherwise we can just push properties over since the
cc::LayerImpl tree structure is never changed outside of commit.

Since scroll offset updates are tied into the tree synchronization algorithm, we currently
have to do a full tree sync for scroll updates. Fixing this will speed up scroll operations
but requires a bit more work.

Speeds up cc_perftests tenTenSingleThread by 7.3%

BUG=161166
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170881

[danakj, 2012-12-03 18:46:24]

https://codereview.chromium.org/11316297/diff/3001/cc/layer.cc
File cc/layer.cc (right):

https://codereview.chromium.org/11316297/diff/3001/cc/layer.cc#newcode420
cc/layer.cc:420: setNeedsFullTreeSync();
How come a scrollOffset needs a full sync?

https://codereview.chromium.org/11316297/diff/3001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11316297/diff/3001/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:328: DCHECK_EQ(children.size(), implChildren.size());
should it CRASH() in this case? We could end up passing an address off the end
of the implChildren array to pushPropertiesTo() which could result in bad times.

[enne (OOO), 2012-12-03 21:26:47]

Ooh, this is a nice optimization.  lgtm other than danakj's concerns.

I am not sure how this is going to work for two trees on the impl side; maybe we
can recycle the previous active tree and keep track of more state for whether
the two previous commits needed a tree sync.  It seems plausible we could make
it work there.

[jamesr, 2012-12-03 22:25:23]

On 2012/12/03 21:26:47, enne wrote:
> Ooh, this is a nice optimization.  lgtm other than danakj's concerns.
> 
> I am not sure how this is going to work for two trees on the impl side; maybe
we
> can recycle the previous active tree and keep track of more state for whether
> the two previous commits needed a tree sync.  It seems plausible we could make
> it work there.

This tracking is all main thread, so I think it should still work in the same
way since if the main thread structure is unchanged from the previous commit,
both the pending and active trees can be assumed to have the same structure.

[jamesr, 2012-12-04 01:35:50]

On 2012/12/03 18:46:24, danakj wrote:
> https://codereview.chromium.org/11316297/diff/3001/cc/layer.cc
> File cc/layer.cc (right):
> 
> https://codereview.chromium.org/11316297/diff/3001/cc/layer.cc#newcode420
> cc/layer.cc:420: setNeedsFullTreeSync();
> How come a scrollOffset needs a full sync?
> 

To get the scroll offset to the scrollbar layers.  Currently this is done as a
side effect of LayerImpl::set(Horiz|Verti)ScrollbarLayer, which pulls the scroll
offset from the LayerImpl to the animator.  This call is made from a post-sync
operation in TreeSynchronizer that also fixes up scrollbar related pointers in
the case that they change.  I think the best fix is to just run
pushPropertiesTo() after the tree sync (including scrollbar fixups) in all
cases, but that's for a future patch.


> https://codereview.chromium.org/11316297/diff/3001/cc/layer_tree_host.cc
> File cc/layer_tree_host.cc (right):
> 
>
https://codereview.chromium.org/11316297/diff/3001/cc/layer_tree_host.cc#newc...
> cc/layer_tree_host.cc:328: DCHECK_EQ(children.size(), implChildren.size());
> should it CRASH() in this case? We could end up passing an address off the end
> of the implChildren array to pushPropertiesTo() which could result in bad
times.

It will almost certainly crash.  I don't really think it's worthwhile to put
explicit CRASH()es here unless we start getting reports from the field - we
haven't had any tree syncing bugs that I can recall in a while and I don't
expect to see too many soon that won't be caught by our unit tests.

[danakj, 2012-12-04 01:38:48]

ok lgtm2

[jamesr, 2012-12-04 01:40:31]

On 2012/12/04 01:35:50, jamesr wrote:
> On 2012/12/03 18:46:24, danakj wrote:
> > https://codereview.chromium.org/11316297/diff/3001/cc/layer.cc
> > File cc/layer.cc (right):
> > 
> > https://codereview.chromium.org/11316297/diff/3001/cc/layer.cc#newcode420
> > cc/layer.cc:420: setNeedsFullTreeSync();
> > How come a scrollOffset needs a full sync?
> > 
> 
> To get the scroll offset to the scrollbar layers.  Currently this is done as a
> side effect of LayerImpl::set(Horiz|Verti)ScrollbarLayer, which pulls the
scroll
> offset from the LayerImpl to the animator.  This call is made from a post-sync
> operation in TreeSynchronizer that also fixes up scrollbar related pointers in
> the case that they change.  I think the best fix is to just run
> pushPropertiesTo() after the tree sync (including scrollbar fixups) in all
> cases, but that's for a future patch.

Filed http://code.google.com/p/chromium/issues/detail?id=164086

[commit-bot: I haz the power, 2012-12-04 01:42:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11316297/3001

[commit-bot: I haz the power, 2012-12-04 04:25:12]

Change committed as 170881