From e057941ee752b1f4db5126ed1189fbcadcb662f2 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Fri, 24 Jan 2025 15:24:47 -0800 Subject: [PATCH] [Impeller] when a command pool has many unused buffers, reset with release resources flag. (#162171) Fixes https://github.com/flutter/flutter/issues/161861 Resetting a command pool is not sufficient to reclaim memory. The pool must be trimmed. Contrary to what you would expect, it appears that just resetting the pool, even if the cmd buffers are reclaimed, never frees any memory. --- .../backend/vulkan/command_pool_vk.cc | 25 ++++--- .../renderer/backend/vulkan/command_pool_vk.h | 7 +- .../vulkan/command_pool_vk_unittests.cc | 66 +++++++++++++++++++ .../backend/vulkan/test/mock_vulkan.cc | 15 +++++ 4 files changed, 101 insertions(+), 12 deletions(-) diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk.cc index 9bb3d9d898..d6176b2c3b 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk.cc @@ -12,6 +12,7 @@ #include "impeller/renderer/backend/vulkan/resource_manager_vk.h" #include "impeller/renderer/backend/vulkan/vk.h" // IWYU pragma: keep. +#include "vulkan/vulkan_enums.hpp" #include "vulkan/vulkan_handles.hpp" #include "vulkan/vulkan_structs.hpp" @@ -46,14 +47,11 @@ class BackgroundCommandPoolVK final { if (!recycler) { return; } - // If there are many unused command buffers, release some of them. - if (unused_count_ > kUnusedCommandBufferLimit) { - for (auto i = 0u; i < unused_count_; i++) { - buffers_.pop_back(); - } - } - - recycler->Reclaim(std::move(pool_), std::move(buffers_)); + // If there are many unused command buffers, release some of them and + // trim the command pool. + bool should_trim = unused_count_ > kUnusedCommandBufferLimit; + recycler->Reclaim(std::move(pool_), std::move(buffers_), + /*should_trim=*/should_trim); } private: @@ -255,14 +253,21 @@ CommandPoolRecyclerVK::Reuse() { void CommandPoolRecyclerVK::Reclaim( vk::UniqueCommandPool&& pool, - std::vector&& buffers) { + std::vector&& buffers, + bool should_trim) { // 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()); + if (should_trim) { + buffers.clear(); + device.resetCommandPool(pool.get(), + vk::CommandPoolResetFlagBits::eReleaseResources); + } else { + device.resetCommandPool(pool.get(), {}); + } // Move the pool to the recycled list. Lock recycled_lock(recycled_mutex_); diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk.h b/engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk.h index dd60026b44..30f2b3c368 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk.h +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk.h @@ -130,9 +130,12 @@ class CommandPoolRecyclerVK final /// @brief Returns a command pool to be reset on a background thread. /// - /// @param[in] pool The pool to recycler. + /// @param[in] pool The pool to recycle. + /// @param[in] should_trim whether to trim command pool memory before + /// reseting. void Reclaim(vk::UniqueCommandPool&& pool, - std::vector&& buffers); + std::vector&& buffers, + bool should_trim = false); /// @brief Clears all recycled command pools to let them be reclaimed. void Dispose(); diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc index 880c40d664..42da1b896e 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/command_pool_vk_unittests.cc @@ -162,5 +162,71 @@ TEST(CommandPoolRecyclerVKTest, CommandBuffersAreRecycled) { context->Shutdown(); } +TEST(CommandPoolRecyclerVKTest, ExtraCommandBufferAllocationsTriggerTrim) { + auto const context = MockVulkanContextBuilder().Build(); + + { + // Fetch a pool (which will be created). + auto const recycler = context->GetCommandPoolRecycler(); + auto pool = recycler->Get(); + + // Allocate a large number of command buffers + for (auto i = 0; i < 64; i++) { + auto buffer = pool->CreateCommandBuffer(); + pool->CollectCommandBuffer(std::move(buffer)); + } + + // This normally is called at the end of a frame. + recycler->Dispose(); + } + + // Wait for the pool to be reclaimed. + for (auto i = 0u; i < 2u; i++) { + auto waiter = fml::AutoResetWaitableEvent(); + auto rattle = DeathRattle([&waiter]() { waiter.Signal(); }); + { + UniqueResourceVKT resource(context->GetResourceManager(), + std::move(rattle)); + } + waiter.Wait(); + } + + // Command pool is reset but does not release resources. + auto called = GetMockVulkanFunctions(context->GetDevice()); + EXPECT_EQ(std::count(called->begin(), called->end(), "vkResetCommandPool"), + 1u); + + // Create the pool a second time, but dont use any command buffers. + { + // Fetch a pool (which will be created). + auto const recycler = context->GetCommandPoolRecycler(); + auto pool = recycler->Get(); + + // This normally is called at the end of a frame. + recycler->Dispose(); + } + + // Wait for the pool to be reclaimed. + for (auto i = 0u; i < 2u; i++) { + auto waiter = fml::AutoResetWaitableEvent(); + auto rattle = DeathRattle([&waiter]() { waiter.Signal(); }); + { + UniqueResourceVKT resource(context->GetResourceManager(), + std::move(rattle)); + } + waiter.Wait(); + } + + // Verify that the cmd pool was trimmed. + + // Now check that we only ever created one pool and one command buffer. + called = GetMockVulkanFunctions(context->GetDevice()); + EXPECT_EQ(std::count(called->begin(), called->end(), + "vkResetCommandPoolReleaseResources"), + 1u); + + context->Shutdown(); +} + } // namespace testing } // namespace impeller diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index 51273c2c0b..0ea8beaec3 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -270,6 +270,12 @@ VkResult vkCreateCommandPool(VkDevice device, VkResult vkResetCommandPool(VkDevice device, VkCommandPool commandPool, VkCommandPoolResetFlags flags) { + MockDevice* mock_device = reinterpret_cast(device); + if (flags & VK_COMMAND_POOL_RESET_RELEASE_RESOURCES_BIT) { + mock_device->AddCalledFunction("vkResetCommandPoolReleaseResources"); + } else { + mock_device->AddCalledFunction("vkResetCommandPool"); + } return VK_SUCCESS; } @@ -751,6 +757,13 @@ void vkDestroyFramebuffer(VkDevice device, delete reinterpret_cast(framebuffer); } +void vkTrimCommandPool(VkDevice device, + VkCommandPool commandPool, + VkCommandPoolTrimFlags flags) { + MockDevice* mock_device = reinterpret_cast(device); + mock_device->AddCalledFunction("vkTrimCommandPool"); +} + PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance, const char* pName) { if (strcmp("vkEnumerateInstanceExtensionProperties", pName) == 0) { @@ -902,6 +915,8 @@ PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance, return reinterpret_cast(vkCreateFramebuffer); } else if (strcmp("vkDestroyFramebuffer", pName) == 0) { return reinterpret_cast(vkDestroyFramebuffer); + } else if (strcmp("vkTrimCommandPool", pName) == 0) { + return reinterpret_cast(vkTrimCommandPool); } return noop; }