Bug 68733 – REGRESSION (r95239): [chromium] Twitter.com now extremely slow from border-radius clips.

• Component: Layout and Rendering
• Status: RESOLVED WONTFIX

[Dave Hyatt]

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.

[Dave Hyatt]

Created attachment 108541 [details]
Patch

[Dave Hyatt]

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.

[Dave Hyatt]

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.

[WebKit Review Bot]

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

[Tom Hudson]

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?

[Dave Hyatt]

(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.

[Dave Hyatt]

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.

[Tom Hudson]

Created attachment 110386 [details]
Patch

[Tom Hudson]

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.

[WebKit Review Bot]

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

[Tom Hudson]

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.

[Tom Hudson]

Created attachment 111064 [details]
Patch

[Tom Hudson]

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?

[WebKit Review Bot]

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

[Robert Hogan]

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!

[Julien Chaffraix]

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.

[Tom Hudson]

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.

[Stephen Chenney]

This was otherwise resolved.