[webkit-reviews] review canceled: [Bug 73349] compareBorders() is called too often during painting : [Attachment 121552] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Robert Hogan
<robert at webkit.org>'s request for review:
Bug 73349: compareBorders() is called too often during painting
https://bugs.webkit.org/show_bug.cgi?id=73349

Attachment 121552: Patch
https://bugs.webkit.org/attachment.cgi?id=121552&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=121552&action=review


I am fine with the new patch. I would love to see the potential performance
degradation measured or at least investigated before giving the final r+ and
added to the ChangeLog (also the performance benefit would be nice to give
somewhere).

> Source/WebCore/rendering/RenderTableCell.cpp:401
> +    if (includeColor)
> +	   section()->setCollapsedBorder(this, CBSStart, result);

Adding a table lookup here could slow down some operations. Have you checked
that it does not make us regress some of the already optimized code paths? (see
https://bugs.webkit.org/show_bug.cgi?id=74813 for an example)

> Source/WebCore/rendering/RenderTableCell.cpp:716
> +inline CollapsedBorderValue
RenderTableCell::collapsedLeftBorder(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() ?
collapsedStartBorder(IncludeBorderColor, UseCachedBorderValue) :
collapsedEndBorder(IncludeBorderColor, UseCachedBorderValue);
> +    return tableStyle->isFlippedBlocksWritingMode() ?
collapsedAfterBorder(IncludeBorderColor, UseCachedBorderValue) :
collapsedBeforeBorder(IncludeBorderColor, UseCachedBorderValue);
>  }

You could inline your cache checks here (section()->cachedCollapsedBorder(this,
...);)	and remove a branch from the common case. I don't mind keeping the
argument in collapsed*Border as this could enable more call-sites to use it.

> Source/WebCore/rendering/RenderTableSection.cpp:1345
> +    for (int side = CBSBefore; side <= CBSEnd; ++side)

s/int/CollapsedBorderSide/ ?