[Impeller] CommandPoolVK recycles command buffers too. (#50468)

Its a bit ambiguous whether or not resetting a cmd pool returns the cmd buffers resources back to the pool in a way that can be reused on subsequent frames. Since reseting the pool resets the cmd buffers to their initial state, they are safe to reuse once reset. Additionally, through profiling I can observe that there is less allocation in cmd buffer creation when the buffers themselves are recycled, at least on the Mali drivers.

### Before (Animated Advanced Blend Macrobenchmark)

![image](https://github.com/flutter/engine/assets/8975114/c0022d7a-d16e-4db5-862a-01d5a6157e18)

### After

![image](https://github.com/flutter/engine/assets/8975114/3b45f9a9-8811-4fbc-8b90-24338317df6e)

Author: jonahwilliams

impeller/renderer/backend/vulkan/command_pool_vk.cc

@@ -12,6 +12,7 @@
 #include "impeller/renderer/backend/vulkan/resource_manager_vk.h"
 
 #include "impeller/renderer/backend/vulkan/vk.h"  // IWYU pragma: keep.
+#include "vulkan/vulkan_handles.hpp"
 #include "vulkan/vulkan_structs.hpp"
 
 namespace impeller {
@@ -21,12 +22,18 @@ class BackgroundCommandPoolVK final {
  public:
   BackgroundCommandPoolVK(BackgroundCommandPoolVK&&) = default;
 
+  // The recycler also recycles command buffers that were never used, up to a
+  // limit of 16 per frame. This number was somewhat arbitrarily chosen.
+  static constexpr size_t kUnusedCommandBufferLimit = 16u;
+
   explicit BackgroundCommandPoolVK(
       vk::UniqueCommandPool&& pool,
       std::vector<vk::UniqueCommandBuffer>&& buffers,
+      size_t unused_count,
       std::weak_ptr<CommandPoolRecyclerVK> recycler)
       : pool_(std::move(pool)),
         buffers_(std::move(buffers)),
+        unused_count_(unused_count),
         recycler_(std::move(recycler)) {}
 
   ~BackgroundCommandPoolVK() {
@@ -39,9 +46,14 @@ class BackgroundCommandPoolVK final {
     if (!recycler) {
       return;
     }
-    buffers_.clear();
+    // If there are many unused command buffers, release some of them.
+    if (unused_count_ > kUnusedCommandBufferLimit) {
+      for (auto i = 0u; i < unused_count_; i++) {
+        buffers_.pop_back();
+      }
+    }
 
-    recycler->Reclaim(std::move(pool_));
+    recycler->Reclaim(std::move(pool_), std::move(buffers_));
   }
 
  private:
@@ -55,6 +67,7 @@ class BackgroundCommandPoolVK final {
   // wrapper type will attempt to reset the cmd buffer, and doing so may be a
   // thread safety violation as this may happen on the fence waiter thread.
   std::vector<vk::UniqueCommandBuffer> buffers_;
+  const size_t unused_count_;
   std::weak_ptr<CommandPoolRecyclerVK> recycler_;
 };
 
@@ -71,9 +84,16 @@ CommandPoolVK::~CommandPoolVK() {
   if (!recycler) {
     return;
   }
+  // Any unused command buffers are added to the set of used command buffers.
+  // both will be reset to the initial state when the pool is reset.
+  size_t unused_count = unused_command_buffers_.size();
+  for (auto i = 0u; i < unused_command_buffers_.size(); i++) {
+    collected_buffers_.push_back(std::move(unused_command_buffers_[i]));
+  }
+  unused_command_buffers_.clear();
 
   auto reset_pool_when_dropped = BackgroundCommandPoolVK(
-      std::move(pool_), std::move(collected_buffers_), recycler);
+      std::move(pool_), std::move(collected_buffers_), unused_count, recycler);
 
   UniqueResourceVKT<BackgroundCommandPoolVK> pool(
       context->GetResourceManager(), std::move(reset_pool_when_dropped));
@@ -90,6 +110,11 @@ vk::UniqueCommandBuffer CommandPoolVK::CreateCommandBuffer() {
   if (!pool_) {
     return {};
   }
+  if (!unused_command_buffers_.empty()) {
+    vk::UniqueCommandBuffer buffer = std::move(unused_command_buffers_.back());
+    unused_command_buffers_.pop_back();
+    return buffer;
+  }
 
   auto const device = context->GetDevice();
   vk::CommandBufferAllocateInfo info;
@@ -123,6 +148,10 @@ void CommandPoolVK::Destroy() {
   for (auto& buffer : collected_buffers_) {
     buffer.release();
   }
+  for (auto& buffer : unused_command_buffers_) {
+    buffer.release();
+  }
+  unused_command_buffers_.clear();
   collected_buffers_.clear();
 }
 
@@ -158,13 +187,13 @@ std::shared_ptr<CommandPoolVK> CommandPoolRecyclerVK::Get() {
   }
 
   // Otherwise, create a new resource and return it.
-  auto pool = Create();
-  if (!pool) {
+  auto data = Create();
+  if (!data || !data->pool) {
     return nullptr;
   }
 
-  auto const resource =
-      std::make_shared<CommandPoolVK>(std::move(*pool), context_);
+  auto const resource = std::make_shared<CommandPoolVK>(
+      std::move(data->pool), std::move(data->buffers), context_);
   pool_map.emplace(hash, resource);
 
   {
@@ -176,10 +205,11 @@ std::shared_ptr<CommandPoolVK> CommandPoolRecyclerVK::Get() {
 }
 
 // TODO(matanlurey): Return a status_or<> instead of nullopt when we have one.
-std::optional<vk::UniqueCommandPool> CommandPoolRecyclerVK::Create() {
-  // If we can reuse a command pool, do so.
-  if (auto pool = Reuse()) {
-    return pool;
+std::optional<CommandPoolRecyclerVK::RecycledData>
+CommandPoolRecyclerVK::Create() {
+  // If we can reuse a command pool and its buffers, do so.
+  if (auto data = Reuse()) {
+    return data;
   }
 
   // Otherwise, create a new one.
@@ -196,23 +226,27 @@ std::optional<vk::UniqueCommandPool> CommandPoolRecyclerVK::Create() {
   if (result != vk::Result::eSuccess) {
     return std::nullopt;
   }
-  return std::move(pool);
+  return CommandPoolRecyclerVK::RecycledData{.pool = std::move(pool),
+                                             .buffers = {}};
 }
 
-std::optional<vk::UniqueCommandPool> CommandPoolRecyclerVK::Reuse() {
+std::optional<CommandPoolRecyclerVK::RecycledData>
+CommandPoolRecyclerVK::Reuse() {
   // If there are no recycled pools, return nullopt.
   Lock recycled_lock(recycled_mutex_);
   if (recycled_.empty()) {
     return std::nullopt;
   }
 
   // Otherwise, remove and return a recycled pool.
-  auto pool = std::move(recycled_.back());
+  auto data = std::move(recycled_.back());
   recycled_.pop_back();
-  return std::move(pool);
+  return std::move(data);
 }
 
-void CommandPoolRecyclerVK::Reclaim(vk::UniqueCommandPool&& pool) {
+void CommandPoolRecyclerVK::Reclaim(
+    vk::UniqueCommandPool&& pool,
+    std::vector<vk::UniqueCommandBuffer>&& buffers) {
   // Reset the pool on a background thread.
   auto strong_context = context_.lock();
   if (!strong_context) {
@@ -223,7 +257,8 @@ void CommandPoolRecyclerVK::Reclaim(vk::UniqueCommandPool&& pool) {
 
   // Move the pool to the recycled list.
   Lock recycled_lock(recycled_mutex_);
-  recycled_.push_back(std::move(pool));
+  recycled_.push_back(
+      RecycledData{.pool = std::move(pool), .buffers = std::move(buffers)});
 }
 
 CommandPoolRecyclerVK::~CommandPoolRecyclerVK() {

impeller/renderer/backend/vulkan/command_pool_vk.h

@@ -11,6 +11,7 @@
 
 #include "impeller/base/thread.h"
 #include "impeller/renderer/backend/vulkan/vk.h"  // IWYU pragma: keep.
+#include "vulkan/vulkan_handles.hpp"
 
 namespace impeller {
 
@@ -34,10 +35,14 @@ class CommandPoolVK final {
   /// @brief      Creates a resource that manages the life of a command pool.
   ///
   /// @param[in]  pool      The command pool to manage.
+  /// @param[in]  buffers   Zero or more command buffers in an initial state.
   /// @param[in]  recycler  The context that will be notified on destruction.
-  explicit CommandPoolVK(vk::UniqueCommandPool pool,
-                         std::weak_ptr<ContextVK>& context)
-      : pool_(std::move(pool)), context_(context) {}
+  CommandPoolVK(vk::UniqueCommandPool pool,
+                std::vector<vk::UniqueCommandBuffer>&& buffers,
+                std::weak_ptr<ContextVK>& context)
+      : pool_(std::move(pool)),
+        unused_command_buffers_(std::move(buffers)),
+        context_(context) {}
 
   /// @brief      Creates and returns a new |vk::CommandBuffer|.
   ///
@@ -63,6 +68,7 @@ class CommandPoolVK final {
 
   Mutex pool_mutex_;
   vk::UniqueCommandPool pool_ IPLR_GUARDED_BY(pool_mutex_);
+  std::vector<vk::UniqueCommandBuffer> unused_command_buffers_;
   std::weak_ptr<ContextVK>& context_;
 
   // Used to retain a reference on these until the pool is reset.
@@ -99,6 +105,12 @@ class CommandPoolRecyclerVK final
  public:
   ~CommandPoolRecyclerVK();
 
+  /// A unique command pool and zero or more recycled command buffers.
+  struct RecycledData {
+    vk::UniqueCommandPool pool;
+    std::vector<vk::UniqueCommandBuffer> buffers;
+  };
+
   /// @brief      Clean up resources held by all per-thread command pools
   ///             associated with the given context.
   ///
@@ -119,7 +131,8 @@ class CommandPoolRecyclerVK final
   /// @brief      Returns a command pool to be reset on a background thread.
   ///
   /// @param[in]  pool The pool to recycler.
-  void Reclaim(vk::UniqueCommandPool&& pool);
+  void Reclaim(vk::UniqueCommandPool&& pool,
+               std::vector<vk::UniqueCommandBuffer>&& buffers);
 
   /// @brief      Clears all recycled command pools to let them be reclaimed.
   void Dispose();
@@ -128,17 +141,17 @@ class CommandPoolRecyclerVK final
   std::weak_ptr<ContextVK> context_;
 
   Mutex recycled_mutex_;
-  std::vector<vk::UniqueCommandPool> recycled_ IPLR_GUARDED_BY(recycled_mutex_);
+  std::vector<RecycledData> recycled_ IPLR_GUARDED_BY(recycled_mutex_);
 
   /// @brief      Creates a new |vk::CommandPool|.
   ///
   /// @returns    Returns a |std::nullopt| if a pool could not be created.
-  std::optional<vk::UniqueCommandPool> Create();
+  std::optional<CommandPoolRecyclerVK::RecycledData> Create();
 
-  /// @brief      Reuses a recycled |vk::CommandPool|, if available.
+  /// @brief      Reuses a recycled |RecycledData|, if available.
   ///
   /// @returns    Returns a |std::nullopt| if a pool was not available.
-  std::optional<vk::UniqueCommandPool> Reuse();
+  std::optional<RecycledData> Reuse();
 
   CommandPoolRecyclerVK(const CommandPoolRecyclerVK&) = delete;
 

impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc

@@ -112,5 +112,53 @@ TEST(CommandPoolRecyclerVKTest, ReclaimMakesCommandPoolAvailable) {
   context->Shutdown();
 }
 
+TEST(CommandPoolRecyclerVKTest, CommandBuffersAreRecycled) {
+  auto const context = MockVulkanContextBuilder().Build();
+
+  {
+    // Fetch a pool (which will be created).
+    auto const recycler = context->GetCommandPoolRecycler();
+    auto pool = recycler->Get();
+
+    auto buffer = pool->CreateCommandBuffer();
+    pool->CollectCommandBuffer(std::move(buffer));
+
+    // This normally is called at the end of a frame.
+    recycler->Dispose();
+  }
+
+  // Wait for the pool to be reclaimed.
+  auto waiter = fml::AutoResetWaitableEvent();
+  auto rattle = DeathRattle([&waiter]() { waiter.Signal(); });
+  {
+    UniqueResourceVKT<DeathRattle> resource(context->GetResourceManager(),
+                                            std::move(rattle));
+  }
+  waiter.Wait();
+
+  {
+    // Create a second pool and command buffer, which should reused the existing
+    // pool and cmd buffer.
+    auto const recycler = context->GetCommandPoolRecycler();
+    auto pool = recycler->Get();
+
+    auto buffer = pool->CreateCommandBuffer();
+    pool->CollectCommandBuffer(std::move(buffer));
+
+    // This normally is called at the end of a frame.
+    recycler->Dispose();
+  }
+
+  // Now check that we only ever created one pool and one command buffer.
+  auto const called = GetMockVulkanFunctions(context->GetDevice());
+  EXPECT_EQ(std::count(called->begin(), called->end(), "vkCreateCommandPool"),
+            1u);
+  EXPECT_EQ(
+      std::count(called->begin(), called->end(), "vkAllocateCommandBuffers"),
+      1u);
+
+  context->Shutdown();
+}
+
 }  // namespace testing
 }  // namespace impeller

impeller/renderer/backend/vulkan/test/mock_vulkan.cc

@@ -261,6 +261,7 @@ VkResult vkAllocateCommandBuffers(
     const VkCommandBufferAllocateInfo* pAllocateInfo,
     VkCommandBuffer* pCommandBuffers) {
   MockDevice* mock_device = reinterpret_cast<MockDevice*>(device);
+  mock_device->AddCalledFunction("vkAllocateCommandBuffers");
   *pCommandBuffers =
       reinterpret_cast<VkCommandBuffer>(mock_device->NewCommandBuffer());
   return VK_SUCCESS;