diff --git a/ci/licenses_golden/excluded_files b/ci/licenses_golden/excluded_files index dd7a5cb1cbf97..512261ff9e50b 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/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/allocator_vk.cc b/impeller/renderer/backend/vulkan/allocator_vk.cc index 93104a1c91944..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_.Reset(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 1acc90f60e3b8..f68c9ef0bd366 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" @@ -16,10 +15,13 @@ 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; @@ -33,66 +35,95 @@ 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: //---------------------------------------------------------------------------- - /// @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. /// 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 Terminate the resource manager. Any resources given to the - /// resource manager post termination will be collected when the - /// resource manager is collected. + /// @brief Destroys the resource manager. /// - void Terminate(); + /// The resource manager will stop collecting resources and will be destroyed + /// when all references to it are dropped. + ~ResourceManagerVK(); 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); }; +//------------------------------------------------------------------------------ +/// @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); @@ -105,13 +136,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 `Swap`. + /// + /// @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 `Swap`. + /// + /// @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)), @@ -120,19 +163,30 @@ class UniqueResourceVKT { ~UniqueResourceVKT() { Reset(); } - const ResourceType* operator->() const { return resource_.get()->Get(); } + /// @brief Returns a pointer to the resource. + const ResourceType* operator->() const { + // If this would segfault, fail with a nicer error message. + FML_CHECK(resource_) << "UniqueResourceVKT was reclaimed."; + + return resource_.get()->Get(); + } - void Reset(ResourceType&& other) { + /// @brief Reclaims the existing resource, if any, and replaces it. + /// + /// @param[in] other The (new) resource to move. + void Swap(ResourceType&& other) { Reset(); 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_)); } 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..dc629b730b1ab --- /dev/null +++ b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc @@ -0,0 +1,62 @@ +// 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 +#include +#include +#include "fml/synchronization/waitable_event.h" +#include "gtest/gtest.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, CreatesANewInstance) { + auto const a = ResourceManagerVK::Create(); + auto const b = ResourceManagerVK::Create(); + EXPECT_NE(a, b); +} + +namespace { + +// 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_; +}; + +} // namespace + +TEST(ResourceManagerVKTest, ReclaimMovesAResourceAndDestroysIt) { + auto const manager = ResourceManagerVK::Create(); + + auto waiter = fml::AutoResetWaitableEvent(); + auto dead = false; + auto rattle = DeathRattle([&waiter]() { waiter.Signal(); }); + + // Not killed immediately. + EXPECT_FALSE(waiter.IsSignaledForTest()); + + { + auto resource = UniqueResourceVKT(manager, std::move(rattle)); + } + + waiter.Wait(); +} + +} // namespace testing +} // namespace impeller