gpu: Reuse transfer buffers more aggresively

gpu/command_buffer/client/fenced_allocator.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // This file contains the implementation of the FencedAllocator class.
 
 #include "gpu/command_buffer/client/fenced_allocator.h"
 
 #include <algorithm>
 
@@ skipping 16 matching lines @@
   return (size + (kAllocAlignment - 1)) & ~(kAllocAlignment - 1);
 }
 
 }  // namespace
 
 #ifndef _MSC_VER
 const FencedAllocator::Offset FencedAllocator::kInvalidOffset;
 #endif
 
 FencedAllocator::FencedAllocator(unsigned int size,
-                                 CommandBufferHelper *helper)
+                                 CommandBufferHelper *helper,
+                                 const base::Closure& poll_callback)
     : helper_(helper),
+      poll_callback_(poll_callback),
       bytes_in_use_(0) {
   Block block = { FREE, 0, RoundDown(size), kUnusedToken };
   blocks_.push_back(block);
 }
 
 FencedAllocator::~FencedAllocator() {
   // Free blocks pending tokens.
   for (unsigned int i = 0; i < blocks_.size(); ++i) {
     if (blocks_[i].state == FREE_PENDING_TOKEN) {
       i = WaitForTokenAndFreeBlock(i);
     }
   }
   // These checks are not valid if the service has crashed or lost the context.
   // DCHECK_EQ(blocks_.size(), 1u);
   // DCHECK_EQ(blocks_[0].state, FREE);

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

I would like to reinstate those checks, for the async case - to ensure we
guaranteed we finished all async uploads before destroying the MemoryManager.

For the lost context case, once we know the context is lost, we know that either
the service already used the buffer that's in flight, or it won't use it in the
future. At that point it is safe to free/reuse (we could check that condition in
FreeUnused).

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

Should maybe the ~GLES2Implementation() take care of free:ing all of the memory
it manages without waiting in the lost context case?

Regarding the blocks pending token, sounds a bit unrelated to this patch.
Wouldn't it be better doing that separately, for bisectability? I could upload
another patch once this has landed that reinstates this check.

[jadahl, 2014/02/14 15:32:21]

I added lost-context handling (free's all non-free blocks (both in use and
pending token)) to the FreeUnused() and free:ing all unmanaged memory in the
Poll callback in gles2_implementation.cc. Commenting out these two lines leads
to tons of asserts when running the unit tests, since they assume its ok to
leak.

I suggest leaving these two commented out, fixing context-lost handling as well
as making the tests pass in a separate patch.

 }
 
 // Looks for a non-allocated block that is big enough. Search in the FREE
 // blocks first (for direct usage), first-fit, then in the FREE_PENDING_TOKEN
 // blocks, waiting for them. The current implementation isn't smart about
 // optimizing what to wait for, just looks inside the block in order (first-fit
 // as well).
 FencedAllocator::Offset FencedAllocator::Alloc(unsigned int size) {
   // size of 0 is not allowed because it would be inconsistent to only sometimes
   // have it succeed. Example: Alloc(SizeOfBuffer), Alloc(0).
@@ skipping 132 matching lines @@
     BlockIndex index) {
   Block &block = blocks_[index];
   DCHECK_EQ(block.state, FREE_PENDING_TOKEN);
   helper_->WaitForToken(block.token);
   block.state = FREE;
   return CollapseFreeBlock(index);
 }
 
 // Frees any blocks pending a token for which the token has been read.
 void FencedAllocator::FreeUnused() {
+  // Free any potential blocks that has its lifetime handled outside.
+  poll_callback_.Run();
+
   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) {

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

While you're here... it looks like the 'block.token <= last_token_read' check
doesn't take wraparound into account. Could you make it use your new
HasTokenPassed ?

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

Sure.

       block.state = FREE;
       i = CollapseFreeBlock(i);
     } else {
       ++i;
     }
   }
 }
 
 // If the block is exactly the requested size, simply mark it IN_USE, otherwise
 // split it and mark the first one (of the requested size) IN_USE.
@@ skipping 19 matching lines @@
 // 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 };
   Container::iterator it = std::lower_bound(blocks_.begin(), blocks_.end(),
                                             templ, OffsetCmp());
   DCHECK(it != blocks_.end() && it->offset == offset);
   return it-blocks_.begin();
 }
 
 }  // namespace gpu