[SPv2] Defer decision of raster invalidation after paint for changes z-index, transform, etc.

[SPv2] Defer decision of raster invalidation after paint for changes of z-index, transform, etc.

Previously we invalidate paint (which will also cause raster invalidation)
of the subtree if z-index, transform changed on an object which will not
be composited. This requires logic to precisely predict compositing status
which is fragile and causes unnecessary paint invalidations.

We don't need to invalidate paint of object which just changes z-index,
transform etc. We can invalidate raster if needed after painting.

If a cached display item is moved out of the original chunk, invalidate
raster for it on both the old chunk and the new chunk.

If a cached display item is moved within the chunk behind any other
cached display item, invalidate raster for it.

Otherwise we don't invalidate raster.

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

Committed: https://crrev.com/eb1f8854d9d8fa17d65ad4daf9de3d97cc9c2538
Cr-Commit-Position: refs/heads/master@{#417520}

[Xianzhu, 2016-09-01 18:49:11]

Description was changed from

==========
Invalidate raster if needed for reordered display items

With this change, we no longer need to invalidate paint of object
which just changes z-index. This also avoids paint invalidations that
we don't need on SPv1 if the layer is composited.

BUG=526191
==========

to

==========
Invalidate raster if needed for reordered display items

With this change, we no longer need to invalidate paint of object
which just changes z-index. This also avoids paint invalidations that
we don't need on SPv1 if the layer is composited.

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

[Xianzhu, 2016-09-01 18:49:16]

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

[commit-bot: I haz the power, 2016-09-01 18:49:46]

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

[Xianzhu, 2016-09-01 19:01:57]

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

[commit-bot: I haz the power, 2016-09-01 19:03:02]

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

[Xianzhu, 2016-09-01 19:17:35]

Description was changed from

==========
Invalidate raster if needed for reordered display items

With this change, we no longer need to invalidate paint of object
which just changes z-index. This also avoids paint invalidations that
we don't need on SPv1 if the layer is composited.

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

to

==========
Don't invalidate paint but invalidate raster if needed for reordered display
items

We don't need to invalidate paint of object which just changes z-index.
Invalidate raster if needed during painting.

If a cached display item is moved out of the original chunk, invalidate
raster for it on both the old chunk and the new chunk.

If a cached display item is moved within the chunk behind any other
cached display item, invalidate raster for it.

Otherwise we don't invalidate raster.

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

[commit-bot: I haz the power, 2016-09-01 20:28:06]

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

[commit-bot: I haz the power, 2016-09-01 20:28:07]

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

[Xianzhu, 2016-09-01 20:43:14]

Description was changed from

==========
Don't invalidate paint but invalidate raster if needed for reordered display
items

We don't need to invalidate paint of object which just changes z-index.
Invalidate raster if needed during painting.

If a cached display item is moved out of the original chunk, invalidate
raster for it on both the old chunk and the new chunk.

If a cached display item is moved within the chunk behind any other
cached display item, invalidate raster for it.

Otherwise we don't invalidate raster.

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

to

==========
[SPv2] Don't invalidate paint but invalidate raster if needed for reordered
display items

We don't need to invalidate paint of object which just changes z-index.
Invalidate raster if needed during painting.

If a cached display item is moved out of the original chunk, invalidate
raster for it on both the old chunk and the new chunk.

If a cached display item is moved within the chunk behind any other
cached display item, invalidate raster for it.

Otherwise we don't invalidate raster.

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

[Xianzhu, 2016-09-01 20:43:42]

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

[Xianzhu, 2016-09-01 20:43:58]

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

[commit-bot: I haz the power, 2016-09-01 20:44:18]

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

[Xianzhu, 2016-09-01 20:57:40]

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

[commit-bot: I haz the power, 2016-09-01 20:57:49]

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-09-01 22:10:37]

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

[commit-bot: I haz the power, 2016-09-01 22:10:38]

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

[chrishtr, 2016-09-02 04:35:19]

Is there a measurable performance impact from this change? Will not invalidating
the objects and
just raster save much?

[Xianzhu, 2016-09-02 05:46:13]

On 2016/09/02 04:35:19, chrishtr wrote:
> Is there a measurable performance impact from this change? Will not
invalidating
> the objects and
> just raster save much?

This CL is more a implementation of better raster invalidation decision than a
performance improvement.

For z-index (or transform, opacity, etc.) change, paint invalidation is fully
unnecessary, and raster invalidation is not always necessary. For example,
z-index change of a composited layer needs neither paint invalidation nor raster
invalidation. Not invalidating paint not only saves paint operations, but also
enables better decision of raster invalidation after painting and/or compositing
update.

In SPv1, when z-index (or transform, opacity, etc.) changes, we check if the
object will be composited to determine whether to issue paint invalidations for
the subtree:
  if
(!toLayoutBoxModelObject(this)->layer()->hasStyleDeterminedDirectCompositingReasons())
    diff.setNeedsPaintInvalidationSubtree();

The logic predicts what compositing update will do for early decision of paint
invalidation. It is fragile because the condition must be exactly the same as
the condition used in compositing update (or we use a conservative condition to
avoid wrong decision causing under-invalidation but may result some
over-invalidations).

In SPv2, we can decide raster invalidation after paint (for invalidations within
original PaintChunks) or compositing update (when combining multiple PaintChunks
into bigger composited layers, not covered by this CL), and make decision based
on output of previous stages instead of predicting what later stages will do.

[Xianzhu, 2016-09-06 17:43:24]

Description was changed from

==========
[SPv2] Don't invalidate paint but invalidate raster if needed for reordered
display items

We don't need to invalidate paint of object which just changes z-index.
Invalidate raster if needed during painting.

If a cached display item is moved out of the original chunk, invalidate
raster for it on both the old chunk and the new chunk.

If a cached display item is moved within the chunk behind any other
cached display item, invalidate raster for it.

Otherwise we don't invalidate raster.

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

to

==========
[SPv2] Defer decision of raster invalidation after paint for z-index, transform,
etc. change

Previously we invalidate paint (which will also cause raster invalidation)
of the subtree if z-index, transform changed on an object which will not
be composited. This requires logic to precisely predict compositing status
which is fragile. 

We don't need to invalidate paint of object which just changes z-index,
transform etc. We can invalidate raster if needed after painting.

If a cached display item is moved out of the original chunk, invalidate
raster for it on both the old chunk and the new chunk.

If a cached display item is moved within the chunk behind any other
cached display item, invalidate raster for it.

Otherwise we don't invalidate raster.

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

[Xianzhu, 2016-09-06 17:43:47]

Updated description to better reflect the intent.

[Xianzhu, 2016-09-06 17:45:46]

Description was changed from

==========
[SPv2] Defer decision of raster invalidation after paint for z-index, transform,
etc. change

Previously we invalidate paint (which will also cause raster invalidation)
of the subtree if z-index, transform changed on an object which will not
be composited. This requires logic to precisely predict compositing status
which is fragile. 

We don't need to invalidate paint of object which just changes z-index,
transform etc. We can invalidate raster if needed after painting.

If a cached display item is moved out of the original chunk, invalidate
raster for it on both the old chunk and the new chunk.

If a cached display item is moved within the chunk behind any other
cached display item, invalidate raster for it.

Otherwise we don't invalidate raster.

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

to

==========
[SPv2] Defer decision of raster invalidation after paint for z-index, transform,
etc. change

Previously we invalidate paint (which will also cause raster invalidation)
of the subtree if z-index, transform changed on an object which will not
be composited. This requires logic to precisely predict compositing status
which is fragile and causes unnecessary paint invalidations.

We don't need to invalidate paint of object which just changes z-index,
transform etc. We can invalidate raster if needed after painting.

If a cached display item is moved out of the original chunk, invalidate
raster for it on both the old chunk and the new chunk.

If a cached display item is moved within the chunk behind any other
cached display item, invalidate raster for it.

Otherwise we don't invalidate raster.

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

[Xianzhu, 2016-09-06 17:50:15]

Description was changed from

==========
[SPv2] Defer decision of raster invalidation after paint for z-index, transform,
etc. change

Previously we invalidate paint (which will also cause raster invalidation)
of the subtree if z-index, transform changed on an object which will not
be composited. This requires logic to precisely predict compositing status
which is fragile and causes unnecessary paint invalidations.

We don't need to invalidate paint of object which just changes z-index,
transform etc. We can invalidate raster if needed after painting.

If a cached display item is moved out of the original chunk, invalidate
raster for it on both the old chunk and the new chunk.

If a cached display item is moved within the chunk behind any other
cached display item, invalidate raster for it.

Otherwise we don't invalidate raster.

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

to

==========
[SPv2] Defer decision of raster invalidation after paint for changes of z-index,
transform, etc.

Previously we invalidate paint (which will also cause raster invalidation)
of the subtree if z-index, transform changed on an object which will not
be composited. This requires logic to precisely predict compositing status
which is fragile and causes unnecessary paint invalidations.

We don't need to invalidate paint of object which just changes z-index,
transform etc. We can invalidate raster if needed after painting.

If a cached display item is moved out of the original chunk, invalidate
raster for it on both the old chunk and the new chunk.

If a cached display item is moved within the chunk behind any other
cached display item, invalidate raster for it.

Otherwise we don't invalidate raster.

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

[chrishtr, 2016-09-06 22:14:12]

https://codereview.chromium.org/2307623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.h (right):

https://codereview.chromium.org/2307623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.h:52:
Vector<PaintChunk>::const_iterator findChunkByDisplayItemIndex(size_t index)
const { return findChunkInVectorByDisplayItemIndex(m_paintChunks, index); }
Why does this call site not need:

 DCHECK(chunk != m_chunks.end());

https://codereview.chromium.org/2307623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):

https://codereview.chromium.org/2307623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:359:
AutoReset<size_t>
subsequenceBeginIndex(&m_currentCachedSubsequenceBeginIndexInNewList,
m_newDisplayItemList.size());
What does changing to m_currentCachedSubsequenceBeginIndexInNewList achieve? Not
sure I see it yet.

https://codereview.chromium.org/2307623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:608: //
that is probably exposed by the item.
 "exposed by the item moving earlier"

[Xianzhu, 2016-09-06 22:42:38]

https://codereview.chromium.org/2307623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.h (right):

https://codereview.chromium.org/2307623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.h:52:
Vector<PaintChunk>::const_iterator findChunkByDisplayItemIndex(size_t index)
const { return findChunkInVectorByDisplayItemIndex(m_paintChunks, index); }
On 2016/09/06 22:14:12, chrishtr wrote:
> Why does this call site not need:
> 
>  DCHECK(chunk != m_chunks.end());

It's not obvious from the function return type that the returned value is never
m_paintChunks.end(), so I loosed the restriction here to allow not-found
situation. Then the caller should check the result if it can ensure a chunk must
be found with the given index.

https://codereview.chromium.org/2307623002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):

https://codereview.chromium.org/2307623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:359:
AutoReset<size_t>
subsequenceBeginIndex(&m_currentCachedSubsequenceBeginIndexInNewList,
m_newDisplayItemList.size());
On 2016/09/06 22:14:12, chrishtr wrote:
> What does changing to m_currentCachedSubsequenceBeginIndexInNewList achieve?
Not
> sure I see it yet.

This replaces the original m_currentChunkIsFromCachedSubsequence which had a bug
when a chunk is not from cached subsequence but contains no invalidated
cacheable display items. Now use newChunk.beginIndex >=
m_currentCachedSubsequenceBeginIndexInNewList to check if a chunk is fully from
a cached subsequence.

https://codereview.chromium.org/2307623002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:608: //
that is probably exposed by the item.
On 2016/09/06 22:14:12, chrishtr wrote:
>  "exposed by the item moving earlier"

Done.

[Xianzhu, 2016-09-06 22:43:17]

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

[commit-bot: I haz the power, 2016-09-06 22:44:00]

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-09-07 00:21:20]

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

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

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

[chrishtr, 2016-09-07 20:49:03]

https://codereview.chromium.org/2307623002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):

https://codereview.chromium.org/2307623002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:603:
fast/css-grid-layout/grid-item-z-index-change-repaint.html [ Failure ]
These are marked as Failure because they invalidate
differently, right? Should these be part of the virtual test
suite with different expectations perhaps?

[Xianzhu, 2016-09-07 21:38:35]

pdr@ what's your opinion about running tests with spv2 expectations different
from spv1 expectations?

https://codereview.chromium.org/2307623002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):

https://codereview.chromium.org/2307623002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:603:
fast/css-grid-layout/grid-item-z-index-change-repaint.html [ Failure ]
On 2016/09/07 20:49:02, chrishtr wrote:
> These are marked as Failure because they invalidate
> differently, right?

Yes.

> Should these be part of the virtual test
> suite with different expectations perhaps?

There are also many other repaint tests having the similar issue. I think it's a
good way to run them as virtual tests with new baselines.

[pdr., 2016-09-08 01:36:23]

https://codereview.chromium.org/2307623002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):

https://codereview.chromium.org/2307623002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:603:
fast/css-grid-layout/grid-item-z-index-change-repaint.html [ Failure ]
On 2016/09/07 at 21:38:35, Xianzhu wrote:
> On 2016/09/07 20:49:02, chrishtr wrote:
> > These are marked as Failure because they invalidate
> > differently, right?
> 
> Yes.
> 
> > Should these be part of the virtual test
> > suite with different expectations perhaps?
> 
> There are also many other repaint tests having the similar issue. I think it's
a good way to run them as virtual tests with new baselines.

There are about 426 repaint tests in fast/ (43 outside fast/repaint) and another
534 repaint tests in svg/ (490 outside svg/repaint) so we're talking about a lot
of tests to maintain.

I don't have a great plan for this. What if we moved all repaint tests to a
repaint/ directory and just ran those directories as virtual tests suites? Do
you have a feeling for how many failures we would need to initially add to
TestExpectations for that to work?

https://codereview.chromium.org/2307623002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/2307623002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h:352: union {
This approach of using a union is very efficient but also very brittle. I can
imagine a person reading this for the first time would have a difficult time
understanding why a subclass like DrawingDisplayItem has two flavors in base
class. The way we move DisplayItems out of the contiguous container and replace
them with an invalid DisplayItem was already a lot of mental overhead.

What do you think of a simpler approach where we use an auxiliary array and
optimize later if needed? For example, before painting we could allocate a flat
array of size_t's that stores the index where the item has moved to.

[Xianzhu, 2016-09-08 03:01:42]

https://codereview.chromium.org/2307623002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):

https://codereview.chromium.org/2307623002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:603:
fast/css-grid-layout/grid-item-z-index-change-repaint.html [ Failure ]
On 2016/09/08 01:36:23, pdr. wrote:
> On 2016/09/07 at 21:38:35, Xianzhu wrote:
> > On 2016/09/07 20:49:02, chrishtr wrote:
> > > These are marked as Failure because they invalidate
> > > differently, right?
> > 
> > Yes.
> > 
> > > Should these be part of the virtual test
> > > suite with different expectations perhaps?
> > 
> > There are also many other repaint tests having the similar issue. I think
it's
> a good way to run them as virtual tests with new baselines.
> 
> There are about 426 repaint tests in fast/ (43 outside fast/repaint) and
another
> 534 repaint tests in svg/ (490 outside svg/repaint) so we're talking about a
lot
> of tests to maintain.
> 
> I don't have a great plan for this. What if we moved all repaint tests to a
> repaint/ directory and just ran those directories as virtual tests suites? Do
> you have a feeling for how many failures we would need to initially add to
> TestExpectations for that to work?

I planned to move all repaint tests to paint/invalidation directory, but haven't
found a time to do it yet. It might be a proper time to do this after this CL.
When moving them, I would generate new baselines instead of adding them to
TestExpectations. We can verify the validity of the new baselines after we get
raster-invalidation raster-under-invalidation-checking work.

https://codereview.chromium.org/2307623002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/2307623002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h:352: union {
On 2016/09/08 01:36:23, pdr. wrote:
> This approach of using a union is very efficient but also very brittle. I can
> imagine a person reading this for the first time would have a difficult time
> understanding why a subclass like DrawingDisplayItem has two flavors in base
> class. The way we move DisplayItems out of the contiguous container and
replace
> them with an invalid DisplayItem was already a lot of mental overhead.
> 
> What do you think of a simpler approach where we use an auxiliary array and
> optimize later if needed? For example, before painting we could allocate a
flat
> array of size_t's that stores the index where the item has moved to.

Done.

[Xianzhu, 2016-09-08 03:01:47]

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

[commit-bot: I haz the power, 2016-09-08 03:02:03]

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-09-08 03:20:30]

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

[commit-bot: I haz the power, 2016-09-08 03:20:31]

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

[pdr., 2016-09-08 03:25:48]

On 2016/09/08 at 03:01:42, wangxianzhu wrote:
>
https://codereview.chromium.org/2307623002/diff/80001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):
> 
>
https://codereview.chromium.org/2307623002/diff/80001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:603:
fast/css-grid-layout/grid-item-z-index-change-repaint.html [ Failure ]
> On 2016/09/08 01:36:23, pdr. wrote:
> > On 2016/09/07 at 21:38:35, Xianzhu wrote:
> > > On 2016/09/07 20:49:02, chrishtr wrote:
> > > > These are marked as Failure because they invalidate
> > > > differently, right?
> > > 
> > > Yes.
> > > 
> > > > Should these be part of the virtual test
> > > > suite with different expectations perhaps?
> > > 
> > > There are also many other repaint tests having the similar issue. I think
it's
> > a good way to run them as virtual tests with new baselines.
> > 
> > There are about 426 repaint tests in fast/ (43 outside fast/repaint) and
another
> > 534 repaint tests in svg/ (490 outside svg/repaint) so we're talking about a
lot
> > of tests to maintain.
> > 
> > I don't have a great plan for this. What if we moved all repaint tests to a
> > repaint/ directory and just ran those directories as virtual tests suites?
Do
> > you have a feeling for how many failures we would need to initially add to
> > TestExpectations for that to work?
> 
> I planned to move all repaint tests to paint/invalidation directory, but
haven't found a time to do it yet. It might be a proper time to do this after
this CL. When moving them, I would generate new baselines instead of adding them
to TestExpectations. We can verify the validity of the new baselines after we
get raster-invalidation raster-under-invalidation-checking work.

This plan sounds good to me.

> 
>
https://codereview.chromium.org/2307623002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h (right):
> 
>
https://codereview.chromium.org/2307623002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h:352: union {
> On 2016/09/08 01:36:23, pdr. wrote:
> > This approach of using a union is very efficient but also very brittle. I
can
> > imagine a person reading this for the first time would have a difficult time
> > understanding why a subclass like DrawingDisplayItem has two flavors in base
> > class. The way we move DisplayItems out of the contiguous container and
replace
> > them with an invalid DisplayItem was already a lot of mental overhead.
> > 
> > What do you think of a simpler approach where we use an auxiliary array and
> > optimize later if needed? For example, before painting we could allocate a
flat
> > array of size_t's that stores the index where the item has moved to.
> 
> Done.

Thanks, I think your new patch looks good.

[Xianzhu, 2016-09-08 15:56:23]

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

[commit-bot: I haz the power, 2016-09-08 15:56:40]

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-09-08 16:50:01]

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

[commit-bot: I haz the power, 2016-09-08 16:50:02]

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

[chrishtr, 2016-09-08 20:30:08]

lgtm

[Xianzhu, 2016-09-08 23:01:25]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-09-08 23:01:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2307623002/#ps140001
(title: "Update spv2 expectations")

[commit-bot: I haz the power, 2016-09-08 23:02:58]

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-09-08 23:06:29]

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

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

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-09-08 23:32:45]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-09-08 23:32:45]

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

[commit-bot: I haz the power, 2016-09-08 23:33:14]

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-09-09 00:00:07]

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

[commit-bot: I haz the power, 2016-09-09 00:00:09]

Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[Xianzhu, 2016-09-09 00:34:40]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-09-09 00:35:23]

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-09-09 01:26:14]

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

[commit-bot: I haz the power, 2016-09-09 01:26:16]

Exceeded global retry quota

[Xianzhu, 2016-09-09 05:17:16]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-09-09 05:17:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2307623002/#ps180001
(title: "Remove duplicated spv2 expectation entries")

[commit-bot: I haz the power, 2016-09-09 05:17:32]

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-09-09 06:38:21]

Description was changed from

==========
[SPv2] Defer decision of raster invalidation after paint for changes of z-index,
transform, etc.

Previously we invalidate paint (which will also cause raster invalidation)
of the subtree if z-index, transform changed on an object which will not
be composited. This requires logic to precisely predict compositing status
which is fragile and causes unnecessary paint invalidations.

We don't need to invalidate paint of object which just changes z-index,
transform etc. We can invalidate raster if needed after painting.

If a cached display item is moved out of the original chunk, invalidate
raster for it on both the old chunk and the new chunk.

If a cached display item is moved within the chunk behind any other
cached display item, invalidate raster for it.

Otherwise we don't invalidate raster.

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

to

==========
[SPv2] Defer decision of raster invalidation after paint for changes of z-index,
transform, etc.

Previously we invalidate paint (which will also cause raster invalidation)
of the subtree if z-index, transform changed on an object which will not
be composited. This requires logic to precisely predict compositing status
which is fragile and causes unnecessary paint invalidations.

We don't need to invalidate paint of object which just changes z-index,
transform etc. We can invalidate raster if needed after painting.

If a cached display item is moved out of the original chunk, invalidate
raster for it on both the old chunk and the new chunk.

If a cached display item is moved within the chunk behind any other
cached display item, invalidate raster for it.

Otherwise we don't invalidate raster.

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

[commit-bot: I haz the power, 2016-09-09 06:38:23]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-09 06:40:19]

Description was changed from

==========
[SPv2] Defer decision of raster invalidation after paint for changes of z-index,
transform, etc.

Previously we invalidate paint (which will also cause raster invalidation)
of the subtree if z-index, transform changed on an object which will not
be composited. This requires logic to precisely predict compositing status
which is fragile and causes unnecessary paint invalidations.

We don't need to invalidate paint of object which just changes z-index,
transform etc. We can invalidate raster if needed after painting.

If a cached display item is moved out of the original chunk, invalidate
raster for it on both the old chunk and the new chunk.

If a cached display item is moved within the chunk behind any other
cached display item, invalidate raster for it.

Otherwise we don't invalidate raster.

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

to

==========
[SPv2] Defer decision of raster invalidation after paint for changes of z-index,
transform, etc.

Previously we invalidate paint (which will also cause raster invalidation)
of the subtree if z-index, transform changed on an object which will not
be composited. This requires logic to precisely predict compositing status
which is fragile and causes unnecessary paint invalidations.

We don't need to invalidate paint of object which just changes z-index,
transform etc. We can invalidate raster if needed after painting.

If a cached display item is moved out of the original chunk, invalidate
raster for it on both the old chunk and the new chunk.

If a cached display item is moved within the chunk behind any other
cached display item, invalidate raster for it.

Otherwise we don't invalidate raster.

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

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

[commit-bot: I haz the power, 2016-09-09 06:40:21]

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