Changeset 132591 in webkit

Timestamp:

Oct 26, 2012 2:34:07 AM (12 years ago)

Author:

jchaffraix@webkit.org

Message:

Generate less repaint calls during subtree detaching
​https://bugs.webkit.org/show_bug.cgi?id=99741

Reviewed by Eric Seidel.

Source/WebCore:

Following bug 98336, detach is now a subtree top-down operation. Because we
track visual overflow from our children during layout for most cases, we can
generate a repaint only on the subtree root.

On ​http://dglazkov.github.com/performance-tests/redraw.html, this ups the FPS to
26 fps from 22 fps on my MBP (+ 15%). On PerformanceTests/layout/subtree-detach.html,
it decreases the time by 35%. This is due to being the best case and we now generate
only one repaint per detach phase.

Covered by existing pixels tests (including but not limited to repaint ones).

• rendering/RenderObject.cpp:

(WebCore::RenderObject::destroyAndCleanupAnonymousWrappers):
Changed the function not to be recursive anymore to generate one painting for
our root only. Added a FIXME about using our RenderLayer for repainting to avoid
the cost of computing our absolute repaint rectangle.

• rendering/RenderObjectChildList.cpp:

(WebCore::RenderObjectChildList::removeChildNode):
Removed the logic for repainting in the general case. However we still force a repaint
if we have a RenderLayer as we don't track their overflow in some cases (e.g. positioned
objects). This check is conservative and could be narrowed down (e.g overflow RenderLayers
are properly accounted for in our clipppedOverflowRectForRepaint).

LayoutTests:

• platform/chromium-linux/fast/repaint/selection-after-remove-expected.png:

Rebaselined this test after the change. It is progressing as we still properly repaint
the selection, but repaint less (ie don't repaint the top border which didn't change).

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/chromium-linux/fast/repaint/selection-after-remove-expected.png (modified) (previous)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderObject.cpp (modified) (1 diff)
• Source/WebCore/rendering/RenderObjectChildList.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2012-10-26  Julien Chaffraix  <jchaffraix@webkit.org>
+
+        Generate less repaint calls during subtree detaching
+        https://bugs.webkit.org/show_bug.cgi?id=99741
+
+        Reviewed by Eric Seidel.
+
+        * platform/chromium-linux/fast/repaint/selection-after-remove-expected.png:
+        Rebaselined this test after the change. It is progressing as we still properly repaint
+        the selection, but repaint less (ie don't repaint the top border which didn't change).
+
 2012-10-26  Kent Tamura  <tkent@chromium.org>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-10-26  Julien Chaffraix  <jchaffraix@webkit.org>
+
+        Generate less repaint calls during subtree detaching
+        https://bugs.webkit.org/show_bug.cgi?id=99741
+
+        Reviewed by Eric Seidel.
+
+        Following bug 98336, detach is now a subtree top-down operation. Because we
+        track visual overflow from our children during layout for most cases, we can
+        generate a repaint only on the subtree root.
+
+        On http://dglazkov.github.com/performance-tests/redraw.html, this ups the FPS to
+        26 fps from 22 fps on my MBP (+ 15%). On PerformanceTests/layout/subtree-detach.html,
+        it decreases the time by 35%. This is due to being the best case and we now generate
+        only one repaint per detach phase.
+
+        Covered by existing pixels tests (including but not limited to repaint ones).
+
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::destroyAndCleanupAnonymousWrappers):
+        Changed the function not to be recursive anymore to generate one painting for
+        our root only. Added a FIXME about using our RenderLayer for repainting to avoid
+        the cost of computing our absolute repaint rectangle.
+
+        * rendering/RenderObjectChildList.cpp:
+        (WebCore::RenderObjectChildList::removeChildNode):
+        Removed the logic for repainting in the general case. However we still force a repaint
+        if we have a RenderLayer as we don't track their overflow in some cases (e.g. positioned
+        objects). This check is conservative and could be narrowed down (e.g overflow RenderLayers
+        are properly accounted for in our clipppedOverflowRectForRepaint).
+
 2012-10-26  Kenichi Ishibashi  <bashi@chromium.org>
 

trunk/Source/WebCore/rendering/RenderObject.cpp

@@ -2451 +2451 @@
 void RenderObject::destroyAndCleanupAnonymousWrappers()
 {
-    RenderObject* parent = this->parent();
-
-    // If the tree is destroyed or our parent is not anonymous, there is no need for a clean-up phase.
-    if (documentBeingDestroyed() || !parent || !parent->isAnonymous()) {
+    // If the tree is destroyed, there is no need for a clean-up phase.
+    if (documentBeingDestroyed()) {
         destroy();
         return;
     }
 
-    bool parentIsLeftOverAnonymousWrapper = false;
-
-    // Currently we only remove anonymous cells' wrapper but we should remove all unneeded
-    // wrappers. See http://webkit.org/b/52123 as an example where this is needed.
-    if (parent->isTableCell())
-        parentIsLeftOverAnonymousWrapper = parent->firstChild() == this && parent->lastChild() == this;
-
-    destroy();
+    RenderObject* destroyRoot = this;
+    for (RenderObject* destroyRootParent = destroyRoot->parent(); destroyRootParent && destroyRootParent->isAnonymous(); destroyRoot = destroyRootParent, destroyRootParent = destroyRootParent->parent()) {
+        // Currently we only remove anonymous cells' wrapper but we should remove all unneeded
+        // wrappers. See http://webkit.org/b/52123 as an example where this is needed.
+        if (!destroyRootParent->isTableCell())
+            break;
+
+        if (destroyRootParent->firstChild() != this || destroyRootParent->lastChild() != this)
+            break;
+    }
+
+    // We repaint, so that the area exposed when this object disappears gets repainted properly.
+    // FIXME: A RenderObject with RenderLayer should probably repaint through it as getting the
+    // repaint rects is O(1) through a RenderLayer (assuming it's up-to-date).
+    if (destroyRoot->everHadLayout()) {
+        if (destroyRoot->isBody())
+            destroyRoot->view()->repaint();
+        else
+            destroyRoot->repaint();
+    }
+
+    destroyRoot->destroy();
 
     // WARNING: |this| is deleted here.
-
-    if (parentIsLeftOverAnonymousWrapper) {
-        ASSERT(!parent->firstChild());
-        parent->destroyAndCleanupAnonymousWrappers();
-    }
 }
 

trunk/Source/WebCore/rendering/RenderObjectChildList.cpp

@@ -66 +66 @@
 
     // So that we'll get the appropriate dirty bit set (either that a normal flow child got yanked or
-    // that a positioned child got yanked).  We also repaint, so that the area exposed when the child
-    // disappears gets repainted properly.
+    // that a positioned child got yanked).
     if (!owner->documentBeingDestroyed() && notifyRenderer && oldChild->everHadLayout()) {
         oldChild->setNeedsLayoutAndPrefWidthsRecalc();
-        if (oldChild->isBody())
-            owner->view()->repaint();
-        else
+        // We only repaint |oldChild| if we have a RenderLayer as its visual overflow may not be tracked by its parent.
+        if (oldChild->hasLayer())
             oldChild->repaint();
     }