Fixed not-unique event 'id' in TRACE_EVENT from GLARBTimerTrace

gpu/command_buffer/service/gpu_tracer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "gpu/command_buffer/service/gpu_tracer.h"
 
 #include <deque>
 
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
-#include "base/memory/weak_ptr.h"
 #include "base/strings/string_util.h"
-#include "base/threading/thread.h"
 #include "base/time/time.h"
-#include "ui/gl/gl_bindings.h"
 
 namespace gpu {
 namespace gles2 {
-namespace {
-
-class Outputter;
 
 static const unsigned int kProcessInterval = 16;
-static Outputter* g_outputter_thread = NULL;
+static TraceOutputter* g_outputter_thread = NULL;
 
-class Outputter
-    : private base::Thread,
-      public base::RefCounted<Outputter> {
- public:
-  static scoped_refptr<Outputter> Create(const std::string& name) {
-    if (!g_outputter_thread) {
-      g_outputter_thread = new Outputter(name);
-      g_outputter_thread->Start();
-      g_outputter_thread->Stop();
-    }
-    return g_outputter_thread;
+scoped_refptr<TraceOutputter> TraceOutputter::Create(const std::string& name) {
+  if (!g_outputter_thread) {
+    g_outputter_thread = new TraceOutputter(name);
+    g_outputter_thread->Start();
+    g_outputter_thread->Stop();

[piman, 2014/01/07 22:47:49]

Are we really creating a thread just to get a unique id? :/

[vmiura, 2014/01/07 23:26:47]

Yep :/.  This thread use is not part of my change btw.

I think it was intended to give a name to the trace in about:tracing (but it
doesn't actually work).

I'd like to remove this later with some updates in about:tracing.

   }
+  return g_outputter_thread;
+}
 
-  uint64 Id() { return thread_id(); }
+TraceOutputter::TraceOutputter(const std::string& name)
+    : base::Thread(name.c_str()),
+    local_trace_id_(0) {
+}
 
- private:
-  friend class base::RefCounted<Outputter>;
+TraceOutputter::~TraceOutputter() {
+  g_outputter_thread = NULL;
+}
 
-  explicit Outputter(const std::string& name) : base::Thread(name.c_str()) {}
+void TraceOutputter::Trace(const std::string& name,
+                          int64 start_time,

[piman, 2014/01/07 22:47:49]

nit: indentation ('int64' needs to be right under 'const')

[vmiura, 2014/01/07 23:26:47]

Done.

+                          int64 end_time) {
+  TRACE_EVENT_COPY_BEGIN_WITH_ID_TID_AND_TIMESTAMP0("gpu", name.c_str(),
+                                   local_trace_id_, thread_id(), start_time);

[piman, 2014/01/07 22:47:49]

nit: indentation for the line continuation (either +4, or aligned under "gpu").

Maybe try git cl format.

[vmiura, 2014/01/07 23:26:47]

Done.

+  TRACE_EVENT_COPY_END_WITH_ID_TID_AND_TIMESTAMP0("gpu", name.c_str(),
+                                   local_trace_id_, thread_id(), end_time);

[piman, 2014/01/07 22:47:49]

nit: indentation

[vmiura, 2014/01/07 23:26:47]

Done.

+  ++local_trace_id_;
+}
 
-  virtual ~Outputter() {
-    g_outputter_thread = NULL;
-  }
-
-  DISALLOW_COPY_AND_ASSIGN(Outputter);
-};
-
-class Trace : public base::RefCounted<Trace> {
- public:
-  explicit Trace(const std::string& name) : name_(name) {}
-
-  virtual void Start() = 0;
-  virtual void End() = 0;
-
-  // True if the the results of this query are available.
-  virtual bool IsAvailable() = 0;
-
-  virtual bool IsProcessable() { return true; }
-  virtual void Process() = 0;
-
-  virtual const std::string& name() {
-    return name_;
-  }
-
- protected:
-  virtual ~Trace() {}
-
- private:
-  friend class base::RefCounted<Trace>;
-
-  std::string name_;
-
-  DISALLOW_COPY_AND_ASSIGN(Trace);
-};
-
-class GLARBTimerTrace : public Trace {
- public:
-  GLARBTimerTrace(scoped_refptr<Outputter> outputter, const std::string& name,
-                  int64 offset);
-
-  // Implementation of Tracer
-  virtual void Start() OVERRIDE;
-  virtual void End() OVERRIDE;
-  virtual bool IsAvailable() OVERRIDE;
-  virtual void Process() OVERRIDE;
-
- private:
-  virtual ~GLARBTimerTrace();
-
-  void Output();
-
-  scoped_refptr<Outputter> outputter_;
-
-  int64 offset_;
-  int64 start_time_;
-  int64 end_time_;
-  bool end_requested_;
-
-  GLuint queries_[2];
-
-  DISALLOW_COPY_AND_ASSIGN(GLARBTimerTrace);
-};
 
 class NoopTrace : public Trace {
  public:
   explicit NoopTrace(const std::string& name) : Trace(name) {}
 
   // Implementation of Tracer
   virtual void Start() OVERRIDE {}
   virtual void End() OVERRIDE {}
   virtual bool IsAvailable() OVERRIDE { return true; }
   virtual bool IsProcessable() OVERRIDE { return false; }
@@ skipping 56 matching lines @@
   void CalculateTimerOffset();
 
   scoped_refptr<Outputter> outputter_;
 
   int64 timer_offset_;
   int64 last_offset_check_;
 
   DISALLOW_COPY_AND_ASSIGN(GPUTracerARBTimerQuery);
 };
 
-GLARBTimerTrace::GLARBTimerTrace(scoped_refptr<Outputter> outputter,
-                                 const std::string& name, int64 offset)
+bool Trace::IsProcessable() {
+  return true;
+}
+
+const std::string& Trace::name() {
+  return name_;
+}
+
+GLARBTimerTrace::GLARBTimerTrace(
+          scoped_refptr<Outputter> outputter,
+          const std::string& name, int64 offset)
     : Trace(name),
       outputter_(outputter),
       offset_(offset),
       start_time_(0),
       end_time_(0),
       end_requested_(false) {
   glGenQueries(2, queries_);
 }
 
 GLARBTimerTrace::~GLARBTimerTrace() {
@@ skipping 13 matching lines @@
     return false;
 
   GLint done = 0;
   glGetQueryObjectiv(queries_[1], GL_QUERY_RESULT_AVAILABLE, &done);
   return !!done;
 }
 
 void GLARBTimerTrace::Process() {
   DCHECK(IsAvailable());
 
-  GLint64 timestamp;
+  GLuint64 timestamp;
 
   // TODO(dsinclair): It's possible for the timer to wrap during the start/end.
   // We need to detect if the end is less then the start and correct for the
   // wrapping.
-  glGetQueryObjecti64v(queries_[0], GL_QUERY_RESULT, &timestamp);
+  glGetQueryObjectui64v(queries_[0], GL_QUERY_RESULT, &timestamp);
   start_time_ = (timestamp / base::Time::kNanosecondsPerMicrosecond) + offset_;
 
-  glGetQueryObjecti64v(queries_[1], GL_QUERY_RESULT, &timestamp);
+  glGetQueryObjectui64v(queries_[1], GL_QUERY_RESULT, &timestamp);
   end_time_ = (timestamp / base::Time::kNanosecondsPerMicrosecond) + offset_;
 
   glDeleteQueries(2, queries_);
-
-  TRACE_EVENT_COPY_BEGIN_WITH_ID_TID_AND_TIMESTAMP0("gpu", name().c_str(),
-                                   this, outputter_->Id(), start_time_);
-  TRACE_EVENT_COPY_END_WITH_ID_TID_AND_TIMESTAMP0("gpu", name().c_str(),
-                                   this, outputter_->Id(), end_time_);
+  outputter_->Trace(name(), start_time_, end_time_);
 }
 
 bool GPUTracerImpl::Begin(const std::string& name) {
   // Make sure we are not nesting trace commands.
   if (current_trace_.get())
     return false;
 
   current_trace_ = CreateTrace(name);
   current_trace_->Start();
   return true;
@@ skipping 42 matching lines @@
       FROM_HERE,
       base::Bind(&GPUTracerImpl::Process, base::AsWeakPtr(this)),
       base::TimeDelta::FromMilliseconds(kProcessInterval));
 }
 
 GPUTracerARBTimerQuery::GPUTracerARBTimerQuery()
     : GPUTracerImpl(),
       timer_offset_(0),
       last_offset_check_(0) {
   CalculateTimerOffset();
-  outputter_ = Outputter::Create("GL_ARB_timer_query");
+  outputter_ = TraceOutputter::Create("GL_ARB_timer_query");
 }
 
 GPUTracerARBTimerQuery::~GPUTracerARBTimerQuery() {
 }
 
 scoped_refptr<Trace> GPUTracerARBTimerQuery::CreateTrace(
     const std::string& name) {
   if (*gpu_category_enabled_)
     return new GLARBTimerTrace(outputter_, name, timer_offset_);
   return GPUTracerImpl::CreateTrace(name);
@@ skipping 19 matching lines @@
   glGetQueryObjectui64v(query, GL_QUERY_RESULT, &gl_now);
   base::TimeTicks system_now = base::TimeTicks::NowFromSystemTraceTime();
 
   gl_now /= base::Time::kNanosecondsPerMicrosecond;
   timer_offset_ = system_now.ToInternalValue() - gl_now;
   glDeleteQueries(1, &query);
 
   last_offset_check_ = system_now.ToInternalValue();
 }
 
-}  // namespace
-
 scoped_ptr<GPUTracer> GPUTracer::Create() {
-  if (gfx::g_driver_gl.ext.b_GL_ARB_timer_query)
+  if (gfx::g_driver_gl.ext.b_GL_ARB_timer_query) {
     return scoped_ptr<GPUTracer>(new GPUTracerARBTimerQuery());
+  }
   return scoped_ptr<GPUTracer>(new GPUTracerImpl());
 }
 
 }  // namespace gles2
 }  // namespace gpu