Changed the GPU Tracer so the category names can be customized.

Changed the GPU Tracer so the category names can be customized.

R=vmiura@chromium.org
BUG=None
TEST=trybots

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

[vmiura, 2014-12-02 19:24:42]

This looks fine.  How are you planning on customizing individual event
categories?

Could we make GPU tracing look similar to CPU tracing?

CPU has macros in "base/debug/trace_event.h" for:

TRACE_EVENT_BEGIN0("category", "name")
TRACE_EVENT_BEGIN1("category", "name", "variable1", "value1")
TRACE_EVENT_BEGIN2("category", "name", "variable1", "value1", "variable2",
"value2")
TRACE_EVENT_END0("category", "name")
etc.

What if we added command buffer APIs like TraceBeginCHROMIUM(category, value),
TraceEndCHROMIUM(category, value) and then add equivalent macros?

TRACE_GPU_EVENT_BEGIN0(context, "category", "name")
TRACE_GPU_EVENT_END0(context, "category", "name")
etc.

The nice thing is then the macros can skip calling
TraceBeginCHROMIUM/TraceEndCHROMIUM if the category is not enabled.

[David Yen, 2014-12-04 21:53:47]

I've added a category field to the trace calls as well as TraceBeginCHROMIUM().

One thing I wasn't sure about was the category names for existing calls. I've
set decoder calls to use the category "gpu_decoder" and PushGroupMarkerEXT calls
to use the category "gpu_group_marker".

Since this CL is already getting too big, I will add a macro to call this
function easily in another CL.

PTAL.

[David Yen, 2014-12-08 17:18:54]

On 2014/12/04 21:53:47, David Yen wrote:
> I've added a category field to the trace calls as well as
TraceBeginCHROMIUM().
> 
> One thing I wasn't sure about was the category names for existing calls. I've
> set decoder calls to use the category "gpu_decoder" and PushGroupMarkerEXT
calls
> to use the category "gpu_group_marker".
> 
> Since this CL is already getting too big, I will add a macro to call this
> function easily in another CL.
> 
> PTAL.

ping

[vmiura, 2014-12-08 23:23:06]

Looks good.  A few comments.

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3964: if (DebugImpl &&
gpu_trace_commands_ && *gpu_decoder_category_) {
I think we could combine the *gpu_decoder_category_ check into where
gpu_trace_commands_ is initialized.

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11079:
!name_bucket->GetAsString(&command_name)) {
nit: trace_name for consistency.

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_tracer.cc:100:
TRACE_EVENT_COPY_ASYNC_BEGIN0(category().c_str(), name().c_str(), this);
Shoud we check the gpu.service category is enabled here?

if (*gpu_trace_srv_category) {
  TRACE_EVENT_COPY_ASYNC_BEGIN0(category().c_str(), name().c_str(), this);
}

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_tracer.cc:140:
TRACE_EVENT_COPY_ASYNC_END0(category().c_str(), name().c_str(), this);
And here.

if (*gpu_trace_srv_category) {
  ...
}

[David Yen, 2014-12-08 23:45:38]

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3964: if (DebugImpl &&
gpu_trace_commands_ && *gpu_decoder_category_) {
On 2014/12/08 23:23:06, vmiura wrote:
> I think we could combine the *gpu_decoder_category_ check into where
> gpu_trace_commands_ is initialized.

Done.

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:11079:
!name_bucket->GetAsString(&command_name)) {
On 2014/12/08 23:23:06, vmiura wrote:
> nit: trace_name for consistency.

Done.

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_tracer.cc:100:
TRACE_EVENT_COPY_ASYNC_BEGIN0(category().c_str(), name().c_str(), this);
On 2014/12/08 23:23:06, vmiura wrote:
> Shoud we check the gpu.service category is enabled here?
> 
> if (*gpu_trace_srv_category) {
>   TRACE_EVENT_COPY_ASYNC_BEGIN0(category().c_str(), name().c_str(), this);
> }

I believe Start() is only called from the GPUTracer when IsTracing() is false,
which does the category checking.

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_tracer.cc:140:
TRACE_EVENT_COPY_ASYNC_END0(category().c_str(), name().c_str(), this);
On 2014/12/08 23:23:06, vmiura wrote:
> And here.
> 
> if (*gpu_trace_srv_category) {
>   ...
> }

Same reason as above.

[vmiura, 2014-12-08 23:53:00]

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_tracer.cc:100:
TRACE_EVENT_COPY_ASYNC_BEGIN0(category().c_str(), name().c_str(), this);
On 2014/12/08 23:45:38, David Yen wrote:
> On 2014/12/08 23:23:06, vmiura wrote:
> > Shoud we check the gpu.service category is enabled here?
> > 
> > if (*gpu_trace_srv_category) {
> >   TRACE_EVENT_COPY_ASYNC_BEGIN0(category().c_str(), name().c_str(), this);
> > }
> 
> I believe Start() is only called from the GPUTracer when IsTracing() is false,
> which does the category checking.

True, but IsTracing() returns true both (*gpu_trace_srv_category != 0) ||
(*gpu_trace_dev_category != 0).

If the user turns on gpu.device, won't they effectively get the gpu.service
events as well?  Checking for *gpu_trace_srv_category will block that.

[David Yen, 2014-12-09 00:10:41]

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_tracer.cc:100:
TRACE_EVENT_COPY_ASYNC_BEGIN0(category().c_str(), name().c_str(), this);
On 2014/12/08 23:53:00, vmiura wrote:
> On 2014/12/08 23:45:38, David Yen wrote:
> > On 2014/12/08 23:23:06, vmiura wrote:
> > > Shoud we check the gpu.service category is enabled here?
> > > 
> > > if (*gpu_trace_srv_category) {
> > >   TRACE_EVENT_COPY_ASYNC_BEGIN0(category().c_str(), name().c_str(), this);
> > > }
> > 
> > I believe Start() is only called from the GPUTracer when IsTracing() is
false,
> > which does the category checking.
> 
> True, but IsTracing() returns true both (*gpu_trace_srv_category != 0) ||
> (*gpu_trace_dev_category != 0).
> 
> If the user turns on gpu.device, won't they effectively get the gpu.service
> events as well?  Checking for *gpu_trace_srv_category will block that.

Good point, done.

https://codereview.chromium.org/776603003/diff/100001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_tracer.cc:140:
TRACE_EVENT_COPY_ASYNC_END0(category().c_str(), name().c_str(), this);
On 2014/12/08 23:45:38, David Yen wrote:
> On 2014/12/08 23:23:06, vmiura wrote:
> > And here.
> > 
> > if (*gpu_trace_srv_category) {
> >   ...
> > }
> 
> Same reason as above.

Done.

[vmiura, 2014-12-09 00:20:17]

vmiura@chromium.org changed reviewers:
+ jamesr@chromium.org

[vmiura, 2014-12-09 00:20:18]

jamesr@chromium.org: Please review changes in
  mojo/public/c/gles2/gles2_call_visitor_autogen.h

[vmiura, 2014-12-09 00:21:03]

LGTM for command_buffer changes.

[jamesr, 2014-12-09 00:52:03]

lgtm

[David Yen, 2014-12-09 17:33:07]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2014-12-09 17:33:31]

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

[David Yen, 2014-12-09 18:29:19]

The CQ bit was unchecked by dyen@chromium.org

[David Yen, 2014-12-09 18:33:03]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2014-12-09 18:34:19]

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

[commit-bot: I haz the power, 2014-12-09 18:35:49]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-12-09 18:36:35]

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