Make compositing updates 30.7% faster for calculator

Make compositing updates 30.7% faster for calculator

This CL makes the compositing updates for the Polymer calculator sample app
30.7% faster by adding a tree-ordered tree walk before computing compositing
requirements.

Previously, we computed the absolute rects for RenderLayers during a
paint-ordered tree walk in computeCompositingRequirements. However, that's
inefficient for three reasons:

1) Most of the time is spent maintaining the RenderGeometryMap, but the
   RenderGeometryMap is optimized for tree-ordered tree walks. If the
   paint-order walk and the tree-order walk were different, we would thrash the
   geometry map.

2) The dirty bits for the absolute rects need to propagate down the tree-order,
   but when walking in paint-order, we didn't have a good way to propagate the
   bits to the tree-order decendants. Instead, we would set update type to
   "force" for the rest of the siblings in the paint-order walk, which is both
   inefficient and incorrect.

3) Finally, this change lets us move the RenderGeometryMap out of
   computeCompositingRequirements entirely, which means we don't need to
   maintain the map as we visit every RenderLayer in the system. Intead, we
   need to maintain the map only for the RenderLayers that we visit in the
   tree-ordered walk, which is pruned by dirty bits.

Now that we have this tree-ordered phase, we can move a bunch of other
computation of tree-ordered data to this phase. Currently we compute that data
during the paint-ordered walks by walking the tree excessively. However, that
works is reserved for a future CL.

R=ojan@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170141

[ojan, 2014-03-27 01:06:41]

lgtm

https://codereview.chromium.org/213773002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/213773002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.h:727: unsigned
m_decendantNeedsToUpdateAncestorDependentProperties : 1;
In the rest of the render tree code we would call this
m_childNeedsToUpdateAncestorDependantProperties. It's less accurate of course,
but it's a bit less unwieldy. I don't feel strongly, but it might be worth it
for consistency sake.

https://codereview.chromium.org/213773002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.h:793: mutable
OwnPtr<AncestorDependentPropertyCache> m_ancestorDependentPropertyCache;
Doesn't putting this pointer among non-pointers lead to sub-optimal packing?

https://codereview.chromium.org/213773002/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/CompositingPropertyUpdater.cpp (right):

https://codereview.chromium.org/213773002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/CompositingPropertyUpdater.cpp:43:
layer->clearDecendantNeedsToUpdateAncestorDependentProperties();
Nit: I'd put this line at the very end of this function.

https://codereview.chromium.org/213773002/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/213773002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:493:
CompositingPropertyUpdater().updateAncestorDependentProperties(updateRoot,
m_pendingPropertyUpdateType);
Can we assert after this that all the dirty bits have been cleared? The new
equivalent to assertNeedsRecomputeBoundsBitsCleared.

https://codereview.chromium.org/213773002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:509:
assertNeedsRecomputeBoundsBitsCleared(updateRoot);
You need to delete this function.

[abarth-chromium, 2014-03-27 01:40:11]

Thanks for the review!

https://codereview.chromium.org/213773002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/213773002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.h:727: unsigned
m_decendantNeedsToUpdateAncestorDependentProperties : 1;
On 2014/03/27 01:06:42, ojan wrote:
> In the rest of the render tree code we would call this
> m_childNeedsToUpdateAncestorDependantProperties. It's less accurate of course,
> but it's a bit less unwieldy. I don't feel strongly, but it might be worth it
> for consistency sake.

I'll make it consistent.

https://codereview.chromium.org/213773002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.h:793: mutable
OwnPtr<AncestorDependentPropertyCache> m_ancestorDependentPropertyCache;
On 2014/03/27 01:06:42, ojan wrote:
> Doesn't putting this pointer among non-pointers lead to sub-optimal packing?

Yes.  I'll put it back.

This object is very poorly packed.  We might want do a follow up CL to optimize
its packing.

https://codereview.chromium.org/213773002/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/CompositingPropertyUpdater.cpp (right):

https://codereview.chromium.org/213773002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/CompositingPropertyUpdater.cpp:43:
layer->clearDecendantNeedsToUpdateAncestorDependentProperties();
On 2014/03/27 01:06:42, ojan wrote:
> Nit: I'd put this line at the very end of this function.

Ok.  Maybe I should make a RAII for the geometry map pushing/popping.

https://codereview.chromium.org/213773002/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/213773002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:493:
CompositingPropertyUpdater().updateAncestorDependentProperties(updateRoot,
m_pendingPropertyUpdateType);
On 2014/03/27 01:06:42, ojan wrote:
> Can we assert after this that all the dirty bits have been cleared? The new
> equivalent to assertNeedsRecomputeBoundsBitsCleared.

Sure.

https://codereview.chromium.org/213773002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:509:
assertNeedsRecomputeBoundsBitsCleared(updateRoot);
On 2014/03/27 01:06:42, ojan wrote:
> You need to delete this function.

Will do.

[abarth-chromium, 2014-03-27 03:13:08]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-03-27 03:13:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/213773002/20001

[commit-bot: I haz the power, 2014-03-27 03:21:05]

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

[commit-bot: I haz the power, 2014-03-27 03:21:06]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_compile_dbg

[abarth-chromium, 2014-03-27 03:29:08]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-03-27 03:29:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/213773002/40001

[commit-bot: I haz the power, 2014-03-27 03:32:22]

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

[commit-bot: I haz the power, 2014-03-27 03:32:22]

Try jobs failed on following builders:
  tryserver.blink on blink_presubmit

[abarth-chromium, 2014-03-27 03:47:36]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-03-27 03:47:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/213773002/40001

[commit-bot: I haz the power, 2014-03-27 04:35:23]

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

[commit-bot: I haz the power, 2014-03-27 04:35:24]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[abarth-chromium, 2014-03-27 04:57:32]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-03-27 04:57:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/213773002/60001

[commit-bot: I haz the power, 2014-03-27 05:56:11]

Change committed as 170141

[dcheng, 2014-04-12 18:21:44]

On 2014/03/27 05:56:11, I haz the power (commit-bot) wrote:
> Change committed as 170141

This patch appears to be causing one of the browser tests on ChromeOS to fail
occasionally (browser_tests
--gtest_filter=NavigationList/FileManagerBrowserTest.Test/0) by triggering this
assert:
ASSERTION FAILED: !m_needsToUpdateAncestorDependentProperties
../../third_party/WebKit/Source/core/rendering/RenderLayer.h(493) : const
WebCore::RenderLayer::AncestorDependentProperties&
WebCore::RenderLayer::ancestorDependentProperties() const

The bot is at:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

[abarth-chromium, 2014-04-12 18:49:26]

On 2014/04/12 18:21:44, dcheng wrote:
> On 2014/03/27 05:56:11, I haz the power (commit-bot) wrote:
> > Change committed as 170141
> 
> This patch appears to be causing one of the browser tests on ChromeOS to fail
> occasionally (browser_tests
> --gtest_filter=NavigationList/FileManagerBrowserTest.Test/0) by triggering
this
> assert:
> ASSERTION FAILED: !m_needsToUpdateAncestorDependentProperties
> ../../third_party/WebKit/Source/core/rendering/RenderLayer.h(493) : const
> WebCore::RenderLayer::AncestorDependentProperties&
> WebCore::RenderLayer::ancestorDependentProperties() const
> 
> The bot is at:
>
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

That's very likely to be the same bug as
https://code.google.com/p/chromium/issues/detail?id=362753.  It appears to only
affect HighDPI devices.  We'll get it fixed soon.