gpu: Reuse transfer buffers more aggresively

gpu/command_buffer/client/fenced_allocator.cc

Index: gpu/command_buffer/client/fenced_allocator.cc
diff --git a/gpu/command_buffer/client/fenced_allocator.cc b/gpu/command_buffer/client/fenced_allocator.cc
index 0e90bf385b4d0f8a9dded299cfb5a04b1aef2137..ae715320f99abaf9841a012ef5808e301c3931a6 100644
--- a/gpu/command_buffer/client/fenced_allocator.cc
+++ b/gpu/command_buffer/client/fenced_allocator.cc
@@ -42,10 +42,18 @@ FencedAllocator::FencedAllocator(unsigned int size,
 }
 
 FencedAllocator::~FencedAllocator() {
-  // Free blocks pending tokens.
+  // Free blocks pending tokens and serials.
   for (unsigned int i = 0; i < blocks_.size(); ++i) {
-    if (blocks_[i].state == FREE_PENDING_TOKEN) {
-      i = WaitForTokenAndFreeBlock(i);
+    switch (blocks_[i].state) {
+      case FREE_PENDING_TOKEN:
+        i = WaitForTokenAndFreeBlock(i);
+        break;
+      case FREE_PENDING_SERIAL:
+        blocks_[i].state = FREE;
+        CollapseFreeBlock(i);

[reveman, 2014/01/16 17:24:49]

I don't think it makes sense free this immediately but wait for a token in the
above case. If this doesn't need to wait for a serial then it at least has to
wait for a token.

I assume that the WaitForToken code above protects against freeing a block
before all commands that might reference it have been processed on the service
side. Why wouldn't we need the same protection for blocks used with async
transfers?

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

That is actually not needed to ensure things on the service side, since the
commands that refer to the SHM would execute before the IPC to destroy the SHM
gets handled (assuming proper order of operations).

As I mentioned earlier, If the assumption is that the SHM will be destroyed
after this, then there is no synchronization issue. If the SHM will get reused
with a fresh new FencedAllocator, then we need some sort of synchronization,
yes.
I agree that being consistent is better. However to be able to "finish", we need
to dependency-inject a way to call the right WaitAsyncTexImage2DCHROMIUM that
corresponds to the serial.
An alternative is to document that it's the responsibility of the client to
either destroy the SHM, or ensure synchronization before reuse.

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

We could just helper_->Finish() before the loop to make sure all potential
tokens have passed; but that wouldn't synchronize the async commands. To do that
now we'd need to busy-wait, hoping the uploader didn't stop uploading.

If we really don't want to put the responsibility to some layer above, could we
put some kind of waitable here and add another flush-serial command that either
signals the waitable when done, or when aborted?

[reveman, 2014/01/17 16:56:47]

If we're adding an async-token, then some command that will wait for the token
on the service side probably makes sense too. We already have a command that
will wait for an async upload to finish so this shouldn't be hard to implement.

+        break;
+      default:
+        break;
     }
   }
   // These checks are not valid if the service has crashed or lost the context.
@@ -113,6 +121,16 @@ void FencedAllocator::FreePendingToken(
   block.token = token;
 }
 
+void FencedAllocator::FreePendingSerial(
+    FencedAllocator::Offset offset, uint32 serial) {
+  BlockIndex index = GetBlockByOffset(offset);
+  Block &block = blocks_[index];
+  if (block.state == IN_USE)
+    bytes_in_use_ -= block.size;
+  block.state = FREE_PENDING_SERIAL;
+  block.serial = serial;
+}
+
 // Gets the max of the size of the blocks marked as free.
 unsigned int FencedAllocator::GetLargestFreeSize() {
   FreeUnused();
@@ -132,12 +150,16 @@ unsigned int FencedAllocator::GetLargestFreeOrPendingSize() {
   unsigned int current_size = 0;
   for (unsigned int i = 0; i < blocks_.size(); ++i) {
     Block &block = blocks_[i];
-    if (block.state == IN_USE) {
-      max_size = std::max(max_size, current_size);
-      current_size = 0;
-    } else {
-      DCHECK(block.state == FREE || block.state == FREE_PENDING_TOKEN);
-      current_size += block.size;
+    switch (block.state) {
+      case IN_USE:
+      case FREE_PENDING_SERIAL:
+        max_size = std::max(max_size, current_size);
+        current_size = 0;
+        break;
+      default:
+        DCHECK(block.state == FREE || block.state == FREE_PENDING_TOKEN);
+        current_size += block.size;
+        break;
     }
   }
   return std::max(max_size, current_size);
@@ -203,14 +225,28 @@ FencedAllocator::BlockIndex FencedAllocator::WaitForTokenAndFreeBlock(
 
 // Frees any blocks pending a token for which the token has been read.
 void FencedAllocator::FreeUnused() {
-  int32 last_token_read = helper_->last_token_read();
   for (unsigned int i = 0; i < blocks_.size();) {
     Block& block = blocks_[i];
-    if (block.state == FREE_PENDING_TOKEN && block.token <= last_token_read) {
-      block.state = FREE;
-      i = CollapseFreeBlock(i);
-    } else {
-      ++i;
+    switch (block.state) {
+      case FREE_PENDING_TOKEN:
+        if (helper_->HasTokenPassed(block.token)) {
+          block.state = FREE;
+          i = CollapseFreeBlock(i);
+        } else {
+          ++i;
+        }
+        break;
+      case FREE_PENDING_SERIAL:
+        if (helper_->HasSerialPassed(block.serial)) {
+          block.state = FREE;
+          i = CollapseFreeBlock(i);
+        } else {
+          ++i;
+        }
+        break;
+      default:
+        ++i;
+        break;
     }
   }
 }
@@ -228,7 +264,12 @@ FencedAllocator::Offset FencedAllocator::AllocInBlock(BlockIndex index,
     block.state = IN_USE;
     return offset;
   }
-  Block newblock = { FREE, offset + size, block.size - size, kUnusedToken};
+  Block newblock = {
+    FREE, offset + size,
+    block.size - size,
+    kUnusedToken,
+    kUnusedSerial
+  };
   block.state = IN_USE;
   block.size = size;
   // this is the last thing being done because it may invalidate block;
@@ -238,7 +279,13 @@ FencedAllocator::Offset FencedAllocator::AllocInBlock(BlockIndex index,
 
 // The blocks are in offset order, so we can do a binary search.
 FencedAllocator::BlockIndex FencedAllocator::GetBlockByOffset(Offset offset) {
-  Block templ = { IN_USE, offset, 0, kUnusedToken };
+  Block templ = {
+    IN_USE,
+    offset,
+    0,
+    kUnusedToken,
+    kUnusedSerial
+  };
   Container::iterator it = std::lower_bound(blocks_.begin(), blocks_.end(),
                                             templ, OffsetCmp());
   DCHECK(it != blocks_.end() && it->offset == offset);