From fe13390331fb7e764729c0e0efe2e3ff0cdb6e7d Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 5 Sep 2023 14:22:15 -0700 Subject: [PATCH 01/10] Clarify that `::Create` does not enforce 1-per context. Add a test. --- impeller/renderer/backend/vulkan/BUILD.gn | 1 + .../backend/vulkan/resource_manager_vk.h | 11 +++++++--- .../vulkan/resource_manager_vk_unittests.cc | 20 +++++++++++++++++++ 3 files changed, 29 insertions(+), 3 deletions(-) create mode 100644 impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc diff --git a/impeller/renderer/backend/vulkan/BUILD.gn b/impeller/renderer/backend/vulkan/BUILD.gn index a0e44503e20d8..d7abddb096d74 100644 --- a/impeller/renderer/backend/vulkan/BUILD.gn +++ b/impeller/renderer/backend/vulkan/BUILD.gn @@ -11,6 +11,7 @@ impeller_component("vulkan_unittests") { "blit_command_vk_unittests.cc", "context_vk_unittests.cc", "pass_bindings_cache_unittests.cc", + "resource_manager_vk_unittests.cc", "test/mock_vulkan.cc", "test/mock_vulkan.h", ] diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk.h b/impeller/renderer/backend/vulkan/resource_manager_vk.h index 1acc90f60e3b8..89118e952a825 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk.h +++ b/impeller/renderer/backend/vulkan/resource_manager_vk.h @@ -37,9 +37,14 @@ class ResourceManagerVK : public std::enable_shared_from_this { public: //---------------------------------------------------------------------------- - /// @brief Create a shared resource manager. This creates a dedicated - /// thread for resource management. Only one is created per Vulkan - /// context. + /// @brief Creates a shared resource manager (a dedicated thread). + /// + /// Upon creation, a thread is spawned which will collect resources as they + /// are reclaimed (passed to `Reclaim`). The thread will exit when the + /// resource manager is destroyed. + /// + /// @note Only one |ResourceManagerVK| should be created per Vulkan + /// context, but that contract is not enforced by this method. /// /// @return A resource manager if one could be created. /// diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc new file mode 100644 index 0000000000000..79a68efb9359f --- /dev/null +++ b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc @@ -0,0 +1,20 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include +#include "flutter/testing/testing.h" +#include "impeller/renderer/backend/vulkan/resource_manager_vk.h" + +namespace impeller { +namespace testing { + +// While expected to be a singleton per context, the class does not enforce it. +TEST(ResourceManagerVKTest, CreateCreatesANewInstance) { + auto const a = ResourceManagerVK::Create(); + auto const b = ResourceManagerVK::Create(); + EXPECT_NE(a, b); +} + +} // namespace testing +} // namespace impeller From 6c5721f9c3f79e6e736190ccac7d0bd58614cb9f Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 5 Sep 2023 14:51:42 -0700 Subject: [PATCH 02/10] Tweak some naming and comments. --- .../renderer/backend/vulkan/allocator_vk.cc | 4 +- .../backend/vulkan/resource_manager_vk.h | 54 ++++++++++++++++--- 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/impeller/renderer/backend/vulkan/allocator_vk.cc b/impeller/renderer/backend/vulkan/allocator_vk.cc index 93104a1c91944..bbdd19616e23d 100644 --- a/impeller/renderer/backend/vulkan/allocator_vk.cc +++ b/impeller/renderer/backend/vulkan/allocator_vk.cc @@ -365,8 +365,8 @@ class AllocatedTextureSourceVK final : public TextureSourceVK { << vk::to_string(result); return; } - resource_.Reset(ImageResource(ImageVMA{allocator, allocation, image}, - std::move(image_view))); + resource_.Replace(ImageResource(ImageVMA{allocator, allocation, image}, + std::move(image_view))); is_valid_ = true; } diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk.h b/impeller/renderer/backend/vulkan/resource_manager_vk.h index 89118e952a825..8378827fc1e9d 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk.h +++ b/impeller/renderer/backend/vulkan/resource_manager_vk.h @@ -16,13 +16,20 @@ namespace impeller { //------------------------------------------------------------------------------ -/// @brief A resource that will be reclaimed by the resource manager. Do -/// not subclass this yourself. Instead, use the `UniqueResourceVKT` -/// template +/// @brief A resource that may be reclaimed by a |ResourceManagerVK|. /// +/// To create a resource, use `UniqueResourceVKT` to create a unique handle: +/// +/// auto resource = UniqueResourceVKT(resource_manager); +/// +/// @see |ResourceManagerVK::Reclaim|. class ResourceVK { public: virtual ~ResourceVK() = default; + + private: + // Prevents subclassing, use `UniqueResourceVKT`. + ResourceVK() = default; }; //------------------------------------------------------------------------------ @@ -87,17 +94,32 @@ class ResourceManagerVK FML_DISALLOW_COPY_AND_ASSIGN(ResourceManagerVK); }; +//------------------------------------------------------------------------------ +/// @brief An internal type that is used to move a resource reference. +/// +/// Do not use directly, use `UniqueResourceVKT` instead. +/// +/// @tparam ResourceType_ The type of the resource. +/// +/// @see |UniqueResourceVKT|. template class ResourceVKT : public ResourceVK { public: using ResourceType = ResourceType_; + /// @brief Construct a resource from a move-constructible resource. + /// + /// @param[in] resource The resource to move. explicit ResourceVKT(ResourceType&& resource) : resource_(std::move(resource)) {} + /// @brief Returns a pointer to the resource. const ResourceType* Get() const { return &resource_; } private: + // Prevents subclassing, use `UniqueResourceVKT`. + ResourceVKT() = default; + ResourceType resource_; FML_DISALLOW_COPY_AND_ASSIGN(ResourceVKT); @@ -110,13 +132,25 @@ class ResourceVKT : public ResourceVK { /// @tparam ResourceType_ A move-constructible resource type. /// template -class UniqueResourceVKT { +class UniqueResourceVKT final { public: using ResourceType = ResourceType_; + /// @brief Construct a unique resource handle belonging to a manager. + /// + /// Initially the handle is empty, and can be populated by calling `Replace`. + /// + /// @param[in] resource_manager The resource manager. explicit UniqueResourceVKT(std::weak_ptr resource_manager) : resource_manager_(std::move(resource_manager)) {} + /// @brief Construct a unique resource handle belonging to a manager. + /// + /// Initially the handle is populated with the specified resource, but can + /// be replaced (reclaiming the old resource) by calling `Replace`. + /// + /// @param[in] resource_manager The resource manager. + /// @param[in] resource The resource to move. explicit UniqueResourceVKT(std::weak_ptr resource_manager, ResourceType&& resource) : resource_manager_(std::move(resource_manager)), @@ -125,19 +159,25 @@ class UniqueResourceVKT { ~UniqueResourceVKT() { Reset(); } + /// @brief Returns a pointer to the resource. const ResourceType* operator->() const { return resource_.get()->Get(); } - void Reset(ResourceType&& other) { - Reset(); + /// @brief Reclaims the existing resource, if any, and replaces it. + /// + /// @param[in] other The (new) resource to move. + void Replace(ResourceType&& other) { + Replace(); resource_ = std::make_unique>(std::move(other)); } + /// @brief Reclaims the existing resource, if any. void Reset() { if (!resource_) { return; } // If there is a manager, ask it to reclaim the resource. If there isn't a - // manager, just drop it on the floor here. + // manager (because the manager has been destroyed), just drop it on the + // floor here. if (auto manager = resource_manager_.lock()) { manager->Reclaim(std::move(resource_)); } From f15946b1fcca9fd8801f2c100d1fd9abec9a0882 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 5 Sep 2023 14:52:01 -0700 Subject: [PATCH 03/10] Remove unused include. --- impeller/renderer/backend/vulkan/resource_manager_vk.h | 1 - 1 file changed, 1 deletion(-) diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk.h b/impeller/renderer/backend/vulkan/resource_manager_vk.h index 8378827fc1e9d..f67242cff71ad 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk.h +++ b/impeller/renderer/backend/vulkan/resource_manager_vk.h @@ -8,7 +8,6 @@ #include #include #include -#include #include #include "flutter/fml/macros.h" From 89f3fd4d26d95e6275ff99bb0df831fe8365841f Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 5 Sep 2023 17:45:27 -0700 Subject: [PATCH 04/10] Finish unit tests and docs. --- .../renderer/backend/vulkan/allocator_vk.cc | 4 +- .../backend/vulkan/resource_manager_vk.cc | 16 +++- .../backend/vulkan/resource_manager_vk.h | 38 ++++----- .../vulkan/resource_manager_vk_unittests.cc | 85 +++++++++++++++++++ 4 files changed, 119 insertions(+), 24 deletions(-) diff --git a/impeller/renderer/backend/vulkan/allocator_vk.cc b/impeller/renderer/backend/vulkan/allocator_vk.cc index bbdd19616e23d..abf418688f118 100644 --- a/impeller/renderer/backend/vulkan/allocator_vk.cc +++ b/impeller/renderer/backend/vulkan/allocator_vk.cc @@ -365,8 +365,8 @@ class AllocatedTextureSourceVK final : public TextureSourceVK { << vk::to_string(result); return; } - resource_.Replace(ImageResource(ImageVMA{allocator, allocation, image}, - std::move(image_view))); + resource_.Swap(ImageResource(ImageVMA{allocator, allocation, image}, + std::move(image_view))); is_valid_ = true; } diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk.cc b/impeller/renderer/backend/vulkan/resource_manager_vk.cc index 1be606bfdba17..6569f7a61667f 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk.cc +++ b/impeller/renderer/backend/vulkan/resource_manager_vk.cc @@ -6,21 +6,28 @@ #include "flutter/fml/thread.h" #include "flutter/fml/trace_event.h" +#include "fml/logging.h" namespace impeller { std::shared_ptr ResourceManagerVK::Create() { - return std::shared_ptr(new ResourceManagerVK()); + auto manager = std::shared_ptr(new ResourceManagerVK()); + manager->waiter_ = std::thread([manager]() { manager->Start(); }); + manager->waiter_.detach(); + return manager; } -ResourceManagerVK::ResourceManagerVK() : waiter_([&]() { Main(); }) {} +ResourceManagerVK::ResourceManagerVK() {} ResourceManagerVK::~ResourceManagerVK() { Terminate(); waiter_.join(); } -void ResourceManagerVK::Main() { +void ResourceManagerVK::Start() { + // This thread should not be started more than once. + FML_DCHECK(!should_exit_); + fml::Thread::SetCurrentThreadName( fml::Thread::ThreadConfig{"io.flutter.impeller.resource_manager"}); @@ -65,6 +72,9 @@ void ResourceManagerVK::Reclaim(std::unique_ptr resource) { } void ResourceManagerVK::Terminate() { + // The thread should not be terminated more than once. + FML_DCHECK(!should_exit_); + { std::scoped_lock lock(reclaimables_mutex_); should_exit_ = true; diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk.h b/impeller/renderer/backend/vulkan/resource_manager_vk.h index f67242cff71ad..2f9230ac8d225 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk.h +++ b/impeller/renderer/backend/vulkan/resource_manager_vk.h @@ -25,10 +25,6 @@ namespace impeller { class ResourceVK { public: virtual ~ResourceVK() = default; - - private: - // Prevents subclassing, use `UniqueResourceVKT`. - ResourceVK() = default; }; //------------------------------------------------------------------------------ @@ -39,7 +35,7 @@ class ResourceVK { /// thread. In the future, the resource manager may allow resource /// pooling/reuse, delaying reclamation past frame workloads, etc... /// -class ResourceManagerVK +class ResourceManagerVK final : public std::enable_shared_from_this { public: //---------------------------------------------------------------------------- @@ -70,25 +66,29 @@ class ResourceManagerVK /// void Reclaim(std::unique_ptr resource); - //---------------------------------------------------------------------------- - /// @brief Terminate the resource manager. Any resources given to the - /// resource manager post termination will be collected when the - /// resource manager is collected. - /// - void Terminate(); - private: - ResourceManagerVK(); - using Reclaimables = std::vector>; + ResourceManagerVK(); + std::thread waiter_; std::mutex reclaimables_mutex_; std::condition_variable reclaimables_cv_; Reclaimables reclaimables_; bool should_exit_ = false; - void Main(); + //---------------------------------------------------------------------------- + /// @brief Starts the resource manager thread. + /// + /// This method is called implicitly by `Create`. + void Start(); + + //---------------------------------------------------------------------------- + /// @brief Terminates the resource manager thread. + /// + /// Any resources given to the resource manager post termination will be + /// collected when the resource manager is collected. + void Terminate(); FML_DISALLOW_COPY_AND_ASSIGN(ResourceManagerVK); }; @@ -137,7 +137,7 @@ class UniqueResourceVKT final { /// @brief Construct a unique resource handle belonging to a manager. /// - /// Initially the handle is empty, and can be populated by calling `Replace`. + /// Initially the handle is empty, and can be populated by calling `Swap`. /// /// @param[in] resource_manager The resource manager. explicit UniqueResourceVKT(std::weak_ptr resource_manager) @@ -146,7 +146,7 @@ class UniqueResourceVKT final { /// @brief Construct a unique resource handle belonging to a manager. /// /// Initially the handle is populated with the specified resource, but can - /// be replaced (reclaiming the old resource) by calling `Replace`. + /// be replaced (reclaiming the old resource) by calling `Swap`. /// /// @param[in] resource_manager The resource manager. /// @param[in] resource The resource to move. @@ -164,8 +164,8 @@ class UniqueResourceVKT final { /// @brief Reclaims the existing resource, if any, and replaces it. /// /// @param[in] other The (new) resource to move. - void Replace(ResourceType&& other) { - Replace(); + void Swap(ResourceType&& other) { + Reset(); resource_ = std::make_unique>(std::move(other)); } diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc index 79a68efb9359f..fab4f02e0c1bc 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc @@ -9,6 +9,11 @@ namespace impeller { namespace testing { +// In a real app, this might be a texture or another resource buffer. +struct FakeBuffer { + int id; +}; + // While expected to be a singleton per context, the class does not enforce it. TEST(ResourceManagerVKTest, CreateCreatesANewInstance) { auto const a = ResourceManagerVK::Create(); @@ -16,5 +21,85 @@ TEST(ResourceManagerVKTest, CreateCreatesANewInstance) { EXPECT_NE(a, b); } +TEST(ResourceManagerVKTest, ReclaimResetsTheUnderlyingResource) { + auto const manager = ResourceManagerVK::Create(); + + UniqueResourceVKT resource(manager, FakeBuffer{1}); + EXPECT_EQ(resource->id, 1); + + resource.Reset(); + EXPECT_EQ(resource->id, 0); +} + +TEST(ResourceManagerVKTest, CallingResetTwiceHasNoEffect) { + auto const manager = ResourceManagerVK::Create(); + + UniqueResourceVKT resource(manager, FakeBuffer{1}); + EXPECT_EQ(resource->id, 1); + + resource.Reset(); + resource.Reset(); + EXPECT_EQ(resource->id, 0); +} + +TEST(ResourceManagerVKTest, ResetWaitsForLock) { + auto const manager = ResourceManagerVK::Create(); + + // Create a resource and lock it by holding a reference to it. + UniqueResourceVKT resource(manager, FakeBuffer{1}); + auto const& resource_ref = resource; + + // Reset the resource in a separate thread. + std::thread reset_thread([&resource]() { resource.Reset(); }); + + // Wait for the thread to start. + while (resource->id != 1) { + std::this_thread::yield(); + } + + // The resource should not have been reset yet. + EXPECT_EQ(resource->id, 1); + + // Wait for the thread to finish. + reset_thread.join(); + + // The resource should have been reset. + EXPECT_EQ(resource->id, 0); +} + +TEST(ResourceManagerVKTest, SwapReplacesTheUnderlyingResource) { + auto const manager = ResourceManagerVK::Create(); + + UniqueResourceVKT resource(manager, FakeBuffer{1}); + EXPECT_EQ(resource->id, 1); + + resource.Swap(FakeBuffer{2}); + EXPECT_EQ(resource->id, 2); +} + +// A class that, upon destruction, invokes a callback. +class Scoped { + public: + explicit Scoped(std::function callback) + : callback_(std::move(callback)) {} + ~Scoped() { callback_(); } + + private: + std::function callback_; +}; + +TEST(ResourceManagerVKTest, ResourceFallingOutOfScopeResetsTheResource) { + auto const manager = ResourceManagerVK::Create(); + auto was_destroyed = false; + + { + UniqueResourceVKT resource( + manager, Scoped([&was_destroyed]() { was_destroyed = true; })); + EXPECT_FALSE(was_destroyed); + } + + EXPECT_TRUE(was_destroyed); +} + } // namespace testing } // namespace impeller From ca719baa4bfecf88637e4765822b4262bafe4725 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 5 Sep 2023 19:53:13 -0700 Subject: [PATCH 05/10] Aspirational tests were merely aspirational. --- ci/licenses_golden/excluded_files | 1 + .../backend/vulkan/resource_manager_vk.h | 26 ++++-- .../vulkan/resource_manager_vk_unittests.cc | 88 +------------------ 3 files changed, 22 insertions(+), 93 deletions(-) diff --git a/ci/licenses_golden/excluded_files b/ci/licenses_golden/excluded_files index 6313f5fba1325..fafe38dc63bb0 100644 --- a/ci/licenses_golden/excluded_files +++ b/ci/licenses_golden/excluded_files @@ -153,6 +153,7 @@ ../../../flutter/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc ../../../flutter/impeller/renderer/backend/vulkan/context_vk_unittests.cc ../../../flutter/impeller/renderer/backend/vulkan/pass_bindings_cache_unittests.cc +../../../flutter/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc ../../../flutter/impeller/renderer/backend/vulkan/test ../../../flutter/impeller/renderer/capabilities_unittests.cc ../../../flutter/impeller/renderer/compute_subgroup_unittests.cc diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk.h b/impeller/renderer/backend/vulkan/resource_manager_vk.h index 2f9230ac8d225..f68c9ef0bd366 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk.h +++ b/impeller/renderer/backend/vulkan/resource_manager_vk.h @@ -53,19 +53,24 @@ class ResourceManagerVK final static std::shared_ptr Create(); //---------------------------------------------------------------------------- - /// @brief Destroy the resource manager and join all thread. An - /// unreclaimed resources will be destroyed now. + /// @brief Mark a resource as being reclaimable. /// - ~ResourceManagerVK(); - - //---------------------------------------------------------------------------- - /// @brief Mark a resource as being reclaimable by giving ownership of - /// the resource to the resource manager. + /// The resource will be reset at some point in the future. /// /// @param[in] resource The resource to reclaim. /// + /// @note Despite being a public API, this method cannot be invoked + /// directly. Instead, use `UniqueResourceVKT` to create a unique + /// handle to a resource, which will call this method. void Reclaim(std::unique_ptr resource); + //---------------------------------------------------------------------------- + /// @brief Destroys the resource manager. + /// + /// The resource manager will stop collecting resources and will be destroyed + /// when all references to it are dropped. + ~ResourceManagerVK(); + private: using Reclaimables = std::vector>; @@ -159,7 +164,12 @@ class UniqueResourceVKT final { ~UniqueResourceVKT() { Reset(); } /// @brief Returns a pointer to the resource. - const ResourceType* operator->() const { return resource_.get()->Get(); } + const ResourceType* operator->() const { + // If this would segfault, fail with a nicer error message. + FML_CHECK(resource_) << "UniqueResourceVKT was reclaimed."; + + return resource_.get()->Get(); + } /// @brief Reclaims the existing resource, if any, and replaces it. /// diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc index fab4f02e0c1bc..cca7b7f794581 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc @@ -3,103 +3,21 @@ // found in the LICENSE file. #include +#include #include "flutter/testing/testing.h" #include "impeller/renderer/backend/vulkan/resource_manager_vk.h" namespace impeller { namespace testing { -// In a real app, this might be a texture or another resource buffer. -struct FakeBuffer { - int id; -}; - // While expected to be a singleton per context, the class does not enforce it. -TEST(ResourceManagerVKTest, CreateCreatesANewInstance) { +TEST(ResourceManagerVKTest, CreatesANewInstance) { auto const a = ResourceManagerVK::Create(); auto const b = ResourceManagerVK::Create(); EXPECT_NE(a, b); } -TEST(ResourceManagerVKTest, ReclaimResetsTheUnderlyingResource) { - auto const manager = ResourceManagerVK::Create(); - - UniqueResourceVKT resource(manager, FakeBuffer{1}); - EXPECT_EQ(resource->id, 1); - - resource.Reset(); - EXPECT_EQ(resource->id, 0); -} - -TEST(ResourceManagerVKTest, CallingResetTwiceHasNoEffect) { - auto const manager = ResourceManagerVK::Create(); - - UniqueResourceVKT resource(manager, FakeBuffer{1}); - EXPECT_EQ(resource->id, 1); - - resource.Reset(); - resource.Reset(); - EXPECT_EQ(resource->id, 0); -} - -TEST(ResourceManagerVKTest, ResetWaitsForLock) { - auto const manager = ResourceManagerVK::Create(); - - // Create a resource and lock it by holding a reference to it. - UniqueResourceVKT resource(manager, FakeBuffer{1}); - auto const& resource_ref = resource; - - // Reset the resource in a separate thread. - std::thread reset_thread([&resource]() { resource.Reset(); }); - - // Wait for the thread to start. - while (resource->id != 1) { - std::this_thread::yield(); - } - - // The resource should not have been reset yet. - EXPECT_EQ(resource->id, 1); - - // Wait for the thread to finish. - reset_thread.join(); - - // The resource should have been reset. - EXPECT_EQ(resource->id, 0); -} - -TEST(ResourceManagerVKTest, SwapReplacesTheUnderlyingResource) { - auto const manager = ResourceManagerVK::Create(); - - UniqueResourceVKT resource(manager, FakeBuffer{1}); - EXPECT_EQ(resource->id, 1); - - resource.Swap(FakeBuffer{2}); - EXPECT_EQ(resource->id, 2); -} - -// A class that, upon destruction, invokes a callback. -class Scoped { - public: - explicit Scoped(std::function callback) - : callback_(std::move(callback)) {} - ~Scoped() { callback_(); } - - private: - std::function callback_; -}; - -TEST(ResourceManagerVKTest, ResourceFallingOutOfScopeResetsTheResource) { - auto const manager = ResourceManagerVK::Create(); - auto was_destroyed = false; - - { - UniqueResourceVKT resource( - manager, Scoped([&was_destroyed]() { was_destroyed = true; })); - EXPECT_FALSE(was_destroyed); - } - - EXPECT_TRUE(was_destroyed); -} +// TODO: See what Aaron and Chinmay think can be tested here, and how. } // namespace testing } // namespace impeller From 280fc767c4b85d69f8ffe6e9d489136c3a848e24 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 7 Sep 2023 09:27:28 -0700 Subject: [PATCH 06/10] Added a test that yields for resource cleanup. --- .../vulkan/resource_manager_vk_unittests.cc | 49 ++++++++++++++++++- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc index cca7b7f794581..26e73ab0b8d9b 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc @@ -3,8 +3,10 @@ // found in the LICENSE file. #include +#include #include -#include "flutter/testing/testing.h" +#include +#include "gtest/gtest.h" #include "impeller/renderer/backend/vulkan/resource_manager_vk.h" namespace impeller { @@ -17,7 +19,50 @@ TEST(ResourceManagerVKTest, CreatesANewInstance) { EXPECT_NE(a, b); } -// TODO: See what Aaron and Chinmay think can be tested here, and how. +// Invokes the provided callback when the destructor is called. +// +// Can be moved, but not copied. +class DeathRattle final { + public: + explicit DeathRattle(std::function callback) + : callback_(std::move(callback)) {} + + DeathRattle(DeathRattle&&) = default; + DeathRattle& operator=(DeathRattle&&) = default; + + ~DeathRattle() { callback_(); } + + private: + std::function callback_; +}; + +TEST(ResourceManagerVKTest, ReclaimMovesAResourceAndDestroysIt) { + auto const manager = ResourceManagerVK::Create(); + + auto dead = false; + auto rattle = DeathRattle([&dead]() { dead = true; }); + + // Not killed immediately. + EXPECT_FALSE(dead); + + { + auto resource = UniqueResourceVKT(manager, std::move(rattle)); + + // Not killed on moving. + EXPECT_FALSE(dead); + } + + // Not killed synchronously. + EXPECT_FALSE(dead); + + // A background thread reclaims; give it a chance to finish killing. + while (!dead) { + std::this_thread::yield(); + } + + // The resource should be dead now. + EXPECT_TRUE(dead); +} } // namespace testing } // namespace impeller From 82fb32c271f77e339e86b0f7b18d196331a302ea Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 7 Sep 2023 09:52:24 -0700 Subject: [PATCH 07/10] Thanks Aaron. --- .../vulkan/resource_manager_vk_unittests.cc | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc index 26e73ab0b8d9b..8f597a54f6127 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc @@ -6,6 +6,7 @@ #include #include #include +#include "fml/synchronization/waitable_event.h" #include "gtest/gtest.h" #include "impeller/renderer/backend/vulkan/resource_manager_vk.h" @@ -19,6 +20,8 @@ TEST(ResourceManagerVKTest, CreatesANewInstance) { EXPECT_NE(a, b); } +namespace { + // Invokes the provided callback when the destructor is called. // // Can be moved, but not copied. @@ -36,32 +39,26 @@ class DeathRattle final { std::function callback_; }; +} // namespace + TEST(ResourceManagerVKTest, ReclaimMovesAResourceAndDestroysIt) { auto const manager = ResourceManagerVK::Create(); + auto waiter = fml::AutoResetWaitableEvent(); auto dead = false; - auto rattle = DeathRattle([&dead]() { dead = true; }); + auto rattle = DeathRattle([&waiter]() { waiter.Signal(); }); // Not killed immediately. - EXPECT_FALSE(dead); + EXPECT_FALSE(waiter.IsSignaledForTest()); { auto resource = UniqueResourceVKT(manager, std::move(rattle)); // Not killed on moving. - EXPECT_FALSE(dead); - } - - // Not killed synchronously. - EXPECT_FALSE(dead); - - // A background thread reclaims; give it a chance to finish killing. - while (!dead) { - std::this_thread::yield(); + EXPECT_FALSE(waiter.IsSignaledForTest()); } - // The resource should be dead now. - EXPECT_TRUE(dead); + waiter.Wait(); } } // namespace testing From 5ccc222661b60c739d514fae22d93dcb7e84d996 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 7 Sep 2023 09:53:26 -0700 Subject: [PATCH 08/10] Use make_shared. --- impeller/renderer/backend/vulkan/resource_manager_vk.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk.cc b/impeller/renderer/backend/vulkan/resource_manager_vk.cc index 6569f7a61667f..73681f9056db2 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk.cc +++ b/impeller/renderer/backend/vulkan/resource_manager_vk.cc @@ -11,7 +11,7 @@ namespace impeller { std::shared_ptr ResourceManagerVK::Create() { - auto manager = std::shared_ptr(new ResourceManagerVK()); + auto manager = std::make_shared(); manager->waiter_ = std::thread([manager]() { manager->Start(); }); manager->waiter_.detach(); return manager; From 504853d6c29d36f588d631c575418096ce73ef27 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 7 Sep 2023 10:54:35 -0700 Subject: [PATCH 09/10] Go back to shared_ptr. --- impeller/renderer/backend/vulkan/resource_manager_vk.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk.cc b/impeller/renderer/backend/vulkan/resource_manager_vk.cc index 73681f9056db2..6569f7a61667f 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk.cc +++ b/impeller/renderer/backend/vulkan/resource_manager_vk.cc @@ -11,7 +11,7 @@ namespace impeller { std::shared_ptr ResourceManagerVK::Create() { - auto manager = std::make_shared(); + auto manager = std::shared_ptr(new ResourceManagerVK()); manager->waiter_ = std::thread([manager]() { manager->Start(); }); manager->waiter_.detach(); return manager; From 4b6dad042ec53d1344e1627f1e8efbfec35b86a0 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 7 Sep 2023 10:56:28 -0700 Subject: [PATCH 10/10] Make test more resilient. --- .../renderer/backend/vulkan/resource_manager_vk_unittests.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc index 8f597a54f6127..dc629b730b1ab 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc @@ -53,9 +53,6 @@ TEST(ResourceManagerVKTest, ReclaimMovesAResourceAndDestroysIt) { { auto resource = UniqueResourceVKT(manager, std::move(rattle)); - - // Not killed on moving. - EXPECT_FALSE(waiter.IsSignaledForTest()); } waiter.Wait();