gpu: Reuse transfer buffers more aggresively

gpu/command_buffer/client/gles2_implementation.cc

Index: gpu/command_buffer/client/gles2_implementation.cc
diff --git a/gpu/command_buffer/client/gles2_implementation.cc b/gpu/command_buffer/client/gles2_implementation.cc
index 2d02579b116672e26da92edc5e66c7dcc6ae56b8..6f8ca1d2ac2012d699e33899550a52f10e62acca 100644
--- a/gpu/command_buffer/client/gles2_implementation.cc
+++ b/gpu/command_buffer/client/gles2_implementation.cc
@@ -1378,16 +1378,19 @@ void GLES2Implementation::BufferDataHelper(
 
     BufferTracker::Buffer* buffer = buffer_tracker_->GetBuffer(buffer_id);
     if (buffer) {
-      // Free buffer memory, pending the passage of a token.
-      buffer_tracker_->FreePendingToken(buffer, helper_->InsertToken());
+      if (buffer->used()) {
+        buffer->set_used(false);
+        buffer->set_last_usage_token(helper_->InsertToken());
+      }

[piman, 2014/01/11 02:02:32]

Where does the last_usage_token get set if it's not bound to
GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM ?

[jadahl, 2014/01/11 11:35:29]

Right, should do the same for other buffer targets in BindBufferHelper.

[reveman, 2014/01/11 23:39:04]

What if we set last usage token when issuing the command that will use the
buffer (ie. DrawArrays) instead of bind/unbind? Then you can get rid of
set_used() and the token would be more tightly packed with the usage. Also more
consistent with how we handle AsyncTexImage2D.

[jadahl, 2014/01/12 09:52:24]

I think this sounds like a good idea.

[jadahl, 2014/01/14 14:15:47]

Hmm, is what you mean to do this for the functions that deal with transfer
buffers? i.e. UnmapBufferCHROMIUM, (Compressed)(Sub)TexImage2D? and not
DrawArrays, which does not deal with pixel transfer buffers at all.

[reveman, 2014/01/14 15:27:20]

Not only transfer buffers but all buffer objects so that you can get rid of
set_used() completely. So both (Compressed)(Sub)TexImage2D and DrawArrays, as
well as any other command that can use a buffer object.

[reveman, 2014/01/14 15:54:26]

Sorry, DrawArrays doesn't use a buffer object as we don't have shared memory
VBOs. The general idea is that any command that will use the contents of a
buffer object on the service side needs a set_last_usage_token call. ie.
BufferSubData in case of VBOs and (Compressed)(Sub)TexImage2D in case of pixel
transfer buffers.

Think this will also allow us to remove set_transfer_ready_token().

[jadahl, 2014/01/14 16:32:09]

I don't see how BufferSubData would be used service side for a
BufferTracker::Buffer, as in those cases it'll be a simple client side memcpy.

[reveman, 2014/01/14 16:55:25]

Right. I guess it's really just (Compressed)(Sub)TexImage2D that need a
set_last_usage_token.

 
-      // Remove old buffer.
-      buffer_tracker_->RemoveBuffer(buffer_id);
+      FreeTransferBuffer(buffer);
+      buffer_tracker_->RemoveBuffer(buffer->id());
     }
 
     // Create new buffer.
     buffer = buffer_tracker_->CreateBuffer(buffer_id, size);
     DCHECK(buffer);
+    buffer->set_used(true);
     if (buffer->address() && data)
       memcpy(buffer->address(), data, size);
     return;
@@ -1511,6 +1514,25 @@ void GLES2Implementation::BufferSubData(
   CheckGLError();
 }
 
+void GLES2Implementation::FreeTransferBuffer(BufferTracker::Buffer* buffer) {
+  int32 token = buffer->last_usage_token();
+
+  if (buffer->async_query_id()) {
+    QueryTracker::Query* query =
+        query_tracker_->GetQuery(buffer->async_query_id());
+    if (!query->CheckResultsAvailable(helper_))
+      buffer_tracker_->FreePendingSerial(buffer, query->serial());
+    else
+      buffer_tracker_->Free(buffer);
+  } else if (token) {
+    if (helper_->HasTokenPassed(token))
+      buffer_tracker_->Free(buffer);
+    else
+      buffer_tracker_->FreePendingToken(buffer, token);
+  }
+  // TODO(jadahl): What to do if there is both token and query
+}
+
 bool GLES2Implementation::GetBoundPixelTransferBuffer(
     GLenum target,
     const char* function_name,
@@ -2403,24 +2425,42 @@ void GLES2Implementation::GenQueriesEXTHelper(
 // deleted the resource.
 
 bool GLES2Implementation::BindBufferHelper(
-    GLenum target, GLuint buffer) {
+    GLenum target, GLuint buffer_id) {
+  BufferTracker::Buffer* buffer;

[piman, 2014/01/11 02:02:32]

nit: = NULL

+
   // TODO(gman): See note #1 above.
   bool changed = false;
   switch (target) {
     case GL_ARRAY_BUFFER:
-      if (bound_array_buffer_id_ != buffer) {
-        bound_array_buffer_id_ = buffer;
+      if (bound_array_buffer_id_ != buffer_id) {
+        bound_array_buffer_id_ = buffer_id;
         changed = true;
       }
       break;
     case GL_ELEMENT_ARRAY_BUFFER:
-      changed = vertex_array_object_manager_->BindElementArray(buffer);
+      changed = vertex_array_object_manager_->BindElementArray(buffer_id);
       break;
     case GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM:
-      bound_pixel_pack_transfer_buffer_id_ = buffer;
+      bound_pixel_pack_transfer_buffer_id_ = buffer_id;
       break;
     case GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM:
-      bound_pixel_unpack_transfer_buffer_id_ = buffer;
+      // Mark any previously bound buffer as unused.
+      if (bound_pixel_unpack_transfer_buffer_id_ != buffer_id &&
+          bound_pixel_unpack_transfer_buffer_id_ != 0) {
+        buffer =
+            buffer_tracker_->GetBuffer(bound_pixel_unpack_transfer_buffer_id_);
+        if (buffer) {
+          buffer->set_last_usage_token(helper_->InsertToken());
+        }
+      }
+
+      bound_pixel_unpack_transfer_buffer_id_ = buffer_id;
+      buffer = buffer_tracker_->GetBuffer(buffer_id);
+      if (buffer) {
+        buffer->set_used(true);
+        buffer->set_last_usage_token(0);
+      }
+
       break;
     default:
       changed = true;
@@ -2428,7 +2468,7 @@ bool GLES2Implementation::BindBufferHelper(
   }
   // TODO(gman): There's a bug here. If the target is invalid the ID will not be
   // used even though it's marked it as used here.
-  GetIdHandler(id_namespaces::kBuffers)->MarkAsUsedForBind(buffer);
+  GetIdHandler(id_namespaces::kBuffers)->MarkAsUsedForBind(buffer_id);
   return changed;
 }
 
@@ -2562,13 +2602,16 @@ void GLES2Implementation::DeleteBuffersHelper(
       bound_array_buffer_id_ = 0;
     }
     vertex_array_object_manager_->UnbindBuffer(buffers[ii]);
+
     BufferTracker::Buffer* buffer = buffer_tracker_->GetBuffer(buffers[ii]);
     if (buffer) {
-      // Free buffer memory, pending the passage of a token.
-      buffer_tracker_->FreePendingToken(buffer, helper_->InsertToken());
-      // Remove buffer.
-      buffer_tracker_->RemoveBuffer(buffers[ii]);
+      buffer->set_used(false);
+      buffer->set_last_usage_token(helper_->InsertToken());
+
+      FreeTransferBuffer(buffer);
+      buffer_tracker_->RemoveBuffer(buffer->id());
     }
+
     if (buffers[ii] == bound_pixel_unpack_transfer_buffer_id_) {
       bound_pixel_unpack_transfer_buffer_id_ = 0;
     }
@@ -3288,8 +3331,10 @@ void GLES2Implementation::BeginQueryEXT(GLenum target, GLuint id) {
                  << GLES2Util::GetStringQueryTarget(target)
                  << ", " << id << ")");
 
+  QueryKey key = std::make_pair(target, false);
+
   // if any outstanding queries INV_OP
-  QueryMap::iterator it = current_queries_.find(target);
+  QueryMap::iterator it = current_queries_.find(key);
   if (it != current_queries_.end()) {
     SetGLError(
         GL_INVALID_OPERATION, "glBeginQueryEXT", "query already in progress");
@@ -3318,7 +3363,7 @@ void GLES2Implementation::BeginQueryEXT(GLenum target, GLuint id) {
     return;
   }
 
-  current_queries_[target] = query;
+  current_queries_[key] = query;
 
   query->Begin(this);
   CheckGLError();
@@ -3333,7 +3378,8 @@ void GLES2Implementation::EndQueryEXT(GLenum target) {
     return;
   }
 
-  QueryMap::iterator it = current_queries_.find(target);
+  QueryKey key = std::make_pair(target, false);
+  QueryMap::iterator it = current_queries_.find(key);
   if (it == current_queries_.end()) {
     SetGLError(GL_INVALID_OPERATION, "glEndQueryEXT", "no active query");
     return;
@@ -3357,7 +3403,8 @@ void GLES2Implementation::GetQueryivEXT(
     SetGLErrorInvalidEnum("glGetQueryivEXT", pname, "pname");
     return;
   }
-  QueryMap::iterator it = current_queries_.find(target);
+  QueryKey key = std::make_pair(target, false);
+  QueryMap::iterator it = current_queries_.find(key);
   if (it != current_queries_.end()) {
     QueryTracker::Query* query = it->second;
     *params = query->id();
@@ -3381,7 +3428,8 @@ void GLES2Implementation::GetQueryObjectuivEXT(
     return;
   }
 
-  QueryMap::iterator it = current_queries_.find(query->target());
+  QueryKey key = std::make_pair(query->target(), false);
+  QueryMap::iterator it = current_queries_.find(key);
   if (it != current_queries_.end()) {
     SetGLError(
         GL_INVALID_OPERATION,
@@ -3647,6 +3695,40 @@ GLboolean GLES2Implementation::UnmapBufferCHROMIUM(GLuint target) {
   return true;
 }
 
+void GLES2Implementation::BeginInternalQueryAsyncPixelUnpackCompleted(
+    BufferTracker::Buffer *buffer) {
+  QueryKey key = std::make_pair(GL_ASYNC_PIXEL_UNPACK_COMPLETED_CHROMIUM, true);
+
+  DCHECK(current_queries_.find(key) == current_queries_.end());
+
+  GLuint id;
+  GetIdHandler(id_namespaces::kQueries)->MakeIds(this, 0, 1, &id);
+  QueryTracker::Query* query = query_tracker_->CreateInternalQuery(
+      id, GL_ASYNC_PIXEL_UNPACK_COMPLETED_CHROMIUM);
+  if (!query) {
+    MustBeContextLost();
+    return;
+  }
+
+  buffer->set_async_query_id(id);

[piman, 2014/01/11 02:02:32]

What if we do several AsyncTexImage2DCHROMIUM on the same buffer?

[jadahl, 2014/01/11 11:35:29]

That would not work, true. I guess it could work by associating a buffer with a
query serial instead, and if a later AsyncTexImage2DCHROMIUM would occur, it'd
be updated with the new serial.

[piman, 2014/01/16 21:22:50]

We need to fix this, or change the API to prevent multiple
AsyncTexImage2DCHROMIUM on the same buffer (and document that), but I think the
latter is very limiting.

[jadahl, 2014/01/17 08:50:25]

Then should we just introduce a new internal command as discussed, without
having to hack gl queries to deal with this?

+  query->Begin(this);
+  CheckGLError();
+
+  current_queries_[key] = query;
+}
+
+void GLES2Implementation::EndInternalQueryAsyncPixelUnpackCompleted() {
+  QueryKey key = std::make_pair(GL_ASYNC_PIXEL_UNPACK_COMPLETED_CHROMIUM, true);
+
+  DCHECK(current_queries_.find(key) != current_queries_.end());
+
+  QueryMap::iterator it = current_queries_.find(key);
+  QueryTracker::Query* query = it->second;
+  query->End(this);
+  current_queries_.erase(it);
+  CheckGLError();
+}
+
 void GLES2Implementation::AsyncTexImage2DCHROMIUM(
     GLenum target, GLint level, GLint internalformat, GLsizei width,
     GLsizei height, GLint border, GLenum format, GLenum type,
@@ -3691,9 +3773,11 @@ void GLES2Implementation::AsyncTexImage2DCHROMIUM(
       bound_pixel_unpack_transfer_buffer_id_,
       "glAsyncTexImage2DCHROMIUM", offset, size);
   if (buffer && buffer->shm_id() != -1) {
+    BeginInternalQueryAsyncPixelUnpackCompleted(buffer);
     helper_->AsyncTexImage2DCHROMIUM(
         target, level, internalformat, width, height, border, format, type,
         buffer->shm_id(), buffer->shm_offset() + offset);
+    EndInternalQueryAsyncPixelUnpackCompleted();
   }
 }
 
@@ -3735,9 +3819,11 @@ void GLES2Implementation::AsyncTexSubImage2DCHROMIUM(
       bound_pixel_unpack_transfer_buffer_id_,
       "glAsyncTexSubImage2DCHROMIUM", offset, size);
   if (buffer && buffer->shm_id() != -1) {
+    BeginInternalQueryAsyncPixelUnpackCompleted(buffer);
     helper_->AsyncTexSubImage2DCHROMIUM(
         target, level, xoffset, yoffset, width, height, format, type,
         buffer->shm_id(), buffer->shm_offset() + offset);
+    EndInternalQueryAsyncPixelUnpackCompleted();
   }
 }