REGRESSION (r87351): toggling display of lots (thousands) of elements with display...
authordglazkov@chromium.org <dglazkov@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Sep 2011 03:25:04 +0000 (03:25 +0000)
committerdglazkov@chromium.org <dglazkov@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Sep 2011 03:25:04 +0000 (03:25 +0000)
https://bugs.webkit.org/show_bug.cgi?id=67581

Reviewed by Darin Adler.

Source/WebCore:

Test: perf/show-hide-table-rows.html

* dom/NodeRenderingContext.cpp:
(WebCore::NodeRendererFactory::createRendererAndStyle): Moved style-creating code into createRendererIfNeeded, renamed
    to createRenderer.
(WebCore::NodeRendererFactory::createRendererIfNeeded): Re-arrange code to avoid unnecessary creation of renderers.

LayoutTests:

* perf/show-hide-table-rows-expected.txt: Added.
* perf/show-hide-table-rows.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@94938 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/perf/show-hide-table-rows-expected.txt [new file with mode: 0644]
LayoutTests/perf/show-hide-table-rows.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/NodeRenderingContext.cpp
Source/WebCore/dom/NodeRenderingContext.h

index c65f595..86913aa 100755 (executable)
@@ -1,3 +1,13 @@
+2011-09-11  Dimitri Glazkov  <dglazkov@chromium.org>
+
+        REGRESSION (r87351): toggling display of lots (thousands) of elements with display:none is very slow
+        https://bugs.webkit.org/show_bug.cgi?id=67581
+
+        Reviewed by Darin Adler.
+
+        * perf/show-hide-table-rows-expected.txt: Added.
+        * perf/show-hide-table-rows.html: Added.
+
 2011-09-11  Fumitoshi Ukai  <ukai@chromium.org>
 
         Unreviewed, one more rebaseline for r94912
diff --git a/LayoutTests/perf/show-hide-table-rows-expected.txt b/LayoutTests/perf/show-hide-table-rows-expected.txt
new file mode 100644 (file)
index 0000000..2cce74f
--- /dev/null
@@ -0,0 +1,3 @@
+Tests that hiding/showing of table rows is linear.
+PASS
+
diff --git a/LayoutTests/perf/show-hide-table-rows.html b/LayoutTests/perf/show-hide-table-rows.html
new file mode 100644 (file)
index 0000000..2a0bb96
--- /dev/null
@@ -0,0 +1,39 @@
+<style>
+.hidden { display: none; }
+</style>
+<script src="../resources/magnitude-perf.js"></script>
+<body>
+<div></div>
+<script>
+
+function setupFunction(magnitude)
+{
+    var html = '<table>';
+    for (var i = 0; i < magnitude; ++i)
+        html += '<tr><td>A</td><td>B</td><td>C</td><td>D</td><td>E</td><td>F</td></tr>\n';
+    html += '</table>';
+    document.querySelector('div').innerHTML = html;
+}
+
+function forEachRow(what)
+{
+    Array.prototype.forEach.call(document.querySelectorAll("tr"), what);
+}
+
+function test(magnitude)
+{
+    forEachRow(function(tr) {
+        tr.className = 'hidden';
+    });
+    document.body.offsetWidth;
+    forEachRow(function(tr) {
+        tr.className = '';
+    });
+    document.body.offsetWidth;
+}
+
+Magnitude.description("Tests that hiding/showing of table rows is linear.");
+Magnitude.run(setupFunction, test, Magnitude.LINEAR);
+document.querySelector('div').textContent = '';
+</script>
+</body>
index 07087e9..16b2929 100755 (executable)
@@ -1,3 +1,17 @@
+2011-09-11  Dimitri Glazkov  <dglazkov@chromium.org>
+
+        REGRESSION (r87351): toggling display of lots (thousands) of elements with display:none is very slow
+        https://bugs.webkit.org/show_bug.cgi?id=67581
+
+        Reviewed by Darin Adler.
+
+        Test: perf/show-hide-table-rows.html
+
+        * dom/NodeRenderingContext.cpp:
+        (WebCore::NodeRendererFactory::createRendererAndStyle): Moved style-creating code into createRendererIfNeeded, renamed
+            to createRenderer.
+        (WebCore::NodeRendererFactory::createRendererIfNeeded): Re-arrange code to avoid unnecessary creation of renderers.
+
 2011-09-11  Jeremy Moskovich  <jeremy@chromium.org>
 
         [Chromium] Change OOP Font loading code to use CGFont*() APIs.
index 6266750..f4743e4 100644 (file)
@@ -286,29 +286,10 @@ NodeRendererFactory::NodeRendererFactory(Node* node)
 {
 }
 
-RenderObject* NodeRendererFactory::createRendererAndStyle()
+RenderObject* NodeRendererFactory::createRenderer()
 {
     Node* node = m_context.node();
-    Document* document = node->document();
-    ASSERT(!node->renderer());
-    ASSERT(document->shouldCreateRenderers());
-
-    if (!m_context.shouldCreateRenderer())
-        return 0;
-
-    Element* element = node->isElementNode() ? toElement(node) : 0;
-    if (element)
-        m_context.setStyle(element->styleForRenderer());
-    else if (RenderObject* parentRenderer = m_context.parentRenderer())
-        m_context.setStyle(parentRenderer->style());
-
-    if (!node->rendererIsNeeded(m_context)) {
-        if (element && m_context.style()->affectedByEmpty())
-            element->setStyleAffectedByEmpty();
-        return 0;
-    }
-
-    RenderObject* newRenderer = node->createRenderer(document->renderArena(), m_context.style());
+    RenderObject* newRenderer = node->createRenderer(node->document()->renderArena(), m_context.style());
     if (!newRenderer)
         return 0;
 
@@ -345,25 +326,38 @@ void NodeRendererFactory::createRendererIfNeeded()
     if (!document->shouldCreateRenderers())
         return;
 
-    RenderObject* parentRenderer = m_context.parentRenderer();
-    RenderObject* nextRenderer = m_context.nextRenderer();
-    RenderObject* newRenderer = createRendererAndStyle();
+    ASSERT(!node->renderer());
+    ASSERT(document->shouldCreateRenderers());
 
-    if (m_context.hasFlowThreadParent()) {
-        parentRenderer = m_context.parentFlowRenderer();
-        // Do not call m_context.nextRenderer() here, because it expects to have 
-        // the renderer added to its parent already.
-        nextRenderer = m_context.parentFlowRenderer()->nextRendererForNode(node);
+    // FIXME: This side effect should be visible from attach() code.
+    m_context.hostChildrenChanged();
+
+    if (!m_context.shouldCreateRenderer())
+        return;
+
+    Element* element = node->isElementNode() ? toElement(node) : 0;
+    if (element)
+        m_context.setStyle(element->styleForRenderer());
+    else if (RenderObject* parentRenderer = m_context.parentRenderer())
+        m_context.setStyle(parentRenderer->style());
+
+    if (!node->rendererIsNeeded(m_context)) {
+        if (element && m_context.style()->affectedByEmpty())
+            element->setStyleAffectedByEmpty();
+        return;
     }
 
+    RenderObject* parentRenderer = m_context.hasFlowThreadParent() ? m_context.parentFlowRenderer() : m_context.parentRenderer();
+    // Do not call m_context.nextRenderer() here in the first clause, because it expects to have
+    // the renderer added to its parent already.
+    RenderObject* nextRenderer = m_context.hasFlowThreadParent() ? m_context.parentFlowRenderer()->nextRendererForNode(node) : m_context.nextRenderer();
+    RenderObject* newRenderer = createRenderer();
+
 #if ENABLE(FULLSCREEN_API)
     if (document->webkitIsFullScreen() && document->webkitCurrentFullScreenElement() == node)
         newRenderer = wrapWithRenderFullScreen(newRenderer, document);
 #endif
 
-    // FIXME: This side effect should be visible from attach() code.
-    m_context.hostChildrenChanged();
-
     if (!newRenderer)
         return;
 
index ac6f2e3..b4e8912 100644 (file)
@@ -123,7 +123,7 @@ public:
     void createRendererIfNeeded();
 
 private:
-    RenderObject* createRendererAndStyle();
+    RenderObject* createRenderer();
 
     NodeRenderingContext m_context;
 };