cc: Add message passing mechanism to micro benchmarking.

cc: Add message passing mechanism to micro benchmarking.

Add a mechansim to send messages to running micro benchmarks through the
GPU benchmarking extension. This will be used for example to shut down a
running micro benchmark from telemetry.

R=vmpstr@chromium.org,jamesr@chromium.org
BUG=376379
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273726

[ernstm, 2014-05-28 21:00:32]

PTAL

[vmpstr, 2014-05-28 21:16:25]

Overall looks good, just a few comments below. Also, I think it might be worth
it to make the adjustment to existing telemetry benchmarks to expect a non-zero
instead of a true (I mean luckily one is convertible into another, but maybe
there are places where it can be more explicit)

https://codereview.chromium.org/300963004/diff/1/cc/debug/micro_benchmark.h
File cc/debug/micro_benchmark.h (right):

https://codereview.chromium.org/300963004/diff/1/cc/debug/micro_benchmark.h#n...
cc/debug/micro_benchmark.h:27: explicit MicroBenchmark(const DoneCallback&
callback, int id);
Since this id isn't used by any particular benchmark, do you think it's better
to have a set_id(int id) that the controller calls at the end of the if/else
if/else if creation thing. That way each individual benchmark doesn't have to
take an id in the constructor.

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

https://codereview.chromium.org/300963004/diff/1/cc/debug/micro_benchmark_con...
cc/debug/micro_benchmark_controller.cc:79: if (next_id_ < 1)
signed int overflow is undefined... Maybe something like

if (next_id_ == std::numeric_limits<int>::max())
  next_id_ = 1;

https://codereview.chromium.org/300963004/diff/1/cc/debug/micro_benchmark_con...
cc/debug/micro_benchmark_controller.cc:84: bool
MicroBenchmarkController::SendMessage(int id,
I'm sorry to go back on what I said earlier, but... :P


Maybe we can have SendMessageToMicroBenchmark ->
MicroBenchmarkController::ProcessMessage -> MicroBenchmark::ProcessMessage?

It's kind of ambiguous what "SendMessage" means in the benchmark context... is
it that it's a request to send a message or is it a message being sent?

https://codereview.chromium.org/300963004/diff/1/cc/debug/micro_benchmark_con...
File cc/debug/micro_benchmark_controller.h (right):

https://codereview.chromium.org/300963004/diff/1/cc/debug/micro_benchmark_con...
cc/debug/micro_benchmark_controller.h:31: int ScheduleRun(const std::string&
micro_benchmark_name,
Make a comment about the return value, please.

https://codereview.chromium.org/300963004/diff/1/cc/debug/micro_benchmark_con...
cc/debug/micro_benchmark_controller.h:45: int next_id_;
I think we only have one controller, but maybe make this static just to be on
the safe side?

https://codereview.chromium.org/300963004/diff/1/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/300963004/diff/1/cc/trees/layer_tree_host.h#n...
cc/trees/layer_tree_host.h:278: int ScheduleMicroBenchmark(const std::string&
benchmark_name,
Comment about the return value please

[ernstm, 2014-05-28 21:51:55]

https://codereview.chromium.org/300963004/diff/1/cc/debug/micro_benchmark.h
File cc/debug/micro_benchmark.h (right):

https://codereview.chromium.org/300963004/diff/1/cc/debug/micro_benchmark.h#n...
cc/debug/micro_benchmark.h:27: explicit MicroBenchmark(const DoneCallback&
callback, int id);
On 2014/05/28 21:16:25, vmpstr wrote:
> Since this id isn't used by any particular benchmark, do you think it's better
> to have a set_id(int id) that the controller calls at the end of the if/else
> if/else if creation thing. That way each individual benchmark doesn't have to
> take an id in the constructor.

Done. The id is now set in ScheduleRun, after the benchmark was successfully
created.

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

https://codereview.chromium.org/300963004/diff/1/cc/debug/micro_benchmark_con...
cc/debug/micro_benchmark_controller.cc:79: if (next_id_ < 1)
On 2014/05/28 21:16:25, vmpstr wrote:
> signed int overflow is undefined... Maybe something like
> 
> if (next_id_ == std::numeric_limits<int>::max())
>   next_id_ = 1;

Done.

https://codereview.chromium.org/300963004/diff/1/cc/debug/micro_benchmark_con...
cc/debug/micro_benchmark_controller.cc:84: bool
MicroBenchmarkController::SendMessage(int id,
On 2014/05/28 21:16:25, vmpstr wrote:
> I'm sorry to go back on what I said earlier, but... :P
> 
> 
> Maybe we can have SendMessageToMicroBenchmark ->
> MicroBenchmarkController::ProcessMessage -> MicroBenchmark::ProcessMessage?
> 
> It's kind of ambiguous what "SendMessage" means in the benchmark context... is
> it that it's a request to send a message or is it a message being sent?

As discussed offline, renamed the function to ProcessMessage on MicroBenchmark,
but I kept SendMessage on the controller, because this forwards the message to
the benchmark.

https://codereview.chromium.org/300963004/diff/1/cc/debug/micro_benchmark_con...
File cc/debug/micro_benchmark_controller.h (right):

https://codereview.chromium.org/300963004/diff/1/cc/debug/micro_benchmark_con...
cc/debug/micro_benchmark_controller.h:31: int ScheduleRun(const std::string&
micro_benchmark_name,
On 2014/05/28 21:16:25, vmpstr wrote:
> Make a comment about the return value, please.

Done.

https://codereview.chromium.org/300963004/diff/1/cc/debug/micro_benchmark_con...
cc/debug/micro_benchmark_controller.h:45: int next_id_;
On 2014/05/28 21:16:25, vmpstr wrote:
> I think we only have one controller, but maybe make this static just to be on
> the safe side?

Done.

https://codereview.chromium.org/300963004/diff/1/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/300963004/diff/1/cc/trees/layer_tree_host.h#n...
cc/trees/layer_tree_host.h:278: int ScheduleMicroBenchmark(const std::string&
benchmark_name,
On 2014/05/28 21:16:25, vmpstr wrote:
> Comment about the return value please

Done.

[vmpstr, 2014-05-28 22:07:28]

cc/ lgtm. Thanks!

https://codereview.chromium.org/300963004/diff/20001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/300963004/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:281: // Returns the id of the benchmark on success, 0
otherwise.
nit: comment is in the wrong place, but you can have a comment on this function
too just a different one :)

[ernstm, 2014-05-28 22:29:22]

https://codereview.chromium.org/300963004/diff/20001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/300963004/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:281: // Returns the id of the benchmark on success, 0
otherwise.
On 2014/05/28 22:07:29, vmpstr wrote:
> nit: comment is in the wrong place, but you can have a comment on this
function
> too just a different one :)

Done.

[ernstm, 2014-05-29 20:19:59]

On 2014/05/28 22:29:22, ernstm wrote:
>
https://codereview.chromium.org/300963004/diff/20001/cc/trees/layer_tree_host.h
> File cc/trees/layer_tree_host.h (right):
> 
>
https://codereview.chromium.org/300963004/diff/20001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host.h:281: // Returns the id of the benchmark on success,
0
> otherwise.
> On 2014/05/28 22:07:29, vmpstr wrote:
> > nit: comment is in the wrong place, but you can have a comment on this
> function
> > too just a different one :)
> 
> Done.

James, could you please take a look at the content/ part of this patch?

[jamesr, 2014-05-29 20:35:00]

I'm having trouble understanding the diff. Can you tell what's going on?

https://codereview.chromium.org/300963004/diff/40001/content/renderer/gpu/gpu...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/300963004/diff/40001/content/renderer/gpu/gpu...
content/renderer/gpu/gpu_benchmarking_extension.cc:217: "if (typeof(chrome) ==
'undefined') {"
hmm, why are this and the follow lines marked as changed? what actually changed
here?

[ernstm, 2014-05-29 20:38:36]

https://codereview.chromium.org/300963004/diff/40001/content/renderer/gpu/gpu...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/300963004/diff/40001/content/renderer/gpu/gpu...
content/renderer/gpu/gpu_benchmarking_extension.cc:217: "if (typeof(chrome) ==
'undefined') {"
On 2014/05/29 20:35:01, jamesr wrote:
> hmm, why are this and the follow lines marked as changed? what actually
changed
> here?

"git cl format" changed most of these lines. 

This is what was actually added:
 331 "chrome.gpuBenchmarking.sendMessageToMicroBenchmark ="
 332 "    function(id, arguments) {"
 333 "  native function SendMessageToMicroBenchmark();"
 334 "  return SendMessageToMicroBenchmark(id, arguments);"
 335 "};"

[jamesr, 2014-05-29 20:39:41]

lgtm

https://codereview.chromium.org/300963004/diff/40001/content/renderer/gpu/gpu...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/300963004/diff/40001/content/renderer/gpu/gpu...
content/renderer/gpu/gpu_benchmarking_extension.cc:217: "if (typeof(chrome) ==
'undefined') {"
On 2014/05/29 20:38:36, ernstm wrote:
> On 2014/05/29 20:35:01, jamesr wrote:
> > hmm, why are this and the follow lines marked as changed? what actually
> changed
> > here?
> 
> "git cl format" changed most of these lines. 
> 
> This is what was actually added:
>  331 "chrome.gpuBenchmarking.sendMessageToMicroBenchmark ="
>  332 "    function(id, arguments) {"
>  333 "  native function SendMessageToMicroBenchmark();"
>  334 "  return SendMessageToMicroBenchmark(id, arguments);"
>  335 "};"

Ah, it added 2 spaces to the leading indent.  I wish intra-line diffing was
smarter about this but oh well

[ernstm, 2014-05-29 20:44:31]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-05-29 20:46:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ernstm@chromium.org/300963004/40001

[commit-bot: I haz the power, 2014-05-30 05:31:56]

Change committed as 273726