Revert 172805

> Made the registration and deletion of transfer buffers async.
> 
> Added an IPC to request multiple transfer buffer IDs up front.
> 
> Every renderer command buffer proxy gets some IPCs immediately after initialization.
> 
> Removed request_id arg from CreateTransferBuffer. It wasn't being used.
> 
> Review URL: https://chromiumcodereview.appspot.com/11308296

TBR=apatrick@chromium.org
Review URL: https://codereview.chromium.org/11578020

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@173018 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 158 insertions, 156 deletions

diff --git a/gpu/command_buffer/service/command_buffer_service.cc b/gpu/command_buffer/service/command_buffer_service.cc
index 391f335..8b00595 100644
--- a/gpu/command_buffer/service/command_buffer_service.cc
+++ b/gpu/command_buffer/service/command_buffer_service.cc
@@ -116,26 +116,20 @@ void CommandBufferService::SetGetOffset(int32 get_offset) {
   get_offset_ = get_offset;
 }
 
-Buffer CommandBufferService::CreateTransferBuffer(size_t size,
-                                                  int32* id) {
-  *id = -1;
-
-  SharedMemory buffer;
-  if (!buffer.CreateAnonymous(size))
-    return Buffer();
-
-  static int32 next_id = 1;
-  *id = next_id++;
-
-  if (!RegisterTransferBuffer(*id, &buffer, size))
-    return Buffer();
+int32 CommandBufferService::CreateTransferBuffer(size_t size,
+                                                 int32 id_request) {
+  return transfer_buffer_manager_->CreateTransferBuffer(size, id_request);
+}
 
-  return GetTransferBuffer(*id);
+int32 CommandBufferService::RegisterTransferBuffer(
+    base::SharedMemory* shared_memory, size_t size, int32 id_request) {
+  return transfer_buffer_manager_->RegisterTransferBuffer(
+      shared_memory, size, id_request);
 }
 
-void CommandBufferService::DestroyTransferBuffer(int32 id) {
-  transfer_buffer_manager_->DestroyTransferBuffer(id);
-  if (id == ring_buffer_id_) {
+void CommandBufferService::DestroyTransferBuffer(int32 handle) {
+  transfer_buffer_manager_->DestroyTransferBuffer(handle);
+  if (handle == ring_buffer_id_) {
     ring_buffer_id_ = -1;
     ring_buffer_ = Buffer();
     num_entries_ = 0;
@@ -144,17 +138,8 @@ void CommandBufferService::DestroyTransferBuffer(int32 id) {
   }
 }
 
-Buffer CommandBufferService::GetTransferBuffer(int32 id) {
-  return transfer_buffer_manager_->GetTransferBuffer(id);
-}
-
-bool CommandBufferService::RegisterTransferBuffer(
-    int32 id,
-    base::SharedMemory* shared_memory,
-    size_t size) {
-  return transfer_buffer_manager_->RegisterTransferBuffer(id,
-                                                          shared_memory,
-                                                          size);
+Buffer CommandBufferService::GetTransferBuffer(int32 handle) {
+  return transfer_buffer_manager_->GetTransferBuffer(handle);
 }
 
 void CommandBufferService::SetToken(int32 token) {
diff --git a/gpu/command_buffer/service/command_buffer_service.h b/gpu/command_buffer/service/command_buffer_service.h
index c53a4bc..c3bb841 100644
--- a/gpu/command_buffer/service/command_buffer_service.h
+++ b/gpu/command_buffer/service/command_buffer_service.h
@@ -31,9 +31,12 @@ class GPU_EXPORT CommandBufferService : public CommandBuffer {
   virtual State FlushSync(int32 put_offset, int32 last_known_get) OVERRIDE;
   virtual void SetGetBuffer(int32 transfer_buffer_id) OVERRIDE;
   virtual void SetGetOffset(int32 get_offset) OVERRIDE;
-  virtual Buffer CreateTransferBuffer(size_t size, int32* id) OVERRIDE;
+  virtual int32 CreateTransferBuffer(size_t size, int32 id_request) OVERRIDE;
+  virtual int32 RegisterTransferBuffer(base::SharedMemory* shared_memory,
+                                       size_t size,
+                                       int32 id_request) OVERRIDE;
   virtual void DestroyTransferBuffer(int32 id) OVERRIDE;
-  virtual Buffer GetTransferBuffer(int32 id) OVERRIDE;
+  virtual Buffer GetTransferBuffer(int32 handle) OVERRIDE;
   virtual void SetToken(int32 token) OVERRIDE;
   virtual void SetParseError(error::Error error) OVERRIDE;
   virtual void SetContextLostReason(error::ContextLostReason) OVERRIDE;
@@ -58,13 +61,6 @@ class GPU_EXPORT CommandBufferService : public CommandBuffer {
   // Copy the current state into the shared state transfer buffer.
   void UpdateState();
 
-  // Register an existing shared memory object and get an ID that can be used
-  // to identify it in the command buffer. Callee dups the handle until
-  // DestroyTransferBuffer is called.
-  bool RegisterTransferBuffer(int32 id,
-                              base::SharedMemory* shared_memory,
-                              size_t size);
-
  private:
   int32 ring_buffer_id_;
   Buffer ring_buffer_;
diff --git a/gpu/command_buffer/service/command_buffer_service_unittest.cc b/gpu/command_buffer/service/command_buffer_service_unittest.cc
index bf4d619..2c9093b 100644
--- a/gpu/command_buffer/service/command_buffer_service_unittest.cc
+++ b/gpu/command_buffer/service/command_buffer_service_unittest.cc
@@ -50,8 +50,7 @@ class CommandBufferServiceTest : public testing::Test {
   }
 
   bool Initialize(size_t size) {
-    int32 id;
-    command_buffer_->CreateTransferBuffer(size, &id);
+    int32 id = command_buffer_->CreateTransferBuffer(size, -1);
     EXPECT_GT(id, 0);
     command_buffer_->SetGetBuffer(id);
     return true;
@@ -118,8 +117,7 @@ TEST_F(CommandBufferServiceTest, CanSyncGetAndPutOffset) {
 }
 
 TEST_F(CommandBufferServiceTest, SetGetBuffer) {
-  int32 ring_buffer_id;
-  command_buffer_->CreateTransferBuffer(1024, &ring_buffer_id);
+  int32 ring_buffer_id = command_buffer_->CreateTransferBuffer(1024, -1);
   EXPECT_GT(ring_buffer_id, 0);
 
   scoped_ptr<StrictMock<MockCallbackTest> > change_callback(
diff --git a/gpu/command_buffer/service/transfer_buffer_manager.cc b/gpu/command_buffer/service/transfer_buffer_manager.cc
index 12c9fb1..7bde12a 100644
--- a/gpu/command_buffer/service/transfer_buffer_manager.cc
+++ b/gpu/command_buffer/service/transfer_buffer_manager.cc
@@ -8,7 +8,6 @@
 
 #include "base/process_util.h"
 #include "base/debug/trace_event.h"
-#include "gpu/command_buffer/common/gles2_cmd_utils.h"
 
 using ::base::SharedMemory;
 
@@ -19,15 +18,17 @@ TransferBufferManagerInterface::~TransferBufferManagerInterface() {
 
 TransferBufferManager::TransferBufferManager()
     : shared_memory_bytes_allocated_(0) {
+  // Element zero is always NULL.
+  registered_objects_.push_back(Buffer());
 }
 
 TransferBufferManager::~TransferBufferManager() {
-  while (!registered_buffers_.empty()) {
-    BufferMap::iterator it = registered_buffers_.begin();
-    DCHECK(shared_memory_bytes_allocated_ >= it->second.size);
-    shared_memory_bytes_allocated_ -= it->second.size;
-    delete it->second.shared_memory;
-    registered_buffers_.erase(it);
+  for (size_t i = 0; i < registered_objects_.size(); ++i) {
+    if (registered_objects_[i].shared_memory) {
+      DCHECK(shared_memory_bytes_allocated_ >= registered_objects_[i].size);
+      shared_memory_bytes_allocated_ -= registered_objects_[i].size;
+      delete registered_objects_[i].shared_memory;
+    }
   }
   DCHECK(!shared_memory_bytes_allocated_);
 }
@@ -36,38 +37,42 @@ bool TransferBufferManager::Initialize() {
   return true;
 }
 
-bool TransferBufferManager::RegisterTransferBuffer(
-    int32 id,
-    base::SharedMemory* shared_memory,
-    size_t size) {
-  if (id <= 0) {
-    DVLOG(ERROR) << "Cannot register transfer buffer with non-positive ID.";
-    return false;
-  }
+int32 TransferBufferManager::CreateTransferBuffer(
+    size_t size, int32 id_request) {
+  SharedMemory buffer;
+  if (!buffer.CreateAnonymous(size))
+    return -1;
+
+  return RegisterTransferBuffer(&buffer, size, id_request);
+}
 
-  // Fail if the ID is in use.
-  if (registered_buffers_.find(id) != registered_buffers_.end()) {
-    DVLOG(ERROR) << "Buffer ID already in use.";
-    return false;
+int32 TransferBufferManager::RegisterTransferBuffer(
+    base::SharedMemory* shared_memory, size_t size, int32 id_request) {
+  // Check we haven't exceeded the range that fits in a 32-bit integer.
+  if (unused_registered_object_elements_.empty()) {
+    if (registered_objects_.size() > std::numeric_limits<uint32>::max())
+      return -1;
   }
 
+  // Check that the requested ID is sane (not too large, or less than -1)
+  if (id_request != -1 && (id_request > 100 || id_request < -1))
+    return -1;
+
   // Duplicate the handle.
   base::SharedMemoryHandle duped_shared_memory_handle;
   if (!shared_memory->ShareToProcess(base::GetCurrentProcessHandle(),
                                      &duped_shared_memory_handle)) {
-    DVLOG(ERROR) << "Failed to duplicate shared memory handle.";
-    return false;
+    return -1;
   }
   scoped_ptr<SharedMemory> duped_shared_memory(
       new SharedMemory(duped_shared_memory_handle, false));
 
   // Map the shared memory into this process. This validates the size.
-  if (!duped_shared_memory->Map(size)) {
-    DVLOG(ERROR) << "Failed to map shared memory.";
-    return false;
-  }
+  if (!duped_shared_memory->Map(size))
+    return -1;
 
-  // If it could be mapped register the shared memory with the ID.
+  // If it could be mapped, allocate an ID and register the shared memory with
+  // that ID.
   Buffer buffer;
   buffer.ptr = duped_shared_memory->memory();
   buffer.size = size;
@@ -77,36 +82,73 @@ bool TransferBufferManager::RegisterTransferBuffer(
   TRACE_COUNTER_ID1(
       "gpu", "GpuTransferBufferMemory", this, shared_memory_bytes_allocated_);
 
-  registered_buffers_[id] = buffer;
+  // If caller requested specific id, first try to use id_request.
+  if (id_request != -1) {
+    int32 cur_size = static_cast<int32>(registered_objects_.size());
+    if (cur_size <= id_request) {
+      // Pad registered_objects_ to reach id_request.
+      registered_objects_.resize(static_cast<size_t>(id_request + 1));
+      for (int32 id = cur_size; id < id_request; ++id)
+        unused_registered_object_elements_.insert(id);
+      registered_objects_[id_request] = buffer;
+      return id_request;
+    } else if (!registered_objects_[id_request].shared_memory) {
+      // id_request is already in free list.
+      registered_objects_[id_request] = buffer;
+      unused_registered_object_elements_.erase(id_request);
+      return id_request;
+    }
+  }
 
-  return true;
+  if (unused_registered_object_elements_.empty()) {
+    int32 handle = static_cast<int32>(registered_objects_.size());
+    registered_objects_.push_back(buffer);
+    return handle;
+  } else {
+    int32 handle = *unused_registered_object_elements_.begin();
+    unused_registered_object_elements_.erase(
+        unused_registered_object_elements_.begin());
+    DCHECK(!registered_objects_[handle].shared_memory);
+    registered_objects_[handle] = buffer;
+    return handle;
+  }
 }
 
-void TransferBufferManager::DestroyTransferBuffer(int32 id) {
-  BufferMap::iterator it = registered_buffers_.find(id);
-  if (it == registered_buffers_.end()) {
-    DVLOG(ERROR) << "Transfer buffer ID was not registered.";
+void TransferBufferManager::DestroyTransferBuffer(int32 handle) {
+  if (handle <= 0)
     return;
-  }
 
-  DCHECK(shared_memory_bytes_allocated_ >= it->second.size);
-  shared_memory_bytes_allocated_ -= it->second.size;
+  if (static_cast<size_t>(handle) >= registered_objects_.size())
+    return;
+
+  DCHECK(shared_memory_bytes_allocated_ >= registered_objects_[handle].size);
+  shared_memory_bytes_allocated_ -= registered_objects_[handle].size;
   TRACE_COUNTER_ID1(
       "CommandBuffer", "SharedMemory", this, shared_memory_bytes_allocated_);
 
-  delete it->second.shared_memory;
-  registered_buffers_.erase(it);
+  delete registered_objects_[handle].shared_memory;
+  registered_objects_[handle] = Buffer();
+  unused_registered_object_elements_.insert(handle);
+
+  // Remove all null objects from the end of the vector. This allows the vector
+  // to shrink when, for example, all objects are unregistered. Note that this
+  // loop never removes element zero, which is always NULL.
+  while (registered_objects_.size() > 1 &&
+      !registered_objects_.back().shared_memory) {
+    registered_objects_.pop_back();
+    unused_registered_object_elements_.erase(
+        static_cast<int32>(registered_objects_.size()));
+  }
 }
 
-Buffer TransferBufferManager::GetTransferBuffer(int32 id) {
-  if (id == 0)
+Buffer TransferBufferManager::GetTransferBuffer(int32 handle) {
+  if (handle < 0)
     return Buffer();
 
-  BufferMap::iterator it = registered_buffers_.find(id);
-  if (it == registered_buffers_.end())
+  if (static_cast<size_t>(handle) >= registered_objects_.size())
     return Buffer();
 
-  return it->second;
+  return registered_objects_[handle];
 }
 
 }  // namespace gpu
diff --git a/gpu/command_buffer/service/transfer_buffer_manager.h b/gpu/command_buffer/service/transfer_buffer_manager.h
index b50a5ae..9f24e2c 100644
--- a/gpu/command_buffer/service/transfer_buffer_manager.h
+++ b/gpu/command_buffer/service/transfer_buffer_manager.h
@@ -10,7 +10,6 @@
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
-#include "base/hash_tables.h"
 #include "base/shared_memory.h"
 #include "gpu/command_buffer/common/command_buffer_shared.h"
 
@@ -20,11 +19,13 @@ class GPU_EXPORT TransferBufferManagerInterface {
  public:
   virtual ~TransferBufferManagerInterface();
 
-  virtual bool RegisterTransferBuffer(int32 id,
-                                      base::SharedMemory* shared_memory,
-                                      size_t size) = 0;
+  virtual int32 CreateTransferBuffer(size_t size, int32 id_request) = 0;
+  virtual int32 RegisterTransferBuffer(
+      base::SharedMemory* shared_memory,
+      size_t size,
+      int32 id_request) = 0;
   virtual void DestroyTransferBuffer(int32 id) = 0;
-  virtual Buffer GetTransferBuffer(int32 id) = 0;
+  virtual Buffer GetTransferBuffer(int32 handle) = 0;
 
 };
 
@@ -34,17 +35,19 @@ class GPU_EXPORT TransferBufferManager
   TransferBufferManager();
 
   bool Initialize();
-  virtual bool RegisterTransferBuffer(int32 id,
-                                      base::SharedMemory* shared_memory,
-                                      size_t size) OVERRIDE;
+  virtual int32 CreateTransferBuffer(size_t size, int32 id_request) OVERRIDE;
+  virtual int32 RegisterTransferBuffer(
+      base::SharedMemory* shared_memory,
+      size_t size,
+      int32 id_request) OVERRIDE;
   virtual void DestroyTransferBuffer(int32 id) OVERRIDE;
-  virtual Buffer GetTransferBuffer(int32 id) OVERRIDE;
+  virtual Buffer GetTransferBuffer(int32 handle) OVERRIDE;
 
  private:
   virtual ~TransferBufferManager();
 
-  typedef base::hash_map<int32, Buffer> BufferMap;
-  BufferMap registered_buffers_;
+  std::set<int32> unused_registered_object_elements_;
+  std::vector<Buffer> registered_objects_;
   size_t shared_memory_bytes_allocated_;
 
   DISALLOW_COPY_AND_ASSIGN(TransferBufferManager);
diff --git a/gpu/command_buffer/service/transfer_buffer_manager_unittest.cc b/gpu/command_buffer/service/transfer_buffer_manager_unittest.cc
index 68c5c2e..029cb0e 100644
--- a/gpu/command_buffer/service/transfer_buffer_manager_unittest.cc
+++ b/gpu/command_buffer/service/transfer_buffer_manager_unittest.cc
@@ -12,22 +12,14 @@ using base::SharedMemory;
 
 namespace gpu {
 
-const static size_t kBufferSize = 1024;
-
 class TransferBufferManagerTest : public testing::Test {
  protected:
   virtual void SetUp() {
-    for (size_t i = 0; i < arraysize(buffers_); ++i) {
-      buffers_[i].CreateAnonymous(kBufferSize);
-      buffers_[i].Map(kBufferSize);
-    }
-
     TransferBufferManager* manager = new TransferBufferManager();
     transfer_buffer_manager_.reset(manager);
-    ASSERT_TRUE(manager->Initialize());
+    manager->Initialize();
   }
 
-  base::SharedMemory buffers_[3];
   scoped_ptr<TransferBufferManagerInterface> transfer_buffer_manager_;
 };
 
@@ -43,73 +35,59 @@ TEST_F(TransferBufferManagerTest, OutOfRangeHandleMapsToNull) {
   EXPECT_TRUE(NULL == transfer_buffer_manager_->GetTransferBuffer(1).ptr);
 }
 
-TEST_F(TransferBufferManagerTest, CanRegisterTransferBuffer) {
-  EXPECT_TRUE(transfer_buffer_manager_->RegisterTransferBuffer(1,
-                                                               &buffers_[0],
-                                                               kBufferSize));
-  Buffer registered = transfer_buffer_manager_->GetTransferBuffer(1);
-
-  // Distinct memory range and shared memory handle from that originally
-  // registered.
-  EXPECT_NE(static_cast<void*>(NULL), registered.ptr);
-  EXPECT_NE(buffers_[0].memory(), registered.ptr);
-  EXPECT_EQ(kBufferSize, registered.size);
-  EXPECT_NE(&buffers_[0], registered.shared_memory);
-
-  // But maps to the same physical memory.
-  *static_cast<int*>(registered.ptr) = 7;
-  *static_cast<int*>(buffers_[0].memory()) = 8;
-  EXPECT_EQ(8, *static_cast<int*>(registered.ptr));
+TEST_F(TransferBufferManagerTest, CanCreateTransferBuffers) {
+  int32 handle = transfer_buffer_manager_->CreateTransferBuffer(1024, -1);
+  EXPECT_EQ(1, handle);
+  Buffer buffer = transfer_buffer_manager_->GetTransferBuffer(handle);
+  ASSERT_TRUE(NULL != buffer.ptr);
+  EXPECT_EQ(1024u, buffer.size);
 }
 
-TEST_F(TransferBufferManagerTest, CanDestroyTransferBuffer) {
-  EXPECT_TRUE(transfer_buffer_manager_->RegisterTransferBuffer(1,
-                                                               &buffers_[0],
-                                                               kBufferSize));
-  transfer_buffer_manager_->DestroyTransferBuffer(1);
-  Buffer registered = transfer_buffer_manager_->GetTransferBuffer(1);
+TEST_F(TransferBufferManagerTest, CreateTransferBufferReturnsDistinctHandles) {
+  EXPECT_EQ(1, transfer_buffer_manager_->CreateTransferBuffer(1024, -1));
+}
 
-  EXPECT_EQ(static_cast<void*>(NULL), registered.ptr);
-  EXPECT_EQ(0U, registered.size);
-  EXPECT_EQ(static_cast<base::SharedMemory*>(NULL), registered.shared_memory);
+TEST_F(TransferBufferManagerTest,
+    CreateTransferBufferReusesUnregisteredHandles) {
+  EXPECT_EQ(1, transfer_buffer_manager_->CreateTransferBuffer(1024, -1));
+  EXPECT_EQ(2, transfer_buffer_manager_->CreateTransferBuffer(1024, -1));
+  transfer_buffer_manager_->DestroyTransferBuffer(1);
+  EXPECT_EQ(1, transfer_buffer_manager_->CreateTransferBuffer(1024, -1));
+  EXPECT_EQ(3, transfer_buffer_manager_->CreateTransferBuffer(1024, -1));
 }
 
-TEST_F(TransferBufferManagerTest, CannotRegregisterTransferBufferId) {
-  EXPECT_TRUE(transfer_buffer_manager_->RegisterTransferBuffer(1,
-                                                               &buffers_[0],
-                                                               kBufferSize));
-  EXPECT_FALSE(transfer_buffer_manager_->RegisterTransferBuffer(1,
-                                                                &buffers_[0],
-                                                                kBufferSize));
-  EXPECT_FALSE(transfer_buffer_manager_->RegisterTransferBuffer(1,
-                                                                &buffers_[1],
-                                                                kBufferSize));
+TEST_F(TransferBufferManagerTest, CannotUnregisterHandleZero) {
+  transfer_buffer_manager_->DestroyTransferBuffer(0);
+  EXPECT_TRUE(NULL == transfer_buffer_manager_->GetTransferBuffer(0).ptr);
+  EXPECT_EQ(1, transfer_buffer_manager_->CreateTransferBuffer(1024, -1));
 }
 
-TEST_F(TransferBufferManagerTest, CanReuseTransferBufferIdAfterDestroying) {
-  EXPECT_TRUE(transfer_buffer_manager_->RegisterTransferBuffer(1,
-                                                               &buffers_[0],
-                                                               kBufferSize));
-  transfer_buffer_manager_->DestroyTransferBuffer(1);
-  EXPECT_TRUE(transfer_buffer_manager_->RegisterTransferBuffer(1,
-                                                               &buffers_[1],
-                                                               kBufferSize));
+TEST_F(TransferBufferManagerTest, CannotUnregisterNegativeHandles) {
+  transfer_buffer_manager_->DestroyTransferBuffer(-1);
+  EXPECT_EQ(1, transfer_buffer_manager_->CreateTransferBuffer(1024, -1));
 }
 
-TEST_F(TransferBufferManagerTest, DestroyUnusedTransferBufferIdDoesNotCrash) {
+TEST_F(TransferBufferManagerTest, CannotUnregisterUnregisteredHandles) {
   transfer_buffer_manager_->DestroyTransferBuffer(1);
+  EXPECT_EQ(1, transfer_buffer_manager_->CreateTransferBuffer(1024, -1));
 }
 
-TEST_F(TransferBufferManagerTest, CannotRegisterNullTransferBuffer) {
-  EXPECT_FALSE(transfer_buffer_manager_->RegisterTransferBuffer(0,
-                                                                &buffers_[0],
-                                                                kBufferSize));
+// Testing this case specifically because there is an optimization that takes
+// a different code path in this case.
+TEST_F(TransferBufferManagerTest, UnregistersLastRegisteredHandle) {
+  EXPECT_EQ(1, transfer_buffer_manager_->CreateTransferBuffer(1024, -1));
+  transfer_buffer_manager_->DestroyTransferBuffer(1);
+  EXPECT_EQ(1, transfer_buffer_manager_->CreateTransferBuffer(1024, -1));
 }
 
-TEST_F(TransferBufferManagerTest, CannotRegisterNegativeTransferBufferId) {
-  EXPECT_FALSE(transfer_buffer_manager_->RegisterTransferBuffer(-1,
-                                                                &buffers_[0],
-                                                                kBufferSize));
+// Testing this case specifically because there is an optimization that takes
+// a different code path in this case.
+TEST_F(TransferBufferManagerTest, UnregistersTwoLastRegisteredHandles) {
+  EXPECT_EQ(1, transfer_buffer_manager_->CreateTransferBuffer(1024, -1));
+  EXPECT_EQ(2, transfer_buffer_manager_->CreateTransferBuffer(1024, -1));
+  transfer_buffer_manager_->DestroyTransferBuffer(2);
+  transfer_buffer_manager_->DestroyTransferBuffer(1);
+  EXPECT_EQ(1, transfer_buffer_manager_->CreateTransferBuffer(1024, -1));
 }
 
 }  // namespace gpu