From 2ff38d22fc8eeea8c79b0ed5bca95c64a25dcb1e Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 14 Sep 2023 09:42:51 -0700 Subject: [PATCH 01/21] [Impeller] transitioned mock vulkan context to the builder pattern --- .../vulkan/blit_command_vk_unittests.cc | 8 ++--- .../vulkan/command_encoder_vk_unittests.cc | 4 +-- .../backend/vulkan/context_vk_unittests.cc | 15 ++++---- .../vulkan/pass_bindings_cache_unittests.cc | 8 ++--- .../backend/vulkan/test/mock_vulkan.cc | 7 ++-- .../backend/vulkan/test/mock_vulkan.h | 34 ++++++++++++------- .../vulkan/test/mock_vulkan_unittests.cc | 2 +- 7 files changed, 45 insertions(+), 33 deletions(-) diff --git a/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc b/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc index 44206c2e7ddb5..e1172651353b7 100644 --- a/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc @@ -11,7 +11,7 @@ namespace impeller { namespace testing { TEST(BlitCommandVkTest, BlitCopyTextureToTextureCommandVK) { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); auto pool = CommandPoolVK::GetThreadLocal(context.get()); auto encoder = std::make_unique(context)->Create(); BlitCopyTextureToTextureCommandVK cmd; @@ -28,7 +28,7 @@ TEST(BlitCommandVkTest, BlitCopyTextureToTextureCommandVK) { } TEST(BlitCommandVkTest, BlitCopyTextureToBufferCommandVK) { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); auto encoder = std::make_unique(context)->Create(); BlitCopyTextureToBufferCommandVK cmd; cmd.source = context->GetResourceAllocator()->CreateTexture({ @@ -44,7 +44,7 @@ TEST(BlitCommandVkTest, BlitCopyTextureToBufferCommandVK) { } TEST(BlitCommandVkTest, BlitCopyBufferToTextureCommandVK) { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); auto encoder = std::make_unique(context)->Create(); BlitCopyBufferToTextureCommandVK cmd; cmd.destination = context->GetResourceAllocator()->CreateTexture({ @@ -62,7 +62,7 @@ TEST(BlitCommandVkTest, BlitCopyBufferToTextureCommandVK) { } TEST(BlitCommandVkTest, BlitGenerateMipmapCommandVK) { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); auto encoder = std::make_unique(context)->Create(); BlitGenerateMipmapCommandVK cmd; cmd.texture = context->GetResourceAllocator()->CreateTexture({ diff --git a/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc b/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc index 0fa38fdb82985..2314796f54415 100644 --- a/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc @@ -18,7 +18,7 @@ TEST(CommandEncoderVKTest, DeleteEncoderAfterThreadDies) { // command buffers before it cleans up its command pool. std::shared_ptr> called_functions; { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); called_functions = GetMockVulkanFunctions(context->GetDevice()); std::shared_ptr encoder; std::thread thread([&] { @@ -46,7 +46,7 @@ TEST(CommandEncoderVKTest, CleanupAfterSubmit) { { fml::AutoResetWaitableEvent wait_for_submit; fml::AutoResetWaitableEvent wait_for_thread_join; - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); std::thread thread([&] { CommandEncoderFactoryVK factory(context); std::shared_ptr encoder = factory.Create(); diff --git a/impeller/renderer/backend/vulkan/context_vk_unittests.cc b/impeller/renderer/backend/vulkan/context_vk_unittests.cc index 8a577be6d79bc..48ae3a08be1d4 100644 --- a/impeller/renderer/backend/vulkan/context_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/context_vk_unittests.cc @@ -14,7 +14,7 @@ TEST(ContextVKTest, DeletesCommandPools) { std::weak_ptr weak_context; std::weak_ptr weak_pool; { - std::shared_ptr context = CreateMockVulkanContext(); + std::shared_ptr context = MockVulkanContextBuilder().Build(); std::shared_ptr pool = CommandPoolVK::GetThreadLocal(context.get()); weak_pool = pool; @@ -30,7 +30,7 @@ TEST(ContextVKTest, DeletePipelineAfterContext) { std::shared_ptr> pipeline; std::shared_ptr> functions; { - std::shared_ptr context = CreateMockVulkanContext(); + std::shared_ptr context = MockVulkanContextBuilder().Build(); PipelineDescriptor pipeline_desc; pipeline_desc.SetVertexDescriptor(std::make_shared()); PipelineFuture pipeline_future = @@ -49,7 +49,7 @@ TEST(ContextVKTest, DeleteShaderFunctionAfterContext) { std::shared_ptr shader_function; std::shared_ptr> functions; { - std::shared_ptr context = CreateMockVulkanContext(); + std::shared_ptr context = MockVulkanContextBuilder().Build(); PipelineDescriptor pipeline_desc; pipeline_desc.SetVertexDescriptor(std::make_shared()); std::vector data = {0x03, 0x02, 0x23, 0x07}; @@ -71,7 +71,7 @@ TEST(ContextVKTest, DeletePipelineLibraryAfterContext) { std::shared_ptr pipeline_library; std::shared_ptr> functions; { - std::shared_ptr context = CreateMockVulkanContext(); + std::shared_ptr context = MockVulkanContextBuilder().Build(); PipelineDescriptor pipeline_desc; pipeline_desc.SetVertexDescriptor(std::make_shared()); pipeline_library = context->GetPipelineLibrary(); @@ -86,8 +86,11 @@ TEST(ContextVKTest, DeletePipelineLibraryAfterContext) { TEST(ContextVKTest, CanCreateContextInAbsenceOfValidationLayers) { // The mocked methods don't report the presence of a validation layer but we // explicitly ask for validation. Context creation should continue anyway. - auto context = CreateMockVulkanContext( - [](auto& settings) { settings.enable_validation = true; }); + auto context = MockVulkanContextBuilder() + .SetSettingsCallback([](auto& settings) { + settings.enable_validation = true; + }) + .Build(); ASSERT_NE(context, nullptr); } diff --git a/impeller/renderer/backend/vulkan/pass_bindings_cache_unittests.cc b/impeller/renderer/backend/vulkan/pass_bindings_cache_unittests.cc index bab2a25db7f35..e76213c200493 100644 --- a/impeller/renderer/backend/vulkan/pass_bindings_cache_unittests.cc +++ b/impeller/renderer/backend/vulkan/pass_bindings_cache_unittests.cc @@ -24,7 +24,7 @@ int32_t CountStringViewInstances(const std::vector& strings, } // namespace TEST(PassBindingsCacheTest, bindPipeline) { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); PassBindingsCache cache; auto encoder = std::make_unique(context)->Create(); auto buffer = encoder->GetCommandBuffer(); @@ -38,7 +38,7 @@ TEST(PassBindingsCacheTest, bindPipeline) { } TEST(PassBindingsCacheTest, setStencilReference) { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); PassBindingsCache cache; auto encoder = std::make_unique(context)->Create(); auto buffer = encoder->GetCommandBuffer(); @@ -53,7 +53,7 @@ TEST(PassBindingsCacheTest, setStencilReference) { } TEST(PassBindingsCacheTest, setScissor) { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); PassBindingsCache cache; auto encoder = std::make_unique(context)->Create(); auto buffer = encoder->GetCommandBuffer(); @@ -66,7 +66,7 @@ TEST(PassBindingsCacheTest, setScissor) { } TEST(PassBindingsCacheTest, setViewport) { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); PassBindingsCache cache; auto encoder = std::make_unique(context)->Create(); auto buffer = encoder->GetCommandBuffer(); diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index aafb3cf410153..726ecf304abf9 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -513,13 +513,12 @@ PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance, } // namespace -std::shared_ptr CreateMockVulkanContext( - const std::function& settings_callback) { +std::shared_ptr MockVulkanContextBuilder::Build() { auto message_loop = fml::ConcurrentMessageLoop::Create(); ContextVK::Settings settings; settings.proc_address_callback = GetMockVulkanProcAddress; - if (settings_callback) { - settings_callback(settings); + if (settings_callback_) { + settings_callback_(settings); } return ContextVK::Create(std::move(settings)); } diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.h b/impeller/renderer/backend/vulkan/test/mock_vulkan.h index 2984db473ff09..fababd3997f26 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.h +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.h @@ -16,18 +16,28 @@ namespace testing { std::shared_ptr> GetMockVulkanFunctions( VkDevice device); -//------------------------------------------------------------------------------ -/// @brief Create a Vulkan context with Vulkan functions mocked. The caller -/// is given a chance to tinker on the settings right before a -/// context is created. -/// -/// @param[in] settings_callback The settings callback -/// -/// @return A context if one can be created. -/// -std::shared_ptr CreateMockVulkanContext( - const std::function& settings_callback = - nullptr); +class MockVulkanContextBuilder { + public: + //------------------------------------------------------------------------------ + /// @brief Create a Vulkan context with Vulkan functions mocked. The + /// caller is given a chance to tinker on the settings right + /// before a context is created. + /// + /// @return A context if one can be created. + /// + std::shared_ptr Build(); + + /// A callback that allows the modification of the ContextVK::Settings before + /// the context is made. + MockVulkanContextBuilder& SetSettingsCallback( + const std::function& settings_callback) { + settings_callback_ = settings_callback; + return *this; + } + + private: + std::function settings_callback_; +}; } // 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..de28491e69fed 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc @@ -14,7 +14,7 @@ TEST(MockVulkanContextTest, IsThreadSafe) { // In a typical app, there is a single ContextVK per app, shared b/w threads. // // This test ensures that the (mock) ContextVK is thread-safe. - auto const context = CreateMockVulkanContext(); + auto const context = MockVulkanContextBuilder().Build(); // Spawn two threads, and have them create a CommandPoolVK each. std::thread thread1([&context]() { From 60b871d888583b9707d6a907e862a3e854b25b51 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 14 Sep 2023 10:44:09 -0700 Subject: [PATCH 02/21] made creating a context with validation layers work --- .../backend/vulkan/context_vk_unittests.cc | 12 +++++ .../backend/vulkan/test/mock_vulkan.cc | 54 ++++++++++++++++--- .../backend/vulkan/test/mock_vulkan.h | 16 ++++++ 3 files changed, 74 insertions(+), 8 deletions(-) diff --git a/impeller/renderer/backend/vulkan/context_vk_unittests.cc b/impeller/renderer/backend/vulkan/context_vk_unittests.cc index 48ae3a08be1d4..b69cdfdf4bfaf 100644 --- a/impeller/renderer/backend/vulkan/context_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/context_vk_unittests.cc @@ -94,5 +94,17 @@ TEST(ContextVKTest, CanCreateContextInAbsenceOfValidationLayers) { ASSERT_NE(context, nullptr); } +TEST(ContextVKTest, CanCreateContextWithValidationLayers) { + auto context = + MockVulkanContextBuilder() + .SetSettingsCallback( + [](auto& settings) { settings.enable_validation = true; }) + .SetInstanceExtensions( + {"VK_KHR_surface", "VK_MVK_macos_surface", "VK_EXT_debug_utils"}) + .SetInstanceLayers({"VK_LAYER_KHRONOS_validation"}) + .Build(); + ASSERT_NE(context, nullptr); +} + } // 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 726ecf304abf9..3272fb2bd17c5 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -54,25 +54,39 @@ class MockDevice final { void noop() {} +thread_local std::vector g_instance_extensions; + VkResult vkEnumerateInstanceExtensionProperties( const char* pLayerName, uint32_t* pPropertyCount, VkExtensionProperties* pProperties) { if (!pProperties) { - *pPropertyCount = 2; - + *pPropertyCount = g_instance_extensions.size(); } else { - strcpy(pProperties[0].extensionName, "VK_KHR_surface"); - pProperties[0].specVersion = 0; - strcpy(pProperties[1].extensionName, "VK_MVK_macos_surface"); - pProperties[1].specVersion = 0; + uint32_t count = 0; + for (const std::string& ext : g_instance_extensions) { + strcpy(pProperties[count].extensionName, ext.c_str()); + pProperties[count].specVersion = 0; + count++; + } } return VK_SUCCESS; } +thread_local std::vector g_instance_layers; + VkResult vkEnumerateInstanceLayerProperties(uint32_t* pPropertyCount, VkLayerProperties* pProperties) { - *pPropertyCount = 0; + if (!pProperties) { + *pPropertyCount = g_instance_layers.size(); + } else { + uint32_t count = 0; + for (const std::string& layer : g_instance_layers) { + strcpy(pProperties[count].layerName, layer.c_str()); + pProperties[count].specVersion = 0; + count++; + } + } return VK_SUCCESS; } @@ -415,6 +429,20 @@ VkResult vkGetFenceStatus(VkDevice device, VkFence fence) { return VK_SUCCESS; } +VkResult vkCreateDebugUtilsMessengerEXT( + VkInstance instance, + const VkDebugUtilsMessengerCreateInfoEXT* pCreateInfo, + const VkAllocationCallbacks* pAllocator, + VkDebugUtilsMessengerEXT* pMessenger) { + return VK_SUCCESS; +} + +VkResult vkSetDebugUtilsObjectNameEXT( + VkDevice device, + const VkDebugUtilsObjectNameInfoEXT* pNameInfo) { + return VK_SUCCESS; +} + PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance, const char* pName) { if (strcmp("vkEnumerateInstanceExtensionProperties", pName) == 0) { @@ -507,12 +535,19 @@ PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance, return (PFN_vkVoidFunction)vkWaitForFences; } else if (strcmp("vkGetFenceStatus", pName) == 0) { return (PFN_vkVoidFunction)vkGetFenceStatus; + } else if (strcmp("vkCreateDebugUtilsMessengerEXT", pName) == 0) { + return (PFN_vkVoidFunction)vkCreateDebugUtilsMessengerEXT; + } else if (strcmp("vkSetDebugUtilsObjectNameEXT", pName) == 0) { + return (PFN_vkVoidFunction)vkSetDebugUtilsObjectNameEXT; } return noop; } } // namespace +MockVulkanContextBuilder::MockVulkanContextBuilder() + : instance_extensions_({"VK_KHR_surface", "VK_MVK_macos_surface"}) {} + std::shared_ptr MockVulkanContextBuilder::Build() { auto message_loop = fml::ConcurrentMessageLoop::Create(); ContextVK::Settings settings; @@ -520,7 +555,10 @@ std::shared_ptr MockVulkanContextBuilder::Build() { if (settings_callback_) { settings_callback_(settings); } - return ContextVK::Create(std::move(settings)); + g_instance_extensions = instance_extensions_; + g_instance_layers = instance_layers_; + std::shared_ptr result = ContextVK::Create(std::move(settings)); + return result; } std::shared_ptr> GetMockVulkanFunctions( diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.h b/impeller/renderer/backend/vulkan/test/mock_vulkan.h index fababd3997f26..41c82c6da2a90 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.h +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.h @@ -18,6 +18,8 @@ std::shared_ptr> GetMockVulkanFunctions( class MockVulkanContextBuilder { public: + MockVulkanContextBuilder(); + //------------------------------------------------------------------------------ /// @brief Create a Vulkan context with Vulkan functions mocked. The /// caller is given a chance to tinker on the settings right @@ -35,8 +37,22 @@ class MockVulkanContextBuilder { return *this; } + MockVulkanContextBuilder& SetInstanceExtensions( + const std::vector& instance_extensions) { + instance_extensions_ = instance_extensions; + return *this; + } + + MockVulkanContextBuilder& SetInstanceLayers( + const std::vector& instance_layers) { + instance_layers_ = instance_layers; + return *this; + } + private: std::function settings_callback_; + std::vector instance_extensions_; + std::vector instance_layers_; }; } // namespace testing From 67e90553c80f3d69044ab1c7f1285e31dc25018a Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 14 Sep 2023 10:50:11 -0700 Subject: [PATCH 03/21] added capabilties asserts --- impeller/renderer/backend/vulkan/context_vk_unittests.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/impeller/renderer/backend/vulkan/context_vk_unittests.cc b/impeller/renderer/backend/vulkan/context_vk_unittests.cc index b69cdfdf4bfaf..11f8b1599bc21 100644 --- a/impeller/renderer/backend/vulkan/context_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/context_vk_unittests.cc @@ -92,6 +92,9 @@ TEST(ContextVKTest, CanCreateContextInAbsenceOfValidationLayers) { }) .Build(); ASSERT_NE(context, nullptr); + const CapabilitiesVK* capabilites_vk = + reinterpret_cast(context->GetCapabilities().get()); + ASSERT_FALSE(capabilites_vk->AreValidationsEnabled()); } TEST(ContextVKTest, CanCreateContextWithValidationLayers) { @@ -104,6 +107,9 @@ TEST(ContextVKTest, CanCreateContextWithValidationLayers) { .SetInstanceLayers({"VK_LAYER_KHRONOS_validation"}) .Build(); ASSERT_NE(context, nullptr); + const CapabilitiesVK* capabilites_vk = + reinterpret_cast(context->GetCapabilities().get()); + ASSERT_TRUE(capabilites_vk->AreValidationsEnabled()); } } // namespace testing From ba4bc1ebaa197ec844e6e1cf9d4949f25574a53d Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 14 Sep 2023 12:29:48 -0700 Subject: [PATCH 04/21] moved to fml thread_local --- impeller/renderer/backend/vulkan/test/mock_vulkan.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index 3272fb2bd17c5..5067426d83885 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -5,6 +5,7 @@ #include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" #include #include "fml/macros.h" +#include "fml/thread_local.h" #include "impeller/base/thread_safety.h" namespace impeller { @@ -54,7 +55,7 @@ class MockDevice final { void noop() {} -thread_local std::vector g_instance_extensions; +FML_THREAD_LOCAL std::vector g_instance_extensions; VkResult vkEnumerateInstanceExtensionProperties( const char* pLayerName, @@ -73,7 +74,7 @@ VkResult vkEnumerateInstanceExtensionProperties( return VK_SUCCESS; } -thread_local std::vector g_instance_layers; +FML_THREAD_LOCAL std::vector g_instance_layers; VkResult vkEnumerateInstanceLayerProperties(uint32_t* pPropertyCount, VkLayerProperties* pProperties) { From 004f79b7ba8062d235fd277dd392db8353c01502 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 14 Sep 2023 12:50:37 -0700 Subject: [PATCH 05/21] removed strcpy --- impeller/renderer/backend/vulkan/test/mock_vulkan.cc | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index 5067426d83885..4d8e1ddbb152b 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 #include #include "fml/macros.h" #include "fml/thread_local.h" @@ -13,6 +14,12 @@ namespace testing { namespace { +void StrcpyChecked(char* dst, const char* src) { + static constexpr size_t kMaxStrSize = 1024; + size_t result = strlcpy(dst, src, kMaxStrSize); + FML_CHECK(result < kMaxStrSize); +} + struct MockCommandBuffer { explicit MockCommandBuffer( std::shared_ptr> called_functions) @@ -66,7 +73,7 @@ VkResult vkEnumerateInstanceExtensionProperties( } else { uint32_t count = 0; for (const std::string& ext : g_instance_extensions) { - strcpy(pProperties[count].extensionName, ext.c_str()); + StrcpyChecked(pProperties[count].extensionName, ext.c_str()); pProperties[count].specVersion = 0; count++; } @@ -83,7 +90,7 @@ VkResult vkEnumerateInstanceLayerProperties(uint32_t* pPropertyCount, } else { uint32_t count = 0; for (const std::string& layer : g_instance_layers) { - strcpy(pProperties[count].layerName, layer.c_str()); + StrcpyChecked(pProperties[count].layerName, layer.c_str()); pProperties[count].specVersion = 0; count++; } From da1aada63eba01bfbeb55ddda96f010d99e05b7e Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 14 Sep 2023 13:07:00 -0700 Subject: [PATCH 06/21] switched to strncpy --- impeller/renderer/backend/vulkan/test/mock_vulkan.cc | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index 4d8e1ddbb152b..75cee3d40d8b3 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -14,12 +14,6 @@ namespace testing { namespace { -void StrcpyChecked(char* dst, const char* src) { - static constexpr size_t kMaxStrSize = 1024; - size_t result = strlcpy(dst, src, kMaxStrSize); - FML_CHECK(result < kMaxStrSize); -} - struct MockCommandBuffer { explicit MockCommandBuffer( std::shared_ptr> called_functions) @@ -73,7 +67,8 @@ VkResult vkEnumerateInstanceExtensionProperties( } else { uint32_t count = 0; for (const std::string& ext : g_instance_extensions) { - StrcpyChecked(pProperties[count].extensionName, ext.c_str()); + strncpy(pProperties[count].extensionName, ext.c_str(), + VK_MAX_EXTENSION_NAME_SIZE); pProperties[count].specVersion = 0; count++; } @@ -90,7 +85,8 @@ VkResult vkEnumerateInstanceLayerProperties(uint32_t* pPropertyCount, } else { uint32_t count = 0; for (const std::string& layer : g_instance_layers) { - StrcpyChecked(pProperties[count].layerName, layer.c_str()); + strncpy(pProperties[count].layerName, layer.c_str(), + VK_MAX_EXTENSION_NAME_SIZE); pProperties[count].specVersion = 0; count++; } From 630f91593d3a833acbad94dbd75e1ec7757bfd23 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 14 Sep 2023 13:10:32 -0700 Subject: [PATCH 07/21] moved to sizeof since its a bit more robust --- impeller/renderer/backend/vulkan/test/mock_vulkan.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index 75cee3d40d8b3..ed311ac017a81 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -68,7 +68,7 @@ VkResult vkEnumerateInstanceExtensionProperties( uint32_t count = 0; for (const std::string& ext : g_instance_extensions) { strncpy(pProperties[count].extensionName, ext.c_str(), - VK_MAX_EXTENSION_NAME_SIZE); + sizeof(VkExtensionProperties::extensionName)); pProperties[count].specVersion = 0; count++; } @@ -86,7 +86,7 @@ VkResult vkEnumerateInstanceLayerProperties(uint32_t* pPropertyCount, uint32_t count = 0; for (const std::string& layer : g_instance_layers) { strncpy(pProperties[count].layerName, layer.c_str(), - VK_MAX_EXTENSION_NAME_SIZE); + sizeof(VkLayerProperties::layerName)); pProperties[count].specVersion = 0; count++; } From c89d30d84ebd2074b05e8f3bd52b4fa11162657d Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 14 Sep 2023 17:01:29 -0700 Subject: [PATCH 08/21] Add MockFence and MockFence::SetStatus. --- .../backend/vulkan/test/mock_vulkan.cc | 18 +++++++++-- .../backend/vulkan/test/mock_vulkan.h | 30 +++++++++++++++++++ .../vulkan/test/mock_vulkan_unittests.cc | 22 ++++++++++++++ 3 files changed, 68 insertions(+), 2 deletions(-) diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index ed311ac017a81..01749168695d1 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -4,10 +4,13 @@ #include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" #include +#include #include #include "fml/macros.h" #include "fml/thread_local.h" #include "impeller/base/thread_safety.h" +#include "vulkan/vulkan_core.h" +#include "vulkan/vulkan_enums.hpp" namespace impeller { namespace testing { @@ -42,6 +45,11 @@ class MockDevice final { called_functions_->push_back(function); } + void SetFenceFactory( + std::function()> fence_factory) { + fence_factory_ = std::move(fence_factory); + } + private: FML_DISALLOW_COPY_AND_ASSIGN(MockDevice); @@ -52,6 +60,8 @@ class MockDevice final { Mutex command_buffers_mutex_; std::vector> command_buffers_ IPLR_GUARDED_BY(command_buffers_mutex_); + + std::function()> fence_factory_; }; void noop() {} @@ -410,7 +420,8 @@ VkResult vkCreateFence(VkDevice device, const VkFenceCreateInfo* pCreateInfo, const VkAllocationCallbacks* pAllocator, VkFence* pFence) { - *pFence = reinterpret_cast(0xfe0ce); + MockDevice* mock_device = reinterpret_cast(device); + *pFence = reinterpret_cast(new MockFence()); return VK_SUCCESS; } @@ -430,7 +441,9 @@ VkResult vkWaitForFences(VkDevice device, } VkResult vkGetFenceStatus(VkDevice device, VkFence fence) { - return VK_SUCCESS; + MockDevice* mock_device = reinterpret_cast(device); + MockFence* mock_fence = reinterpret_cast(fence); + return mock_fence->GetStatus(); } VkResult vkCreateDebugUtilsMessengerEXT( @@ -562,6 +575,7 @@ std::shared_ptr MockVulkanContextBuilder::Build() { g_instance_extensions = instance_extensions_; g_instance_layers = instance_layers_; std::shared_ptr result = ContextVK::Create(std::move(settings)); + return result; } diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.h b/impeller/renderer/backend/vulkan/test/mock_vulkan.h index 41c82c6da2a90..fa440f586c8da 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.h +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.h @@ -5,10 +5,12 @@ #pragma once #include +#include #include #include #include "impeller/renderer/backend/vulkan/context_vk.h" +#include "vulkan/vulkan_enums.hpp" namespace impeller { namespace testing { @@ -16,6 +18,28 @@ namespace testing { std::shared_ptr> GetMockVulkanFunctions( VkDevice device); +// A test-controlled version of |vk::Fence|. +class MockFence final { + public: + MockFence() = default; + + // Returns the result that was set in the constructor or |SetStatus|. + VkResult GetStatus() { return static_cast(result_); } + + // Sets the result that will be returned by `GetFenceStatus`. + static void SetStatus(vk::UniqueFence& fence, vk::Result result) { + // Cast the fence to a MockFence and set the result. + VkFence raw_fence = fence.get(); + MockFence* mock_fence = reinterpret_cast(raw_fence); + mock_fence->result_ = result; + } + + private: + vk::Result result_ = vk::Result::eSuccess; + + FML_DISALLOW_COPY_AND_ASSIGN(MockFence); +}; + class MockVulkanContextBuilder { public: MockVulkanContextBuilder(); @@ -53,6 +77,12 @@ class MockVulkanContextBuilder { std::function settings_callback_; std::vector instance_extensions_; std::vector instance_layers_; + + static std::shared_ptr DefaultFenceCallback() { + return std::make_shared(); + } + std::function()> fence_factory_ = + DefaultFenceCallback; }; } // namespace testing diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc index de28491e69fed..1aa57b0345f21 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc @@ -6,6 +6,7 @@ #include "gtest/gtest.h" #include "impeller/renderer/backend/vulkan/command_pool_vk.h" #include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" +#include "vulkan/vulkan_enums.hpp" namespace impeller { namespace testing { @@ -33,5 +34,26 @@ TEST(MockVulkanContextTest, IsThreadSafe) { context->Shutdown(); } +TEST(MockVulkanContextTest, DefaultFenceAlwaysReportsSuccess) { + auto const context = MockVulkanContextBuilder().Build(); + auto const device = context->GetDevice(); + + auto fence = device.createFenceUnique({}).value; + EXPECT_EQ(vk::Result::eSuccess, device.getFenceStatus(*fence)); +} + +TEST(MockVulkanContextTest, MockedFenceReportsStatus) { + auto const context = MockVulkanContextBuilder().Build(); + + auto const device = context->GetDevice(); + auto fence = device.createFenceUnique({}).value; + MockFence::SetStatus(fence, vk::Result::eNotReady); + + EXPECT_EQ(vk::Result::eNotReady, device.getFenceStatus(fence.get())); + + MockFence::SetStatus(fence, vk::Result::eSuccess); + EXPECT_EQ(vk::Result::eSuccess, device.getFenceStatus(*fence)); +} + } // namespace testing } // namespace impeller From ce6a1c517d7c8c8afe689374b192e0e083e9f204 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 14 Sep 2023 17:12:00 -0700 Subject: [PATCH 09/21] Remove unused code. --- impeller/renderer/backend/vulkan/test/mock_vulkan.h | 6 ------ 1 file changed, 6 deletions(-) diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.h b/impeller/renderer/backend/vulkan/test/mock_vulkan.h index fa440f586c8da..71a88a42e02eb 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.h +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.h @@ -77,12 +77,6 @@ class MockVulkanContextBuilder { std::function settings_callback_; std::vector instance_extensions_; std::vector instance_layers_; - - static std::shared_ptr DefaultFenceCallback() { - return std::make_shared(); - } - std::function()> fence_factory_ = - DefaultFenceCallback; }; } // namespace testing From ac485e1ab764cd4c6aea22d94ea52d4de2648fb8 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 14 Sep 2023 22:38:39 -0700 Subject: [PATCH 10/21] Add stub unit test, remove unused IsValid() method. --- ci/licenses_golden/excluded_files | 1 + impeller/renderer/backend/vulkan/BUILD.gn | 1 + impeller/renderer/backend/vulkan/fence_waiter_vk.cc | 5 ----- .../backend/vulkan/fence_waiter_vk_unittests.cc | 10 ++++++++++ 4 files changed, 12 insertions(+), 5 deletions(-) create mode 100644 impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc diff --git a/ci/licenses_golden/excluded_files b/ci/licenses_golden/excluded_files index 08a10f984b050..8aa84c50351f1 100644 --- a/ci/licenses_golden/excluded_files +++ b/ci/licenses_golden/excluded_files @@ -154,6 +154,7 @@ ../../../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/fence_waiter_vk_unittests.cc ../../../flutter/impeller/renderer/backend/vulkan/pass_bindings_cache_unittests.cc ../../../flutter/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc ../../../flutter/impeller/renderer/backend/vulkan/test diff --git a/impeller/renderer/backend/vulkan/BUILD.gn b/impeller/renderer/backend/vulkan/BUILD.gn index 0c71fab1035a1..52b6b7aebbb9e 100644 --- a/impeller/renderer/backend/vulkan/BUILD.gn +++ b/impeller/renderer/backend/vulkan/BUILD.gn @@ -11,6 +11,7 @@ impeller_component("vulkan_unittests") { "blit_command_vk_unittests.cc", "command_encoder_vk_unittests.cc", "context_vk_unittests.cc", + "fnce_waiter_vk_unittests.cc", "pass_bindings_cache_unittests.cc", "resource_manager_vk_unittests.cc", "test/mock_vulkan.cc", diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc index 73ba0879dd3c2..ba9c1be9c22c5 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc @@ -47,7 +47,6 @@ class WaitSetEntry { FenceWaiterVK::FenceWaiterVK(std::weak_ptr device_holder) : device_holder_(std::move(device_holder)) { waiter_thread_ = std::make_unique([&]() { Main(); }); - is_valid_ = true; } FenceWaiterVK::~FenceWaiterVK() { @@ -57,10 +56,6 @@ FenceWaiterVK::~FenceWaiterVK() { } } -bool FenceWaiterVK::IsValid() const { - return is_valid_; -} - bool FenceWaiterVK::AddFence(vk::UniqueFence fence, const fml::closure& callback) { TRACE_EVENT0("flutter", "FenceWaiterVK::AddFence"); diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc new file mode 100644 index 0000000000000..f49a150f1c138 --- /dev/null +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc @@ -0,0 +1,10 @@ +// 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 "gtest/gtest.h" // IWYU pragma: keep +#include "impeller/renderer/backend/vulkan/fence_waiter_vk.h" + +TEST(FenceWaiterVK, IsValid) { + EXPECT_TRUE(true); +} From 50ef1c00a2296cb79d4a820a3e9cc67f3a6b8d7b Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 14 Sep 2023 23:57:10 -0700 Subject: [PATCH 11/21] Add unit tests, including those that fail. --- impeller/renderer/backend/vulkan/BUILD.gn | 2 +- .../renderer/backend/vulkan/context_vk.cc | 4 - .../backend/vulkan/fence_waiter_vk.cc | 7 +- .../vulkan/fence_waiter_vk_unittests.cc | 129 +++++++++++++++++- .../backend/vulkan/test/mock_vulkan.cc | 10 ++ .../backend/vulkan/test/mock_vulkan.h | 11 ++ 6 files changed, 151 insertions(+), 12 deletions(-) diff --git a/impeller/renderer/backend/vulkan/BUILD.gn b/impeller/renderer/backend/vulkan/BUILD.gn index 52b6b7aebbb9e..90781bdb0ad85 100644 --- a/impeller/renderer/backend/vulkan/BUILD.gn +++ b/impeller/renderer/backend/vulkan/BUILD.gn @@ -11,7 +11,7 @@ impeller_component("vulkan_unittests") { "blit_command_vk_unittests.cc", "command_encoder_vk_unittests.cc", "context_vk_unittests.cc", - "fnce_waiter_vk_unittests.cc", + "fence_waiter_vk_unittests.cc", "pass_bindings_cache_unittests.cc", "resource_manager_vk_unittests.cc", "test/mock_vulkan.cc", diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index a4748427e104a..88a86fa2c71a2 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -377,10 +377,6 @@ void ContextVK::Setup(Settings settings) { /// auto fence_waiter = std::shared_ptr(new FenceWaiterVK(device_holder)); - if (!fence_waiter->IsValid()) { - VALIDATION_LOG << "Could not create fence waiter."; - return; - } //---------------------------------------------------------------------------- /// Create the resource manager. diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc index ba9c1be9c22c5..e29490d071467 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc @@ -6,6 +6,7 @@ #include #include +#include #include "flutter/fml/thread.h" #include "flutter/fml/trace_event.h" @@ -51,15 +52,13 @@ FenceWaiterVK::FenceWaiterVK(std::weak_ptr device_holder) FenceWaiterVK::~FenceWaiterVK() { Terminate(); - if (waiter_thread_) { - waiter_thread_->join(); - } + waiter_thread_->join(); } bool FenceWaiterVK::AddFence(vk::UniqueFence fence, const fml::closure& callback) { TRACE_EVENT0("flutter", "FenceWaiterVK::AddFence"); - if (!IsValid() || !fence || !callback) { + if (!fence || !callback) { return false; } { diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc index f49a150f1c138..e295adbc96f0d 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc @@ -2,9 +2,132 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include "fml/synchronization/waitable_event.h" +#include "fml/time/time_delta.h" #include "gtest/gtest.h" // IWYU pragma: keep -#include "impeller/renderer/backend/vulkan/fence_waiter_vk.h" +#include "impeller/renderer/backend/vulkan/fence_waiter_vk.h" // IWYU pragma: keep +#include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" -TEST(FenceWaiterVK, IsValid) { - EXPECT_TRUE(true); +namespace impeller { +namespace testing { + +TEST(FenceWaiterVKTest, IgnoresNullFence) { + auto const context = MockVulkanContextBuilder().Build(); + auto const waiter = context->GetFenceWaiter(); + EXPECT_FALSE(waiter->AddFence(vk::UniqueFence(), []() {})); } + +TEST(FenceWaiterVKTest, IgnoresNullCallback) { + auto const context = MockVulkanContextBuilder().Build(); + auto const device = context->GetDevice(); + auto const waiter = context->GetFenceWaiter(); + + auto fence = device.createFenceUnique({}).value; + EXPECT_FALSE(waiter->AddFence(std::move(fence), nullptr)); +} + +TEST(FenceWaiterVKTest, ExecutesFenceCallback) { + auto const context = MockVulkanContextBuilder().Build(); + auto const device = context->GetDevice(); + auto const waiter = context->GetFenceWaiter(); + + auto signal = fml::ManualResetWaitableEvent(); + auto fence = device.createFenceUnique({}).value; + waiter->AddFence(std::move(fence), [&signal]() { signal.Signal(); }); + + signal.Wait(); +} + +TEST(FenceWaiterVKTest, ExecutesFenceCallbackX2) { + auto const context = MockVulkanContextBuilder().Build(); + auto const device = context->GetDevice(); + auto const waiter = context->GetFenceWaiter(); + + auto signal = fml::ManualResetWaitableEvent(); + auto fence = device.createFenceUnique({}).value; + waiter->AddFence(std::move(fence), [&signal]() { signal.Signal(); }); + + auto signal2 = fml::ManualResetWaitableEvent(); + auto fence2 = device.createFenceUnique({}).value; + waiter->AddFence(std::move(fence2), [&signal2]() { signal2.Signal(); }); + + signal.Wait(); + signal2.Wait(); +} + +TEST(FenceWaiterVKTest, ExecutesNewFenceThenOldFence) { + auto const context = MockVulkanContextBuilder().Build(); + auto const device = context->GetDevice(); + auto const waiter = context->GetFenceWaiter(); + + auto signal = fml::ManualResetWaitableEvent(); + auto fence = device.createFenceUnique({}).value; + auto raw_fence = MockFence::GetRawPointer(fence); + waiter->AddFence(std::move(fence), [&signal]() { signal.Signal(); }); + + // The easiest way to verify that the callback was _not_ called is to wait + // for a timeout, but that could introduce flakiness. Instead, we'll add a + // second fence that will signal immediately, and wait for that one instead. + { + auto signal2 = fml::ManualResetWaitableEvent(); + auto fence2 = device.createFenceUnique({}).value; + MockFence::SetStatus(fence2, vk::Result::eSuccess); + waiter->AddFence(std::move(fence2), [&signal2]() { signal2.Signal(); }); + signal2.Wait(); + } + + // Now, we'll signal the first fence, and wait for the callback to be called. + raw_fence->SetStatus(vk::Result::eSuccess); + + // Now, we'll signal the first fence, and wait for the callback to be called. + signal.Wait(); +} + +TEST(FenceWaiterVKTest, DoesNotLeakFenceAddedAfterTermination) { + MockFence* raw_fence = nullptr; + auto signal = fml::ManualResetWaitableEvent(); + + { + auto const context = MockVulkanContextBuilder().Build(); + auto const device = context->GetDevice(); + auto const waiter = context->GetFenceWaiter(); + waiter->Terminate(); + + auto fence = device.createFenceUnique({}).value; + raw_fence = MockFence::GetRawPointer(fence); + waiter->AddFence(std::move(fence), [&signal]() { signal.Signal(); }); + } + + // Ensure the fence was destroyed. + EXPECT_FALSE(raw_fence) << "Fence was not destroyed despite destruction."; +} + +TEST(FenceWaiterVKTest, InProgressFencesStillWaitIfTerminated) { + MockFence* raw_fence = nullptr; + auto signal = fml::ManualResetWaitableEvent(); + + { + auto const context = MockVulkanContextBuilder().Build(); + auto const device = context->GetDevice(); + auto const waiter = context->GetFenceWaiter(); + + // Add a fence that isn't signalled yet. + auto fence = device.createFenceUnique({}).value; + + // Even if the fence is eSuccess, it's not guaranteed to be called in time. + MockFence::SetStatus(fence, vk::Result::eNotReady); + raw_fence = MockFence::GetRawPointer(fence); + waiter->AddFence(std::move(fence), [&signal]() { signal.Signal(); }); + + // Terminate the waiter by letting it drop out of scope. + } + + // Signal the fence. + raw_fence->SetStatus(vk::Result::eSuccess); + + // Ensure the callback is still called, because that would cleanup the fence. + EXPECT_TRUE(signal.WaitWithTimeout(fml::TimeDelta::FromMilliseconds(2000))); +} + +} // 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 01749168695d1..5e91522f52d90 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -425,6 +425,14 @@ VkResult vkCreateFence(VkDevice device, return VK_SUCCESS; } +VkResult vkDestroyFence(VkDevice device, + VkFence fence, + const VkAllocationCallbacks* pAllocator) { + MockDevice* mock_device = reinterpret_cast(device); + delete reinterpret_cast(fence); + return VK_SUCCESS; +} + VkResult vkQueueSubmit(VkQueue queue, uint32_t submitCount, const VkSubmitInfo* pSubmits, @@ -546,6 +554,8 @@ PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance, return (PFN_vkVoidFunction)vkEndCommandBuffer; } else if (strcmp("vkCreateFence", pName) == 0) { return (PFN_vkVoidFunction)vkCreateFence; + } else if (strcmp("vkDestroyFence", pName) == 0) { + return (PFN_vkVoidFunction)vkDestroyFence; } else if (strcmp("vkQueueSubmit", pName) == 0) { return (PFN_vkVoidFunction)vkQueueSubmit; } else if (strcmp("vkWaitForFences", pName) == 0) { diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.h b/impeller/renderer/backend/vulkan/test/mock_vulkan.h index 71a88a42e02eb..f28c96b445b5b 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.h +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.h @@ -26,6 +26,9 @@ class MockFence final { // Returns the result that was set in the constructor or |SetStatus|. VkResult GetStatus() { return static_cast(result_); } + // Sets the result that will be returned by `GetFenceStatus`. + void SetStatus(vk::Result result) { result_ = result; } + // Sets the result that will be returned by `GetFenceStatus`. static void SetStatus(vk::UniqueFence& fence, vk::Result result) { // Cast the fence to a MockFence and set the result. @@ -34,6 +37,14 @@ class MockFence final { mock_fence->result_ = result; } + // Gets a raw pointer to manipulate the fence after it's been moved. + static MockFence* GetRawPointer(vk::UniqueFence& fence) { + // Cast the fence to a MockFence and get the result. + VkFence raw_fence = fence.get(); + MockFence* mock_fence = reinterpret_cast(raw_fence); + return mock_fence; + } + private: vk::Result result_ = vk::Result::eSuccess; From 94500af487c79a827c6dccd4c5c57eae97182388 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 15 Sep 2023 00:30:23 -0700 Subject: [PATCH 12/21] Fix fence waiter and add tests. --- .../backend/vulkan/fence_waiter_vk.cc | 122 ++++++++++-------- .../renderer/backend/vulkan/fence_waiter_vk.h | 4 +- .../vulkan/fence_waiter_vk_unittests.cc | 28 ++-- 3 files changed, 87 insertions(+), 67 deletions(-) diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc index e29490d071467..ca4c62ff13b26 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc @@ -95,76 +95,90 @@ void FenceWaiterVK::Main() { // critical section. Copy the array of entries and immediately unlock the // mutex. WaitSet wait_set = wait_set_; - - const auto terminate = terminate_; - lock.unlock(); + const auto terminate = terminate_; if (terminate) { + WaitUntilEmpty(); break; } - // Check if the context had died in the meantime. - auto device_holder = device_holder_.lock(); - if (!device_holder) { + if (!Wait(wait_set)) { break; } + } +} - const auto& device = device_holder->GetDevice(); +void FenceWaiterVK::WaitUntilEmpty() { + while (!wait_set_.empty() && Wait(wait_set_)) { + // Intentionally empty. + } +} - // 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 - // to be signaled at an abnormally long deadline is the only one in the set, - // a timeout will bail out the wait. - auto fences = GetFencesForWaitSet(wait_set); - if (fences.empty()) { - continue; - } +bool FenceWaiterVK::Wait(WaitSet& wait_set) { + using namespace std::literals::chrono_literals; - 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. Tearing " - "down the waiter thread."; - break; - } + // Check if the context had died in the meantime. + auto device_holder = device_holder_.lock(); + if (!device_holder) { + return false; + } - // One or more fences have been signaled. Find out which ones and update - // their signaled statuses. - { - TRACE_EVENT0("impeller", "CheckFenceStatus"); - for (auto& entry : wait_set) { - entry->UpdateSignalledStatus(device); - } - wait_set.clear(); - } + 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 + // to be signaled at an abnormally long deadline is the only one in the set, + // a timeout will bail out the wait. + auto fences = GetFencesForWaitSet(wait_set); + if (fences.empty()) { + return true; + } - // Quickly acquire the wait set lock and erase signaled entries. Make sure - // the mutex is unlocked before calling the destructors of the erased - // entries. These might touch allocators. - WaitSet erased_entries; - { - static auto is_signalled = [](const auto& entry) { - return entry->IsSignalled(); - }; - std::scoped_lock lock(wait_set_mutex_); - std::copy_if(wait_set_.begin(), wait_set_.end(), - std::back_inserter(erased_entries), is_signalled); - wait_set_.erase( - std::remove_if(wait_set_.begin(), wait_set_.end(), is_signalled), - wait_set_.end()); - } + 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. Tearing " + "down the waiter thread."; + return false; + } - { - TRACE_EVENT0("impeller", "ClearSignaledFences"); - // Erase the erased entries which will invoke callbacks. - erased_entries.clear(); // Bit redundant because of scope but hey. + // One or more fences have been signaled. Find out which ones and update + // their signaled statuses. + { + TRACE_EVENT0("impeller", "CheckFenceStatus"); + for (auto& entry : wait_set) { + entry->UpdateSignalledStatus(device); } + wait_set.clear(); + } + + // Quickly acquire the wait set lock and erase signaled entries. Make sure + // the mutex is unlocked before calling the destructors of the erased + // entries. These might touch allocators. + WaitSet erased_entries; + { + static auto is_signalled = [](const auto& entry) { + return entry->IsSignalled(); + }; + std::scoped_lock lock(wait_set_mutex_); + std::copy_if(wait_set_.begin(), wait_set_.end(), + std::back_inserter(erased_entries), is_signalled); + wait_set_.erase( + std::remove_if(wait_set_.begin(), wait_set_.end(), is_signalled), + wait_set_.end()); } + + { + TRACE_EVENT0("impeller", "ClearSignaledFences"); + // Erase the erased entries which will invoke callbacks. + erased_entries.clear(); // Bit redundant because of scope but hey. + } + + return true; } void FenceWaiterVK::Terminate() { diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk.h b/impeller/renderer/backend/vulkan/fence_waiter_vk.h index 7e562433ddb9d..0ddb18bd5f870 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk.h +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk.h @@ -42,12 +42,14 @@ class FenceWaiterVK { std::condition_variable wait_set_cv_; WaitSet wait_set_; bool terminate_ = false; - bool is_valid_ = false; explicit FenceWaiterVK(std::weak_ptr device_holder); void Main(); + bool Wait(WaitSet& wait_set); + void WaitUntilEmpty(); + FML_DISALLOW_COPY_AND_ASSIGN(FenceWaiterVK); }; diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc index e295adbc96f0d..3f622330b795a 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc @@ -83,8 +83,7 @@ TEST(FenceWaiterVKTest, ExecutesNewFenceThenOldFence) { signal.Wait(); } -TEST(FenceWaiterVKTest, DoesNotLeakFenceAddedAfterTermination) { - MockFence* raw_fence = nullptr; +TEST(FenceWaiterVKTest, StillDestroysFenceIfTerminating) { auto signal = fml::ManualResetWaitableEvent(); { @@ -94,17 +93,15 @@ TEST(FenceWaiterVKTest, DoesNotLeakFenceAddedAfterTermination) { waiter->Terminate(); auto fence = device.createFenceUnique({}).value; - raw_fence = MockFence::GetRawPointer(fence); waiter->AddFence(std::move(fence), [&signal]() { signal.Signal(); }); } - // Ensure the fence was destroyed. - EXPECT_FALSE(raw_fence) << "Fence was not destroyed despite destruction."; + // Ensure the fence still triggers. + signal.Wait(); } TEST(FenceWaiterVKTest, InProgressFencesStillWaitIfTerminated) { MockFence* raw_fence = nullptr; - auto signal = fml::ManualResetWaitableEvent(); { auto const context = MockVulkanContextBuilder().Build(); @@ -117,16 +114,23 @@ TEST(FenceWaiterVKTest, InProgressFencesStillWaitIfTerminated) { // Even if the fence is eSuccess, it's not guaranteed to be called in time. MockFence::SetStatus(fence, vk::Result::eNotReady); raw_fence = MockFence::GetRawPointer(fence); - waiter->AddFence(std::move(fence), [&signal]() { signal.Signal(); }); + waiter->AddFence(std::move(fence), []() { + // Intentionally empty, imagine this is holding on to a reference. + }); + + // Spawn a thread to signal the fence. + std::thread([&]() { + // Wait 1 second. + std::this_thread::sleep_for(std::chrono::seconds(1)); + + // Signal the fence. + raw_fence->SetStatus(vk::Result::eSuccess); + }).detach(); // Terminate the waiter by letting it drop out of scope. } - // Signal the fence. - raw_fence->SetStatus(vk::Result::eSuccess); - - // Ensure the callback is still called, because that would cleanup the fence. - EXPECT_TRUE(signal.WaitWithTimeout(fml::TimeDelta::FromMilliseconds(2000))); + // This will hang if the fence was not signalled. } } // namespace testing From 55405424ff203152cb672119f1a0a34d1bd3a56f Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 15 Sep 2023 01:35:04 -0700 Subject: [PATCH 13/21] Fix buggy fence waiter test. --- impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc index 3f622330b795a..020568d3278e4 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc @@ -62,6 +62,7 @@ TEST(FenceWaiterVKTest, ExecutesNewFenceThenOldFence) { auto signal = fml::ManualResetWaitableEvent(); auto fence = device.createFenceUnique({}).value; + MockFence::SetStatus(fence, vk::Result::eNotReady); auto raw_fence = MockFence::GetRawPointer(fence); waiter->AddFence(std::move(fence), [&signal]() { signal.Signal(); }); From 2efd5e8bab78bc25dbdee3bc13c54b849294d347 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 15 Sep 2023 01:51:15 -0700 Subject: [PATCH 14/21] Make mocks thread-safe and tweak. --- .../vulkan/fence_waiter_vk_unittests.cc | 37 ++++++++----------- .../backend/vulkan/test/mock_vulkan.h | 9 ++++- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc index 020568d3278e4..72f5c0e52b99a 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc @@ -103,35 +103,28 @@ TEST(FenceWaiterVKTest, StillDestroysFenceIfTerminating) { TEST(FenceWaiterVKTest, InProgressFencesStillWaitIfTerminated) { MockFence* raw_fence = nullptr; + auto signal = fml::ManualResetWaitableEvent(); - { - auto const context = MockVulkanContextBuilder().Build(); - auto const device = context->GetDevice(); - auto const waiter = context->GetFenceWaiter(); - - // Add a fence that isn't signalled yet. - auto fence = device.createFenceUnique({}).value; + auto const context = MockVulkanContextBuilder().Build(); + auto const device = context->GetDevice(); + auto const waiter = context->GetFenceWaiter(); - // Even if the fence is eSuccess, it's not guaranteed to be called in time. - MockFence::SetStatus(fence, vk::Result::eNotReady); - raw_fence = MockFence::GetRawPointer(fence); - waiter->AddFence(std::move(fence), []() { - // Intentionally empty, imagine this is holding on to a reference. - }); + // Add a fence that isn't signalled yet. + auto fence = device.createFenceUnique({}).value; - // Spawn a thread to signal the fence. - std::thread([&]() { - // Wait 1 second. - std::this_thread::sleep_for(std::chrono::seconds(1)); + // Even if the fence is eSuccess, it's not guaranteed to be called in time. + MockFence::SetStatus(fence, vk::Result::eNotReady); + raw_fence = MockFence::GetRawPointer(fence); + waiter->AddFence(std::move(fence), [&signal]() { signal.Signal(); }); - // Signal the fence. - raw_fence->SetStatus(vk::Result::eSuccess); - }).detach(); + // Terminate the waiter. + waiter->Terminate(); - // Terminate the waiter by letting it drop out of scope. - } + // Signal the fence. + raw_fence->SetStatus(vk::Result::eSuccess); // This will hang if the fence was not signalled. + signal.Wait(); } } // namespace testing diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.h b/impeller/renderer/backend/vulkan/test/mock_vulkan.h index f28c96b445b5b..9800b2a17b813 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.h +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.h @@ -9,6 +9,7 @@ #include #include +#include "impeller/base/thread.h" #include "impeller/renderer/backend/vulkan/context_vk.h" #include "vulkan/vulkan_enums.hpp" @@ -24,7 +25,10 @@ class MockFence final { MockFence() = default; // Returns the result that was set in the constructor or |SetStatus|. - VkResult GetStatus() { return static_cast(result_); } + VkResult GetStatus() { + Lock lock(result_mutex_); + return static_cast(result_); + } // Sets the result that will be returned by `GetFenceStatus`. void SetStatus(vk::Result result) { result_ = result; } @@ -34,7 +38,7 @@ class MockFence final { // Cast the fence to a MockFence and set the result. VkFence raw_fence = fence.get(); MockFence* mock_fence = reinterpret_cast(raw_fence); - mock_fence->result_ = result; + mock_fence->SetStatus(result); } // Gets a raw pointer to manipulate the fence after it's been moved. @@ -47,6 +51,7 @@ class MockFence final { private: vk::Result result_ = vk::Result::eSuccess; + Mutex result_mutex_; FML_DISALLOW_COPY_AND_ASSIGN(MockFence); }; From 4e1861c4373f97fac282c86f749ab68f256f8b4d Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 15 Sep 2023 10:39:59 -0700 Subject: [PATCH 15/21] Use atomics, delete unused code. --- impeller/renderer/backend/vulkan/test/mock_vulkan.cc | 1 - impeller/renderer/backend/vulkan/test/mock_vulkan.h | 8 ++------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index f46c25f22b701..53017eeee8172 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -428,7 +428,6 @@ VkResult vkCreateFence(VkDevice device, VkResult vkDestroyFence(VkDevice device, VkFence fence, const VkAllocationCallbacks* pAllocator) { - MockDevice* mock_device = reinterpret_cast(device); delete reinterpret_cast(fence); return VK_SUCCESS; } diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.h b/impeller/renderer/backend/vulkan/test/mock_vulkan.h index 9800b2a17b813..8f41b9f86ee62 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.h +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.h @@ -25,10 +25,7 @@ class MockFence final { MockFence() = default; // Returns the result that was set in the constructor or |SetStatus|. - VkResult GetStatus() { - Lock lock(result_mutex_); - return static_cast(result_); - } + VkResult GetStatus() { return static_cast(result_.load()); } // Sets the result that will be returned by `GetFenceStatus`. void SetStatus(vk::Result result) { result_ = result; } @@ -50,8 +47,7 @@ class MockFence final { } private: - vk::Result result_ = vk::Result::eSuccess; - Mutex result_mutex_; + std::atomic result_ = vk::Result::eSuccess; FML_DISALLOW_COPY_AND_ASSIGN(MockFence); }; From 14608419cc405bb9ed28249d61c043f0864cb181 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 15 Sep 2023 10:54:48 -0700 Subject: [PATCH 16/21] Tweak fence termination contract. --- impeller/renderer/backend/vulkan/fence_waiter_vk.cc | 2 +- .../renderer/backend/vulkan/fence_waiter_vk_unittests.cc | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc index ca4c62ff13b26..adee355018f6c 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc @@ -58,7 +58,7 @@ FenceWaiterVK::~FenceWaiterVK() { bool FenceWaiterVK::AddFence(vk::UniqueFence fence, const fml::closure& callback) { TRACE_EVENT0("flutter", "FenceWaiterVK::AddFence"); - if (!fence || !callback) { + if (!fence || !callback || terminate_) { return false; } { diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc index 72f5c0e52b99a..4acfdc1131fab 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc @@ -3,7 +3,6 @@ // found in the LICENSE file. #include "fml/synchronization/waitable_event.h" -#include "fml/time/time_delta.h" #include "gtest/gtest.h" // IWYU pragma: keep #include "impeller/renderer/backend/vulkan/fence_waiter_vk.h" // IWYU pragma: keep #include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" @@ -84,7 +83,7 @@ TEST(FenceWaiterVKTest, ExecutesNewFenceThenOldFence) { signal.Wait(); } -TEST(FenceWaiterVKTest, StillDestroysFenceIfTerminating) { +TEST(FenceWaiterVKTest, AddFenceDoesNothingIfTerminating) { auto signal = fml::ManualResetWaitableEvent(); { @@ -97,8 +96,8 @@ TEST(FenceWaiterVKTest, StillDestroysFenceIfTerminating) { waiter->AddFence(std::move(fence), [&signal]() { signal.Signal(); }); } - // Ensure the fence still triggers. - signal.Wait(); + // Ensure the fence did _not_ signal. + EXPECT_FALSE(signal.WaitWithTimeout(fml::TimeDelta::FromMilliseconds(100))); } TEST(FenceWaiterVKTest, InProgressFencesStillWaitIfTerminated) { From 5bdcb65f21c49e5a179e2d585b66186b9ab651b9 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 15 Sep 2023 11:15:13 -0700 Subject: [PATCH 17/21] false -> true --- impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc index 4acfdc1131fab..ef2ff5d40cbb9 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc @@ -97,7 +97,7 @@ TEST(FenceWaiterVKTest, AddFenceDoesNothingIfTerminating) { } // Ensure the fence did _not_ signal. - EXPECT_FALSE(signal.WaitWithTimeout(fml::TimeDelta::FromMilliseconds(100))); + EXPECT_TRUE(signal.WaitWithTimeout(fml::TimeDelta::FromMilliseconds(100))); } TEST(FenceWaiterVKTest, InProgressFencesStillWaitIfTerminated) { From e5ab21b0b9f1c012bd724c7726f947cad28c6ade Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 15 Sep 2023 11:29:49 -0700 Subject: [PATCH 18/21] Address some comments and add a TODO. --- impeller/renderer/backend/vulkan/fence_waiter_vk.cc | 13 +++++++------ .../renderer/backend/vulkan/test/mock_vulkan.cc | 7 ------- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc index adee355018f6c..b816163595e66 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc @@ -135,11 +135,10 @@ bool FenceWaiterVK::Wait(WaitSet& wait_set) { } auto result = device.waitForFences( - fences.size(), // fences count - fences.data(), // fences - false, // wait for all - std::chrono::nanoseconds{100ms}.count() // timeout (ns) - ); + /*fenceCount=*/fences.size(), + /*pFences=*/fences.data(), + /*waitAll=*/false, + /*timeout=*/std::chrono::nanoseconds{100ms}.count()); if (!(result == vk::Result::eSuccess || result == vk::Result::eTimeout)) { VALIDATION_LOG << "Fence waiter encountered an unexpected error. Tearing " "down the waiter thread."; @@ -161,10 +160,12 @@ bool FenceWaiterVK::Wait(WaitSet& wait_set) { // entries. These might touch allocators. WaitSet erased_entries; { - static auto is_signalled = [](const auto& entry) { + static constexpr auto is_signalled = [](const auto& entry) { return entry->IsSignalled(); }; std::scoped_lock lock(wait_set_mutex_); + + // TODO(matanlurey): Iterate the list 1x by copying is_signaled into erased. std::copy_if(wait_set_.begin(), wait_set_.end(), std::back_inserter(erased_entries), is_signalled); wait_set_.erase( diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index 53017eeee8172..393916b81023f 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -45,11 +45,6 @@ class MockDevice final { called_functions_->push_back(function); } - void SetFenceFactory( - std::function()> fence_factory) { - fence_factory_ = std::move(fence_factory); - } - private: FML_DISALLOW_COPY_AND_ASSIGN(MockDevice); @@ -60,8 +55,6 @@ class MockDevice final { Mutex command_buffers_mutex_; std::vector> command_buffers_ IPLR_GUARDED_BY(command_buffers_mutex_); - - std::function()> fence_factory_; }; void noop() {} From 110f745caa388bac5c2892cb20d8ea09e1753074 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 15 Sep 2023 11:35:10 -0700 Subject: [PATCH 19/21] Add FML_DCHECK. --- impeller/renderer/backend/vulkan/fence_waiter_vk.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc index b816163595e66..5ad1cf8fdd9fa 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc @@ -110,6 +110,9 @@ void FenceWaiterVK::Main() { } void FenceWaiterVK::WaitUntilEmpty() { + // Note, there is no lock because once terminate_ is set to true, no other + // fence can be added to the wait set. Just in case, here's a FML_DCHECK: + FML_DCHECK(terminate_) << "Fence waiter must be terminated."; while (!wait_set_.empty() && Wait(wait_set_)) { // Intentionally empty. } From d9735741e0980df0020e23f48a5cff904b923970 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 15 Sep 2023 12:57:03 -0700 Subject: [PATCH 20/21] Address feedback. --- .../backend/vulkan/fence_waiter_vk.cc | 43 +++++++++++++------ .../renderer/backend/vulkan/fence_waiter_vk.h | 2 +- 2 files changed, 31 insertions(+), 14 deletions(-) diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc index 5ad1cf8fdd9fa..b729640896343 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc @@ -58,11 +58,15 @@ FenceWaiterVK::~FenceWaiterVK() { bool FenceWaiterVK::AddFence(vk::UniqueFence fence, const fml::closure& callback) { TRACE_EVENT0("flutter", "FenceWaiterVK::AddFence"); - if (!fence || !callback || terminate_) { + if (!fence || !callback) { return false; } { + // Maintain the invariant that terminate_ is accessed only under the lock. std::scoped_lock lock(wait_set_mutex_); + if (terminate_) { + return false; + } wait_set_.emplace_back(WaitSetEntry::Create(std::move(fence), callback)); } wait_set_cv_.notify_one(); @@ -86,24 +90,30 @@ void FenceWaiterVK::Main() { using namespace std::literals::chrono_literals; while (true) { - std::unique_lock lock(wait_set_mutex_); + bool terminate; + + { + std::unique_lock lock(wait_set_mutex_); - // If there are no fences to wait on, wait on the condition variable. - wait_set_cv_.wait(lock, [&]() { return !wait_set_.empty() || terminate_; }); + // If there are no fences to wait on, wait on the condition variable. + wait_set_cv_.wait(lock, + [&]() { return !wait_set_.empty() || terminate_; }); - // We don't want to check on fence status or collect wait set entries in the - // critical section. Copy the array of entries and immediately unlock the - // mutex. - WaitSet wait_set = wait_set_; - lock.unlock(); + // We don't want to check on fence status or collect wait set entries in + // the critical section. Copy the array of entries and immediately unlock + // the mutex. + WaitSet wait_set = wait_set_; + + // Still under the lock, check if the waiter has been terminated. + terminate = terminate_; + } - const auto terminate = terminate_; if (terminate) { WaitUntilEmpty(); break; } - if (!Wait(wait_set)) { + if (!Wait()) { break; } } @@ -113,12 +123,19 @@ void FenceWaiterVK::WaitUntilEmpty() { // Note, there is no lock because once terminate_ is set to true, no other // fence can be added to the wait set. Just in case, here's a FML_DCHECK: FML_DCHECK(terminate_) << "Fence waiter must be terminated."; - while (!wait_set_.empty() && Wait(wait_set_)) { + while (!wait_set_.empty() && Wait()) { // Intentionally empty. } } -bool FenceWaiterVK::Wait(WaitSet& wait_set) { +bool FenceWaiterVK::Wait() { + // Snapshot the wait set and wait on the fences. + WaitSet wait_set; + { + std::scoped_lock lock(wait_set_mutex_); + wait_set = wait_set_; + } + using namespace std::literals::chrono_literals; // Check if the context had died in the meantime. diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk.h b/impeller/renderer/backend/vulkan/fence_waiter_vk.h index 0ddb18bd5f870..83050c554d037 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk.h +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk.h @@ -47,7 +47,7 @@ class FenceWaiterVK { void Main(); - bool Wait(WaitSet& wait_set); + bool Wait(); void WaitUntilEmpty(); FML_DISALLOW_COPY_AND_ASSIGN(FenceWaiterVK); From fe3c9266b83d3f00cb89a449abc8015f034c7eec Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 15 Sep 2023 14:31:52 -0700 Subject: [PATCH 21/21] Address comments. --- impeller/renderer/backend/vulkan/fence_waiter_vk.cc | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc index b729640896343..4a16c7a79a716 100644 --- a/impeller/renderer/backend/vulkan/fence_waiter_vk.cc +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk.cc @@ -90,7 +90,8 @@ void FenceWaiterVK::Main() { using namespace std::literals::chrono_literals; while (true) { - bool terminate; + // We'll read the terminate_ flag within the lock below. + bool terminate = false; { std::unique_lock lock(wait_set_mutex_); @@ -99,11 +100,6 @@ void FenceWaiterVK::Main() { wait_set_cv_.wait(lock, [&]() { return !wait_set_.empty() || terminate_; }); - // We don't want to check on fence status or collect wait set entries in - // the critical section. Copy the array of entries and immediately unlock - // the mutex. - WaitSet wait_set = wait_set_; - // Still under the lock, check if the waiter has been terminated. terminate = terminate_; }