Twitter.com is now extremely slow from border radius clips. Pushing these clips is correct, but due to the large number of layers on a twitter.com page, we need to find a way to make the performance better. An optimization idea I had is to start treating layers whose children all will respect their clips more like transform layers. Basically we could start by extending to cover positioned and relative positioned ancestor layers as long as they contain no fixed positioned descendants and establish a stacking context. This may not be good enough, though, since we might need to handle the case where an object doesn't establish a stacking context. That's the really hard case that we may have to figure out.
Created attachment 108541 [details] Patch
Attached a patch. Feel free to try it out on Chromium and report if it makes Twitter ok again for you. I'll need to refine it to not do this is fixed positioned objects are present before it will be correct, but let's find out if it works well enough first.
Note that performance isn't that bad in Safari using CG, so this does lead me to suspect that there is room for optimization in Skia when rounded rect clips are applied.
Comment on attachment 108541 [details] Patch Attachment 108541 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9841728 New failing tests: svg/custom/inline-svg-in-xhtml.xml fast/backgrounds/positioned-root.html fast/backgrounds/table-root.html
It looks like this gets scrolling on many-layered sites back up to 20 FPS on Chromium, which is a much better place to be than below 1. Can we pursue this change?
(In reply to comment #5) > It looks like this gets scrolling on many-layered sites back up to 20 FPS on Chromium, which is a much better place to be than below 1. Can we pursue this change? Glad to hear it improves things. The patch needs to be extended so that RenderLayers start tracking all fixed positioned objects that they contain in their subtrees. If that count is non-zero, then this optimization has to be turned off. That's the reason (presumably) that the three layout tests are failing.
Chromium bug tracking the issue there: http://code.google.com/p/chromium/issues/detail?id=97716 Note that it's trivial to apply a workaround on Chromium only to revert the bug fix in the function that applies the clip. Just don't make it apply rounded clips for now.
Created attachment 110386 [details] Patch
First cut of what I think Dave Hyatt is suggesting in comments #2 & #5. Deserves more testing. Uploaded because I can't reproduce the same set of LayoutTest failures locally that the bots reported in comment #4.
Comment on attachment 110386 [details] Patch Attachment 110386 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10027060 New failing tests: svg/custom/inline-svg-in-xhtml.xml fast/backgrounds/positioned-root.html
Because my first attempt to implement Dave's suggestions didn't fix the layout test issues, I've created https://bugs.webkit.org/show_bug.cgi?id=69844 to roll out the change for Chromium only until we have a handle on the performance.
Created attachment 111064 [details] Patch
Dave, that previous version had at least one logic error that I think I've corrected. However, two of the layout tests that your initial cut had trouble with are still failing. Is what I'm doing what you thought needed to be done?
Comment on attachment 111064 [details] Patch Attachment 111064 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10068450 New failing tests: svg/custom/inline-svg-in-xhtml.xml fast/repaint/fixed-scale.html fast/backgrounds/positioned-root.html fast/repaint/fixed-tranformed.html
Comment on attachment 111064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111064&action=review You forgot the changelog! > Source/WebCore/rendering/RenderLayer.cpp:4347 > + layer->m_fixedPositionedObjectsInSubtree -= delta; If the delta is -1, won't this increment the count? If not, or it that's the intention, then the code needs more comments!
Comment on attachment 111064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111064&action=review r- as the patch does not pass the tests and contains several errors in the logic. It would also be really nice to add a performance test for this bug (if that's possible). > Source/WebCore/rendering/RenderLayer.cpp:2642 > + if (paintFlags & RenderLayer::PaintLayerAppliedTransform) This can be replaced with a boolean at the call site (paintFlags & PaintLayerAppliedTransform). Now if you take the paintsWithTransform() check out and transform() out of shouldApplyHitTestTransform, your functions are equivalent. Unless the ordering is important for performance (you need a comment about that then) that I missed here. > Source/WebCore/rendering/RenderLayer.cpp:4219 > + && oldStyle && oldStyle->position() == FixedPosition) { Those 2 if's are not the negation of one another. This line should be: || (oldStyle && oldStyle->position() == FixedPosition) { To make the code more readable, please use some boolean variables. For example, this would be a lot easier to read: bool isFixedPositionedLayer = renderer()->style()->position() == FixedPosition; bool wasFixedPositionedLayer = oldStyle && oldStyle->position() == FixedPosition; if (isFixedPosition != wasFixedPosition) { ... } >> Source/WebCore/rendering/RenderLayer.cpp:4347 >> + layer->m_fixedPositionedObjectsInSubtree -= delta; > > If the delta is -1, won't this increment the count? If not, or it that's the intention, then the code needs more comments! It looks like the logic is backwards here. You would like your delta to be added to your count not to be subtracted. > Source/WebCore/rendering/RenderLayer.h:806 > + int m_fixedPositionedObjectsInSubtree; I really wonder if using a boolean would not make the whole logic better (also make us use less memory). You just need to know if you have *one* fixed descendant, not really the exact number. It should be renamed to m_hasFixedPositionedDescendant in this case (or an equivalent name). If that's not possible, the field should be unsigned as we don't expect it to be negative. A better name would be m_fixedPositionedDescendantCount.
Comment on attachment 111064 [details] Patch Thanks for the reviews, but with the responsible person ignoring the bug I'm marking my attempted patches obsolete. Chromium should be able to support the inefficient version early in the new year.
This was otherwise resolved.