cc: Adding DidFinishImplFrame to LTHI.

cc: Adding DidFinishImplFrame to LTHI.

This change moves the responsibility of clearing state inside the LTHI after an
impl frame is finished from the thread proxies to the LTHI class. This makes
WillBeginImplFrame and DidFinishImplFrame a logical pair.

This CL also;
 * Adds a test that checks the number of WillBeginImplFrame
   calls matches the DidFinishImplFrame calls.
 * Cleans up classes in cc/test/layer_tree_test.h around WillBeginImplFrame.
 * Removes the UpdateCurrentBeginFrameArgs / ResetCurrentBeginFrameArgs methods.
 * Fixes a bug where STP was calling ResetCurrentBeginFrameArgs twice every frame.

BUG=346230, 481810
R=brianderson,enne

Committed: https://crrev.com/2caee4f89de17858822c11379da9c6cd80f90708
Cr-Commit-Position: refs/heads/master@{#328689}

Committed: https://crrev.com/51693e3839cf85fad73979258c00e5964d627240
Cr-Commit-Position: refs/heads/master@{#328883}

[mithro-old, 2015-04-29 00:32:29]

mithro@mithis.com changed reviewers:
+ brianderson@chromium.org, enne@chromium.org

[mithro-old, 2015-04-29 00:46:39]

Hi Brian / Enne,

This patch came out of trying to write a test for the fix
https://codereview.chromium.org/1099703004/ -- cc: Make
SingleThreadProxy::CompositeImmediately match scheduler path. (There are also a
couple of other related CLs from trying to write that test I'll upload shortly).

The basic aim of this patch is to have the LTHI understand the "impl frame
period".

Previously the LTHI only knew about the start of an impl frame, not the end. The
end of an impl frame was always signaled by the scheduler, it just never made it
all the way to the LTHI.

Once LTHI understands the impl frame period, it becomes apparent that the
methods to set/reset the current begin frame args are no longer needed. Removing
these methods (or making them internal only) has been a long term goal because
of the huge potential for them to be abused.

This patch also does a small cleanup of layer_tree_test.xx as needed to
implement a test checking the begin / end messages.

Thanks for your review.

Tim 'mithro' Ansell

[danakj, 2015-04-30 18:43:37]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-04-30 18:43:38]

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

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5522: did_begin_impl_frame_deadline_count_
+ 1);
nit: reverse the order of the increment with this comparison and you can just
compare EQ instead of EQ+1 which is a bit nicer to read

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5526: // get one more impl frames than the
number of commits requested.
can you say why?

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5527: if
(did_begin_impl_frame_deadline_count_ < kExpectedNumImplFrames - 1)
else EndTest()?

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5531: void
SendBeginMainFrameNotExpectedSoon() override { EndTest(); }
prefer to do this as else to PostSNC

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5537: // one more frame then
expected.Possibly related to
spacing

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5539: //
EXPECT_EQ(will_begin_impl_frame_count_, kExpectedNumImplFrames);
You can ask if there's a thread and compare against different values here
instead of just not checking

[sunnyps, 2015-05-01 00:31:43]

sunnyps@chromium.org changed reviewers:
+ sunnyps@chromium.org

[sunnyps, 2015-05-01 00:31:44]

I like the patch overall. Can you please rename DidBeginImplFrameDeadline to
DidFinishImplFrame everywhere? The deadline is an internal detail of the
scheduler implementation, I would like if we didn't leak that information.

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

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5536: // TODO(mithro): Figure out why the
multithread version of this test has
I've had issue before too - I think it has something to do with activation and
the scheduler's deadline racing in the multithread version i.e. activation
doesn't happen in time for the deadline.

In any case, instead of expecting a fixed number of frames why not wait until a
full frame is drawn and then check if the number of WillBeginImplFrames is equal
to DidBeginImplFrameDeadlines and not worry about the total number of frames.

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:722: tracked_objects::ScopedTracker
tracking_profile7(
tracking_profile6?

[mithro-old, 2015-05-01 02:52:36]

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

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5522: did_begin_impl_frame_deadline_count_
+ 1);
On 2015/04/30 18:43:38, danakj wrote:
> nit: reverse the order of the increment with this comparison and you can just
> compare EQ instead of EQ+1 which is a bit nicer to read

Done. I was thinking of it like "do the checks first, then do any updates".

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5526: // get one more impl frames than the
number of commits requested.
On 2015/04/30 18:43:38, danakj wrote:
> can you say why?

Done.

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5527: if
(did_begin_impl_frame_deadline_count_ < kExpectedNumImplFrames - 1)
On 2015/04/30 18:43:38, danakj wrote:
> else EndTest()?

Putting the EndTest inside DidBeginImplFrameDeadline makes the final AfterCheck
begin/end equality test impossible because there becomes a race condition
between ending the test and starting another frame. For the same reason checking
the actual number of frames processed becomes flaky too.

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5531: void
SendBeginMainFrameNotExpectedSoon() override { EndTest(); }
On 2015/04/30 18:43:38, danakj wrote:
> prefer to do this as else to PostSNC

Using SendBeginMainFrameNotExpectedSoon guarantees that we are not going to get
any more ImplFrames and that the state is now idle. As there is no way in the
test to get out of the idle state, this removes the race condition problem.

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5536: // TODO(mithro): Figure out why the
multithread version of this test has
On 2015/05/01 00:31:43, sunnyps wrote:
> I've had issue before too - I think it has something to do with activation and
> the scheduler's deadline racing in the multithread version i.e. activation
> doesn't happen in time for the deadline.
> 
> In any case, instead of expecting a fixed number of frames why not wait until
a
> full frame is drawn and then check if the number of WillBeginImplFrames is
equal
> to DidBeginImplFrameDeadlines and not worry about the total number of frames.

The number of WillBeginImplFrames equal to the DidBeginImplFrameDeadlines check
is on line 5534.

The reason I want to check the final count is because the number of frames
should be deterministic. The fact it is behaves differently on the multithread
verse single thread proxy. This is a real bug somewhere (either in our test
framework, or in real code).

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5537: // one more frame then
expected.Possibly related to
On 2015/04/30 18:43:38, danakj wrote:
> spacing

Done.

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5539: //
EXPECT_EQ(will_begin_impl_frame_count_, kExpectedNumImplFrames);
On 2015/04/30 18:43:38, danakj wrote:
> You can ask if there's a thread and compare against different values here
> instead of just not checking

Done. I'm now checking the value where it is correct. I'm leaving the TODO and
not checking the value on the threaded version because the value is flaky.

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:722: tracked_objects::ScopedTracker
tracking_profile7(
On 2015/05/01 00:31:44, sunnyps wrote:
> tracking_profile6?

I didn't want to modify the other tracking profile code incase it disturbed
things in any way.

[sunnyps, 2015-05-01 20:35:25]

LGTM

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

https://codereview.chromium.org/1111743002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5505: did_begin_impl_frame_finish_count_(0)
{}
nit: did_finish_impl_frame_count_

https://codereview.chromium.org/1111743002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5550:
EXPECT_EQ(did_begin_impl_frame_finish_count_, kExpectedNumImplFrames);
Can we at least check EXPECT_EQ(will_begin_impl_frame_count_,
did_begin_impl_frame_finish_count_) when threaded?

[mithro-old, 2015-05-04 01:22:58]

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

https://codereview.chromium.org/1111743002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5505: did_begin_impl_frame_finish_count_(0)
{}
On 2015/05/01 20:35:25, sunnyps wrote:
> nit: did_finish_impl_frame_count_

Done! Me English good :P

https://codereview.chromium.org/1111743002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5550:
EXPECT_EQ(did_begin_impl_frame_finish_count_, kExpectedNumImplFrames);
On 2015/05/01 20:35:25, sunnyps wrote:
> Can we at least check EXPECT_EQ(will_begin_impl_frame_count_,
> did_begin_impl_frame_finish_count_) when threaded?

Line 5543 above does this.

[danakj, 2015-05-04 17:39:20]

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

https://codereview.chromium.org/1111743002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5548: if (!threaded_) {
You can just do HasImplThread() there's a function on LayerTreeTest. No need to
store threaded_ in this class.

[danakj, 2015-05-04 17:42:05]

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

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5527: if
(did_begin_impl_frame_deadline_count_ < kExpectedNumImplFrames - 1)
On 2015/05/01 02:52:35, mithro wrote:
> On 2015/04/30 18:43:38, danakj wrote:
> > else EndTest()?
> 
> Putting the EndTest inside DidBeginImplFrameDeadline makes the final
AfterCheck
> begin/end equality test impossible because there becomes a race condition
> between ending the test and starting another frame. For the same reason
checking
> the actual number of frames processed becomes flaky too.

Oh, we should make LayerTreeTest::RealEndTest not end the test while an impl
frame is still in progress, the same way it doesn't while a commit is in
progress/requested.

Can you leave a TODO here to fix that and move the EndTest() here?

[danakj, 2015-05-04 17:43:00]

https://codereview.chromium.org/1111743002/diff/80001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/1111743002/diff/80001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:48: inside_impl_frame_(false),
We could make this variable only exist when DCHECK_IS_ON() is true.

[mithro-old, 2015-05-05 04:13:13]

Hi Dana,

Thanks for your review. Are you happy for this to land now?

Tim 'mithro' Ansell

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

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5527: if
(did_begin_impl_frame_deadline_count_ < kExpectedNumImplFrames - 1)
On 2015/05/04 17:42:04, danakj wrote:
> On 2015/05/01 02:52:35, mithro wrote:
> > On 2015/04/30 18:43:38, danakj wrote:
> > > else EndTest()?
> > 
> > Putting the EndTest inside DidBeginImplFrameDeadline makes the final
> AfterCheck
> > begin/end equality test impossible because there becomes a race condition
> > between ending the test and starting another frame. For the same reason
> checking
> > the actual number of frames processed becomes flaky too.
> 
> Oh, we should make LayerTreeTest::RealEndTest not end the test while an impl
> frame is still in progress, the same way it doesn't while a commit is in
> progress/requested.
> 
> Can you leave a TODO here to fix that and move the EndTest() here?

I added the TODO inside LayerTreeTest::RealEndTest() method.

I actually think using the SendBeginMainFrameNotExpectedSoon is the right thing
here because it means this code is checking that no extra impl frames are
triggered after we stop requesting commits.

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

https://codereview.chromium.org/1111743002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5548: if (!threaded_) {
On 2015/05/04 17:39:19, danakj wrote:
> You can just do HasImplThread() there's a function on LayerTreeTest. No need
to
> store threaded_ in this class.

I tried to use HasImplThread() here but it seems to mean something different
then the threaded_ argument (which seems to determine when the extra frame
occurs).

I added a check that EXPECT_EQ(threaded_, HasImplThread()) and got the following
errors;
------------------------------------------------------------
[ RUN      ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DirectRenderer_MainThreadPaint
../../cc/trees/layer_tree_host_unittest.cc:5502: Failure
Value of: HasImplThread()
  Actual: true
Expected: threaded_
Which is: false
[  FAILED  ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DirectRenderer_MainThreadPaint
(55 ms)
[ RUN      ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DirectRenderer_ImplSidePaint
../../cc/trees/layer_tree_host_unittest.cc:5502: Failure
Value of: HasImplThread()
  Actual: true
Expected: threaded_
Which is: false
[  FAILED  ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DirectRenderer_ImplSidePaint
(56 ms)
[ RUN      ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunSingleThread_DelegatingRenderer_MainThreadPaint
[       OK ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunSingleThread_DelegatingRenderer_MainThreadPaint
(52 ms)
[ RUN      ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunSingleThread_DelegatingRenderer_ImplSidePaint
[       OK ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunSingleThread_DelegatingRenderer_ImplSidePaint
(50 ms)
[ RUN      ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DelegatingRenderer_MainThreadPaint
../../cc/trees/layer_tree_host_unittest.cc:5502: Failure
Value of: HasImplThread()
  Actual: true
Expected: threaded_
Which is: false
[  FAILED  ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DelegatingRenderer_MainThreadPaint
(50 ms)
[ RUN      ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DelegatingRenderer_ImplSidePaint
../../cc/trees/layer_tree_host_unittest.cc:5502: Failure
Value of: HasImplThread()
  Actual: true
Expected: threaded_
Which is: false
[  FAILED  ]
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DelegatingRenderer_ImplSidePaint
(56 ms)
------------------------------------------------------------

Looking at the code in LayerTreeTest::RunTest, it does a;
------------------------------------------------------------
  if (threaded) {
    impl_thread_.reset(new base::Thread("Compositor"));
    ASSERT_TRUE(impl_thread_->Start());
  }
------------------------------------------------------------

While HasImplThread() has the following definition;
------------------------------------------------------------
  bool HasImplThread() { return proxy() ? proxy()->HasImplThread() : false; }

  bool Proxy::HasImplThread() const { return !!impl_task_runner_.get(); }
------------------------------------------------------------

Also looking at the macro definitions;

threaded == false
------------------------------------------------------------------
#define SINGLE_THREAD_DIRECT_RENDERER_NOIMPL_TEST_F(TEST_FIXTURE_NAME)        \
  TEST_F(TEST_FIXTURE_NAME, RunSingleThread_DirectRenderer_MainThreadPaint) { \
    RunTest(false, false, false);                                             \
  }                                                                           \
  class SingleThreadDirectNoImplNeedsSemicolon##TEST_FIXTURE_NAME {}

#define SINGLE_THREAD_DIRECT_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME)        \
  TEST_F(TEST_FIXTURE_NAME, RunSingleThread_DirectRenderer_ImplSidePaint) { \
    RunTest(false, false, true);                                            \
  }                                                                         \
  class SingleThreadDirectImplNeedsSemicolon##TEST_FIXTURE_NAME {}

#define SINGLE_THREAD_DELEGATING_RENDERER_NOIMPL_TEST_F(TEST_FIXTURE_NAME) \
  TEST_F(TEST_FIXTURE_NAME,                                                \
         RunSingleThread_DelegatingRenderer_MainThreadPaint) {             \
    RunTest(false, true, false);                                           \
  }                                                                        \
  class SingleThreadDelegatingNoImplNeedsSemicolon##TEST_FIXTURE_NAME {}

#define SINGLE_THREAD_DELEGATING_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME) \
  TEST_F(TEST_FIXTURE_NAME,                                              \
         RunSingleThread_DelegatingRenderer_ImplSidePaint) {             \
    RunTest(false, true, true);                                          \
  }                                                                      \
  class SingleThreadDelegatingImplNeedsSemicolon##TEST_FIXTURE_NAME {}

------------------------------------------------------------------


threaded == true
------------------------------------------------------------------
#define MULTI_THREAD_DIRECT_RENDERER_NOIMPL_TEST_F(TEST_FIXTURE_NAME)        \
  TEST_F(TEST_FIXTURE_NAME, RunMultiThread_DirectRenderer_MainThreadPaint) { \
    RunTest(true, false, false);                                             \
  }                                                                          \
  class MultiThreadDirectNoImplNeedsSemicolon##TEST_FIXTURE_NAME {}

#define MULTI_THREAD_DIRECT_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME)        \
  TEST_F(TEST_FIXTURE_NAME, RunMultiThread_DirectRenderer_ImplSidePaint) { \
    RunTest(true, false, true);                                            \
  }                                                                        \
  class MultiThreadDirectImplNeedsSemicolon##TEST_FIXTURE_NAME {}

#define MULTI_THREAD_DELEGATING_RENDERER_NOIMPL_TEST_F(TEST_FIXTURE_NAME) \
  TEST_F(TEST_FIXTURE_NAME,                                               \
         RunMultiThread_DelegatingRenderer_MainThreadPaint) {             \
    RunTest(true, true, false);                                           \
  }                                                                       \
  class MultiThreadDelegatingNoImplNeedsSemicolon##TEST_FIXTURE_NAME {}

#define MULTI_THREAD_DELEGATING_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME)        \
  TEST_F(TEST_FIXTURE_NAME, RunMultiThread_DelegatingRenderer_ImplSidePaint) { \
    RunTest(true, true, true);                                                 \
  }                                                                            \
  class MultiThreadDelegatingImplNeedsSemicolon##TEST_FIXTURE_NAME {}
------------------------------------------------------------------

https://codereview.chromium.org/1111743002/diff/80001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/1111743002/diff/80001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:48: inside_impl_frame_(false),
On 2015/05/04 17:43:00, danakj wrote:
> We could make this variable only exist when DCHECK_IS_ON() is true.

I attempted this, but because of the number of places it would be needed it
makes things kind of ugly.

[mithro-old, 2015-05-05 04:33:31]

Updated the commit message to match the rename to DidFinishImplFrame.

[danakj, 2015-05-05 18:42:00]

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

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5527: if
(did_begin_impl_frame_deadline_count_ < kExpectedNumImplFrames - 1)
On 2015/05/05 04:13:13, mithro wrote:
> On 2015/05/04 17:42:04, danakj wrote:
> > On 2015/05/01 02:52:35, mithro wrote:
> > > On 2015/04/30 18:43:38, danakj wrote:
> > > > else EndTest()?
> > > 
> > > Putting the EndTest inside DidBeginImplFrameDeadline makes the final
> > AfterCheck
> > > begin/end equality test impossible because there becomes a race condition
> > > between ending the test and starting another frame. For the same reason
> > checking
> > > the actual number of frames processed becomes flaky too.
> > 
> > Oh, we should make LayerTreeTest::RealEndTest not end the test while an impl
> > frame is still in progress, the same way it doesn't while a commit is in
> > progress/requested.
> > 
> > Can you leave a TODO here to fix that and move the EndTest() here?
> 
> I added the TODO inside LayerTreeTest::RealEndTest() method.
> 
> I actually think using the SendBeginMainFrameNotExpectedSoon is the right
thing
> here because it means this code is checking that no extra impl frames are
> triggered after we stop requesting commits.

I disagree, I think the test should EndTest() when it is logically done, and the
harness should not exit until there are no more impl frames happening. This way
we see extra impl frames clearly in tests instead of just hiding them and
waiting for them like this.

We have the same expectations/behaviour around commits and catching extra
commits in tests has been very useful and prevented regressions.

I think SendBeginMainFrameNotExpectedSoon() probably doesn't need to be exposed
to the tests at all.

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

https://codereview.chromium.org/1111743002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5548: if (!threaded_) {
On 2015/05/05 04:13:13, mithro wrote:
> On 2015/05/04 17:39:19, danakj wrote:
> > You can just do HasImplThread() there's a function on LayerTreeTest. No need
> to
> > store threaded_ in this class.
> 
> I tried to use HasImplThread() here but it seems to mean something different
> then the threaded_ argument (which seems to determine when the extra frame
> occurs).
> 
> I added a check that EXPECT_EQ(threaded_, HasImplThread()) and got the
following
> errors;
> ------------------------------------------------------------
> [ RUN      ]
>
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DirectRenderer_MainThreadPaint
> ../../cc/trees/layer_tree_host_unittest.cc:5502: Failure
> Value of: HasImplThread()
>   Actual: true
> Expected: threaded_
> Which is: false
> [  FAILED  ]
>
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DirectRenderer_MainThreadPaint
> (55 ms)
> [ RUN      ]
>
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DirectRenderer_ImplSidePaint
> ../../cc/trees/layer_tree_host_unittest.cc:5502: Failure
> Value of: HasImplThread()
>   Actual: true
> Expected: threaded_
> Which is: false
> [  FAILED  ]
>
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DirectRenderer_ImplSidePaint
> (56 ms)
> [ RUN      ]
>
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunSingleThread_DelegatingRenderer_MainThreadPaint
> [       OK ]
>
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunSingleThread_DelegatingRenderer_MainThreadPaint
> (52 ms)
> [ RUN      ]
>
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunSingleThread_DelegatingRenderer_ImplSidePaint
> [       OK ]
>
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunSingleThread_DelegatingRenderer_ImplSidePaint
> (50 ms)
> [ RUN      ]
>
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DelegatingRenderer_MainThreadPaint
> ../../cc/trees/layer_tree_host_unittest.cc:5502: Failure
> Value of: HasImplThread()
>   Actual: true
> Expected: threaded_
> Which is: false
> [  FAILED  ]
>
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DelegatingRenderer_MainThreadPaint
> (50 ms)
> [ RUN      ]
>
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DelegatingRenderer_ImplSidePaint
> ../../cc/trees/layer_tree_host_unittest.cc:5502: Failure
> Value of: HasImplThread()
>   Actual: true
> Expected: threaded_
> Which is: false
> [  FAILED  ]
>
LayerTreeHostTestWillBeginImplFrameHasDidFinishImplFrame.RunMultiThread_DelegatingRenderer_ImplSidePaint
> (56 ms)

It looks to me that your threaded_ is always false. They match for
RunSingleThread, and for RunMultiThread there is a thread (HasImplThread() is
true) but your threaded_ variable is false?

> ------------------------------------------------------------
> 
> Looking at the code in LayerTreeTest::RunTest, it does a;
> ------------------------------------------------------------
>   if (threaded) {
>     impl_thread_.reset(new base::Thread("Compositor"));
>     ASSERT_TRUE(impl_thread_->Start());
>   }
> ------------------------------------------------------------
> 
> While HasImplThread() has the following definition;
> ------------------------------------------------------------
>   bool HasImplThread() { return proxy() ? proxy()->HasImplThread() : false; }
> 
>   bool Proxy::HasImplThread() const { return !!impl_task_runner_.get(); }
> ------------------------------------------------------------

When there's a thread, a task runner is given to the proxy. HasImplThread()
means the test is threaded.

> Also looking at the macro definitions;
> 
> threaded == false
> ------------------------------------------------------------------
> #define SINGLE_THREAD_DIRECT_RENDERER_NOIMPL_TEST_F(TEST_FIXTURE_NAME)       
\
>   TEST_F(TEST_FIXTURE_NAME, RunSingleThread_DirectRenderer_MainThreadPaint) {
\
>     RunTest(false, false, false);                                            
\
>   }                                                                          
\
>   class SingleThreadDirectNoImplNeedsSemicolon##TEST_FIXTURE_NAME {}
> 
> #define SINGLE_THREAD_DIRECT_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME)        \
>   TEST_F(TEST_FIXTURE_NAME, RunSingleThread_DirectRenderer_ImplSidePaint) { \
>     RunTest(false, false, true);                                            \
>   }                                                                         \
>   class SingleThreadDirectImplNeedsSemicolon##TEST_FIXTURE_NAME {}
> 
> #define SINGLE_THREAD_DELEGATING_RENDERER_NOIMPL_TEST_F(TEST_FIXTURE_NAME) \
>   TEST_F(TEST_FIXTURE_NAME,                                                \
>          RunSingleThread_DelegatingRenderer_MainThreadPaint) {             \
>     RunTest(false, true, false);                                           \
>   }                                                                        \
>   class SingleThreadDelegatingNoImplNeedsSemicolon##TEST_FIXTURE_NAME {}
> 
> #define SINGLE_THREAD_DELEGATING_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME) \
>   TEST_F(TEST_FIXTURE_NAME,                                              \
>          RunSingleThread_DelegatingRenderer_ImplSidePaint) {             \
>     RunTest(false, true, true);                                          \
>   }                                                                      \
>   class SingleThreadDelegatingImplNeedsSemicolon##TEST_FIXTURE_NAME {}
> 
> ------------------------------------------------------------------
> 
> 
> threaded == true
> ------------------------------------------------------------------
> #define MULTI_THREAD_DIRECT_RENDERER_NOIMPL_TEST_F(TEST_FIXTURE_NAME)        \
>   TEST_F(TEST_FIXTURE_NAME, RunMultiThread_DirectRenderer_MainThreadPaint) { \
>     RunTest(true, false, false);                                             \
>   }                                                                          \
>   class MultiThreadDirectNoImplNeedsSemicolon##TEST_FIXTURE_NAME {}
> 
> #define MULTI_THREAD_DIRECT_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME)        \
>   TEST_F(TEST_FIXTURE_NAME, RunMultiThread_DirectRenderer_ImplSidePaint) { \
>     RunTest(true, false, true);                                            \
>   }                                                                        \
>   class MultiThreadDirectImplNeedsSemicolon##TEST_FIXTURE_NAME {}
> 
> #define MULTI_THREAD_DELEGATING_RENDERER_NOIMPL_TEST_F(TEST_FIXTURE_NAME) \
>   TEST_F(TEST_FIXTURE_NAME,                                               \
>          RunMultiThread_DelegatingRenderer_MainThreadPaint) {             \
>     RunTest(true, true, false);                                           \
>   }                                                                       \
>   class MultiThreadDelegatingNoImplNeedsSemicolon##TEST_FIXTURE_NAME {}
> 
> #define MULTI_THREAD_DELEGATING_RENDERER_IMPL_TEST_F(TEST_FIXTURE_NAME)       
\
>   TEST_F(TEST_FIXTURE_NAME, RunMultiThread_DelegatingRenderer_ImplSidePaint) {
\
>     RunTest(true, true, true);                                                
\
>   }                                                                           
\
>   class MultiThreadDelegatingImplNeedsSemicolon##TEST_FIXTURE_NAME {}
> ------------------------------------------------------------------

https://codereview.chromium.org/1111743002/diff/80001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/1111743002/diff/80001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:48: inside_impl_frame_(false),
On 2015/05/05 04:13:13, mithro wrote:
> On 2015/05/04 17:43:00, danakj wrote:
> > We could make this variable only exist when DCHECK_IS_ON() is true.
> 
> I attempted this, but because of the number of places it would be needed it
> makes things kind of ugly.

Well, there's like 4 places? There's no need to grow objects in the shipping
binary that is for DCHECKs that aren't used.

[mithro-old, 2015-05-06 02:14:11]

Hi Dana,

Please see the updates.

This patch now needs https://codereview.chromium.org/1126973002/ to land as it
is dependent on HasImplThread() method being correct.

Tim

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

https://codereview.chromium.org/1111743002/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5527: if
(did_begin_impl_frame_deadline_count_ < kExpectedNumImplFrames - 1)
On 2015/05/05 18:42:00, danakj wrote:
> On 2015/05/05 04:13:13, mithro wrote:
> > On 2015/05/04 17:42:04, danakj wrote:
> > > On 2015/05/01 02:52:35, mithro wrote:
> > > > On 2015/04/30 18:43:38, danakj wrote:
> > > > > else EndTest()?
> > > > 
> > > > Putting the EndTest inside DidBeginImplFrameDeadline makes the final
> > > AfterCheck
> > > > begin/end equality test impossible because there becomes a race
condition
> > > > between ending the test and starting another frame. For the same reason
> > > checking
> > > > the actual number of frames processed becomes flaky too.
> > > 
> > > Oh, we should make LayerTreeTest::RealEndTest not end the test while an
impl
> > > frame is still in progress, the same way it doesn't while a commit is in
> > > progress/requested.
> > > 
> > > Can you leave a TODO here to fix that and move the EndTest() here?
> > 
> > I added the TODO inside LayerTreeTest::RealEndTest() method.
> > 
> > I actually think using the SendBeginMainFrameNotExpectedSoon is the right
> thing
> > here because it means this code is checking that no extra impl frames are
> > triggered after we stop requesting commits.
> 
> I disagree, I think the test should EndTest() when it is logically done,

I agree that the test should call EndTest() when it is logically done. I think
we disagree with when "logically done" occurs.

This test is "logically done" when all impl frames are finished and no more are
going to occur. **Not** when we have the expected number of impl frames.

> and the
> harness should not exit until there are no more impl frames happening. 

I believe this is impossible to detect 100% correctly and especially impossible
to detect without intimate knowledge of how impl frames work. If you want to try
write some logic, I bet that I can come up with a scenario where it will exit
too early, missing some real impl frames that would have occurred.

> This way
> we see extra impl frames clearly in tests instead of just hiding them and
> waiting for them like this.

I'm not hiding anything here. In the AfterTest() we clearly check that the
number of frames we received was the number of frames we expected.

The fact these checks don't work for the multithread version is showing that
there is a real race condition or other bug in the system somewhere.

> We have the same expectations/behaviour around commits and catching extra
> commits in tests has been very useful and prevented regressions.

I agree that checking the number of commits expected it useful. 

I'm not confident that you are catching all the issues as expected with your
current method. For example, looking at EndTest() it is currently dependent on
only Scheduler::MainFrameForTestingWillHappen which knows nothing about what
operations are currently in the message queue. These pending actions **could**
make MainFrameForTestingWillHappen become true again but we have already exited
the test. You can't make this reliable by adding a test to check if the
MessageQueue is empty either because there are plenty of cases where it won't be
empty.

> I think SendBeginMainFrameNotExpectedSoon() probably doesn't need to be
exposed
> to the tests at all.


I think that SendBeginMainFrameNotExpectedSoon is the best compromise because it
is a signal from the scheduler that it thinks it is finished and detects the
following issues;
  * If this message occurs before the expected number of frames, then it's a bug
in the scheduler logic. 
  * If there are more than the expected number of frames, then it is again a
bug. 

The only bug it misses is where the scheduler calls
SendBeginMainFrameNotExpectedSoon at exactly the right number of frames while
there are still frames pending. Due to the async nature of the system this would
make the test flaky because there is no reliable way to make this situation
occur even if we wanted too.

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

https://codereview.chromium.org/1111743002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5548: if (!threaded_) {
I tracked down the issue and uploaded a patch at
https://codereview.chromium.org/1126973002

tl;dr - HasImplThread() function returned the incorrect value in AfterTest().

https://codereview.chromium.org/1111743002/diff/80001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/1111743002/diff/80001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:48: inside_impl_frame_(false),
On 2015/05/05 18:42:00, danakj wrote:
> On 2015/05/05 04:13:13, mithro wrote:
> > On 2015/05/04 17:43:00, danakj wrote:
> > > We could make this variable only exist when DCHECK_IS_ON() is true.
> > 
> > I attempted this, but because of the number of places it would be needed it
> > makes things kind of ugly.
> 
> Well, there's like 4 places? There's no need to grow objects in the shipping
> binary that is for DCHECKs that aren't used.

Added it back in.

BTW It's actually 8 places. Even with DCHECK is disabled the value inside the
DCHECK is still referenced (so you don't get "this value is unused" warnings),
so I have to wrap those calls too.

[danakj, 2015-05-06 22:44:44]

LGTM

[mithro-old, 2015-05-07 01:16:12]

The CQ bit was checked by mithro@mithis.com

[mithro-old, 2015-05-07 01:16:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
sunnyps@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1111743002/#ps180001
(title: "Rebase onto master.")

[commit-bot: I haz the power, 2015-05-07 01:18:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1111743002/180001

[commit-bot: I haz the power, 2015-05-07 02:50:42]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-05-07 02:52:20]

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

[Kunihiko Sakamoto, 2015-05-07 03:30:48]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/1127963003/ by ksakamoto@chromium.org.

The reason for reverting is: Broke Win x64 GN build.

http://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN/builds/1013/....

[mithro-old, 2015-05-07 04:05:50]

mithro@mithis.com changed reviewers:
+ cpu@chromium.org, erg@chromium.org, jochen@chromium.org, sky@chromium.org

[mithro-old, 2015-05-07 04:05:51]

Hi owners of components/surfaces,

I need a LGTM from one of you to land this patch because your
surfaces_scheduler.cc inherits from cc/scheduler/scheduler.h

As far as I can see, this is a nop change from your point of view.

Thanks!

Tim

[sky, 2015-05-07 16:22:20]

LGTM

[mithro-old, 2015-05-07 23:41:07]

The CQ bit was checked by mithro@mithis.com

[mithro-old, 2015-05-07 23:41:07]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
sunnyps@chromium.org
Link to the patchset: https://codereview.chromium.org/1111743002/#ps200001
(title: "Fixing surfaces_scheduler.")

[commit-bot: I haz the power, 2015-05-07 23:41:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1111743002/200001

[commit-bot: I haz the power, 2015-05-07 23:52:56]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-05-07 23:56:01]

Patchset 11 (id:??) landed as
https://crrev.com/51693e3839cf85fad73979258c00e5964d627240
Cr-Commit-Position: refs/heads/master@{#328883}