Skip to content

GitLab

T
tangled
Commit 2f8f447e authored by danakj's avatar danakj Committed by Commit Bot
Browse files
Options

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

This reverts commit 7c4f701b.

Reason for revert: This is asserting on bots, something is wrong.

Original change's description:
> 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}

TBR=danakj@chromium.org,piman@chromium.org

Change-Id: I37c9b08ad6cb2d18aeee224f4c282e22612bb1d4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 881295
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1219996Reviewed-by: default avatardanakj <danakj@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590442}
parent e874b3cd
Show whitespace changes
Inline Side-by-side
Showing with 28 additions and 295 deletions
components/viz/client/client_resource_provider.cc
View file @ 2f8f447e
......@@ -133,133 +133,34 @@ void ClientResourceProvider::PrepareSendToParent(
}
void ClientResourceProvider::ReceiveReturnsFromParent(
std::vector<ReturnedResource> resources) {
const std::vector<ReturnedResource>& resources) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
// |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;
}
for (const ReturnedResource& returned : resources) {
ResourceId local_id = returned.id;
const ReturnedResource& returned = *returned_it;
ImportedResource& imported = imported_compare_it->second;
auto import_it = imported_resources_.find(local_id);
DCHECK(import_it != imported_resources_.end());
ImportedResource& imported = import_it->second;
DCHECK_GE(imported.exported_count, returned.count);
imported.exported_count -= returned.count;
imported.returned_lost |= returned.lost;
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;
if (imported.exported_count)
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) {
// 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;
if (imported.marked_for_deletion) {
imported.release_callback->Run(imported.returned_sync_token,
imported.returned_lost);
imported_resources_.erase(import_it);
}
// 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(
......@@ -287,30 +188,21 @@ void ClientResourceProvider::RemoveImportedResource(ResourceId id) {
}
void ClientResourceProvider::ReleaseAllExportedResources(bool lose) {
auto release_and_remove =
[lose](std::pair<ResourceId, ImportedResource>& pair) {
std::vector<ResourceId> to_remove;
for (auto& pair : imported_resources_) {
ImportedResource& imported = pair.second;
if (!imported.exported_count) {
// Not exported, not up for consideration to be returned here.
return false;
}
if (!imported.exported_count)
continue;
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;
}
if (imported.marked_for_deletion) {
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);
to_remove.push_back(pair.first);
}
}
for (ResourceId id : to_remove)
imported_resources_.erase(id);
}
void ClientResourceProvider::ShutdownAndReleaseAllResources() {
......
components/viz/client/client_resource_provider.h
View file @ 2f8f447e
......@@ -13,7 +13,6 @@
#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"
......@@ -65,7 +64,7 @@ class VIZ_CLIENT_EXPORT ClientResourceProvider {
// NOTE: if the sync_token is set on any TransferableResource, this will
// wait on it.
void ReceiveReturnsFromParent(
std::vector<ReturnedResource> transferable_resources);
const 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
View file @ 2f8f447e
......@@ -16,7 +16,6 @@
#include "testing/gtest/include/gtest/gtest.h"
using testing::_;
using testing::Each;
namespace viz {
namespace {
......@@ -87,8 +86,6 @@ 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) {
......@@ -241,65 +238,6 @@ 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);
......@@ -616,7 +554,7 @@ TEST_P(ClientResourceProviderTest, ReleaseExportedResourcesThenRemove) {
provider().PrepareSendToParent({resource}, &list, context_provider());
EXPECT_EQ(1u, list.size());
// Drop any exported resources. They are now considered lost for gpu
// Drop any exported resources. Yhey 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());
......@@ -629,101 +567,5 @@ 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
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment