Changeset 80330 in webkit

Timestamp:

Mar 3, 2011 11:03:55 PM (13 years ago)

Author:

eric@webkit.org

Message:

2011-03-03 Eric Seidel <​eric@webkit.org>

Reviewed by Dimitri Glazkov.

Refactor createRendererIfNeeded to avoid premature nextRenderer calculation
​https://bugs.webkit.org/show_bug.cgi?id=55720

There are two thing going on here:

1. Delaying nextRenderer calculation until we actually use it, previously we would always compute nextRenderer (expensive!) even if no renderer insertion was to occur.
2. Fix fullscreen elements to be inserted into the right place in the rendering tree. Previously they would always be the last child in their parent's list, even if that wasn't the right place.

I don't know of any way to trigger the fullscreen bug,
but I tested this with peacekeeper and saw no performance change.

Peacekeeper's domJQueryBasics is now possibly as much as 2% faster
but I don't really trust the stability of peacekeeper to begin with.

This paves the way for further improvement in our nextRenderer calculation.

• dom/Node.cpp: (WebCore::Node::attach): (WebCore::Node::previousRenderer): (WebCore::Node::nextRenderer): (WebCore::Node::createRendererAndStyle): (WebCore::wrapWithRenderFullScreen): (WebCore::Node::createRendererIfNeeded):
• dom/Node.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/Node.cpp (modified) (5 diffs)
• dom/Node.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-03-03  Eric Seidel  <eric@webkit.org>
+
+        Reviewed by Dimitri Glazkov.
+
+        Refactor createRendererIfNeeded to avoid premature nextRenderer calculation
+        https://bugs.webkit.org/show_bug.cgi?id=55720
+
+        There are two thing going on here:
+        1. Delaying nextRenderer calculation until we actually use it,
+           previously we would always compute nextRenderer (expensive!)
+           even if no renderer insertion was to occur.
+        2. Fix fullscreen elements to be inserted into the right place
+           in the rendering tree.  Previously they would always be the last
+           child in their parent's list, even if that wasn't the right place.
+
+        I don't know of any way to trigger the fullscreen bug,
+        but I tested this with peacekeeper and saw no performance change.
+
+        Peacekeeper's domJQueryBasics is now possibly as much as 2% faster
+        but I don't really trust the stability of peacekeeper to begin with.
+
+        This paves the way for further improvement in our nextRenderer calculation.
+
+        * dom/Node.cpp:
+        (WebCore::Node::attach):
+        (WebCore::Node::previousRenderer):
+        (WebCore::Node::nextRenderer):
+        (WebCore::Node::createRendererAndStyle):
+        (WebCore::wrapWithRenderFullScreen):
+        (WebCore::Node::createRendererIfNeeded):
+        * dom/Node.h:
+
 2011-03-03  Ryosuke Niwa  <rniwa@webkit.org>
 

trunk/Source/WebCore/dom/Node.cpp

@@ -1243 +1243 @@
     ASSERT(!renderer() || (renderer()->style() && renderer()->parent()));
 
+    // FIXME: This is O(N^2) for the innerHTML case, where all children are replaced at once (and not attached).
     // If this node got a renderer it may be the previousRenderer() of sibling text nodes and thus affect the
     // result of Text::rendererIsNeeded() for those nodes.
@@ -1286 +1287 @@
 }
 
-RenderObject * Node::previousRenderer()
-{
-    for (Node *n = previousSibling(); n; n = n->previousSibling()) {
+RenderObject* Node::previousRenderer()
+{
+    // FIXME: We should have the same O(N^2) avoidance as nextRenderer does
+    // however, when I tried adding it, several tests failed.
+    for (Node* n = previousSibling(); n; n = n->previousSibling()) {
         if (n->renderer())
             return n->renderer();
@@ -1295 +1298 @@
 }
 
-RenderObject * Node::nextRenderer()
-{
-    // Avoid an O(n^2) problem with this function by not checking for nextRenderer() when the parent element hasn't even 
-    // been attached yet.
+RenderObject* Node::nextRenderer()
+{
+    // Avoid an O(n^2) problem with this function by not checking for
+    // nextRenderer() when the parent element hasn't attached yet.
     if (parentOrHostNode() && !parentOrHostNode()->attached())
         return 0;
 
-    for (Node *n = nextSibling(); n; n = n->nextSibling()) {
+    for (Node* n = nextSibling(); n; n = n->nextSibling()) {
         if (n->renderer())
             return n->renderer();
@@ -1362 +1365 @@
 }
 
+RenderObject* Node::createRendererAndStyle()
+{
+    ASSERT(!renderer());
+    ASSERT(document()->shouldCreateRenderers());
+
+    ContainerNode* parent = parentOrHostNode();
+    ASSERT(parent);
+    RenderObject* parentRenderer = parent->renderer();
+
+    // FIXME: Ignoring canHaveChildren() in a case of isShadowRoot() might be wrong.
+    // See https://bugs.webkit.org/show_bug.cgi?id=52423
+    if (!parentRenderer || (!parentRenderer->canHaveChildren() && !isShadowRoot()) || !parent->childShouldCreateRenderer(this))
+        return 0;
+
+    RefPtr<RenderStyle> style = styleForRenderer();
+    if (!rendererIsNeeded(style.get()))
+        return 0;
+
+    RenderObject* newRenderer = createRenderer(document()->renderArena(), style.get());
+    if (!newRenderer)
+        return 0;
+
+    if (!parentRenderer->isChildAllowed(newRenderer, style.get())) {
+        newRenderer->destroy();
+        return 0;
+    }
+    setRenderer(newRenderer);
+    newRenderer->setAnimatableStyle(style.release()); // setAnimatableStyle() can depend on renderer() already being set.
+    return newRenderer;
+}
+
+#if ENABLE(FULLSCREEN_API)
+static RenderFullScreen* wrapWithRenderFullScreen(RenderObject* object, Document* document)
+{
+    RenderFullScreen* fullscreenRenderer = new (document->renderArena()) RenderFullScreen(document);
+    fullscreenRenderer->setStyle(RenderFullScreen::createFullScreenStyle());
+    // It's possible that we failed to create the new render and end up wrapping nothing.
+    // We'll end up displaying a black screen, but Jer says this is expected.
+    if (object)
+        fullscreenRenderer->addChild(object);
+    document->setFullScreenRenderer(fullscreenRenderer);
+    return fullscreenRenderer;
+}
+#endif
+
 void Node::createRendererIfNeeded()
 {
@@ -1368 +1416 @@
 
     ASSERT(!renderer());
-    
-    ContainerNode* parent = parentOrHostNode();
-    ASSERT(parent);
-    
-    RenderObject* parentRenderer = parent->renderer();
-    RenderObject* nextRenderer = this->nextRenderer();
-    
+
+    RenderObject* newRenderer = createRendererAndStyle();
+
 #if ENABLE(FULLSCREEN_API)
-    // If this node is a fullscreen node, create a new anonymous full screen
-    // renderer.
-    if (document()->webkitIsFullScreen() && document()->webkitCurrentFullScreenElement() == this) {
-        RenderFullScreen* fullscreenRenderer = new (document()->renderArena()) RenderFullScreen(document());
-        fullscreenRenderer->setStyle(RenderFullScreen::createFullScreenStyle());
-        parentRenderer->addChild(fullscreenRenderer, 0);
-        parentRenderer = fullscreenRenderer;
-        nextRenderer = 0;
-        document()->setFullScreenRenderer(fullscreenRenderer);
-    }
+    if (document()->webkitIsFullScreen() && document()->webkitCurrentFullScreenElement() == this)
+        newRenderer = wrapWithRenderFullScreen(newRenderer, document());
 #endif
 
-    // FIXME: Ignoreing canHaveChildren() in a case of isShadowRoot() might be wrong.
-    // See https://bugs.webkit.org/show_bug.cgi?id=52423
-    if (parentRenderer && (parentRenderer->canHaveChildren() || isShadowRoot()) && parent->childShouldCreateRenderer(this)) {
-        RefPtr<RenderStyle> style = styleForRenderer();
-        if (rendererIsNeeded(style.get())) {
-            if (RenderObject* r = createRenderer(document()->renderArena(), style.get())) {
-                if (!parentRenderer->isChildAllowed(r, style.get()))
-                    r->destroy();
-                else {
-                    setRenderer(r);
-                    renderer()->setAnimatableStyle(style.release());
-                    parentRenderer->addChild(renderer(), nextRenderer);
-                }
-            }
-        }
-    }
+    if (!newRenderer)
+        return;
+
+    // Note: Adding newRenderer instead of renderer(). renderer() may be a child of newRenderer.
+    parentOrHostNode()->renderer()->addChild(newRenderer, nextRenderer());
 }
 

trunk/Source/WebCore/dom/Node.h

@@ -660 +660 @@
     void markAncestorsWithChildNeedsStyleRecalc();
 
+    RenderObject* createRendererAndStyle();
+
     virtual void refEventTarget();
     virtual void derefEventTarget();