Regression(r176212): Carousel on mbusa.com is choppy

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Chris Dumez]

Carousel on mbusa.com is choppy due to a DOM Timer being throttled when it shouldn't.

radar: <rdar://problem/19209406>.

[Chris Dumez]

Created attachment 243137 [details]
Patch

[Chris Dumez]

Created attachment 243143 [details]
Patch

[Chris Dumez]

Comment on attachment 243143 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1338
> +        accountForSelfPaintingDescendantsOverflow(child, repaintRect);

We can probably optimize further by not traversing the descendants if hasOverflowClip() return true?

[Chris Dumez]

Created attachment 243203 [details]
Patch

[Chris Dumez]

Comment on attachment 243203 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1341
> +    if (renderer.hasOverflowClip())

We now stop traversing descendants if the renderer clips its overflow to avoid unnecessary work. I also added a layout test to make sure we still throttle if the overflow is hidden.

[Chris Dumez]

Simon, what do you think?

[Simon Fraser (smfr)]

Comment on attachment 243203 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1348
> +    for (auto& child : childrenOfType<RenderLayerModelObject>(renderer)) {
> +        if (child.hasSelfPaintingLayer())
> +            repaintRect.unite(child.absoluteClippedOverflowRect());
> +        accountForSelfPaintingDescendantsOverflow(child, repaintRect);
> +    }

This seems very costly for deep trees with no overflow. You're also doing a tree walk where you do a mapping back to the root (under child.absoluteClippedOverflowRect()) for each node, so this is potentially O(N^2).

[Chris Dumez]

(In reply to comment #7)
> Comment on attachment 243203 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243203&action=review
> 
> > Source/WebCore/rendering/RenderElement.cpp:1348
> > +    for (auto& child : childrenOfType<RenderLayerModelObject>(renderer)) {
> > +        if (child.hasSelfPaintingLayer())
> > +            repaintRect.unite(child.absoluteClippedOverflowRect());
> > +        accountForSelfPaintingDescendantsOverflow(child, repaintRect);
> > +    }
> 
> This seems very costly for deep trees with no overflow. You're also doing a
> tree walk where you do a mapping back to the root (under
> child.absoluteClippedOverflowRect()) for each node, so this is potentially
> O(N^2).

As per our offline discussion. I will re-upload a patch that is very conservative and does not attempt to compute the overflow rect if:
- The element does not have its own layer
- Any descendant has its own layer

Both checks are very cheap and it will still cover the cases I am interested in.

I will also prepare another patch to only throttle timers that change a few CSS properties we know won't cause other elements to relayout (and will cause the element to have its own layer), namely left/opacity.

[Chris Dumez]

Created attachment 243262 [details]
Patch

[Chris Dumez]

Simon, is the latest iteration acceptable?

[Chris Dumez]

ping review?

[WebKit Commit Bot]

Comment on attachment 243262 [details]
Patch

Clearing flags on attachment: 243262

Committed r177964: <http://trac.webkit.org/changeset/177964>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.