Paint collapsed borders of a table as one display item

Paint collapsed borders of a table as one display item

This simplifies paint invalidation logic on collapsed border changes.

About performance:
With this CL we create a big display item on the table containing all
collapsed border paintings, instead of many small display items on cells.
This will cause more painting when the display item is invalidated, but
saves time to handle many cached display items. Perf try jobs [1] show
that this CL actually improves performance of one test
(blink_perf.paint.large-table-background-change-with-invisible-collapsed-borders.html),
and doesn't affect much of performance of other tests.
Cluster-telemetry run showed no change or slight progression [2].

[1] https://codereview.chromium.org/2430313004/#ps60001
[2] https://ct.skia.org/chromium_perf_runs/, run 1433

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=663208
Committed: https://crrev.com/c5cc11e88930c6e3f5e08b330fb14b6b571a0404
Cr-Commit-Position: refs/heads/master@{#431744}

[Xianzhu, 2016-10-19 16:45:10]

Description was changed from

==========
Paint collapsed borders of a table as one display item
==========

to

==========
Paint collapsed borders of a table as one display item
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-10-19 21:15:46]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 21:16:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 21:36:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-19 21:36:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2016-11-02 17:53:45]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 17:54:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 17:56:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 17:56:50]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2016-11-02 17:57:07]

Description was changed from

==========
Paint collapsed borders of a table as one display item
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Paint collapsed borders of a table as one display item

This simplifies paint invalidation logic on collapsed border changes.

About performance:
With this CL we create a big display item on the table containing all
collapsed border paintings, instead of many small display items on cells.
This will cause more painting when the display item is invalidated, but
saves time to handle many cached display items. Perf try jobs show that
this CL actually improves performance of one test, and doesn't affect
much of performance of other tests.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-11-02 21:14:02]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 21:14:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 22:46:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 22:46:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2016-11-02 23:28:40]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, wkorman@chromium.org

[wkorman, 2016-11-03 05:26:13]

lgtm

I'm in favor of landing this ahead of my change http://crrev.com/2469903002 and
we can merge my change as needed. Since mine needs more work anyway, and is
smaller.

Does this change fix or change any behavior re: overpainting of cells with a
border-color with opacity? If so perhaps add http://crbug.com/548903 to BUG=.

Can you link or summarize perf results with some numbers? Are there a few
top-10k-type sites that use huge tables with border-collapse that we can
additionally checkpoint on as representative sample? Maybe something in
Wikipedia or a W3C spec, open to other thoughts.

https://codereview.chromium.org/2430313004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (left):

https://codereview.chromium.org/2430313004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1433: void
LayoutTableCell::invalidateDisplayItemClients(
I think we can just delete this whole method override.

https://codereview.chromium.org/2430313004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TablePainter.cpp (right):

https://codereview.chromium.org/2430313004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TablePainter.cpp:63: if
(paintInfo.context.getPaintController().clientCacheIsValid(
This looks like the first call of clientCacheIsValid() outside of
PaintController itself, may be worth a comment noting that this is unusual since
painting collapsed borders is itself unusual and so we manage its cache
invalidation specially. Or maybe we expect there may be more calls to
clientCacheIsValid() for similar cases later. Figured worth mentioning since it
caught my eye.

[Xianzhu, 2016-11-03 15:46:09]

On 2016/11/03 05:26:13, wkorman wrote:
> lgtm
> 
> I'm in favor of landing this ahead of my change http://crrev.com/2469903002
and
> we can merge my change as needed. Since mine needs more work anyway, and is
> smaller.
> 
> Does this change fix or change any behavior re: overpainting of cells with a
> border-color with opacity? If so perhaps add http://crbug.com/548903 to BUG=.
> 

No. We still paint each shared border twice to ensure the "border cases"
correct. We need to continue atotic@'s work to fix the bug.

> Can you link or summarize perf results with some numbers? Are there a few
> top-10k-type sites that use huge tables with border-collapse that we can
> additionally checkpoint on as representative sample? Maybe something in
> Wikipedia or a W3C spec, open to other thoughts.

Added in descriptions. I ran blink_perf.paint perf suite which runs tests under
third_party/WebKit/PerformanceTests/Paint which contains test cases specially
for huge tables with collapsed borders which are representative for our case.

[Xianzhu, 2016-11-03 15:46:46]

Description was changed from

==========
Paint collapsed borders of a table as one display item

This simplifies paint invalidation logic on collapsed border changes.

About performance:
With this CL we create a big display item on the table containing all
collapsed border paintings, instead of many small display items on cells.
This will cause more painting when the display item is invalidated, but
saves time to handle many cached display items. Perf try jobs show that
this CL actually improves performance of one test, and doesn't affect
much of performance of other tests.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Paint collapsed borders of a table as one display item

This simplifies paint invalidation logic on collapsed border changes.

About performance:
With this CL we create a big display item on the table containing all
collapsed border paintings, instead of many small display items on cells.
This will cause more painting when the display item is invalidated, but
saves time to handle many cached display items. Perf try jobs [1] show
that this CL actually improves performance of one test
(blink_perf.paint.large-table-background-change-with-invisible-collapsed-borders.html),
and doesn't affect much of performance of other tests.

[1] https://codereview.chromium.org/2430313004/#ps60001

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-11-03 16:06:13]

https://codereview.chromium.org/2430313004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (left):

https://codereview.chromium.org/2430313004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1433: void
LayoutTableCell::invalidateDisplayItemClients(
On 2016/11/03 05:26:13, wkorman wrote:
> I think we can just delete this whole method override.

Done.

https://codereview.chromium.org/2430313004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TablePainter.cpp (right):

https://codereview.chromium.org/2430313004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TablePainter.cpp:63: if
(paintInfo.context.getPaintController().clientCacheIsValid(
On 2016/11/03 05:26:13, wkorman wrote:
> This looks like the first call of clientCacheIsValid() outside of
> PaintController itself, may be worth a comment noting that this is unusual
since
> painting collapsed borders is itself unusual and so we manage its cache
> invalidation specially. Or maybe we expect there may be more calls to
> clientCacheIsValid() for similar cases later. Figured worth mentioning since
it
> caught my eye.

Done.

[Xianzhu, 2016-11-03 16:06:21]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 16:06:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2016-11-03 16:54:50]

Description was changed from

==========
Paint collapsed borders of a table as one display item

This simplifies paint invalidation logic on collapsed border changes.

About performance:
With this CL we create a big display item on the table containing all
collapsed border paintings, instead of many small display items on cells.
This will cause more painting when the display item is invalidated, but
saves time to handle many cached display items. Perf try jobs [1] show
that this CL actually improves performance of one test
(blink_perf.paint.large-table-background-change-with-invisible-collapsed-borders.html),
and doesn't affect much of performance of other tests.

[1] https://codereview.chromium.org/2430313004/#ps60001

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Paint collapsed borders of a table as one display item

This simplifies paint invalidation logic on collapsed border changes.

About performance:
With this CL we create a big display item on the table containing all
collapsed border paintings, instead of many small display items on cells.
This will cause more painting when the display item is invalidated, but
saves time to handle many cached display items. Perf try jobs [1] show
that this CL actually improves performance of one test
(blink_perf.paint.large-table-background-change-with-invisible-collapsed-borders.html),
and doesn't affect much of performance of other tests.
Cluster-telemetry run showed no change or slight progression [2].

[1] https://codereview.chromium.org/2430313004/#ps60001
[2] https://ct.skia.org/chromium_perf_runs/, run 1433

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-11-03 21:00:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 21:00:07]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2016-11-03 21:00:59]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-11-03 21:00:59]

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

[Xianzhu, 2016-11-03 21:01:28]

The CQ bit was unchecked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-11-03 21:01:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2016-11-03 21:16:29]

https://codereview.chromium.org/2430313004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2430313004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:212: if
(object.isTable() && toLayoutTable(object).hasCollapsedBorders())
The old code was just inaccurate I guess?

https://codereview.chromium.org/2430313004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/TablePainter.cpp (right):

https://codereview.chromium.org/2430313004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TablePainter.cpp:64: // Normally we don't
clip individual display items by paint dirty rect
Is such an optimization already implemented in the old code?

https://codereview.chromium.org/2430313004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TablePainter.cpp:75: if
(!DrawingRecorder::useCachedDrawingIfPossible(
if (DrawingRecorder::useCachedDrawingIfPossible(...))
  return;

[Xianzhu, 2016-11-03 21:26:55]

https://codereview.chromium.org/2430313004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2430313004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:212: if
(object.isTable() && toLayoutTable(object).hasCollapsedBorders())
On 2016/11/03 21:16:29, chrishtr wrote:
> The old code was just inaccurate I guess?

They are equivalent. The new function LayoutTable::hasCollapsedBorders() checks
the same condition.

https://codereview.chromium.org/2430313004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/TablePainter.cpp (right):

https://codereview.chromium.org/2430313004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TablePainter.cpp:64: // Normally we don't
clip individual display items by paint dirty rect
On 2016/11/03 21:16:29, chrishtr wrote:
> Is such an optimization already implemented in the old code?

Yes, in TableSectionPainter, to skip painting collapsed borders for cells out of
the interest rect. Actually the new code here is to adapt to the old
optimization, because the display item may change when interest rect changes.

https://codereview.chromium.org/2430313004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TablePainter.cpp:75: if
(!DrawingRecorder::useCachedDrawingIfPossible(
On 2016/11/03 21:16:29, chrishtr wrote:
> if (DrawingRecorder::useCachedDrawingIfPossible(...))
>   return;

Done.

[wkorman, 2016-11-07 23:25:24]

On 2016/11/03 15:46:09, Xianzhu wrote:
> Added in descriptions. I ran blink_perf.paint perf suite which runs tests
under
> third_party/WebKit/PerformanceTests/Paint which contains test cases specially
> for huge tables with collapsed borders which are representative for our case.

I looked at these tests briefly with chrishtr@ just now, and one question I had
was whether table row/cols of 300x320 is large enough to reveal performance
impact of this change. What do you think? I had assumed "huge" was likely 1K -
10K+.

[Xianzhu, 2016-11-07 23:35:50]

On 2016/11/07 23:25:24, wkorman wrote:
> On 2016/11/03 15:46:09, Xianzhu wrote:
> > Added in descriptions. I ran blink_perf.paint perf suite which runs tests
> under
> > third_party/WebKit/PerformanceTests/Paint which contains test cases
specially
> > for huge tables with collapsed borders which are representative for our
case.
> 
> I looked at these tests briefly with chrishtr@ just now, and one question I
had
> was whether table row/cols of 300x320 is large enough to reveal performance
> impact of this change. What do you think? I had assumed "huge" was likely 1K -
> 10K+.

Will try.

[Xianzhu, 2016-11-08 01:38:02]

Filed a bug and added performance data in crbug.com/647809. Rasterization
performance seems a blocking issue of this CL.

[Xianzhu, 2016-11-08 01:38:17]

On 2016/11/08 01:38:02, Xianzhu wrote:
> Filed a bug and added performance data in crbug.com/647809. Rasterization
> performance seems a blocking issue of this CL.

The bug should be crbug.com/663208.

[Xianzhu, 2016-11-10 23:16:15]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 23:16:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2016-11-10 23:17:55]

Description was changed from

==========
Paint collapsed borders of a table as one display item

This simplifies paint invalidation logic on collapsed border changes.

About performance:
With this CL we create a big display item on the table containing all
collapsed border paintings, instead of many small display items on cells.
This will cause more painting when the display item is invalidated, but
saves time to handle many cached display items. Perf try jobs [1] show
that this CL actually improves performance of one test
(blink_perf.paint.large-table-background-change-with-invisible-collapsed-borders.html),
and doesn't affect much of performance of other tests.
Cluster-telemetry run showed no change or slight progression [2].

[1] https://codereview.chromium.org/2430313004/#ps60001
[2] https://ct.skia.org/chromium_perf_runs/, run 1433

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Paint collapsed borders of a table as one display item

This simplifies paint invalidation logic on collapsed border changes.

About performance:
With this CL we create a big display item on the table containing all
collapsed border paintings, instead of many small display items on cells.
This will cause more painting when the display item is invalidated, but
saves time to handle many cached display items. Perf try jobs [1] show
that this CL actually improves performance of one test
(blink_perf.paint.large-table-background-change-with-invisible-collapsed-borders.html),
and doesn't affect much of performance of other tests.
Cluster-telemetry run showed no change or slight progression [2].

[1] https://codereview.chromium.org/2430313004/#ps60001
[2] https://ct.skia.org/chromium_perf_runs/, run 1433

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=647809,663208
==========

[Xianzhu, 2016-11-10 23:23:43]

The latest patch improves raster performance by not always invalidating the
whole table's visual rect.

Local performance test shows no regressions with blink_perf.paint tests with
1000x1000 table size. It also shows progression for some of the tests.

The new method is not compatible with SPv2's raster invalidation method. It's a
performance issue instead of under-invalidation issue. Will track this with
crbug.com/663028. We can try to use one display item for each row for collapsed
borders to leverage sizes and number of display items.

[commit-bot: I haz the power, 2016-11-10 23:33:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-10 23:33:09]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[chrishtr, 2016-11-11 01:39:18]

On 2016/11/10 at 23:23:43, wangxianzhu wrote:
> The latest patch improves raster performance by not always invalidating the
whole table's visual rect.

How does it do that exactly? I can't see where this code is yet.

> 
> Local performance test shows no regressions with blink_perf.paint tests with
1000x1000 table size. It also shows progression for some of the tests.
> 
> The new method is not compatible with SPv2's raster invalidation method. It's
a performance issue instead of under-invalidation issue. Will track this with
crbug.com/663028. We can try to use one display item for each row for collapsed
borders to leverage sizes and number of display items.

I think you linked to the wrong bug.

[wkorman, 2016-11-11 02:07:13]

On 2016/11/11 01:39:18, chrishtr wrote:
> On 2016/11/10 at 23:23:43, wangxianzhu wrote:
> > The latest patch improves raster performance by not always invalidating the
> whole table's visual rect.
> 
> How does it do that exactly? I can't see where this code is yet.

I think it's the logic in LayoutTable::recalcCollapsedBordersIfNeeded.

> > Local performance test shows no regressions with blink_perf.paint tests with
> 1000x1000 table size. It also shows progression for some of the tests.
> > 
> > The new method is not compatible with SPv2's raster invalidation method.
It's
> a performance issue instead of under-invalidation issue. Will track this with
> crbug.com/663028. We can try to use one display item for each row for
collapsed
> borders to leverage sizes and number of display items.
> 
> I think you linked to the wrong bug.

Yes, the right one is in BUG= it's 663208.

[wkorman, 2016-11-11 02:19:42]

lgtm

> We can try to use one display item for each row for collapsed
> borders to leverage sizes and number of display items.

This sounds like a good idea balancing tradeoffs.

It looks like a couple of small bits in this change could be pulled into smaller
ones -- factoring PaintResult into its own file, and the CullRect changes. Which
could be useful if we end up needing to revert other bits for a while but it's a
minority of the code in this change.

I'm still supportive if you want to land this ahead of the table cell background
work I am just getting back to, I know chrishtr@ was concerned re: merge
overhead if we merge back to M55 but I don't think it will be too bad.

[Xianzhu, 2016-11-11 05:42:52]

Description was changed from

==========
Paint collapsed borders of a table as one display item

This simplifies paint invalidation logic on collapsed border changes.

About performance:
With this CL we create a big display item on the table containing all
collapsed border paintings, instead of many small display items on cells.
This will cause more painting when the display item is invalidated, but
saves time to handle many cached display items. Perf try jobs [1] show
that this CL actually improves performance of one test
(blink_perf.paint.large-table-background-change-with-invisible-collapsed-borders.html),
and doesn't affect much of performance of other tests.
Cluster-telemetry run showed no change or slight progression [2].

[1] https://codereview.chromium.org/2430313004/#ps60001
[2] https://ct.skia.org/chromium_perf_runs/, run 1433

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=647809,663208
==========

to

==========
Paint collapsed borders of a table as one display item

This simplifies paint invalidation logic on collapsed border changes.

About performance:
With this CL we create a big display item on the table containing all
collapsed border paintings, instead of many small display items on cells.
This will cause more painting when the display item is invalidated, but
saves time to handle many cached display items. Perf try jobs [1] show
that this CL actually improves performance of one test
(blink_perf.paint.large-table-background-change-with-invisible-collapsed-borders.html),
and doesn't affect much of performance of other tests.
Cluster-telemetry run showed no change or slight progression [2].

[1] https://codereview.chromium.org/2430313004/#ps60001
[2] https://ct.skia.org/chromium_perf_runs/, run 1433

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=663208
==========

[Xianzhu, 2016-11-11 05:44:27]

On 2016/11/11 01:39:18, chrishtr wrote:
> On 2016/11/10 at 23:23:43, wangxianzhu wrote:
> > The latest patch improves raster performance by not always invalidating the
> whole table's visual rect.
> 
> How does it do that exactly? I can't see where this code is yet.
> 

You can view the difference of LayoutTable.cpp between patch set 7 and 8:
https://codereview.chromium.org/2430313004/diff2/120001:140001/third_party/We...

> > 
> > Local performance test shows no regressions with blink_perf.paint tests with
> 1000x1000 table size. It also shows progression for some of the tests.
> > 
> > The new method is not compatible with SPv2's raster invalidation method.
It's
> a performance issue instead of under-invalidation issue. Will track this with
> crbug.com/663028. We can try to use one display item for each row for
collapsed
> borders to leverage sizes and number of display items.
> 
> I think you linked to the wrong bug.

Updated.

[chrishtr, 2016-11-11 19:18:01]

https://codereview.chromium.org/2430313004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/2430313004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:819: if
(changedWithChildCompositedLayers) {
How about using invalidatePaintRectangle of the union of the affected cells? We
will
need to make paint rectangle invalidation work for SPv2, for at least the use
case of
HTMLCanvas.

[Xianzhu, 2016-11-11 20:06:55]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 20:07:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2016-11-11 20:09:30]

https://codereview.chromium.org/2430313004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/2430313004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:819: if
(changedWithChildCompositedLayers) {
On 2016/11/11 19:18:01, chrishtr wrote:
> How about using invalidatePaintRectangle of the union of the affected cells?
We
> will
> need to make paint rectangle invalidation work for SPv2, for at least the use
> case of
> HTMLCanvas.

Done.

The new patch issues redundant rectangle paint invalidations for the table and
cells (existing, caused by style change) when there is no composited layers
within the table. The extra cost should be trivial, and this seems the best for
code clarity and SPv2.

[chrishtr, 2016-11-11 20:56:48]

lgtm

[commit-bot: I haz the power, 2016-11-11 21:37:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 21:37:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xianzhu, 2016-11-11 23:05:53]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-11-11 23:05:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
wkorman@chromium.org, chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2430313004/#ps180001
(title: "Rebaseline on mac and win")

[commit-bot: I haz the power, 2016-11-11 23:06:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-12 02:14:35]

Description was changed from

==========
Paint collapsed borders of a table as one display item

This simplifies paint invalidation logic on collapsed border changes.

About performance:
With this CL we create a big display item on the table containing all
collapsed border paintings, instead of many small display items on cells.
This will cause more painting when the display item is invalidated, but
saves time to handle many cached display items. Perf try jobs [1] show
that this CL actually improves performance of one test
(blink_perf.paint.large-table-background-change-with-invisible-collapsed-borders.html),
and doesn't affect much of performance of other tests.
Cluster-telemetry run showed no change or slight progression [2].

[1] https://codereview.chromium.org/2430313004/#ps60001
[2] https://ct.skia.org/chromium_perf_runs/, run 1433

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=663208
==========

to

==========
Paint collapsed borders of a table as one display item

This simplifies paint invalidation logic on collapsed border changes.

About performance:
With this CL we create a big display item on the table containing all
collapsed border paintings, instead of many small display items on cells.
This will cause more painting when the display item is invalidated, but
saves time to handle many cached display items. Perf try jobs [1] show
that this CL actually improves performance of one test
(blink_perf.paint.large-table-background-change-with-invisible-collapsed-borders.html),
and doesn't affect much of performance of other tests.
Cluster-telemetry run showed no change or slight progression [2].

[1] https://codereview.chromium.org/2430313004/#ps60001
[2] https://ct.skia.org/chromium_perf_runs/, run 1433

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=663208
==========

[commit-bot: I haz the power, 2016-11-12 02:14:37]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-11-12 02:16:19]

Description was changed from

==========
Paint collapsed borders of a table as one display item

This simplifies paint invalidation logic on collapsed border changes.

About performance:
With this CL we create a big display item on the table containing all
collapsed border paintings, instead of many small display items on cells.
This will cause more painting when the display item is invalidated, but
saves time to handle many cached display items. Perf try jobs [1] show
that this CL actually improves performance of one test
(blink_perf.paint.large-table-background-change-with-invisible-collapsed-borders.html),
and doesn't affect much of performance of other tests.
Cluster-telemetry run showed no change or slight progression [2].

[1] https://codereview.chromium.org/2430313004/#ps60001
[2] https://ct.skia.org/chromium_perf_runs/, run 1433

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=663208
==========

to

==========
Paint collapsed borders of a table as one display item

This simplifies paint invalidation logic on collapsed border changes.

About performance:
With this CL we create a big display item on the table containing all
collapsed border paintings, instead of many small display items on cells.
This will cause more painting when the display item is invalidated, but
saves time to handle many cached display items. Perf try jobs [1] show
that this CL actually improves performance of one test
(blink_perf.paint.large-table-background-change-with-invisible-collapsed-borders.html),
and doesn't affect much of performance of other tests.
Cluster-telemetry run showed no change or slight progression [2].

[1] https://codereview.chromium.org/2430313004/#ps60001
[2] https://ct.skia.org/chromium_perf_runs/, run 1433

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=663208

Committed: https://crrev.com/c5cc11e88930c6e3f5e08b330fb14b6b571a0404
Cr-Commit-Position: refs/heads/master@{#431744}
==========

[commit-bot: I haz the power, 2016-11-12 02:16:21]

Patchset 10 (id:??) landed as
https://crrev.com/c5cc11e88930c6e3f5e08b330fb14b6b571a0404
Cr-Commit-Position: refs/heads/master@{#431744}

[Xianzhu, 2016-11-14 17:00:55]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2495343002/ by wangxianzhu@chromium.org.

The reason for reverting is: Caused bug 664887. We miss invalidation of
collapsed borders when they are changed by layout.

BUG=664887.