Remove begin/end subseq. display items, and store on PaintController instead.

Remove begin/end subseq. display items, and store on PaintController instead.

Subsequence display items cause complications for the SPv2 compositing algorithm
because they confuse the PaintChunker. Also, they are the last remaining
paired display item in SPv2 that blocks simplifying the display list format.

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

Review-Url: https://codereview.chromium.org/2793233002
Cr-Commit-Position: refs/heads/master@{#462735}
Committed: https://chromium.googlesource.com/chromium/src/+/256619b40e56be177620efb04b57086b2f6bcbed

[chrishtr, 2017-04-04 00:25:16]

Description was changed from

==========
none


none


none


none


none


none


none


none


none

BUG=
==========

to

==========
none


none


none


none


none


none


none


none


none

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

[chrishtr, 2017-04-04 00:36:46]

Description was changed from

==========
none


none


none


none


none


none


none


none


none

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

to

==========
Remove begin/end subseq. display items, and store on PaintController instead.

Subsequence display items cause complications for the SPv2 compositing algorithm
because they confuse the PaintChunker. Also, they are the last remaining
paired display item in SPv2 that blocks simplifying the display list format.

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

[chrishtr, 2017-04-04 00:41:28]

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

[commit-bot: I haz the power, 2017-04-04 00:41:58]

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

[chrishtr, 2017-04-04 18:12:04]

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

[commit-bot: I haz the power, 2017-04-04 18:12:32]

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

[chrishtr, 2017-04-04 18:12:41]

Description was changed from

==========
Remove begin/end subseq. display items, and store on PaintController instead.

Subsequence display items cause complications for the SPv2 compositing algorithm
because they confuse the PaintChunker. Also, they are the last remaining
paired display item in SPv2 that blocks simplifying the display list format.

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

to

==========
Remove begin/end subseq. display items, and store on PaintController instead.

Subsequence display items cause complications for the SPv2 compositing algorithm
because they confuse the PaintChunker. Also, they are the last remaining
paired display item in SPv2 that blocks simplifying the display list format.

BUG=692614

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

[chrishtr, 2017-04-04 18:12:47]

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

[chrishtr, 2017-04-04 18:12:48]

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

https://codereview.chromium.org/2793233002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:1171:
EXPECT_EQ(0, numSequentialMatches());
I'm not quite sure these are correct. Could you check?

https://codereview.chromium.org/2793233002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:1552:
EXPECT_EQ(0, numSequentialMatches());
I'm not quite sure these are correct. Could you check?

[chrishtr, 2017-04-04 20:21:16]

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

[commit-bot: I haz the power, 2017-04-04 20:22:16]

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, 2017-04-04 22:13:26]

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

[commit-bot: I haz the power, 2017-04-04 22:13:27]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[chrishtr, 2017-04-05 21:10:49]

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

[commit-bot: I haz the power, 2017-04-05 21:11:52]

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, 2017-04-05 21:17:08]

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

[commit-bot: I haz the power, 2017-04-05 21:17:09]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[chrishtr, 2017-04-05 21:39:16]

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

[chrishtr, 2017-04-05 21:39:38]

Updated to fix a bug in under-invalidation checking in PaintController. The
change also
simplifies the patch a little bit.

[commit-bot: I haz the power, 2017-04-05 21:39:54]

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

[Xianzhu, 2017-04-05 22:39:18]

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

https://codereview.chromium.org/2793233002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:1171:
EXPECT_EQ(0, numSequentialMatches());
On 2017/04/04 18:12:48, chrishtr wrote:
> I'm not quite sure these are correct. Could you check?

These look good if stat of subsequences are excluded. These are to ensure we use
the best performance path when possible (sequential match), and match
out-of-order (using HashMap::find) and index items (using HashMap::add) only
when needed.

https://codereview.chromium.org/2793233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):

https://codereview.chromium.org/2793233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:165:
m_newCachedSubsequences.insert(&client, SubsequenceMarkers(start, end));
I'm afraid this may degrade performance. Previously we don't index at all during
the first paint, and only add out-of-order subsequences into the index during
the next paint. Now we add all subsequences into the map during the current
paint and always look up the map for cached subsequences.

Let's look at the performance data first before deciding if we need to improve
it.

https://codereview.chromium.org/2793233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/2793233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:217: ,
Nit: Move ',' before #endif and the ',' before #ifndef.

https://codereview.chromium.org/2793233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp
(right):

https://codereview.chromium.org/2793233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:1450:
EXPECT_EQ(0ull, markers->start);
Nit: I think you can just use 0u which will be upgraded to size_t if needed.
0ull also can't ensure the same type as size_t.

[commit-bot: I haz the power, 2017-04-05 22:50:36]

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

[commit-bot: I haz the power, 2017-04-05 22:50:36]

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, 2017-04-06 00:05:29]

https://codereview.chromium.org/2793233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):

https://codereview.chromium.org/2793233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:165:
m_newCachedSubsequences.insert(&client, SubsequenceMarkers(start, end));
On 2017/04/05 at 22:39:17, Xianzhu wrote:
> I'm afraid this may degrade performance. Previously we don't index at all
during the first paint, and only add out-of-order subsequences into the index
during the next paint. Now we add all subsequences into the map during the
current paint and always look up the map for cached subsequences.
> 
> Let's look at the performance data first before deciding if we need to improve
it.

Ok.

You're right about indexing, but OTOH there are many fewer subsequences than
display items.

Alternatively we could use a vector.

https://codereview.chromium.org/2793233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/2793233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:217: ,
On 2017/04/05 at 22:39:17, Xianzhu wrote:
> Nit: Move ',' before #endif and the ',' before #ifndef.

Done.

https://codereview.chromium.org/2793233002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp
(right):

https://codereview.chromium.org/2793233002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:1450:
EXPECT_EQ(0ull, markers->start);
On 2017/04/05 at 22:39:18, Xianzhu wrote:
> Nit: I think you can just use 0u which will be upgraded to size_t if needed.
0ull also can't ensure the same type as size_t.

Done.

[chrishtr, 2017-04-06 01:03:24]

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

[commit-bot: I haz the power, 2017-04-06 01:03:58]

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

[chrishtr, 2017-04-06 01:04:19]

https://codereview.chromium.org/2793233002/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
(right):

https://codereview.chromium.org/2793233002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2:1363:
Bug(none) virtual/threaded/animations/3d/change-transform-in-end-event.html [
Failure ]
It appears that removing the subsequence display items changes compositing
decisions, which in turn has some other bug (or that is my assumption).
These tests still fail if I force subsequence caching off, so I don't think
my CL broke them.

[commit-bot: I haz the power, 2017-04-06 04:03:42]

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

[commit-bot: I haz the power, 2017-04-06 04:03:43]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2017-04-06 17:46:57]

Hi, does this CL look ok?

[Xianzhu, 2017-04-06 18:22:25]

lgtm with one comments about CHECK_DISPLAY_ITEM_CLIENT_ALIVENESS.

https://codereview.chromium.org/2793233002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(left):

https://codereview.chromium.org/2793233002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:189: }
Keep this.

https://codereview.chromium.org/2793233002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h (left):

https://codereview.chromium.org/2793233002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:380: #endif
I think we should keep this which is useful for detecting short-lived clients.

We'll need new methods to be called from SubsequenceRecorder:

#if CHECK_DISPLAY_ITEM_CLIENT_ALIVENESS
  void beginSubsequence(const DisplayItemClient& client) {
    m_currentSubsequenceClients.push_back(&client);
  }

  void endSubsequence() {
    m_currentSubsequence.pop_back();
  }
#endif

[Xianzhu, 2017-04-06 18:30:44]

https://codereview.chromium.org/2793233002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(left):

https://codereview.chromium.org/2793233002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:199:
#endif
The whole block (excluding line 192-196) can be moved into a new method
beginShouldKeepAlive() which is called from here and beginSubsequence() (to
check for subsequence client aliveness).

[chrishtr, 2017-04-06 20:01:14]

https://codereview.chromium.org/2793233002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(left):

https://codereview.chromium.org/2793233002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:189: }
On 2017/04/06 at 18:22:25, Xianzhu wrote:
> Keep this.

Done.

https://codereview.chromium.org/2793233002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:199:
#endif
On 2017/04/06 at 18:30:43, Xianzhu wrote:
> The whole block (excluding line 192-196) can be moved into a new method
beginShouldKeepAlive() which is called from here and beginSubsequence() (to
check for subsequence client aliveness).

Done.

https://codereview.chromium.org/2793233002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h (left):

https://codereview.chromium.org/2793233002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:380: #endif
On 2017/04/06 at 18:22:25, Xianzhu wrote:
> I think we should keep this which is useful for detecting short-lived clients.
> 
> We'll need new methods to be called from SubsequenceRecorder:
> 
> #if CHECK_DISPLAY_ITEM_CLIENT_ALIVENESS
>   void beginSubsequence(const DisplayItemClient& client) {
>     m_currentSubsequenceClients.push_back(&client);
>   }
> 
>   void endSubsequence() {
>     m_currentSubsequence.pop_back();
>   }
> #endif

Done.

[chrishtr, 2017-04-06 20:01:31]

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

[commit-bot: I haz the power, 2017-04-06 20:02:54]

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

[chrishtr, 2017-04-06 20:17:38]

The CQ bit was unchecked by chrishtr@chromium.org

[chrishtr, 2017-04-06 20:17:40]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-04-06 20:17:40]

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

[commit-bot: I haz the power, 2017-04-06 20:18:40]

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, 2017-04-06 21:01:31]

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

[commit-bot: I haz the power, 2017-04-06 21:01:33]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2017-04-06 21:18:30]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-04-06 21:18:31]

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

[commit-bot: I haz the power, 2017-04-06 21:20:15]

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, 2017-04-06 22:21:43]

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

[commit-bot: I haz the power, 2017-04-06 22:21:45]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chrishtr, 2017-04-06 23:19:06]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-04-06 23:19:07]

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

[commit-bot: I haz the power, 2017-04-06 23:20:03]

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, 2017-04-07 01:29:30]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1491520746968670,
"parent_rev": "5cba6a6eeb7463afbc2445f05914f26a66874454", "commit_rev":
"256619b40e56be177620efb04b57086b2f6bcbed"}

[commit-bot: I haz the power, 2017-04-07 01:30:21]

Description was changed from

==========
Remove begin/end subseq. display items, and store on PaintController instead.

Subsequence display items cause complications for the SPv2 compositing algorithm
because they confuse the PaintChunker. Also, they are the last remaining
paired display item in SPv2 that blocks simplifying the display list format.

BUG=692614

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

to

==========
Remove begin/end subseq. display items, and store on PaintController instead.

Subsequence display items cause complications for the SPv2 compositing algorithm
because they confuse the PaintChunker. Also, they are the last remaining
paired display item in SPv2 that blocks simplifying the display list format.

BUG=692614

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2793233002
Cr-Commit-Position: refs/heads/master@{#462735}
Committed:
https://chromium.googlesource.com/chromium/src/+/256619b40e56be177620efb04b57...
==========

[commit-bot: I haz the power, 2017-04-07 01:30:22]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/256619b40e56be177620efb04b57...

[foolip, 2017-04-07 05:12:52]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2804883006/ by foolip@chromium.org.

The reason for reverting is:
PaintControllerUnderInvalidationTest.LessDrawingInSubsequence is failing on many
builders.

Error from WebKit Linux Trusty:
PaintControllerUnderInvalidationTest.LessDrawingInSubsequence (run #1):
[ RUN      ] PaintControllerUnderInvalidationTest.LessDrawingInSubsequence

[WARNING] ../../testing/gtest/src/gtest-death-test.cc:834:: Death tests use
fork(), which is unsafe particularly in a threaded context. For this test,
Google Test detected 2 threads.
../../third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:2331:
Failure
Death test: testLessDrawingInSubsequence()
    Result: failed to die.
 Error msg:
[  DEATH   ]
[11609:11609:0406/212249.088581:18118180277:ERROR:PaintController.cpp(827)] "(In
cached subsequence of first)" under-invalidation: new subsequence wrong length
[  DEATH   ]
[11609:11609:0406/212249.088701:18118180330:ERROR:PaintController.cpp(828)]
Subsequence client: "first"
[  DEATH   ]
[11609:11609:0406/212249.088713:18118180341:ERROR:PaintController.cpp(832)] Run
debug build to get more details.
[  DEATH   ]
[11609:11609:0406/212249.088718:18118180346:ERROR:PaintController.cpp(834)] See
http://crbug.com/619103.
[  DEATH   ]
[  FAILED  ] PaintControllerUnderInvalidationTest.LessDrawingInSubsequence.