Bug 115063 – Improve performance of the RenderLayerCompositor::OverlapMap

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

[Alexandru Chiculita]

The RenderLayerCompositor::computeCompositingRequirements is using the OverlapMap to check if layers need to be promoted to composited layers, because they display in front of other composited layers.

There are two problems:
1. Sometimes we already know we want to make the layer composited (ie. it has a 3d transform)
2. All the previous layers are not overlapping.

[Alexandru Chiculita]

Created attachment 199336 [details]
Patch V1

[Simon Fraser (smfr)]

Comment on attachment 199336 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=199336&action=review

> Source/WebCore/rendering/RenderLayerCompositor.cpp:123
> +    Vector<IntRect> m_layerRects;
> +    IntRect m_boundingBox;
> +};

This is becoming more like a Region. How does the performance of this compare with a Region? (I think we used them in the past with poor results.)

> Source/WebCore/rendering/RenderLayerCompositor.cpp:943
> +    if (overlapMap && !overlapMap->isEmpty() && compositingState.m_testingOverlap && !requiresCompositingLayer(layer)) {

requiresCompositingLayer() is now getting called twice for each computeCompositingRequirements() call; once here, and once inside needsToBeComposited(). It's not very cheap, so we should refactor to call it only once.

[Alexandru Chiculita]

(In reply to comment #2)
> (From update of attachment 199336 [details])
Thanks for your review!

> View in context: https://bugs.webkit.org/attachment.cgi?id=199336&action=review
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:123
> > +    Vector<IntRect> m_layerRects;
> > +    IntRect m_boundingBox;
> > +};
> 
> This is becoming more like a Region. How does the performance of this compare with a Region? (I think we used them in the past with poor results.)
> 
I've tested the Region and it seems to have poor performance because of the maintenance overhead. For example in this example it makes the test go from 700 to around 4500. Considering that I didn't remove the bounding box check, the increase was only accounting for the creation of the Region and not the actual queries.

I think that the Region is too generic for this scenario. I was thinking that something like a tree should better cover this problem as it can optimise both the insertion and the queries: http://en.wikipedia.org/wiki/Spatial_index#Spatial_index . 

Another simple check that I could do without implementing anything would be to have two "pod interval trees". We've used that structure with floats before. The trees could to store the rectangles for X and Y axis, respectively. We can then search both trees and compute the intersection of the resulting lists. If there's a common rect in both lists, we can assume the layer will overlap with that rect.

I'm not yet sure if this kind of scenario really exists in real web applications (ie. lots of 3d layers that need to check for overlap), so having a more complex structure might introduce an insertion overhead for the most common cases.

That's why I think the bounding box is reasonable enough for the most common cases, where layers are not overlapping at all and are the result of the block layout positioning mechanism.

Also, checking if the layer already has a 3D transform allows app developers to optimise their apps in such a way that the overlap map is never consulted. I would rather consider the overlap map an emergency mechanism. In that case we might even consider computing the overlap map, lazily, the first time it is needed. 

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:943
> > +    if (overlapMap && !overlapMap->isEmpty() && compositingState.m_testingOverlap && !requiresCompositingLayer(layer)) {
> 
> requiresCompositingLayer() is now getting called twice for each computeCompositingRequirements() call; once here, and once inside needsToBeComposited(). It's not very cheap, so we should refactor to call it only once.

Agree, my initial thought was to have it in a separate patch. 

The only difference that the second call would have is the indirect reason that is computed part of this initial step. I'm fine with doing that in this patch. Will have a new patch tomorrow.

[Simon Fraser (smfr)]

It would be instructive here to say if you saw marked performance issues on any real web sites.

[Alexandru Chiculita]

Created attachment 199507 [details]
Patch V1

Refactored the willBeComposited to avoid calling requiresCompositingLayer twice.

[Alexandru Chiculita]

(In reply to comment #4)
> It would be instructive here to say if you saw marked performance issues on any real web sites.

I couldn't find a real website that would create so many layers to trigger the issue. However, the check for being composited or not should not hurt performance.

[Brent Fulgham]

Do we still want to consider this issue? It's languishing in the review queue.

[WebKit Commit Bot]

Comment on attachment 199507 [details]
Patch V1

Rejecting attachment 199507 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 199507, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:

Hunk #1 succeeded at 105 (offset 15 lines).
Hunk #2 succeeded at 155 (offset 15 lines).
Hunk #3 FAILED at 166.
Hunk #4 FAILED at 176.
Hunk #5 FAILED at 947.
Hunk #6 FAILED at 979.
Hunk #7 succeeded at 1162 with fuzz 2 (offset 187 lines).
4 out of 7 hunks FAILED -- saving rejects to file Source/WebCore/rendering/RenderLayerCompositor.cpp.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Simon Fraser']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/5512279156064256

[Brent Fulgham]

Created attachment 229496 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 229496 [details]
Patch

Clearing flags on attachment: 229496

Committed r167407: <http://trac.webkit.org/changeset/167407>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.