Optimize collapsed border painting

third_party/WebKit/Source/core/layout/LayoutTableCell.cpp

 /*
  * Copyright (C) 1997 Martin Jones (mjones@kde.org)
  *           (C) 1997 Torben Weis (weis@kde.org)
  *           (C) 1998 Waldo Bastian (bastian@kde.org)
  *           (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 25 matching lines @@
 #include "platform/geometry/FloatQuad.h"
 #include "platform/geometry/TransformState.h"
 
 namespace blink {
 
 using namespace HTMLNames;
 
 struct SameSizeAsLayoutTableCell : public LayoutBlockFlow {
     unsigned bitfields;
     int paddings[2];
+    void* pointer;
 };
 
 static_assert(sizeof(LayoutTableCell) == sizeof(SameSizeAsLayoutTableCell), "LayoutTableCell should stay small");
 static_assert(sizeof(CollapsedBorderValue) == 8, "CollapsedBorderValue should stay small");
 
 LayoutTableCell::LayoutTableCell(Element* element)
     : LayoutBlockFlow(element)
     , m_column(unsetColumnIndex)
     , m_cellWidthChanged(false)
     , 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();
 }
 
 void LayoutTableCell::willBeRemovedFromTree()
 {
     LayoutBlockFlow::willBeRemovedFromTree();
 
     section()->setNeedsCellRecalc();
-    section()->removeCachedCollapsedBorders(this);
 }
 
 unsigned LayoutTableCell::parseColSpanFromDOM() const
 {
     ASSERT(node());
     if (isHTMLTableCellElement(*node()))
         return std::min<unsigned>(toHTMLTableCellElement(*node()).colSpan(), maxColumnIndex);
     return 1;
 }
 
@@ skipping 822 matching lines @@
 }
 
 void LayoutTableCell::paint(const PaintInfo& paintInfo, const LayoutPoint& paintOffset) const
 {
     TableCellPainter(*this).paint(paintInfo, paintOffset);
 }
 
 static void addBorderStyle(LayoutTable::CollapsedBorderValues& borderValues,
     CollapsedBorderValue borderValue)
 {
-    if (!borderValue.exists())
+    if (!borderValue.isVisible())
         return;
     size_t count = borderValues.size();
     for (size_t i = 0; i < count; ++i) {
         if (borderValues[i].isSameIgnoringColor(borderValue))
             return;
     }
     borderValues.append(borderValue);
 }
 
 void LayoutTableCell::collectBorderValues(LayoutTable::CollapsedBorderValues& borderValues)
 {
-    CollapsedBorderValue startBorder = computeCollapsedStartBorder();
-    CollapsedBorderValue endBorder = computeCollapsedEndBorder();
-    CollapsedBorderValue beforeBorder = computeCollapsedBeforeBorder();
-    CollapsedBorderValue afterBorder = computeCollapsedAfterBorder();
-    LayoutTableSection* section = this->section();
-    bool changed = section->setCachedCollapsedBorder(this, CBSStart, startBorder);
-    changed |= section->setCachedCollapsedBorder(this, CBSEnd, endBorder);
-    changed |= section->setCachedCollapsedBorder(this, CBSBefore, beforeBorder);
-    changed |= section->setCachedCollapsedBorder(this, CBSAfter, afterBorder);
+    LayoutTableCell::CollapsedBorderValues newValues = {
+        computeCollapsedStartBorder(),
+        computeCollapsedEndBorder(),
+        computeCollapsedBeforeBorder(),
+        computeCollapsedAfterBorder()
+    };
 
-    // In slimming paint mode, we need to invalidate all cells with collapsed border changed.
-    // FIXME: Need a way to invalidate/repaint the borders only. crbug.com/451090#c5.
+    bool changed;
+    if (!newValues.startBorder.isVisible() && !newValues.endBorder.isVisible() && !newValues.beforeBorder.isVisible() && !newValues.afterBorder.isVisible()) {

[Julien - ping for review, 2016/01/08 14:18:15]

How frequent is this case? It seems like a corner-case optimization that adds
more complexity than it saves memory.

[Xianzhu, 2016/01/08 17:50:45]

This optimization does improve performance of
PerformanceTest/Paint/large-table-background-change-with-invisible-collapsed-borders.html
(previously
PerformanceTest/Layout/large-table-background-change-with-collapsed-borders.html)
from 91ms to 42ms :)
(http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-12...)
by eliminating the whole loop and treewalks for collapsed borders in
TablePainter::paintObject().

I think the case is reasonable in pages that set all tables to collapse borders
but don't specify borders for all tables.

I can stat with top 10k pages.

[Julien - ping for review, 2016/01/11 18:11:36]

This really should have a comment about what it's targeting and include the
great statistics:

"- 11.72% LayoutTableCells have invisible collapsed borders: optimization of
invisible collapsed borders is necessary;"

Also having a helper function for the 4 checks would be nice as it would clarify
what the code is testing.

+        changed = m_collapsedBorderValues;
+        m_collapsedBorderValues = nullptr;
+    } else if (!m_collapsedBorderValues) {
+        changed = true;

[Julien - ping for review, 2016/01/08 14:18:15]

Do we really need that for the first this code runs? If it is needed, we should
put a comment as to *why*.

[Xianzhu, 2016/01/08 17:50:45]

Sorry I'm not sure, do you mean I should comment on the purpose of the 'changed'
variable?

[Julien - ping for review, 2016/01/11 18:11:36]

m_collapsedBorderValue will be nullptr the first time we run this code as this
is run for every paint. I was assuming this wouldn't be needed in this case but
it should be required in a subsequent paint as we could get out of the
optimization above.

+        m_collapsedBorderValues = adoptPtr(new CollapsedBorderValues(newValues));
+    } else {
+        changed = !m_collapsedBorderValues->startBorder.isEquivalentForPainting(newValues.startBorder)
+            || !m_collapsedBorderValues->endBorder.isEquivalentForPainting(newValues.endBorder)
+            || !m_collapsedBorderValues->beforeBorder.isEquivalentForPainting(newValues.beforeBorder)
+            || !m_collapsedBorderValues->afterBorder.isEquivalentForPainting(newValues.afterBorder);
+        if (changed)
+            *m_collapsedBorderValues = newValues;
+    }
+
     if (changed)
         table()->invalidateDisplayItemClient(*this);
 
-    addBorderStyle(borderValues, startBorder);
-    addBorderStyle(borderValues, endBorder);
-    addBorderStyle(borderValues, beforeBorder);
-    addBorderStyle(borderValues, afterBorder);
+    addBorderStyle(borderValues, newValues.startBorder);
+    addBorderStyle(borderValues, newValues.endBorder);
+    addBorderStyle(borderValues, newValues.beforeBorder);
+    addBorderStyle(borderValues, newValues.afterBorder);
 }
 
 static int compareBorderValuesForQSort(const void* pa, const void* pb)
 {
     const CollapsedBorderValue* a = static_cast<const CollapsedBorderValue*>(pa);
     const CollapsedBorderValue* b = static_cast<const CollapsedBorderValue*>(pb);
     if (a->isSameIgnoringColor(*b))
         return 0;
     return compareBorders(*a, *b);
 }
@@ skipping 53 matching lines @@
 
 LayoutTableCell* LayoutTableCell::createAnonymousWithParent(const LayoutObject* parent)
 {
     LayoutTableCell* newCell = LayoutTableCell::createAnonymous(&parent->document());
     RefPtr<ComputedStyle> newStyle = ComputedStyle::createAnonymousStyleWithDisplay(parent->styleRef(), TABLE_CELL);
     newCell->setStyle(newStyle.release());
     return newCell;
 }
 
 } // namespace blink