From 71010953aa96d2d02612f717073b44cb306bf078 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Thu, 13 Mar 2025 14:42:54 -0700 Subject: [PATCH] [Impeller] cache descriptor set layouts. (#164952) Currently we reset the entire descriptor pool(s) each frame. We can actually reuse the allocated descriptor sets, as that is safe to do as soon as the cmd buffer that references them has been submitted (AFAIK). This provides a minor speed up for a large amount of small drawing ops, but not much beyond that. --- .../backend/vulkan/command_buffer_vk.cc | 5 +- .../backend/vulkan/command_buffer_vk.h | 1 + .../backend/vulkan/compute_pass_vk.cc | 3 +- .../backend/vulkan/compute_pipeline_vk.cc | 10 +- .../backend/vulkan/compute_pipeline_vk.h | 8 +- .../renderer/backend/vulkan/context_vk.cc | 5 +- .../backend/vulkan/descriptor_pool_vk.cc | 119 ++++++++--------- .../backend/vulkan/descriptor_pool_vk.h | 37 ++++-- .../vulkan/descriptor_pool_vk_unittests.cc | 121 ++++++++---------- .../backend/vulkan/pipeline_library_vk.cc | 29 ++--- .../backend/vulkan/pipeline_library_vk.h | 9 +- .../renderer/backend/vulkan/pipeline_vk.cc | 9 +- .../renderer/backend/vulkan/pipeline_vk.h | 8 +- .../renderer/backend/vulkan/render_pass_vk.cc | 3 +- .../src/flutter/impeller/renderer/pipeline.h | 2 + 15 files changed, 188 insertions(+), 181 deletions(-) diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/command_buffer_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/command_buffer_vk.cc index 32080ca547a..0501f2717e1 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/command_buffer_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/command_buffer_vk.cc @@ -160,13 +160,14 @@ bool CommandBufferVK::Track(const std::shared_ptr& texture) { fml::StatusOr CommandBufferVK::AllocateDescriptorSets( const vk::DescriptorSetLayout& layout, + PipelineKey pipeline_key, const ContextVK& context) { if (!IsValid()) { return fml::Status(fml::StatusCode::kUnknown, "command encoder invalid"); } - return tracked_objects_->GetDescriptorPool().AllocateDescriptorSets(layout, - context); + return tracked_objects_->GetDescriptorPool().AllocateDescriptorSets( + layout, pipeline_key, context); } void CommandBufferVK::PushDebugGroup(std::string_view label) const { diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/command_buffer_vk.h b/engine/src/flutter/impeller/renderer/backend/vulkan/command_buffer_vk.h index 47ded4867d4..0b7c096dde9 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/command_buffer_vk.h +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/command_buffer_vk.h @@ -74,6 +74,7 @@ class CommandBufferVK final /// @brief Allocate a new descriptor set for the given [layout]. fml::StatusOr AllocateDescriptorSets( const vk::DescriptorSetLayout& layout, + PipelineKey pipeline_key, const ContextVK& context); // Visible for testing. diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/compute_pass_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/compute_pass_vk.cc index 46ee5fd09af..a82210c7bb8 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/compute_pass_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/compute_pass_vk.cc @@ -58,7 +58,8 @@ void ComputePassVK::SetPipeline( pipeline_layout_ = pipeline_vk.GetPipelineLayout(); auto descriptor_result = command_buffer_->AllocateDescriptorSets( - pipeline_vk.GetDescriptorSetLayout(), ContextVK::Cast(*context_)); + pipeline_vk.GetDescriptorSetLayout(), pipeline_vk.GetPipelineKey(), + ContextVK::Cast(*context_)); if (!descriptor_result.ok()) { return; } diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/compute_pipeline_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/compute_pipeline_vk.cc index a6806561873..f1823abb3b7 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/compute_pipeline_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/compute_pipeline_vk.cc @@ -12,12 +12,14 @@ ComputePipelineVK::ComputePipelineVK( const ComputePipelineDescriptor& desc, vk::UniquePipeline pipeline, vk::UniquePipelineLayout layout, - vk::UniqueDescriptorSetLayout descriptor_set_layout) + vk::UniqueDescriptorSetLayout descriptor_set_layout, + PipelineKey pipeline_key) : Pipeline(std::move(library), desc), device_holder_(std::move(device_holder)), pipeline_(std::move(pipeline)), layout_(std::move(layout)), - descriptor_set_layout_(std::move(descriptor_set_layout)) { + descriptor_set_layout_(std::move(descriptor_set_layout)), + pipeline_key_(pipeline_key) { is_valid_ = pipeline_ && layout_ && descriptor_set_layout_; } @@ -51,4 +53,8 @@ const vk::DescriptorSetLayout& ComputePipelineVK::GetDescriptorSetLayout() return *descriptor_set_layout_; } +PipelineKey ComputePipelineVK::GetPipelineKey() const { + return pipeline_key_; +} + } // namespace impeller diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/compute_pipeline_vk.h b/engine/src/flutter/impeller/renderer/backend/vulkan/compute_pipeline_vk.h index 1516d0eeb9f..839a8ffeb9d 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/compute_pipeline_vk.h +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/compute_pipeline_vk.h @@ -24,7 +24,8 @@ class ComputePipelineVK final const ComputePipelineDescriptor& desc, vk::UniquePipeline pipeline, vk::UniquePipelineLayout layout, - vk::UniqueDescriptorSetLayout descriptor_set_layout); + vk::UniqueDescriptorSetLayout descriptor_set_layout, + PipelineKey pipeline_key); // |Pipeline| ~ComputePipelineVK() override; @@ -35,6 +36,10 @@ class ComputePipelineVK final const vk::DescriptorSetLayout& GetDescriptorSetLayout() const; + /// @brief Retrieve the unique identifier for this pipeline's descriptor set + /// layout. + PipelineKey GetPipelineKey() const; + private: friend class PipelineLibraryVK; @@ -42,6 +47,7 @@ class ComputePipelineVK final vk::UniquePipeline pipeline_; vk::UniquePipelineLayout layout_; vk::UniqueDescriptorSetLayout descriptor_set_layout_; + const PipelineKey pipeline_key_; bool is_valid_ = false; // |Pipeline| diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/context_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/context_vk.cc index e135250d56a..fd3955bcce9 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/context_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/context_vk.cc @@ -546,9 +546,8 @@ std::shared_ptr ContextVK::CreateCommandBuffer() const { DescriptorPoolMap::iterator current_pool = cached_descriptor_pool_.find(std::this_thread::get_id()); if (current_pool == cached_descriptor_pool_.end()) { - descriptor_pool = - (cached_descriptor_pool_[std::this_thread::get_id()] = - std::make_shared(weak_from_this())); + descriptor_pool = (cached_descriptor_pool_[std::this_thread::get_id()] = + descriptor_pool_recycler_->GetDescriptorPool()); } else { descriptor_pool = current_pool->second; } diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk.cc index c7545ea9a15..44178962076 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk.cc @@ -7,9 +7,6 @@ #include #include "impeller/base/validation.h" -#include "impeller/renderer/backend/vulkan/resource_manager_vk.h" -#include "vulkan/vulkan_enums.hpp" -#include "vulkan/vulkan_handles.hpp" namespace impeller { @@ -29,44 +26,20 @@ static const constexpr DescriptorPoolSize kDefaultBindingSize = .subpass_bindings = 4u // Subpass Bindings }; -// Holds the command pool in a background thread, recyling it when not in use. -class BackgroundDescriptorPoolVK final { - public: - BackgroundDescriptorPoolVK(BackgroundDescriptorPoolVK&&) = default; - - explicit BackgroundDescriptorPoolVK( - vk::UniqueDescriptorPool&& pool, - std::weak_ptr recycler) - : pool_(std::move(pool)), recycler_(std::move(recycler)) {} - - ~BackgroundDescriptorPoolVK() { - auto const recycler = recycler_.lock(); - - // Not only does this prevent recycling when the context is being destroyed, - // but it also prevents the destructor from effectively being called twice; - // once for the original BackgroundCommandPoolVK() and once for the moved - // BackgroundCommandPoolVK(). - if (!recycler) { - return; - } - - recycler->Reclaim(std::move(pool_)); - } - - private: - BackgroundDescriptorPoolVK(const BackgroundDescriptorPoolVK&) = delete; - - BackgroundDescriptorPoolVK& operator=(const BackgroundDescriptorPoolVK&) = - delete; - - vk::UniqueDescriptorPool pool_; - uint32_t allocated_capacity_; - std::weak_ptr recycler_; -}; - DescriptorPoolVK::DescriptorPoolVK(std::weak_ptr context) : context_(std::move(context)) {} +void DescriptorPoolVK::Destroy() { + pools_.clear(); +} + +DescriptorPoolVK::DescriptorPoolVK(std::weak_ptr context, + DescriptorCacheMap descriptor_sets, + std::vector pools) + : context_(std::move(context)), + descriptor_sets_(std::move(descriptor_sets)), + pools_(std::move(pools)) {} + DescriptorPoolVK::~DescriptorPoolVK() { if (pools_.empty()) { return; @@ -81,19 +54,21 @@ DescriptorPoolVK::~DescriptorPoolVK() { return; } - for (auto i = 0u; i < pools_.size(); i++) { - auto reset_pool_when_dropped = - BackgroundDescriptorPoolVK(std::move(pools_[i]), recycler); - - UniqueResourceVKT pool( - context->GetResourceManager(), std::move(reset_pool_when_dropped)); - } - pools_.clear(); + recycler->Reclaim(std::move(descriptor_sets_), std::move(pools_)); } fml::StatusOr DescriptorPoolVK::AllocateDescriptorSets( const vk::DescriptorSetLayout& layout, + PipelineKey pipeline_key, const ContextVK& context_vk) { + DescriptorCacheMap::iterator existing = descriptor_sets_.find(pipeline_key); + if (existing != descriptor_sets_.end() && !existing->second.unused.empty()) { + auto descriptor_set = existing->second.unused.back(); + existing->second.unused.pop_back(); + existing->second.used.push_back(descriptor_set); + return descriptor_set; + } + if (pools_.empty()) { CreateNewPool(context_vk); } @@ -111,6 +86,9 @@ fml::StatusOr DescriptorPoolVK::AllocateDescriptorSets( set_info.setDescriptorPool(pools_.back().get()); result = context_vk.GetDevice().allocateDescriptorSets(&set_info, &set); } + auto lookup_result = + descriptor_sets_.try_emplace(pipeline_key, DescriptorCache{}); + lookup_result.first->second.used.push_back(set); if (result != vk::Result::eSuccess) { VALIDATION_LOG << "Could not allocate descriptor sets: " @@ -130,30 +108,35 @@ fml::Status DescriptorPoolVK::CreateNewPool(const ContextVK& context_vk) { return fml::Status(); } -void DescriptorPoolRecyclerVK::Reclaim(vk::UniqueDescriptorPool&& pool) { +void DescriptorPoolRecyclerVK::Reclaim( + DescriptorCacheMap descriptor_sets, + std::vector pools) { // Reset the pool on a background thread. auto strong_context = context_.lock(); if (!strong_context) { return; } - auto device = strong_context->GetDevice(); - device.resetDescriptorPool(pool.get()); - // Move the pool to the recycled list. - Lock recycled_lock(recycled_mutex_); - - if (recycled_.size() < kMaxRecycledPools) { - recycled_.push_back(std::move(pool)); - return; + for (auto& [_, cache] : descriptor_sets) { + cache.unused.insert(cache.unused.end(), cache.used.begin(), + cache.used.end()); + cache.used.clear(); } + + // Move the pool to the recycled list. If more than 32 pool are + // cached then delete the newest entry. + Lock recycled_lock(recycled_mutex_); + while (recycled_.size() >= kMaxRecycledPools) { + auto& back_entry = recycled_.back(); + back_entry->Destroy(); + recycled_.pop_back(); + } + recycled_.push_back(std::make_shared( + context_, std::move(descriptor_sets), std::move(pools))); } vk::UniqueDescriptorPool DescriptorPoolRecyclerVK::Get() { // Recycle a pool with a matching minumum capcity if it is available. - auto recycled_pool = Reuse(); - if (recycled_pool.has_value()) { - return std::move(recycled_pool.value()); - } return Create(); } @@ -187,15 +170,17 @@ vk::UniqueDescriptorPool DescriptorPoolRecyclerVK::Create() { return std::move(pool); } -std::optional DescriptorPoolRecyclerVK::Reuse() { - Lock lock(recycled_mutex_); - if (recycled_.empty()) { - return std::nullopt; +std::shared_ptr +DescriptorPoolRecyclerVK::GetDescriptorPool() { + { + Lock recycled_lock(recycled_mutex_); + if (!recycled_.empty()) { + auto result = recycled_.back(); + recycled_.pop_back(); + return result; + } } - - auto recycled = std::move(recycled_[recycled_.size() - 1]); - recycled_.pop_back(); - return recycled; + return std::make_shared(context_); } } // namespace impeller diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk.h b/engine/src/flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk.h index 53f35a94ab8..88a911ffc54 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk.h +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk.h @@ -6,13 +6,22 @@ #define FLUTTER_IMPELLER_RENDERER_BACKEND_VULKAN_DESCRIPTOR_POOL_VK_H_ #include +#include #include "fml/status_or.h" #include "impeller/renderer/backend/vulkan/context_vk.h" -#include "vulkan/vulkan_handles.hpp" +#include "impeller/renderer/pipeline.h" namespace impeller { +/// Used and un-used descriptor sets. +struct DescriptorCache { + std::vector unused; + std::vector used; +}; + +using DescriptorCacheMap = std::unordered_map; + //------------------------------------------------------------------------------ /// @brief A per-frame descriptor pool. Descriptors /// from this pool don't need to be freed individually. Instead, the @@ -28,16 +37,26 @@ class DescriptorPoolVK { public: explicit DescriptorPoolVK(std::weak_ptr context); + DescriptorPoolVK(std::weak_ptr context, + DescriptorCacheMap descriptor_sets, + std::vector pools); + ~DescriptorPoolVK(); fml::StatusOr AllocateDescriptorSets( const vk::DescriptorSetLayout& layout, + PipelineKey pipeline_key, const ContextVK& context_vk); private: + friend class DescriptorPoolRecyclerVK; + std::weak_ptr context_; + DescriptorCacheMap descriptor_sets_; std::vector pools_; + void Destroy(); + fml::Status CreateNewPool(const ContextVK& context_vk); DescriptorPoolVK(const DescriptorPoolVK&) = delete; @@ -68,17 +87,16 @@ class DescriptorPoolRecyclerVK final /// the necessary capacity. vk::UniqueDescriptorPool Get(); - /// @brief Returns the descriptor pool to be reset on a background - /// thread. - /// - /// @param[in] pool The pool to recycler. - void Reclaim(vk::UniqueDescriptorPool&& pool); + std::shared_ptr GetDescriptorPool(); + + void Reclaim(DescriptorCacheMap descriptor_sets, + std::vector pools); private: std::weak_ptr context_; Mutex recycled_mutex_; - std::vector recycled_ IPLR_GUARDED_BY( + std::vector> recycled_ IPLR_GUARDED_BY( recycled_mutex_); /// @brief Creates a new |vk::CommandPool|. @@ -86,11 +104,6 @@ class DescriptorPoolRecyclerVK final /// @returns Returns a |std::nullopt| if a pool could not be created. vk::UniqueDescriptorPool Create(); - /// @brief Reuses a recycled |vk::CommandPool|, if available. - /// - /// @returns Returns a |std::nullopt| if a pool was not available. - std::optional Reuse(); - DescriptorPoolRecyclerVK(const DescriptorPoolRecyclerVK&) = delete; DescriptorPoolRecyclerVK& operator=(const DescriptorPoolRecyclerVK&) = delete; diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk_unittests.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk_unittests.cc index fea47418dcd..843c2a68749 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk_unittests.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/descriptor_pool_vk_unittests.cc @@ -3,21 +3,18 @@ // found in the LICENSE file. #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" namespace impeller { namespace testing { TEST(DescriptorPoolRecyclerVKTest, GetDescriptorPoolRecyclerCreatesNewPools) { - auto const context = MockVulkanContextBuilder().Build(); + std::shared_ptr context = MockVulkanContextBuilder().Build(); - auto const pool1 = context->GetDescriptorPoolRecycler()->Get(); - auto const pool2 = context->GetDescriptorPoolRecycler()->Get(); + vk::UniqueDescriptorPool pool1 = context->GetDescriptorPoolRecycler()->Get(); + vk::UniqueDescriptorPool pool2 = context->GetDescriptorPoolRecycler()->Get(); // The two descriptor pools should be different. EXPECT_NE(pool1.get(), pool2.get()); @@ -26,37 +23,20 @@ TEST(DescriptorPoolRecyclerVKTest, GetDescriptorPoolRecyclerCreatesNewPools) { } TEST(DescriptorPoolRecyclerVKTest, ReclaimMakesDescriptorPoolAvailable) { - auto const context = MockVulkanContextBuilder().Build(); + std::shared_ptr context = MockVulkanContextBuilder().Build(); { // Fetch a pool (which will be created). - auto pool = DescriptorPoolVK(context); - pool.AllocateDescriptorSets({}, *context); + DescriptorPoolVK pool = DescriptorPoolVK(context); + pool.AllocateDescriptorSets({}, /*pipeline_key=*/0, *context); } - // There is a chance that the first death rattle item below is destroyed in - // the same reclaim cycle as the pool allocation above. These items are placed - // into a std::vector and free'd, which may free in reverse order. That would - // imply that the death rattle and subsequent waitable event fires before the - // pool is reset. To work around this, we can either manually remove items - // from the vector or use two death rattles. - for (auto i = 0u; i < 2; i++) { - // Add something to the resource manager and have it notify us when it's - // destroyed. That should give us a non-flaky signal that the pool has been - // reclaimed as well. - auto waiter = fml::AutoResetWaitableEvent(); - auto rattle = fml::ScopedCleanupClosure([&waiter]() { waiter.Signal(); }); - { - UniqueResourceVKT resource( - context->GetResourceManager(), std::move(rattle)); - } - waiter.Wait(); - } - - auto const pool = context->GetDescriptorPoolRecycler()->Get(); + std::shared_ptr pool = + context->GetDescriptorPoolRecycler()->GetDescriptorPool(); // Now check that we only ever created one pool. - auto const called = GetMockVulkanFunctions(context->GetDevice()); + std::shared_ptr> called = + GetMockVulkanFunctions(context->GetDevice()); EXPECT_EQ( std::count(called->begin(), called->end(), "vkCreateDescriptorPool"), 1u); @@ -64,58 +44,39 @@ TEST(DescriptorPoolRecyclerVKTest, ReclaimMakesDescriptorPoolAvailable) { } TEST(DescriptorPoolRecyclerVKTest, ReclaimDropsDescriptorPoolIfSizeIsExceeded) { - auto const context = MockVulkanContextBuilder().Build(); + std::shared_ptr context = MockVulkanContextBuilder().Build(); // Create 33 pools { std::vector> pools; - for (auto i = 0u; i < 33; i++) { - auto pool = std::make_unique(context); - pool->AllocateDescriptorSets({}, *context); + for (size_t i = 0u; i < 33; i++) { + std::unique_ptr pool = + std::make_unique(context); + pool->AllocateDescriptorSets({}, /*pipeline_key=*/0, *context); pools.push_back(std::move(pool)); } } - // See note above. - for (auto i = 0u; i < 2; i++) { - auto waiter = fml::AutoResetWaitableEvent(); - auto rattle = fml::ScopedCleanupClosure([&waiter]() { waiter.Signal(); }); - { - UniqueResourceVKT resource( - context->GetResourceManager(), std::move(rattle)); - } - waiter.Wait(); - } - - auto const called = GetMockVulkanFunctions(context->GetDevice()); + std::shared_ptr> called = + GetMockVulkanFunctions(context->GetDevice()); EXPECT_EQ( std::count(called->begin(), called->end(), "vkCreateDescriptorPool"), 33u); - EXPECT_EQ(std::count(called->begin(), called->end(), "vkResetDescriptorPool"), - 33u); // Now create 33 more descriptor pools and observe that only one more is // allocated. { - std::vector> pools; - for (auto i = 0u; i < 33; i++) { - auto pool = std::make_unique(context); - pool->AllocateDescriptorSets({}, *context); + std::vector> pools; + for (size_t i = 0u; i < 33; i++) { + std::shared_ptr pool = + context->GetDescriptorPoolRecycler()->GetDescriptorPool(); + pool->AllocateDescriptorSets({}, /*pipeline_key=*/0, *context); pools.push_back(std::move(pool)); } } - for (auto i = 0u; i < 2; i++) { - auto waiter = fml::AutoResetWaitableEvent(); - auto rattle = fml::ScopedCleanupClosure([&waiter]() { waiter.Signal(); }); - { - UniqueResourceVKT resource( - context->GetResourceManager(), std::move(rattle)); - } - waiter.Wait(); - } - - auto const called_twice = GetMockVulkanFunctions(context->GetDevice()); + std::shared_ptr> called_twice = + GetMockVulkanFunctions(context->GetDevice()); // 32 of the descriptor pools were recycled, so only one more is created. EXPECT_EQ( std::count(called->begin(), called->end(), "vkCreateDescriptorPool"), @@ -125,10 +86,10 @@ TEST(DescriptorPoolRecyclerVKTest, ReclaimDropsDescriptorPoolIfSizeIsExceeded) { } TEST(DescriptorPoolRecyclerVKTest, MultipleCommandBuffersShareDescriptorPool) { - auto const context = MockVulkanContextBuilder().Build(); + std::shared_ptr context = MockVulkanContextBuilder().Build(); - auto cmd_buffer_1 = context->CreateCommandBuffer(); - auto cmd_buffer_2 = context->CreateCommandBuffer(); + std::shared_ptr cmd_buffer_1 = context->CreateCommandBuffer(); + std::shared_ptr cmd_buffer_2 = context->CreateCommandBuffer(); CommandBufferVK& vk_1 = CommandBufferVK::Cast(*cmd_buffer_1); CommandBufferVK& vk_2 = CommandBufferVK::Cast(*cmd_buffer_2); @@ -138,7 +99,7 @@ TEST(DescriptorPoolRecyclerVKTest, MultipleCommandBuffersShareDescriptorPool) { // Resetting resources creates a new pool. context->DisposeThreadLocalCachedResources(); - auto cmd_buffer_3 = context->CreateCommandBuffer(); + std::shared_ptr cmd_buffer_3 = context->CreateCommandBuffer(); CommandBufferVK& vk_3 = CommandBufferVK::Cast(*cmd_buffer_3); EXPECT_NE(&vk_1.GetDescriptorPool(), &vk_3.GetDescriptorPool()); @@ -146,5 +107,31 @@ TEST(DescriptorPoolRecyclerVKTest, MultipleCommandBuffersShareDescriptorPool) { context->Shutdown(); } +TEST(DescriptorPoolRecyclerVKTest, DescriptorsAreRecycled) { + std::shared_ptr context = MockVulkanContextBuilder().Build(); + + { + DescriptorPoolVK pool = DescriptorPoolVK(context); + pool.AllocateDescriptorSets({}, /*pipeline_key=*/0, *context); + } + + // Should reuse the same descriptor set allocated above. + std::shared_ptr pool = + context->GetDescriptorPoolRecycler()->GetDescriptorPool(); + pool->AllocateDescriptorSets({}, /*pipeline_key=*/0, *context); + + std::shared_ptr> called = + GetMockVulkanFunctions(context->GetDevice()); + EXPECT_EQ( + std::count(called->begin(), called->end(), "vkAllocateDescriptorSets"), + 1); + + // Should allocate a new descriptor set. + pool->AllocateDescriptorSets({}, /*pipeline_key=*/0, *context); + EXPECT_EQ( + std::count(called->begin(), called->end(), "vkAllocateDescriptorSets"), + 2); +} + } // namespace testing } // namespace impeller diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_library_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_library_vk.cc index daa57a37139..109b4064675 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_library_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_library_vk.cc @@ -4,23 +4,16 @@ #include "impeller/renderer/backend/vulkan/pipeline_library_vk.h" -#include #include -#include -#include #include "flutter/fml/container.h" #include "flutter/fml/trace_event.h" #include "impeller/base/promise.h" -#include "impeller/base/timing.h" #include "impeller/base/validation.h" #include "impeller/renderer/backend/vulkan/context_vk.h" #include "impeller/renderer/backend/vulkan/formats_vk.h" #include "impeller/renderer/backend/vulkan/pipeline_vk.h" #include "impeller/renderer/backend/vulkan/shader_function_vk.h" -#include "impeller/renderer/backend/vulkan/vertex_descriptor_vk.h" -#include "vulkan/vulkan_core.h" -#include "vulkan/vulkan_enums.hpp" namespace impeller { @@ -50,7 +43,8 @@ bool PipelineLibraryVK::IsValid() const { } std::unique_ptr PipelineLibraryVK::CreateComputePipeline( - const ComputePipelineDescriptor& desc) { + const ComputePipelineDescriptor& desc, + PipelineKey pipeline_key) { TRACE_EVENT0("flutter", __FUNCTION__); vk::ComputePipelineCreateInfo pipeline_info; @@ -151,8 +145,8 @@ std::unique_ptr PipelineLibraryVK::CreateComputePipeline( desc, // std::move(pipeline), // std::move(pipeline_layout.value), // - std::move(descs_layout) // - ); + std::move(descs_layout), // + pipeline_key); } // |PipelineLibrary| @@ -179,7 +173,8 @@ PipelineFuture PipelineLibraryVK::GetPipeline( auto weak_this = weak_from_this(); - auto generation_task = [descriptor, weak_this, promise]() { + PipelineKey next_key = pipeline_key_++; + auto generation_task = [descriptor, weak_this, promise, next_key]() { auto thiz = weak_this.lock(); if (!thiz) { promise->set_value(nullptr); @@ -191,7 +186,8 @@ PipelineFuture PipelineLibraryVK::GetPipeline( promise->set_value(PipelineVK::Create( descriptor, // PipelineLibraryVK::Cast(*thiz).device_holder_.lock(), // - weak_this // + weak_this, // + next_key // )); }; @@ -208,7 +204,7 @@ PipelineFuture PipelineLibraryVK::GetPipeline( PipelineFuture PipelineLibraryVK::GetPipeline( ComputePipelineDescriptor descriptor, bool async) { - Lock lock(compute_pipelines_mutex_); + Lock lock(pipelines_mutex_); if (auto found = compute_pipelines_.find(descriptor); found != compute_pipelines_.end()) { return found->second; @@ -230,7 +226,8 @@ PipelineFuture PipelineLibraryVK::GetPipeline( auto weak_this = weak_from_this(); - auto generation_task = [descriptor, weak_this, promise]() { + PipelineKey next_key = pipeline_key_++; + auto generation_task = [descriptor, weak_this, promise, next_key]() { auto self = weak_this.lock(); if (!self) { promise->set_value(nullptr); @@ -239,8 +236,8 @@ PipelineFuture PipelineLibraryVK::GetPipeline( return; } - auto pipeline = - PipelineLibraryVK::Cast(*self).CreateComputePipeline(descriptor); + auto pipeline = PipelineLibraryVK::Cast(*self).CreateComputePipeline( + descriptor, next_key); if (!pipeline) { promise->set_value(nullptr); VALIDATION_LOG << "Could not create pipeline: " << descriptor.GetLabel(); diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_library_vk.h b/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_library_vk.h index 4396cdcab86..2c2eac667b9 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_library_vk.h +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_library_vk.h @@ -15,6 +15,7 @@ #include "impeller/renderer/backend/vulkan/pipeline_cache_vk.h" #include "impeller/renderer/backend/vulkan/pipeline_vk.h" #include "impeller/renderer/backend/vulkan/vk.h" +#include "impeller/renderer/pipeline.h" #include "impeller/renderer/pipeline_library.h" namespace impeller { @@ -42,10 +43,9 @@ class PipelineLibraryVK final std::shared_ptr worker_task_runner_; Mutex pipelines_mutex_; PipelineMap pipelines_ IPLR_GUARDED_BY(pipelines_mutex_); - Mutex compute_pipelines_mutex_; - ComputePipelineMap compute_pipelines_ IPLR_GUARDED_BY( - compute_pipelines_mutex_); + ComputePipelineMap compute_pipelines_ IPLR_GUARDED_BY(pipelines_mutex_); std::atomic_size_t frames_acquired_ = 0u; + PipelineKey pipeline_key_ IPLR_GUARDED_BY(pipelines_mutex_) = 1; bool is_valid_ = false; bool cache_dirty_ = false; @@ -75,7 +75,8 @@ class PipelineLibraryVK final std::shared_ptr function) override; std::unique_ptr CreateComputePipeline( - const ComputePipelineDescriptor& desc); + const ComputePipelineDescriptor& desc, + PipelineKey pipeline_key); void PersistPipelineCacheToDisk(); diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_vk.cc index 618f3334888..55cd4be5ad8 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_vk.cc @@ -465,6 +465,7 @@ std::unique_ptr PipelineVK::Create( const PipelineDescriptor& desc, const std::shared_ptr& device_holder, const std::weak_ptr& weak_library, + PipelineKey pipeline_key, std::shared_ptr immutable_sampler) { TRACE_EVENT1("flutter", "PipelineVK::Create", "Name", desc.GetLabel().data()); @@ -510,6 +511,7 @@ std::unique_ptr PipelineVK::Create( std::move(render_pass), // std::move(pipeline_layout.value()), // std::move(descs_layout.value()), // + pipeline_key, // std::move(immutable_sampler) // )); if (!pipeline_vk->IsValid()) { @@ -526,6 +528,7 @@ PipelineVK::PipelineVK(std::weak_ptr device_holder, vk::UniqueRenderPass render_pass, vk::UniquePipelineLayout layout, vk::UniqueDescriptorSetLayout descriptor_set_layout, + PipelineKey pipeline_key, std::shared_ptr immutable_sampler) : Pipeline(std::move(library), desc), device_holder_(std::move(device_holder)), @@ -533,7 +536,8 @@ PipelineVK::PipelineVK(std::weak_ptr device_holder, render_pass_(std::move(render_pass)), layout_(std::move(layout)), descriptor_set_layout_(std::move(descriptor_set_layout)), - immutable_sampler_(std::move(immutable_sampler)) { + immutable_sampler_(std::move(immutable_sampler)), + pipeline_key_(pipeline_key) { is_valid_ = pipeline_ && render_pass_ && layout_ && descriptor_set_layout_; } @@ -578,7 +582,8 @@ std::shared_ptr PipelineVK::CreateVariantForImmutableSamplers( return nullptr; } return (immutable_sampler_variants_[cache_key] = - Create(desc_, device_holder, library_, immutable_sampler)); + Create(desc_, device_holder, library_, pipeline_key_, + immutable_sampler)); } } // namespace impeller diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_vk.h b/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_vk.h index 5ab4feccfde..8ef19546f94 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_vk.h +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/pipeline_vk.h @@ -10,10 +10,7 @@ #include "impeller/base/backend_cast.h" #include "impeller/base/thread.h" -#include "impeller/core/texture.h" #include "impeller/renderer/backend/vulkan/device_holder_vk.h" -#include "impeller/renderer/backend/vulkan/formats_vk.h" -#include "impeller/renderer/backend/vulkan/pipeline_cache_vk.h" #include "impeller/renderer/backend/vulkan/sampler_vk.h" #include "impeller/renderer/backend/vulkan/vk.h" #include "impeller/renderer/backend/vulkan/yuv_conversion_vk.h" @@ -33,6 +30,7 @@ class PipelineVK final const PipelineDescriptor& desc, const std::shared_ptr& device_holder, const std::weak_ptr& weak_library, + PipelineKey pipeline_key, std::shared_ptr immutable_sampler = {}); // |Pipeline| @@ -47,6 +45,8 @@ class PipelineVK final std::shared_ptr CreateVariantForImmutableSamplers( const std::shared_ptr& immutable_sampler) const; + PipelineKey GetPipelineKey() const { return pipeline_key_; } + private: friend class PipelineLibraryVK; @@ -62,6 +62,7 @@ class PipelineVK final vk::UniquePipelineLayout layout_; vk::UniqueDescriptorSetLayout descriptor_set_layout_; std::shared_ptr immutable_sampler_; + const PipelineKey pipeline_key_; mutable Mutex immutable_sampler_variants_mutex_; mutable ImmutableSamplerVariants immutable_sampler_variants_ IPLR_GUARDED_BY( immutable_sampler_variants_mutex_); @@ -74,6 +75,7 @@ class PipelineVK final vk::UniqueRenderPass render_pass, vk::UniquePipelineLayout layout, vk::UniqueDescriptorSetLayout descriptor_set_layout, + PipelineKey pipeline_key, std::shared_ptr immutable_sampler); // |Pipeline| diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_vk.cc index a770765a79b..12bc86b14f9 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/render_pass_vk.cc @@ -489,7 +489,8 @@ fml::Status RenderPassVK::Draw() { const auto& pipeline_vk = PipelineVK::Cast(*pipeline_); auto descriptor_result = command_buffer_->AllocateDescriptorSets( - pipeline_vk.GetDescriptorSetLayout(), context_vk); + pipeline_vk.GetDescriptorSetLayout(), pipeline_vk.GetPipelineKey(), + context_vk); if (!descriptor_result.ok()) { return fml::Status(fml::StatusCode::kAborted, "Could not allocate descriptor sets."); diff --git a/engine/src/flutter/impeller/renderer/pipeline.h b/engine/src/flutter/impeller/renderer/pipeline.h index 5c191c19b5f..a6bbd982aea 100644 --- a/engine/src/flutter/impeller/renderer/pipeline.h +++ b/engine/src/flutter/impeller/renderer/pipeline.h @@ -18,6 +18,8 @@ namespace impeller { +using PipelineKey = uint64_t; + class PipelineLibrary; template class Pipeline;