Reverts "[Impeller] one descriptor pool per frame. (#55939)" (#55959)

Reverts: #55939
Initiated by: jonahwilliams
Reason for reverting: failing on scenario test
Original PR Author: jonahwilliams

Reviewed By: {matanlurey}

This change reverts the following previous change:
Creating descriptor pools is expensive, no need to have more than one per frame.

The lifecycle of the descriptor pool is exactly the same as the cmd pool, which indicates that we can probably do some consolidation/refactoring in the future.

Fixes flutter/flutter#157115

Author: auto-submit[bot]

impeller/renderer/backend/vulkan/command_buffer_vk.cc

@@ -22,10 +22,12 @@ namespace impeller {
 CommandBufferVK::CommandBufferVK(
     std::weak_ptr<const Context> context,
     std::weak_ptr<const DeviceHolderVK> device_holder,
-    std::shared_ptr<TrackedObjectsVK> tracked_objects)
+    std::shared_ptr<TrackedObjectsVK> tracked_objects,
+    std::shared_ptr<FenceWaiterVK> fence_waiter)
     : CommandBuffer(std::move(context)),
       device_holder_(std::move(device_holder)),
-      tracked_objects_(std::move(tracked_objects)) {}
+      tracked_objects_(std::move(tracked_objects)),
+      fence_waiter_(std::move(fence_waiter)) {}
 
 CommandBufferVK::~CommandBufferVK() = default;
 
@@ -210,8 +212,4 @@ void CommandBufferVK::InsertDebugMarker(std::string_view label) const {
   }
 }
 
-DescriptorPoolVK& CommandBufferVK::GetDescriptorPool() const {
-  return tracked_objects_->GetDescriptorPool();
-}
-
 }  // namespace impeller

impeller/renderer/backend/vulkan/command_buffer_vk.h

@@ -8,7 +8,6 @@
 #include "fml/status_or.h"
 #include "impeller/base/backend_cast.h"
 #include "impeller/renderer/backend/vulkan/command_queue_vk.h"
-#include "impeller/renderer/backend/vulkan/descriptor_pool_vk.h"
 #include "impeller/renderer/backend/vulkan/device_holder_vk.h"
 #include "impeller/renderer/backend/vulkan/texture_source_vk.h"
 #include "impeller/renderer/backend/vulkan/tracked_objects_vk.h"
@@ -82,19 +81,18 @@ class CommandBufferVK final
   // Visible for testing.
   bool IsTracking(const std::shared_ptr<const Texture>& texture) const;
 
-  // Visible for testing.
-  DescriptorPoolVK& GetDescriptorPool() const;
-
  private:
   friend class ContextVK;
   friend class CommandQueueVK;
 
   std::weak_ptr<const DeviceHolderVK> device_holder_;
   std::shared_ptr<TrackedObjectsVK> tracked_objects_;
+  std::shared_ptr<FenceWaiterVK> fence_waiter_;
 
   CommandBufferVK(std::weak_ptr<const Context> context,
                   std::weak_ptr<const DeviceHolderVK> device_holder,
-                  std::shared_ptr<TrackedObjectsVK> tracked_objects);
+                  std::shared_ptr<TrackedObjectsVK> tracked_objects,
+                  std::shared_ptr<FenceWaiterVK> fence_waiter);
 
   // |CommandBuffer|
   void SetLabel(std::string_view label) const override;

impeller/renderer/backend/vulkan/command_pool_vk.cc

@@ -9,7 +9,6 @@
 #include <utility>
 
 #include "impeller/renderer/backend/vulkan/context_vk.h"
-#include "impeller/renderer/backend/vulkan/descriptor_pool_vk.h"
 #include "impeller/renderer/backend/vulkan/resource_manager_vk.h"
 
 #include "impeller/renderer/backend/vulkan/vk.h"  // IWYU pragma: keep.
@@ -156,6 +155,10 @@ void CommandPoolVK::Destroy() {
   collected_buffers_.clear();
 }
 
+// Associates a resource with a thread and context.
+using CommandPoolMap =
+    std::unordered_map<uint64_t, std::shared_ptr<CommandPoolVK>>;
+
 // CommandPoolVK Lifecycle:
 // 1. End of frame will reset the command pool (clearing this on a thread).
 //    There will still be references to the command pool from the uncompleted
@@ -166,47 +169,17 @@ void CommandPoolVK::Destroy() {
 //    available for reuse ("recycle").
 static thread_local std::unique_ptr<CommandPoolMap> tls_command_pool_map;
 
-struct WeakThreadLocalData {
-  std::weak_ptr<CommandPoolVK> command_pool;
-  std::weak_ptr<DescriptorPoolVK> descriptor_pool;
-};
-
 // Map each context to a list of all thread-local command pools associated
 // with that context.
 static Mutex g_all_pools_map_mutex;
-static std::unordered_map<const ContextVK*,
-                          std::vector<WeakThreadLocalData>> g_all_pools_map
+static std::unordered_map<
+    const ContextVK*,
+    std::vector<std::weak_ptr<CommandPoolVK>>> g_all_pools_map
     IPLR_GUARDED_BY(g_all_pools_map_mutex);
 
-std::shared_ptr<DescriptorPoolVK> CommandPoolRecyclerVK::GetDescriptorPool() {
-  const auto& strong_context = context_.lock();
-  if (!strong_context) {
-    return nullptr;
-  }
-
-  // If there is a resource in used for this thread and context, return it.
-  if (!tls_command_pool_map.get()) {
-    tls_command_pool_map.reset(new CommandPoolMap());
-  }
-  CommandPoolMap& pool_map = *tls_command_pool_map.get();
-  auto const hash = strong_context->GetHash();
-  auto const it = pool_map.find(hash);
-  if (it != pool_map.end()) {
-    return it->second.descriptor_pool;
-  }
-
-  const auto& result =
-      InitializeThreadLocalResources(strong_context, pool_map, hash);
-  if (result.has_value()) {
-    return result->descriptor_pool;
-  }
-
-  return nullptr;
-}
-
 // TODO(matanlurey): Return a status_or<> instead of nullptr when we have one.
 std::shared_ptr<CommandPoolVK> CommandPoolRecyclerVK::Get() {
-  const auto& strong_context = context_.lock();
+  auto const strong_context = context_.lock();
   if (!strong_context) {
     return nullptr;
   }
@@ -219,40 +192,25 @@ std::shared_ptr<CommandPoolVK> CommandPoolRecyclerVK::Get() {
   auto const hash = strong_context->GetHash();
   auto const it = pool_map.find(hash);
   if (it != pool_map.end()) {
-    return it->second.command_pool;
+    return it->second;
   }
 
-  const auto& result =
-      InitializeThreadLocalResources(strong_context, pool_map, hash);
-  if (result.has_value()) {
-    return result->command_pool;
-  }
-
-  return nullptr;
-}
-
-std::optional<ThreadLocalData>
-CommandPoolRecyclerVK::InitializeThreadLocalResources(
-    const std::shared_ptr<ContextVK>& context,
-    CommandPoolMap& pool_map,
-    uint64_t pool_key) {
+  // Otherwise, create a new resource and return it.
   auto data = Create();
   if (!data || !data->pool) {
-    return std::nullopt;
+    return nullptr;
   }
 
-  auto command_pool = std::make_shared<CommandPoolVK>(
+  auto const resource = std::make_shared<CommandPoolVK>(
       std::move(data->pool), std::move(data->buffers), context_);
-  auto descriptor_pool = std::make_shared<DescriptorPoolVK>(context_);
+  pool_map.emplace(hash, resource);
+
   {
     Lock all_pools_lock(g_all_pools_map_mutex);
-    g_all_pools_map[context.get()].push_back(WeakThreadLocalData{
-        .command_pool = command_pool, .descriptor_pool = descriptor_pool});
+    g_all_pools_map[strong_context.get()].push_back(resource);
   }
 
-  return pool_map[pool_key] =
-             ThreadLocalData{.command_pool = std::move(command_pool),
-                             .descriptor_pool = std::move(descriptor_pool)};
+  return resource;
 }
 
 // TODO(matanlurey): Return a status_or<> instead of nullopt when we have one.
@@ -335,13 +293,14 @@ void CommandPoolRecyclerVK::DestroyThreadLocalPools(const ContextVK* context) {
   Lock all_pools_lock(g_all_pools_map_mutex);
   auto found = g_all_pools_map.find(context);
   if (found != g_all_pools_map.end()) {
-    for (auto& [command_pool, desc_pool] : found->second) {
-      const auto& strong_pool = command_pool.lock();
-      if (strong_pool) {
-        // Delete all objects held by this pool.  The destroyed pool will still
-        // remain in its thread's TLS map until that thread exits.
-        strong_pool->Destroy();
+    for (auto& weak_pool : found->second) {
+      auto pool = weak_pool.lock();
+      if (!pool) {
+        continue;
       }
+      // Delete all objects held by this pool.  The destroyed pool will still
+      // remain in its thread's TLS map until that thread exits.
+      pool->Destroy();
     }
     g_all_pools_map.erase(found);
   }

impeller/renderer/backend/vulkan/command_pool_vk.h

@@ -10,7 +10,6 @@
 #include <utility>
 
 #include "impeller/base/thread.h"
-#include "impeller/renderer/backend/vulkan/descriptor_pool_vk.h"
 #include "impeller/renderer/backend/vulkan/vk.h"  // IWYU pragma: keep.
 #include "vulkan/vulkan_handles.hpp"
 
@@ -77,14 +76,6 @@ class CommandPoolVK final {
       pool_mutex_);
 };
 
-struct ThreadLocalData {
-  std::shared_ptr<CommandPoolVK> command_pool;
-  std::shared_ptr<DescriptorPoolVK> descriptor_pool;
-};
-
-// Associates a resource with a thread and context.
-using CommandPoolMap = std::unordered_map<uint64_t, ThreadLocalData>;
-
 //------------------------------------------------------------------------------
 /// @brief      Creates and manages the lifecycle of |vk::CommandPool| objects.
 ///
@@ -137,11 +128,6 @@ class CommandPoolRecyclerVK final
   /// @warning    Returns a |nullptr| if a pool could not be created.
   std::shared_ptr<CommandPoolVK> Get();
 
-  /// @brief      Gets a descriptor pool for the current thread.
-  ///
-  /// @warning    Returns a |nullptr| if a pool could not be created.
-  std::shared_ptr<DescriptorPoolVK> GetDescriptorPool();
-
   /// @brief      Returns a command pool to be reset on a background thread.
   ///
   /// @param[in]  pool The pool to recycler.
@@ -167,13 +153,6 @@ class CommandPoolRecyclerVK final
   /// @returns    Returns a |std::nullopt| if a pool was not available.
   std::optional<RecycledData> Reuse();
 
-  /// Create a DescriptorPoolVK and a CommandPoolVK and stash them in the TLS
-  /// map.
-  std::optional<ThreadLocalData> InitializeThreadLocalResources(
-      const std::shared_ptr<ContextVK>& context,
-      CommandPoolMap& pool_map,
-      uint64_t pool_key);
-
   CommandPoolRecyclerVK(const CommandPoolRecyclerVK&) = delete;
 
   CommandPoolRecyclerVK& operator=(const CommandPoolRecyclerVK&) = delete;

impeller/renderer/backend/vulkan/context_vk.cc

@@ -6,7 +6,6 @@
 
 #include "fml/concurrent_message_loop.h"
 #include "impeller/renderer/backend/vulkan/command_queue_vk.h"
-#include "impeller/renderer/backend/vulkan/descriptor_pool_vk.h"
 #include "impeller/renderer/backend/vulkan/render_pass_builder_vk.h"
 #include "impeller/renderer/render_target.h"
 
@@ -496,14 +495,9 @@ std::shared_ptr<CommandBuffer> ContextVK::CreateCommandBuffer() const {
   if (!tls_pool) {
     return nullptr;
   }
-  auto tls_desc_pool = recycler->GetDescriptorPool();
-  if (!tls_desc_pool) {
-    return nullptr;
-  }
 
   auto tracked_objects = std::make_shared<TrackedObjectsVK>(
-      weak_from_this(), std::move(tls_pool), tls_desc_pool,
-      GetGPUTracer()->CreateGPUProbe());
+      weak_from_this(), std::move(tls_pool), GetGPUTracer()->CreateGPUProbe());
   auto queue = GetGraphicsQueue();
 
   if (!tracked_objects || !tracked_objects->IsValid() || !queue) {
@@ -522,9 +516,10 @@ std::shared_ptr<CommandBuffer> ContextVK::CreateCommandBuffer() const {
       tracked_objects->GetCommandBuffer());
 
   return std::shared_ptr<CommandBufferVK>(new CommandBufferVK(
-      shared_from_this(),         //
-      GetDeviceHolder(),          //
-      std::move(tracked_objects)  //
+      shared_from_this(),          //
+      GetDeviceHolder(),           //
+      std::move(tracked_objects),  //
+      GetFenceWaiter()             //
       ));
 }
 

impeller/renderer/backend/vulkan/context_vk.h

@@ -10,9 +10,11 @@
 #include "flutter/fml/concurrent_message_loop.h"
 #include "flutter/fml/mapping.h"
 #include "flutter/fml/unique_fd.h"
+#include "fml/thread.h"
 #include "impeller/base/backend_cast.h"
 #include "impeller/base/strings.h"
 #include "impeller/core/formats.h"
+#include "impeller/renderer/backend/vulkan/command_pool_vk.h"
 #include "impeller/renderer/backend/vulkan/device_holder_vk.h"
 #include "impeller/renderer/backend/vulkan/driver_info_vk.h"
 #include "impeller/renderer/backend/vulkan/pipeline_library_vk.h"

impeller/renderer/backend/vulkan/descriptor_pool_vk_unittests.cc

@@ -5,7 +5,6 @@
 #include "flutter/testing/testing.h"  // IWYU pragma: keep.
 #include "fml/closure.h"
 #include "fml/synchronization/waitable_event.h"
-#include "impeller/renderer/backend/vulkan/command_buffer_vk.h"
 #include "impeller/renderer/backend/vulkan/descriptor_pool_vk.h"
 #include "impeller/renderer/backend/vulkan/resource_manager_vk.h"
 #include "impeller/renderer/backend/vulkan/test/mock_vulkan.h"
@@ -124,27 +123,5 @@ TEST(DescriptorPoolRecyclerVKTest, ReclaimDropsDescriptorPoolIfSizeIsExceeded) {
   context->Shutdown();
 }
 
-TEST(DescriptorPoolRecyclerVKTest, MultipleCommandBuffersShareDescriptorPool) {
-  auto const context = MockVulkanContextBuilder().Build();
-
-  auto cmd_buffer_1 = context->CreateCommandBuffer();
-  auto cmd_buffer_2 = context->CreateCommandBuffer();
-
-  CommandBufferVK& vk_1 = CommandBufferVK::Cast(*cmd_buffer_1);
-  CommandBufferVK& vk_2 = CommandBufferVK::Cast(*cmd_buffer_2);
-
-  EXPECT_EQ(&vk_1.GetDescriptorPool(), &vk_2.GetDescriptorPool());
-
-  // Resetting resources creates a new pool.
-  context->DisposeThreadLocalCachedResources();
-
-  auto cmd_buffer_3 = context->CreateCommandBuffer();
-  CommandBufferVK& vk_3 = CommandBufferVK::Cast(*cmd_buffer_3);
-
-  EXPECT_NE(&vk_1.GetDescriptorPool(), &vk_3.GetDescriptorPool());
-
-  context->Shutdown();
-}
-
 }  // namespace testing
 }  // namespace impeller

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

@@ -4,7 +4,6 @@
 
 #include "flutter/testing/testing.h"  // IWYU pragma: keep
 #include "gtest/gtest.h"
-#include "impeller/renderer/backend/vulkan/command_pool_vk.h"
 #include "impeller/renderer/backend/vulkan/test/mock_vulkan.h"
 #include "vulkan/vulkan_enums.hpp"
 

impeller/renderer/backend/vulkan/tracked_objects_vk.cc

@@ -4,17 +4,15 @@
 
 #include "impeller/renderer/backend/vulkan/tracked_objects_vk.h"
 
-#include "impeller/renderer/backend/vulkan/command_pool_vk.h"
 #include "impeller/renderer/backend/vulkan/gpu_tracer_vk.h"
 
 namespace impeller {
 
 TrackedObjectsVK::TrackedObjectsVK(
     const std::weak_ptr<const ContextVK>& context,
     const std::shared_ptr<CommandPoolVK>& pool,
-    std::shared_ptr<DescriptorPoolVK> descriptor_pool,
     std::unique_ptr<GPUProbe> probe)
-    : desc_pool_(std::move(descriptor_pool)), probe_(std::move(probe)) {
+    : desc_pool_(context), probe_(std::move(probe)) {
   if (!pool) {
     return;
   }
@@ -80,7 +78,7 @@ vk::CommandBuffer TrackedObjectsVK::GetCommandBuffer() const {
 }
 
 DescriptorPoolVK& TrackedObjectsVK::GetDescriptorPool() {
-  return *desc_pool_;
+  return desc_pool_;
 }
 
 GPUProbe& TrackedObjectsVK::GetGPUProbe() const {

impeller/renderer/backend/vulkan/tracked_objects_vk.h

@@ -14,15 +14,12 @@
 
 namespace impeller {
 
-class CommandPoolVK;
-
 /// @brief A per-frame object used to track resource lifetimes and allocate
 ///        command buffers and descriptor sets.
 class TrackedObjectsVK {
  public:
   explicit TrackedObjectsVK(const std::weak_ptr<const ContextVK>& context,
                             const std::shared_ptr<CommandPoolVK>& pool,
-                            std::shared_ptr<DescriptorPoolVK> descriptor_pool,
                             std::unique_ptr<GPUProbe> probe);
 
   ~TrackedObjectsVK();
@@ -46,7 +43,7 @@ class TrackedObjectsVK {
   GPUProbe& GetGPUProbe() const;
 
  private:
-  std::shared_ptr<DescriptorPoolVK> desc_pool_;
+  DescriptorPoolVK desc_pool_;
   // `shared_ptr` since command buffers have a link to the command pool.
   std::shared_ptr<CommandPoolVK> pool_;
   vk::UniqueCommandBuffer buffer_;