From 4f4240d716f023c7b118be78324e565c13cf5d62 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Wed, 30 Aug 2023 13:06:53 -0700 Subject: [PATCH 1/6] [Impeller] started tracking the pool with the command buffer --- impeller/renderer/backend/vulkan/command_encoder_vk.cc | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/impeller/renderer/backend/vulkan/command_encoder_vk.cc b/impeller/renderer/backend/vulkan/command_encoder_vk.cc index de7e82b28ff21..53186afc5cde0 100644 --- a/impeller/renderer/backend/vulkan/command_encoder_vk.cc +++ b/impeller/renderer/backend/vulkan/command_encoder_vk.cc @@ -34,13 +34,7 @@ class TrackedObjectsVK { if (!buffer_) { return; } - auto pool = pool_.lock(); - if (!pool) { - // The buffer can not be freed if its command pool has been destroyed. - buffer_.release(); - return; - } - pool->CollectGraphicsCommandBuffer(std::move(buffer_)); + pool_->CollectGraphicsCommandBuffer(std::move(buffer_)); } bool IsValid() const { return is_valid_; } @@ -86,7 +80,7 @@ class TrackedObjectsVK { private: DescriptorPoolVK desc_pool_; - std::weak_ptr pool_; + std::shared_ptr pool_; vk::UniqueCommandBuffer buffer_; std::set> tracked_objects_; std::set> tracked_buffers_; From 0f47509620399c67488753cf36f6d4bb4d79a022 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 31 Aug 2023 10:29:04 -0700 Subject: [PATCH 2/6] added test --- impeller/renderer/backend/vulkan/BUILD.gn | 1 + .../vulkan/command_encoder_vk_unittests.cc | 38 +++++++++++++++++++ .../backend/vulkan/test/mock_vulkan.cc | 19 ++++++++++ 3 files changed, 58 insertions(+) create mode 100644 impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc diff --git a/impeller/renderer/backend/vulkan/BUILD.gn b/impeller/renderer/backend/vulkan/BUILD.gn index d7abddb096d74..ae2c71be87ca8 100644 --- a/impeller/renderer/backend/vulkan/BUILD.gn +++ b/impeller/renderer/backend/vulkan/BUILD.gn @@ -9,6 +9,7 @@ impeller_component("vulkan_unittests") { testonly = true sources = [ "blit_command_vk_unittests.cc", + "command_encoder_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_encoder_vk_unittests.cc b/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc new file mode 100644 index 0000000000000..95e9ec9b9e1ea --- /dev/null +++ b/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc @@ -0,0 +1,38 @@ +// 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 + +#include "flutter/testing/testing.h" +#include "impeller/renderer/backend/vulkan/command_encoder_vk.h" +#include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" + +namespace impeller { +namespace testing { + +TEST(CommandEncoderVKTest, DeleteEncoderAfterThreadDies) { + std::shared_ptr> called_functions; + { + auto context = CreateMockVulkanContext(); + called_functions = GetMockVulkanFunctions(context->GetDevice()); + std::shared_ptr encoder; + std::thread thread([&] { + CommandEncoderFactoryVK factory(context); + encoder = factory.Create(); + }); + thread.join(); + } + auto destroy_pool = + std::find(called_functions->begin(), called_functions->end(), + "vkDestroyCommandPool"); + auto free_buffers = + std::find(called_functions->begin(), called_functions->end(), + "vkFreeCommandBuffers"); + EXPECT_TRUE(destroy_pool != called_functions->end()); + EXPECT_TRUE(free_buffers != called_functions->end()); + EXPECT_TRUE(free_buffers < destroy_pool); +} + +} // namespace testing +} // namespace impeller \ No newline at end of file diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index a6f3d08e16dc1..d5a199bdcec4c 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -346,6 +346,21 @@ void vkCmdSetViewport(VkCommandBuffer commandBuffer, mock_command_buffer->called_functions_->push_back("vkCmdSetViewport"); } +void vkFreeCommandBuffers(VkDevice device, + VkCommandPool commandPool, + uint32_t commandBufferCount, + const VkCommandBuffer* pCommandBuffers) { + MockDevice* mock_device = reinterpret_cast(device); + mock_device->called_functions_->push_back("vkFreeCommandBuffers"); +} + +void vkDestroyCommandPool(VkDevice device, + VkCommandPool commandPool, + const VkAllocationCallbacks* pAllocator) { + MockDevice* mock_device = reinterpret_cast(device); + mock_device->called_functions_->push_back("vkDestroyCommandPool"); +} + PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance, const char* pName) { if (strcmp("vkEnumerateInstanceExtensionProperties", pName) == 0) { @@ -424,6 +439,10 @@ PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance, return (PFN_vkVoidFunction)vkCmdSetScissor; } else if (strcmp("vkCmdSetViewport", pName) == 0) { return (PFN_vkVoidFunction)vkCmdSetViewport; + } else if (strcmp("vkDestroyCommandPool", pName) == 0) { + return (PFN_vkVoidFunction)vkDestroyCommandPool; + } else if (strcmp("vkFreeCommandBuffers", pName) == 0) { + return (PFN_vkVoidFunction)vkFreeCommandBuffers; } return noop; } From 1199826d17b1a50bd519b84a6ee0c8ef0c750803 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 31 Aug 2023 12:34:57 -0700 Subject: [PATCH 3/6] added second test --- .../vulkan/command_encoder_vk_unittests.cc | 40 +++++++++++++++++- .../backend/vulkan/resource_manager_vk.h | 5 ++- .../backend/vulkan/test/mock_vulkan.cc | 41 +++++++++++++++++++ 3 files changed, 83 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 95e9ec9b9e1ea..565fd794859de 100644 --- a/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc @@ -4,8 +4,10 @@ #include +#include "flutter/fml/synchronization/count_down_latch.h" #include "flutter/testing/testing.h" #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 { @@ -34,5 +36,41 @@ TEST(CommandEncoderVKTest, DeleteEncoderAfterThreadDies) { EXPECT_TRUE(free_buffers < destroy_pool); } +TEST(CommandEncoderVKTest, CleanupAfterSubmit) { + // This tests deleting the TrackedObjects where the thread is killed before + // the fence waiter has disposed of them. + std::shared_ptr> called_functions; + { + fml::AutoResetWaitableEvent wait_for_submit; + fml::AutoResetWaitableEvent wait_for_thread_join; + auto context = CreateMockVulkanContext(); + std::thread thread([&] { + CommandEncoderFactoryVK factory(context); + std::shared_ptr encoder = factory.Create(); + encoder->Submit([&](bool success) { + ASSERT_TRUE(success); + wait_for_thread_join.Wait(); + wait_for_submit.Signal(); + }); + }); + thread.join(); + wait_for_thread_join.Signal(); + auto [fence_result, fence] = context->GetDevice().createFenceUnique({}); + ASSERT_EQ(fence_result, vk::Result::eSuccess); + context->GetFenceWaiter()->AddFence(std::move(fence), [] {}); + wait_for_submit.Wait(); + called_functions = GetMockVulkanFunctions(context->GetDevice()); + } + auto destroy_pool = + std::find(called_functions->begin(), called_functions->end(), + "vkDestroyCommandPool"); + auto free_buffers = + std::find(called_functions->begin(), called_functions->end(), + "vkFreeCommandBuffers"); + EXPECT_TRUE(destroy_pool != called_functions->end()); + EXPECT_TRUE(free_buffers != called_functions->end()); + EXPECT_TRUE(free_buffers < destroy_pool); +} + } // namespace testing -} // namespace impeller \ No newline at end of file +} // namespace impeller diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk.h b/impeller/renderer/backend/vulkan/resource_manager_vk.h index f68c9ef0bd366..9ccf34ad95916 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk.h +++ b/impeller/renderer/backend/vulkan/resource_manager_vk.h @@ -75,12 +75,13 @@ class ResourceManagerVK final using Reclaimables = std::vector>; ResourceManagerVK(); - - std::thread waiter_; std::mutex reclaimables_mutex_; std::condition_variable reclaimables_cv_; Reclaimables reclaimables_; bool should_exit_ = false; + // This should be initialized last since it references the other instance + // variables. + std::thread waiter_; //---------------------------------------------------------------------------- /// @brief Starts the resource manager thread. diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index d5a199bdcec4c..c2ee56e309ff2 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -361,6 +361,37 @@ void vkDestroyCommandPool(VkDevice device, mock_device->called_functions_->push_back("vkDestroyCommandPool"); } +VkResult vkEndCommandBuffer(VkCommandBuffer commandBuffer) { + return VK_SUCCESS; +} + +VkResult vkCreateFence(VkDevice device, + const VkFenceCreateInfo* pCreateInfo, + const VkAllocationCallbacks* pAllocator, + VkFence* pFence) { + *pFence = reinterpret_cast(0xfe0ce); + return VK_SUCCESS; +} + +VkResult vkQueueSubmit(VkQueue queue, + uint32_t submitCount, + const VkSubmitInfo* pSubmits, + VkFence fence) { + return VK_SUCCESS; +} + +VkResult vkWaitForFences(VkDevice device, + uint32_t fenceCount, + const VkFence* pFences, + VkBool32 waitAll, + uint64_t timeout) { + return VK_SUCCESS; +} + +VkResult vkGetFenceStatus(VkDevice device, VkFence fence) { + return VK_SUCCESS; +} + PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance, const char* pName) { if (strcmp("vkEnumerateInstanceExtensionProperties", pName) == 0) { @@ -443,6 +474,16 @@ PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance, return (PFN_vkVoidFunction)vkDestroyCommandPool; } else if (strcmp("vkFreeCommandBuffers", pName) == 0) { return (PFN_vkVoidFunction)vkFreeCommandBuffers; + } else if (strcmp("vkEndCommandBuffer", pName) == 0) { + return (PFN_vkVoidFunction)vkEndCommandBuffer; + } else if (strcmp("vkCreateFence", pName) == 0) { + return (PFN_vkVoidFunction)vkCreateFence; + } else if (strcmp("vkQueueSubmit", pName) == 0) { + return (PFN_vkVoidFunction)vkQueueSubmit; + } else if (strcmp("vkWaitForFences", pName) == 0) { + return (PFN_vkVoidFunction)vkWaitForFences; + } else if (strcmp("vkGetFenceStatus", pName) == 0) { + return (PFN_vkVoidFunction)vkGetFenceStatus; } return noop; } From c709f396bc02f9eda1cd76b6b5745e3da8d5f101 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 31 Aug 2023 12:51:17 -0700 Subject: [PATCH 4/6] updated 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 512261ff9e50b..6eafa7868a51c 100644 --- a/ci/licenses_golden/excluded_files +++ b/ci/licenses_golden/excluded_files @@ -151,6 +151,7 @@ ../../../flutter/impeller/image/README.md ../../../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/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 30b3b3d6457a11b98543eecede9cb51b4f02baf0 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 31 Aug 2023 12:54:13 -0700 Subject: [PATCH 5/6] added comment --- impeller/renderer/backend/vulkan/command_encoder_vk.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/impeller/renderer/backend/vulkan/command_encoder_vk.cc b/impeller/renderer/backend/vulkan/command_encoder_vk.cc index 53186afc5cde0..9b187896a4f6d 100644 --- a/impeller/renderer/backend/vulkan/command_encoder_vk.cc +++ b/impeller/renderer/backend/vulkan/command_encoder_vk.cc @@ -80,6 +80,7 @@ class TrackedObjectsVK { private: DescriptorPoolVK desc_pool_; + // `shared_ptr` since command buffers have a link to the command pool. std::shared_ptr pool_; vk::UniqueCommandBuffer buffer_; std::set> tracked_objects_; From 2313fd89a28312c35f2b2f8983e72dac629483d8 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 7 Sep 2023 13:13:06 -0700 Subject: [PATCH 6/6] expanded and simplified test --- .../backend/vulkan/command_encoder_vk_unittests.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc b/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc index 565fd794859de..0fa38fdb82985 100644 --- a/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc @@ -14,6 +14,8 @@ namespace impeller { namespace testing { TEST(CommandEncoderVKTest, DeleteEncoderAfterThreadDies) { + // Tests that when a CommandEncoderVK is deleted that it will clean up its + // command buffers before it cleans up its command pool. std::shared_ptr> called_functions; { auto context = CreateMockVulkanContext(); @@ -38,7 +40,8 @@ TEST(CommandEncoderVKTest, DeleteEncoderAfterThreadDies) { TEST(CommandEncoderVKTest, CleanupAfterSubmit) { // This tests deleting the TrackedObjects where the thread is killed before - // the fence waiter has disposed of them. + // the fence waiter has disposed of them, making sure the command buffer and + // its pools are deleted in that order. std::shared_ptr> called_functions; { fml::AutoResetWaitableEvent wait_for_submit; @@ -55,9 +58,6 @@ TEST(CommandEncoderVKTest, CleanupAfterSubmit) { }); thread.join(); wait_for_thread_join.Signal(); - auto [fence_result, fence] = context->GetDevice().createFenceUnique({}); - ASSERT_EQ(fence_result, vk::Result::eSuccess); - context->GetFenceWaiter()->AddFence(std::move(fence), [] {}); wait_for_submit.Wait(); called_functions = GetMockVulkanFunctions(context->GetDevice()); }