[Impeller] Document and slightly refactor ResourceManagerVK & friends. (flutter#45474)

@chinmaygarde specifically, let me know if I violated any correctness you were looking for, or if you would prefer I _don't_ make 1 or more of the changes I made here.

@gaaclarke would love your input on C++ style specifically, or anything that stands out in general.

---

## Summary

I plan to use the resource strategy in flutter/flutter#133198, but wanted to understand the code better first, so this is my contribution to make it a bit easier to understand (hopefully! push back if not!) and contribute tests.

1. Renamed `Reset(ResourceType)` to `Swap()` for consistency with the std smart pointers.
2. Moved non-trivial work out of the constructor into `::Create` ([style guide](https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors)).
3. Added some `FML_DCHECK`s to private APIs to enforce correctness.
4. Made classes final and methods private where they were effectively that ([style guide](https://google.github.io/styleguide/cppguide.html#Inheritance)).
5. Added tests to make sure I understood the contracts.

Author: matanlurey

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

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",
   ]

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;
   }
 

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> ResourceManagerVK::Create() {
-  return std::shared_ptr<ResourceManagerVK>(new ResourceManagerVK());
+  auto manager = std::shared_ptr<ResourceManagerVK>(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<ResourceVK> 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;

impeller/renderer/backend/vulkan/resource_manager_vk.h

@@ -8,18 +8,20 @@
 #include <memory>
 #include <mutex>
 #include <thread>
-#include <type_traits>
 #include <vector>
 
 #include "flutter/fml/macros.h"
 
 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<SomeResource>(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<ResourceManagerVK> {
  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<ResourceManagerVK> 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<ResourceVK> 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<std::unique_ptr<ResourceVK>>;
 
+  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 ResourceType_>
 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 ResourceType_>
-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<ResourceManagerVK> 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<ResourceManagerVK> 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<ResourceVKT<ResourceType>>(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_));
     }

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 <sys/types.h>
+#include <functional>
+#include <memory>
+#include <utility>
+#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<void()> callback)
+      : callback_(std::move(callback)) {}
+
+  DeathRattle(DeathRattle&&) = default;
+  DeathRattle& operator=(DeathRattle&&) = default;
+
+  ~DeathRattle() { callback_(); }
+
+ private:
+  std::function<void()> 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<DeathRattle>(manager, std::move(rattle));
+  }
+
+  waiter.Wait();
+}
+
+}  // namespace testing
+}  // namespace impeller