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..90781bdb0ad85 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", + "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 73ba0879dd3c2..4a16c7a79a716 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" @@ -47,28 +48,25 @@ 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() { Terminate(); - if (waiter_thread_) { - waiter_thread_->join(); - } -} - -bool FenceWaiterVK::IsValid() const { - return is_valid_; + 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; } { + // 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(); @@ -92,85 +90,112 @@ void FenceWaiterVK::Main() { using namespace std::literals::chrono_literals; while (true) { - 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_; }); + // We'll read the terminate_ flag within the lock below. + bool terminate = false; - // 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_; + { + std::unique_lock lock(wait_set_mutex_); - const auto terminate = terminate_; + // If there are no fences to wait on, wait on the condition variable. + wait_set_cv_.wait(lock, + [&]() { return !wait_set_.empty() || terminate_; }); - lock.unlock(); + // Still under the lock, check if the waiter has been terminated. + 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()) { break; } + } +} - const auto& device = device_holder->GetDevice(); +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()) { + // 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() { + // Snapshot the wait set and wait on the fences. + WaitSet wait_set; + { + std::scoped_lock lock(wait_set_mutex_); + wait_set = wait_set_; + } - 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; - } + using namespace std::literals::chrono_literals; - // 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(); - } + // Check if the context had died in the meantime. + auto device_holder = device_holder_.lock(); + if (!device_holder) { + return false; + } - // 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()); - } + 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; + } - { - TRACE_EVENT0("impeller", "ClearSignaledFences"); - // Erase the erased entries which will invoke callbacks. - erased_entries.clear(); // Bit redundant because of scope but hey. + auto result = device.waitForFences( + /*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."; + 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(); } + + // 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 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( + 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..83050c554d037 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(); + 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 new file mode 100644 index 0000000000000..ef2ff5d40cbb9 --- /dev/null +++ b/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc @@ -0,0 +1,130 @@ +// 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 "fml/synchronization/waitable_event.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" + +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; + MockFence::SetStatus(fence, vk::Result::eNotReady); + 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, AddFenceDoesNothingIfTerminating) { + 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; + waiter->AddFence(std::move(fence), [&signal]() { signal.Signal(); }); + } + + // Ensure the fence did _not_ signal. + EXPECT_TRUE(signal.WaitWithTimeout(fml::TimeDelta::FromMilliseconds(100))); +} + +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. + waiter->Terminate(); + + // Signal the fence. + raw_fence->SetStatus(vk::Result::eSuccess); + + // This will hang if the fence was not signalled. + signal.Wait(); +} + +} // 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 ed311ac017a81..393916b81023f 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 { @@ -410,7 +413,15 @@ 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; +} + +VkResult vkDestroyFence(VkDevice device, + VkFence fence, + const VkAllocationCallbacks* pAllocator) { + delete reinterpret_cast(fence); 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( @@ -533,6 +546,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 41c82c6da2a90..8f41b9f86ee62 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.h +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.h @@ -5,10 +5,13 @@ #pragma once #include +#include #include #include +#include "impeller/base/thread.h" #include "impeller/renderer/backend/vulkan/context_vk.h" +#include "vulkan/vulkan_enums.hpp" namespace impeller { namespace testing { @@ -16,6 +19,39 @@ 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_.load()); } + + // 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. + VkFence raw_fence = fence.get(); + MockFence* mock_fence = reinterpret_cast(raw_fence); + mock_fence->SetStatus(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: + std::atomic result_ = vk::Result::eSuccess; + + FML_DISALLOW_COPY_AND_ASSIGN(MockFence); +}; + class MockVulkanContextBuilder { public: MockVulkanContextBuilder(); 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