From fd75ddef5c45cdf7b749e451754ac6aadfcfea06 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Mon, 2 Jun 2025 11:31:59 -0700 Subject: [PATCH] [Impeller] Fix flakes in the CommandPoolRecyclerVKTest suite (#169785) The vector of API calls returned by GetMockVulkanFunctions is not thread safe. The tests need to wait for the the background thread used by CommandPoolRecyclerVK to finish reclaiming resources before checking for mock Vulkan calls. --- .../vulkan/command_pool_vk_unittests.cc | 83 +++++++++---------- 1 file changed, 37 insertions(+), 46 deletions(-) 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 69b4e06c4ea..98f221ac95b 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 @@ -71,6 +71,37 @@ class DeathRattle final { std::function callback_; }; +// Wait for reclaim of recycled command pools. +void WaitForReclaim(const std::shared_ptr& context) { + // Add a resource to the resource manager and wait for its destructor to + // signal an event. + // + // This must be done twice because the resource manager does not guarantee + // the order in which resources are handled within the set of reclaimable + // resources. When the first DeathRattle is signaled there may be pools + // within the pending set that have not yet been reclaimed. After the second + // DeathRattle is signaled all resources in the original set will have been + // reclaimed. + for (int i = 0; i < 2; i++) { + auto waiter = fml::AutoResetWaitableEvent(); + auto rattle = DeathRattle([&waiter]() { waiter.Signal(); }); + { + UniqueResourceVKT resource(context->GetResourceManager(), + std::move(rattle)); + } + waiter.Wait(); + } +} + +// The list of function calls returned by the mock Vulkan device is not thread +// safe. Wait for the background thread to finish any pending reclaim +// operations before obtaining the list. +std::shared_ptr> ReclaimAndGetMockVulkanFunctions( + const std::shared_ptr& context) { + WaitForReclaim(context); + return GetMockVulkanFunctions(context->GetDevice()); +} + } // namespace TEST(CommandPoolRecyclerVKTest, ReclaimMakesCommandPoolAvailable) { @@ -85,16 +116,7 @@ TEST(CommandPoolRecyclerVKTest, ReclaimMakesCommandPoolAvailable) { recycler->Dispose(); } - // 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(); + WaitForReclaim(context); // On another thread explicitly, request a new pool. std::thread thread([&]() { @@ -105,7 +127,7 @@ TEST(CommandPoolRecyclerVKTest, ReclaimMakesCommandPoolAvailable) { thread.join(); // Now check that we only ever created one pool. - auto const called = GetMockVulkanFunctions(context->GetDevice()); + auto const called = ReclaimAndGetMockVulkanFunctions(context); EXPECT_EQ(std::count(called->begin(), called->end(), "vkCreateCommandPool"), 1u); @@ -127,16 +149,7 @@ TEST(CommandPoolRecyclerVKTest, CommandBuffersAreRecycled) { 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(); - } + WaitForReclaim(context); { // Create a second pool and command buffer, which should reused the existing @@ -152,7 +165,7 @@ TEST(CommandPoolRecyclerVKTest, CommandBuffersAreRecycled) { } // Now check that we only ever created one pool and one command buffer. - auto const called = GetMockVulkanFunctions(context->GetDevice()); + auto const called = ReclaimAndGetMockVulkanFunctions(context); EXPECT_EQ(std::count(called->begin(), called->end(), "vkCreateCommandPool"), 1u); EXPECT_EQ( @@ -180,19 +193,8 @@ TEST(CommandPoolRecyclerVKTest, ExtraCommandBufferAllocationsTriggerTrim) { 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()); + auto called = ReclaimAndGetMockVulkanFunctions(context); EXPECT_EQ(std::count(called->begin(), called->end(), "vkResetCommandPool"), 1u); @@ -206,21 +208,10 @@ TEST(CommandPoolRecyclerVKTest, ExtraCommandBufferAllocationsTriggerTrim) { 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()); + called = ReclaimAndGetMockVulkanFunctions(context); EXPECT_EQ(std::count(called->begin(), called->end(), "vkResetCommandPoolReleaseResources"), 1u);