From bf2238ae315e598f616504c0977ab9b6d3866f18 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 8 Sep 2023 11:49:28 -0700 Subject: [PATCH 01/26] WIP for discussion. --- .../backend/vulkan/command_pool_vk.cc | 6 +++ .../renderer/backend/vulkan/command_pool_vk.h | 14 +++++ .../renderer/backend/vulkan/context_vk.cc | 52 +++++++++++++++++++ impeller/renderer/backend/vulkan/context_vk.h | 18 +++++++ .../backend/vulkan/surface_context_vk.cc | 2 + 5 files changed, 92 insertions(+) diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.cc b/impeller/renderer/backend/vulkan/command_pool_vk.cc index ad657e3dac60d..7892c21c2af1a 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.cc +++ b/impeller/renderer/backend/vulkan/command_pool_vk.cc @@ -67,6 +67,12 @@ void CommandPoolVK::ClearAllPools(const ContextVK* context) { } } +void CommandPoolVK::ReleaseThreadLocalPool(const ContextVK* context) { + if (tls_command_pool.get()) { + tls_command_pool.get()->erase(context->GetHash()); + } +} + CommandPoolVK::CommandPoolVK(const ContextVK* context) : owner_id_(std::this_thread::get_id()) { vk::CommandPoolCreateInfo pool_info; diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.h b/impeller/renderer/backend/vulkan/command_pool_vk.h index 44722dba4b895..cb077da4c1a80 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.h +++ b/impeller/renderer/backend/vulkan/command_pool_vk.h @@ -51,6 +51,9 @@ class CommandPoolVK final { /// @note Should only be called when the |ContextVK| is being destroyed. static void ClearAllPools(const ContextVK* context); + /// @brief Releases references to thread-local command pools. + static void ReleaseThreadLocalPool(const ContextVK* context); + ~CommandPoolVK(); /// @brief Whether or not this |CommandPoolVK| is valid. @@ -82,6 +85,8 @@ class CommandPoolVK final { void CollectGraphicsCommandBuffer(vk::UniqueCommandBuffer buffer); private: + friend class ContextVK; + const std::thread::id owner_id_; std::weak_ptr device_holder_; vk::UniqueCommandPool graphics_pool_; @@ -98,6 +103,15 @@ class CommandPoolVK final { explicit CommandPoolVK(const ContextVK* context); + explicit CommandPoolVK(const std::thread::id& owner_id, + std::weak_ptr device_holder, + vk::UniqueCommandPool graphics_pool, + bool is_valid) + : owner_id_(owner_id), + device_holder_(std::move(device_holder)), + graphics_pool_(std::move(graphics_pool)), + is_valid_(is_valid) {} + /// @brief Collects buffers for recycling if able. /// /// If any buffers have been returned through |CollectGraphicsCommandBuffer|, diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index 875f770637bea..1160f24f215ea 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "impeller/renderer/backend/vulkan/context_vk.h" +#include "fml/macros.h" #ifdef FML_OS_ANDROID #include @@ -515,4 +516,55 @@ ContextVK::CreateGraphicsCommandEncoderFactory() const { return std::make_unique(weak_from_this()); } +std::optional ContextVK::CreateCommandPool() { + vk::CommandPoolCreateInfo pool_info; + + pool_info.queueFamilyIndex = GetGraphicsQueue()->GetIndex().family; + pool_info.flags = vk::CommandPoolCreateFlagBits::eTransient | + vk::CommandPoolCreateFlagBits::eResetCommandBuffer; + auto pool = GetDevice().createCommandPoolUnique(pool_info); + + if (pool.result != vk::Result::eSuccess) { + return std::nullopt; + } + + return std::move(pool.value); +} + +void ContextVK::RecycleCommandPool(vk::UniqueCommandPool pool) { + Lock lock(recycled_command_pools_mutex_); + recycled_command_pools_.push_back(std::move(pool)); +} + +std::optional ContextVK::ReuseCommandPool() { + Lock lock(recycled_command_pools_mutex_); + if (recycled_command_pools_.empty()) { + return std::nullopt; + } + auto pool = std::move(recycled_command_pools_.back()); + recycled_command_pools_.pop_back(); + return std::move(pool); +} + +std::optional ContextVK::GetCommandPool() { + vk::UniqueCommandPool pool; + + // First try reusing, then try creating. + if (auto reuse_pool = ReuseCommandPool()) { + pool = std::move(reuse_pool.value()); + } else if (auto create_pool = CreateCommandPool()) { + pool = std::move(create_pool.value()); + } else { + return std::nullopt; + } + + // FIXME: Resource handling: + // 1. Wrap pool in a class that has a reference to this ContextVK. + // (Q: A refrence to the pool? Ptr? Not sure yet) + // 2. In the destructor of that resource class, call: + // context_->RecycleCommandPool(std::move(pool)); (or similar) + + return std::move(pool); +} + } // namespace impeller diff --git a/impeller/renderer/backend/vulkan/context_vk.h b/impeller/renderer/backend/vulkan/context_vk.h index 316cf1cbb2161..fab228d3c5b8b 100644 --- a/impeller/renderer/backend/vulkan/context_vk.h +++ b/impeller/renderer/backend/vulkan/context_vk.h @@ -5,11 +5,13 @@ #pragma once #include +#include #include "flutter/fml/concurrent_message_loop.h" #include "flutter/fml/macros.h" #include "flutter/fml/mapping.h" #include "flutter/fml/unique_fd.h" +#include "fml/status.h" #include "impeller/base/backend_cast.h" #include "impeller/core/formats.h" #include "impeller/renderer/backend/vulkan/device_holder.h" @@ -140,6 +142,16 @@ class ContextVK final : public Context, std::shared_ptr GetResourceManager() const; + /// @brief Returns a |vk::UniqueCommandPool|, or |std::nullopt| if the + /// pool could not be created. + /// + /// If able, this method will reuse any |vk::CommandBuffer|s that have reset, + /// otherwise a new pool will be created. + /// + /// When all references to the returned |vk::UniqueCommandPool| are released, + /// the pool will automatically be sent to a background thread for recycling. + std::optional GetCommandPool(); + private: struct DeviceHolderImpl : public DeviceHolder { // |DeviceHolder| @@ -169,6 +181,12 @@ class ContextVK final : public Context, bool sync_presentation_ = false; const uint64_t hash_; + std::vector recycled_command_pools_; + Mutex recycled_command_pools_mutex_; + std::optional CreateCommandPool(); + void RecycleCommandPool(vk::UniqueCommandPool pool); + std::optional ReuseCommandPool(); + bool is_valid_ = false; ContextVK(); diff --git a/impeller/renderer/backend/vulkan/surface_context_vk.cc b/impeller/renderer/backend/vulkan/surface_context_vk.cc index 083d02c2b5ac9..6c3695db23f35 100644 --- a/impeller/renderer/backend/vulkan/surface_context_vk.cc +++ b/impeller/renderer/backend/vulkan/surface_context_vk.cc @@ -5,6 +5,7 @@ #include "impeller/renderer/backend/vulkan/surface_context_vk.h" #include "flutter/fml/trace_event.h" +#include "impeller/renderer/backend/vulkan/command_pool_vk.h" #include "impeller/renderer/backend/vulkan/context_vk.h" #include "impeller/renderer/backend/vulkan/swapchain_vk.h" @@ -78,6 +79,7 @@ std::unique_ptr SurfaceContextVK::AcquireNextSurface() { if (allocator) { allocator->DidAcquireSurfaceFrame(); } + CommandPoolVK::ReleaseThreadLocalPool(parent_.get()); return surface; } From 4f8d1437c77b70b7c2f4340dc2651b54ed786663 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 8 Sep 2023 15:32:13 -0700 Subject: [PATCH 02/26] The hackiest hacks that ever hacked, but it works. --- .../backend/vulkan/command_pool_vk.cc | 216 ++++++++++-------- .../renderer/backend/vulkan/command_pool_vk.h | 67 ++++-- .../renderer/backend/vulkan/context_vk.cc | 28 +-- impeller/renderer/backend/vulkan/context_vk.h | 14 +- .../backend/vulkan/surface_context_vk.cc | 2 +- 5 files changed, 191 insertions(+), 136 deletions(-) diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.cc b/impeller/renderer/backend/vulkan/command_pool_vk.cc index 7892c21c2af1a..20f336e16adee 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.cc +++ b/impeller/renderer/backend/vulkan/command_pool_vk.cc @@ -5,22 +5,33 @@ #include "impeller/renderer/backend/vulkan/command_pool_vk.h" #include +#include #include #include #include "flutter/fml/thread_local.h" #include "impeller/base/thread.h" #include "impeller/renderer/backend/vulkan/context_vk.h" +#include "impeller/renderer/backend/vulkan/resource_manager_vk.h" namespace impeller { using CommandPoolMap = std::map>; +// NOTE: How TF do we reset multiple threads? FML_THREAD_LOCAL fml::ThreadLocalUniquePtr tls_command_pool; -static Mutex g_all_pools_mutex; -static std::unordered_map>> - g_all_pools IPLR_GUARDED_BY(g_all_pools_mutex); +// static Mutex g_all_pools_mutex; +// static std::unordered_map>> +// g_all_pools IPLR_GUARDED_BY(g_all_pools_mutex); + +void CommandPoolVK::ReleaseAllPools(const ContextVK* context) { + if (tls_command_pool.get() == nullptr) { + return; + } + CommandPoolMap& pool_map = *tls_command_pool.get(); + pool_map.clear(); +} std::shared_ptr CommandPoolVK::GetThreadLocal( const ContextVK* context) { @@ -35,140 +46,159 @@ std::shared_ptr CommandPoolVK::GetThreadLocal( if (found != pool_map.end() && found->second->IsValid()) { return found->second; } - auto pool = std::shared_ptr(new CommandPoolVK(context)); - if (!pool->IsValid()) { + auto pool = CommandPoolVK::FromContext(context); + if (!pool || !pool->IsValid()) { return nullptr; } pool_map[context->GetHash()] = pool; - { - Lock pool_lock(g_all_pools_mutex); - g_all_pools[context].push_back(pool); - } + // { + // Lock pool_lock(g_all_pools_mutex); + // g_all_pools[context].push_back(pool); + // } return pool; } void CommandPoolVK::ClearAllPools(const ContextVK* context) { - if (tls_command_pool.get()) { - tls_command_pool.get()->erase(context->GetHash()); - } - Lock pool_lock(g_all_pools_mutex); - if (auto found = g_all_pools.find(context); found != g_all_pools.end()) { - for (auto& weak_pool : found->second) { - auto pool = weak_pool.lock(); - if (!pool) { - // The pool has already died because the thread died. - continue; - } - // The pool is reset but its reference in the TLS map remains till the - // thread dies. - pool->Reset(); - } - g_all_pools.erase(found); - } + // if (tls_command_pool.get()) { + // tls_command_pool.get()->erase(context->GetHash()); + // } + // Lock pool_lock(g_all_pools_mutex); + // if (auto found = g_all_pools.find(context); found != g_all_pools.end()) { + // for (auto& weak_pool : found->second) { + // auto pool = weak_pool.lock(); + // if (!pool) { + // // The pool has already died because the thread died. + // continue; + // } + // // The pool is reset but its reference in the TLS map remains till the + // // thread dies. + // pool->Reset(); + // } + // g_all_pools.erase(found); + // } } -void CommandPoolVK::ReleaseThreadLocalPool(const ContextVK* context) { - if (tls_command_pool.get()) { - tls_command_pool.get()->erase(context->GetHash()); +std::shared_ptr CommandPoolVK::FromContext( + const ContextVK* context) { + auto pool = context->GetCommandPool(); + if (!pool) { + return nullptr; } + + return std::make_shared(CommandPoolVK( + std::this_thread::get_id(), context->weak_from_this(), std::move(*pool))); +} + +CommandPoolVK::CommandPoolVK(std::thread::id thread_id, + std::weak_ptr context, + ManagedCommandPoolVK graphics_pool) + : owner_id_(thread_id), + context_(std::move(context)), + graphics_pool_(std::move(graphics_pool)) { + // FIXME: Not always true. + is_valid_ = true; } -CommandPoolVK::CommandPoolVK(const ContextVK* context) - : owner_id_(std::this_thread::get_id()) { - vk::CommandPoolCreateInfo pool_info; +struct JustFuckingCompile { + ManagedCommandPoolVK pool; + std::vector buffers; +}; - pool_info.queueFamilyIndex = context->GetGraphicsQueue()->GetIndex().family; - pool_info.flags = vk::CommandPoolCreateFlagBits::eTransient | - vk::CommandPoolCreateFlagBits::eResetCommandBuffer; - auto pool = context->GetDevice().createCommandPoolUnique(pool_info); - if (pool.result != vk::Result::eSuccess) { +CommandPoolVK::~CommandPoolVK() { + auto strong_context = context_.lock(); + if (!strong_context) { return; } - - device_holder_ = context->GetDeviceHolder(); - graphics_pool_ = std::move(pool.value); - is_valid_ = true; + auto pool = std::move(graphics_pool_); + UniqueResourceVKT( + strong_context->GetResourceManager(), + JustFuckingCompile{.pool = std::move(pool), + .buffers = std::move(recycled_buffers_)}); } -CommandPoolVK::~CommandPoolVK() = default; - bool CommandPoolVK::IsValid() const { return is_valid_; } void CommandPoolVK::Reset() { - { - Lock lock(buffers_to_collect_mutex_); - graphics_pool_.reset(); - - // When the command pool is destroyed, all of its command buffers are freed. - // Handles allocated from that pool are now invalid and must be discarded. - for (vk::UniqueCommandBuffer& buffer : buffers_to_collect_) { - buffer.release(); - } - buffers_to_collect_.clear(); - } - - for (vk::UniqueCommandBuffer& buffer : recycled_buffers_) { - buffer.release(); - } - recycled_buffers_.clear(); - - is_valid_ = false; + // { + // Lock lock(buffers_to_collect_mutex_); + // graphics_pool_.reset(); + + // // When the command pool is destroyed, all of its command buffers are + // freed. + // // Handles allocated from that pool are now invalid and must be + // discarded. for (vk::UniqueCommandBuffer& buffer : buffers_to_collect_) + // { + // buffer.release(); + // } + // buffers_to_collect_.clear(); + // } + + // for (vk::UniqueCommandBuffer& buffer : recycled_buffers_) { + // buffer.release(); + // } + // recycled_buffers_.clear(); + + // is_valid_ = false; } vk::UniqueCommandBuffer CommandPoolVK::CreateGraphicsCommandBuffer() { - std::shared_ptr strong_device = device_holder_.lock(); - if (!strong_device) { + auto strong_context = context_.lock(); + if (!strong_context) { return {}; } FML_DCHECK(std::this_thread::get_id() == owner_id_); - { - Lock lock(buffers_to_collect_mutex_); - GarbageCollectBuffersIfAble(); - } + // { + // Lock lock(buffers_to_collect_mutex_); + // GarbageCollectBuffersIfAble(); + // } - if (!recycled_buffers_.empty()) { - vk::UniqueCommandBuffer result = std::move(recycled_buffers_.back()); - recycled_buffers_.pop_back(); - return result; - } + // if (!recycled_buffers_.empty()) { + // vk::UniqueCommandBuffer result = std::move(recycled_buffers_.back()); + // recycled_buffers_.pop_back(); + // return result; + // } vk::CommandBufferAllocateInfo alloc_info; - alloc_info.commandPool = graphics_pool_.get(); + alloc_info.commandPool = graphics_pool_.pool_.get(); alloc_info.commandBufferCount = 1u; alloc_info.level = vk::CommandBufferLevel::ePrimary; auto [result, buffers] = - strong_device->GetDevice().allocateCommandBuffersUnique(alloc_info); + strong_context->GetDevice().allocateCommandBuffersUnique(alloc_info); if (result != vk::Result::eSuccess) { return {}; } + buffer_count_++; return std::move(buffers[0]); } void CommandPoolVK::CollectGraphicsCommandBuffer( vk::UniqueCommandBuffer buffer) { - Lock lock(buffers_to_collect_mutex_); - if (!graphics_pool_) { - // If the command pool has already been destroyed, then its command buffers - // have been freed and are now invalid. - buffer.release(); - } - buffers_to_collect_.emplace_back(std::move(buffer)); - GarbageCollectBuffersIfAble(); + buffer_count_--; + recycled_buffers_.push_back(std::move(buffer)); + // Lock lock(buffers_to_collect_mutex_); + // if (!graphics_pool_) { + // // If the command pool has already been destroyed, then its command + // buffers + // // have been freed and are now invalid. + // buffer.release(); + // } + // buffers_to_collect_.emplace_back(std::move(buffer)); + // GarbageCollectBuffersIfAble(); } -void CommandPoolVK::GarbageCollectBuffersIfAble() { - if (std::this_thread::get_id() != owner_id_) { - return; - } +// void CommandPoolVK::GarbageCollectBuffersIfAble() { +// // if (std::this_thread::get_id() != owner_id_) { +// // return; +// // } - for (auto& buffer : buffers_to_collect_) { - buffer->reset(); - recycled_buffers_.emplace_back(std::move(buffer)); - } +// // for (auto& buffer : buffers_to_collect_) { +// // buffer->reset(); +// // recycled_buffers_.emplace_back(std::move(buffer)); +// // } - buffers_to_collect_.clear(); -} +// // buffers_to_collect_.clear(); +// } } // namespace impeller diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.h b/impeller/renderer/backend/vulkan/command_pool_vk.h index cb077da4c1a80..5cb12c444b9b4 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.h +++ b/impeller/renderer/backend/vulkan/command_pool_vk.h @@ -6,14 +6,47 @@ #include #include +#include +#include #include "flutter/fml/macros.h" +#include "flutter/fml/trace_event.h" #include "impeller/base/thread.h" #include "impeller/renderer/backend/vulkan/context_vk.h" #include "impeller/renderer/backend/vulkan/device_holder.h" namespace impeller { +class CommandPoolVK; + +class ManagedCommandPoolVK { + public: + ManagedCommandPoolVK(vk::UniqueCommandPool pool, + std::weak_ptr context) + : pool_(std::move(pool)), context_(std::move(context)) {} + + ManagedCommandPoolVK(ManagedCommandPoolVK&&) = default; + + ~ManagedCommandPoolVK() { + TRACE_EVENT0("flutter", "ResetCommandPool"); + auto strong_context = context_.lock(); + if (!strong_context) { + return; + } + auto device = strong_context->GetDevice(); + device.resetCommandPool(pool_.get()); + strong_context->RecycleCommandPool(std::move(pool_)); + } + + private: + friend class CommandPoolVK; + + vk::UniqueCommandPool pool_; + std::weak_ptr context_; + + FML_DISALLOW_COPY_AND_ASSIGN(ManagedCommandPoolVK); +}; + //------------------------------------------------------------------------------ /// @brief An opaque object that provides |vk::CommandBuffer| objects. /// @@ -41,6 +74,8 @@ class CommandPoolVK final { static std::shared_ptr GetThreadLocal( const ContextVK* context); + static std::shared_ptr FromContext(const ContextVK* context); + /// @brief Clears all |CommandPoolVK|s for the given |ContextVK|. /// /// @param[in] context The |ContextVK| to clear. @@ -51,9 +86,10 @@ class CommandPoolVK final { /// @note Should only be called when the |ContextVK| is being destroyed. static void ClearAllPools(const ContextVK* context); - /// @brief Releases references to thread-local command pools. - static void ReleaseThreadLocalPool(const ContextVK* context); + /// @brief Releases all command pools. + static void ReleaseAllPools(const ContextVK* context); + CommandPoolVK(CommandPoolVK&&) = default; ~CommandPoolVK(); /// @brief Whether or not this |CommandPoolVK| is valid. @@ -88,11 +124,12 @@ class CommandPoolVK final { friend class ContextVK; const std::thread::id owner_id_; - std::weak_ptr device_holder_; - vk::UniqueCommandPool graphics_pool_; - Mutex buffers_to_collect_mutex_; - std::vector buffers_to_collect_ - IPLR_GUARDED_BY(buffers_to_collect_mutex_); + std::weak_ptr context_; + ManagedCommandPoolVK graphics_pool_; + size_t buffer_count_ = 0; + // Mutex buffers_to_collect_mutex_; + // std::vector buffers_to_collect_ + // IPLR_GUARDED_BY(buffers_to_collect_mutex_); std::vector recycled_buffers_; bool is_valid_ = false; @@ -101,16 +138,9 @@ class CommandPoolVK final { /// @note "All" includes active and recycled buffers. void Reset(); - explicit CommandPoolVK(const ContextVK* context); - - explicit CommandPoolVK(const std::thread::id& owner_id, - std::weak_ptr device_holder, - vk::UniqueCommandPool graphics_pool, - bool is_valid) - : owner_id_(owner_id), - device_holder_(std::move(device_holder)), - graphics_pool_(std::move(graphics_pool)), - is_valid_(is_valid) {} + explicit CommandPoolVK(std::thread::id thread_id, + std::weak_ptr context, + ManagedCommandPoolVK graphics_pool); /// @brief Collects buffers for recycling if able. /// @@ -119,7 +149,8 @@ class CommandPoolVK final { /// |CreateGraphicsCommandBuffer|. /// /// @note This method is a noop if a different thread created the pool. - void GarbageCollectBuffersIfAble() IPLR_REQUIRES(buffers_to_collect_mutex_); + // void GarbageCollectBuffersIfAble() + // IPLR_REQUIRES(buffers_to_collect_mutex_); FML_DISALLOW_COPY_AND_ASSIGN(CommandPoolVK); }; diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index 1160f24f215ea..53b80f495e24a 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -516,12 +516,11 @@ ContextVK::CreateGraphicsCommandEncoderFactory() const { return std::make_unique(weak_from_this()); } -std::optional ContextVK::CreateCommandPool() { +std::optional ContextVK::CreateCommandPool() const { vk::CommandPoolCreateInfo pool_info; pool_info.queueFamilyIndex = GetGraphicsQueue()->GetIndex().family; - pool_info.flags = vk::CommandPoolCreateFlagBits::eTransient | - vk::CommandPoolCreateFlagBits::eResetCommandBuffer; + pool_info.flags = vk::CommandPoolCreateFlagBits::eTransient; auto pool = GetDevice().createCommandPoolUnique(pool_info); if (pool.result != vk::Result::eSuccess) { @@ -531,12 +530,12 @@ std::optional ContextVK::CreateCommandPool() { return std::move(pool.value); } -void ContextVK::RecycleCommandPool(vk::UniqueCommandPool pool) { +void ContextVK::RecycleCommandPool(vk::UniqueCommandPool pool) const { Lock lock(recycled_command_pools_mutex_); recycled_command_pools_.push_back(std::move(pool)); } -std::optional ContextVK::ReuseCommandPool() { +std::optional ContextVK::ReuseCommandPool() const { Lock lock(recycled_command_pools_mutex_); if (recycled_command_pools_.empty()) { return std::nullopt; @@ -546,25 +545,18 @@ std::optional ContextVK::ReuseCommandPool() { return std::move(pool); } -std::optional ContextVK::GetCommandPool() { - vk::UniqueCommandPool pool; - +std::optional ContextVK::GetCommandPool() const { // First try reusing, then try creating. if (auto reuse_pool = ReuseCommandPool()) { - pool = std::move(reuse_pool.value()); + return ManagedCommandPoolVK(std::move(reuse_pool.value()), + weak_from_this()); + } else if (auto create_pool = CreateCommandPool()) { - pool = std::move(create_pool.value()); + return ManagedCommandPoolVK(std::move(create_pool.value()), + weak_from_this()); } else { return std::nullopt; } - - // FIXME: Resource handling: - // 1. Wrap pool in a class that has a reference to this ContextVK. - // (Q: A refrence to the pool? Ptr? Not sure yet) - // 2. In the destructor of that resource class, call: - // context_->RecycleCommandPool(std::move(pool)); (or similar) - - return std::move(pool); } } // namespace impeller diff --git a/impeller/renderer/backend/vulkan/context_vk.h b/impeller/renderer/backend/vulkan/context_vk.h index fab228d3c5b8b..f94a2fa42fc5c 100644 --- a/impeller/renderer/backend/vulkan/context_vk.h +++ b/impeller/renderer/backend/vulkan/context_vk.h @@ -32,6 +32,7 @@ class DebugReportVK; class FenceWaiterVK; class ResourceManagerVK; class SurfaceContextVK; +class ManagedCommandPoolVK; class ContextVK final : public Context, public BackendCast, @@ -150,7 +151,9 @@ class ContextVK final : public Context, /// /// When all references to the returned |vk::UniqueCommandPool| are released, /// the pool will automatically be sent to a background thread for recycling. - std::optional GetCommandPool(); + std::optional GetCommandPool() const; + + void RecycleCommandPool(vk::UniqueCommandPool pool) const; private: struct DeviceHolderImpl : public DeviceHolder { @@ -181,11 +184,10 @@ class ContextVK final : public Context, bool sync_presentation_ = false; const uint64_t hash_; - std::vector recycled_command_pools_; - Mutex recycled_command_pools_mutex_; - std::optional CreateCommandPool(); - void RecycleCommandPool(vk::UniqueCommandPool pool); - std::optional ReuseCommandPool(); + mutable std::vector recycled_command_pools_; + mutable Mutex recycled_command_pools_mutex_; + std::optional CreateCommandPool() const; + std::optional ReuseCommandPool() const; bool is_valid_ = false; diff --git a/impeller/renderer/backend/vulkan/surface_context_vk.cc b/impeller/renderer/backend/vulkan/surface_context_vk.cc index 6c3695db23f35..08a1f41dec7d0 100644 --- a/impeller/renderer/backend/vulkan/surface_context_vk.cc +++ b/impeller/renderer/backend/vulkan/surface_context_vk.cc @@ -79,7 +79,7 @@ std::unique_ptr SurfaceContextVK::AcquireNextSurface() { if (allocator) { allocator->DidAcquireSurfaceFrame(); } - CommandPoolVK::ReleaseThreadLocalPool(parent_.get()); + CommandPoolVK::ReleaseAllPools(parent_.get()); return surface; } From 2603a87957f2b3adcc0cc025120b47bc01fb5d0e Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 8 Sep 2023 20:56:27 -0700 Subject: [PATCH 03/26] Checkpoint. --- .../vulkan/command_pool_recycler_vk.cc | 95 +++++++++++++++ .../backend/vulkan/command_pool_recycler_vk.h | 114 ++++++++++++++++++ 2 files changed, 209 insertions(+) create mode 100644 impeller/renderer/backend/vulkan/command_pool_recycler_vk.cc create mode 100644 impeller/renderer/backend/vulkan/command_pool_recycler_vk.h diff --git a/impeller/renderer/backend/vulkan/command_pool_recycler_vk.cc b/impeller/renderer/backend/vulkan/command_pool_recycler_vk.cc new file mode 100644 index 0000000000000..175788a47e949 --- /dev/null +++ b/impeller/renderer/backend/vulkan/command_pool_recycler_vk.cc @@ -0,0 +1,95 @@ +// 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 "impeller/renderer/backend/vulkan/command_pool_recycler_vk.h" +#include +#include +#include "fml/trace_event.h" + +namespace impeller { + +CommandPoolResourceVK::~CommandPoolResourceVK() { + auto recycler = recycler_.lock(); + if (!recycler) { + return; + } + recycler->Reclaim(std::move(pool_)); +} + +// Associates a resource with a thread and context. +thread_local std::unordered_map> + resources_; + +// TODO(matanlurey): Return a status_or<> instead of nullptr when we have one. +std::shared_ptr CommandPoolRecyclerVK::Get() { + auto const strong_context = context_.lock(); + if (!strong_context) { + return nullptr; + } + + // If there is a resource in used for this thread and context, return it. + auto const hash = strong_context->GetHash(); + auto const it = resources_.find(hash); + if (it != resources_.end()) { + return it->second; + } + + // Otherwise, create a new resource and return it. + auto const pool = Create(); + if (!pool) { + return nullptr; + } + auto const result = std::make_shared(std::move(*pool), + weak_from_this()); + resources_[hash] = result; + return result; +} + +// TODO(matanlurey): Return a status_or<> instead of nullopt when we have one. +std::optional CommandPoolRecyclerVK::Create() { + // If we can reuse a command pool, do so. + if (auto pool = Reuse()) { + return pool; + } + + // Otherwise, create a new one. + return std::nullopt; +} + +std::optional CommandPoolRecyclerVK::Reuse() { + // If there are no recycled pools, return nullopt. + Lock _(recycled_mutex_); + if (recycled_.empty()) { + return std::nullopt; + } + + // Otherwise, remove and return a recycled pool. + auto pool = std::move(recycled_.back()); + recycled_.pop_back(); + return std::move(pool); +} + +void CommandPoolRecyclerVK::Reclaim(vk::UniqueCommandPool&& pool) { + TRACE_EVENT0("flutter", "ReclaimCommandPool"); + // FIXME: Assert that this is called on a background thread. + + // Reset the pool on a background thread. + auto strong_context = context_.lock(); + if (!strong_context) { + return; + } + auto device = strong_context->GetDevice(); + device.resetCommandPool(pool.get()); + + // Move the pool to the recycled list. + Lock _(recycled_mutex_); + recycled_.push_back(std::move(pool)); +} + +void CommandPoolRecyclerVK::Recycle() { + resources_.clear(); +} + +} // namespace impeller diff --git a/impeller/renderer/backend/vulkan/command_pool_recycler_vk.h b/impeller/renderer/backend/vulkan/command_pool_recycler_vk.h new file mode 100644 index 0000000000000..4ec3e5c53cf3c --- /dev/null +++ b/impeller/renderer/backend/vulkan/command_pool_recycler_vk.h @@ -0,0 +1,114 @@ +// 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. + +#pragma once + +#include +#include +#include "fml/macros.h" +#include "impeller/base/thread.h" +#include "impeller/renderer/backend/vulkan/context_vk.h" +#include "impeller/renderer/backend/vulkan/vk.h" // IWYU pragma: keep. + +namespace impeller { + +class CommandPoolRecyclerVK; + +//------------------------------------------------------------------------------ +/// @brief Manages the lifecycle of a single |vk::CommandPool|. +/// +/// A |vk::CommandPool| is expensive to create and reset. This class manages +/// the lifecycle of a single |vk::CommandPool| by returning to the origin +/// (|CommandPoolRecyclerVK|) when it is destroyed to be reused. +/// +/// @warning This class is not thread-safe. +/// +/// @see |CommandPoolRecyclerVK| +class CommandPoolResourceVK final { + public: + CommandPoolResourceVK(CommandPoolResourceVK&&) = default; + ~CommandPoolResourceVK(); + + /// @brief Creates a resource that manages the life of a command pool. + /// + /// @param[in] pool The command pool to manage. + /// @param[in] recycler The recycler that will be notified on destruction. + explicit CommandPoolResourceVK( + vk::UniqueCommandPool&& pool, + const std::weak_ptr& recycler) + : pool_(std::move(pool)), recycler_(recycler) {} + + /// @brief Returns a reference to the underlying |vk::CommandPool|. + const vk::CommandPool& Get() const { return *pool_; } + + private: + FML_DISALLOW_COPY_AND_ASSIGN(CommandPoolResourceVK); + + vk::UniqueCommandPool pool_; + const std::weak_ptr& recycler_; +}; + +//------------------------------------------------------------------------------ +/// @brief Creates and manages the lifecycle of |vk::CommandPool| objects. +/// +/// A |vk::CommandPool| is expensive to create and reset. This class manages +/// the lifecycle of |vk::CommandPool| objects by creating and recycling them; +/// or in other words, a pool for command pools. +/// +/// A single instance should be created per |ContextVK|. +/// +/// Every "frame", a single |CommandPoolResourceVk| is made available for each +/// thread that calls |Get|. After calling |Recycle|, the current thread's pool +/// is moved to a background thread, reset, and made available for the next time +/// |Get| is called and needs to create a command pool. +/// +/// @note This class is thread-safe. +/// +/// @see |vk::CommandPoolResourceVk| +/// @see |ContextVK| +/// @see +/// https://arm-software.github.io/vulkan_best_practice_for_mobile_developers/samples/performance/command_buffer_usage/command_buffer_usage_tutorial.html +class CommandPoolRecyclerVK final + : public std::enable_shared_from_this { + public: + ~CommandPoolRecyclerVK(); + + /// @brief Creates a recycler for the given |ContextVK|. + /// + /// @param[in] context The context to create the recycler for. + explicit CommandPoolRecyclerVK(const ContextVK& context); + + /// @brief Gets a command pool for the current thread. + /// + /// @warning Returns a |nullptr| if a pool could not be created. + std::shared_ptr Get(); + + /// @brief Returns a command pool to be reset on a background thread. + /// + /// @param[in] pool The pool to recycle. + void Reclaim(vk::UniqueCommandPool&& pool); + + /// @brief Clears all recycled command pools to let them be reclaimed. + void Recycle(); + + private: + FML_DISALLOW_COPY_AND_ASSIGN(CommandPoolRecyclerVK); + + /// @brief Creates a new |vk::CommandPool|. + /// + /// @returns Returns a |std::nullopt| if a pool could not be created. + std::optional Create(); + + /// @brief Reuses a recycled |vk::CommandPool|, if available. + /// + /// @returns Returns a |std::nullopt| if a pool was not available. + std::optional Reuse(); + + const std::weak_ptr context_; + + Mutex recycled_mutex_; + std::vector recycled_ IPLR_GUARDED_BY(recycled_mutex_); +}; + +} // namespace impeller From 3e7cb5e096da1d68172aa1555d410ff741be824e Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 8 Sep 2023 22:52:48 -0700 Subject: [PATCH 04/26] Checkpoint: Runs, but segfaults in EncoderVK::Track. --- .../vulkan/blit_command_vk_unittests.cc | 3 +- .../backend/vulkan/command_encoder_vk.cc | 11 +- .../vulkan/command_pool_recycler_vk.cc | 95 ------ .../backend/vulkan/command_pool_recycler_vk.h | 114 ------- .../backend/vulkan/command_pool_vk.cc | 287 ++++++++---------- .../renderer/backend/vulkan/command_pool_vk.h | 205 ++++++------- .../renderer/backend/vulkan/context_vk.cc | 51 ++-- impeller/renderer/backend/vulkan/context_vk.h | 17 +- .../backend/vulkan/context_vk_unittests.cc | 26 +- .../backend/vulkan/surface_context_vk.cc | 2 +- 10 files changed, 260 insertions(+), 551 deletions(-) delete mode 100644 impeller/renderer/backend/vulkan/command_pool_recycler_vk.cc delete mode 100644 impeller/renderer/backend/vulkan/command_pool_recycler_vk.h diff --git a/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc b/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc index 44206c2e7ddb5..839808c7b7ea0 100644 --- a/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "flutter/testing/testing.h" +#include "flutter/testing/testing.h" // IWYU pragma: keep #include "impeller/renderer/backend/vulkan/blit_command_vk.h" #include "impeller/renderer/backend/vulkan/command_encoder_vk.h" #include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" @@ -12,7 +12,6 @@ namespace testing { TEST(BlitCommandVkTest, BlitCopyTextureToTextureCommandVK) { auto context = CreateMockVulkanContext(); - auto pool = CommandPoolVK::GetThreadLocal(context.get()); auto encoder = std::make_unique(context)->Create(); BlitCopyTextureToTextureCommandVK cmd; cmd.source = context->GetResourceAllocator()->CreateTexture({ diff --git a/impeller/renderer/backend/vulkan/command_encoder_vk.cc b/impeller/renderer/backend/vulkan/command_encoder_vk.cc index 9b187896a4f6d..e41681eda4c38 100644 --- a/impeller/renderer/backend/vulkan/command_encoder_vk.cc +++ b/impeller/renderer/backend/vulkan/command_encoder_vk.cc @@ -5,7 +5,6 @@ #include "impeller/renderer/backend/vulkan/command_encoder_vk.h" #include "flutter/fml/closure.h" -#include "flutter/fml/trace_event.h" #include "impeller/renderer/backend/vulkan/context_vk.h" #include "impeller/renderer/backend/vulkan/fence_waiter_vk.h" #include "impeller/renderer/backend/vulkan/texture_vk.h" @@ -16,12 +15,12 @@ class TrackedObjectsVK { public: explicit TrackedObjectsVK( const std::weak_ptr& device_holder, - const std::shared_ptr& pool) + const std::shared_ptr& pool) : desc_pool_(device_holder) { if (!pool) { return; } - auto buffer = pool->CreateGraphicsCommandBuffer(); + auto buffer = pool->CreateBuffer(); if (!buffer) { return; } @@ -34,7 +33,7 @@ class TrackedObjectsVK { if (!buffer_) { return; } - pool_->CollectGraphicsCommandBuffer(std::move(buffer_)); + pool_->CollectBuffer(std::move(buffer_)); } bool IsValid() const { return is_valid_; } @@ -81,7 +80,7 @@ class TrackedObjectsVK { private: DescriptorPoolVK desc_pool_; // `shared_ptr` since command buffers have a link to the command pool. - std::shared_ptr pool_; + std::shared_ptr pool_; vk::UniqueCommandBuffer buffer_; std::set> tracked_objects_; std::set> tracked_buffers_; @@ -105,7 +104,7 @@ std::shared_ptr CommandEncoderFactoryVK::Create() { return nullptr; } auto& context_vk = ContextVK::Cast(*context); - auto tls_pool = CommandPoolVK::GetThreadLocal(&context_vk); + auto tls_pool = context_vk.GetCommandPoolRecycler()->Get(); if (!tls_pool) { return nullptr; } diff --git a/impeller/renderer/backend/vulkan/command_pool_recycler_vk.cc b/impeller/renderer/backend/vulkan/command_pool_recycler_vk.cc deleted file mode 100644 index 175788a47e949..0000000000000 --- a/impeller/renderer/backend/vulkan/command_pool_recycler_vk.cc +++ /dev/null @@ -1,95 +0,0 @@ -// 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 "impeller/renderer/backend/vulkan/command_pool_recycler_vk.h" -#include -#include -#include "fml/trace_event.h" - -namespace impeller { - -CommandPoolResourceVK::~CommandPoolResourceVK() { - auto recycler = recycler_.lock(); - if (!recycler) { - return; - } - recycler->Reclaim(std::move(pool_)); -} - -// Associates a resource with a thread and context. -thread_local std::unordered_map> - resources_; - -// TODO(matanlurey): Return a status_or<> instead of nullptr when we have one. -std::shared_ptr CommandPoolRecyclerVK::Get() { - auto const strong_context = context_.lock(); - if (!strong_context) { - return nullptr; - } - - // If there is a resource in used for this thread and context, return it. - auto const hash = strong_context->GetHash(); - auto const it = resources_.find(hash); - if (it != resources_.end()) { - return it->second; - } - - // Otherwise, create a new resource and return it. - auto const pool = Create(); - if (!pool) { - return nullptr; - } - auto const result = std::make_shared(std::move(*pool), - weak_from_this()); - resources_[hash] = result; - return result; -} - -// TODO(matanlurey): Return a status_or<> instead of nullopt when we have one. -std::optional CommandPoolRecyclerVK::Create() { - // If we can reuse a command pool, do so. - if (auto pool = Reuse()) { - return pool; - } - - // Otherwise, create a new one. - return std::nullopt; -} - -std::optional CommandPoolRecyclerVK::Reuse() { - // If there are no recycled pools, return nullopt. - Lock _(recycled_mutex_); - if (recycled_.empty()) { - return std::nullopt; - } - - // Otherwise, remove and return a recycled pool. - auto pool = std::move(recycled_.back()); - recycled_.pop_back(); - return std::move(pool); -} - -void CommandPoolRecyclerVK::Reclaim(vk::UniqueCommandPool&& pool) { - TRACE_EVENT0("flutter", "ReclaimCommandPool"); - // FIXME: Assert that this is called on a background thread. - - // Reset the pool on a background thread. - auto strong_context = context_.lock(); - if (!strong_context) { - return; - } - auto device = strong_context->GetDevice(); - device.resetCommandPool(pool.get()); - - // Move the pool to the recycled list. - Lock _(recycled_mutex_); - recycled_.push_back(std::move(pool)); -} - -void CommandPoolRecyclerVK::Recycle() { - resources_.clear(); -} - -} // namespace impeller diff --git a/impeller/renderer/backend/vulkan/command_pool_recycler_vk.h b/impeller/renderer/backend/vulkan/command_pool_recycler_vk.h deleted file mode 100644 index 4ec3e5c53cf3c..0000000000000 --- a/impeller/renderer/backend/vulkan/command_pool_recycler_vk.h +++ /dev/null @@ -1,114 +0,0 @@ -// 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. - -#pragma once - -#include -#include -#include "fml/macros.h" -#include "impeller/base/thread.h" -#include "impeller/renderer/backend/vulkan/context_vk.h" -#include "impeller/renderer/backend/vulkan/vk.h" // IWYU pragma: keep. - -namespace impeller { - -class CommandPoolRecyclerVK; - -//------------------------------------------------------------------------------ -/// @brief Manages the lifecycle of a single |vk::CommandPool|. -/// -/// A |vk::CommandPool| is expensive to create and reset. This class manages -/// the lifecycle of a single |vk::CommandPool| by returning to the origin -/// (|CommandPoolRecyclerVK|) when it is destroyed to be reused. -/// -/// @warning This class is not thread-safe. -/// -/// @see |CommandPoolRecyclerVK| -class CommandPoolResourceVK final { - public: - CommandPoolResourceVK(CommandPoolResourceVK&&) = default; - ~CommandPoolResourceVK(); - - /// @brief Creates a resource that manages the life of a command pool. - /// - /// @param[in] pool The command pool to manage. - /// @param[in] recycler The recycler that will be notified on destruction. - explicit CommandPoolResourceVK( - vk::UniqueCommandPool&& pool, - const std::weak_ptr& recycler) - : pool_(std::move(pool)), recycler_(recycler) {} - - /// @brief Returns a reference to the underlying |vk::CommandPool|. - const vk::CommandPool& Get() const { return *pool_; } - - private: - FML_DISALLOW_COPY_AND_ASSIGN(CommandPoolResourceVK); - - vk::UniqueCommandPool pool_; - const std::weak_ptr& recycler_; -}; - -//------------------------------------------------------------------------------ -/// @brief Creates and manages the lifecycle of |vk::CommandPool| objects. -/// -/// A |vk::CommandPool| is expensive to create and reset. This class manages -/// the lifecycle of |vk::CommandPool| objects by creating and recycling them; -/// or in other words, a pool for command pools. -/// -/// A single instance should be created per |ContextVK|. -/// -/// Every "frame", a single |CommandPoolResourceVk| is made available for each -/// thread that calls |Get|. After calling |Recycle|, the current thread's pool -/// is moved to a background thread, reset, and made available for the next time -/// |Get| is called and needs to create a command pool. -/// -/// @note This class is thread-safe. -/// -/// @see |vk::CommandPoolResourceVk| -/// @see |ContextVK| -/// @see -/// https://arm-software.github.io/vulkan_best_practice_for_mobile_developers/samples/performance/command_buffer_usage/command_buffer_usage_tutorial.html -class CommandPoolRecyclerVK final - : public std::enable_shared_from_this { - public: - ~CommandPoolRecyclerVK(); - - /// @brief Creates a recycler for the given |ContextVK|. - /// - /// @param[in] context The context to create the recycler for. - explicit CommandPoolRecyclerVK(const ContextVK& context); - - /// @brief Gets a command pool for the current thread. - /// - /// @warning Returns a |nullptr| if a pool could not be created. - std::shared_ptr Get(); - - /// @brief Returns a command pool to be reset on a background thread. - /// - /// @param[in] pool The pool to recycle. - void Reclaim(vk::UniqueCommandPool&& pool); - - /// @brief Clears all recycled command pools to let them be reclaimed. - void Recycle(); - - private: - FML_DISALLOW_COPY_AND_ASSIGN(CommandPoolRecyclerVK); - - /// @brief Creates a new |vk::CommandPool|. - /// - /// @returns Returns a |std::nullopt| if a pool could not be created. - std::optional Create(); - - /// @brief Reuses a recycled |vk::CommandPool|, if available. - /// - /// @returns Returns a |std::nullopt| if a pool was not available. - std::optional Reuse(); - - const std::weak_ptr context_; - - Mutex recycled_mutex_; - std::vector recycled_ IPLR_GUARDED_BY(recycled_mutex_); -}; - -} // namespace impeller diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.cc b/impeller/renderer/backend/vulkan/command_pool_vk.cc index 20f336e16adee..c4ff73e78858e 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.cc +++ b/impeller/renderer/backend/vulkan/command_pool_vk.cc @@ -3,202 +3,173 @@ // found in the LICENSE file. #include "impeller/renderer/backend/vulkan/command_pool_vk.h" - -#include +#include #include -#include -#include - -#include "flutter/fml/thread_local.h" -#include "impeller/base/thread.h" -#include "impeller/renderer/backend/vulkan/context_vk.h" +#include +#include "fml/macros.h" +#include "fml/thread_local.h" +#include "fml/trace_event.h" #include "impeller/renderer/backend/vulkan/resource_manager_vk.h" +#include "vulkan/vulkan_structs.hpp" namespace impeller { -using CommandPoolMap = std::map>; -// NOTE: How TF do we reset multiple threads? -FML_THREAD_LOCAL fml::ThreadLocalUniquePtr tls_command_pool; +class RecyclingCommandPoolVK { + public: + RecyclingCommandPoolVK(RecyclingCommandPoolVK&&) = default; + + explicit RecyclingCommandPoolVK( + vk::UniqueCommandPool&& pool, + std::vector&& buffers, + std::weak_ptr recycler) + : pool_(std::move(pool)), + buffers_(std::move(buffers)), + recycler_(std::move(recycler)) {} + + ~RecyclingCommandPoolVK() { + auto const recycler = recycler_.lock(); + if (!recycler) { + return; + } + recycler->Reclaim(std::move(pool_)); + } + + private: + FML_DISALLOW_COPY_AND_ASSIGN(RecyclingCommandPoolVK); -// static Mutex g_all_pools_mutex; -// static std::unordered_map>> -// g_all_pools IPLR_GUARDED_BY(g_all_pools_mutex); + vk::UniqueCommandPool pool_; + std::vector buffers_; + std::weak_ptr recycler_; +}; -void CommandPoolVK::ReleaseAllPools(const ContextVK* context) { - if (tls_command_pool.get() == nullptr) { +CommandPoolResourceVK::~CommandPoolResourceVK() { + auto const context = context_.lock(); + if (!context) { return; } - CommandPoolMap& pool_map = *tls_command_pool.get(); - pool_map.clear(); + auto const recycler = context->GetCommandPoolRecycler(); + if (!recycler) { + return; + } + UniqueResourceVKT pool( + context->GetResourceManager(), + RecyclingCommandPoolVK(std::move(pool_), std::move(collected_buffers_), + recycler)); } -std::shared_ptr CommandPoolVK::GetThreadLocal( - const ContextVK* context) { +// TODO(matanlurey): Return a status_or<> instead of {} when we have one. +vk::UniqueCommandBuffer CommandPoolResourceVK::CreateBuffer() { + auto const context = context_.lock(); if (!context) { + return {}; + } + + auto const device = context->GetDevice(); + vk::CommandBufferAllocateInfo info; + info.setCommandPool(pool_.get()); + info.setCommandBufferCount(1u); + info.setLevel(vk::CommandBufferLevel::ePrimary); + auto [result, buffers] = device.allocateCommandBuffersUnique(info); + if (result != vk::Result::eSuccess) { + return {}; + } + + return std::move(buffers[0]); +} + +void CommandPoolResourceVK::CollectBuffer(vk::UniqueCommandBuffer&& buffer) { + collected_buffers_.push_back(std::move(buffer)); +} + +// Associates a resource with a thread and context. +using CommandPoolMap = + std::unordered_map>; +FML_THREAD_LOCAL fml::ThreadLocalUniquePtr resources_; + +// TODO(matanlurey): Return a status_or<> instead of nullptr when we have one. +std::shared_ptr CommandPoolRecyclerVK::Get() { + auto const strong_context = context_.lock(); + if (!strong_context) { return nullptr; } - if (tls_command_pool.get() == nullptr) { - tls_command_pool.reset(new CommandPoolMap()); + + // If there is a resource in used for this thread and context, return it. + auto resources = resources_.get(); + if (!resources) { + resources = new CommandPoolMap(); + resources_.reset(resources); } - CommandPoolMap& pool_map = *tls_command_pool.get(); - auto found = pool_map.find(context->GetHash()); - if (found != pool_map.end() && found->second->IsValid()) { - return found->second; + auto map = *resources; + auto const hash = strong_context->GetHash(); + auto const it = map.find(hash); + if (it != map.end()) { + return it->second; } - auto pool = CommandPoolVK::FromContext(context); - if (!pool || !pool->IsValid()) { + + // Otherwise, create a new resource and return it. + auto pool = Create(); + if (!pool) { return nullptr; } - pool_map[context->GetHash()] = pool; - // { - // Lock pool_lock(g_all_pools_mutex); - // g_all_pools[context].push_back(pool); - // } - return pool; -} -void CommandPoolVK::ClearAllPools(const ContextVK* context) { - // if (tls_command_pool.get()) { - // tls_command_pool.get()->erase(context->GetHash()); - // } - // Lock pool_lock(g_all_pools_mutex); - // if (auto found = g_all_pools.find(context); found != g_all_pools.end()) { - // for (auto& weak_pool : found->second) { - // auto pool = weak_pool.lock(); - // if (!pool) { - // // The pool has already died because the thread died. - // continue; - // } - // // The pool is reset but its reference in the TLS map remains till the - // // thread dies. - // pool->Reset(); - // } - // g_all_pools.erase(found); - // } + auto const resource = + std::make_shared(std::move(*pool), context_); + map[hash] = resource; + return resource; } -std::shared_ptr CommandPoolVK::FromContext( - const ContextVK* context) { - auto pool = context->GetCommandPool(); - if (!pool) { - return nullptr; +// TODO(matanlurey): Return a status_or<> instead of nullopt when we have one. +std::optional CommandPoolRecyclerVK::Create() { + // If we can reuse a command pool, do so. + if (auto pool = Reuse()) { + return pool; } - return std::make_shared(CommandPoolVK( - std::this_thread::get_id(), context->weak_from_this(), std::move(*pool))); + // Otherwise, create a new one. + return std::nullopt; } -CommandPoolVK::CommandPoolVK(std::thread::id thread_id, - std::weak_ptr context, - ManagedCommandPoolVK graphics_pool) - : owner_id_(thread_id), - context_(std::move(context)), - graphics_pool_(std::move(graphics_pool)) { - // FIXME: Not always true. - is_valid_ = true; +std::optional CommandPoolRecyclerVK::Reuse() { + // If there are no recycled pools, return nullopt. + Lock _(recycled_mutex_); + if (recycled_.empty()) { + return std::nullopt; + } + + // Otherwise, remove and return a recycled pool. + auto pool = std::move(recycled_.back()); + recycled_.pop_back(); + return std::move(pool); } -struct JustFuckingCompile { - ManagedCommandPoolVK pool; - std::vector buffers; -}; +void CommandPoolRecyclerVK::Reclaim(vk::UniqueCommandPool&& pool) { + TRACE_EVENT0("impeller", "ReclaimCommandPool"); + // FIXME: Assert that this is called on a background thread. -CommandPoolVK::~CommandPoolVK() { + // Reset the pool on a background thread. auto strong_context = context_.lock(); if (!strong_context) { return; } - auto pool = std::move(graphics_pool_); - UniqueResourceVKT( - strong_context->GetResourceManager(), - JustFuckingCompile{.pool = std::move(pool), - .buffers = std::move(recycled_buffers_)}); -} + auto device = strong_context->GetDevice(); + device.resetCommandPool(pool.get()); -bool CommandPoolVK::IsValid() const { - return is_valid_; + // Move the pool to the recycled list. + Lock _(recycled_mutex_); + recycled_.push_back(std::move(pool)); } -void CommandPoolVK::Reset() { - // { - // Lock lock(buffers_to_collect_mutex_); - // graphics_pool_.reset(); - - // // When the command pool is destroyed, all of its command buffers are - // freed. - // // Handles allocated from that pool are now invalid and must be - // discarded. for (vk::UniqueCommandBuffer& buffer : buffers_to_collect_) - // { - // buffer.release(); - // } - // buffers_to_collect_.clear(); - // } - - // for (vk::UniqueCommandBuffer& buffer : recycled_buffers_) { - // buffer.release(); - // } - // recycled_buffers_.clear(); - - // is_valid_ = false; +CommandPoolRecyclerVK::~CommandPoolRecyclerVK() { + // Ensure all recycled pools are reclaimed before this is destroyed. + Recycle(); } -vk::UniqueCommandBuffer CommandPoolVK::CreateGraphicsCommandBuffer() { - auto strong_context = context_.lock(); - if (!strong_context) { - return {}; - } - FML_DCHECK(std::this_thread::get_id() == owner_id_); - // { - // Lock lock(buffers_to_collect_mutex_); - // GarbageCollectBuffersIfAble(); - // } - - // if (!recycled_buffers_.empty()) { - // vk::UniqueCommandBuffer result = std::move(recycled_buffers_.back()); - // recycled_buffers_.pop_back(); - // return result; - // } - - vk::CommandBufferAllocateInfo alloc_info; - alloc_info.commandPool = graphics_pool_.pool_.get(); - alloc_info.commandBufferCount = 1u; - alloc_info.level = vk::CommandBufferLevel::ePrimary; - auto [result, buffers] = - strong_context->GetDevice().allocateCommandBuffersUnique(alloc_info); - if (result != vk::Result::eSuccess) { - return {}; +void CommandPoolRecyclerVK::Recycle() { + auto const resources = resources_.get(); + if (!resources) { + return; } - buffer_count_++; - return std::move(buffers[0]); + resources->clear(); } -void CommandPoolVK::CollectGraphicsCommandBuffer( - vk::UniqueCommandBuffer buffer) { - buffer_count_--; - recycled_buffers_.push_back(std::move(buffer)); - // Lock lock(buffers_to_collect_mutex_); - // if (!graphics_pool_) { - // // If the command pool has already been destroyed, then its command - // buffers - // // have been freed and are now invalid. - // buffer.release(); - // } - // buffers_to_collect_.emplace_back(std::move(buffer)); - // GarbageCollectBuffersIfAble(); -} - -// void CommandPoolVK::GarbageCollectBuffersIfAble() { -// // if (std::this_thread::get_id() != owner_id_) { -// // return; -// // } - -// // for (auto& buffer : buffers_to_collect_) { -// // buffer->reset(); -// // recycled_buffers_.emplace_back(std::move(buffer)); -// // } - -// // buffers_to_collect_.clear(); -// } - } // namespace impeller diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.h b/impeller/renderer/backend/vulkan/command_pool_vk.h index 5cb12c444b9b4..5c6009f40330a 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.h +++ b/impeller/renderer/backend/vulkan/command_pool_vk.h @@ -5,154 +5,129 @@ #pragma once #include -#include +#include #include -#include - -#include "flutter/fml/macros.h" -#include "flutter/fml/trace_event.h" +#include "fml/macros.h" #include "impeller/base/thread.h" #include "impeller/renderer/backend/vulkan/context_vk.h" -#include "impeller/renderer/backend/vulkan/device_holder.h" +#include "impeller/renderer/backend/vulkan/vk.h" // IWYU pragma: keep. namespace impeller { -class CommandPoolVK; - -class ManagedCommandPoolVK { - public: - ManagedCommandPoolVK(vk::UniqueCommandPool pool, - std::weak_ptr context) - : pool_(std::move(pool)), context_(std::move(context)) {} - - ManagedCommandPoolVK(ManagedCommandPoolVK&&) = default; - - ~ManagedCommandPoolVK() { - TRACE_EVENT0("flutter", "ResetCommandPool"); - auto strong_context = context_.lock(); - if (!strong_context) { - return; - } - auto device = strong_context->GetDevice(); - device.resetCommandPool(pool_.get()); - strong_context->RecycleCommandPool(std::move(pool_)); - } - - private: - friend class CommandPoolVK; - - vk::UniqueCommandPool pool_; - std::weak_ptr context_; - - FML_DISALLOW_COPY_AND_ASSIGN(ManagedCommandPoolVK); -}; +class ContextVK; +class CommandPoolRecyclerVK; //------------------------------------------------------------------------------ -/// @brief An opaque object that provides |vk::CommandBuffer| objects. +/// @brief Manages the lifecycle of a single |vk::CommandPool|. /// -/// A best practice is to create a |CommandPoolVK| for each thread that will -/// submit commands to the GPU, and to recycle (reuse) |vk::CommandBuffer|s by -/// resetting them in a background thread. +/// A |vk::CommandPool| is expensive to create and reset. This class manages +/// the lifecycle of a single |vk::CommandPool| by returning to the origin +/// (|CommandPoolRecyclerVK|) when it is destroyed to be reused. /// -/// @see -/// https://arm-software.github.io/vulkan_best_practice_for_mobile_developers/samples/performance/command_buffer_usage/command_buffer_usage_tutorial.html#resetting-the-command-pool -class CommandPoolVK final { +/// @warning This class is not thread-safe. +/// +/// @see |CommandPoolRecyclerVK| +class CommandPoolResourceVK final { public: - /// @brief Gets the |CommandPoolVK| for the current thread. - /// - /// @param[in] context The |ContextVK| to use. - /// - /// If the current thread does not have a |CommandPoolVK|, one will be created - /// and returned. If the current thread already has a |CommandPoolVK|, it will - /// be returned. - /// - /// @return The |CommandPoolVK| for the current thread, or |nullptr| if - /// either the |ContextVK| is invalid, or a pool could not be - /// created for any reason. - /// - /// In other words an invalid command pool will never be returned. - static std::shared_ptr GetThreadLocal( - const ContextVK* context); + CommandPoolResourceVK(CommandPoolResourceVK&&) = default; + ~CommandPoolResourceVK(); - static std::shared_ptr FromContext(const ContextVK* context); - - /// @brief Clears all |CommandPoolVK|s for the given |ContextVK|. - /// - /// @param[in] context The |ContextVK| to clear. - /// - /// Every |CommandPoolVK| that was created for every thread that has ever - /// called |GetThreadLocal| with the given |ContextVK| will be cleared. + /// @brief Creates a resource that manages the life of a command pool. /// - /// @note Should only be called when the |ContextVK| is being destroyed. - static void ClearAllPools(const ContextVK* context); + /// @param[in] pool The command pool to manage. + /// @param[in] recycler The context that will be notified on destruction. + explicit CommandPoolResourceVK(vk::UniqueCommandPool pool, + std::weak_ptr& context) + : pool_(std::move(pool)), context_(context) {} - /// @brief Releases all command pools. - static void ReleaseAllPools(const ContextVK* context); - - CommandPoolVK(CommandPoolVK&&) = default; - ~CommandPoolVK(); - - /// @brief Whether or not this |CommandPoolVK| is valid. - /// - /// A command pool is no longer when valid once it's been |Reset|. - bool IsValid() const; + /// @brief Returns a reference to the underlying |vk::CommandPool|. + const vk::CommandPool& Get() const { return *pool_; } /// @brief Creates and returns a new |vk::CommandBuffer|. /// - /// An attempt is made to reuse existing buffers (instead of creating new - /// ones) by recycling buffers that have been collected by - /// |CollectGraphicsCommandBuffer|. - /// /// @return Always returns a new |vk::CommandBuffer|, but if for any /// reason a valid command buffer could not be created, it will be /// a `{}` default instance (i.e. while being torn down). - vk::UniqueCommandBuffer CreateGraphicsCommandBuffer(); + vk::UniqueCommandBuffer CreateBuffer(); - /// @brief Collects the given |vk::CommandBuffer| for recycling. - /// - /// The given |vk::CommandBuffer| will be recycled (reused) in the future when - /// |CreateGraphicsCommandBuffer| is called. + /// @brief Collects the given |vk::CommandBuffer| to be retained. /// /// @param[in] buffer The |vk::CommandBuffer| to collect. /// - /// @note This method is a noop if a different thread created the pool. - /// /// @see |GarbageCollectBuffersIfAble| - void CollectGraphicsCommandBuffer(vk::UniqueCommandBuffer buffer); + void CollectBuffer(vk::UniqueCommandBuffer&& buffer); private: - friend class ContextVK; - - const std::thread::id owner_id_; - std::weak_ptr context_; - ManagedCommandPoolVK graphics_pool_; - size_t buffer_count_ = 0; - // Mutex buffers_to_collect_mutex_; - // std::vector buffers_to_collect_ - // IPLR_GUARDED_BY(buffers_to_collect_mutex_); - std::vector recycled_buffers_; - bool is_valid_ = false; - - /// @brief Resets, releasing all |vk::CommandBuffer|s. + FML_DISALLOW_COPY_AND_ASSIGN(CommandPoolResourceVK); + + vk::UniqueCommandPool pool_; + std::weak_ptr& context_; + + // Used to retain a reference on these until the pool is reset. + std::vector collected_buffers_; +}; + +//------------------------------------------------------------------------------ +/// @brief Creates and manages the lifecycle of |vk::CommandPool| objects. +/// +/// A |vk::CommandPool| is expensive to create and reset. This class manages +/// the lifecycle of |vk::CommandPool| objects by creating and recycling them; +/// or in other words, a pool for command pools. +/// +/// A single instance should be created per |ContextVK|. +/// +/// Every "frame", a single |CommandPoolResourceVk| is made available for each +/// thread that calls |Get|. After calling |Recycle|, the current thread's pool +/// is moved to a background thread, reset, and made available for the next time +/// |Get| is called and needs to create a command pool. +/// +/// @note This class is thread-safe. +/// +/// @see |vk::CommandPoolResourceVk| +/// @see |ContextVK| +/// @see +/// https://arm-software.github.io/vulkan_best_practice_for_mobile_developers/samples/performance/command_buffer_usage/command_buffer_usage_tutorial.html +class CommandPoolRecyclerVK final + : public std::enable_shared_from_this { + public: + ~CommandPoolRecyclerVK(); + + /// @brief Creates a recycler for the given |ContextVK|. /// - /// @note "All" includes active and recycled buffers. - void Reset(); + /// @param[in] context The context to create the recycler for. + explicit CommandPoolRecyclerVK(std::weak_ptr context) + : context_(std::move(context)) {} - explicit CommandPoolVK(std::thread::id thread_id, - std::weak_ptr context, - ManagedCommandPoolVK graphics_pool); + /// @brief Gets a command pool for the current thread. + /// + /// @warning Returns a |nullptr| if a pool could not be created. + std::shared_ptr Get(); - /// @brief Collects buffers for recycling if able. + /// @brief Returns a command pool to be reset on a background thread. /// - /// If any buffers have been returned through |CollectGraphicsCommandBuffer|, - /// then they are reset and made available to future calls to - /// |CreateGraphicsCommandBuffer|. + /// @param[in] pool The pool to recycle. + void Reclaim(vk::UniqueCommandPool&& pool); + + /// @brief Clears all recycled command pools to let them be reclaimed. + void Recycle(); + + private: + FML_DISALLOW_COPY_AND_ASSIGN(CommandPoolRecyclerVK); + + /// @brief Creates a new |vk::CommandPool|. /// - /// @note This method is a noop if a different thread created the pool. - // void GarbageCollectBuffersIfAble() - // IPLR_REQUIRES(buffers_to_collect_mutex_); + /// @returns Returns a |std::nullopt| if a pool could not be created. + std::optional Create(); + + /// @brief Reuses a recycled |vk::CommandPool|, if available. + /// + /// @returns Returns a |std::nullopt| if a pool was not available. + std::optional Reuse(); + + std::weak_ptr context_; - FML_DISALLOW_COPY_AND_ASSIGN(CommandPoolVK); + Mutex recycled_mutex_; + std::vector recycled_ IPLR_GUARDED_BY(recycled_mutex_); }; } // namespace impeller diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index 53b80f495e24a..7d70c26e548e2 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -3,7 +3,6 @@ // found in the LICENSE file. #include "impeller/renderer/backend/vulkan/context_vk.h" -#include "fml/macros.h" #ifdef FML_OS_ANDROID #include @@ -14,11 +13,9 @@ #include #include #include -#include #include #include -#include "flutter/fml/build_config.h" #include "flutter/fml/trace_event.h" #include "impeller/base/validation.h" #include "impeller/renderer/backend/vulkan/allocator_vk.h" @@ -115,7 +112,9 @@ ContextVK::~ContextVK() { if (device_holder_ && device_holder_->device) { [[maybe_unused]] auto result = device_holder_->device->waitIdle(); } - CommandPoolVK::ClearAllPools(this); + if (command_pool_recycler_) { + command_pool_recycler_.get()->Recycle(); + } } Context::BackendType ContextVK::GetBackendType() const { @@ -386,7 +385,7 @@ void ContextVK::Setup(Settings settings) { } //---------------------------------------------------------------------------- - /// Create the resource manager. + /// Create the resource manager and command pool recycler. /// auto resource_manager = ResourceManagerVK::Create(); if (!resource_manager) { @@ -394,6 +393,13 @@ void ContextVK::Setup(Settings settings) { return; } + auto command_pool_recycler = + std::make_shared(weak_from_this()); + if (!command_pool_recycler) { + VALIDATION_LOG << "Could not create command pool recycler."; + return; + } + //---------------------------------------------------------------------------- /// Fetch the queues. /// @@ -424,6 +430,7 @@ void ContextVK::Setup(Settings settings) { device_capabilities_ = std::move(caps); fence_waiter_ = std::move(fence_waiter); resource_manager_ = std::move(resource_manager); + command_pool_recycler_ = std::move(command_pool_recycler); device_name_ = std::string(physical_device_properties.deviceName); is_valid_ = true; @@ -511,6 +518,11 @@ std::shared_ptr ContextVK::GetResourceManager() const { return resource_manager_; } +std::shared_ptr ContextVK::GetCommandPoolRecycler() + const { + return command_pool_recycler_; +} + std::unique_ptr ContextVK::CreateGraphicsCommandEncoderFactory() const { return std::make_unique(weak_from_this()); @@ -530,33 +542,4 @@ std::optional ContextVK::CreateCommandPool() const { return std::move(pool.value); } -void ContextVK::RecycleCommandPool(vk::UniqueCommandPool pool) const { - Lock lock(recycled_command_pools_mutex_); - recycled_command_pools_.push_back(std::move(pool)); -} - -std::optional ContextVK::ReuseCommandPool() const { - Lock lock(recycled_command_pools_mutex_); - if (recycled_command_pools_.empty()) { - return std::nullopt; - } - auto pool = std::move(recycled_command_pools_.back()); - recycled_command_pools_.pop_back(); - return std::move(pool); -} - -std::optional ContextVK::GetCommandPool() const { - // First try reusing, then try creating. - if (auto reuse_pool = ReuseCommandPool()) { - return ManagedCommandPoolVK(std::move(reuse_pool.value()), - weak_from_this()); - - } else if (auto create_pool = CreateCommandPool()) { - return ManagedCommandPoolVK(std::move(create_pool.value()), - weak_from_this()); - } else { - return std::nullopt; - } -} - } // namespace impeller diff --git a/impeller/renderer/backend/vulkan/context_vk.h b/impeller/renderer/backend/vulkan/context_vk.h index f94a2fa42fc5c..1982970bdd469 100644 --- a/impeller/renderer/backend/vulkan/context_vk.h +++ b/impeller/renderer/backend/vulkan/context_vk.h @@ -11,9 +11,9 @@ #include "flutter/fml/macros.h" #include "flutter/fml/mapping.h" #include "flutter/fml/unique_fd.h" -#include "fml/status.h" #include "impeller/base/backend_cast.h" #include "impeller/core/formats.h" +#include "impeller/renderer/backend/vulkan/command_pool_vk.h" #include "impeller/renderer/backend/vulkan/device_holder.h" #include "impeller/renderer/backend/vulkan/pipeline_library_vk.h" #include "impeller/renderer/backend/vulkan/queue_vk.h" @@ -28,11 +28,11 @@ bool HasValidationLayers(); class CommandEncoderFactoryVK; class CommandEncoderVK; +class CommandPoolRecyclerVK; class DebugReportVK; class FenceWaiterVK; class ResourceManagerVK; class SurfaceContextVK; -class ManagedCommandPoolVK; class ContextVK final : public Context, public BackendCast, @@ -143,17 +143,7 @@ class ContextVK final : public Context, std::shared_ptr GetResourceManager() const; - /// @brief Returns a |vk::UniqueCommandPool|, or |std::nullopt| if the - /// pool could not be created. - /// - /// If able, this method will reuse any |vk::CommandBuffer|s that have reset, - /// otherwise a new pool will be created. - /// - /// When all references to the returned |vk::UniqueCommandPool| are released, - /// the pool will automatically be sent to a background thread for recycling. - std::optional GetCommandPool() const; - - void RecycleCommandPool(vk::UniqueCommandPool pool) const; + std::shared_ptr GetCommandPoolRecycler() const; private: struct DeviceHolderImpl : public DeviceHolder { @@ -179,6 +169,7 @@ class ContextVK final : public Context, std::shared_ptr device_capabilities_; std::shared_ptr fence_waiter_; std::shared_ptr resource_manager_; + std::shared_ptr command_pool_recycler_; std::string device_name_; std::shared_ptr raster_message_loop_; bool sync_presentation_ = false; diff --git a/impeller/renderer/backend/vulkan/context_vk_unittests.cc b/impeller/renderer/backend/vulkan/context_vk_unittests.cc index f584f104689ee..c62fece1d4010 100644 --- a/impeller/renderer/backend/vulkan/context_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/context_vk_unittests.cc @@ -11,19 +11,19 @@ namespace impeller { namespace testing { TEST(ContextVKTest, DeletesCommandPools) { - std::weak_ptr weak_context; - std::weak_ptr weak_pool; - { - std::shared_ptr context = CreateMockVulkanContext(); - std::shared_ptr pool = - CommandPoolVK::GetThreadLocal(context.get()); - weak_pool = pool; - weak_context = context; - ASSERT_TRUE(weak_pool.lock()); - ASSERT_TRUE(weak_context.lock()); - } - ASSERT_FALSE(weak_pool.lock()); - ASSERT_FALSE(weak_context.lock()); + // std::weak_ptr weak_context; + // std::weak_ptr weak_pool; + // { + // std::shared_ptr context = CreateMockVulkanContext(); + // std::shared_ptr pool = + // CommandPoolVK::GetThreadLocal(context.get()); + // weak_pool = pool; + // weak_context = context; + // ASSERT_TRUE(weak_pool.lock()); + // ASSERT_TRUE(weak_context.lock()); + // } + // ASSERT_FALSE(weak_pool.lock()); + // ASSERT_FALSE(weak_context.lock()); } TEST(ContextVKTest, DeletePipelineAfterContext) { diff --git a/impeller/renderer/backend/vulkan/surface_context_vk.cc b/impeller/renderer/backend/vulkan/surface_context_vk.cc index 08a1f41dec7d0..f2b5dd0e6997e 100644 --- a/impeller/renderer/backend/vulkan/surface_context_vk.cc +++ b/impeller/renderer/backend/vulkan/surface_context_vk.cc @@ -79,7 +79,7 @@ std::unique_ptr SurfaceContextVK::AcquireNextSurface() { if (allocator) { allocator->DidAcquireSurfaceFrame(); } - CommandPoolVK::ReleaseAllPools(parent_.get()); + parent_->GetCommandPoolRecycler()->Recycle(); return surface; } From f35dc1f37ea56d72288d8a2f02d3974a3ca8f9f6 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 8 Sep 2023 23:02:51 -0700 Subject: [PATCH 05/26] Checkpoint: Runs, but segfaults in EncoderVK::Track. --- .../renderer/backend/vulkan/command_pool_vk.cc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.cc b/impeller/renderer/backend/vulkan/command_pool_vk.cc index c4ff73e78858e..70eaa30eecf6b 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.cc +++ b/impeller/renderer/backend/vulkan/command_pool_vk.cc @@ -14,11 +14,11 @@ namespace impeller { -class RecyclingCommandPoolVK { +class BackgroundCommandPoolVK { public: - RecyclingCommandPoolVK(RecyclingCommandPoolVK&&) = default; + BackgroundCommandPoolVK(BackgroundCommandPoolVK&&) = default; - explicit RecyclingCommandPoolVK( + explicit BackgroundCommandPoolVK( vk::UniqueCommandPool&& pool, std::vector&& buffers, std::weak_ptr recycler) @@ -26,7 +26,7 @@ class RecyclingCommandPoolVK { buffers_(std::move(buffers)), recycler_(std::move(recycler)) {} - ~RecyclingCommandPoolVK() { + ~BackgroundCommandPoolVK() { auto const recycler = recycler_.lock(); if (!recycler) { return; @@ -35,7 +35,7 @@ class RecyclingCommandPoolVK { } private: - FML_DISALLOW_COPY_AND_ASSIGN(RecyclingCommandPoolVK); + FML_DISALLOW_COPY_AND_ASSIGN(BackgroundCommandPoolVK); vk::UniqueCommandPool pool_; std::vector buffers_; @@ -51,10 +51,10 @@ CommandPoolResourceVK::~CommandPoolResourceVK() { if (!recycler) { return; } - UniqueResourceVKT pool( + UniqueResourceVKT pool( context->GetResourceManager(), - RecyclingCommandPoolVK(std::move(pool_), std::move(collected_buffers_), - recycler)); + BackgroundCommandPoolVK(std::move(pool_), std::move(collected_buffers_), + recycler)); } // TODO(matanlurey): Return a status_or<> instead of {} when we have one. From bfe727e7987f4c49a895b2452e655f5b9a676c38 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 8 Sep 2023 23:44:57 -0700 Subject: [PATCH 06/26] Ah, the old forgot to finish implementing... Fun. --- .../backend/vulkan/command_encoder_vk.cc | 13 ++++-- .../backend/vulkan/command_pool_vk.cc | 46 +++++++++++++------ .../renderer/backend/vulkan/command_pool_vk.h | 17 +++---- .../renderer/backend/vulkan/context_vk.cc | 14 ------ impeller/renderer/backend/vulkan/context_vk.h | 6 --- 5 files changed, 48 insertions(+), 48 deletions(-) diff --git a/impeller/renderer/backend/vulkan/command_encoder_vk.cc b/impeller/renderer/backend/vulkan/command_encoder_vk.cc index e41681eda4c38..99e8e790fc25a 100644 --- a/impeller/renderer/backend/vulkan/command_encoder_vk.cc +++ b/impeller/renderer/backend/vulkan/command_encoder_vk.cc @@ -15,7 +15,7 @@ class TrackedObjectsVK { public: explicit TrackedObjectsVK( const std::weak_ptr& device_holder, - const std::shared_ptr& pool) + const std::shared_ptr& pool) : desc_pool_(device_holder) { if (!pool) { return; @@ -80,7 +80,7 @@ class TrackedObjectsVK { private: DescriptorPoolVK desc_pool_; // `shared_ptr` since command buffers have a link to the command pool. - std::shared_ptr pool_; + std::shared_ptr pool_; vk::UniqueCommandBuffer buffer_; std::set> tracked_objects_; std::set> tracked_buffers_; @@ -104,11 +104,18 @@ std::shared_ptr CommandEncoderFactoryVK::Create() { return nullptr; } auto& context_vk = ContextVK::Cast(*context); - auto tls_pool = context_vk.GetCommandPoolRecycler()->Get(); + FML_LOG(ERROR) << "CommandEncoderFactoryVK::Create() #1"; + auto recycler = context_vk.GetCommandPoolRecycler(); + if (!recycler) { + return nullptr; + } + FML_LOG(ERROR) << "CommandEncoderFactoryVK::Create() #2"; + auto tls_pool = recycler->Get(); if (!tls_pool) { return nullptr; } + FML_LOG(ERROR) << "CommandEncoderFactoryVK::Create() #3"; auto tracked_objects = std::make_shared( context_vk.GetDeviceHolder(), tls_pool); auto queue = context_vk.GetGraphicsQueue(); diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.cc b/impeller/renderer/backend/vulkan/command_pool_vk.cc index 70eaa30eecf6b..e5d66cb233818 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.cc +++ b/impeller/renderer/backend/vulkan/command_pool_vk.cc @@ -14,17 +14,15 @@ namespace impeller { +// Holds the command pool in a background thread, recyling it when not in use. class BackgroundCommandPoolVK { public: BackgroundCommandPoolVK(BackgroundCommandPoolVK&&) = default; explicit BackgroundCommandPoolVK( vk::UniqueCommandPool&& pool, - std::vector&& buffers, std::weak_ptr recycler) - : pool_(std::move(pool)), - buffers_(std::move(buffers)), - recycler_(std::move(recycler)) {} + : pool_(std::move(pool)), recycler_(std::move(recycler)) {} ~BackgroundCommandPoolVK() { auto const recycler = recycler_.lock(); @@ -38,11 +36,10 @@ class BackgroundCommandPoolVK { FML_DISALLOW_COPY_AND_ASSIGN(BackgroundCommandPoolVK); vk::UniqueCommandPool pool_; - std::vector buffers_; std::weak_ptr recycler_; }; -CommandPoolResourceVK::~CommandPoolResourceVK() { +CommandPoolVK::~CommandPoolVK() { auto const context = context_.lock(); if (!context) { return; @@ -53,12 +50,11 @@ CommandPoolResourceVK::~CommandPoolResourceVK() { } UniqueResourceVKT pool( context->GetResourceManager(), - BackgroundCommandPoolVK(std::move(pool_), std::move(collected_buffers_), - recycler)); + BackgroundCommandPoolVK(std::move(pool_), recycler)); } // TODO(matanlurey): Return a status_or<> instead of {} when we have one. -vk::UniqueCommandBuffer CommandPoolResourceVK::CreateBuffer() { +vk::UniqueCommandBuffer CommandPoolVK::CreateBuffer() { auto const context = context_.lock(); if (!context) { return {}; @@ -73,23 +69,27 @@ vk::UniqueCommandBuffer CommandPoolResourceVK::CreateBuffer() { if (result != vk::Result::eSuccess) { return {}; } - + FML_LOG(ERROR) << "CommandPoolResourceVK::CreateBuffer()"; return std::move(buffers[0]); } -void CommandPoolResourceVK::CollectBuffer(vk::UniqueCommandBuffer&& buffer) { +void CommandPoolVK::CollectBuffer(vk::UniqueCommandBuffer&& buffer) { + FML_LOG(ERROR) << "CommandPoolResourceVK::CollectBuffer()"; collected_buffers_.push_back(std::move(buffer)); } // Associates a resource with a thread and context. using CommandPoolMap = - std::unordered_map>; + std::unordered_map>; FML_THREAD_LOCAL fml::ThreadLocalUniquePtr resources_; // TODO(matanlurey): Return a status_or<> instead of nullptr when we have one. -std::shared_ptr CommandPoolRecyclerVK::Get() { +std::shared_ptr CommandPoolRecyclerVK::Get() { + FML_LOG(ERROR) << "CommandPoolRecyclerVK::Get() Enter"; + auto const strong_context = context_.lock(); if (!strong_context) { + FML_LOG(ERROR) << "CommandPoolRecyclerVK::Get() failed due to context."; return nullptr; } @@ -109,12 +109,14 @@ std::shared_ptr CommandPoolRecyclerVK::Get() { // Otherwise, create a new resource and return it. auto pool = Create(); if (!pool) { + FML_LOG(ERROR) << "CommandPoolRecyclerVK::Get() failed due to pool create."; return nullptr; } auto const resource = - std::make_shared(std::move(*pool), context_); + std::make_shared(std::move(*pool), context_); map[hash] = resource; + FML_LOG(ERROR) << "CommandPoolRecyclerVK::Get()"; return resource; } @@ -126,7 +128,20 @@ std::optional CommandPoolRecyclerVK::Create() { } // Otherwise, create a new one. - return std::nullopt; + auto context = context_.lock(); + if (!context) { + return std::nullopt; + } + vk::CommandPoolCreateInfo info; + info.setQueueFamilyIndex(context->GetGraphicsQueue()->GetIndex().family); + info.setFlags(vk::CommandPoolCreateFlagBits::eTransient); + + auto device = context->GetDevice(); + auto [result, pool] = device.createCommandPoolUnique(info); + if (result != vk::Result::eSuccess) { + return std::nullopt; + } + return std::move(pool); } std::optional CommandPoolRecyclerVK::Reuse() { @@ -143,6 +158,7 @@ std::optional CommandPoolRecyclerVK::Reuse() { } void CommandPoolRecyclerVK::Reclaim(vk::UniqueCommandPool&& pool) { + FML_LOG(INFO) << "CommandPoolRecyclerVK::Reclaim"; TRACE_EVENT0("impeller", "ReclaimCommandPool"); // FIXME: Assert that this is called on a background thread. diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.h b/impeller/renderer/backend/vulkan/command_pool_vk.h index 5c6009f40330a..2a385e7c3ed2c 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.h +++ b/impeller/renderer/backend/vulkan/command_pool_vk.h @@ -27,22 +27,19 @@ class CommandPoolRecyclerVK; /// @warning This class is not thread-safe. /// /// @see |CommandPoolRecyclerVK| -class CommandPoolResourceVK final { +class CommandPoolVK final { public: - CommandPoolResourceVK(CommandPoolResourceVK&&) = default; - ~CommandPoolResourceVK(); + CommandPoolVK(CommandPoolVK&&) = default; + ~CommandPoolVK(); /// @brief Creates a resource that manages the life of a command pool. /// /// @param[in] pool The command pool to manage. /// @param[in] recycler The context that will be notified on destruction. - explicit CommandPoolResourceVK(vk::UniqueCommandPool pool, - std::weak_ptr& context) + explicit CommandPoolVK(vk::UniqueCommandPool pool, + std::weak_ptr& context) : pool_(std::move(pool)), context_(context) {} - /// @brief Returns a reference to the underlying |vk::CommandPool|. - const vk::CommandPool& Get() const { return *pool_; } - /// @brief Creates and returns a new |vk::CommandBuffer|. /// /// @return Always returns a new |vk::CommandBuffer|, but if for any @@ -58,7 +55,7 @@ class CommandPoolResourceVK final { void CollectBuffer(vk::UniqueCommandBuffer&& buffer); private: - FML_DISALLOW_COPY_AND_ASSIGN(CommandPoolResourceVK); + FML_DISALLOW_COPY_AND_ASSIGN(CommandPoolVK); vk::UniqueCommandPool pool_; std::weak_ptr& context_; @@ -101,7 +98,7 @@ class CommandPoolRecyclerVK final /// @brief Gets a command pool for the current thread. /// /// @warning Returns a |nullptr| if a pool could not be created. - std::shared_ptr Get(); + std::shared_ptr Get(); /// @brief Returns a command pool to be reset on a background thread. /// diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index 7d70c26e548e2..834e443ebd017 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -528,18 +528,4 @@ ContextVK::CreateGraphicsCommandEncoderFactory() const { return std::make_unique(weak_from_this()); } -std::optional ContextVK::CreateCommandPool() const { - vk::CommandPoolCreateInfo pool_info; - - pool_info.queueFamilyIndex = GetGraphicsQueue()->GetIndex().family; - pool_info.flags = vk::CommandPoolCreateFlagBits::eTransient; - auto pool = GetDevice().createCommandPoolUnique(pool_info); - - if (pool.result != vk::Result::eSuccess) { - return std::nullopt; - } - - return std::move(pool.value); -} - } // namespace impeller diff --git a/impeller/renderer/backend/vulkan/context_vk.h b/impeller/renderer/backend/vulkan/context_vk.h index 1982970bdd469..144de15b26913 100644 --- a/impeller/renderer/backend/vulkan/context_vk.h +++ b/impeller/renderer/backend/vulkan/context_vk.h @@ -5,7 +5,6 @@ #pragma once #include -#include #include "flutter/fml/concurrent_message_loop.h" #include "flutter/fml/macros.h" @@ -175,11 +174,6 @@ class ContextVK final : public Context, bool sync_presentation_ = false; const uint64_t hash_; - mutable std::vector recycled_command_pools_; - mutable Mutex recycled_command_pools_mutex_; - std::optional CreateCommandPool() const; - std::optional ReuseCommandPool() const; - bool is_valid_ = false; ContextVK(); From 1af18786827841aab3130ca8411ae98c5948531b Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 8 Sep 2023 23:51:10 -0700 Subject: [PATCH 07/26] Back in action. Good night for now. --- impeller/renderer/backend/vulkan/command_pool_vk.cc | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.cc b/impeller/renderer/backend/vulkan/command_pool_vk.cc index e5d66cb233818..0b12eb66d1385 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.cc +++ b/impeller/renderer/backend/vulkan/command_pool_vk.cc @@ -21,8 +21,11 @@ class BackgroundCommandPoolVK { explicit BackgroundCommandPoolVK( vk::UniqueCommandPool&& pool, + std::vector&& buffers, std::weak_ptr recycler) - : pool_(std::move(pool)), recycler_(std::move(recycler)) {} + : pool_(std::move(pool)), + buffers_(std::move(buffers)), + recycler_(std::move(recycler)) {} ~BackgroundCommandPoolVK() { auto const recycler = recycler_.lock(); @@ -36,6 +39,11 @@ class BackgroundCommandPoolVK { FML_DISALLOW_COPY_AND_ASSIGN(BackgroundCommandPoolVK); vk::UniqueCommandPool pool_; + + // These are retained otherwise it is a Vulkan thread-safety violation + // because the buffers and the originating pool do not belong to the same + // thread. + std::vector buffers_; std::weak_ptr recycler_; }; @@ -50,7 +58,8 @@ CommandPoolVK::~CommandPoolVK() { } UniqueResourceVKT pool( context->GetResourceManager(), - BackgroundCommandPoolVK(std::move(pool_), recycler)); + BackgroundCommandPoolVK(std::move(pool_), std::move(collected_buffers_), + recycler)); } // TODO(matanlurey): Return a status_or<> instead of {} when we have one. From df64736eacf0dc2dd0c0cb87dc577514815d04af Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 11 Sep 2023 09:30:04 -0700 Subject: [PATCH 08/26] Remove FML_LOG statements. --- impeller/renderer/backend/vulkan/command_encoder_vk.cc | 3 --- impeller/renderer/backend/vulkan/command_pool_vk.cc | 8 -------- 2 files changed, 11 deletions(-) diff --git a/impeller/renderer/backend/vulkan/command_encoder_vk.cc b/impeller/renderer/backend/vulkan/command_encoder_vk.cc index 99e8e790fc25a..60e9b116f2e34 100644 --- a/impeller/renderer/backend/vulkan/command_encoder_vk.cc +++ b/impeller/renderer/backend/vulkan/command_encoder_vk.cc @@ -104,18 +104,15 @@ std::shared_ptr CommandEncoderFactoryVK::Create() { return nullptr; } auto& context_vk = ContextVK::Cast(*context); - FML_LOG(ERROR) << "CommandEncoderFactoryVK::Create() #1"; auto recycler = context_vk.GetCommandPoolRecycler(); if (!recycler) { return nullptr; } - FML_LOG(ERROR) << "CommandEncoderFactoryVK::Create() #2"; auto tls_pool = recycler->Get(); if (!tls_pool) { return nullptr; } - FML_LOG(ERROR) << "CommandEncoderFactoryVK::Create() #3"; auto tracked_objects = std::make_shared( context_vk.GetDeviceHolder(), tls_pool); auto queue = context_vk.GetGraphicsQueue(); diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.cc b/impeller/renderer/backend/vulkan/command_pool_vk.cc index 0b12eb66d1385..b3f0124520ec7 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.cc +++ b/impeller/renderer/backend/vulkan/command_pool_vk.cc @@ -78,12 +78,10 @@ vk::UniqueCommandBuffer CommandPoolVK::CreateBuffer() { if (result != vk::Result::eSuccess) { return {}; } - FML_LOG(ERROR) << "CommandPoolResourceVK::CreateBuffer()"; return std::move(buffers[0]); } void CommandPoolVK::CollectBuffer(vk::UniqueCommandBuffer&& buffer) { - FML_LOG(ERROR) << "CommandPoolResourceVK::CollectBuffer()"; collected_buffers_.push_back(std::move(buffer)); } @@ -94,11 +92,8 @@ FML_THREAD_LOCAL fml::ThreadLocalUniquePtr resources_; // TODO(matanlurey): Return a status_or<> instead of nullptr when we have one. std::shared_ptr CommandPoolRecyclerVK::Get() { - FML_LOG(ERROR) << "CommandPoolRecyclerVK::Get() Enter"; - auto const strong_context = context_.lock(); if (!strong_context) { - FML_LOG(ERROR) << "CommandPoolRecyclerVK::Get() failed due to context."; return nullptr; } @@ -118,14 +113,12 @@ std::shared_ptr CommandPoolRecyclerVK::Get() { // Otherwise, create a new resource and return it. auto pool = Create(); if (!pool) { - FML_LOG(ERROR) << "CommandPoolRecyclerVK::Get() failed due to pool create."; return nullptr; } auto const resource = std::make_shared(std::move(*pool), context_); map[hash] = resource; - FML_LOG(ERROR) << "CommandPoolRecyclerVK::Get()"; return resource; } @@ -167,7 +160,6 @@ std::optional CommandPoolRecyclerVK::Reuse() { } void CommandPoolRecyclerVK::Reclaim(vk::UniqueCommandPool&& pool) { - FML_LOG(INFO) << "CommandPoolRecyclerVK::Reclaim"; TRACE_EVENT0("impeller", "ReclaimCommandPool"); // FIXME: Assert that this is called on a background thread. From 8d22f908e57e5def6756685764a9b18ddfe5f0be Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 11 Sep 2023 13:40:19 -0700 Subject: [PATCH 09/26] Rewrite comment, fix race condition in destruction of ContextVK. --- impeller/renderer/backend/vulkan/command_pool_vk.cc | 6 +++--- impeller/renderer/backend/vulkan/context_vk.cc | 8 ++++++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.cc b/impeller/renderer/backend/vulkan/command_pool_vk.cc index b3f0124520ec7..d2da181e6d2e9 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.cc +++ b/impeller/renderer/backend/vulkan/command_pool_vk.cc @@ -40,9 +40,9 @@ class BackgroundCommandPoolVK { vk::UniqueCommandPool pool_; - // These are retained otherwise it is a Vulkan thread-safety violation - // because the buffers and the originating pool do not belong to the same - // thread. + // These are retained because the destructor of the C++ UniqueCommandBuffer + // 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 buffers_; std::weak_ptr recycler_; }; diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index 834e443ebd017..94c12a27c34a6 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -491,6 +491,14 @@ ContextVK::GetConcurrentWorkerTaskRunner() const { } void ContextVK::Shutdown() { + // There are multiple objects, for example |CommandPoolVK|, that in their + // destructors make a strong reference to |ContextVK|. Resetting these shared + // pointers ensures that cleanup happens in a correct order. + // + // tl;dr: Without it, we get thread::join failures on shutdown. + resource_manager_.reset(); + fence_waiter_.reset(); + raster_message_loop_->Terminate(); } From 12d456da4064b847cdb6aeb5d4826c547b582e6a Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 11 Sep 2023 13:46:21 -0700 Subject: [PATCH 10/26] Restore commented-out test. --- .../backend/vulkan/context_vk_unittests.cc | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/impeller/renderer/backend/vulkan/context_vk_unittests.cc b/impeller/renderer/backend/vulkan/context_vk_unittests.cc index c62fece1d4010..d659eefaa36fe 100644 --- a/impeller/renderer/backend/vulkan/context_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/context_vk_unittests.cc @@ -11,19 +11,18 @@ namespace impeller { namespace testing { TEST(ContextVKTest, DeletesCommandPools) { - // std::weak_ptr weak_context; - // std::weak_ptr weak_pool; - // { - // std::shared_ptr context = CreateMockVulkanContext(); - // std::shared_ptr pool = - // CommandPoolVK::GetThreadLocal(context.get()); - // weak_pool = pool; - // weak_context = context; - // ASSERT_TRUE(weak_pool.lock()); - // ASSERT_TRUE(weak_context.lock()); - // } - // ASSERT_FALSE(weak_pool.lock()); - // ASSERT_FALSE(weak_context.lock()); + std::weak_ptr weak_context; + std::weak_ptr weak_pool; + { + auto const context = CreateMockVulkanContext(); + auto const pool = context->GetCommandPoolRecycler()->Get(); + weak_pool = pool; + weak_context = context; + ASSERT_TRUE(weak_pool.lock()); + ASSERT_TRUE(weak_context.lock()); + } + ASSERT_FALSE(weak_pool.lock()); + ASSERT_FALSE(weak_context.lock()); } TEST(ContextVKTest, DeletePipelineAfterContext) { From 443ac713c7d1281d4b1b358ded49f09fe7a2161e Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 11 Sep 2023 15:29:00 -0700 Subject: [PATCH 11/26] Add unit tests, fix bugs. --- impeller/renderer/backend/vulkan/BUILD.gn | 1 + .../backend/vulkan/command_pool_vk.cc | 15 ++- .../vulkan/command_pool_vk_unittests.cc | 107 ++++++++++++++++++ .../backend/vulkan/test/mock_vulkan.cc | 13 +++ 4 files changed, 131 insertions(+), 5 deletions(-) create mode 100644 impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc diff --git a/impeller/renderer/backend/vulkan/BUILD.gn b/impeller/renderer/backend/vulkan/BUILD.gn index ae2c71be87ca8..eea7b9f48b118 100644 --- a/impeller/renderer/backend/vulkan/BUILD.gn +++ b/impeller/renderer/backend/vulkan/BUILD.gn @@ -10,6 +10,7 @@ impeller_component("vulkan_unittests") { sources = [ "blit_command_vk_unittests.cc", "command_encoder_vk_unittests.cc", + "command_pool_vk_unittests.cc", "context_vk_unittests.cc", "pass_bindings_cache_unittests.cc", "resource_manager_vk_unittests.cc", diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.cc b/impeller/renderer/backend/vulkan/command_pool_vk.cc index d2da181e6d2e9..677c0be1f755e 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.cc +++ b/impeller/renderer/backend/vulkan/command_pool_vk.cc @@ -29,9 +29,15 @@ class BackgroundCommandPoolVK { ~BackgroundCommandPoolVK() { 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_)); } @@ -56,6 +62,7 @@ CommandPoolVK::~CommandPoolVK() { if (!recycler) { return; } + UniqueResourceVKT pool( context->GetResourceManager(), BackgroundCommandPoolVK(std::move(pool_), std::move(collected_buffers_), @@ -103,10 +110,9 @@ std::shared_ptr CommandPoolRecyclerVK::Get() { resources = new CommandPoolMap(); resources_.reset(resources); } - auto map = *resources; auto const hash = strong_context->GetHash(); - auto const it = map.find(hash); - if (it != map.end()) { + auto const it = resources->find(hash); + if (it != resources->end()) { return it->second; } @@ -118,7 +124,7 @@ std::shared_ptr CommandPoolRecyclerVK::Get() { auto const resource = std::make_shared(std::move(*pool), context_); - map[hash] = resource; + resources->emplace(hash, resource); return resource; } @@ -161,7 +167,6 @@ std::optional CommandPoolRecyclerVK::Reuse() { void CommandPoolRecyclerVK::Reclaim(vk::UniqueCommandPool&& pool) { TRACE_EVENT0("impeller", "ReclaimCommandPool"); - // FIXME: Assert that this is called on a background thread. // Reset the pool on a background thread. auto strong_context = context_.lock(); diff --git a/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc b/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc new file mode 100644 index 0000000000000..6059c25166c3e --- /dev/null +++ b/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc @@ -0,0 +1,107 @@ +// 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 "flutter/testing/testing.h" // IWYU pragma: keep. +#include "fml/synchronization/waitable_event.h" +#include "impeller/renderer/backend/vulkan/resource_manager_vk.h" +#include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" + +namespace impeller { +namespace testing { + +TEST(CommandPoolRecyclerVKTest, GetsACommandPoolPerThread) { + auto const context = CreateMockVulkanContext(); + + // Record the memory location of each pointer to a command pool. + int* pool1 = nullptr; + int* pool2 = nullptr; + + // Create a command pool in two threads and record the memory location. + std::thread thread1([&]() { + auto const pool = context->GetCommandPoolRecycler()->Get(); + pool1 = reinterpret_cast(pool.get()); + }); + + std::thread thread2([&]() { + auto const pool = context->GetCommandPoolRecycler()->Get(); + pool2 = reinterpret_cast(pool.get()); + }); + + thread1.join(); + thread2.join(); + + // The two command pools should be different. + EXPECT_NE(pool1, pool2); +} + +TEST(CommandPoolRecyclerVKTest, GetsTheSameCommandPoolOnSameThread) { + auto const context = CreateMockVulkanContext(); + + auto const pool1 = context->GetCommandPoolRecycler()->Get(); + auto const pool2 = context->GetCommandPoolRecycler()->Get(); + + // The two command pools should be the same. + EXPECT_EQ(pool1.get(), pool2.get()); +} + +namespace { + +// Invokes the provided callback when the destructor is called. +// +// Can be moved, but not copied. +class DeathRattle final { + public: + explicit DeathRattle(std::function callback) + : callback_(std::move(callback)) {} + + DeathRattle(DeathRattle&&) = default; + DeathRattle& operator=(DeathRattle&&) = default; + + ~DeathRattle() { callback_(); } + + private: + std::function callback_; +}; + +} // namespace + +TEST(CommandPoolRecyclerVKTest, ReclaimMakesCommandPoolAvailable) { + auto const context = CreateMockVulkanContext(); + + { + // Fetch a pool (which will be created). + auto const recycler = context->GetCommandPoolRecycler(); + auto const pool = recycler->Get(); + + // This normally is called at the end of a frame. + recycler->Recycle(); + } + + // 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 = DeathRattle([&waiter]() { waiter.Signal(); }); + { + UniqueResourceVKT resource(context->GetResourceManager(), + std::move(rattle)); + } + waiter.Wait(); + + // On another thread explicitly, request a new pool. + std::thread thread([&]() { + auto const pool = context->GetCommandPoolRecycler()->Get(); + EXPECT_NE(pool.get(), nullptr); + }); + + thread.join(); + + // Now check that we only ever created one pool. + auto const called = GetMockVulkanFunctions(context->GetDevice()); + EXPECT_EQ(std::count(called->begin(), called->end(), "vkCreateCommandPool"), + 1u); +} + +} // namespace testing +} // namespace impeller diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index c2ee56e309ff2..39c035fbb1054 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" +#include "vulkan/vulkan_core.h" namespace impeller { namespace testing { @@ -16,6 +17,8 @@ struct MockCommandBuffer { std::shared_ptr> called_functions_; }; +struct MockCommandPool {}; + struct MockDevice { MockDevice() : called_functions_(new std::vector()) {} MockCommandBuffer* NewCommandBuffer() { @@ -156,10 +159,18 @@ VkResult vkCreateCommandPool(VkDevice device, const VkCommandPoolCreateInfo* pCreateInfo, const VkAllocationCallbacks* pAllocator, VkCommandPool* pCommandPool) { + MockDevice* mock_device = reinterpret_cast(device); + mock_device->called_functions_->push_back("vkCreateCommandPool"); *pCommandPool = reinterpret_cast(0xc0de0001); return VK_SUCCESS; } +VkResult vkResetCommandPool(VkDevice device, + VkCommandPool commandPool, + VkCommandPoolResetFlags flags) { + return VK_SUCCESS; +} + VkResult vkAllocateCommandBuffers( VkDevice device, const VkCommandBufferAllocateInfo* pAllocateInfo, @@ -418,6 +429,8 @@ PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance, return (PFN_vkVoidFunction)vkCreatePipelineCache; } else if (strcmp("vkCreateCommandPool", pName) == 0) { return (PFN_vkVoidFunction)vkCreateCommandPool; + } else if (strcmp("vkResetCommandPool", pName) == 0) { + return (PFN_vkVoidFunction)vkResetCommandPool; } else if (strcmp("vkAllocateCommandBuffers", pName) == 0) { return (PFN_vkVoidFunction)vkAllocateCommandBuffers; } else if (strcmp("vkBeginCommandBuffer", pName) == 0) { From 2ca3c6725d8ac5728e7f4914e09dd640e20f0925 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 11 Sep 2023 15:50:58 -0700 Subject: [PATCH 12/26] Fix licenses. --- ci/licenses_golden/excluded_files | 1 + 1 file changed, 1 insertion(+) diff --git a/ci/licenses_golden/excluded_files b/ci/licenses_golden/excluded_files index 6eafa7868a51c..3cadbbcbc7863 100644 --- a/ci/licenses_golden/excluded_files +++ b/ci/licenses_golden/excluded_files @@ -152,6 +152,7 @@ ../../../flutter/impeller/playground ../../../flutter/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc ../../../flutter/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc +../../../flutter/impeller/renderer/backend/vulkan/command_pool_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 From adb61c1b68aa376a7a58fab9dd7ca1ac6a4c075a Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Sep 2023 11:17:12 -0700 Subject: [PATCH 13/26] It appears... to be working. --- .../vulkan/command_pool_vk_unittests.cc | 6 ++++ .../backend/vulkan/resource_manager_vk.cc | 5 ++++ .../vulkan/resource_manager_vk_unittests.cc | 30 ++++++++++++++++++- .../vulkan/test/mock_vulkan_unittests.cc | 5 ++-- 4 files changed, 42 insertions(+), 4 deletions(-) diff --git a/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc b/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc index 6059c25166c3e..2df6a526e0215 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc @@ -33,6 +33,8 @@ TEST(CommandPoolRecyclerVKTest, GetsACommandPoolPerThread) { // The two command pools should be different. EXPECT_NE(pool1, pool2); + + context->Shutdown(); } TEST(CommandPoolRecyclerVKTest, GetsTheSameCommandPoolOnSameThread) { @@ -43,6 +45,8 @@ TEST(CommandPoolRecyclerVKTest, GetsTheSameCommandPoolOnSameThread) { // The two command pools should be the same. EXPECT_EQ(pool1.get(), pool2.get()); + + context->Shutdown(); } namespace { @@ -101,6 +105,8 @@ TEST(CommandPoolRecyclerVKTest, ReclaimMakesCommandPoolAvailable) { auto const called = GetMockVulkanFunctions(context->GetDevice()); EXPECT_EQ(std::count(called->begin(), called->end(), "vkCreateCommandPool"), 1u); + + context->Shutdown(); } } // namespace testing diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk.cc b/impeller/renderer/backend/vulkan/resource_manager_vk.cc index 382f7aa715e7c..f981321c7115e 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk.cc +++ b/impeller/renderer/backend/vulkan/resource_manager_vk.cc @@ -22,6 +22,11 @@ std::shared_ptr ResourceManagerVK::Create() { ResourceManagerVK::ResourceManagerVK() : waiter_([&]() { Start(); }) {} ResourceManagerVK::~ResourceManagerVK() { + FML_DCHECK(waiter_.get_id() != std::this_thread::get_id()) + << "The ResourceManager being destructed on its own spawned thread is a " + << "sign that ContextVK was not properly destroyed. A usual fix for this " + << "is to ensure that ContextVK is shutdown (i.e. context->Shutdown()) " + "before the ResourceManager is destroyed (i.e. at the end of a test)."; Terminate(); waiter_.join(); } diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc index 30df654d3dac0..fd5f10d8e0f70 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc @@ -6,7 +6,6 @@ #include #include #include -#include "fml/logging.h" #include "fml/synchronization/waitable_event.h" #include "gtest/gtest.h" #include "impeller/renderer/backend/vulkan/resource_manager_vk.h" @@ -75,5 +74,34 @@ TEST(ResourceManagerVKTest, TerminatesWhenOutOfScope) { EXPECT_EQ(manager.lock(), nullptr); } +TEST(ResourceManagerVKTest, IsThreadSafe) { + // In a typical app, there is a single ResourceManagerVK per app, shared b/w + // threads. + // + // This test ensures that the ResourceManagerVK is thread-safe. + std::weak_ptr manager; + + { + auto const manager = ResourceManagerVK::Create(); + + // Spawn two threads, and have them both put resources into the manager. + struct MockResource {}; + + std::thread thread1([&manager]() { + UniqueResourceVKT(manager, MockResource{}); + }); + + std::thread thread2([&manager]() { + UniqueResourceVKT(manager, MockResource{}); + }); + + thread1.join(); + thread2.join(); + } + + // The thread should have terminated. + EXPECT_EQ(manager.lock(), nullptr); +} + } // namespace testing } // namespace impeller diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc index 05cc7682e004d..5e632b95bf8ae 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc +++ b/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" namespace impeller { @@ -18,12 +17,12 @@ TEST(MockVulkanContextTest, IsThreadSafe) { // Spawn two threads, and have them create a CommandPoolVK each. std::thread thread1([&context]() { - auto const pool = CommandPoolVK::GetThreadLocal(context.get()); + auto const pool = context->GetCommandPoolRecycler()->Get(); EXPECT_TRUE(pool); }); std::thread thread2([&context]() { - auto const pool = CommandPoolVK::GetThreadLocal(context.get()); + auto const pool = context->GetCommandPoolRecycler()->Get(); EXPECT_TRUE(pool); }); From 7251349e6995f21f82e508fd8fed03baba4f56c7 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Sep 2023 12:02:32 -0700 Subject: [PATCH 14/26] Fix race condition by using context->Shutdown(). --- .../renderer/backend/vulkan/command_encoder_vk_unittests.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc b/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc index 0fa38fdb82985..2563bfe8b719c 100644 --- a/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc @@ -4,10 +4,8 @@ #include -#include "flutter/fml/synchronization/count_down_latch.h" -#include "flutter/testing/testing.h" +#include "flutter/testing/testing.h" // IWYU pragma: keep #include "impeller/renderer/backend/vulkan/command_encoder_vk.h" -#include "impeller/renderer/backend/vulkan/fence_waiter_vk.h" #include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" namespace impeller { @@ -26,6 +24,7 @@ TEST(CommandEncoderVKTest, DeleteEncoderAfterThreadDies) { encoder = factory.Create(); }); thread.join(); + context->Shutdown(); } auto destroy_pool = std::find(called_functions->begin(), called_functions->end(), @@ -60,6 +59,7 @@ TEST(CommandEncoderVKTest, CleanupAfterSubmit) { wait_for_thread_join.Signal(); wait_for_submit.Wait(); called_functions = GetMockVulkanFunctions(context->GetDevice()); + context->Shutdown(); } auto destroy_pool = std::find(called_functions->begin(), called_functions->end(), From 2a3922bb9c2ed7b9c01d9378144f0b588faba875 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Sep 2023 12:07:29 -0700 Subject: [PATCH 15/26] Address feedback, fix race conditions. --- .../backend/vulkan/command_encoder_vk_unittests.cc | 1 + .../renderer/backend/vulkan/command_pool_vk.cc | 7 +++++-- impeller/renderer/backend/vulkan/command_pool_vk.h | 14 +++++++------- .../renderer/backend/vulkan/test/mock_vulkan.cc | 1 + 4 files changed, 14 insertions(+), 9 deletions(-) diff --git a/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc b/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc index 2563bfe8b719c..f68945ab4af8e 100644 --- a/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc @@ -5,6 +5,7 @@ #include #include "flutter/testing/testing.h" // IWYU pragma: keep +#include "fml/synchronization/waitable_event.h" #include "impeller/renderer/backend/vulkan/command_encoder_vk.h" #include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.cc b/impeller/renderer/backend/vulkan/command_pool_vk.cc index 677c0be1f755e..944c46e053639 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.cc +++ b/impeller/renderer/backend/vulkan/command_pool_vk.cc @@ -3,13 +3,16 @@ // found in the LICENSE file. #include "impeller/renderer/backend/vulkan/command_pool_vk.h" + #include #include #include + #include "fml/macros.h" #include "fml/thread_local.h" #include "fml/trace_event.h" #include "impeller/renderer/backend/vulkan/resource_manager_vk.h" +#include "impeller/renderer/backend/vulkan/vk.h" // IWYU pragma: keep. #include "vulkan/vulkan_structs.hpp" namespace impeller { @@ -70,7 +73,7 @@ CommandPoolVK::~CommandPoolVK() { } // TODO(matanlurey): Return a status_or<> instead of {} when we have one. -vk::UniqueCommandBuffer CommandPoolVK::CreateBuffer() { +vk::UniqueCommandBuffer CommandPoolVK::CreateCommandBuffer() { auto const context = context_.lock(); if (!context) { return {}; @@ -88,7 +91,7 @@ vk::UniqueCommandBuffer CommandPoolVK::CreateBuffer() { return std::move(buffers[0]); } -void CommandPoolVK::CollectBuffer(vk::UniqueCommandBuffer&& buffer) { +void CommandPoolVK::CollectCommandBuffer(vk::UniqueCommandBuffer&& buffer) { collected_buffers_.push_back(std::move(buffer)); } diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.h b/impeller/renderer/backend/vulkan/command_pool_vk.h index 2a385e7c3ed2c..f1d9d39e6b519 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.h +++ b/impeller/renderer/backend/vulkan/command_pool_vk.h @@ -45,14 +45,14 @@ class CommandPoolVK final { /// @return Always returns a new |vk::CommandBuffer|, but if for any /// reason a valid command buffer could not be created, it will be /// a `{}` default instance (i.e. while being torn down). - vk::UniqueCommandBuffer CreateBuffer(); + vk::UniqueCommandBuffer CreateCommandBuffer(); /// @brief Collects the given |vk::CommandBuffer| to be retained. /// /// @param[in] buffer The |vk::CommandBuffer| to collect. /// /// @see |GarbageCollectBuffersIfAble| - void CollectBuffer(vk::UniqueCommandBuffer&& buffer); + void CollectCommandBuffer(vk::UniqueCommandBuffer&& buffer); private: FML_DISALLOW_COPY_AND_ASSIGN(CommandPoolVK); @@ -109,7 +109,10 @@ class CommandPoolRecyclerVK final void Recycle(); private: - FML_DISALLOW_COPY_AND_ASSIGN(CommandPoolRecyclerVK); + std::weak_ptr context_; + + Mutex recycled_mutex_; + std::vector recycled_ IPLR_GUARDED_BY(recycled_mutex_); /// @brief Creates a new |vk::CommandPool|. /// @@ -121,10 +124,7 @@ class CommandPoolRecyclerVK final /// @returns Returns a |std::nullopt| if a pool was not available. std::optional Reuse(); - std::weak_ptr context_; - - Mutex recycled_mutex_; - std::vector recycled_ IPLR_GUARDED_BY(recycled_mutex_); + FML_DISALLOW_COPY_AND_ASSIGN(CommandPoolRecyclerVK); }; } // namespace impeller diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index a76f4d35d8791..fe6a8a1e6ba00 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -6,6 +6,7 @@ #include #include "fml/macros.h" #include "impeller/base/thread_safety.h" +#include "impeller/renderer/backend/vulkan/vk.h" // IWYU pragma: keep. namespace impeller { namespace testing { From 74438e6c48b08d3dca715589d81b3b2a252cc58d Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Sep 2023 12:13:49 -0700 Subject: [PATCH 16/26] Fix renames. --- impeller/renderer/backend/vulkan/command_encoder_vk.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/impeller/renderer/backend/vulkan/command_encoder_vk.cc b/impeller/renderer/backend/vulkan/command_encoder_vk.cc index 60e9b116f2e34..8f973bd0fa4d1 100644 --- a/impeller/renderer/backend/vulkan/command_encoder_vk.cc +++ b/impeller/renderer/backend/vulkan/command_encoder_vk.cc @@ -20,7 +20,7 @@ class TrackedObjectsVK { if (!pool) { return; } - auto buffer = pool->CreateBuffer(); + auto buffer = pool->CreateCommandBuffer(); if (!buffer) { return; } @@ -33,7 +33,7 @@ class TrackedObjectsVK { if (!buffer_) { return; } - pool_->CollectBuffer(std::move(buffer_)); + pool_->CollectCommandBuffer(std::move(buffer_)); } bool IsValid() const { return is_valid_; } From a4c6c1d3d2f4412ef76789535542cb08778aad84 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Sep 2023 13:22:24 -0700 Subject: [PATCH 17/26] Re-order ptr resets. --- impeller/renderer/backend/vulkan/context_vk.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index 94c12a27c34a6..fc5e280bb912a 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -496,8 +496,8 @@ void ContextVK::Shutdown() { // pointers ensures that cleanup happens in a correct order. // // tl;dr: Without it, we get thread::join failures on shutdown. - resource_manager_.reset(); fence_waiter_.reset(); + resource_manager_.reset(); raster_message_loop_->Terminate(); } From 852482f6e6d56acf162544a156ad0122a74a0f74 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Sep 2023 13:47:42 -0700 Subject: [PATCH 18/26] Re-order fence waiter loop to termintae last. --- impeller/renderer/backend/vulkan/fence_waiter_vk.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc index 73ba0879dd3c2..174e48987998a 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc @@ -106,10 +106,6 @@ void FenceWaiterVK::Main() { lock.unlock(); - if (terminate) { - break; - } - // Check if the context had died in the meantime. auto device_holder = device_holder_.lock(); if (!device_holder) { @@ -170,6 +166,11 @@ void FenceWaiterVK::Main() { // Erase the erased entries which will invoke callbacks. erased_entries.clear(); // Bit redundant because of scope but hey. } + + // This is the last check because we want to wait for/clear fences first. + if (terminate) { + break; + } } } From c27bedddcbc96aa1dd0fab187ce28ed5641994b5 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Sep 2023 13:57:05 -0700 Subject: [PATCH 19/26] Address feedback, tweak the waiter. --- impeller/renderer/backend/vulkan/command_pool_vk.cc | 8 ++++---- impeller/renderer/backend/vulkan/command_pool_vk.h | 10 +++++++--- .../backend/vulkan/command_pool_vk_unittests.cc | 2 +- impeller/renderer/backend/vulkan/context_vk.cc | 2 +- impeller/renderer/backend/vulkan/fence_waiter_vk.cc | 4 ++-- impeller/renderer/backend/vulkan/surface_context_vk.cc | 2 +- 6 files changed, 16 insertions(+), 12 deletions(-) diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.cc b/impeller/renderer/backend/vulkan/command_pool_vk.cc index 944c46e053639..218df9ef343f0 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.cc +++ b/impeller/renderer/backend/vulkan/command_pool_vk.cc @@ -157,7 +157,7 @@ std::optional CommandPoolRecyclerVK::Create() { std::optional CommandPoolRecyclerVK::Reuse() { // If there are no recycled pools, return nullopt. - Lock _(recycled_mutex_); + Lock recycled_lock(recycled_mutex_); if (recycled_.empty()) { return std::nullopt; } @@ -180,16 +180,16 @@ void CommandPoolRecyclerVK::Reclaim(vk::UniqueCommandPool&& pool) { device.resetCommandPool(pool.get()); // Move the pool to the recycled list. - Lock _(recycled_mutex_); + Lock recycled_lock(recycled_mutex_); recycled_.push_back(std::move(pool)); } CommandPoolRecyclerVK::~CommandPoolRecyclerVK() { // Ensure all recycled pools are reclaimed before this is destroyed. - Recycle(); + Dispose(); } -void CommandPoolRecyclerVK::Recycle() { +void CommandPoolRecyclerVK::Dispose() { auto const resources = resources_.get(); if (!resources) { return; diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.h b/impeller/renderer/backend/vulkan/command_pool_vk.h index f1d9d39e6b519..de7aa8b78ffcc 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.h +++ b/impeller/renderer/backend/vulkan/command_pool_vk.h @@ -74,10 +74,14 @@ class CommandPoolVK final { /// A single instance should be created per |ContextVK|. /// /// Every "frame", a single |CommandPoolResourceVk| is made available for each -/// thread that calls |Get|. After calling |Recycle|, the current thread's pool +/// thread that calls |Get|. After calling |Dispose|, the current thread's pool /// is moved to a background thread, reset, and made available for the next time /// |Get| is called and needs to create a command pool. /// +/// Commands in the command pool are not necessarily done executing when the +/// pool is recycled, when all references are dropped to the pool, they are +/// reset and returned to the pool of available pools. +/// /// @note This class is thread-safe. /// /// @see |vk::CommandPoolResourceVk| @@ -102,11 +106,11 @@ class CommandPoolRecyclerVK final /// @brief Returns a command pool to be reset on a background thread. /// - /// @param[in] pool The pool to recycle. + /// @param[in] pool The pool to recycler. void Reclaim(vk::UniqueCommandPool&& pool); /// @brief Clears all recycled command pools to let them be reclaimed. - void Recycle(); + void Dispose(); private: std::weak_ptr context_; diff --git a/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc b/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc index 2df6a526e0215..8088ae0c2d647 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc @@ -79,7 +79,7 @@ TEST(CommandPoolRecyclerVKTest, ReclaimMakesCommandPoolAvailable) { auto const pool = recycler->Get(); // This normally is called at the end of a frame. - recycler->Recycle(); + recycler->Dispose(); } // Add something to the resource manager and have it notify us when it's diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index fc5e280bb912a..6de7dcf3bd42b 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -113,7 +113,7 @@ ContextVK::~ContextVK() { [[maybe_unused]] auto result = device_holder_->device->waitIdle(); } if (command_pool_recycler_) { - command_pool_recycler_.get()->Recycle(); + command_pool_recycler_.get()->Dispose(); } } diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc index 174e48987998a..c9d2bf8a82765 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc @@ -63,6 +63,7 @@ bool FenceWaiterVK::IsValid() const { bool FenceWaiterVK::AddFence(vk::UniqueFence fence, const fml::closure& callback) { + FML_DCHECK(!terminate_); TRACE_EVENT0("flutter", "FenceWaiterVK::AddFence"); if (!IsValid() || !fence || !callback) { return false; @@ -102,8 +103,6 @@ void FenceWaiterVK::Main() { // mutex. WaitSet wait_set = wait_set_; - const auto terminate = terminate_; - lock.unlock(); // Check if the context had died in the meantime. @@ -168,6 +167,7 @@ void FenceWaiterVK::Main() { } // This is the last check because we want to wait for/clear fences first. + const auto terminate = terminate_; if (terminate) { break; } diff --git a/impeller/renderer/backend/vulkan/surface_context_vk.cc b/impeller/renderer/backend/vulkan/surface_context_vk.cc index f2b5dd0e6997e..7faef6507242a 100644 --- a/impeller/renderer/backend/vulkan/surface_context_vk.cc +++ b/impeller/renderer/backend/vulkan/surface_context_vk.cc @@ -79,7 +79,7 @@ std::unique_ptr SurfaceContextVK::AcquireNextSurface() { if (allocator) { allocator->DidAcquireSurfaceFrame(); } - parent_->GetCommandPoolRecycler()->Recycle(); + parent_->GetCommandPoolRecycler()->Dispose(); return surface; } From 15472ac31624e89bc2430eedb0f30686873bb79e Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Sep 2023 15:53:44 -0700 Subject: [PATCH 20/26] Tweak fence waiter and try on CI again. --- .../renderer/backend/vulkan/fence_waiter_vk.cc | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc index c9d2bf8a82765..a96b97c6be40b 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc @@ -63,7 +63,6 @@ bool FenceWaiterVK::IsValid() const { bool FenceWaiterVK::AddFence(vk::UniqueFence fence, const fml::closure& callback) { - FML_DCHECK(!terminate_); TRACE_EVENT0("flutter", "FenceWaiterVK::AddFence"); if (!IsValid() || !fence || !callback) { return false; @@ -103,6 +102,8 @@ void FenceWaiterVK::Main() { // mutex. WaitSet wait_set = wait_set_; + const auto terminate = terminate_; + lock.unlock(); // Check if the context had died in the meantime. @@ -111,7 +112,7 @@ void FenceWaiterVK::Main() { break; } - const auto& device = device_holder->GetDevice(); + auto const device = device_holder->GetDevice(); // Wait for one or more fences to be signaled. Any additional fences added // to the waiter will be serviced in the next pass. If a fence that is going @@ -134,6 +135,10 @@ void FenceWaiterVK::Main() { break; } + if (terminate) { + break; + } + // One or more fences have been signaled. Find out which ones and update // their signaled statuses. { @@ -165,12 +170,6 @@ void FenceWaiterVK::Main() { // Erase the erased entries which will invoke callbacks. erased_entries.clear(); // Bit redundant because of scope but hey. } - - // This is the last check because we want to wait for/clear fences first. - const auto terminate = terminate_; - if (terminate) { - break; - } } } From 82a8944de7d41f3632574a2cdf8ced1008d68a43 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Sep 2023 16:38:37 -0700 Subject: [PATCH 21/26] Try another CI tweak. --- impeller/renderer/backend/vulkan/context_vk.cc | 2 +- impeller/renderer/backend/vulkan/fence_waiter_vk.cc | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index 6de7dcf3bd42b..51f30a6a59a15 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -496,7 +496,7 @@ void ContextVK::Shutdown() { // pointers ensures that cleanup happens in a correct order. // // tl;dr: Without it, we get thread::join failures on shutdown. - fence_waiter_.reset(); + fence_waiter_->Terminate(); resource_manager_.reset(); raster_message_loop_->Terminate(); diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc index a96b97c6be40b..73ba0879dd3c2 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc @@ -106,13 +106,17 @@ void FenceWaiterVK::Main() { lock.unlock(); + if (terminate) { + break; + } + // Check if the context had died in the meantime. auto device_holder = device_holder_.lock(); if (!device_holder) { break; } - auto const device = device_holder->GetDevice(); + const auto& device = device_holder->GetDevice(); // Wait for one or more fences to be signaled. Any additional fences added // to the waiter will be serviced in the next pass. If a fence that is going @@ -135,10 +139,6 @@ void FenceWaiterVK::Main() { break; } - if (terminate) { - break; - } - // One or more fences have been signaled. Find out which ones and update // their signaled statuses. { From d2ab1c4ab243b6185a33a7a8cba0b511c8cf1032 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Sep 2023 17:13:20 -0700 Subject: [PATCH 22/26] Command encoder tests are passing. --- .../renderer/backend/vulkan/command_encoder_vk_unittests.cc | 1 + impeller/renderer/backend/vulkan/context_vk.cc | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc b/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc index f68945ab4af8e..12b16cb321dcf 100644 --- a/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc @@ -62,6 +62,7 @@ TEST(CommandEncoderVKTest, CleanupAfterSubmit) { called_functions = GetMockVulkanFunctions(context->GetDevice()); context->Shutdown(); } + auto destroy_pool = std::find(called_functions->begin(), called_functions->end(), "vkDestroyCommandPool"); diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index 51f30a6a59a15..6de7dcf3bd42b 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -496,7 +496,7 @@ void ContextVK::Shutdown() { // pointers ensures that cleanup happens in a correct order. // // tl;dr: Without it, we get thread::join failures on shutdown. - fence_waiter_->Terminate(); + fence_waiter_.reset(); resource_manager_.reset(); raster_message_loop_->Terminate(); From 869d5c01989e1fcb0bd8e4a896fd829a09d28acb Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Sep 2023 17:19:28 -0700 Subject: [PATCH 23/26] Try another variant. --- .../backend/vulkan/fence_waiter_vk.cc | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc index 73ba0879dd3c2..7f3b4c047dab2 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc @@ -107,6 +107,33 @@ void FenceWaiterVK::Main() { lock.unlock(); if (terminate) { + // This code is copied because checking for the terminate flag after the + // fence waiting appears insufficient. It's possible to refactor this into + // a method and re-use it, but it's not clear if that might have other + // unintended consequences. + auto fences = GetFencesForWaitSet(wait_set); + if (fences.empty()) { + break; + } + + // If there are fences to wait on, wait for them to be signaled. + auto device_holder = device_holder_.lock(); + if (!device_holder) { + break; + } + + const auto& device = device_holder->GetDevice(); + auto result = device.waitForFences( + fences.size(), // fences count + fences.data(), // fences + false, // wait for all + std::chrono::nanoseconds{100ms}.count() // timeout (ns) + ); + if (!(result == vk::Result::eSuccess || result == vk::Result::eTimeout)) { + VALIDATION_LOG << "Fence waiter encountered an unexpected error " + << "while terminating."; + } + break; } From 3bc4f0af02ae0185c16c24bdf97c744d824b2551 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Wed, 13 Sep 2023 09:11:33 -0700 Subject: [PATCH 24/26] Avoid holding on to buffers on collected pools. --- impeller/renderer/backend/vulkan/command_pool_vk.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/impeller/renderer/backend/vulkan/command_pool_vk.cc b/impeller/renderer/backend/vulkan/command_pool_vk.cc index 218df9ef343f0..6b72f1a92361e 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk.cc +++ b/impeller/renderer/backend/vulkan/command_pool_vk.cc @@ -18,7 +18,7 @@ namespace impeller { // Holds the command pool in a background thread, recyling it when not in use. -class BackgroundCommandPoolVK { +class BackgroundCommandPoolVK final { public: BackgroundCommandPoolVK(BackgroundCommandPoolVK&&) = default; @@ -92,6 +92,10 @@ vk::UniqueCommandBuffer CommandPoolVK::CreateCommandBuffer() { } void CommandPoolVK::CollectCommandBuffer(vk::UniqueCommandBuffer&& buffer) { + if (!pool_) { + // If the command pool has already been destroyed, just free the buffer. + return; + } collected_buffers_.push_back(std::move(buffer)); } From 6e1a89b2103eafca214b35d52a1d2aeee2279161 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 15 Sep 2023 20:33:26 -0700 Subject: [PATCH 25/26] Fix test. --- .../renderer/backend/vulkan/command_pool_vk_unittests.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc b/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc index 8088ae0c2d647..66cde90c5cac2 100644 --- a/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc @@ -11,7 +11,7 @@ namespace impeller { namespace testing { TEST(CommandPoolRecyclerVKTest, GetsACommandPoolPerThread) { - auto const context = CreateMockVulkanContext(); + auto const context = MockVulkanContextBuilder().Build(); // Record the memory location of each pointer to a command pool. int* pool1 = nullptr; @@ -38,7 +38,7 @@ TEST(CommandPoolRecyclerVKTest, GetsACommandPoolPerThread) { } TEST(CommandPoolRecyclerVKTest, GetsTheSameCommandPoolOnSameThread) { - auto const context = CreateMockVulkanContext(); + auto const context = MockVulkanContextBuilder().Build(); auto const pool1 = context->GetCommandPoolRecycler()->Get(); auto const pool2 = context->GetCommandPoolRecycler()->Get(); @@ -71,7 +71,7 @@ class DeathRattle final { } // namespace TEST(CommandPoolRecyclerVKTest, ReclaimMakesCommandPoolAvailable) { - auto const context = CreateMockVulkanContext(); + auto const context = MockVulkanContextBuilder().Build(); { // Fetch a pool (which will be created). From 2d0452912f50d069dfdf915837327a5f1b472bbe Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 15 Sep 2023 20:40:31 -0700 Subject: [PATCH 26/26] Revert fence waiter changes. --- .../backend/vulkan/fence_waiter_vk.cc | 30 ------------------- 1 file changed, 30 deletions(-) diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc index e9513b70f1791..4a16c7a79a716 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc @@ -105,37 +105,7 @@ void FenceWaiterVK::Main() { } if (terminate) { -<<<<<<< HEAD - // This code is copied because checking for the terminate flag after the - // fence waiting appears insufficient. It's possible to refactor this into - // a method and re-use it, but it's not clear if that might have other - // unintended consequences. - auto fences = GetFencesForWaitSet(wait_set); - if (fences.empty()) { - break; - } - - // If there are fences to wait on, wait for them to be signaled. - auto device_holder = device_holder_.lock(); - if (!device_holder) { - break; - } - - const auto& device = device_holder->GetDevice(); - auto result = device.waitForFences( - fences.size(), // fences count - fences.data(), // fences - false, // wait for all - std::chrono::nanoseconds{100ms}.count() // timeout (ns) - ); - if (!(result == vk::Result::eSuccess || result == vk::Result::eTimeout)) { - VALIDATION_LOG << "Fence waiter encountered an unexpected error " - << "while terminating."; - } - -======= WaitUntilEmpty(); ->>>>>>> upstream/main break; }