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 b0efc3769da617acfcc5423f07951c6f8d1dc208..d137473f400fbf894962eab93e0bda8933adac94 100644
--- a/gpu/command_buffer/client/gles2_implementation.cc
+++ b/gpu/command_buffer/client/gles2_implementation.cc
@@ -1379,16 +1379,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());
-
-      // Remove old buffer.
-      buffer_tracker_->RemoveBuffer(buffer_id);
+      if (buffer->used()) {
+        buffer->set_used(false);
+        buffer->set_unused_token(helper_->InsertToken());
+      }
+      DisposeTransferBuffer(buffer);
     }
 
+    CheckBuffersPendingAsyncComplete();
+
     // 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;
@@ -1512,6 +1515,49 @@ void GLES2Implementation::BufferSubData(
   CheckGLError();
 }
 
+void GLES2Implementation::FreeTransferBuffer(BufferTracker::Buffer *buffer) {
+  int32 token = buffer->unused_token();
+  // FIXME: This kind of checks exist elsewhere; is it correct?
+  if (token) {
+    if (helper_->HasTokenPassed(token))

[piman, 2014/01/08 05:08:04]

Maybe this logic (if the token is passed, free immediately) makes sense as low
as possible, maybe FencedAllocator. That avoids some duplication of logic, and
might also hit some other cases.

+      buffer_tracker_->Free(buffer);
+    else
+      buffer_tracker_->FreePendingToken(buffer, token);
+  }
+}
+
+void GLES2Implementation::DisposeTransferBuffer(BufferTracker::Buffer* buffer) {
+  if (buffer->async_query_id()) {
+    QueryTracker::Query* query =
+        query_tracker_->GetQuery(buffer->async_query_id());
+    if (!query->CheckResultsAvailable(helper_)) {
+      // Async upload not completed, queue release.
+      released_buffers_pending_async_complete_.push_back(buffer);
+      buffer_tracker_->UnmanageBuffer(buffer->id());
+      return;
+    }
+  }
+
+  FreeTransferBuffer(buffer);
+  buffer_tracker_->RemoveBuffer(buffer->id());
+}
+
+void GLES2Implementation::CheckBuffersPendingAsyncComplete() {

[epennerAtGoogle, 2014/01/07 00:47:59]

Hmm, it's unfortunate that we have to check all of these every time we create a
buffer.

Uploads are guaranteed to complete in order, but I guess we can't use that
assumption to break out of the loop, since there is no guarantee they are
deleted in order...  I'm not sure how to solve this yet but I'm hesitant of
adding a loop over all pending buffers.

[reveman, 2014/01/07 06:32:06]

you could just keep the list sorted based on query age, which would be the case
if we follow the suggestion I made earlier about enumerating queries and handle
the release of buffers in the mapped memory manager consistent with how we free
based on passed tokens.

[jadahl, 2014/01/07 10:52:03]

The enumeration will improve this situation indeed.

+  BufferList::iterator it = released_buffers_pending_async_complete_.begin();
+  while (it != released_buffers_pending_async_complete_.end()) {
+    BufferTracker::Buffer* buffer = *it;
+    QueryTracker::Query* query =
+        query_tracker_->GetQuery(buffer->async_query_id());
+    if (query->CheckResultsAvailable(helper_)) {
+      it = released_buffers_pending_async_complete_.erase(it);
+      FreeTransferBuffer(buffer);
+      delete buffer;
+    } else {
+      ++it;
+    }
+  }
+}
+
 bool GLES2Implementation::GetBoundPixelTransferBuffer(
     GLenum target,
     const char* function_name,
@@ -2404,24 +2450,42 @@ void GLES2Implementation::GenQueriesEXTHelper(
 // deleted the resource.
 
 bool GLES2Implementation::BindBufferHelper(
-    GLenum target, GLuint buffer) {
+    GLenum target, GLuint buffer_id) {
+  BufferTracker::Buffer* buffer;
+
   // 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_unused_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_unused_token(0);
+      }
+
       break;
     default:
       changed = true;
@@ -2429,7 +2493,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;
 }
 
@@ -2563,13 +2627,14 @@ 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_unused_token(helper_->InsertToken());
+      DisposeTransferBuffer(buffer);
     }
+
     if (buffers[ii] == bound_pixel_unpack_transfer_buffer_id_) {
       bound_pixel_unpack_transfer_buffer_id_ = 0;
     }
@@ -3645,6 +3710,35 @@ GLboolean GLES2Implementation::UnmapBufferCHROMIUM(GLuint target) {
   return true;
 }
 
+void GLES2Implementation::BeginQueryAsyncPixelUnpackCompleted(
+    BufferTracker::Buffer *buffer) {
+  DCHECK(!current_private_unpack_query_);
+
+  GLuint id;
+  GetIdHandler(id_namespaces::kQueries)->MakeIds(this, 0, 1, &id);
+  QueryTracker::Query* query =
+      query_tracker_->CreateQuery(
+          id, GL_ASYNC_PIXEL_UNPACK_COMPLETED_PRIVATE_CHROMIUM);
+  if (!query) {
+    MustBeContextLost();
+    return;
+  }
+
+  buffer->set_async_query_id(id);
+  query->Begin(this);
+  CheckGLError();
+
+  current_private_unpack_query_ = query;
+}
+
+void GLES2Implementation::EndQueryAsyncPixelUnpackCompleted() {
+  DCHECK(current_private_unpack_query_);
+
+  current_private_unpack_query_->End(this);
+  current_private_unpack_query_ = NULL;
+  CheckGLError();
+}
+
 void GLES2Implementation::AsyncTexImage2DCHROMIUM(
     GLenum target, GLint level, GLint internalformat, GLsizei width,
     GLsizei height, GLint border, GLenum format, GLenum type,
@@ -3689,9 +3783,11 @@ void GLES2Implementation::AsyncTexImage2DCHROMIUM(
       bound_pixel_unpack_transfer_buffer_id_,
       "glAsyncTexImage2DCHROMIUM", offset, size);
   if (buffer && buffer->shm_id() != -1) {
+    BeginQueryAsyncPixelUnpackCompleted(buffer);
     helper_->AsyncTexImage2DCHROMIUM(
         target, level, internalformat, width, height, border, format, type,
         buffer->shm_id(), buffer->shm_offset() + offset);
+    EndQueryAsyncPixelUnpackCompleted();
   }
 }
 
@@ -3733,9 +3829,11 @@ void GLES2Implementation::AsyncTexSubImage2DCHROMIUM(
       bound_pixel_unpack_transfer_buffer_id_,
       "glAsyncTexSubImage2DCHROMIUM", offset, size);
   if (buffer && buffer->shm_id() != -1) {
+    BeginQueryAsyncPixelUnpackCompleted(buffer);
     helper_->AsyncTexSubImage2DCHROMIUM(
         target, level, xoffset, yoffset, width, height, format, type,
         buffer->shm_id(), buffer->shm_offset() + offset);
+    EndQueryAsyncPixelUnpackCompleted();
   }
 }