Added disjoint context class which manages disjoints within gpu timing.

gpu/command_buffer/service/gpu_timing.cc

Index: gpu/command_buffer/service/gpu_timing.cc
diff --git a/gpu/command_buffer/service/gpu_timing.cc b/gpu/command_buffer/service/gpu_timing.cc
index 8716448a4ad507c714a1b7c8a061271763d18906..e8472e04f597e287f9cf642686b54c2db889a345 100644
--- a/gpu/command_buffer/service/gpu_timing.cc
+++ b/gpu/command_buffer/service/gpu_timing.cc
@@ -62,10 +62,42 @@ int64 GPUTimer::GetDeltaElapsed() {
   return end - start;
 }
 
+DisjointContext::DisjointContext(GPUTiming* gpu_timing, bool disjoint_value)
+    : gpu_timing_(gpu_timing),

[Daniele Castagna, 2015/02/19 18:15:32]

Can gpu_timing be null?
I'd add a DCHECK.

[David Yen, 2015/02/19 19:00:47]

Done.

+      disjoint_value_(disjoint_value) {
+}
+
+bool DisjointContext::CheckAndResetTimerErrors() {
+  if (gpu_timing_) {

[Daniele Castagna, 2015/02/20 01:38:18]

Should we make sure that gpu_timing_ is set here?
I don't think anyone would want to use a DisjointContext after the gpu_timing_
has been reset, and this method would silently start to return always false.
That can be more confusing than an assertion.

+    gpu_timing_->UpdateDisjointContexts();
+  }
+  bool previous_disjoint_value = disjoint_value_;
+  disjoint_value_ = false;
+  return previous_disjoint_value;
+}
+
+void DisjointContext::ResetParentGPUTiming() {
+  gpu_timing_ = NULL;
+}
+
+void DisjointContext::SetDisjoint() {
+  disjoint_value_ = true;
+}
+
+DisjointContext::~DisjointContext() {
+  if (gpu_timing_) {
+    gpu_timing_->RemoveDisjointContext(this);
+  }
+}
+
 GPUTiming::GPUTiming() : cpu_time_for_testing_() {
 }
 
 GPUTiming::~GPUTiming() {
+  // Notify any disjoint contexts that the parent no longer exists.
+  for (DisjointContext* context_iter : disjoint_contexts_) {

[Daniele Castagna, 2015/02/19 18:15:32]

Do we really need to handle the case where GPUTiming gets delete before the
DisjointContextes it owns?

Can't we just assert that disjoint_contexts_.size() is 0 here if that should
never happen?

[David Yen, 2015/02/19 19:00:47]

Unfortunately I think this is necessary because GPUTiming returns scoped
references of DisjointContexts. The use case I am thinking of is you have a
DisjointContext within your class next to your GPUTiming object (we will most
likely have something like this in the gles2_cmd_decoder for the WebGL Traces).
If we do not handle both cases we are at the mercy of the order the destructors
are called.

[Daniele Castagna, 2015/02/19 20:55:52]

Isn't the lifetime of those objects completely controlled by us?
If GPUTiming gets deleted you end up with DisjointContexts that are still
allocated but can't really do anything.

If you feel strong about this, since you have a better idea of the potential use
cases, I'd suggest at least to add a test that covers this code.

[David Yen, 2015/02/19 22:48:03]

I've added a test for this.

I'm not referring to a case where you would want to continue using the disjoint
context after the GPUTiming has been destroyed. I'm simply talking about the
order of the destruction between the 2 objects. Here is a simple example:

{
  struct Test {
    scoped_refptr<DisjointContext> disjoint_context;
    GPUTiming gpu_timing;
  } test;
  test.disjoint_context = test.gpu_timing.CreateDisjointContext();
}

Because of the destruction order (~GPUTiming(), then ~DisjointContext()), this
will cause a crash without that code.

Technically the lifetime of the objects is controlled by us (user of the code),
but I can't enforce such a strange limitation - that if they are declared at the
same stack level, the DisjointContext must be declared afterwards.

[Daniele Castagna, 2015/02/20 01:38:18]

Sorry if I insist on this, but I still stand with my original suggestion.
Where do you think we're going to use those objects in that way? What you added
to the tests seems a really particular use case. A simple
DCHECK_EQ(disjoint_contexts_.size(), 0) in the destructor would make it clear
not to use it in that way, it would allow you to reduce the code, it would
introduce the nice invariant of having DisjointContext::gpu_timing_ always !=
NULL and you could avoid adding if(gpu_timing_) checks before dereferencing it.

[David Yen, 2015/02/20 02:02:57]

We are going to use those objects in that way in gles2_cmd_decoder.cc. Next to
line 1886 where we declare GPUTiming gpu_timing_, we will have a
scoped_refptr<DisjointContext> disjoint_context_ member which will be
initialized somewhere after line 2450 (most likely right after). This disjoint
context will handle disjoint commands called from the client into the command
buffer, which are calls to DoGetParameter() with the GPU_DISJOINT_EXT argument.

+    context_iter->ResetParentGPUTiming();
+  }
 }
 
 bool GPUTiming::Initialize(gfx::GLContext* gl_context) {
@@ -89,6 +121,11 @@ bool GPUTiming::IsAvailable() {
   return timer_type_ != kTimerTypeInvalid;
 }
 
+scoped_refptr<DisjointContext> GPUTiming::CreateDisjointContext() {
+  disjoint_contexts_.push_back(new DisjointContext(this, disjoint_occurred_));
+  return disjoint_contexts_.back();
+}
+
 const char* GPUTiming::GetTimerTypeName() const {
   switch (timer_type_) {
     case kTimerTypeDisjoint:
@@ -100,16 +137,6 @@ const char* GPUTiming::GetTimerTypeName() const {
   }
 }
 
-bool GPUTiming::CheckAndResetTimerErrors() {
-  if (timer_type_ == kTimerTypeDisjoint) {
-    GLint disjoint_value = 0;
-    glGetIntegerv(GL_GPU_DISJOINT_EXT, &disjoint_value);
-    return disjoint_value != 0;
-  } else {
-    return false;
-  }
-}
-
 int64 GPUTiming::CalculateTimerOffset() {
   if (!offset_valid_) {
     GLint64 gl_now = 0;
@@ -142,4 +169,34 @@ void GPUTiming::SetTimerTypeForTesting(TimerType type) {
   timer_type_ = type;
 }
 
+void GPUTiming::UpdateDisjointContexts() {
+  if (timer_type_ == kTimerTypeDisjoint) {
+    GLint disjoint_value = 0;
+    glGetIntegerv(GL_GPU_DISJOINT_EXT, &disjoint_value);
+    if (disjoint_value != 0) {

[Daniele Castagna, 2015/02/19 18:15:32]

I personally like being explicit with != 0, but in a previous review they
suggested me to omit it.

[David Yen, 2015/02/19 19:00:47]

I just moved this code from the previous version line 107, I can omit it though.

[Daniele Castagna, 2015/02/19 20:55:53]

It was just FYI, I didn't necessarily expect any action on this comment.

+      disjoint_occurred_ = true;
+      for (DisjointContext* context_iter : disjoint_contexts_) {

[Daniele Castagna, 2015/02/19 18:15:32]

nit: "context_iter" is not an iterator, I'd rename it just "context".

[David Yen, 2015/02/19 19:00:47]

Done.

+        context_iter->SetDisjoint();
+      }
+    }
+  }
+}
+
+void GPUTiming::RemoveDisjointContext(DisjointContext* context) {
+  bool item_found = false;

[Daniele Castagna, 2015/02/19 18:15:32]

Assuming we can use algorithms from stl in Chromium.
Wouldn't this be clearer with the erase-removeif idiom?

It'd be something like:
disjoint_contexts_.erase(std::remove_if(...

[David Yen, 2015/02/19 19:00:47]

I was trying to avoid shifting memory because this is a vector, but I would be
willing to replace it if you think swapping to the end and then deleting is too
complicated.

[Daniele Castagna, 2015/02/19 20:55:53]

If you want to avoid shifting a few pointers, what about something like:

auto found = std::find(
    disjoint_contexts_.begin(), disjoint_contexts_.end(), context);
DCHECK_NE(found, disjoint_contexts_.end());
*found = disjoint_contexts_.back();
disjoint_contexts_.pop_back();

[David Yen, 2015/02/19 22:48:03]

Done.

+  auto last_item = disjoint_contexts_.end() - 1;
+  for (auto context_iter = disjoint_contexts_.begin();
+       context_iter != disjoint_contexts_.end();
+       ++context_iter) {
+    if (*context_iter == context) {
+      item_found = true;
+      if (context_iter != last_item) {
+        std::iter_swap(context_iter, last_item);
+      }
+    }
+  }
+  DCHECK(item_found);
+  disjoint_contexts_.resize(disjoint_contexts_.size()-1);
+}
+
 }  // namespace gpu