From bd23944531c8a58299322b7eb93e72861e958fd1 Mon Sep 17 00:00:00 2001 From: mbalfour Date: Mon, 12 Apr 2021 14:57:23 -0500 Subject: [PATCH 1/8] Fix issues with rapid asset cancellation / reload: * Asset::QueueLoad didn't trigger any loads in the case where an asset was in a Queued state, it simply returned the reference. This caused problems in the case where an asset was in the process of being cancelled and garbage collected, as it could be in a queued state with nothing actively loading it. The method now detects this case and calls GetAsset(), which triggers a new load. * AssetContainer::IsValid() was returning true for canceled containers that no longer had a root asset. Now it returns false, to help ensure the container doesn't try to get reused. * AssetContainer would add entries to the preloadList even if any potential preloads were filtered out from the load. They are no longer added, since they shouldn't be waiting for any dependent assets to load. (This could cause incorrect warnings to print in some situations) * AssetContainer was erroneously warning about removing assets from a missing waiting list. The warning was removed, as the condition could occur when the same asset was being loading by two different containers - once with dependencies and once without. * AssetDataStream::RequestCancel has been added, as it was missing, but nothing currently needs to use it. * AssetManager::GetAssetContainer() now verifies that the container is valid before attempting to reuse it. This prevents asset containers that are in the middle of cancellation from getting reused. --- .../AzCore/AzCore/Asset/AssetCommon.h | 7 +++++-- .../AzCore/AzCore/Asset/AssetContainer.cpp | 20 ++++++++++++++++--- .../AzCore/AzCore/Asset/AssetContainer.h | 1 + .../AzCore/AzCore/Asset/AssetDataStream.cpp | 11 ++++++++++ .../AzCore/AzCore/Asset/AssetDataStream.h | 4 ++++ .../AzCore/AzCore/Asset/AssetManager.cpp | 2 +- 6 files changed, 39 insertions(+), 6 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetCommon.h b/Code/Framework/AzCore/AzCore/Asset/AssetCommon.h index c09d531e0a..c8a245af68 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetCommon.h +++ b/Code/Framework/AzCore/AzCore/Asset/AssetCommon.h @@ -1104,8 +1104,11 @@ namespace AZ // If we either already had valid asset data, or just created it via FindOrCreateAsset, try to queue the load. if (m_assetData && m_assetData->GetId().IsValid()) { - // Only try to queue if the asset isn't already loading or loaded. - if (m_assetData->GetStatus() == AZ::Data::AssetData::AssetStatus::NotLoaded) + // Try to queue if the asset isn't already loading or loaded. + // Also try to queue if the asset *is* already loading or loaded, but we're the only one with a strong reference + // (i.e. use count == 1), because that means it was in the process of being garbage-collected. + if ((m_assetData->GetStatus() == AZ::Data::AssetData::AssetStatus::NotLoaded) || + (m_assetData->GetUseCount() == 1)) { *this = AssetInternal::GetAsset(m_assetData->GetId(), m_assetData->GetType(), loadBehavior, loadParams); } diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp b/Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp index 984aeeb756..bcc9e1d6d1 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp +++ b/Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp @@ -255,7 +255,7 @@ namespace AZ bool AssetContainer::IsValid() const { - return (m_containerAssetId.IsValid() && m_initComplete); + return (m_containerAssetId.IsValid() && m_initComplete && m_rootAsset); } void AssetContainer::CheckReady() @@ -341,9 +341,15 @@ namespace AZ void AssetContainer::OnAssetError(Asset asset) { + AZ_Warning("AssetContainer", false, "Error loading asset %s", asset->GetId().ToString().c_str()); HandleReadyAsset(asset); } + void AssetContainer::OnAssetCanceled(AssetId assetId) + { + AZ_Error("AssetContainer", false, "Asset %s load was incorrectly canceled.", assetId.ToString().c_str()); + } + void AssetContainer::HandleReadyAsset(Asset asset) { RemoveFromAllWaitingPreloads(asset->GetId()); @@ -366,7 +372,10 @@ namespace AZ auto remainingPreloadIter = m_preloadList.find(waiterId); if (remainingPreloadIter == m_preloadList.end()) { - AZ_Warning("AssetContainer", !m_initComplete, "Couldn't find waiting list for %s", waiterId.ToString().c_str()); + // If we got here without an entry on the preload list, it probably means this asset was triggered to load multiple + // times, some with dependencies and some without. To ensure that we don't disturb the loads that expect the + // dependencies, just silently return and don't treat the asset as finished loading. We'll rely on the other load + // to send an OnAssetReady() whenever its expected dependencies are met. return; } if (!remainingPreloadIter->second.erase(preloadID)) @@ -610,7 +619,12 @@ namespace AZ } for(auto& thisList : preloadList) { - m_preloadList[thisList.first].insert(thisList.second.begin(), thisList.second.end()); + // Only save the entry to the final preload list if it has at least one dependent asset still remaining after + // the checks above. + if (!thisList.second.empty()) + { + m_preloadList[thisList.first].insert(thisList.second.begin(), thisList.second.end()); + } } } } diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetContainer.h b/Code/Framework/AzCore/AzCore/Asset/AssetContainer.h index fe385efae9..5a548d5e12 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetContainer.h +++ b/Code/Framework/AzCore/AzCore/Asset/AssetContainer.h @@ -80,6 +80,7 @@ namespace AZ // AssetBus void OnAssetReady(Asset asset) override; void OnAssetError(Asset asset) override; + void OnAssetCanceled(AssetId assetId) override; ////////////////////////////////////////////////////////////////////////// // AssetLoadBus diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetDataStream.cpp b/Code/Framework/AzCore/AzCore/Asset/AssetDataStream.cpp index 5ef278c7ea..a17ff6fd6e 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetDataStream.cpp +++ b/Code/Framework/AzCore/AzCore/Asset/AssetDataStream.cpp @@ -208,6 +208,7 @@ namespace AZ::Data void AssetDataStream::Close() { AZ_Assert(m_isOpen, "Attempting to close a stream that hasn't been opened."); + AZ_Assert(m_curReadRequest == nullptr, "Attempting to close a stream with a read request in flight."); // Destroy the asset buffer and unlock the allocator, so the allocator itself knows that it is no longer needed. if (m_buffer != m_preloadedData.data()) @@ -222,6 +223,16 @@ namespace AZ::Data AZ_PROFILE_INTERVAL_END(AZ::Debug::ProfileCategory::AzCore, this); } + void AssetDataStream::RequestCancel() + { + AZStd::scoped_lock lock(m_readRequestMutex); + if (m_curReadRequest) + { + auto streamer = Interface::Get(); + m_curReadRequest = streamer->Cancel(m_curReadRequest); + } + } + void AssetDataStream::Seek(AZ::IO::OffsetType bytes, AZ::IO::GenericStream::SeekMode mode) { AZ_PROFILE_FUNCTION(AZ::Debug::ProfileCategory::AzCore); diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetDataStream.h b/Code/Framework/AzCore/AzCore/Asset/AssetDataStream.h index 58144c2f92..1367c493f5 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetDataStream.h +++ b/Code/Framework/AzCore/AzCore/Asset/AssetDataStream.h @@ -82,6 +82,10 @@ namespace AZ::Data //! Gets the size of data loaded (so far). size_t GetLoadedSize() const { return m_loadedSize; } + //! Request a cancellation of any current IO streamer requests. + //! Note: This is asynchronous and not guaranteed to cancel if the request is already in-process. + void RequestCancel(); + private: //! Perform any operations needed by all variants of Open() void OpenInternal(size_t assetSize, const char* streamName); diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp b/Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp index 214443142b..db8736b4a8 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp +++ b/Code/Framework/AzCore/AzCore/Asset/AssetManager.cpp @@ -2144,7 +2144,7 @@ namespace AZ if (curIter != m_assetContainers.end()) { auto newRef = curIter->second.lock(); - if (newRef) + if (newRef && newRef->IsValid()) { return newRef; } From 3907ffc173d3de79490798dcf8faabc213cfe87a Mon Sep 17 00:00:00 2001 From: amzn-mike Date: Fri, 16 Apr 2021 13:50:39 -0500 Subject: [PATCH 2/8] Add unit test --- .../Tests/Asset/AssetManagerLoadingTests.cpp | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp b/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp index 18e193304a..15f2b0bf86 100644 --- a/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp +++ b/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp @@ -575,6 +575,91 @@ namespace UnitTest EXPECT_EQ(baseStatus, expected_base_status); } + struct DebugListener : AZ::Interface::Registrar + { + void AssetStatusUpdate(AZ::Data::AssetId id, AZ::Data::AssetData::AssetStatus status) override + { + AZ::Debug::Trace::Output( + "", AZStd::string::format("Status %s - %d\n", id.ToString().c_str(), static_cast(status)).c_str()); + } + void ReleaseAsset(AZ::Data::AssetId id) override + { + AZ::Debug::Trace::Output( + "", AZStd::string::format("Release %s\n", id.ToString().c_str()).c_str()); + } + }; + + TEST_F(AssetJobsFloodTest, Cancel) + { + DebugListener listener; + auto assetUuids = { + MyAsset1Id, + //MyAsset2Id, + //MyAsset3Id, + }; + + AZStd::vector threads; + AZStd::mutex mutex; + AZStd::atomic threadCount((int)assetUuids.size()); + AZStd::condition_variable cv; + AZStd::atomic_bool keepDispatching(true); + + auto dispatch = [&keepDispatching]() { + while (keepDispatching) + { + AssetManager::Instance().DispatchEvents(); + } + }; + + AZStd::thread dispatchThread(dispatch); + + for (const auto& assetUuid : assetUuids) + { + threads.emplace_back([this, &threadCount, &cv, assetUuid]() { + bool checkLoaded = true; + + for (int i = 0; i < 1000; i++) + { + Asset asset1 = + m_testAssetManager->GetAsset(assetUuid, azrtti_typeid(), AZ::Data::AssetLoadBehavior::PreLoad); + + if (checkLoaded) + { + asset1.BlockUntilLoadComplete(); + + EXPECT_TRUE(asset1.IsReady()) << "Iteration " << i << " failed. Asset status: " << static_cast(asset1.GetStatus()); + } + + checkLoaded = !checkLoaded; + } + + threadCount--; + cv.notify_one(); + }); + } + + bool timedOut = false; + + // Used to detect a deadlock. If we wait for more than 5 seconds, it's likely a deadlock has occurred + 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)); + } + + ASSERT_EQ(threadCount, 0) << "Thread count is non-zero, a thread has likely deadlocked. Test will not shut down cleanly."; + + for (auto& thread : threads) + { + thread.join(); + } + + keepDispatching = false; + dispatchThread.join(); + + AssetManager::Destroy(); + } + #if AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS TEST_F(AssetJobsFloodTest, DISABLED_AssetLoadBehaviorIsPreserved) #else From 35a47932eb96c5b7b28e481af7e521fea2376f20 Mon Sep 17 00:00:00 2001 From: amzn-mike <80125227+amzn-mike@users.noreply.github.com> Date: Thu, 22 Apr 2021 15:02:41 -0500 Subject: [PATCH 3/8] Disable weak references and unit tests for them. --- .../AzCore/AzCore/Asset/AssetInternal/WeakAsset.h | 8 ++++++-- Code/Framework/AzCore/Tests/Asset/AssetCommon.cpp | 12 ++++++++---- .../Tests/Asset/AssetManagerLoadingTests.cpp | 15 ++++++++++----- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetInternal/WeakAsset.h b/Code/Framework/AzCore/AzCore/Asset/AssetInternal/WeakAsset.h index 1384511e78..3d4fa6c50e 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetInternal/WeakAsset.h +++ b/Code/Framework/AzCore/AzCore/Asset/AssetInternal/WeakAsset.h @@ -141,13 +141,17 @@ namespace AZ::Data::AssetInternal if (assetData) { - assetData->AcquireWeak(); + // This should be AcquireWeak but we're using strong references for now to disable asset cancellation + // until it is more stable + assetData->Acquire(); m_assetId = assetData->GetId(); } if (m_assetData) { - m_assetData->ReleaseWeak(); + // This should be ReleaseWeak but we're using strong references for now to disable asset cancellation + // until it is more stable + m_assetData->Release(); } m_assetData = assetData; diff --git a/Code/Framework/AzCore/Tests/Asset/AssetCommon.cpp b/Code/Framework/AzCore/Tests/Asset/AssetCommon.cpp index d0e34487ea..c829821ca6 100644 --- a/Code/Framework/AzCore/Tests/Asset/AssetCommon.cpp +++ b/Code/Framework/AzCore/Tests/Asset/AssetCommon.cpp @@ -186,7 +186,8 @@ namespace UnitTest int GetWeakUseCount() { return m_weakUseCount.load(); } }; - TEST_F(WeakAssetTest, WeakAsset_ConstructionAndDestruction_UpdatesAssetDataWeakRefCount) + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + TEST_F(WeakAssetTest, DISABLED_WeakAsset_ConstructionAndDestruction_UpdatesAssetDataWeakRefCount) { TestAssetData testData; EXPECT_EQ(testData.GetWeakUseCount(), 0); @@ -202,7 +203,8 @@ namespace UnitTest EXPECT_EQ(testData.GetWeakUseCount(), 0); } - TEST_F(WeakAssetTest, WeakAsset_MoveOperatorWithDifferentData_UpdatesOldAssetDataWeakRefCount) + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + TEST_F(WeakAssetTest, DISABLED_WeakAsset_MoveOperatorWithDifferentData_UpdatesOldAssetDataWeakRefCount) { TestAssetData testData; EXPECT_EQ(testData.GetWeakUseCount(), 0); @@ -217,7 +219,8 @@ namespace UnitTest AZ_TEST_STOP_TRACE_SUPPRESSION(1); } - TEST_F(WeakAssetTest, WeakAsset_MoveOperatorWithSameData_PreservesAssetDataWeakRefCount) + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + TEST_F(WeakAssetTest, DISABLED_WeakAsset_MoveOperatorWithSameData_PreservesAssetDataWeakRefCount) { TestAssetData testData; EXPECT_EQ(testData.GetWeakUseCount(), 0); @@ -234,7 +237,8 @@ namespace UnitTest AZ_TEST_STOP_TRACE_SUPPRESSION(1); } - TEST_F(WeakAssetTest, WeakAsset_AssignmentOperator_CopiesDataAndIncrementsWeakRefCount) + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + TEST_F(WeakAssetTest, DISABLED_WeakAsset_AssignmentOperator_CopiesDataAndIncrementsWeakRefCount) { TestAssetData testData; EXPECT_EQ(testData.GetWeakUseCount(), 0); diff --git a/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp b/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp index 15f2b0bf86..34b6918400 100644 --- a/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp +++ b/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp @@ -2677,7 +2677,8 @@ namespace UnitTest #if AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS TEST_F(AssetManagerCancelTests, DISABLED_CancelLoad_NoReferences_LoadCancels) #else - TEST_F(AssetManagerCancelTests, CancelLoad_NoReferences_LoadCancels) + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + TEST_F(AssetManagerCancelTests, DISABLED_CancelLoad_NoReferences_LoadCancels) #endif // AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS { m_assetHandlerAndCatalog->SetArtificialDelayMilliseconds(0, 100); @@ -2717,7 +2718,8 @@ namespace UnitTest #if AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS TEST_F(AssetManagerCancelTests, DISABLED_CanceledLoad_CanBeLoadedAgainLater) #else - TEST_F(AssetManagerCancelTests, CanceledLoad_CanBeLoadedAgainLater) + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + TEST_F(AssetManagerCancelTests, DISABLED_CanceledLoad_CanBeLoadedAgainLater) #endif // AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS { m_assetHandlerAndCatalog->SetArtificialDelayMilliseconds(0, 50); @@ -2766,7 +2768,8 @@ namespace UnitTest #if AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS TEST_F(AssetManagerCancelTests, DISABLED_CancelLoad_InProgressLoad_Continues) #else - TEST_F(AssetManagerCancelTests, CancelLoad_InProgressLoad_Continues) + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + TEST_F(AssetManagerCancelTests, DISABLED_CancelLoad_InProgressLoad_Continues) #endif // AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS { m_assetHandlerAndCatalog->SetArtificialDelayMilliseconds(0, 100); @@ -2988,8 +2991,9 @@ namespace UnitTest TEST_F(AssetManagerClearAssetReferenceTests, DISABLED_ContainerLoadTest_AssetLosesAndGainsReferencesDuringLoadAndSuspendedRelease_AssetSuccessfullyFinishesLoading) #else + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable TEST_F(AssetManagerClearAssetReferenceTests, - ContainerLoadTest_AssetLosesAndGainsReferencesDuringLoadAndSuspendedRelease_AssetSuccessfullyFinishesLoading) + DISABLED_ContainerLoadTest_AssetLosesAndGainsReferencesDuringLoadAndSuspendedRelease_AssetSuccessfullyFinishesLoading) #endif // AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS { // Start the load and wait for the dependent asset to hit the loading state. @@ -3046,7 +3050,8 @@ namespace UnitTest #if AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS TEST_F(AssetManagerClearAssetReferenceTests, DISABLED_ContainerLoadTest_RootAssetDestroyedWhileContainerLoading_ContainerFinishesLoad) #else - TEST_F(AssetManagerClearAssetReferenceTests, ContainerLoadTest_RootAssetDestroyedWhileContainerLoading_ContainerFinishesLoad) + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + TEST_F(AssetManagerClearAssetReferenceTests, DISABLED_ContainerLoadTest_RootAssetDestroyedWhileContainerLoading_ContainerFinishesLoad) #endif // AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS { OnAssetReadyListener assetStatus1(DependentPreloadAssetId, azrtti_typeid()); From 39789c30d8bb57628b4212aabca9d89317cfa5f1 Mon Sep 17 00:00:00 2001 From: amzn-mike <80125227+amzn-mike@users.noreply.github.com> Date: Thu, 22 Apr 2021 15:03:04 -0500 Subject: [PATCH 4/8] Remove container OnAssetCanceled event --- Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp | 5 ----- Code/Framework/AzCore/AzCore/Asset/AssetContainer.h | 1 - 2 files changed, 6 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp b/Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp index bcc9e1d6d1..4e7a19c197 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp +++ b/Code/Framework/AzCore/AzCore/Asset/AssetContainer.cpp @@ -345,11 +345,6 @@ namespace AZ HandleReadyAsset(asset); } - void AssetContainer::OnAssetCanceled(AssetId assetId) - { - AZ_Error("AssetContainer", false, "Asset %s load was incorrectly canceled.", assetId.ToString().c_str()); - } - void AssetContainer::HandleReadyAsset(Asset asset) { RemoveFromAllWaitingPreloads(asset->GetId()); diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetContainer.h b/Code/Framework/AzCore/AzCore/Asset/AssetContainer.h index 5a548d5e12..fe385efae9 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetContainer.h +++ b/Code/Framework/AzCore/AzCore/Asset/AssetContainer.h @@ -80,7 +80,6 @@ namespace AZ // AssetBus void OnAssetReady(Asset asset) override; void OnAssetError(Asset asset) override; - void OnAssetCanceled(AssetId assetId) override; ////////////////////////////////////////////////////////////////////////// // AssetLoadBus From 0428189bedcc189c99df668617e23d133227bbb6 Mon Sep 17 00:00:00 2001 From: amzn-mike <80125227+amzn-mike@users.noreply.github.com> Date: Wed, 21 Apr 2021 10:11:19 -0500 Subject: [PATCH 5/8] Increase test difficulty --- .../AzCore/Tests/Asset/AssetManagerLoadingTests.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp b/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp index 34b6918400..9208294a32 100644 --- a/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp +++ b/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp @@ -594,13 +594,13 @@ namespace UnitTest DebugListener listener; auto assetUuids = { MyAsset1Id, - //MyAsset2Id, - //MyAsset3Id, + MyAsset2Id, + MyAsset3Id, }; AZStd::vector threads; AZStd::mutex mutex; - AZStd::atomic threadCount((int)assetUuids.size()); + AZStd::atomic threadCount(static_cast(assetUuids.size())); AZStd::condition_variable cv; AZStd::atomic_bool keepDispatching(true); @@ -618,10 +618,13 @@ namespace UnitTest threads.emplace_back([this, &threadCount, &cv, assetUuid]() { bool checkLoaded = true; - for (int i = 0; i < 1000; i++) + for (int i = 0; i < 5000; i++) { Asset asset1 = m_testAssetManager->GetAsset(assetUuid, azrtti_typeid(), AZ::Data::AssetLoadBehavior::PreLoad); + AZ::Debug::Trace::Output("", AZStd::string::format("Got ref from GetAsset: %s. Will block: %s\n", + asset1.GetId().ToString().c_str(), + checkLoaded ? "Yes" : "No").c_str()); if (checkLoaded) { @@ -633,7 +636,7 @@ namespace UnitTest checkLoaded = !checkLoaded; } - threadCount--; + --threadCount; cv.notify_one(); }); } From 58e8233c2c8cac83b752c242ae7aa6edbcc918b6 Mon Sep 17 00:00:00 2001 From: amzn-mike <80125227+amzn-mike@users.noreply.github.com> Date: Thu, 22 Apr 2021 15:27:16 -0500 Subject: [PATCH 6/8] Remove debug message --- .../AzCore/Tests/Asset/AssetManagerLoadingTests.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp b/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp index 9208294a32..d2ffa34e5e 100644 --- a/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp +++ b/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp @@ -622,10 +622,7 @@ namespace UnitTest { Asset asset1 = m_testAssetManager->GetAsset(assetUuid, azrtti_typeid(), AZ::Data::AssetLoadBehavior::PreLoad); - AZ::Debug::Trace::Output("", AZStd::string::format("Got ref from GetAsset: %s. Will block: %s\n", - asset1.GetId().ToString().c_str(), - checkLoaded ? "Yes" : "No").c_str()); - + if (checkLoaded) { asset1.BlockUntilLoadComplete(); From c61b24441683c21893f0b96fdca2464d203ec912 Mon Sep 17 00:00:00 2001 From: amzn-mike <80125227+amzn-mike@users.noreply.github.com> Date: Fri, 23 Apr 2021 09:56:59 -0500 Subject: [PATCH 7/8] Use a const to toggle asset cancellation on and off --- .../AzCore/Asset/AssetInternal/WeakAsset.h | 33 +++++++++++++++---- .../Tests/Asset/AssetManagerLoadingTests.cpp | 2 +- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Asset/AssetInternal/WeakAsset.h b/Code/Framework/AzCore/AzCore/Asset/AssetInternal/WeakAsset.h index 3d4fa6c50e..b772d5a8cb 100644 --- a/Code/Framework/AzCore/AzCore/Asset/AssetInternal/WeakAsset.h +++ b/Code/Framework/AzCore/AzCore/Asset/AssetInternal/WeakAsset.h @@ -28,6 +28,8 @@ namespace AZ::Data::AssetInternal class WeakAsset { public: + static constexpr bool EnableAssetCancellation = false; + WeakAsset() = default; WeakAsset(AssetData* assetData, AssetLoadBehavior assetReferenceLoadBehavior); @@ -111,7 +113,14 @@ namespace AZ::Data::AssetInternal // - If the left and right sides are the same, clearing the right side's reference means one less reference will exist if (m_assetData) { - m_assetData->ReleaseWeak(); + if constexpr (EnableAssetCancellation) + { + m_assetData->ReleaseWeak(); + } + else + { + m_assetData->Release(); + } } m_assetData = AZStd::move(rhs.m_assetData); rhs.m_assetData = nullptr; @@ -141,17 +150,27 @@ namespace AZ::Data::AssetInternal if (assetData) { - // This should be AcquireWeak but we're using strong references for now to disable asset cancellation - // until it is more stable - assetData->Acquire(); + if constexpr (EnableAssetCancellation) + { + assetData->AcquireWeak(); + } + else + { + assetData->Acquire(); + } m_assetId = assetData->GetId(); } if (m_assetData) { - // This should be ReleaseWeak but we're using strong references for now to disable asset cancellation - // until it is more stable - m_assetData->Release(); + if constexpr (EnableAssetCancellation) + { + m_assetData->ReleaseWeak(); + } + else + { + m_assetData->Release(); + } } m_assetData = assetData; diff --git a/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp b/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp index d2ffa34e5e..b9baf0db80 100644 --- a/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp +++ b/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp @@ -589,7 +589,7 @@ namespace UnitTest } }; - TEST_F(AssetJobsFloodTest, Cancel) + TEST_F(AssetJobsFloodTest, RapidAcquireAndRelease) { DebugListener listener; auto assetUuids = { From d25179303ddf0a844a7b648d64266d781e94dba2 Mon Sep 17 00:00:00 2001 From: amzn-mike <80125227+amzn-mike@users.noreply.github.com> Date: Fri, 23 Apr 2021 10:50:58 -0500 Subject: [PATCH 8/8] Add jira ticket for disabled tests --- .../AzCore/Tests/Asset/AssetManagerLoadingTests.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp b/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp index b9baf0db80..78e2288a4c 100644 --- a/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp +++ b/Code/Framework/AzCore/Tests/Asset/AssetManagerLoadingTests.cpp @@ -2677,7 +2677,7 @@ namespace UnitTest #if AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS TEST_F(AssetManagerCancelTests, DISABLED_CancelLoad_NoReferences_LoadCancels) #else - // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable. LYN-3263 TEST_F(AssetManagerCancelTests, DISABLED_CancelLoad_NoReferences_LoadCancels) #endif // AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS { @@ -2718,7 +2718,7 @@ namespace UnitTest #if AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS TEST_F(AssetManagerCancelTests, DISABLED_CanceledLoad_CanBeLoadedAgainLater) #else - // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable. LYN-3263 TEST_F(AssetManagerCancelTests, DISABLED_CanceledLoad_CanBeLoadedAgainLater) #endif // AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS { @@ -2768,7 +2768,7 @@ namespace UnitTest #if AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS TEST_F(AssetManagerCancelTests, DISABLED_CancelLoad_InProgressLoad_Continues) #else - // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable. LYN-3263 TEST_F(AssetManagerCancelTests, DISABLED_CancelLoad_InProgressLoad_Continues) #endif // AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS { @@ -2991,7 +2991,7 @@ namespace UnitTest TEST_F(AssetManagerClearAssetReferenceTests, DISABLED_ContainerLoadTest_AssetLosesAndGainsReferencesDuringLoadAndSuspendedRelease_AssetSuccessfullyFinishesLoading) #else - // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable. LYN-3263 TEST_F(AssetManagerClearAssetReferenceTests, DISABLED_ContainerLoadTest_AssetLosesAndGainsReferencesDuringLoadAndSuspendedRelease_AssetSuccessfullyFinishesLoading) #endif // AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS @@ -3050,7 +3050,7 @@ namespace UnitTest #if AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS TEST_F(AssetManagerClearAssetReferenceTests, DISABLED_ContainerLoadTest_RootAssetDestroyedWhileContainerLoading_ContainerFinishesLoad) #else - // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable + // Asset cancellation is temporarily disabled, re-enable this test when cancellation is more stable. LYN-3263 TEST_F(AssetManagerClearAssetReferenceTests, DISABLED_ContainerLoadTest_RootAssetDestroyedWhileContainerLoading_ContainerFinishesLoad) #endif // AZ_TRAIT_DISABLE_FAILED_ASSET_MANAGER_TESTS {