Bug 31879 - Optimize the hierarchy rebuilding of compositing layers
Summary: Optimize the hierarchy rebuilding of compositing layers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-25 09:51 PST by Simon Fraser (smfr)
Modified: 2009-11-29 19:22 PST (History)
2 users (show)

See Also:


Attachments
Patch (14.80 KB, patch)
2009-11-25 10:02 PST, Simon Fraser (smfr)
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2009-11-25 09:51:44 PST
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.
Comment 1 Simon Fraser (smfr) 2009-11-25 10:02:55 PST
Created attachment 43856 [details]
Patch
Comment 2 Simon Fraser (smfr) 2009-11-25 10:05:05 PST
I filed bug 31880 to fix GraphicsLayerCACF.
Comment 3 mitz 2009-11-27 15:18:39 PST
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!
Comment 4 Simon Fraser (smfr) 2009-11-29 19:22:31 PST
http://trac.webkit.org/changeset/51476