Make ClientResourceProvider::ReceiveReturnsFromParent() be O(N+MlogM)

Currently this methods works as O(N*M), where
N = size of imported_resources_ in the provider
M = number of resources being received by the provider

That is because each received element is removed individually from the
imported_resources_ set, and each removal will cost O(N).

Instead, sort the incoming resources, then walk through the two
sets of resources in parallel, which is O(N+M). During this walk,
replace resources to be removed from imported_resources_ with resources
that we are keeping as we walk, which is O(1). Then erase the empty space
created by this process at the end of the loop, which is O(N). This
makes the complexity O(2N+M) = O(N+M).

This is similar to the std::remove_if() algorithm, except that we have
to write it ourselves to avoid O(M) work at each step, and instead walk
the received resources in parallel.

We also sort the returned M resources as a prelude, to ensure we can do
the walk in parallel. This costs O(MlogM), making the whole thing
O(N+MlogM).

Similarly, for ClientResourceProvider::ReleaseAllExportedResources()
stop removing elements individually. In this case we can use
base::EraseIf to iterate through the set, act on each element, and
return true if it should be erased.

R=piman@chromium.org

Bug: 881295
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I94522eb07e2f09b20a10d8f254faa63fd2ab9d0e
Reviewed-on: https://chromium-review.googlesource.com/1211908
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590347}

components/viz/client/client_resource_provider.cc

@@ -133,34 +133,133 @@ void ClientResourceProvider::PrepareSendToParent(
 }
 
 void ClientResourceProvider::ReceiveReturnsFromParent(
-    const std::vector<ReturnedResource>& resources) {
+    std::vector<ReturnedResource> resources) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 
-  for (const ReturnedResource& returned : resources) {
-    ResourceId local_id = returned.id;
+  // |imported_resources_| is a set sorted by id, so if we sort the incoming
+  // |resources| list by id also, we can walk through both structures in order,
+  // replacing  things to be removed from imported_resources_ with things that
+  // we want to keep, making the removal of all items O(N+MlogM) instead of
+  // O(N*M). This algorithm is modelled after std::remove_if() but with a
+  // parallel walk through |resources| instead of an O(logM) lookup into
+  // |resources| for each step.
+  std::sort(resources.begin(), resources.end(),
+            [](const ReturnedResource& a, const ReturnedResource& b) {
+              return a.id < b.id;
+            });
+
+  auto returned_it = resources.begin();
+  auto returned_end = resources.end();
+  auto imported_keep_end_it = imported_resources_.begin();
+  auto imported_compare_it = imported_resources_.begin();
+  auto imported_end = imported_resources_.end();
+
+  std::vector<base::OnceClosure> release_callbacks;
+  release_callbacks.reserve(resources.size());
+
+  while (imported_compare_it != imported_end) {
+    if (returned_it == returned_end) {
+      // Nothing else to remove from |imported_resources_|.
+      if (imported_keep_end_it == imported_compare_it) {
+        // We didn't remove anything, so we're done.
+        break;
+      }
+      // If something was removed, we need to shift everything into the empty
+      // space that was made.
+      *imported_keep_end_it = std::move(*imported_compare_it);
+      ++imported_keep_end_it;
+      ++imported_compare_it;
+      continue;
+    }
+
+    const ResourceId returned_resource_id = returned_it->id;
+    const ResourceId imported_resource_id = imported_compare_it->first;
+
+    if (returned_resource_id != imported_resource_id) {
+      // They're both sorted, and everything being returned should already
+      // be in the imported list. So we should be able to walk through the
+      // resources list in order, and find each id in both sets when present.
+      // That means if it's not matching, we should be above it in the sorted
+      // order of the |imported_resources_|, allowing us to get to it by
+      // continuing to traverse |imported_resources_|.
+      DCHECK_GT(returned_resource_id, imported_resource_id);
+      // This means we want to keep the resource at |imported_compare_it|. So go
+      // to the next. If we removed anything previously and made empty space,
+      // fill it as we move.
+      if (imported_keep_end_it != imported_compare_it)
+        *imported_keep_end_it = std::move(*imported_compare_it);
+      ++imported_keep_end_it;
+      ++imported_compare_it;
+      continue;
+    }
 
-    auto import_it = imported_resources_.find(local_id);
-    DCHECK(import_it != imported_resources_.end());
-    ImportedResource& imported = import_it->second;
+    const ReturnedResource& returned = *returned_it;
+    ImportedResource& imported = imported_compare_it->second;
 
     DCHECK_GE(imported.exported_count, returned.count);
     imported.exported_count -= returned.count;
     imported.returned_lost |= returned.lost;
 
-    if (imported.exported_count)
+    if (imported.exported_count) {
+      // Can't remove the imported yet so go to the next, looking for the next
+      // returned resource.
+      ++returned_it;
+      // If we removed anything previously and made empty space, fill it as we
+      // move.
+      if (imported_keep_end_it != imported_compare_it)
+        *imported_keep_end_it = std::move(*imported_compare_it);
+      ++imported_keep_end_it;
+      ++imported_compare_it;
       continue;
+    }
 
+    // Save the sync token only when the exported count is going to 0. Or IOW
+    // drop all by the last returned sync token.
     if (returned.sync_token.HasData()) {
       DCHECK(!imported.resource.is_software);
       imported.returned_sync_token = returned.sync_token;
     }
 
-    if (imported.marked_for_deletion) {
-      imported.release_callback->Run(imported.returned_sync_token,
-                                     imported.returned_lost);
-      imported_resources_.erase(import_it);
+    if (!imported.marked_for_deletion) {
+      // Not going to remove the imported yet so go to the next, looking for the
+      // next returned resource.
+      ++returned_it;
+      // If we removed anything previously and made empty space, fill it as we
+      // move.
+      if (imported_keep_end_it != imported_compare_it)
+        *imported_keep_end_it = std::move(*imported_compare_it);
+      ++imported_keep_end_it;
+      ++imported_compare_it;
+      continue;
     }
+
+    // Save the ReleaseCallback to run after iterating through internal data
+    // structures, in case it calls back into this class.
+    auto run_callback = [](std::unique_ptr<SingleReleaseCallback> cb,
+                           const gpu::SyncToken& sync_token, bool lost) {
+      cb->Run(sync_token, lost);
+      // |cb| is destroyed when leaving scope.
+    };
+    release_callbacks.push_back(
+        base::BindOnce(run_callback, base::Passed(&imported.release_callback),
+                       imported.returned_sync_token, imported.returned_lost));
+    // We don't want to keep this resource, so we leave |imported_keep_end_it|
+    // pointing to it (since it points past the end of what we're keeping). We
+    // do move |imported_compare_it| in order to move on to the next resource.
+    ++imported_compare_it;
+    // The imported iterator is pointing at the next element already, so no need
+    // to increment it, and we can move on to looking for the next returned
+    // resource.
+    ++returned_it;
   }
+
+  // Drop anything that was moved after the |imported_end| iterator in a single
+  // O(N) operation.
+  if (imported_keep_end_it != imported_compare_it)
+    imported_resources_.erase(imported_keep_end_it, imported_resources_.end());
+
+  for (auto& cb : release_callbacks)
+    std::move(cb).Run();
 }
 
 ResourceId ClientResourceProvider::ImportResource(
@@ -188,21 +287,30 @@ void ClientResourceProvider::RemoveImportedResource(ResourceId id) {
 }
 
 void ClientResourceProvider::ReleaseAllExportedResources(bool lose) {
-  std::vector<ResourceId> to_remove;
-  for (auto& pair : imported_resources_) {
-    ImportedResource& imported = pair.second;
-    if (!imported.exported_count)
-      continue;
-    imported.exported_count = 0;
-    imported.returned_lost |= lose;
-    if (imported.marked_for_deletion) {
-      imported.release_callback->Run(imported.returned_sync_token,
-                                     imported.returned_lost);
-      to_remove.push_back(pair.first);
-    }
-  }
-  for (ResourceId id : to_remove)
-    imported_resources_.erase(id);
+  auto release_and_remove =
+      [lose](std::pair<ResourceId, ImportedResource>& pair) {
+        ImportedResource& imported = pair.second;
+        if (!imported.exported_count) {
+          // Not exported, not up for consideration to be returned here.
+          return false;
+        }
+        imported.exported_count = 0;
+        imported.returned_lost |= lose;
+        if (!imported.marked_for_deletion) {
+          // Was exported, but not removed by the client, so don't return it
+          // yet.
+          return false;
+        }
+
+        imported.release_callback->Run(imported.returned_sync_token,
+                                       imported.returned_lost);
+        // Was exported and removed by the client, so return it now.
+        return true;
+      };
+
+  // This will run |release_and_remove| on each element of |imported_resources_|
+  // and drop any resources from the set that it requests.
+  base::EraseIf(imported_resources_, release_and_remove);
 }
 
 void ClientResourceProvider::ShutdownAndReleaseAllResources() {

components/viz/client/client_resource_provider.h

@@ -13,6 +13,7 @@
 #include "components/viz/common/resources/release_callback.h"
 #include "components/viz/common/resources/resource_id.h"
 #include "components/viz/common/resources/resource_settings.h"
+#include "components/viz/common/resources/returned_resource.h"
 #include "components/viz/common/resources/single_release_callback.h"
 #include "components/viz/common/resources/transferable_resource.h"
 #include "third_party/khronos/GLES2/gl2.h"
@@ -64,7 +65,7 @@ class VIZ_CLIENT_EXPORT ClientResourceProvider {
   // NOTE: if the sync_token is set on any TransferableResource, this will
   // wait on it.
   void ReceiveReturnsFromParent(
-      const std::vector<ReturnedResource>& transferable_resources);
+      std::vector<ReturnedResource> transferable_resources);
 
   // Receives a resource from an external client that can be used in compositor
   // frames, via the returned ResourceId.

components/viz/client/client_resource_provider_unittest.cc

@@ -16,6 +16,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::_;
+using testing::Each;
 
 namespace viz {
 namespace {
@@ -86,6 +87,8 @@ INSTANTIATE_TEST_CASE_P(ClientResourceProviderTests,
 class MockReleaseCallback {
  public:
   MOCK_METHOD2(Released, void(const gpu::SyncToken& token, bool lost));
+  MOCK_METHOD3(ReleasedWithId,
+               void(ResourceId id, const gpu::SyncToken& token, bool lost));
 };
 
 TEST_P(ClientResourceProviderTest, TransferableResourceReleased) {
@@ -238,6 +241,65 @@ TEST_P(ClientResourceProviderTest,
   DestroyProvider();
 }
 
+TEST_P(ClientResourceProviderTest, TransferableResourceSendToParentManyUnsent) {
+  MockReleaseCallback release;
+
+  // 5 resources to have 2 before and 2 after the one being sent and returned,
+  // to verify the data structures are treated correctly when removing from the
+  // middle. 1 after is not enough as that one will replace the resource being
+  // removed and misses some edge cases. We want another resource after that in
+  // the structure to verify it is also not treated incorrectly.
+  struct Data {
+    TransferableResource tran;
+    ResourceId id;
+  } data[5];
+  for (int i = 0; i < 5; ++i) {
+    data[i].tran = MakeTransferableResource(use_gpu(), 'a', 15);
+    data[i].id = provider().ImportResource(
+        data[i].tran,
+        SingleReleaseCallback::Create(base::BindOnce(
+            &MockReleaseCallback::Released, base::Unretained(&release))));
+  }
+  std::sort(std::begin(data), std::end(data),
+            [](const Data& a, const Data& b) { return a.id < b.id; });
+
+  // Export the resource.
+  std::vector<ResourceId> to_send = {data[2].id};
+  std::vector<TransferableResource> exported;
+  provider().PrepareSendToParent(to_send, &exported, context_provider());
+  ASSERT_EQ(exported.size(), 1u);
+
+  // Exported resource matches except for the id which was mapped
+  // to the local ResourceProvider, and the sync token should be
+  // verified if it's a gpu resource.
+  gpu::SyncToken verified_sync_token = data[2].tran.mailbox_holder.sync_token;
+  if (!data[2].tran.is_software)
+    verified_sync_token.SetVerifyFlush();
+
+  // Exported resources are not released when removed, until the export returns.
+  EXPECT_CALL(release, Released(_, _)).Times(0);
+  provider().RemoveImportedResource(data[2].id);
+
+  // Return the resource, with a sync token if using gpu.
+  std::vector<ReturnedResource> returned;
+  returned.push_back({});
+  returned.back().id = exported[0].id;
+  if (use_gpu())
+    returned.back().sync_token = SyncTokenFromUInt(31);
+  returned.back().count = 1;
+  returned.back().lost = false;
+
+  // The sync token is given to the ReleaseCallback.
+  EXPECT_CALL(release, Released(returned[0].sync_token, false));
+  provider().ReceiveReturnsFromParent(returned);
+
+  EXPECT_CALL(release, Released(_, false)).Times(4);
+  provider().RemoveImportedResource(data[0].id);
+  provider().RemoveImportedResource(data[1].id);
+  provider().RemoveImportedResource(data[3].id);
+  provider().RemoveImportedResource(data[4].id);
+}
+
 TEST_P(ClientResourceProviderTest, TransferableResourceRemovedAfterReturn) {
   MockReleaseCallback release;
   TransferableResource tran = MakeTransferableResource(use_gpu(), 'a', 15);
@@ -554,7 +616,7 @@ TEST_P(ClientResourceProviderTest, ReleaseExportedResourcesThenRemove) {
   provider().PrepareSendToParent({resource}, &list, context_provider());
   EXPECT_EQ(1u, list.size());
 
-  // Drop any exported resources. Yhey are now considered lost for gpu
+  // Drop any exported resources. They are now considered lost for gpu
   // compositing, since gpu resources are modified (in their metadata) while
   // being used by the parent.
   provider().ReleaseAllExportedResources(use_gpu());
@@ -567,5 +629,101 @@ TEST_P(ClientResourceProviderTest, ReleaseExportedResourcesThenRemove) {
   EXPECT_CALL(release, Released(_, _)).Times(0);
 }
 
+TEST_P(ClientResourceProviderTest, ReleaseMultipleResources) {
+  MockReleaseCallback release;
+
+  // Make 5 resources, put them in a non-sorted order.
+  ResourceId resources[5];
+  for (int i = 0; i < 5; ++i) {
+    TransferableResource tran = MakeTransferableResource(use_gpu(), 'a', 1 + i);
+    resources[i] = provider().ImportResource(
+        tran, SingleReleaseCallback::Create(
+                  base::BindOnce(&MockReleaseCallback::ReleasedWithId,
+                                 base::Unretained(&release), i)));
+  }
+
+  // Transfer some resources to the parent, but not in the sorted order.
+  std::vector<TransferableResource> list;
+  provider().PrepareSendToParent({resources[2], resources[0], resources[4]},
+                                 &list, context_provider());
+  EXPECT_EQ(3u, list.size());
+
+  // Receive them back. Since these are not in the same order they were
+  // inserted originally, we verify the ClientResourceProvider can handle
+  // resources being returned (in a group) that are not at the front/back
+  // of its internal storage. IOW This shouldn't crash or corrupt state.
+  std::vector<ReturnedResource> returned_to_child;
+  returned_to_child.push_back(list[0].ToReturnedResource());
+  returned_to_child.push_back(list[1].ToReturnedResource());
+  returned_to_child.push_back(list[2].ToReturnedResource());
+  provider().ReceiveReturnsFromParent(returned_to_child);
+
+  // Remove them from the ClientResourceProvider, they should be returned as
+  // they're no longer exported.
+  EXPECT_CALL(release, ReleasedWithId(0, _, false));
+  provider().RemoveImportedResource(resources[0]);
+  EXPECT_CALL(release, ReleasedWithId(2, _, false));
+  provider().RemoveImportedResource(resources[2]);
+  EXPECT_CALL(release, ReleasedWithId(4, _, false));
+  provider().RemoveImportedResource(resources[4]);
+  testing::Mock::VerifyAndClearExpectations(&release);
+
+  // These were never exported.
+  EXPECT_CALL(release, ReleasedWithId(1, _, false));
+  EXPECT_CALL(release, ReleasedWithId(3, _, false));
+  provider().RemoveImportedResource(resources[1]);
+  provider().RemoveImportedResource(resources[3]);
+
+  EXPECT_CALL(release, Released(_, false)).Times(0);
+}
+
+TEST_P(ClientResourceProviderTest, ReleaseMultipleResourcesBeforeReturn) {
+  MockReleaseCallback release;
+
+  // Make 5 resources, put them in a non-sorted order.
+  ResourceId resources[5];
+  for (int i = 0; i < 5; ++i) {
+    TransferableResource tran = MakeTransferableResource(use_gpu(), 'a', 1 + i);
+    resources[i] = provider().ImportResource(
+        tran, SingleReleaseCallback::Create(
+                  base::BindOnce(&MockReleaseCallback::ReleasedWithId,
+                                 base::Unretained(&release), i)));
+  }
+
+  // Transfer some resources to the parent, but not in the sorted order.
+  std::vector<TransferableResource> list;
+  provider().PrepareSendToParent({resources[2], resources[0], resources[4]},
+                                 &list, context_provider());
+  EXPECT_EQ(3u, list.size());
+
+  // Remove the exported resources from the ClientResourceProvider, they should
+  // not yet be returned, since they are exported.
+  provider().RemoveImportedResource(resources[0]);
+  provider().RemoveImportedResource(resources[2]);
+  provider().RemoveImportedResource(resources[4]);
+
+  // Receive them back now. Since these are not in the same order they were
+  // inserted originally, we verify the ClientResourceProvider can handle
+  // resources being returned (in a group) that are not at the front/back
+  // of its internal storage. IOW This shouldn't crash or corrupt state.
+  std::vector<ReturnedResource> returned_to_child;
+  returned_to_child.push_back(list[0].ToReturnedResource());
+  returned_to_child.push_back(list[1].ToReturnedResource());
+  returned_to_child.push_back(list[2].ToReturnedResource());
+  // The resources are returned immediately since they were previously removed.
+  EXPECT_CALL(release, ReleasedWithId(0, _, false));
+  EXPECT_CALL(release, ReleasedWithId(2, _, false));
+  EXPECT_CALL(release, ReleasedWithId(4, _, false));
+  provider().ReceiveReturnsFromParent(returned_to_child);
+
+  // These were never exported.
+  EXPECT_CALL(release, ReleasedWithId(1, _, false));
+  EXPECT_CALL(release, ReleasedWithId(3, _, false));
+  provider().RemoveImportedResource(resources[1]);
+  provider().RemoveImportedResource(resources[3]);
+
+  EXPECT_CALL(release, Released(_, false)).Times(0);
+}
+
 }  // namespace
 }  // namespace viz