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.
Created attachment 199336 [details] Patch V1
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.
(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.
It would be instructive here to say if you saw marked performance issues on any real web sites.
Created attachment 199507 [details] Patch V1 Refactored the willBeComposited to avoid calling requiresCompositingLayer twice.
(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.
Do we still want to consider this issue? It's languishing in the review queue.
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
Created attachment 229496 [details] Patch
Comment on attachment 229496 [details] Patch Clearing flags on attachment: 229496 Committed r167407: <http://trac.webkit.org/changeset/167407>
All reviewed patches have been landed. Closing bug.