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
<rdar://problem/11286123>
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.
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.
(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?
Ugh, layer->backgroundClipRect(rootRenderLayer(), 0, true) is also super-expensive.
That was added in http://trac.webkit.org/changeset/91114
(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.
We do, because the cache rects are cached for painting (relative to repaint containers), and this rect is root-relative.
I think the best option here will be to cache both clipRects for painting, and absolute clip rects.
(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.
Created attachment 143434 [details] Part 1: use a geometry map to optimize absolute layer bounds
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"
(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.
Created attachment 143929 [details] Patch
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?
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.
Followups: bug 87516, bug 87517.
http://trac.webkit.org/changeset/118567
The reduced testcase in the cr bug takes 25% CPU after fixes for this and bug 87212. It used to be 100%.
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.
(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!
(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.