cc: Plumbing for impl thread micro benchmarks

cc: Plumbing for impl thread micro benchmarks

This patch adds plumbing for running micro benchmarks on the impl
thread. The concept is as follows:

- Create a main thread benchmark via gpu benchmarking extension
- On commit complete, if the benchmark provides an impl thread
  benchmark, it is created and scheduled to be run on the impl thread
- Impl thread benchmarks are run after an appropriate callback
- Impl thread benchmarks post a done callback (with results) that is
  run on the main thread
- The main thread benchmark gets the callback and posts its own done
  callback (with aggregated results)
- This gets back to the gpu benchmarking extension which sends the
  response back to the caller

R=enne@chromium.org, nduca@chromium.org

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

[vmpstr, 2013-11-09 00:41:41]

Please take a look.

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_cont...
File cc/debug/micro_benchmark_controller.cc (right):

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_cont...
cc/debug/micro_benchmark_controller.cc:85: if (!(*it)->IsDone())
If the main thread benchmark was waiting for the impl one to finish, and the
impl finished, then MicroBenchmarkController never had a chance to clean it up.
This means we might have valid cases where DidUpdateLayers is called while there
are benchmarks in the done state.

[enne (OOO), 2013-11-12 01:09:36]

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_cont...
File cc/debug/micro_benchmark_controller.cc (right):

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_cont...
cc/debug/micro_benchmark_controller.cc:75:
(*it)->CreateBenchmarkImpl(main_controller_message_loop_);
So if you have a MicroBenchmarkImpl that doesn't complete immediately, it will
create a second MicroBenchmarkImpl on the next commit? That seems a little
unexpected here.  Should each MicroBenchmark only get one outstanding
MicroBenchmarkImpl? Or, is there some other use case?

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_impl.cc
File cc/debug/micro_benchmark_impl.cc (right):

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_impl...
cc/debug/micro_benchmark_impl.cc:17: void RunCallback(const
MicroBenchmarkImpl::DoneCallback& callback,
Do you expect other MicroBenchmarkImpl to do something else in this callback?
What about just making MicroBenchmarkImpl provide this common  callback and call
some virtual MicroBenchmark::DidFinishMicroBenchmarkImpl function?

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_impl...
cc/debug/micro_benchmark_impl.cc:26: scoped_refptr<base::MessageLoopProxy>
origin_loop)
What about making MicroBenchmarkImpl provide a virtual scoped_ptr<base::Value>
result() const function and have
MicroBenchmarkControllerImpl::CleanUpFinishedBenchmarks() post all the tasks via
host_impl_->proxy()->MainThreadTaskRunner()?

That'd clean up a lot of APIs where you have to ferry around the main thread
message loop (although you lose the NotifyDone symmetry).

[vmpstr, 2013-11-12 17:51:38]

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_cont...
File cc/debug/micro_benchmark_controller.cc (right):

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_cont...
cc/debug/micro_benchmark_controller.cc:75:
(*it)->CreateBenchmarkImpl(main_controller_message_loop_);
On 2013/11/12 01:09:36, enne wrote:
> So if you have a MicroBenchmarkImpl that doesn't complete immediately, it will
> create a second MicroBenchmarkImpl on the next commit? That seems a little
> unexpected here.  Should each MicroBenchmark only get one outstanding
> MicroBenchmarkImpl? Or, is there some other use case?

That's a good point. I mean it _could_ happen that we need an impl side
benchmark on every commit for several commits, but rasterize and record isn't
one of these cases. I renamed this to CreateBenchmarkImplIfNeeded and the
rasterize and record benchmark should only return a benchmark on the first call
and later return null. Either that or we can mark each MicroBenchmark as already
processed for impl benchmarks. Wdyt?

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_impl.cc
File cc/debug/micro_benchmark_impl.cc (right):

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_impl...
cc/debug/micro_benchmark_impl.cc:17: void RunCallback(const
MicroBenchmarkImpl::DoneCallback& callback,
On 2013/11/12 01:09:36, enne wrote:
> Do you expect other MicroBenchmarkImpl to do something else in this callback?
> What about just making MicroBenchmarkImpl provide this common  callback and
call
> some virtual MicroBenchmark::DidFinishMicroBenchmarkImpl function?

I'm not sure I understand what you mean... You mean do something different in
the RunCallback or in |callback|? As it stands now, |callback| is the only link
between the original MicroBenchmark and the Impl part. That is, the original
micro benchmark after creating the impl part doesn't keep track of it, it just
gives it a callback to call when it's done. I could change it so that impl
benchmark has a pointer to the benchmark that created it and then post a
particular function call on the main thread. Is that kind of what you are
suggesting or am I misunderstanding?

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_impl...
cc/debug/micro_benchmark_impl.cc:26: scoped_refptr<base::MessageLoopProxy>
origin_loop)
On 2013/11/12 01:09:36, enne wrote:
> What about making MicroBenchmarkImpl provide a virtual scoped_ptr<base::Value>
> result() const function and have
> MicroBenchmarkControllerImpl::CleanUpFinishedBenchmarks() post all the tasks
via
> host_impl_->proxy()->MainThreadTaskRunner()?
> 
> That'd clean up a lot of APIs where you have to ferry around the main thread
> message loop (although you lose the NotifyDone symmetry).

I do like the NotifyDone style thing, since it makes MicroBenchmarkImpl derived
implementations look very similar to MicroBenchmark derived implementations. I
will try the approach you're suggesting though.

[enne (OOO), 2013-11-12 19:22:55]

What prevents a user-after-free? In your other patch you use base::Unretained. 
It seems possible that you could post a task from the impl thread => ~LTH() =>
handle message, and then the MicroBenchmark no longer exists that you're
referencing.  It seems like you need some weak pointers here.

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_cont...
File cc/debug/micro_benchmark_controller.cc (right):

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_cont...
cc/debug/micro_benchmark_controller.cc:75:
(*it)->CreateBenchmarkImpl(main_controller_message_loop_);
On 2013/11/12 17:51:38, vmpstr wrote:
> On 2013/11/12 01:09:36, enne wrote:
> > So if you have a MicroBenchmarkImpl that doesn't complete immediately, it
will
> > create a second MicroBenchmarkImpl on the next commit? That seems a little
> > unexpected here.  Should each MicroBenchmark only get one outstanding
> > MicroBenchmarkImpl? Or, is there some other use case?
> 
> That's a good point. I mean it _could_ happen that we need an impl side
> benchmark on every commit for several commits, but rasterize and record isn't
> one of these cases. I renamed this to CreateBenchmarkImplIfNeeded and the
> rasterize and record benchmark should only return a benchmark on the first
call
> and later return null. Either that or we can mark each MicroBenchmark as
already
> processed for impl benchmarks. Wdyt?

Yeah, I'm not sure what the right thing to do here is.

What about having the controller have a map of benchmark to benchmark impl. 
Calling CreateBenchmarkImpl adds it to the map.  Being cleaned up removes it. 
CreateBenchmarkImpl can then only get called if it's not already in the map?
That way individual benchmarks don't have to manage this.

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_impl.cc
File cc/debug/micro_benchmark_impl.cc (right):

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_impl...
cc/debug/micro_benchmark_impl.cc:17: void RunCallback(const
MicroBenchmarkImpl::DoneCallback& callback,
On 2013/11/12 17:51:38, vmpstr wrote:
> On 2013/11/12 01:09:36, enne wrote:
> > Do you expect other MicroBenchmarkImpl to do something else in this
callback?
> > What about just making MicroBenchmarkImpl provide this common  callback and
> call
> > some virtual MicroBenchmark::DidFinishMicroBenchmarkImpl function?
> 
> I'm not sure I understand what you mean... You mean do something different in
> the RunCallback or in |callback|? As it stands now, |callback| is the only
link
> between the original MicroBenchmark and the Impl part. That is, the original
> micro benchmark after creating the impl part doesn't keep track of it, it just
> gives it a callback to call when it's done. I could change it so that impl
> benchmark has a pointer to the benchmark that created it and then post a
> particular function call on the main thread. Is that kind of what you are
> suggesting or am I misunderstanding?

Ah, fair enough.

[vmpstr, 2013-11-12 20:20:37]

Please take a look. For the use-after-free, I was kind of working on a (probably
wrong) assumption that LTH outlives all. I don't think it outlives the message
loop though. I'll fix the other patch to pass a weak pointer instead of
unretained.

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_cont...
File cc/debug/micro_benchmark_controller.cc (right):

https://codereview.chromium.org/67563002/diff/1/cc/debug/micro_benchmark_cont...
cc/debug/micro_benchmark_controller.cc:75:
(*it)->CreateBenchmarkImpl(main_controller_message_loop_);
On 2013/11/12 19:22:56, enne wrote:
> On 2013/11/12 17:51:38, vmpstr wrote:
> > On 2013/11/12 01:09:36, enne wrote:
> > > So if you have a MicroBenchmarkImpl that doesn't complete immediately, it
> will
> > > create a second MicroBenchmarkImpl on the next commit? That seems a little
> > > unexpected here.  Should each MicroBenchmark only get one outstanding
> > > MicroBenchmarkImpl? Or, is there some other use case?
> > 
> > That's a good point. I mean it _could_ happen that we need an impl side
> > benchmark on every commit for several commits, but rasterize and record
isn't
> > one of these cases. I renamed this to CreateBenchmarkImplIfNeeded and the
> > rasterize and record benchmark should only return a benchmark on the first
> call
> > and later return null. Either that or we can mark each MicroBenchmark as
> already
> > processed for impl benchmarks. Wdyt?
> 
> Yeah, I'm not sure what the right thing to do here is.
> 
> What about having the controller have a map of benchmark to benchmark impl. 
> Calling CreateBenchmarkImpl adds it to the map.  Being cleaned up removes it. 
> CreateBenchmarkImpl can then only get called if it's not already in the map?
> That way individual benchmarks don't have to manage this.

I went with just keeping the state in the microbenchmark base class. Now
GetBenchmarkImpl is non-virtual and it updates the state of whether we grabbed
the impl part or not. CreateBenchmarkImpl is now protected to avoid accidentally
creating it directly. I kind of prefer this approach over a separate map that
needs to be maintained in the controller. Let me know if this works for you.

[enne (OOO), 2013-11-12 20:27:54]

https://codereview.chromium.org/67563002/diff/110001/cc/debug/micro_benchmark.cc
File cc/debug/micro_benchmark.cc (right):

https://codereview.chromium.org/67563002/diff/110001/cc/debug/micro_benchmark...
cc/debug/micro_benchmark.cc:45: processed_for_benchmark_impl_ = true;
Ok, sure.

[enne (OOO), 2013-11-14 00:18:47]

MicroBenchmarkImpl unit test?

[vmpstr, 2013-11-14 00:39:36]

On 2013/11/14 00:18:47, enne wrote:
> MicroBenchmarkImpl unit test?

You mean the ControllerImpl or the BenchmarkImpl? I'm not really sure what I can
test on the micro benchmark impl aside from that NotifyDone posts to the message
loop.

[enne (OOO), 2013-11-14 00:41:22]

On 2013/11/14 00:39:36, vmpstr wrote:
> On 2013/11/14 00:18:47, enne wrote:
> > MicroBenchmarkImpl unit test?
> 
> You mean the ControllerImpl or the BenchmarkImpl? I'm not really sure what I
can
> test on the micro benchmark impl aside from that NotifyDone posts to the
message
> loop.

Yeah, like a test microbenchmark that creates a benchmarkimpl, finishes,
notifies back, and completes.  Just to make sure this flow is actually
exercised.

[vmpstr, 2013-11-14 18:48:30]

PTAL. Added a unittest.

I know what you mean about needing a WeakPtrFactory... I think I can store that
on the baseclass if I make MicroBenchmark{,Impl} a template, and I think I can
also eliminate the need for the benchmark to care about the message loop (again
by moving that to the base class). However, I'd like to try that as a follow-up
if possible.

[enne (OOO), 2013-11-14 18:51:29]

On 2013/11/14 18:48:30, vmpstr wrote:
> PTAL. Added a unittest.

lgtm, thanks!

[commit-bot: I haz the power, 2013-11-14 20:16:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/67563002/320001

[commit-bot: I haz the power, 2013-11-14 21:13:26]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, chromeos_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, gpu_unittests, interactive_ui_tests,
ipc_tests, jingle_unittests, media_unittests, net_unittests, ppapi_unittests,
printing_unittests, sandbox_linux_unittests, sql_unittests, sync_unit_tests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-11-14 23:44:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/67563002/790001

[commit-bot: I haz the power, 2013-11-15 01:50:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/67563002/790001

[commit-bot: I haz the power, 2013-11-15 04:04:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/67563002/790001

[commit-bot: I haz the power, 2013-11-15 07:37:54]

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

[commit-bot: I haz the power, 2013-11-15 18:37:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/67563002/1140001

[commit-bot: I haz the power, 2013-11-15 22:05:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/67563002/1140001

[commit-bot: I haz the power, 2013-11-16 00:35:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/67563002/1140001

[commit-bot: I haz the power, 2013-11-18 00:39:34]

Change committed as 235585