compareBorders() is called too often during painting

https://bugs.webkit.org/show_bug.cgi?id=73349

Reviewed by Julien Chaffraix.

Collapsed borders are re-calculated every time they are painted.
This is unnecessary, they only change with the layout or style so calculate and
cache them whenever the layout or style changes and use the cached values when
painting. The cache is stored in the table section so that the memory footprint
of a table is only increased when it has collapsing borders.

The gain in performance here consists of skipping collapsed border computation
during painting. The only path that incurs a small performance penalty is the
additional get/set on the cache when a collapsed border is computed. This penalty only applies
during style change, layout, or when the width of the table is calculated. The computed
border value is not stored in the cache when it has been calculated without its color
for borderHalfStart and co., so that code path is unaffected by this change.

No new tests, covered by existing collapsed border tests.

* rendering/RenderTableCell.cpp:
(WebCore::RenderTableCell::willBeDestroyed): remove entries from collapsed border cache
(WebCore::RenderTableCell::collapsedStartBorder):
 Compute, and also cache if the full border including color was computed.
(WebCore::RenderTableCell::computeCollapsedStartBorder):
(WebCore::RenderTableCell::collapsedEndBorder): ditto
(WebCore::RenderTableCell::computeCollapsedEndBorder):
(WebCore::RenderTableCell::collapsedBeforeBorder): ditto
(WebCore::RenderTableCell::computeCollapsedBeforeBorder):
(WebCore::RenderTableCell::collapsedAfterBorder): ditto
(WebCore::RenderTableCell::computeCollapsedAfterBorder):
(WebCore::RenderTableCell::cachedCollapsedLeftBorder):
 Inline the function since it is only called in paintCollapsedBorders and remove
 includeColor as a parameter. Move the call to the table's style to the caller rather
 than call it 4 times in a row. Use the cached border values directly.
(WebCore::RenderTableCell::cachedCollapsedRightBorder): ditto
(WebCore::RenderTableCell::cachedCollapsedTopBorder): ditto
(WebCore::RenderTableCell::cachedCollapsedBottomBorder): ditto
(WebCore::RenderTableCell::paintCollapsedBorders):
 This function always uses the collapsed border cache now.
* rendering/RenderTableCell.h:
 Make the collapsed border functions private.
* rendering/RenderTableSection.cpp:
(WebCore::RenderTableSection::removeCachedCollapsedBorders): The cache is deleted by the HashMap
 since it is the only owner of the cached item.
(WebCore::RenderTableSection::setCachedCollapsedBorder):
(WebCore::RenderTableSection::cachedCollapsedBorder):
 This will assert if there is no cached collapsed border for the side of the
 cell requested by the caller.
* rendering/RenderTableSection.h:
 HashMap wouldn't let me use CollapsedBorderSide in the key value for the cache
 so use int instead.

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

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index e7af233..c68217f 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,58 @@
+2011-12-30  Robert Hogan  <robert@webkit.org>
+
+        compareBorders() is called too often during painting
+        https://bugs.webkit.org/show_bug.cgi?id=73349
+
+        Reviewed by Julien Chaffraix.
+
+        Collapsed borders are re-calculated every time they are painted.
+        This is unnecessary, they only change with the layout or style so calculate and
+        cache them whenever the layout or style changes and use the cached values when
+        painting. The cache is stored in the table section so that the memory footprint
+        of a table is only increased when it has collapsing borders.
+
+        The gain in performance here consists of skipping collapsed border computation
+        during painting. The only path that incurs a small performance penalty is the 
+        additional get/set on the cache when a collapsed border is computed. This penalty only applies
+        during style change, layout, or when the width of the table is calculated. The computed
+        border value is not stored in the cache when it has been calculated without its color
+        for borderHalfStart and co., so that code path is unaffected by this change.
+
+        No new tests, covered by existing collapsed border tests.
+
+        * rendering/RenderTableCell.cpp:
+        (WebCore::RenderTableCell::willBeDestroyed): remove entries from collapsed border cache
+        (WebCore::RenderTableCell::collapsedStartBorder):
+         Compute, and also cache if the full border including color was computed. 
+        (WebCore::RenderTableCell::computeCollapsedStartBorder):
+        (WebCore::RenderTableCell::collapsedEndBorder): ditto
+        (WebCore::RenderTableCell::computeCollapsedEndBorder):
+        (WebCore::RenderTableCell::collapsedBeforeBorder): ditto
+        (WebCore::RenderTableCell::computeCollapsedBeforeBorder):
+        (WebCore::RenderTableCell::collapsedAfterBorder): ditto
+        (WebCore::RenderTableCell::computeCollapsedAfterBorder):
+        (WebCore::RenderTableCell::cachedCollapsedLeftBorder):
+         Inline the function since it is only called in paintCollapsedBorders and remove
+         includeColor as a parameter. Move the call to the table's style to the caller rather
+         than call it 4 times in a row. Use the cached border values directly.
+        (WebCore::RenderTableCell::cachedCollapsedRightBorder): ditto
+        (WebCore::RenderTableCell::cachedCollapsedTopBorder): ditto
+        (WebCore::RenderTableCell::cachedCollapsedBottomBorder): ditto
+        (WebCore::RenderTableCell::paintCollapsedBorders):
+         This function always uses the collapsed border cache now.
+        * rendering/RenderTableCell.h:
+         Make the collapsed border functions private.
+        * rendering/RenderTableSection.cpp:
+        (WebCore::RenderTableSection::removeCachedCollapsedBorders): The cache is deleted by the HashMap
+         since it is the only owner of the cached item.
+        (WebCore::RenderTableSection::setCachedCollapsedBorder):
+        (WebCore::RenderTableSection::cachedCollapsedBorder):
+         This will assert if there is no cached collapsed border for the side of the 
+         cell requested by the caller.
+        * rendering/RenderTableSection.h:
+         HashMap wouldn't let me use CollapsedBorderSide in the key value for the cache
+         so use int instead.
+
 2012-01-14  David Levin  <levin@chromium.org>
 
         HWndDC should be in platform/win instead of wtf.

diff --git a/Source/WebCore/rendering/RenderTableCell.cpp b/Source/WebCore/rendering/RenderTableCell.cpp
index f43e5c1..5b690f4 100644 (file)
--- a/Source/WebCore/rendering/RenderTableCell.cpp
+++ b/Source/WebCore/rendering/RenderTableCell.cpp
@@ -58,8 +58,10 @@ void RenderTableCell::willBeDestroyed()
 
     RenderBlock::willBeDestroyed();
 
-    if (recalcSection)
+    if (recalcSection) {
         recalcSection->setNeedsCellRecalc();
+        recalcSection->removeCachedCollapsedBorders(this);
+    }
 }
 
 unsigned RenderTableCell::colSpan() const
@@ -392,6 +394,14 @@ static CollapsedBorderValue chooseBorder(const CollapsedBorderValue& border1, co
 
 CollapsedBorderValue RenderTableCell::collapsedStartBorder(IncludeBorderColorOrNot includeColor) const
 {
+    CollapsedBorderValue result = computeCollapsedStartBorder(includeColor);
+    if (includeColor)
+        section()->setCachedCollapsedBorder(this, CBSStart, result);
+    return result;
+}
+
+CollapsedBorderValue RenderTableCell::computeCollapsedStartBorder(IncludeBorderColorOrNot includeColor) const
+{
     RenderTable* table = this->table();
     bool isStartColumn = col() == 0;
 
@@ -455,6 +465,14 @@ CollapsedBorderValue RenderTableCell::collapsedStartBorder(IncludeBorderColorOrN
 
 CollapsedBorderValue RenderTableCell::collapsedEndBorder(IncludeBorderColorOrNot includeColor) const
 {
+    CollapsedBorderValue result = computeCollapsedEndBorder(includeColor);
+    if (includeColor)
+        section()->setCachedCollapsedBorder(this, CBSEnd, result);
+    return result;
+}
+
+CollapsedBorderValue RenderTableCell::computeCollapsedEndBorder(IncludeBorderColorOrNot includeColor) const
+{
     RenderTable* table = this->table();
     bool isEndColumn = table->colToEffCol(col() + colSpan() - 1) == table->numEffCols() - 1;
 
@@ -521,6 +539,14 @@ CollapsedBorderValue RenderTableCell::collapsedEndBorder(IncludeBorderColorOrNot
 
 CollapsedBorderValue RenderTableCell::collapsedBeforeBorder(IncludeBorderColorOrNot includeColor) const
 {
+    CollapsedBorderValue result = computeCollapsedBeforeBorder(includeColor);
+    if (includeColor)
+        section()->setCachedCollapsedBorder(this, CBSBefore, result);
+    return result;
+}
+
+CollapsedBorderValue RenderTableCell::computeCollapsedBeforeBorder(IncludeBorderColorOrNot includeColor) const
+{
     RenderTable* table = this->table();
 
     // For before border, we need to check, in order of precedence:
@@ -599,6 +625,14 @@ CollapsedBorderValue RenderTableCell::collapsedBeforeBorder(IncludeBorderColorOr
 
 CollapsedBorderValue RenderTableCell::collapsedAfterBorder(IncludeBorderColorOrNot includeColor) const
 {
+    CollapsedBorderValue result = computeCollapsedAfterBorder(includeColor);
+    if (includeColor)
+        section()->setCachedCollapsedBorder(this, CBSAfter, result);
+    return result;
+}
+
+CollapsedBorderValue RenderTableCell::computeCollapsedAfterBorder(IncludeBorderColorOrNot includeColor) const
+{
     RenderTable* table = this->table();
 
     // For after border, we need to check, in order of precedence:
@@ -666,36 +700,32 @@ CollapsedBorderValue RenderTableCell::collapsedAfterBorder(IncludeBorderColorOrN
     return result;    
 }
 
-CollapsedBorderValue RenderTableCell::collapsedLeftBorder(IncludeBorderColorOrNot includeColor) const
+inline CollapsedBorderValue RenderTableCell::cachedCollapsedLeftBorder(RenderStyle* tableStyle) const
 {
-    RenderStyle* tableStyle = table()->style();
     if (tableStyle->isHorizontalWritingMode())
-        return tableStyle->isLeftToRightDirection() ? collapsedStartBorder(includeColor) : collapsedEndBorder(includeColor);
-    return tableStyle->isFlippedBlocksWritingMode() ? collapsedAfterBorder(includeColor) : collapsedBeforeBorder(includeColor);
+        return tableStyle->isLeftToRightDirection() ? section()->cachedCollapsedBorder(this, CBSStart) : section()->cachedCollapsedBorder(this, CBSEnd);
+    return tableStyle->isFlippedBlocksWritingMode() ? section()->cachedCollapsedBorder(this, CBSAfter) : section()->cachedCollapsedBorder(this, CBSBefore);
 }
 
-CollapsedBorderValue RenderTableCell::collapsedRightBorder(IncludeBorderColorOrNot includeColor) const
+inline CollapsedBorderValue RenderTableCell::cachedCollapsedRightBorder(RenderStyle* tableStyle) const
 {
-    RenderStyle* tableStyle = table()->style();
     if (tableStyle->isHorizontalWritingMode())
-        return tableStyle->isLeftToRightDirection() ? collapsedEndBorder(includeColor) : collapsedStartBorder(includeColor);
-    return tableStyle->isFlippedBlocksWritingMode() ? collapsedBeforeBorder(includeColor) : collapsedAfterBorder(includeColor);
+        return tableStyle->isLeftToRightDirection() ? section()->cachedCollapsedBorder(this, CBSEnd) : section()->cachedCollapsedBorder(this, CBSStart);
+    return tableStyle->isFlippedBlocksWritingMode() ? section()->cachedCollapsedBorder(this, CBSBefore) : section()->cachedCollapsedBorder(this, CBSAfter);
 }
 
-CollapsedBorderValue RenderTableCell::collapsedTopBorder(IncludeBorderColorOrNot includeColor) const
+inline CollapsedBorderValue RenderTableCell::cachedCollapsedTopBorder(RenderStyle* tableStyle) const
 {
-    RenderStyle* tableStyle = table()->style();
     if (tableStyle->isHorizontalWritingMode())
-        return tableStyle->isFlippedBlocksWritingMode() ? collapsedAfterBorder(includeColor) : collapsedBeforeBorder(includeColor);
-    return tableStyle->isLeftToRightDirection() ? collapsedStartBorder(includeColor) : collapsedEndBorder(includeColor);
+        return tableStyle->isFlippedBlocksWritingMode() ? section()->cachedCollapsedBorder(this, CBSAfter) : section()->cachedCollapsedBorder(this, CBSBefore);
+    return tableStyle->isLeftToRightDirection() ? section()->cachedCollapsedBorder(this, CBSStart) : section()->cachedCollapsedBorder(this, CBSEnd);
 }
 
-CollapsedBorderValue RenderTableCell::collapsedBottomBorder(IncludeBorderColorOrNot includeColor) const
+inline CollapsedBorderValue RenderTableCell::cachedCollapsedBottomBorder(RenderStyle* tableStyle) const
 {
-    RenderStyle* tableStyle = table()->style();
     if (tableStyle->isHorizontalWritingMode())
-        return tableStyle->isFlippedBlocksWritingMode() ? collapsedBeforeBorder(includeColor) : collapsedAfterBorder(includeColor);
-    return tableStyle->isLeftToRightDirection() ? collapsedEndBorder(includeColor) : collapsedStartBorder(includeColor);
+        return tableStyle->isFlippedBlocksWritingMode() ? section()->cachedCollapsedBorder(this, CBSBefore) : section()->cachedCollapsedBorder(this, CBSAfter);
+    return tableStyle->isLeftToRightDirection() ? section()->cachedCollapsedBorder(this, CBSEnd) : section()->cachedCollapsedBorder(this, CBSStart);
 }
 
 LayoutUnit RenderTableCell::borderLeft() const
@@ -923,10 +953,11 @@ void RenderTableCell::paintCollapsedBorders(PaintInfo& paintInfo, const LayoutPo
 
     LayoutRect paintRect = LayoutRect(adjustedPaintOffset, size());
 
-    CollapsedBorderValue leftVal = collapsedLeftBorder();
-    CollapsedBorderValue rightVal = collapsedRightBorder();
-    CollapsedBorderValue topVal = collapsedTopBorder();
-    CollapsedBorderValue bottomVal = collapsedBottomBorder();
+    RenderStyle* tableStyle = table()->style();
+    CollapsedBorderValue leftVal = cachedCollapsedLeftBorder(tableStyle);
+    CollapsedBorderValue rightVal = cachedCollapsedRightBorder(tableStyle);
+    CollapsedBorderValue topVal = cachedCollapsedTopBorder(tableStyle);
+    CollapsedBorderValue bottomVal = cachedCollapsedBottomBorder(tableStyle);
      
     // Adjust our x/y/width/height so that we paint the collapsed borders at the correct location.
     LayoutUnit topWidth = topVal.width();

diff --git a/Source/WebCore/rendering/RenderTableCell.h b/Source/WebCore/rendering/RenderTableCell.h
index 351ea50..2548b47 100644 (file)
--- a/Source/WebCore/rendering/RenderTableCell.h
+++ b/Source/WebCore/rendering/RenderTableCell.h
@@ -98,26 +98,6 @@ public:
     virtual LayoutUnit borderBefore() const;
     virtual LayoutUnit borderAfter() const;
 
-    LayoutUnit borderHalfLeft(bool outer) const;
-    LayoutUnit borderHalfRight(bool outer) const;
-    LayoutUnit borderHalfTop(bool outer) const;
-    LayoutUnit borderHalfBottom(bool outer) const;
-
-    LayoutUnit borderHalfStart(bool outer) const;
-    LayoutUnit borderHalfEnd(bool outer) const;
-    LayoutUnit borderHalfBefore(bool outer) const;
-    LayoutUnit borderHalfAfter(bool outer) const;
-
-    CollapsedBorderValue collapsedStartBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
-    CollapsedBorderValue collapsedEndBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
-    CollapsedBorderValue collapsedBeforeBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
-    CollapsedBorderValue collapsedAfterBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
-
-    CollapsedBorderValue collapsedLeftBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
-    CollapsedBorderValue collapsedRightBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
-    CollapsedBorderValue collapsedTopBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
-    CollapsedBorderValue collapsedBottomBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
-
     void collectBorderValues(RenderTable::CollapsedBorderValues&) const;
     static void sortBorderValues(RenderTable::CollapsedBorderValues&);
 
@@ -175,6 +155,31 @@ private:
     virtual LayoutRect clippedOverflowRectForRepaint(RenderBoxModelObject* repaintContainer) const;
     virtual void computeRectForRepaint(RenderBoxModelObject* repaintContainer, LayoutRect&, bool fixed = false) const;
 
+    LayoutUnit borderHalfLeft(bool outer) const;
+    LayoutUnit borderHalfRight(bool outer) const;
+    LayoutUnit borderHalfTop(bool outer) const;
+    LayoutUnit borderHalfBottom(bool outer) const;
+
+    LayoutUnit borderHalfStart(bool outer) const;
+    LayoutUnit borderHalfEnd(bool outer) const;
+    LayoutUnit borderHalfBefore(bool outer) const;
+    LayoutUnit borderHalfAfter(bool outer) const;
+
+    CollapsedBorderValue collapsedStartBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
+    CollapsedBorderValue collapsedEndBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
+    CollapsedBorderValue collapsedBeforeBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
+    CollapsedBorderValue collapsedAfterBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
+
+    CollapsedBorderValue cachedCollapsedLeftBorder(RenderStyle*) const;
+    CollapsedBorderValue cachedCollapsedRightBorder(RenderStyle*) const;
+    CollapsedBorderValue cachedCollapsedTopBorder(RenderStyle*) const;
+    CollapsedBorderValue cachedCollapsedBottomBorder(RenderStyle*) const;
+
+    CollapsedBorderValue computeCollapsedStartBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
+    CollapsedBorderValue computeCollapsedEndBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
+    CollapsedBorderValue computeCollapsedBeforeBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
+    CollapsedBorderValue computeCollapsedAfterBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
+
     unsigned m_row : 31;
     bool m_cellWidthChanged : 1;
     unsigned m_column : 31;

diff --git a/Source/WebCore/rendering/RenderTableSection.cpp b/Source/WebCore/rendering/RenderTableSection.cpp
index dc48a28..0077d5b 100644 (file)
--- a/Source/WebCore/rendering/RenderTableSection.cpp
+++ b/Source/WebCore/rendering/RenderTableSection.cpp
@@ -1349,4 +1349,27 @@ unsigned RenderTableSection::rowIndexForRenderer(const RenderTableRow* row) cons
     return 0;
 }
 
+void RenderTableSection::removeCachedCollapsedBorders(const RenderTableCell* cell)
+{
+    if (!table()->collapseBorders())
+        return;
+    
+    for (int side = CBSBefore; side <= CBSEnd; ++side)
+        m_cellsCollapsedBorders.remove(make_pair(cell, side));
+}
+
+void RenderTableSection::setCachedCollapsedBorder(const RenderTableCell* cell, CollapsedBorderSide side, CollapsedBorderValue border)
+{
+    ASSERT(table()->collapseBorders());
+    m_cellsCollapsedBorders.set(make_pair(cell, side), border);
+}
+
+CollapsedBorderValue& RenderTableSection::cachedCollapsedBorder(const RenderTableCell* cell, CollapsedBorderSide side)
+{
+    ASSERT(table()->collapseBorders());
+    HashMap<pair<const RenderTableCell*, int>, CollapsedBorderValue>::iterator it = m_cellsCollapsedBorders.find(make_pair(cell, side));
+    ASSERT(it != m_cellsCollapsedBorders.end());
+    return it->second;
+}
+
 } // namespace WebCore

diff --git a/Source/WebCore/rendering/RenderTableSection.h b/Source/WebCore/rendering/RenderTableSection.h
index 86f3434..7517a0a 100644 (file)
--- a/Source/WebCore/rendering/RenderTableSection.h
+++ b/Source/WebCore/rendering/RenderTableSection.h
@@ -30,6 +30,13 @@
 
 namespace WebCore {
 
+enum CollapsedBorderSide {
+    CBSBefore,
+    CBSAfter,
+    CBSStart,
+    CBSEnd
+};
+
 class RenderTableCell;
 class RenderTableRow;
 
@@ -130,6 +137,10 @@ public:
 
     unsigned rowIndexForRenderer(const RenderTableRow*) const;
 
+    void removeCachedCollapsedBorders(const RenderTableCell*);
+    void setCachedCollapsedBorder(const RenderTableCell*, CollapsedBorderSide, CollapsedBorderValue);
+    CollapsedBorderValue& cachedCollapsedBorder(const RenderTableCell*, CollapsedBorderSide);
+
 protected:
     virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle);
 
@@ -182,6 +193,10 @@ private:
     bool m_forceSlowPaintPathWithOverflowingCell;
 
     bool m_hasMultipleCellLevels;
+
+    // This map holds the collapsed border values for cells with collapsed borders.
+    // It is held at RenderTableSection level to spare memory consumption by table cells.
+    HashMap<pair<const RenderTableCell*, int>, CollapsedBorderValue > m_cellsCollapsedBorders;
 };
 
 inline RenderTableSection* toRenderTableSection(RenderObject* object)