From c0192e7543f35ba4fcd62b771a65bdcf4b6f7e62 Mon Sep 17 00:00:00 2001 From: moudgils <47460854+moudgils@users.noreply.github.com> Date: Mon, 15 Nov 2021 09:56:28 -0800 Subject: [PATCH 1/3] Remove CPU Spin lock to help conserve power/perf (#5597) Signed-off-by: moudgils --- Gems/Atom/RHI/Code/Include/Atom/RHI/CommandQueue.h | 2 ++ Gems/Atom/RHI/Code/Source/RHI/CommandQueue.cpp | 12 +++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/Gems/Atom/RHI/Code/Include/Atom/RHI/CommandQueue.h b/Gems/Atom/RHI/Code/Include/Atom/RHI/CommandQueue.h index 2985581040..4ae560c816 100644 --- a/Gems/Atom/RHI/Code/Include/Atom/RHI/CommandQueue.h +++ b/Gems/Atom/RHI/Code/Include/Atom/RHI/CommandQueue.h @@ -72,6 +72,8 @@ namespace AZ AZStd::mutex m_workQueueMutex; AZStd::queue m_workQueue; AZStd::condition_variable m_workQueueCondition; + AZStd::mutex m_flushCommandsMutex; + AZStd::condition_variable m_flushCommandsCondition; AZStd::atomic_bool m_isWorkQueueEmpty; AZStd::atomic_bool m_isQuitting; }; diff --git a/Gems/Atom/RHI/Code/Source/RHI/CommandQueue.cpp b/Gems/Atom/RHI/Code/Source/RHI/CommandQueue.cpp index a365b23e94..1cee382189 100644 --- a/Gems/Atom/RHI/Code/Source/RHI/CommandQueue.cpp +++ b/Gems/Atom/RHI/Code/Source/RHI/CommandQueue.cpp @@ -71,6 +71,7 @@ namespace AZ { m_isQuitting = true; m_workQueueCondition.notify_all(); + m_flushCommandsCondition.notify_all(); if (m_thread.joinable()) { m_thread.join(); @@ -102,9 +103,10 @@ namespace AZ void CommandQueue::FlushCommands() { AZ_PROFILE_SCOPE(RHI, "CommandQueue: FlushCommands"); - while (!m_isWorkQueueEmpty && !m_isQuitting) + AZStd::unique_lock lock(m_flushCommandsMutex); + if (!m_isWorkQueueEmpty && !m_isQuitting) { - AZStd::this_thread::yield(); + m_flushCommandsCondition.wait(lock, [this]() { return m_isWorkQueueEmpty.load() || m_isQuitting.load(); }); } } @@ -119,7 +121,11 @@ namespace AZ if (m_workQueue.empty()) { - m_isWorkQueueEmpty = true; + { + AZStd::unique_lock flushCommandsLock(m_flushCommandsMutex); + m_isWorkQueueEmpty = true; + m_flushCommandsCondition.notify_all(); + } m_workQueueCondition.wait(lock, [this]() { return !m_workQueue.empty() || m_isQuitting; }); } From 0cea59d66998585f07c54e55f68dd117363fb4ea Mon Sep 17 00:00:00 2001 From: amzn-mike <80125227+amzn-mike@users.noreply.github.com> Date: Mon, 15 Nov 2021 13:28:06 -0600 Subject: [PATCH 2/3] [LYN-7463] Fix insert streaming request failure (#5604) * Fix race condition where asset would finish loading and another request would start before the streamer request could be cleared Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com> * Re-enable disabled test which was failing for the same reason. Fix test timeout which was way too long. Reduce test iterations to keep test time safely under 5 seconds. Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com> * Instead of using a mutex, re-order the statements to remove the streamer request first Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com> --- Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp | 6 +++++- .../AzCore/Tests/Asset/AssetManagerLoadingTests.cpp | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp b/Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp index 06bb0b0cac..b03e1affdc 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp +++ b/Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp @@ -1677,9 +1677,13 @@ namespace AZ // they will trigger a ReleaseAsset call sometime after the AssetManager has begun to shut down, which can lead to // race conditions. + // Make sure the streamer request is removed first before the asset is released + // If the asset is released first it could lead to a race condition where another thread starts loading the asset + // again and attempts to add a new streamer request with the same ID before the old one has been removed, causing + // that load request to fail + RemoveActiveStreamerRequest(assetId); weakAsset = {}; loadingAsset.Reset(); - RemoveActiveStreamerRequest(assetId); }; auto&& [deadline, priority] = GetEffectiveDeadlineAndPriority(*handler, asset.GetType(), loadParams); diff --git a/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp b/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp index e33dbce9c1..3f2d9491a7 100644 --- a/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp +++ b/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp @@ -652,7 +652,7 @@ namespace UnitTest threads.emplace_back([this, &threadCount, &cv, assetUuid]() { bool checkLoaded = true; - for (int i = 0; i < 5000; i++) + for (int i = 0; i < 1000; i++) { Asset asset1 = m_testAssetManager->GetAsset(assetUuid, azrtti_typeid(), AZ::Data::AssetLoadBehavior::PreLoad); @@ -678,7 +678,7 @@ namespace UnitTest while (threadCount > 0 && !timedOut) { AZStd::unique_lock lock(mutex); - timedOut = (AZStd::cv_status::timeout == cv.wait_until(lock, AZStd::chrono::system_clock::now() + DefaultTimeoutSeconds * 20000)); + timedOut = (AZStd::cv_status::timeout == cv.wait_until(lock, AZStd::chrono::system_clock::now() + DefaultTimeoutSeconds)); } ASSERT_EQ(threadCount, 0) << "Thread count is non-zero, a thread has likely deadlocked. Test will not shut down cleanly."; @@ -1190,7 +1190,7 @@ namespace UnitTest #if AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS TEST_F(AssetJobsFloodTest, DISABLED_ContainerFilterTest_ContainersWithAndWithoutFiltering_Success) #else - TEST_F(AssetJobsFloodTest, DISABLED_ContainerFilterTest_ContainersWithAndWithoutFiltering_Success) + TEST_F(AssetJobsFloodTest, ContainerFilterTest_ContainersWithAndWithoutFiltering_Success) #endif // !AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS { m_assetHandlerAndCatalog->AssetCatalogRequestBus::Handler::BusConnect(); From 63713ca284cb8ed9bd0720359805c92bf2badb13 Mon Sep 17 00:00:00 2001 From: amzn-mike <80125227+amzn-mike@users.noreply.github.com> Date: Mon, 15 Nov 2021 14:23:56 -0600 Subject: [PATCH 3/3] Fix AssetBus connection policy to re-lock the mutex afterward since some destructors in the calling methods are still altering the context (#5575) Also made the unlock conditional. Signed-off-by: amzn-mike <80125227+amzn-mike@users.noreply.github.com> --- .../AzCore/AzCore/Asset/AssetCommon.h | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetCommon.h b/Code/Framework/AzCore/AzCore/Asset/AssetCommon.h index 0c8e5209ca..c45bb21c6d 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetCommon.h +++ b/Code/Framework/AzCore/AzCore/Asset/AssetCommon.h @@ -556,16 +556,24 @@ namespace AZ Asset assetData(AssetInternal::GetAssetData(actualId, AZ::Data::AssetLoadBehavior::Default)); if (assetData) { - auto curStatus = assetData->GetStatus(); + auto isReady = assetData->GetStatus() == AssetData::AssetStatus::Ready; bool isError = assetData->IsError(); - connectLock.unlock(); - if (curStatus == AssetData::AssetStatus::Ready) + + if (isReady || isError) { - handler->OnAssetReady(assetData); - } - else if (isError) - { - handler->OnAssetError(assetData); + connectLock.unlock(); + + if (isReady) + { + handler->OnAssetReady(assetData); + } + else if (isError) + { + handler->OnAssetError(assetData); + } + + // Lock the mutex again since some destructors will be modifying the context afterwards + connectLock.lock(); } } }