Check for deleted discardable handles before allocation

Currently, we never check for deleted entries until the client tries
to lock an entry. In some cases (transfer cache), the client may never
delete or re-lock an entry after creating it, relying on the service
side to purge it. In these cases the client should eventually pick up
these deletions allowing handles to be re-used.

To handle this, we check for deleted entries any time we are about to
allocate a new buffer.

R=khushalsagar

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: Ib4edbf7e339f213cff7a7df85771ecfeb5113936
Reviewed-on: https://chromium-review.googlesource.com/1147495
Commit-Queue: Eric Karl <ericrk@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577364}

gpu/command_buffer/client/client_discardable_manager.cc

@@ -195,17 +195,20 @@ bool ClientDiscardableManager::FindAllocation(CommandBuffer* command_buffer,
                                               uint32_t* offset) {
   CheckPending(command_buffer);
 
-  for (auto& allocation : allocations_) {
-    if (!allocation->free_offsets.HasFreeOffset())
-      continue;
-
-    *offset = allocation->free_offsets.TakeFreeOffset();
-    *shm_id = allocation->shm_id;
-    *buffer = allocation->buffer;
+  if (FindExistingAllocation(command_buffer, buffer, shm_id, offset))
     return true;
+
+  // We couldn't find an existing free entry and are about to allocate more
+  // space. Check whether any handles have been deleted on the service side.
+  if (CheckDeleted(command_buffer)) {
+    // We deleted at least one entry, try to find an allocaiton. If the entry
+    // we deleted was the last one in an allocation, it's possbile that we
+    // *still* won't have allocaitons, so this isn't guaranteed to succeed.
+    if (FindExistingAllocation(command_buffer, buffer, shm_id, offset))
+      return true;
   }
 
-  // We couldn't find an existing free entry. Allocate more space.
+  // Allocate more space.
   auto allocation = std::make_unique<Allocation>(elements_per_allocation_);
   allocation->buffer = command_buffer->CreateTransferBuffer(
       allocation_size_, &allocation->shm_id);
@@ -219,6 +222,24 @@ bool ClientDiscardableManager::FindAllocation(CommandBuffer* command_buffer,
   return true;
 }
 
+bool ClientDiscardableManager::FindExistingAllocation(
+    CommandBuffer* command_buffer,
+    scoped_refptr<Buffer>* buffer,
+    int32_t* shm_id,
+    uint32_t* offset) {
+  for (auto& allocation : allocations_) {
+    if (!allocation->free_offsets.HasFreeOffset())
+      continue;
+
+    *offset = allocation->free_offsets.TakeFreeOffset();
+    *shm_id = allocation->shm_id;
+    *buffer = allocation->buffer;
+    return true;
+  }
+
+  return false;
+}
+
 void ClientDiscardableManager::ReturnAllocation(
     CommandBuffer* command_buffer,
     const ClientDiscardableHandle& handle) {
@@ -246,4 +267,18 @@ void ClientDiscardableManager::CheckPending(CommandBuffer* command_buffer) {
   }
 }
 
+bool ClientDiscardableManager::CheckDeleted(CommandBuffer* command_buffer) {
+  bool freed_entry = false;
+  for (auto it = handles_.begin(); it != handles_.end();) {
+    if (it->second.CanBeReUsed()) {
+      ReturnAllocation(command_buffer, it->second);
+      it = handles_.erase(it);
+      freed_entry = true;
+    } else {
+      ++it;
+    }
+  }
+  return freed_entry;
+}
+
 }  // namespace gpu

gpu/command_buffer/client/client_discardable_manager.h

@@ -45,9 +45,16 @@ class GPU_EXPORT ClientDiscardableManager {
                       scoped_refptr<Buffer>* buffer,
                       int32_t* shm_id,
                       uint32_t* offset);
+  bool FindExistingAllocation(CommandBuffer* command_buffer,
+                              scoped_refptr<Buffer>* buffer,
+                              int32_t* shm_id,
+                              uint32_t* offset);
   void ReturnAllocation(CommandBuffer* command_buffer,
                         const ClientDiscardableHandle& handle);
   void CheckPending(CommandBuffer* command_buffer);
+  // Return true if we found at least one deleted entry.
+  bool CheckDeleted(CommandBuffer* command_buffer);
+  bool CreateNewAllocation(CommandBuffer* command_buffer);
 
  private:
   size_t allocation_size_;

gpu/command_buffer/client/client_discardable_manager_unittest.cc

@@ -33,7 +33,6 @@ class FakeCommandBuffer : public CommandBuffer {
   void SetGetBuffer(int32_t transfer_buffer_id) override { NOTREACHED(); }
   scoped_refptr<gpu::Buffer> CreateTransferBuffer(size_t size,
                                                   int32_t* id) override {
-    EXPECT_GE(size, 2048u);
     *id = next_id_++;
     active_ids_.insert(*id);
     base::UnsafeSharedMemoryRegion shmem_region =
@@ -104,11 +103,12 @@ TEST(ClientDiscardableManagerTest, Reuse) {
     ClientDiscardableHandle handle = manager.GetHandle(handle_id);
     EXPECT_TRUE(handle.IsLockedForTesting());
     EXPECT_EQ(handle.shm_id(), 1);
-    UnlockAndDeleteClientHandleForTesting(handle);
+    UnlockClientHandleForTesting(handle);
     handle_ids.push_back(handle_id);
   }
   // Delete every other entry.
   for (auto it = handle_ids.begin(); it != handle_ids.end();) {
+    DeleteClientHandleForTesting(manager.GetHandle(*it));
     manager.FreeHandle(*it);
     it = handle_ids.erase(it);
     ++it;
@@ -120,11 +120,12 @@ TEST(ClientDiscardableManagerTest, Reuse) {
     ClientDiscardableHandle handle = manager.GetHandle(handle_id);
     EXPECT_TRUE(handle.IsLockedForTesting());
     EXPECT_EQ(handle.shm_id(), 1);
-    UnlockAndDeleteClientHandleForTesting(handle);
+    UnlockClientHandleForTesting(handle);
     handle_ids.push_back(handle_id);
   }
   // Delete all outstanding allocations
   for (const auto& handle_id : handle_ids) {
+    DeleteClientHandleForTesting(manager.GetHandle(handle_id));
     manager.FreeHandle(handle_id);
   }
   manager.CheckPendingForTesting(&command_buffer);
@@ -141,7 +142,7 @@ TEST(ClientDiscardableManagerTest, MultipleAllocations) {
     ClientDiscardableHandle handle = manager.GetHandle(handle_id);
     EXPECT_TRUE(handle.IsLockedForTesting());
     EXPECT_EQ(handle.shm_id(), 1);
-    UnlockAndDeleteClientHandleForTesting(handle);
+    UnlockClientHandleForTesting(handle);
     handle_ids.push_back(handle_id);
   }
   // Allocate and free one entry multiple times, this should cause the
@@ -157,8 +158,54 @@ TEST(ClientDiscardableManagerTest, MultipleAllocations) {
   }
   // Delete all outstanding allocations
   for (const auto& handle_id : handle_ids) {
+    DeleteClientHandleForTesting(manager.GetHandle(handle_id));
+    manager.FreeHandle(handle_id);
+  }
+  manager.CheckPendingForTesting(&command_buffer);
+}
+
+TEST(ClientDiscardableManagerTest, FreeDeleted) {
+  FakeCommandBuffer command_buffer;
+  ClientDiscardableManager manager;
+  manager.SetElementCountForTesting(4);
+  // Fill our allocation with unlocked handles.
+  std::vector<ClientDiscardableHandle::Id> handle_ids;
+  for (int i = 0; i < 4; ++i) {
+    ClientDiscardableHandle::Id handle_id =
+        manager.CreateHandle(&command_buffer);
+    ClientDiscardableHandle handle = manager.GetHandle(handle_id);
+    EXPECT_TRUE(handle.IsLockedForTesting());
+    EXPECT_EQ(handle.shm_id(), 1);
+    UnlockClientHandleForTesting(handle);
+    handle_ids.push_back(handle_id);
+  }
+  // Allocate and free a new entry. It should get a new allocation.
+  {
+    ClientDiscardableHandle::Id handle_id =
+        manager.CreateHandle(&command_buffer);
+    ClientDiscardableHandle handle = manager.GetHandle(handle_id);
+    EXPECT_TRUE(handle.IsLockedForTesting());
+    EXPECT_EQ(handle.shm_id(), 2);
+    UnlockAndDeleteClientHandleForTesting(handle);
     manager.FreeHandle(handle_id);
   }
+  // Delete (but don't free) one of the above entries.
+  DeleteClientHandleForTesting(manager.GetHandle(handle_ids[0]));
+  // Allocate and free a new entry, it should re-use the first allocation.
+  {
+    ClientDiscardableHandle::Id handle_id =
+        manager.CreateHandle(&command_buffer);
+    ClientDiscardableHandle handle = manager.GetHandle(handle_id);
+    EXPECT_TRUE(handle.IsLockedForTesting());
+    EXPECT_EQ(handle.shm_id(), 1);
+    UnlockAndDeleteClientHandleForTesting(handle);
+    manager.FreeHandle(handle_id);
+  }
+  // Delete and free the remaining handles.
+  for (int i = 1; i < 4; ++i) {
+    DeleteClientHandleForTesting(manager.GetHandle(handle_ids[i]));
+    manager.FreeHandle(handle_ids[i]);
+  }
   manager.CheckPendingForTesting(&command_buffer);
 }