Changeset 130548 in webkit

Timestamp:

Oct 5, 2012 1:47:16 PM (12 years ago)

Author:

eric@webkit.org

Message:

Make tables which don't use col/row span much faster to layout
​https://bugs.webkit.org/show_bug.cgi?id=98221

Reviewed by Julien Chaffraix.

My sense is that most tables on webpages do not use colspan/rowspan
so I stole another bit from RenderTableCell::m_column to avoid
having to re-parse the colSpan/rowSpan attributes for every cell
when doing table layout.
This made these symbols disappear from biggrid.html/redraw.html (dglazkov's spreadsheets benchmarks)
as well as moved our robohornet/resizecol.html number from an average of 3221ms to 2608ms (~20%!).

I removed m_hasHTMLTableCellElement (from ​http://trac.webkit.org/changeset/97691)
since it was attempting to do the same sort of optimization.

• rendering/RenderTableCell.cpp:

(WebCore::RenderTableCell::RenderTableCell):
(WebCore::RenderTableCell::parseColSpanFromDOM):
(WebCore::RenderTableCell::parseRowSpanFromDOM):
(WebCore::RenderTableCell::layout):

• rendering/RenderTableCell.h:

(WebCore::RenderTableCell::colSpan):
(WebCore::RenderTableCell::rowSpan):
(RenderTableCell):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• rendering/RenderTableCell.cpp (modified) (3 diffs)
• rendering/RenderTableCell.h (modified) (3 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-10-05  Eric Seidel  <eric@webkit.org>
+
+        Make tables which don't use col/row span much faster to layout
+        https://bugs.webkit.org/show_bug.cgi?id=98221
+
+        Reviewed by Julien Chaffraix.
+
+        My sense is that most tables on webpages do not use colspan/rowspan
+        so I stole another bit from RenderTableCell::m_column to avoid
+        having to re-parse the colSpan/rowSpan attributes for every cell
+        when doing table layout.
+        This made these symbols disappear from biggrid.html/redraw.html (dglazkov's spreadsheets benchmarks)
+        as well as moved our robohornet/resizecol.html number from an average of 3221ms to 2608ms (~20%!).
+
+        I removed m_hasHTMLTableCellElement (from http://trac.webkit.org/changeset/97691)
+        since it was attempting to do the same sort of optimization.
+
+        * rendering/RenderTableCell.cpp:
+        (WebCore::RenderTableCell::RenderTableCell):
+        (WebCore::RenderTableCell::parseColSpanFromDOM):
+        (WebCore::RenderTableCell::parseRowSpanFromDOM):
+        (WebCore::RenderTableCell::layout):
+        * rendering/RenderTableCell.h:
+        (WebCore::RenderTableCell::colSpan):
+        (WebCore::RenderTableCell::rowSpan):
+        (RenderTableCell):
+
 2012-10-05  Oli Lan  <olilan@chromium.org>
 

trunk/Source/WebCore/rendering/RenderTableCell.cpp

@@ -48 +48 @@
 using namespace HTMLNames;
 
+struct SameSizeAsRenderTableCell : public RenderBlock {
+    unsigned bitfields;
+    int paddings[2];
+};
+
+COMPILE_ASSERT(sizeof(RenderTableCell) == sizeof(SameSizeAsRenderTableCell), RenderTableCell_should_stay_small);
+
+
 RenderTableCell::RenderTableCell(Node* node)
     : RenderBlock(node)
     , m_column(unsetColumnIndex)
     , m_cellWidthChanged(false)
-    , m_hasHTMLTableCellElement(node && (node->hasTagName(tdTag) || node->hasTagName(thTag)))
     , m_intrinsicPaddingBefore(0)
     , m_intrinsicPaddingAfter(0)
 {
+    // We only update the flags when notified of DOM changes in colSpanOrRowSpanChanged()
+    // so we need to set their initial values here in case something asks for colSpan()/rowSpan() before then.
+    updateColAndRowSpanFlags();
 }
 
@@ -66 +76 @@
 }
 
+unsigned RenderTableCell::parseColSpanFromDOM() const
+{
+    ASSERT(node());
+    if (node()->hasTagName(tdTag) || node()->hasTagName(thTag))
+        return toHTMLTableCellElement(node())->colSpan();
 #if ENABLE(MATHML)
-inline bool isMathMLElement(Node* node)
-{
-    return node && node->isElementNode() && toElement(node)->isMathMLElement();
-}
+    if (node()->hasTagName(MathMLNames::mtdTag))
+        return toMathMLElement(node())->colSpan();
 #endif
-
-unsigned RenderTableCell::colSpan() const
-{
-    if (UNLIKELY(!m_hasHTMLTableCellElement)) {
+    return 1;
+}
+
+unsigned RenderTableCell::parseRowSpanFromDOM() const
+{
+    ASSERT(node());
+    if (node()->hasTagName(tdTag) || node()->hasTagName(thTag))
+        return toHTMLTableCellElement(node())->rowSpan();
 #if ENABLE(MATHML)
-        if (isMathMLElement(node()))
-            return toMathMLElement(node())->colSpan();
+    if (node()->hasTagName(MathMLNames::mtdTag))
+        return toMathMLElement(node())->rowSpan();
 #endif
-        return 1;
-    }
-
-    return toHTMLTableCellElement(node())->colSpan();
-}
-
-unsigned RenderTableCell::rowSpan() const
-{
-    if (UNLIKELY(!m_hasHTMLTableCellElement)) {
-#if ENABLE(MATHML)
-        if (isMathMLElement(node()))
-            return toMathMLElement(node())->rowSpan();
-#endif
-        return 1;
-    }
-
-    return toHTMLTableCellElement(node())->rowSpan();
+    return 1;
+}
+
+void RenderTableCell::updateColAndRowSpanFlags()
+{
+    // The vast majority of table cells do not have a colspan or rowspan,
+    // so we keep a bool to know if we need to bother reading from the DOM.
+    m_hasColSpan = node() && parseColSpanFromDOM() != 1;
+    m_hasRowSpan = node() && parseRowSpanFromDOM() != 1;
 }
 
 void RenderTableCell::colSpanOrRowSpanChanged()
 {
-#if ENABLE(MATHML)
-    ASSERT(m_hasHTMLTableCellElement || isMathMLElement(node()));
-#else
-    ASSERT(m_hasHTMLTableCellElement);
-#endif
     ASSERT(node());
 #if ENABLE(MATHML)
@@ -112 +116 @@
     ASSERT(node()->hasTagName(tdTag) || node()->hasTagName(thTag));
 #endif
+
+    updateColAndRowSpanFlags();
+
+    // FIXME: I suspect that we could return early here if !m_hasColSpan && !m_hasRowSpan.
 
     setNeedsLayoutAndPrefWidthsRecalc();

trunk/Source/WebCore/rendering/RenderTableCell.h

@@ -31 +31 @@
 namespace WebCore {
 
-static const unsigned unsetColumnIndex = 0x3FFFFFFF;
-static const unsigned maxColumnIndex = 0x3FFFFFFE; // 1,073,741,823
+static const unsigned unsetColumnIndex = 0x1FFFFFFF;
+static const unsigned maxColumnIndex = 0x1FFFFFFE; // 536,870,910
 
 enum IncludeBorderColorOrNot { DoNotIncludeBorderColor, IncludeBorderColor };
@@ -40 +40 @@
     explicit RenderTableCell(Node*);
     
-    unsigned colSpan() const;
-    unsigned rowSpan() const;
+    unsigned colSpan() const
+    {
+        if (!m_hasColSpan)
+            return 1;
+        return parseColSpanFromDOM();
+    }
+    unsigned rowSpan() const
+    {
+        if (!m_hasRowSpan)
+            return 1;
+        return parseRowSpanFromDOM();
+    }
 
     // Called from HTMLTableCellElement.
@@ -228 +238 @@
     CollapsedBorderValue computeCollapsedAfterBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
 
-    unsigned m_column : 30;
-    bool m_cellWidthChanged : 1;
-    bool m_hasHTMLTableCellElement : 1;
+    void updateColAndRowSpanFlags();
+
+    unsigned parseRowSpanFromDOM() const;
+    unsigned parseColSpanFromDOM() const;
+
+    // Note MSVC will only pack members if they have identical types, hence we use unsigned instead of bool here.
+    unsigned m_column : 29;
+    unsigned m_cellWidthChanged : 1;
+    unsigned m_hasColSpan: 1;
+    unsigned m_hasRowSpan: 1;
     int m_intrinsicPaddingBefore;
     int m_intrinsicPaddingAfter;