cc: Abort frame when becoming invisible and waiting for ready to draw.

cc: Abort frame when becoming invisible and waiting for ready to draw.

The scheduler didn't work correctly when becoming invisible if it was
waiting for ready to draw. It wouldn't end the current frame which meant
that it would never get the ready to draw unless we did an extra
PrepareTiles on becoming visible again. This PrepareTiles happened
outside of the impl frame which meant that it would contend for cpu time
with v8 idle time work. This caused a v8 regression which couldn't be
fixed except by removing the extra PrepareTiles.

BUG=510295
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[sunnyps, 2015-07-23 00:08:22]

sunnyps@chromium.org changed reviewers:
+ brianderson@chromium.org, danakj@chromium.org

[sunnyps, 2015-07-23 00:08:22]

PTAL

[brianderson, 2015-07-23 02:00:23]

Only have a few comments regarding testing, but the rest of the patch looks
good.

https://codereview.chromium.org/1247033007/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1247033007/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:224: PostSetVisibleToMainThread(true);
I think it's possible this races with the DidSetVisibleOnImplTree(false) we'd
expect to get the main thread round trip of PostSetVisibleToMainThread(false).
Is that okay?

https://codereview.chromium.org/1247033007/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:235: EXPECT_LE(1, commit_count_);
I'm not sure if this test has a race between READY_TO_ACTIVATE and
DidSetVisibleOnImplTree(false).

Could you override DrawLayersOnThread and verify this test only draws after
toggled_visibility_ is true?

https://codereview.chromium.org/1247033007/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:320: class
LayerTreeHostTestReadyToDrawVisibility
See comments about other test.

https://codereview.chromium.org/1247033007/diff/1/tools/valgrind/gtest_exclud...
File tools/valgrind/gtest_exclude/cc_unittests.gtest-drmemory_win32.txt (right):

https://codereview.chromium.org/1247033007/diff/1/tools/valgrind/gtest_exclud...
tools/valgrind/gtest_exclude/cc_unittests.gtest-drmemory_win32.txt:14:
LayerTreeHostTestReadyToDrawVisibility.*
Is this the only way to avoid the flake for now?

[sunnyps, 2015-07-23 22:20:44]

PTAL

Removed the LayerTreeHostTestReadyToActivateVisibility because there's literally
no way to make it race free without adding a ton of complexity. In any case it's
not being affected by the changes being made here. I've kept
LayerTreeHostTestReadyToDrawVisibility and made it race-free.

https://codereview.chromium.org/1247033007/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1247033007/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:224: PostSetVisibleToMainThread(true);
On 2015/07/23 02:00:23, brianderson wrote:
> I think it's possible this races with the DidSetVisibleOnImplTree(false) we'd
> expect to get the main thread round trip of PostSetVisibleToMainThread(false).
> Is that okay?

That works for the ready to draw test because that's only for STP but it won't
work for this test. I'm removing this test because there's no way to make this
race free without a lot of complexity. Also this isn't being addressed by the CL
anyway.

https://codereview.chromium.org/1247033007/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:235: EXPECT_LE(1, commit_count_);
On 2015/07/23 02:00:23, brianderson wrote:
> I'm not sure if this test has a race between READY_TO_ACTIVATE and
> DidSetVisibleOnImplTree(false).
> 

See reply above.

https://codereview.chromium.org/1247033007/diff/1/tools/valgrind/gtest_exclud...
File tools/valgrind/gtest_exclude/cc_unittests.gtest-drmemory_win32.txt (right):

https://codereview.chromium.org/1247033007/diff/1/tools/valgrind/gtest_exclud...
tools/valgrind/gtest_exclude/cc_unittests.gtest-drmemory_win32.txt:14:
LayerTreeHostTestReadyToDrawVisibility.*
On 2015/07/23 02:00:23, brianderson wrote:
> Is this the only way to avoid the flake for now?

It isn't a flake. The test just takes too long on drmemory_full which is about
10x slower than a debug build. See the other tests which are disabled above this
one.

[sunnyps, 2015-07-23 22:22:03]

Oh and BTW this CL does fix the v8 idle time regression. Results are still noisy
but some key indicators - sites which regressed a lot are in the same ballpark
as before we put in the extra PrepareTiles.

[brianderson, 2015-07-23 22:58:15]

lgtm

https://codereview.chromium.org/1247033007/diff/1/tools/valgrind/gtest_exclud...
File tools/valgrind/gtest_exclude/cc_unittests.gtest-drmemory_win32.txt (right):

https://codereview.chromium.org/1247033007/diff/1/tools/valgrind/gtest_exclud...
tools/valgrind/gtest_exclude/cc_unittests.gtest-drmemory_win32.txt:14:
LayerTreeHostTestReadyToDrawVisibility.*
On 2015/07/23 22:20:44, sunnyps wrote:
> On 2015/07/23 02:00:23, brianderson wrote:
> > Is this the only way to avoid the flake for now?
> 
> It isn't a flake. The test just takes too long on drmemory_full which is about
> 10x slower than a debug build. See the other tests which are disabled above
this
> one.

Understood.

https://codereview.chromium.org/1247033007/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1247033007/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:287: layer_tree_host()->SetVisible(false);
EXPECT_FALSE(host_impl->visible()) here?

https://codereview.chromium.org/1247033007/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:294: layer_tree_host()->SetVisible(true);
EXPECT_TRUE(host_impl->visible()) here?

[brianderson, 2015-07-23 23:06:17]

On 2015/07/23 22:22:03, sunnyps wrote:
> Oh and BTW this CL does fix the v8 idle time regression. Results are still
noisy
> but some key indicators - sites which regressed a lot are in the same ballpark
> as before we put in the extra PrepareTiles.

Excellent.

[sunnyps, 2015-07-23 23:10:20]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-07-23 23:10:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/1247033007/#ps40001
(title: "address comments")

[commit-bot: I haz the power, 2015-07-23 23:10:37]

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

[sunnyps, 2015-07-23 23:10:50]

https://codereview.chromium.org/1247033007/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1247033007/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:287: layer_tree_host()->SetVisible(false);
On 2015/07/23 22:58:15, brianderson wrote:
> EXPECT_FALSE(host_impl->visible()) here?

Done.

https://codereview.chromium.org/1247033007/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:294: layer_tree_host()->SetVisible(true);
On 2015/07/23 22:58:15, brianderson wrote:
> EXPECT_TRUE(host_impl->visible()) here?

Done.

[commit-bot: I haz the power, 2015-07-24 03:58:00]

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

[commit-bot: I haz the power, 2015-07-24 03:58:00]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[sunnyps, 2015-07-24 19:38:26]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-07-24 19:38:36]

The CQ bit was unchecked by sunnyps@chromium.org

[sunnyps, 2015-07-24 23:59:19]

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

[commit-bot: I haz the power, 2015-07-24 23:59:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1247033007/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1247033007/60001

[sunnyps, 2015-07-25 00:14:04]

The CQ bit was unchecked by sunnyps@chromium.org

[sunnyps, 2015-07-25 00:14:09]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-07-25 00:14:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/1247033007/#ps60001
(title: "Remove race from test")

[commit-bot: I haz the power, 2015-07-25 00:14:30]

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

[commit-bot: I haz the power, 2015-07-25 01:01:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-25 01:02:06]

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