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 2f3194258cb7f92ce51e625cf9472f1e5b61a017..d485c98a24fb359bde4fff54f6b9db0c1d221343 100644
--- a/gpu/command_buffer/client/gles2_implementation.cc
+++ b/gpu/command_buffer/client/gles2_implementation.cc
@@ -15,6 +15,7 @@
 #include <string.h>
 #include <GLES2/gl2ext.h>
 #include <GLES2/gl2extchromium.h>
+#include "base/bind.h"
 #include "gpu/command_buffer/client/buffer_tracker.h"
 #include "gpu/command_buffer/client/gpu_memory_buffer_tracker.h"
 #include "gpu/command_buffer/client/program_info_manager.h"
@@ -107,6 +108,7 @@ GLES2Implementation::GLES2Implementation(
       bound_array_buffer_id_(0),
       bound_pixel_pack_transfer_buffer_id_(0),
       bound_pixel_unpack_transfer_buffer_id_(0),
+      async_upload_sync_(NULL),

[piman, 2014/02/07 22:58:20]

Also need initializers for  async_upload_token_, async_upload_sync_shm_id_,
async_upload_sync_shm_offset_

[jadahl, 2014/02/19 14:06:20]

async_upload_token_ is needed indeed, but shm_id and shm_offset would be
redundant since they are coupled with |async_upload_sync_|. |async_upload_sync_|
is used as the guard for checking the validity/no-oom of async upload sync.

[piman, 2014/02/20 01:52:37]

Per style guide (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Initia...
), please initialize all fields that are not default-initialized (i.e. POD).

       error_bits_(0),
       debug_(false),
       use_count_(0),
@@ -154,7 +156,12 @@ bool GLES2Implementation::Initialize(
     return false;
   }
 
-  mapped_memory_.reset(new MappedMemoryManager(helper_, mapped_memory_limit));
+  mapped_memory_.reset(
+      new MappedMemoryManager(helper_,
+                              base::Bind(&GLES2Implementation::PollAsyncUploads,
+                                         base::Unretained(this),

[epennerAtGoogle, 2014/02/07 20:43:58]

Nit: Maybe overkill in this case, but it never hurts to comment why Unretained
is being used. Eg:
// Unretained since 'this' outlives mapped_memory_, and it's called on this
thread.

+                                         false),
+                              mapped_memory_limit));
 
   unsigned chunk_size = 2 * 1024 * 1024;
   if (mapped_memory_limit != kNoLimit) {
@@ -277,6 +284,14 @@ GLES2Implementation::~GLES2Implementation() {
 #endif
   buffer_tracker_.reset();
 
+  // Wait for all async uploads to finish so we can free their buffers.
+  PollAsyncUploads(true);
+
+  if (async_upload_sync_) {
+    mapped_memory_->Free(static_cast<void*>(async_upload_sync_));

[piman, 2014/02/07 22:58:20]

nit: static_cast isn't needed.

+    async_upload_sync_ = NULL;
+  }
+
   // Make sure the commands make it the service.
   WaitForCmd();
 }
@@ -1378,13 +1393,8 @@ 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)
+      RemoveTransferBuffer(buffer);
 
     // Create new buffer.
     buffer = buffer_tracker_->CreateBuffer(buffer_id, size);
@@ -1512,6 +1522,32 @@ void GLES2Implementation::BufferSubData(
   CheckGLError();
 }
 
+void GLES2Implementation::RemoveTransferBuffer(BufferTracker::Buffer* buffer) {
+  int32 token = buffer->last_usage_token();
+  uint32 async_token = buffer->async_token();
+
+  base::subtle::MemoryBarrier();

[epennerAtGoogle, 2014/02/07 20:43:58]

See nit on other MemoryBarrier. This one also seems extraneous if
RemoveTransferBuffer is called several times in a row. Maybe I'm wrong, but see
my other comment on the other MemoryBarrier for my reasoning.

+
+  if (async_token) {
+    if (HasAsyncUploadTokenPassed(async_token)) {
+      buffer_tracker_->Free(buffer);
+    } else {
+      detached_async_upload_memory_.push_back(
+          std::make_pair(buffer->address(), async_token));
+      buffer_tracker_->Unmanage(buffer);
+    }
+  } else if (token) {
+    if (helper_->HasTokenPassed(token))
+      buffer_tracker_->Free(buffer);
+    else
+      buffer_tracker_->FreePendingToken(buffer, token);
+  } else {
+      buffer_tracker_->Free(buffer);
+  }
+
+  buffer_tracker_->RemoveBuffer(buffer->id());
+}
+
 bool GLES2Implementation::GetBoundPixelTransferBuffer(
     GLenum target,
     const char* function_name,
@@ -1587,7 +1623,7 @@ void GLES2Implementation::CompressedTexImage2D(
       helper_->CompressedTexImage2D(
           target, level, internalformat, width, height, border, image_size,
           buffer->shm_id(), buffer->shm_offset() + offset);
-      buffer->set_transfer_ready_token(helper_->InsertToken());
+      buffer->set_last_usage_token(helper_->InsertToken());
     }
     return;
   }
@@ -1628,7 +1664,7 @@ void GLES2Implementation::CompressedTexSubImage2D(
       helper_->CompressedTexSubImage2D(
           target, level, xoffset, yoffset, width, height, format, image_size,
           buffer->shm_id(), buffer->shm_offset() + offset);
-      buffer->set_transfer_ready_token(helper_->InsertToken());
+      buffer->set_last_usage_token(helper_->InsertToken());
       CheckGLError();
     }
     return;
@@ -1715,7 +1751,7 @@ void GLES2Implementation::TexImage2D(
       helper_->TexImage2D(
           target, level, internalformat, width, height, border, format, type,
           buffer->shm_id(), buffer->shm_offset() + offset);
-      buffer->set_transfer_ready_token(helper_->InsertToken());
+      buffer->set_last_usage_token(helper_->InsertToken());
       CheckGLError();
     }
     return;
@@ -1821,7 +1857,7 @@ void GLES2Implementation::TexSubImage2D(
       helper_->TexSubImage2D(
           target, level, xoffset, yoffset, width, height, format, type,
           buffer->shm_id(), buffer->shm_offset() + offset, false);
-      buffer->set_transfer_ready_token(helper_->InsertToken());
+      buffer->set_last_usage_token(helper_->InsertToken());
       CheckGLError();
     }
     return;
@@ -2404,24 +2440,24 @@ void GLES2Implementation::GenQueriesEXTHelper(
 // deleted the resource.
 
 bool GLES2Implementation::BindBufferHelper(
-    GLenum target, GLuint buffer) {
+    GLenum target, GLuint buffer_id) {
   // 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;
+      bound_pixel_unpack_transfer_buffer_id_ = buffer_id;
       break;
     default:
       changed = true;
@@ -2429,7 +2465,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 +2599,11 @@ 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]);
-    }
+    if (buffer)
+      RemoveTransferBuffer(buffer);
+
     if (buffers[ii] == bound_pixel_unpack_transfer_buffer_id_) {
       bound_pixel_unpack_transfer_buffer_id_ = 0;
     }
@@ -3591,9 +3625,9 @@ void* GLES2Implementation::MapBufferCHROMIUM(GLuint target, GLenum access) {
   // with this method of synchronization. Until this is fixed,
   // MapBufferCHROMIUM will not block even if the transfer is not ready
   // for these calls.
-  if (buffer->transfer_ready_token()) {
-    helper_->WaitForToken(buffer->transfer_ready_token());
-    buffer->set_transfer_ready_token(0);
+  if (buffer->last_usage_token()) {
+    helper_->WaitForToken(buffer->last_usage_token());
+    buffer->set_last_usage_token(0);
   }
   buffer->set_mapped(true);
 
@@ -3627,6 +3661,54 @@ GLboolean GLES2Implementation::UnmapBufferCHROMIUM(GLuint target) {
   return true;
 }
 
+bool GLES2Implementation::EnsureAsyncUploadSync() {
+  if (async_upload_sync_)
+    return true;
+
+  int32 shm_id;
+  unsigned int shm_offset;
+  void* mem = mapped_memory_->Alloc(sizeof(AsyncUploadSync),
+                                    &shm_id,
+                                    &shm_offset);
+  if (!mem)
+    return false;
+
+  async_upload_sync_shm_id_ = shm_id;
+  async_upload_sync_shm_offset_ = shm_offset;
+  async_upload_sync_ = static_cast<AsyncUploadSync*>(mem);
+  async_upload_sync_->Reset();
+
+  return true;
+}
+
+uint32 GLES2Implementation::NextAsyncUploadToken() {
+  async_upload_token_++;
+  if (async_upload_token_ == 0)
+    async_upload_token_++;
+  return async_upload_token_;
+}
+
+void GLES2Implementation::PollAsyncUploads(bool wait) {
+  if (!async_upload_sync_)
+    return;
+
+  do {
+    base::subtle::MemoryBarrier();

[epennerAtGoogle, 2014/02/07 20:43:58]

It wouldn't hurt to document each of these base::subtle calls, and why exactly
it is needed. It seems mostly clear now, but for posterity it's always nice to
know.

Two things on this one:

First, do we really need a MemoryBarrier here? Don't we just need the latest
token, and could that be done without a full MemoryBarrier?

Second, do we need to have a memory barrier in a loop here? The chances of the
token changing while we are in this loop seems low and not worth caring about.
If we did this only once would the token just have a slight chance of being out
of date? Alternatively could we get the token once and use it for testing all
uploads?

[piman, 2014/02/07 22:58:20]

TBH, I'd rather make the access to the AsyncUploadSync be explicit about the
memory semantics (i.e. use Acquire_Load to read and Release_Store to write).
Basically encapsulating the atomicity as low as possible (in AsyncUploadSync),
so that 'subtle' doesn't leak everywhere. Like we did for e.g. gpu::SharedState 

[jadahl, 2014/02/08 09:18:25]

To be honest, I added the barrier here mostly because it was used in a similar
way by the query manager. But I don't think it is really necessary here, as I
don't see how we could ever write out-of-order tokens as whoever writes next
will do so from the same message loop at some later point in time.

I guess we could just go with very explicit inc-store/load here and in the
client. But when would this really matter? Atomic increase is used to protect
writing from multiple threads, but when would it matter if we read without
synchronization? The only risk is we read one token before the most recent, and
that would be extremely rare and harmless.

[piman, 2014/02/11 00:09:09]

Some useful background:
http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could...
The compiler/cpu only guarantees serialization as seen from one thread. But what
can appear as serial in one thread may be seen out-of-order (or worse,
"impossible" states) from another thread.

Just throwing MemoryBarriers around isn't so useful because:
- it's hard to convince oneself of whether it's necessary and/or sufficient.
- they don't convey the semantics.
In this case, I'm pretty sure that this one MemoryBarrier is neither necessary
nor sufficient.


Aquire_Load and Release_Store convey the proper semantics. The AsyncUploadSync
is your semaphore that guards the shared state which is the transfer buffer.
On the GPU side, you need to make sure that all memory accesses to the transfer
buffer are not reordered to after the write to the AsyncUploadSync is visible to
other threads. That's the semantic of Release_Store.
On the client side, you need to make sure all memory accesses to the transfer
buffer are not reordered to before the read of the AsyncUploadSync. That's the
semantic of Acquire_Load.

The good news is that on x86, you can elide the memory barrier in those 2 cases
- see the implementation of Acquire_Load/Release_Store (they just need a
compiler barrier to make sure the compiler itself doesn't reorder). That's
properly abstracted by the per-platform implementations of
Acquire_Load/Release_Store

Since the behavior is very sublte (hence the namespace), the use of atomic
operations should be invisible/irrelevant to the higher-level code, and instead
should be abstracted as low as possible.
Best, I think, would be to have it in AsyncUploadSync, encapsulating the
semantic that we care about, something like:
void SignalCompletion(uint32_t token) {
  Release_Store(&async_toke, token);
}

bool IsCompleted(uint32_t token) {
  uint32_t current_token = Acquire_Load(&async_token);
  return (current_token - token < 0x80000000);
}


After that, all the memory logic is irrelevant in the rest of the code. We can
test the hell out of AsyncUploadSync for "strange" races, and make sure the rest
of the code will be correct.

+    DetachedAsyncUploadMemoryList::iterator it =
+        detached_async_upload_memory_.begin();
+    while (it != detached_async_upload_memory_.end()) {
+      if (HasAsyncUploadTokenPassed(it->second)) {

[piman, 2014/02/07 22:58:20]

Similar to what I mentioned in FencedAllocator, we can free this if the context
is lost too (it either has consumed the buffer already, or it will never do so).

+        mapped_memory_->Free(it->first);
+        it = detached_async_upload_memory_.erase(it);
+      } else {
+        break;
+      }
+    }
+    if (wait)
+      WaitForCmd();

[piman, 2014/02/07 22:58:20]

We need more than this, otherwise we'll spin loop (possibly preventing the GPU
process from making progress). WaitForCmd only ensures all the commands have
been consumed by the service side, but doesn't ensure completion of the async
uploads.
As is, nothing ensures that on the next loop the GPU process will have made
progress (in truth, WaitForCmd becomes a noop after the first one).
I think we want the semantics of WaitAsyncTexImage2DCHROMIUM, but applied to all
async uploads. Maybe a new WaitAllAsyncTexImage2DCHROMIUM?

Also, we need this to exit the loop if the context is lost and the service side
doesn't make progress any more (WaitForCmd is a noop).

+  } while (!detached_async_upload_memory_.empty() && wait);
+}
+
 void GLES2Implementation::AsyncTexImage2DCHROMIUM(
     GLenum target, GLint level, GLint internalformat, GLsizei width,
     GLsizei height, GLint border, GLenum format, GLenum type,
@@ -3658,7 +3740,12 @@ void GLES2Implementation::AsyncTexImage2DCHROMIUM(
   if (!pixels && !bound_pixel_unpack_transfer_buffer_id_) {
     helper_->AsyncTexImage2DCHROMIUM(
        target, level, internalformat, width, height, border, format, type,
-       0, 0);
+       0, 0, 0, 0, 0);
+    return;
+  }
+
+  if (!EnsureAsyncUploadSync()) {
+    SetGLError(GL_OUT_OF_MEMORY, "glTexImage2D", "out of memory");
     return;
   }
 
@@ -3671,9 +3758,13 @@ void GLES2Implementation::AsyncTexImage2DCHROMIUM(
       bound_pixel_unpack_transfer_buffer_id_,
       "glAsyncTexImage2DCHROMIUM", offset, size);
   if (buffer && buffer->shm_id() != -1) {
+    uint32 async_token = NextAsyncUploadToken();
+    buffer->set_async_token(async_token);
     helper_->AsyncTexImage2DCHROMIUM(
         target, level, internalformat, width, height, border, format, type,
-        buffer->shm_id(), buffer->shm_offset() + offset);
+        buffer->shm_id(), buffer->shm_offset() + offset,
+        async_token,
+        async_upload_sync_shm_id_, async_upload_sync_shm_offset_);
   }
 }
 
@@ -3706,6 +3797,11 @@ void GLES2Implementation::AsyncTexSubImage2DCHROMIUM(
     return;
   }
 
+  if (!EnsureAsyncUploadSync()) {
+    SetGLError(GL_OUT_OF_MEMORY, "glTexImage2D", "out of memory");
+    return;
+  }
+
   // Async uploads require a transfer buffer to be bound.
   // TODO(hubbe): Make MapBufferCHROMIUM block if someone tries to re-use
   // the buffer before the transfer is finished. (Currently such
@@ -3715,9 +3811,13 @@ void GLES2Implementation::AsyncTexSubImage2DCHROMIUM(
       bound_pixel_unpack_transfer_buffer_id_,
       "glAsyncTexSubImage2DCHROMIUM", offset, size);
   if (buffer && buffer->shm_id() != -1) {
+    uint32 async_token = NextAsyncUploadToken();
+    buffer->set_async_token(async_token);
     helper_->AsyncTexSubImage2DCHROMIUM(
         target, level, xoffset, yoffset, width, height, format, type,
-        buffer->shm_id(), buffer->shm_offset() + offset);
+        buffer->shm_id(), buffer->shm_offset() + offset,
+        async_token,
+        async_upload_sync_shm_id_, async_upload_sync_shm_offset_);
   }
 }