Simplify subtree (now subsequence) caching

Simplify subtree (now subsequence) caching

We no longer need paint-phase-based subtree display items because we'll
use paint layers as units of subtrees. This greatly simplifies subtree
caching.

Renamed subtree to subsequence.

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

[Xianzhu, 2015-08-25 22:40:43]

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

[chrishtr, 2015-08-25 23:48:19]

https://codereview.chromium.org/1313223002/diff/1/Source/platform/graphics/pa...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/1313223002/diff/1/Source/platform/graphics/pa...
Source/platform/graphics/paint/DisplayItem.h:170: CachedSubtree,
How about CachedStackingContext or some other more precise name? Alternatively,
it could be CachedSubsequence, since from another point of view, subtree is an
implementation detail.

https://codereview.chromium.org/1313223002/diff/1/Source/platform/graphics/pa...
File Source/platform/graphics/paint/DisplayItemListTest.cpp (right):

https://codereview.chromium.org/1313223002/diff/1/Source/platform/graphics/pa...
Source/platform/graphics/paint/DisplayItemListTest.cpp:591: EXPECT_EQ((size_t)7,
newPaintListBeforeUpdate().size());
Check that newPaintListBeforeUpdate() has cached subtree items?

https://codereview.chromium.org/1313223002/diff/1/Source/platform/graphics/pa...
File Source/platform/graphics/paint/SubtreeRecorder.cpp (left):

https://codereview.chromium.org/1313223002/diff/1/Source/platform/graphics/pa...
Source/platform/graphics/paint/SubtreeRecorder.cpp:44: else if
(m_displayItemList->lastDisplayItemIsNoopBegin())
The no-op optimization is no longer useful?

[chrishtr, 2015-08-25 23:48:50]

Also, have you tried implementingng the business logic for stacking context DPLs
to see if it passes all layout tests?

[Xianzhu, 2015-08-26 16:47:38]

On 2015/08/25 23:48:50, chrishtr wrote:
> Also, have you tried implementingng the business logic for stacking context
DPLs
> to see if it passes all layout tests?

I'm still working on this.

[Xianzhu, 2015-08-26 18:21:03]

Patchset #2 (id:20001) has been deleted

[Xianzhu, 2015-08-26 23:21:06]

Tried subsequence caching with painting with several websites and it seems
working well.

Didn't run layout tests because enabling slimming paint v2 already caused too
many failures. It seems that we need a baseline of spv2 layout tests asap.

https://codereview.chromium.org/1313223002/diff/1/Source/platform/graphics/pa...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/1313223002/diff/1/Source/platform/graphics/pa...
Source/platform/graphics/paint/DisplayItem.h:170: CachedSubtree,
On 2015/08/25 23:48:19, chrishtr wrote:
> How about CachedStackingContext or some other more precise name?
Alternatively,
> it could be CachedSubsequence, since from another point of view, subtree is an
> implementation detail.

CachedSubsequence sgtm.

https://codereview.chromium.org/1313223002/diff/1/Source/platform/graphics/pa...
File Source/platform/graphics/paint/DisplayItemListTest.cpp (right):

https://codereview.chromium.org/1313223002/diff/1/Source/platform/graphics/pa...
Source/platform/graphics/paint/DisplayItemListTest.cpp:591: EXPECT_EQ((size_t)7,
newPaintListBeforeUpdate().size());
On 2015/08/25 23:48:19, chrishtr wrote:
> Check that newPaintListBeforeUpdate() has cached subtree items?

Done.

https://codereview.chromium.org/1313223002/diff/1/Source/platform/graphics/pa...
File Source/platform/graphics/paint/SubtreeRecorder.cpp (left):

https://codereview.chromium.org/1313223002/diff/1/Source/platform/graphics/pa...
Source/platform/graphics/paint/SubtreeRecorder.cpp:44: else if
(m_displayItemList->lastDisplayItemIsNoopBegin())
On 2015/08/25 23:48:19, chrishtr wrote:
> The no-op optimization is no longer useful?

Right. The situation is similar to that of empty drawing display items:
- Empty subtrees will seldom happen when we let DPL issue them;
- CachedSubtrees without previously cached empty subtrees will break
synchronized matching of the merge algorithm.

[chrishtr, 2015-08-26 23:34:23]

https://codereview.chromium.org/1313223002/diff/50015/Source/platform/graphic...
File Source/platform/graphics/paint/SubsequenceRecorder.cpp (right):

https://codereview.chromium.org/1313223002/diff/50015/Source/platform/graphic...
Source/platform/graphics/paint/SubsequenceRecorder.cpp:18: // The conditions of
returning false is not complete for a specific client.
Move to .h file, and suggest changing as described in a comment there.

https://codereview.chromium.org/1313223002/diff/50015/Source/platform/graphic...
File Source/platform/graphics/paint/SubsequenceRecorder.h (right):

https://codereview.chromium.org/1313223002/diff/50015/Source/platform/graphic...
Source/platform/graphics/paint/SubsequenceRecorder.h:17: // for the paint phase.
This comment is not quite accurate, because it depends on the use. Maybe just
say that the SubsequenceRecorder records BeginSubsequenceDisplayItem and
EndSubsequenceDisplayItem sentinels at either end of a continguous sequence
of DisplayItems, and supports caching via a CachedDisplayItem with the
CachedSubsequence DisplayItem type.

Also note that useCachedSubsequenceIfPossible is not sufficient to determine
whether a CachedSubsequence can be used. In particular, the client is
responsible for checking that none of the DisplayItemClients that contribute to
the subsequence have been invalidated.

[Xianzhu, 2015-08-26 23:42:36]

https://codereview.chromium.org/1313223002/diff/50015/Source/platform/graphic...
File Source/platform/graphics/paint/SubsequenceRecorder.cpp (right):

https://codereview.chromium.org/1313223002/diff/50015/Source/platform/graphic...
Source/platform/graphics/paint/SubsequenceRecorder.cpp:18: // The conditions of
returning false is not complete for a specific client.
On 2015/08/26 23:34:23, chrishtr wrote:
> Move to .h file, and suggest changing as described in a comment there.

Done.

https://codereview.chromium.org/1313223002/diff/50015/Source/platform/graphic...
File Source/platform/graphics/paint/SubsequenceRecorder.h (right):

https://codereview.chromium.org/1313223002/diff/50015/Source/platform/graphic...
Source/platform/graphics/paint/SubsequenceRecorder.h:17: // for the paint phase.
On 2015/08/26 23:34:23, chrishtr wrote:
> This comment is not quite accurate, because it depends on the use. Maybe just
> say that the SubsequenceRecorder records BeginSubsequenceDisplayItem and
> EndSubsequenceDisplayItem sentinels at either end of a continguous sequence
> of DisplayItems, and supports caching via a CachedDisplayItem with the
> CachedSubsequence DisplayItem type.
> 
> Also note that useCachedSubsequenceIfPossible is not sufficient to determine
> whether a CachedSubsequence can be used. In particular, the client is
> responsible for checking that none of the DisplayItemClients that contribute
to
> the subsequence have been invalidated.

The new comment looks great. Thanks.

Done.

[chrishtr, 2015-08-27 00:03:30]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-08-27 00:03:30]

lgtm

[commit-bot: I haz the power, 2015-08-27 00:03:47]

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

[commit-bot: I haz the power, 2015-08-27 03:03:07]

Committed patchset #4 (id:70001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201282