update3dRenderingContext is 13.5% of GraphicsLayerUpdater::rebuildTree

update3dRenderingContext is 13.5% of GraphicsLayerUpdater::rebuildTree

Prior to this CL, we were recomputing the 3D rendering context root for every
RenderLayer every frame. That work accounted for 13.5% of
GraphicsLayerUpdater::rebuildTree and 2.4% of the entire compositing update.
This work is almost entirely wasted because the 3D rendering context is
completely determined by style information and therefore can change only when
style changes.

Elliott and I looked at integrating this computation with recalc style, but
upon studying the code closely, I realize that this function doesn't actually
walk up the tree very far. Instead, it looks only at the enclosing render
layer of the containing block. That means we don't need to cache this data on
RenderLayer at all. Instead, we can recompute it when needed by the one caller:
CompositedLayerMapping::updateRenderingContext. That caller only cares about
RenderLayers that actually have a composited layer mapping, which is vastly
fewer than all RenderLayers. Moreover, that caller already understands
dirtiness caused by style changes.

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

[ojan, 2014-03-25 01:35:59]

lgtm

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

https://codereview.chromium.org/209003014/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:569: // FIXME: Why doesn't this need to be
a loop?
Yeah, this seems like it can't possibly be correct. :(

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

https://codereview.chromium.org/209003014/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/GraphicsLayerUpdater.cpp:161: 
Did you mean to add this?

[abarth-chromium, 2014-03-25 02:31:37]

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

https://codereview.chromium.org/209003014/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/GraphicsLayerUpdater.cpp:161: 
On 2014/03/25 01:36:00, ojan wrote:
> Did you mean to add this?

Nope!  That was from an earlier iteration where I tried to move the work here.

[abarth-chromium, 2014-03-25 02:34:58]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-03-25 02:35:03]

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

[esprehn, 2014-03-25 02:39:35]

This changes the behavior. The function previously would reset the value if you
were !shouldFlattenTransform() on yourself, but it would also look up the tree.

You changed the behavior now to early return which doesn't seem right.

https://codereview.chromium.org/209003014/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/209003014/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:566: if (!shouldFlattenTransform())
I don't think this is correct. The previous code would reset if this was false
here, but then still look at the ancestor below.

[esprehn, 2014-03-25 02:39:38]

The CQ bit was unchecked by esprehn@chromium.org

[abarth-chromium, 2014-03-25 02:59:38]

https://codereview.chromium.org/209003014/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/209003014/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:566: if (!shouldFlattenTransform())
On 2014/03/25 02:39:35, esprehn wrote:
> I don't think this is correct. The previous code would reset if this was false
> here, but then still look at the ancestor below.

I see.  Yes, I screwed this up.  I'll work on a test case that would have caught
this issue tomorrow.

Thanks!

[ojan, 2014-03-25 04:16:27]

https://codereview.chromium.org/209003014/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/209003014/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:566: if (!shouldFlattenTransform())
On 2014/03/25 02:59:39, abarth wrote:
> On 2014/03/25 02:39:35, esprehn wrote:
> > I don't think this is correct. The previous code would reset if this was
false
> > here, but then still look at the ancestor below.

Ick. Good catch. The next question is why this code is this way? This seems
super weird to me.

[Ian Vollick, 2014-03-25 14:40:01]

The 3D rendering context id is an ancestor dependent property like containing
block or scroll parent. And in the same way you'd suggested that I pass down
scroll parent info, the previous code bubbled down 3D rendering context info. It
did that by guaranteeing that ancestors are processed before descendants (via
the deleted line in GraphicsLayerUpdater) so that children only needed to
consult either local or parent data.

I can see how it would look like it's only local information that's being
consulted, but this is not the case; the update is only one step in a full tree
bubbling.

If we want to cut down on the updating that we're doing here, I think we'll have
to prune the tree walk in the same way that you did for composited bounds
(https://codereview.chromium.org/208643002).

https://codereview.chromium.org/209003014/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/209003014/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:566: if (!shouldFlattenTransform())
On 2014/03/25 04:16:27, ojan wrote:
> On 2014/03/25 02:59:39, abarth wrote:
> > On 2014/03/25 02:39:35, esprehn wrote:
> > > I don't think this is correct. The previous code would reset if this was
> false
> > > here, but then still look at the ancestor below.
> 
> Ick. Good catch. The next question is why this code is this way? This seems
> super weird to me. 

Some possibly-useful background: the code was set up like this to enforce these
3d rendering context rules:
 - If your parent doesn't flatten the transform, you're in its 3d rendering
context, so you need to have its id. (this is what the 2nd if block is for --
and it's crucial that the parent's data be up to date if you want to use its
values rather than walking all the way up the tree).
 - Else, if you don't flatten the transform, you establish a new 3d rendering
context, so you get a new id. (that's what the 1st if block is for).
 - Otherwise, you're not in a 3d rendering context, so your id is zero.

The code could likely have been laid out more clearly, but that was the intent.

https://codereview.chromium.org/209003014/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:569: // FIXME: Why doesn't this need to be
a loop?
The 3d rendering context was meant to bubble down. The code was set up so that
values on the parent were always updated before visiting the children. The
children could then grab the bubbled values from their parent without ascending
higher in the tree.

This was what I was trying to communicate (probably badly) with the deleted
comment above this function.

[abarth-chromium, 2014-03-25 15:12:42]

Thanks for the detailed explanation.  IMHO, the first step here is for me to
write a test that fails with this patch applied.  :)

[abarth-chromium, 2014-03-25 22:28:49]

I made a test case that fails, but fixing this code didn't actually fix it.  I'm
going to land this updated CL and separately work on fixing the test....

[abarth-chromium, 2014-03-25 22:28:54]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-03-25 22:28:58]

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

[commit-bot: I haz the power, 2014-03-25 23:23:28]

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

[commit-bot: I haz the power, 2014-03-25 23:23:29]

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

[esprehn, 2014-03-25 23:26:41]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-25 23:26:43]

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

[commit-bot: I haz the power, 2014-03-26 00:03:14]

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

[commit-bot: I haz the power, 2014-03-26 00:03:14]

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

[esprehn, 2014-03-26 00:04:53]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-26 00:05:17]

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

[commit-bot: I haz the power, 2014-03-26 00:34:18]

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

[commit-bot: I haz the power, 2014-03-26 00:34:19]

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

[abarth-chromium, 2014-03-26 05:24:21]

https://codereview.chromium.org/209003014/diff/90001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/209003014/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:532: ASSERT_NOT_REACHED();
Looks like this line can be reached.  /me will investigate in the morning.

[abarth-chromium, 2014-03-26 17:28:23]

Turns out FrameView::paintContents is calling enclosingLayer on a null pointer.
I've removed the ASSERT for now. I'll add it back in a future CL when I fix
FrameView::paintContents not to violate the C++ spec.  :)

[abarth-chromium, 2014-03-26 17:28:28]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-03-26 17:28:38]

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

[commit-bot: I haz the power, 2014-03-26 17:39:01]

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

[commit-bot: I haz the power, 2014-03-26 17:39:02]

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

[esprehn, 2014-03-26 17:40:15]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-26 17:40:21]

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

[commit-bot: I haz the power, 2014-03-26 18:16:24]

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

[commit-bot: I haz the power, 2014-03-26 18:16:25]

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

[esprehn, 2014-03-26 18:32:22]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-26 18:32:32]

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

[commit-bot: I haz the power, 2014-03-26 18:33:33]

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

[commit-bot: I haz the power, 2014-03-26 18:33:34]

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

[esprehn, 2014-03-26 19:03:45]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-26 19:03:57]

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

[commit-bot: I haz the power, 2014-03-26 20:06:56]

Change committed as 170090