Let collapsed border drawings be cacheable

Let collapsed border drawings be cacheable

Previously because one table cell may paint its collapsed borders
multiple times, we enclose all collapsed border drawings in a
ScopeRecorder.

However, actually a table cell paints each border only once. When it
paint collapsed borders multiple times, it just paint different borders
in different time.

Use display item type to distinguish the different borders to avoid
the ScopeRecorder.

Also revert https://codereview.chromium.org/1234463005 to invalidate
table cells with changed collapsed borders.

BUG=510492
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201002

[Xianzhu, 2015-08-19 17:08:59]

Patchset #1 (id:1) has been deleted

[Xianzhu, 2015-08-19 22:37:09]

wangxianzhu@chromium.org changed reviewers:
+ pdr@chromium.org

[pdr., 2015-08-20 07:07:15]

Can you share some numbers about how much of a performance win this approach is?
Tables with collapsed borders are indeed common, but it's surprising to me that
we need this level of complexity to paint table borders.

https://codereview.chromium.org/1306433002/diff/60001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1306433002/diff/60001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:459: 
Nit: extra newline

https://codereview.chromium.org/1306433002/diff/60001/LayoutTests/fast/table/...
File
LayoutTests/fast/table/border-collapsing/cached-change-cell-border-width-expected.txt
(right):

https://codereview.chromium.org/1306433002/diff/60001/LayoutTests/fast/table/...
LayoutTests/fast/table/border-collapsing/cached-change-cell-border-width-expected.txt:23:
"LayoutTableCell TD id='foo'",
Why do we get two invalidations for foo instead of one?

https://codereview.chromium.org/1306433002/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/1306433002/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutTableSection.h:213: bool setCachedCollapsedBorder(const
LayoutTableCell*, CollapsedBorderSide, const CollapsedBorderValue&);
Please add a comment explaining the return value.

// Return true if the collapsed borders change.

https://codereview.chromium.org/1306433002/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/1306433002/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItem.h:94:
TableCollapsedBorderUnalignedBase,
This represents a whole bunch of new types just for collapsed borders. Are they
really special enough to require a new approach that wasn't needed for other
types? I didn't follow the patches that added these cases originally, so I may
be rehashing old discussion :(

It looks like we paint collapsed borders by themselves (i.e., not interleaved
with potentially-overlapping cells). Could we just introduce a single display
item for the entire table's collapsed borders? Or, maybe one per row or column?

[Xianzhu, 2015-08-20 16:39:51]

Previously one extreme performance issue was: for a big table part of which is
outside of the interest rect, on each composited scroll, we had to repaint
collapsed borders of the whole table and also invalidate rasterization of the
area covered by the whole table.

https://codereview.chromium.org/1306433002/diff/60001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1306433002/diff/60001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:459: 
On 2015/08/20 07:07:15, pdr wrote:
> Nit: extra newline

Done.

https://codereview.chromium.org/1306433002/diff/60001/LayoutTests/fast/table/...
File
LayoutTests/fast/table/border-collapsing/cached-change-cell-border-width-expected.txt
(right):

https://codereview.chromium.org/1306433002/diff/60001/LayoutTests/fast/table/...
LayoutTests/fast/table/border-collapsing/cached-change-cell-border-width-expected.txt:23:
"LayoutTableCell TD id='foo'",
On 2015/08/20 07:07:15, pdr wrote:
> Why do we get two invalidations for foo instead of one?

The new one is because of change of collapsed border. The other one is because
of other reasons, such as layout, other style change, etc.

https://codereview.chromium.org/1306433002/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/1306433002/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutTableSection.h:213: bool setCachedCollapsedBorder(const
LayoutTableCell*, CollapsedBorderSide, const CollapsedBorderValue&);
On 2015/08/20 07:07:15, pdr wrote:
> Please add a comment explaining the return value.
> 
> // Return true if the collapsed borders change.

Done.

https://codereview.chromium.org/1306433002/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/1306433002/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItem.h:94:
TableCollapsedBorderUnalignedBase,
On 2015/08/20 07:07:15, pdr wrote:
> This represents a whole bunch of new types just for collapsed borders. Are
they
> really special enough to require a new approach that wasn't needed for other
> types? I didn't follow the patches that added these cases originally, so I may
> be rehashing old discussion :(
>

I know the 16 new types may look scary :), but this doesn't increase the number
of generated actual display items. Each cell still generate at most four display
items for collapsed borders if the borders are all of different priorities
(caused by e.g. different widths). Previously we couldn't distinguish them so
used ScopeRecorder which disabled caching of them.

The following scenarios may help understand the method:
- If all cell have borders which are of the same priority, each cell generates
one display item for collapsed borders with type
TableCollapsedBorderTopLeftRightBottom;
- Given two adjacent cells with certain collapsed border styles, we may generate
the following display items (cell1 border: 2px 4px 2px 4px; cell2 border: 5px
4px 1px 4px)
   cell2, TableCollapsedBorderBottom
   cell1, TableCollapsedBorderTopBottom
   cell2, TableCollapsedBorderRightLeft
   cell1, TableCollapsedBorderRightLeft
   cell2, TableCollapsedBorderTop
(Previously, we also generated 5 display items, all with type
DrawingPaintPhaseTableCollapsedBorder.)

Another way is just to add 4 new display items: TableCollapsedBorder[1-4] (1~4
are the sequence of the collapsed border display items for a cell), then the
above display items will be:
   cell2, TableCollapsedBorder1 (for bottom)
   cell1, TableCollapsedBorder1 (for top and bottom)
   cell2, TableCollapsedBorder2 (for right and left)
   cell1, TableCollapsedBorder2 (for right and left)
   cell2, TableCollapsedBorder3 (for top)
In this way, it seems harder to debug, and we need a counter somewhere
associated with each cell during painting.

> It looks like we paint collapsed borders by themselves (i.e., not interleaved
> with potentially-overlapping cells).

They do interleave, so TablePainter iterate the cells multiple passes to paint
collapsed borders of different priorities. The example above also explains this.

> Could we just introduce a single display
> item for the entire table's collapsed borders?

Tried this, but finally I found that this conflicts with the painting
optimization in TableSectionPainter::paintObject() which skips painting of table
rows outside of the paint rect. If we used a single display item, the item will
be different when paint rect changes.

> Or, maybe one per row or column?

This will make the situation more complex. The number of display items for
collapsed borders for a row or column is deterministic depending on how many
priorities of collapsed borders are, and I see no way to distinguish them except
using scopes. In contrast, a cell generates at most 4 display items.

[Xianzhu, 2015-08-20 16:41:40]

On 2015/08/20 16:39:51, Xianzhu wrote:
> - If all cell have borders which are of the same priority, each cell generates
> one display item for collapsed borders with type
> TableCollapsedBorderTopLeftRightBottom;

s/TableCollapsedBorderTopLeftRightBottom/TableCollapsedBorderTopRightBottomLeft/

[pdr., 2015-08-21 00:08:22]

LGTM, thanks for the extra explanation.

https://codereview.chromium.org/1306433002/diff/80001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/1306433002/diff/80001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItem.h:94:
TableCollapsedBorderUnalignedBase,
Lets add a comment here describing why collapsed borders are different. Maybe
something like:

// Collapsed borders can be painted together (e.g., left & top) but there are at
most 4 phases of collapsed
// border painting for a single cell. To disambiguate these phases of collapsed
border painting, a mask is used.
//
// TableCollapsedBorderBase can be larger than TableCollapsedBorderUnalignedBase
to ensure the base lower bits are 0's.

https://codereview.chromium.org/1306433002/diff/80001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemTest.cpp (right):

https://codereview.chromium.org/1306433002/diff/80001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemTest.cpp:17: DisplayItem::Type
nonCachedType =
DisplayItem::nonCachedType(static_cast<DisplayItem::Type>(type));
Can we remove the special case for collapsed borders here? I think you can just
adjust specialDrawingTypeAsDebugString to print out
TableCollapsedBorderAlignment.

[Xianzhu, 2015-08-21 17:10:39]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-08-21 17:10:40]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1306433002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-08-21 17:11:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306433002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306433002/100001

[Xianzhu, 2015-08-21 17:11:52]

https://codereview.chromium.org/1306433002/diff/80001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/1306433002/diff/80001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItem.h:94:
TableCollapsedBorderUnalignedBase,
On 2015/08/21 00:08:22, pdr wrote:
> Lets add a comment here describing why collapsed borders are different. Maybe
> something like:
> 
> // Collapsed borders can be painted together (e.g., left & top) but there are
at
> most 4 phases of collapsed
> // border painting for a single cell. To disambiguate these phases of
collapsed
> border painting, a mask is used.
> //
> // TableCollapsedBorderBase can be larger than
TableCollapsedBorderUnalignedBase
> to ensure the base lower bits are 0's.

Done.

https://codereview.chromium.org/1306433002/diff/80001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemTest.cpp (right):

https://codereview.chromium.org/1306433002/diff/80001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemTest.cpp:17: DisplayItem::Type
nonCachedType =
DisplayItem::nonCachedType(static_cast<DisplayItem::Type>(type));
On 2015/08/21 00:08:22, pdr wrote:
> Can we remove the special case for collapsed borders here? I think you can
just
> adjust specialDrawingTypeAsDebugString to print out
> TableCollapsedBorderAlignment.

Done.

[commit-bot: I haz the power, 2015-08-21 19:00:56]

Committed patchset #5 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201002