Bug 31879 – Optimize the hierarchy rebuilding of compositing layers

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

When updating the compositing layer tree, at each compositing layer, we unparent all the child layers, and then re-add them one by one as we find they are composited.

This is inefficient; it also leaves the layers without a parent over some calls, which I need to fix in order to work on reflections.

[Simon Fraser (smfr)]

Created attachment 43856 [details]
Patch

[Simon Fraser (smfr)]

I filed bug 31880 to fix GraphicsLayerCACF.

Comment on attachment 43856 [details]
Patch

> +        (WebCore::RenderLayerCompositor::rebuildCompositingLayerTree): Changed to
> +        collect child layers into Vectors of GraphicsLayers, which can be set as
> +        layer childen in one go.

Typo: childen.

> +void GraphicsLayer::setChildren(const Vector<GraphicsLayer*>& newChildren)

Perhaps you should use the GraphicsLayerList type here (move its definition if needed).

> +    for (size_t i = 0; i < listSize; ++i)
> +        addChild(newChildren.at(i));

Usually at() is used with a pointer to a vector. Since you have a reference, please use [] notation.

> +    GraphicsLayer::setChildren(children);
> +    noteLayerPropertyChanged(ChildrenChanged);

Would it be more efficient if setChildren() returned a bool saying whether it had actually done anything, so you could skip notLayerPropertyChanged() if it hadn’t? Or is it very uncommon for setChildren() to be called with the same children vector?

>  
> -    // Now create and parent the compositing layers.
> -    {
> +    if (needLayerRebuild) {
> +        // Now updated and parent the compositing layers.
>          CompositingState compState(updateRoot);
> -        rebuildCompositingLayerTree(updateRoot, compState, needLayerRebuild);
> +        GraphicsLayerList childList;
> +        rebuildCompositingLayerTree(updateRoot, compState, childList);
> +
> +        // Host the document layer in the RenderView's root layer.
> +        if (updateRoot == rootRenderLayer() && !childList.isEmpty())
> +            m_rootPlatformLayer->setChildren(childList);
> +    } else {
> +        // We just need to do a geometry update.
> +        updateLayerTreeGeometry(updateRoot);
>      }
>      
>  #if PROFILE_LAYER_REBUILD
> @@ -596,11 +604,13 @@ bool RenderLayerCompositor::canAccelerateVideoRendering(RenderVideo* o) const
>  }
>  #endif
>  
> -void RenderLayerCompositor::rebuildCompositingLayerTree(RenderLayer* layer, struct CompositingState& compositingState, bool updateHierarchy)
> +void RenderLayerCompositor::rebuildCompositingLayerTree(RenderLayer* layer, const CompositingState& compositingState, GraphicsLayerList& enclosingChildList)

In the declaration, you called the last parameter “childList”. I now understand the “enclosing” part, but I don’t think the name is very good. “Enclosing” doesn’t refer to the children in the list. For one, I’d drop the word “list” and work “GraphicsLayer” into the name. I think this argument is a “list of child GraphicsLayer of the enclosing compositing layer”. Maybe “childGraphicsLayersOfEnclosingLayer”?

> +    GraphicsLayerList layerChildren;

Please move this definition down to where it’s first used.

> +// This just updates layer geometry without doing any reparenting.

Can you say something like “changing the hierarchy” instead of “doing any reparenting”?

I think this is good as-is, so r+, but feel free to post an updated version!

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/51476