Bug 84410 – Terrible performance on http://alliances.commandandconquer.com/ and http://www.lordofultima.com/

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

[Simon Fraser (smfr)]

From <http://code.google.com/p/chromium/issues/detail?id=120871>

URLs (if applicable):
http://alliances.commandandconquer.com/en/
http://www.lordofultima.com/en/

Other browsers tested:
Add OK or FAIL after other browsers where you have tested this issue:
Safari 5:
     Firefox 4.x: OKAY (far better)
     IE 7/8/9: OKAY (far better)

What steps will reproduce the problem?
1. go to a page mentioned above
2. start a game
3. ingame performance is really bad while IE and FF have solid 40 to 60 fps

What is the expected result?

Good performance with at least 30 fps. With the behaviour at this moment the games mentioned above cannot be played with Chrome,


What happens instead?

Laggy/choppy performance

[Radar WebKit Bug Importer]

<rdar://problem/11286123>

[Simon Fraser (smfr)]

Adding a single 3D transform to the page (to turn off overlap testing) takes the CPU usage from 100% to 30% on my machine, using the CC reduction in the chromium bug.

[Simon Fraser (smfr)]

My plans here are to avoid O(N^2) behavior when computing absolute layer bounds to add to the overlap map by storing some data as we walk the layer tree that allows us to map to absolute coords more efficiently.

[Adrienne Walker]

(In reply to comment #3)
> My plans here are to avoid O(N^2) behavior when computing absolute layer bounds to add to the overlap map by storing some data as we walk the layer tree that allows us to map to absolute coords more efficiently.

That sounds great.  I've been having similar thoughts.

As you modify what's going into the overlap map, can you see if this fixes the issue in bug 64201?

[Simon Fraser (smfr)]

Ugh, layer->backgroundClipRect(rootRenderLayer(), 0, true) is also super-expensive.

[Simon Fraser (smfr)]

That was added in http://trac.webkit.org/changeset/91114

[Adrienne Walker]

(In reply to comment #6)
> That was added in http://trac.webkit.org/changeset/91114

In retrospect, I'm pretty sure that we don't need to use temp clip bounds there, which would help immensely.

[Simon Fraser (smfr)]

We do, because the cache rects are cached for painting (relative to repaint containers), and this rect is root-relative.

[Simon Fraser (smfr)]

I think the best option here will be to cache both clipRects for painting, and absolute clip rects.

[Adrienne Walker]

(In reply to comment #8)
> We do, because the cache rects are cached for painting (relative to repaint containers), and this rect is root-relative.

You said you were planning to store some transform data as you walked the tree; would be possible to reuse some of that data to transform cached clip rects into the right space?

Alternatively, RenderLayer::localClipRect gets reused when calculating composited bounds, so maybe that's the extra one to cache.

[Simon Fraser (smfr)]

Created attachment 143434 [details]
Part 1: use a geometry map to optimize absolute layer bounds

[Dave Hyatt]

Comment on attachment 143434 [details]
Part 1: use a geometry map to optimize absolute layer bounds

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

Overall looks nice. I'd like a bit of an explanation regarding why flipped blocks writing modes can't be dealt with. I'm a bit confused about that part. I thought offsetFromContainer did the right thing for those.

> Source/WebCore/rendering/RenderGeometryMap.cpp:157
> +            // The root get special treatment for fixed position

Should be "The root gets"

> Source/WebCore/rendering/RenderInline.cpp:1148
> +    if (container->isBox() && container->style()->isFlippedBlocksWritingMode())
> +        offsetDependsOnPoint = true;

Confused regarding why you can't just flip (somewhere) to handle this.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:725
> +//        absBounds = layer->renderer()->localToAbsoluteQuad(FloatRect(layer->localBoundingBox())).enclosingBoundingBox();

Probably shouldn't leave commented out code in.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:734
> -    
> +

Whitespace.

> Source/WebCore/rendering/RenderObject.cpp:2060
> +    LayoutSize offset;
> +    if (container->hasOverflowClip())
> +        offset = -toRenderBox(container)->scrolledContentOffset();

I'm a little confused about why this has to be special cased.

> Source/WebCore/rendering/RenderObject.cpp:2062
> +    bool isNonUniform = hasColumns() || (container->isBox() && container->style()->isFlippedBlocksWritingMode());

Is it really that hard to deal with flipped blocks? Don't you just have to flipForWritingMode along the correct axis?

> Source/WebCore/rendering/RenderObject.h:885
> +    // Pushes state onto RenderGeometryMap about how to map coordinates from this renderer to its condtainer, or ancestorToStopAt (whichever is encountered first).

Typo. "condtainer" should be "container"

[Simon Fraser (smfr)]

(In reply to comment #12)

> > Source/WebCore/rendering/RenderObject.cpp:2060
> > +    LayoutSize offset;
> > +    if (container->hasOverflowClip())
> > +        offset = -toRenderBox(container)->scrolledContentOffset();
> 
> I'm a little confused about why this has to be special cased.

I was following the code in RenderObject::mapLocalToContainer(). However, now I see that RenderObject::offsetFromContainer() takes the scroll offset into account, so perhaps we're doing it twice.

[Simon Fraser (smfr)]

Created attachment 143929 [details]
Patch

[Simon Fraser (smfr)]

Fixed the patch in response to comments.

The RenderObject mapping methods appear to never be called (because they are always overidden. Maybe they should ASSERT_NOT_REACHED?

[Dave Hyatt]

Comment on attachment 143929 [details]
Patch

r=me. Should follow up with ASSERT_NOT_REACHED and clean those methods up in RenderObject in a future patch.

[Simon Fraser (smfr)]

Followups: bug 87516, bug 87517.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/118567

[Simon Fraser (smfr)]

The reduced testcase in the cr bug takes 25% CPU after fixes for this and bug 87212. It used to be 100%.

[Adrienne Walker]

Comment on attachment 143929 [details]
Patch

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

> Source/WebCore/rendering/RenderGeometryMap.cpp:149
> +        if (currStep->m_hasTransform) {
> +            // If this box has a transform, it acts as a fixed position container for fixed descendants,
> +            // and may itself also be fixed position. So propagate 'fixed' up only if this box is fixed position.
> +            inFixed &= currStep->m_isFixedPosition;

How does this assignment work if inFixed starts false (which it's initialized to), but this layer has both a transform and is fixed position? Shouldn't inFixed be true in that case? I wonder if this should be "=" instead of "&=".

This causes the test in the patch on bug 64201 to break.

[Simon Fraser (smfr)]

(In reply to comment #20)
> (From update of attachment 143929 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143929&action=review
> 
> > Source/WebCore/rendering/RenderGeometryMap.cpp:149
> > +        if (currStep->m_hasTransform) {
> > +            // If this box has a transform, it acts as a fixed position container for fixed descendants,
> > +            // and may itself also be fixed position. So propagate 'fixed' up only if this box is fixed position.
> > +            inFixed &= currStep->m_isFixedPosition;
> 
> How does this assignment work if inFixed starts false (which it's initialized to), but this layer has both a transform and is fixed position? Shouldn't inFixed be true in that case? I wonder if this should be "=" instead of "&=".

That does look wrong. Please file a new bug.

> This causes the test in the patch on bug 64201 to break.

Yay, a testcase!

[Adrienne Walker]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 143929 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=143929&action=review
> > 
> > > Source/WebCore/rendering/RenderGeometryMap.cpp:149
> > > +        if (currStep->m_hasTransform) {
> > > +            // If this box has a transform, it acts as a fixed position container for fixed descendants,
> > > +            // and may itself also be fixed position. So propagate 'fixed' up only if this box is fixed position.
> > > +            inFixed &= currStep->m_isFixedPosition;
> > 
> > How does this assignment work if inFixed starts false (which it's initialized to), but this layer has both a transform and is fixed position? Shouldn't inFixed be true in that case? I wonder if this should be "=" instead of "&=".
> 
> That does look wrong. Please file a new bug.

Bug 64201 covers this.  See the updated patch there.