cc: Release main thread earlier

cc: Release main thread earlier

Previously, the main thread would wait for the compositor thread to do a
good bit of work during its CommitComplete.  This work doesn't involve
the main thread and so there is no reason to not release it earlier.

However, releasing earlier creates raciness between
LayerTreeHost::CommitComplete and LayerTreeHostImpl::CommitComplete that
did not exist before.  Several failing tests were updated to try to take
this into account.

BUG=333013
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246829

[enne (OOO), 2014-01-15 00:36:04]

https://codereview.chromium.org/139053002/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_delegated.cc (left):

https://codereview.chromium.org/139053002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_delegated.cc:1927: virtual void DidCommit()
OVERRIDE {
Main thread DidCommit is now racy with respect to compositor thread activation. 
All these tests are now updated to just make sure that activation has taken
place during the commit, which I think is what they were trying to do.

https://codereview.chromium.org/139053002/diff/40001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/139053002/diff/40001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1025: inside_commit_ = false;
This variable is a little bit weird.  It's only read on the main thread, so it
needs to get updated before the lock is released.

Maybe it needs to be renamed for clarity, since in some ways the commit isn't
done for the compositor thread at this point.

[enne (OOO), 2014-01-15 00:37:11]

+alokp

Sorry for stepping on your toes here.  A bunch of folks kept coming up to me and
asking about this today, so I was poking at it while waiting on other reviews.

[alokp, 2014-01-15 00:46:40]

On 2014/01/15 00:37:11, enne wrote:
> +alokp
> 
> Sorry for stepping on your toes here.  A bunch of folks kept coming up to me
and
> asking about this today, so I was poking at it while waiting on other reviews.

I was attempting something similar:
https://codereview.chromium.org/139063002/

I started fixing the unit-tests just now. Thanks for letting me know.

[brianderson, 2014-01-15 00:59:37]

https://codereview.chromium.org/139053002/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/139053002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_delegated.cc:1898: SetFrameData(frame.Pass());
What is this extra case needed for?

https://codereview.chromium.org/139053002/diff/40001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/139053002/diff/40001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1025: inside_commit_ = false;
On 2014/01/15 00:36:04, enne wrote:
> This variable is a little bit weird.  It's only read on the main thread, so it
> needs to get updated before the lock is released.
> 
> Maybe it needs to be renamed for clarity, since in some ways the commit isn't
> done for the compositor thread at this point.

main_thread_inside_commit_?

[enne (OOO), 2014-01-15 01:21:49]

https://codereview.chromium.org/139053002/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/139053002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_delegated.cc:1898: SetFrameData(frame.Pass());
On 2014/01/15 00:59:38, Brian Anderson wrote:
> What is this extra case needed for?

This is needed to kick off the UnusedResourcesAreAvailable call below by
clearing the frame data.

https://codereview.chromium.org/139053002/diff/40001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/139053002/diff/40001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1025: inside_commit_ = false;
On 2014/01/15 00:59:38, Brian Anderson wrote:
> On 2014/01/15 00:36:04, enne wrote:
> > This variable is a little bit weird.  It's only read on the main thread, so
it
> > needs to get updated before the lock is released.
> > 
> > Maybe it needs to be renamed for clarity, since in some ways the commit
isn't
> > done for the compositor thread at this point.
> 
> main_thread_inside_commit_?

Done.

[brianderson, 2014-01-15 01:39:02]

lgtm

[danakj, 2014-01-15 19:05:38]

LGTM2

https://codereview.chromium.org/139053002/diff/100002/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/139053002/diff/100002/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_delegated.cc:1896:
scoped_ptr<DelegatedFrameData> frame =
Add a comment explaining this case please.

[enne (OOO), 2014-01-15 19:34:24]

Also rebased on top of https://codereview.chromium.org/139553003/

https://codereview.chromium.org/139053002/diff/100002/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/139053002/diff/100002/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_delegated.cc:1896:
scoped_ptr<DelegatedFrameData> frame =
On 2014/01/15 19:05:38, danakj wrote:
> Add a comment explaining this case please.

Done.

[commit-bot: I haz the power, 2014-01-16 20:07:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/139053002/300001

[commit-bot: I haz the power, 2014-01-17 02:26:20]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2014-01-17 02:36:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/139053002/300001

[commit-bot: I haz the power, 2014-01-17 08:52:37]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[enne (OOO), 2014-01-17 21:11:06]

I clearly didn't catch all the unit tests here.  I've locally forced the
ordering of CommitComplete to be opposite of what it was before to flush out bad
tests and there's a whole bunch more to fix before this can land.

[enne (OOO), 2014-01-23 21:40:11]

PTAL at latest changes.  I updated a few more tests to work if CommitComplete
happens in either order.  Changes are pretty small.

The moral of the story is: do not track number of commits in
LayerTreeTest::DidCommit and attempt to do something with it in
CommitCompleteOnThread.  If you need a commit number, just use the source frame
number.  If your test aborts and you really need to count commit numbers, do it
in WillCommit which happens regardless of aborting and always happens before
either DidCommit or CommitCompleteOnThread.

I tried to increment the frame number even when the commit was aborted (to fix
LayerTreeHostTestMemoryLimits), but this broke too many other assumptions in
even more tests, and so I left that terrifying pit of yaksnakes alone.  Sorry. 
:(

[brianderson, 2014-01-23 22:48:48]

https://codereview.chromium.org/139053002/diff/800001/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/139053002/diff/800001/cc/test/layer_tree_test...
cc/test/layer_tree_test.h:184: int
LastCommittedSourceFrameNumber(LayerTreeHostImpl* impl) const;
Nifty!

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

https://codereview.chromium.org/139053002/diff/800001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:4496: num_commits_++;
To handle the aborted commits, could we keep two counters instead? One for the
main commit count and another for the impl commit count. That way, it's easier
to reason about the thread safety.

[enne (OOO), 2014-01-23 23:01:36]

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

https://codereview.chromium.org/139053002/diff/800001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:4496: num_commits_++;
On 2014/01/23 22:48:49, Brian Anderson wrote:
> To handle the aborted commits, could we keep two counters instead? One for the
> main commit count and another for the impl commit count. That way, it's easier
> to reason about the thread safety.

WillCommit is called when the main thread is blocked, so I'm not sure what the
thread safety issue is here.

[brianderson, 2014-01-23 23:48:36]

On 2014/01/23 23:01:36, enne wrote:
>
https://codereview.chromium.org/139053002/diff/800001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_unittest.cc (right):
> 
>
https://codereview.chromium.org/139053002/diff/800001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_unittest.cc:4496: num_commits_++;
> On 2014/01/23 22:48:49, Brian Anderson wrote:
> > To handle the aborted commits, could we keep two counters instead? One for
the
> > main commit count and another for the impl commit count. That way, it's
easier
> > to reason about the thread safety.
> 
> WillCommit is called when the main thread is blocked, so I'm not sure what the
> thread safety issue is here.

Ok. lgtm.

I was thinking of future cases where we start commits before activation, but
that's not likely to be the default behavior for a long time.

[commit-bot: I haz the power, 2014-01-24 00:08:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/139053002/800001

[commit-bot: I haz the power, 2014-01-24 01:54:48]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2014-01-24 01:59:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/139053002/800001

[commit-bot: I haz the power, 2014-01-24 10:22:14]

Change committed as 246829