Hookup the GPUTrace events.

Hookup the GPUTrace events.

Add a factory to GPUTrace to produce either a GL_ARB_timer_query GPUTrace object
or a SystemTime GPUTrace object.

Hookup GPUTraceController to GLES2DecoderImpl to process any received traces.

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

[jonathan.backer, 2012-11-23 18:55:41]

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9364: return error::kNoError;
Do we return an error here?

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:36: friend GPUTracer::Outputter;
Why friend? To read the name? You could hide the Trace class in an anon
namespace.

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:45: public base::NonThreadSafe {
base::NonThreadSafe is for sanity checking that your code is running on the
right thread. There's some documentation in comments in
base/threading/non_thread_safe.h

Ping me if you have questions.

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:47: static
linked_ptr<GPUTracer::Outputter> Create(const std::string& name) {
Why a linked_ptr? And why a factory?

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:345: current_trace_ =
linked_ptr<GPUTracer::Trace>(NULL);
I think that you can just call .reset() here.

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
File gpu/command_buffer/service/gpu_tracer.h (right):

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.h:26: static scoped_ptr<GPUTracer>
create();
nit: Create()

I think that we only use C style for simple getters/setters.

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.h:31: bool Begin(const std::string& name);
This is good. But eventually you'll want an interface for this (everything is
pure virtual except the destructor). Probably now to justify the factory.

The actual implementations would be in an anon namespace in the .cc file.

[dsinclair, 2012-11-23 21:13:55]

PTAL

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9364: return error::kNoError;
On 2012/11/23 18:55:41, jonathan.backer wrote:
> Do we return an error here?

From looking at the other methods in this file, I think kNoError is correct.

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:36: friend GPUTracer::Outputter;
On 2012/11/23 18:55:41, jonathan.backer wrote:
> Why friend? To read the name? You could hide the Trace class in an anon
> namespace.

Friend'd to give access to the start_time and end_time methods.

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:45: public base::NonThreadSafe {
On 2012/11/23 18:55:41, jonathan.backer wrote:
> base::NonThreadSafe is for sanity checking that your code is running on the
> right thread. There's some documentation in comments in
> base/threading/non_thread_safe.h
> 
> Ping me if you have questions.

copy-paste. Removed.

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:47: static
linked_ptr<GPUTracer::Outputter> Create(const std::string& name) {
On 2012/11/23 18:55:41, jonathan.backer wrote:
> Why a linked_ptr? And why a factory?

Was half an implementation of something else. Changed back to a scoped_refptr.

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:345: current_trace_ =
linked_ptr<GPUTracer::Trace>(NULL);
On 2012/11/23 18:55:41, jonathan.backer wrote:
> I think that you can just call .reset() here.

Done.

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
File gpu/command_buffer/service/gpu_tracer.h (right):

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.h:26: static scoped_ptr<GPUTracer>
create();
On 2012/11/23 18:55:41, jonathan.backer wrote:
> nit: Create()
> 
> I think that we only use C style for simple getters/setters.

Done.

https://codereview.chromium.org/11416117/diff/2006/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.h:31: bool Begin(const std::string& name);
On 2012/11/23 18:55:41, jonathan.backer wrote:
> This is good. But eventually you'll want an interface for this (everything is
> pure virtual except the destructor). Probably now to justify the factory.
> 
> The actual implementations would be in an anon namespace in the .cc file.

Done.

[dsinclair, 2012-11-30 16:54:18]

PTAL

[jonathan.backer, 2012-11-30 17:59:04]

https://codereview.chromium.org/11416117/diff/5003/gpu/command_buffer/service...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/5003/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:212: trace->end_time() +
timer_offset_));
Can't we have the |trace| do this? Then we can ditch Trace::Discard and just
no-op it when we don't want it. I think that this would also simplify the Trace
API.

I think that this could avoid the strdup as well --- we'd just hold on to the
Trace via a ref from the post task.

[dsinclair, 2012-11-30 19:45:15]

https://codereview.chromium.org/11416117/diff/5003/gpu/command_buffer/service...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/5003/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:212: trace->end_time() +
timer_offset_));
On 2012/11/30 17:59:04, jonathan.backer wrote:
> Can't we have the |trace| do this? Then we can ditch Trace::Discard and just
> no-op it when we don't want it. I think that this would also simplify the
Trace
> API.
> 
> I think that this could avoid the strdup as well --- we'd just hold on to the
> Trace via a ref from the post task.

Done.

[jonathan.backer, 2012-11-30 20:25:45]

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:35: protected:
private?

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:50: Outputter*
Outputter::g_outputter_thread = NULL;
nit: newline?

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:60: virtual void
Process(scoped_refptr<Outputter> output) = 0;
I'd just pass this in the constructor and hold a ref. Then the no-op doesn't
have it.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:67: return name_.c_str();
Do we need this for the no-op?

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:80: explicit GLARBTimerTrace(const
std::string& name, int64 offset);
no longer need explicit

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:81: 
// Implementation of gpu::Tracer.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:82: void Start() OVERRIDE;
still virtual.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:104: NoopTrace(const std::string& name)
: Trace(name) {}
explicit

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:106: void Start() {}
see above

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:118: class GPUTracerImpl : public
GPUTracer {
I know that you want to future proof, but are we going to subclass this if we
don't have ARB_timer_query?

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:125: // Begin a trace.
// Implementation of gpu::GPUTracer.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:162:
GPUTracerARBTimerQuery::GPUTracerARBTimerQuery() : GPUTracerImpl() {
I think that we list all the class declarations and then implement them in order
of the declarations.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:222: void
GLARBTimerTrace::Process(scoped_refptr<Outputter> output) {
DCHECK(IsAvailable())?

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:255: }
nit: braces

[dsinclair, 2012-11-30 21:51:41]

PTAL.

I've added the modifications to GLES2Implementation and GLES2DecoderImpl to
output the passed name instead of a static string in the ASYNC events.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:35: protected:
On 2012/11/30 20:25:45, jonathan.backer wrote:
> private?

Done.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:50: Outputter*
Outputter::g_outputter_thread = NULL;
On 2012/11/30 20:25:45, jonathan.backer wrote:
> nit: newline?

Done.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:60: virtual void
Process(scoped_refptr<Outputter> output) = 0;
On 2012/11/30 20:25:45, jonathan.backer wrote:
> I'd just pass this in the constructor and hold a ref. Then the no-op doesn't
> have it.

Done.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:67: return name_.c_str();
On 2012/11/30 20:25:45, jonathan.backer wrote:
> Do we need this for the no-op?

Yes, we'll need to use this to get the name for the service ASYNC trace output.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:80: explicit GLARBTimerTrace(const
std::string& name, int64 offset);
On 2012/11/30 20:25:45, jonathan.backer wrote:
> no longer need explicit

Done.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:81: 
On 2012/11/30 20:25:45, jonathan.backer wrote:
> // Implementation of gpu::Tracer.

Done.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:82: void Start() OVERRIDE;
On 2012/11/30 20:25:45, jonathan.backer wrote:
> still virtual.

Done.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:104: NoopTrace(const std::string& name)
: Trace(name) {}
On 2012/11/30 20:25:45, jonathan.backer wrote:
> explicit

Done.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:106: void Start() {}
On 2012/11/30 20:25:45, jonathan.backer wrote:
> see above

Done.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:118: class GPUTracerImpl : public
GPUTracer {
On 2012/11/30 20:25:45, jonathan.backer wrote:
> I know that you want to future proof, but are we going to subclass this if we
> don't have ARB_timer_query?

I think so, there is nothing ARB_timer_query specific inside this code. It
delegates to subclass to create the right Trace object which is where the
specific implementation code lives.

The Begin, End and Process calls are all really generic.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:125: // Begin a trace.
On 2012/11/30 20:25:45, jonathan.backer wrote:
> // Implementation of gpu::GPUTracer.

Done.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:162:
GPUTracerARBTimerQuery::GPUTracerARBTimerQuery() : GPUTracerImpl() {
On 2012/11/30 20:25:45, jonathan.backer wrote:
> I think that we list all the class declarations and then implement them in
order
> of the declarations.

Done.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:222: void
GLARBTimerTrace::Process(scoped_refptr<Outputter> output) {
On 2012/11/30 20:25:45, jonathan.backer wrote:
> DCHECK(IsAvailable())?

Done.

https://codereview.chromium.org/11416117/diff/15002/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:255: }
On 2012/11/30 20:25:45, jonathan.backer wrote:
> nit: braces

Done.

[jonathan.backer, 2012-12-03 14:23:13]

Just nits.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/client/...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3616:
current_trace_name_.reset(new std::string(name));
Can we do a client side check if this is already set? That would be an error
because we don't allow nesting.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3625:
TRACE_EVENT_COPY_ASYNC_END0("gpu", current_trace_name_->c_str(), this);
else ERROR?

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9352: 
nit: nix extra new line

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:8: #include <stack>
necessary?

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:12: #include
"base/threading/non_thread_safe.h"
necessary?

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:65: 
nit: extra new-line

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:67: protected:
private? I don't see why you'd want to subclass this.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:69: virtual ~Trace() {};
semi-colon?

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:95: scoped_refptr<Outputter> output_;
nit: s/output_/outputter_?

I think that instances of this class can be destroyed on a separate thread, so
Outputter needs to be RefCountedThreadSafe as well.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:157: virtual scoped_refptr<Trace>
CreateTrace(const std::string& name) OVERRIDE;
why did this become public? seems like it should be private to prevent
subclassing (not sure on this point).

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:209: GLuint queries[] = {start_timer_,
end_timer_};
nit: maybe just store the timer ids in an array to avoid packing/unpacking? your
call.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:271: glDeleteQueries(1, &query);
seems like we should get |system_now| as close as possible to |gl_now|

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
File gpu/command_buffer/service/gpu_tracer.h (right):

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.h:9: #include <string>
necessary? (maybe if you return std::string as described below)

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.h:11: #include "base/basictypes.h"
necessary?

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.h:34: // Retrieve the name of the current
trace.
current trace ==> current open trace?

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.h:35: virtual const char* CurrentName() =
0;
This is generally an unsafe API because someone can hold the pointer and the
char array could go out of scope. Return a std::string and forget about it?

nit: make this a const method?

[dsinclair, 2012-12-03 16:47:41]

PTAL.

I've also updated to re-calculate the drift each second when tracing is enabled.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/client/...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3616:
current_trace_name_.reset(new std::string(name));
On 2012/12/03 14:23:14, jonathan.backer wrote:
> Can we do a client side check if this is already set? That would be an error
> because we don't allow nesting.

Done.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3625:
TRACE_EVENT_COPY_ASYNC_END0("gpu", current_trace_name_->c_str(), this);
On 2012/12/03 14:23:14, jonathan.backer wrote:
> else ERROR?

Done.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9352: 
On 2012/12/03 14:23:14, jonathan.backer wrote:
> nit: nix extra new line

Done.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:8: #include <stack>
On 2012/12/03 14:23:14, jonathan.backer wrote:
> necessary?

Done.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:12: #include
"base/threading/non_thread_safe.h"
On 2012/12/03 14:23:14, jonathan.backer wrote:
> necessary?

Done.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:65: 
On 2012/12/03 14:23:14, jonathan.backer wrote:
> nit: extra new-line

Done.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:67: protected:
On 2012/12/03 14:23:14, jonathan.backer wrote:
> private? I don't see why you'd want to subclass this.

Done.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:69: virtual ~Trace() {};
On 2012/12/03 14:23:14, jonathan.backer wrote:
> semi-colon?

Done.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:95: scoped_refptr<Outputter> output_;
On 2012/12/03 14:23:14, jonathan.backer wrote:
> nit: s/output_/outputter_?
> 
> I think that instances of this class can be destroyed on a separate thread, so
> Outputter needs to be RefCountedThreadSafe as well.

Done.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:157: virtual scoped_refptr<Trace>
CreateTrace(const std::string& name) OVERRIDE;
On 2012/12/03 14:23:14, jonathan.backer wrote:
> why did this become public? seems like it should be private to prevent
> subclassing (not sure on this point).

Done.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:209: GLuint queries[] = {start_timer_,
end_timer_};
On 2012/12/03 14:23:14, jonathan.backer wrote:
> nit: maybe just store the timer ids in an array to avoid packing/unpacking?
your
> call.

Done.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.cc:271: glDeleteQueries(1, &query);
On 2012/12/03 14:23:14, jonathan.backer wrote:
> seems like we should get |system_now| as close as possible to |gl_now|

Done.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
File gpu/command_buffer/service/gpu_tracer.h (right):

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.h:9: #include <string>
On 2012/12/03 14:23:14, jonathan.backer wrote:
> necessary? (maybe if you return std::string as described below)

Done.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.h:11: #include "base/basictypes.h"
On 2012/12/03 14:23:14, jonathan.backer wrote:
> necessary?

This is needed for the DISALLOW_COPY_AND_ASSIGN macro.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.h:34: // Retrieve the name of the current
trace.
On 2012/12/03 14:23:14, jonathan.backer wrote:
> current trace ==> current open trace?

Done.

https://codereview.chromium.org/11416117/diff/4008/gpu/command_buffer/service...
gpu/command_buffer/service/gpu_tracer.h:35: virtual const char* CurrentName() =
0;
On 2012/12/03 14:23:14, jonathan.backer wrote:
> This is generally an unsafe API because someone can hold the pointer and the
> char array could go out of scope. Return a std::string and forget about it?
> 
> nit: make this a const method?

Done.

[jonathan.backer, 2012-12-03 18:29:32]

Just a few more nits.

https://codereview.chromium.org/11416117/diff/10014/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/11416117/diff/10014/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3612: if
(current_trace_name_.get() != NULL) {
|if (current_trace_name_)| ?

https://codereview.chromium.org/11416117/diff/10014/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3627: if
(current_trace_name_.get() == NULL) {
I think that we can just |!current_trace_name_|

https://codereview.chromium.org/11416117/diff/10014/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/10014/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:137: scoped_refptr<Outputter>
outputter_;
Does this need an |outputter_|?

https://codereview.chromium.org/11416117/diff/10014/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:275: (last_offset_check_ +
base::Time::kMicrosecondsPerSecond) <
How did you come up with this value? Is every 10s good enough?

https://codereview.chromium.org/11416117/diff/10014/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:283: TRACE_EVENT_BEGIN0("gpu",
"CalculateTimerOffset");
Just a regular TRACE_EVENT0?

[dsinclair, 2012-12-03 21:40:04]

https://codereview.chromium.org/11416117/diff/10014/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/11416117/diff/10014/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3612: if
(current_trace_name_.get() != NULL) {
On 2012/12/03 18:29:32, jonathan.backer wrote:
> |if (current_trace_name_)| ?

Done.

https://codereview.chromium.org/11416117/diff/10014/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3627: if
(current_trace_name_.get() == NULL) {
On 2012/12/03 18:29:32, jonathan.backer wrote:
> I think that we can just |!current_trace_name_|

Done.

https://codereview.chromium.org/11416117/diff/10014/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/10014/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:137: scoped_refptr<Outputter>
outputter_;
On 2012/12/03 18:29:32, jonathan.backer wrote:
> Does this need an |outputter_|?

Done.

https://codereview.chromium.org/11416117/diff/10014/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:275: (last_offset_check_ +
base::Time::kMicrosecondsPerSecond) <
On 2012/12/03 18:29:32, jonathan.backer wrote:
> How did you come up with this value? Is every 10s good enough?

It was what I saw a couple of other examples using. There does appear to be
drift even after 1s, so this seems to keep it in line a lot better.

https://codereview.chromium.org/11416117/diff/10014/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:283: TRACE_EVENT_BEGIN0("gpu",
"CalculateTimerOffset");
On 2012/12/03 18:29:32, jonathan.backer wrote:
> Just a regular TRACE_EVENT0?

Done.

[dsinclair, 2012-12-03 21:47:33]

@apatrick_chromium Can you please take a look for GPU OWNERS?

Thanks,
dan

[apatrick, 2012-12-03 22:05:41]

https://codereview.chromium.org/11416117/diff/10027/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/11416117/diff/10027/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6452: gpu_tracer_->Process();
How about contexts that we never call SwapBuffers on, like WebGL contexts?

https://codereview.chromium.org/11416117/diff/10027/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/10027/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:41: static Outputter*
g_outputter_thread;
You could just make this a regular global in an anonymous namespace. If you want
to leave it like this it should be called outputter_thread_. See also
LazyInstance in base as another alternative.

https://codereview.chromium.org/11416117/diff/10027/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.h (right):

https://codereview.chromium.org/11416117/diff/10027/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.h:23: virtual ~GPUTracer() {}
Have you checked to see if clang will treat this as a warning as error. If it
does, you just need to move the definitions into the .cc file.

[dsinclair, 2012-12-04 15:59:27]

Al, thanks for pointing out the issue with WebGL. I'm going to go back and do
some re-design to better handle contexts that don't use SwapBuffers.

I'll ping you when I'm ready for a re-review.

https://codereview.chromium.org/11416117/diff/10027/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/11416117/diff/10027/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6452: gpu_tracer_->Process();
On 2012/12/03 22:05:41, apatrick wrote:
> How about contexts that we never call SwapBuffers on, like WebGL contexts?

Ah, crap. I didn't know that those didn't call SwapBuffers. I'm going to need to
make some changes to make this work. Sorry about that.

https://codereview.chromium.org/11416117/diff/10027/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/10027/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:41: static Outputter*
g_outputter_thread;
On 2012/12/03 22:05:41, apatrick wrote:
> You could just make this a regular global in an anonymous namespace. If you
want
> to leave it like this it should be called outputter_thread_. See also
> LazyInstance in base as another alternative.

Done.

https://codereview.chromium.org/11416117/diff/10027/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.h (right):

https://codereview.chromium.org/11416117/diff/10027/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.h:23: virtual ~GPUTracer() {}
On 2012/12/03 22:05:41, apatrick wrote:
> Have you checked to see if clang will treat this as a warning as error. If it
> does, you just need to move the definitions into the .cc file.

I've been using clang as my compiler, doesn't seem to be complaining about this.

[jonathan.backer, 2012-12-04 16:44:38]

https://codereview.chromium.org/11416117/diff/10027/gpu/command_buffer/servic...
> File gpu/command_buffer/service/gpu_tracer.h (right):
> 
>
https://codereview.chromium.org/11416117/diff/10027/gpu/command_buffer/servic...
> gpu/command_buffer/service/gpu_tracer.h:23: virtual ~GPUTracer() {}
> On 2012/12/03 22:05:41, apatrick wrote:
> > Have you checked to see if clang will treat this as a warning as error. If
it
> > does, you just need to move the definitions into the .cc file.
> 
> I've been using clang as my compiler, doesn't seem to be complaining about
this.

Have you been using the chromium clang plugins as described here:
http://code.google.com/p/chromium/wiki/Clang ?

[dsinclair, 2012-12-04 19:25:22]

On 2012/12/04 16:44:38, jonathan.backer wrote:
> Have you been using the chromium clang plugins as described here:
> http://code.google.com/p/chromium/wiki/Clang ?

I wasn't, but even with the plugins I don't see anything:

dsinclair@dsinclair:src (issue_gl_arb_commands) $ GYP_GENERATORS=ninja
GYP_DEFINES="clang=1 clang_use_chrome_plugins=1" ./build/gyp_chromium
Updating projects from gyp files...
Using parallel processing (experimental).
dsinclair@dsinclair:src (issue_gl_arb_commands) $ ninja -C out/Debug chrome
ninja: Entering directory `out/Debug'
[11224/11224] LINK chrome

[jonathan.backer, 2012-12-07 15:38:11]

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:26: : public base::Thread,
nit: just an idea. maybe you want private inheritance or to own the thread
instead of inherit now. seems like you don't want anyone calling
outputter_->message_loop()->PostTask(...) anymore.

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:27: public
base::RefCountedThreadSafe<Outputter> {
nit: these don't need to be threadsafe anymore.

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:50: class Trace : public
base::RefCountedThreadSafe<Trace> {
nit: no need to be threadsafe.

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:63: virtual std::string name() {
nit: return |const std::string&|?

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:197: glGetQueryObjectiv(queries_[1],
GL_QUERY_RESULT_AVAILABLE, &done);
Just occurred to me, is there a problem if this is called before ::End()?

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:222: if (current_trace_.get())
nit: do you need the get()?

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:231: if (!current_trace_.get())
nit: need the get()?

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:281: if (!process_posted_) {
seems like you want to move this to GPUTracerImpl so that those get Process()ed?

You could check that there is a trace queued if you want to avoid unnecessarily
posting a task.

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:296: (last_offset_check_ +
base::Time::kMicrosecondsPerSecond) <
doing this every microsecond?

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.h (right):

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.h:35: virtual std::string CurrentName()
const = 0;
nit: maybe return a |const std::string&| to save a copy?

[dsinclair, 2012-12-10 16:59:02]

PTAL.

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:26: : public base::Thread,
On 2012/12/07 15:38:11, jonathan.backer wrote:
> nit: just an idea. maybe you want private inheritance or to own the thread
> instead of inherit now. seems like you don't want anyone calling
> outputter_->message_loop()->PostTask(...) anymore.
> 

Done.

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:27: public
base::RefCountedThreadSafe<Outputter> {
On 2012/12/07 15:38:11, jonathan.backer wrote:
> nit: these don't need to be threadsafe anymore.

Done.

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:50: class Trace : public
base::RefCountedThreadSafe<Trace> {
On 2012/12/07 15:38:11, jonathan.backer wrote:
> nit: no need to be threadsafe.

Done.

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:63: virtual std::string name() {
On 2012/12/07 15:38:11, jonathan.backer wrote:
> nit: return |const std::string&|?

Done.

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:197: glGetQueryObjectiv(queries_[1],
GL_QUERY_RESULT_AVAILABLE, &done);
On 2012/12/07 15:38:11, jonathan.backer wrote:
> Just occurred to me, is there a problem if this is called before ::End()?

Done.

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:222: if (current_trace_.get())
On 2012/12/07 15:38:11, jonathan.backer wrote:
> nit: do you need the get()?

I believe so, current_trace_ exists, it just may not store anything as it's
pointer, right?

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:231: if (!current_trace_.get())
On 2012/12/07 15:38:11, jonathan.backer wrote:
> nit: need the get()?

As above.

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:281: if (!process_posted_) {
On 2012/12/07 15:38:11, jonathan.backer wrote:
> seems like you want to move this to GPUTracerImpl so that those get
Process()ed?
> 
> You could check that there is a trace queued if you want to avoid
unnecessarily
> posting a task.

Done.

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:296: (last_offset_check_ +
base::Time::kMicrosecondsPerSecond) <
On 2012/12/07 15:38:11, jonathan.backer wrote:
> doing this every microsecond?

Every second.

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.h (right):

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.h:35: virtual std::string CurrentName()
const = 0;
On 2012/12/07 15:38:11, jonathan.backer wrote:
> nit: maybe return a |const std::string&| to save a copy?

In that case, what do I do when there is no name available? Currently I have:

if (!current_trace_)
  return std::string("");

not sure what to return if I have a const std::string& return?

[jonathan.backer, 2012-12-10 21:31:02]

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.h (right):

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.h:35: virtual std::string CurrentName()
const = 0;
On 2012/12/10 16:59:03, dsinclair wrote:
> On 2012/12/07 15:38:11, jonathan.backer wrote:
> > nit: maybe return a |const std::string&| to save a copy?
> 
> In that case, what do I do when there is no name available? Currently I have:
> 
> if (!current_trace_)
>   return std::string("");
> 
> not sure what to return if I have a const std::string& return?

base/string_util.h defines an EmptyString(). It's part of a singleton.

https://codereview.chromium.org/11416117/diff/40001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:80:
GLARBTimerTrace(scoped_refptr<Outputter> output, const std::string& name,
s/output/outputter

https://codereview.chromium.org/11416117/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:222: outputter_->thread_id(), this,
start_time_);
I'm not sure that the private inheritance did what we thought it did if you can
still call |thread_id()| here. Have you tested?

https://codereview.chromium.org/11416117/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:246: if (!process_posted_) {
Maybe check that you have traces to process (in the queue). This might fix the
unit test that you modified without introducing a message loop.

https://codereview.chromium.org/11416117/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:262: }
Repost another delayed Process if the queue isn't empty?

https://codereview.chromium.org/11416117/diff/40001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.h (right):

https://codereview.chromium.org/11416117/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.h:32: virtual void Process() = 0;
This is no longer called by the GLES2CmdDecoder directly. I think that you can
hide this inside the implementation.

https://codereview.chromium.org/11416117/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.h:34: // Retrieve the name of the current
open trace.
// Returns empty string if no current open trace.

[dsinclair, 2012-12-14 16:06:22]

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:222: if (current_trace_.get())
On 2012/12/10 16:59:03, dsinclair wrote:
> On 2012/12/07 15:38:11, jonathan.backer wrote:
> > nit: do you need the get()?
> 
> I believe so, current_trace_ exists, it just may not store anything as it's
> pointer, right?

Done.

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:231: if (!current_trace_.get())
On 2012/12/10 16:59:03, dsinclair wrote:
> On 2012/12/07 15:38:11, jonathan.backer wrote:
> > nit: need the get()?
> 
> As above.

Done.

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.h (right):

https://codereview.chromium.org/11416117/diff/30001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.h:35: virtual std::string CurrentName()
const = 0;
On 2012/12/10 21:31:02, jonathan.backer wrote:
> On 2012/12/10 16:59:03, dsinclair wrote:
> > On 2012/12/07 15:38:11, jonathan.backer wrote:
> > > nit: maybe return a |const std::string&| to save a copy?
> > 
> > In that case, what do I do when there is no name available? Currently I
have:
> > 
> > if (!current_trace_)
> >   return std::string("");
> > 
> > not sure what to return if I have a const std::string& return?
> 
> base/string_util.h defines an EmptyString(). It's part of a singleton.

Done.

https://codereview.chromium.org/11416117/diff/40001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:80:
GLARBTimerTrace(scoped_refptr<Outputter> output, const std::string& name,
On 2012/12/10 21:31:02, jonathan.backer wrote:
> s/output/outputter

Done.

https://codereview.chromium.org/11416117/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:222: outputter_->thread_id(), this,
start_time_);
On 2012/12/10 21:31:02, jonathan.backer wrote:
> I'm not sure that the private inheritance did what we thought it did if you
can
> still call |thread_id()| here. Have you tested?

Doh, the problem with have stuff you're going to re-enable when you've merged.
Added an Id method to the outputter which will give the thread_id back to me.

https://codereview.chromium.org/11416117/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:246: if (!process_posted_) {
On 2012/12/10 21:31:02, jonathan.backer wrote:
> Maybe check that you have traces to process (in the queue). This might fix the
> unit test that you modified without introducing a message loop.

Done.

https://codereview.chromium.org/11416117/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:262: }
On 2012/12/10 21:31:02, jonathan.backer wrote:
> Repost another delayed Process if the queue isn't empty?

Done.

https://codereview.chromium.org/11416117/diff/40001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.h (right):

https://codereview.chromium.org/11416117/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.h:32: virtual void Process() = 0;
On 2012/12/10 21:31:02, jonathan.backer wrote:
> This is no longer called by the GLES2CmdDecoder directly. I think that you can
> hide this inside the implementation.

Done.

https://codereview.chromium.org/11416117/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.h:34: // Retrieve the name of the current
open trace.
On 2012/12/10 21:31:02, jonathan.backer wrote:
> // Returns empty string if no current open trace.

Done.

[jonathan.backer, 2012-12-14 20:48:28]

lgtm. You'll need owners.

https://codereview.chromium.org/11416117/diff/52001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/11416117/diff/52001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9656: return error::kNoError;
Why return here rather than the next line?

https://codereview.chromium.org/11416117/diff/52001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/52001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:151: void issue_process_task();
I think the style guidelines are IssueProcessTask(). AFAIK, C-style methods are
for simple setters and getters.

[dsinclair, 2012-12-14 21:26:04]

https://codereview.chromium.org/11416117/diff/52001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/11416117/diff/52001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9656: return error::kNoError;
On 2012/12/14 20:48:29, jonathan.backer wrote:
> Why return here rather than the next line?

Paranoia? I originally had code after this if block. Left the return in in case
we added code after again. Wanted to make sure this would return.

https://codereview.chromium.org/11416117/diff/52001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/52001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:151: void issue_process_task();
On 2012/12/14 20:48:29, jonathan.backer wrote:
> I think the style guidelines are IssueProcessTask(). AFAIK, C-style methods
are
> for simple setters and getters.

Done.

[apatrick_chromium, 2012-12-14 23:28:54]

https://codereview.chromium.org/11416117/diff/55001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/55001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:24: static Outputter* kOutputterThread
= NULL;
nit: style for non-const globals is g_outputter_thread.

https://codereview.chromium.org/11416117/diff/55001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:33: kOutputterThread->Start();
Why start then immediately stop the thread? Is this to reserve a thread timeline
in the trace output? Also, this thread never seems to do anything?

[dsinclair, 2012-12-17 14:56:23]

https://codereview.chromium.org/11416117/diff/55001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/11416117/diff/55001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:24: static Outputter* kOutputterThread
= NULL;
On 2012/12/14 23:28:54, apatrick_chromium wrote:
> nit: style for non-const globals is g_outputter_thread.

Done.

https://codereview.chromium.org/11416117/diff/55001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_tracer.cc:33: kOutputterThread->Start();
On 2012/12/14 23:28:54, apatrick_chromium wrote:
> Why start then immediately stop the thread? Is this to reserve a thread
timeline
> in the trace output? Also, this thread never seems to do anything?

We have to start and stop the thread so it will be allocated a thread ID and
have it's thread name set. We don't do anything with the thread directly after
this point, we just use the thread_id when we send events to the trace
framework.

This way, we get our output on it's own line in chrome://tracing which makes
things a lot easier to read.

[apatrick_chromium, 2012-12-17 20:03:46]

LGTM.

[commit-bot: I haz the power, 2013-02-20 19:55:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dsinclair@chromium.org/11416117/74004

[commit-bot: I haz the power, 2013-02-20 23:02:16]

Change committed as 183648