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

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