From d0810892f19998c4ef82219b548075ed37297868 Mon Sep 17 00:00:00 2001 From: Tommy Walton <82672795+amzn-tommy@users.noreply.github.com> Date: Tue, 18 May 2021 10:53:41 -0700 Subject: [PATCH] ATOM-15358 : Culling concurrency checker fails in AtomSampleViewer (merge from 1.0->main) Inserting, updating, and removing entries from a VisibilityScene was made thread safe in a previous change, and now both the MeshFeatureProcessor and DiffuseProbeGridFeatureProcessor update entries at the same time from multiple threads. This leads to an assert in the Culling concurrency_checker, even though this is now valid behavior. However, we still don't want to be adding, removing, or updating cullables between BeginCulling and EndCulling, which could cause a mismatch between the result of OctreeScene::GetEntryCount and the actual number of cullables in the scene. -Added soft_lock_shared/soft_unlock_shared to the concurrency checker to allow multiple threads to acquire a lock when that is the desired behavior, while still asserting that nothing tries to acquire a shared lock when the concurrency checker is already locked. -Update Culling.cpp to use the new soft_lock_shared when adding, updating, or removing cullables -Added unit tests for the concurrency_checker -Updated ArrayView unit test to use AZ_TEST_START_TRACE_SUPPRESSION instead of manually checking the assertion count, so that test now passes in release builds which do not assert. --- .../std/parallel/concurrency_checker.h | 24 ++++- Code/Framework/AtomCore/Tests/ArrayView.cpp | 7 +- .../Tests/ConcurrencyCheckerTests.cpp | 100 ++++++++++++++++++ .../AtomCore/Tests/atomcore_tests_files.cmake | 1 + 4 files changed, 125 insertions(+), 7 deletions(-) create mode 100644 Code/Framework/AtomCore/Tests/ConcurrencyCheckerTests.cpp diff --git a/Code/Framework/AtomCore/AtomCore/std/parallel/concurrency_checker.h b/Code/Framework/AtomCore/AtomCore/std/parallel/concurrency_checker.h index 311b574fdf..4a952721b6 100644 --- a/Code/Framework/AtomCore/AtomCore/std/parallel/concurrency_checker.h +++ b/Code/Framework/AtomCore/AtomCore/std/parallel/concurrency_checker.h @@ -20,10 +20,12 @@ namespace AZStd { - //! Simple class for verifying that no concurrent access is occuring. + //! Simple class for verifying that no concurrent access is occurring. //! This is *not* a synchronization primitive, and is intended simply for checking that no concurrency issues exist. //! It will be compiled out in release builds. //! Use concurrency_checker like a mutex (i.e. call soft_lock() and soft_unlock() around all instances of your data access). + //! Use soft_lock_shared and soft_unlock_shared around places where multiple threads are allowed to have read access + //! at the same time as long as nothing else already has a soft lock //! It will assert if there are multiple threads accessing the locked code/data at the same time. //! Expected use case is for defensive programming: when you do not expect any concurrent access within a system, //! but want to verify that it stays that way in the future, without incurring the overhead of a mutex. @@ -34,7 +36,7 @@ namespace AZStd { #ifdef AZ_CONCURRENCY_CHECKER_ENABLED uint32_t count = ++m_concurrencyCounter; - AZ_Assert(count == 1, "Concurrency check failed. Multiple threads are trying to access data at the same time, or there is a lock/unlock mismatch."); + AZ_Assert(count == 1 && m_sharedConcurrencyCounter == 0, "Concurrency check failed. Multiple threads are trying to access data at the same time, or there is a lock/unlock mismatch."); #endif } @@ -46,9 +48,27 @@ namespace AZStd #endif } + AZ_FORCE_INLINE void soft_lock_shared() + { +#ifdef AZ_CONCURRENCY_CHECKER_ENABLED + AZ_Assert(m_concurrencyCounter == 0, "Concurrency check failed. A soft_lock_shared was attempted when there was already a soft_lock."); + ++m_sharedConcurrencyCounter; +#endif + } + + AZ_FORCE_INLINE void soft_unlock_shared() + { +#ifdef AZ_CONCURRENCY_CHECKER_ENABLED + AZ_Assert(m_sharedConcurrencyCounter != 0, "Concurrency check failed. There is a shared_lock/shared_unlock mismatch."); + --m_sharedConcurrencyCounter; +#endif + } + + private: #ifdef AZ_CONCURRENCY_CHECKER_ENABLED AZStd::atomic_uint32_t m_concurrencyCounter = 0; + AZStd::atomic_uint32_t m_sharedConcurrencyCounter = 0; #endif }; diff --git a/Code/Framework/AtomCore/Tests/ArrayView.cpp b/Code/Framework/AtomCore/Tests/ArrayView.cpp index 83fa20aaf0..56627d280e 100644 --- a/Code/Framework/AtomCore/Tests/ArrayView.cpp +++ b/Code/Framework/AtomCore/Tests/ArrayView.cpp @@ -293,15 +293,12 @@ namespace UnitTest { array_view view({ 1,2,3,4 }); - UnitTest::TestRunner::Instance().StartAssertTests(); + AZ_TEST_START_TRACE_SUPPRESSION; - EXPECT_EQ(0, UnitTest::TestRunner::Instance().m_numAssertsFailed); view[4]; - EXPECT_EQ(1, UnitTest::TestRunner::Instance().m_numAssertsFailed); view[5]; - EXPECT_EQ(2, UnitTest::TestRunner::Instance().m_numAssertsFailed); - UnitTest::TestRunner::Instance().StopAssertTests(); + AZ_TEST_STOP_TRACE_SUPPRESSION(2); } } diff --git a/Code/Framework/AtomCore/Tests/ConcurrencyCheckerTests.cpp b/Code/Framework/AtomCore/Tests/ConcurrencyCheckerTests.cpp new file mode 100644 index 0000000000..7cd1fa27ac --- /dev/null +++ b/Code/Framework/AtomCore/Tests/ConcurrencyCheckerTests.cpp @@ -0,0 +1,100 @@ +/* +* All or portions of this file Copyright (c) Amazon.com, Inc. or its affiliates or +* its licensors. +* +* For complete copyright and license terms please see the LICENSE at the root of this +* distribution (the "License"). All use of this software is governed by the License, +* or, if provided, by the license below or the license accompanying this file. Do not +* remove or modify any license notices. This file is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* +*/ + +#include + +#include + +using namespace AZStd; + +namespace UnitTest +{ + class ConcurrencyCheckerTestFixture + : public AllocatorsTestFixture + { + + void SetUp() override + { + AllocatorsFixture::SetUp(); + } + }; + + TEST_F(AllocatorsTestFixture, SoftLock_NoContention_NoAsserts) + { + concurrency_checker concurrencyChecker; + concurrencyChecker.soft_lock(); + concurrencyChecker.soft_unlock(); + concurrencyChecker.soft_lock(); + concurrencyChecker.soft_unlock(); + } + + TEST_F(AllocatorsTestFixture, SoftLock_AlreadyLocked_Assert) + { + concurrency_checker concurrencyChecker; + concurrencyChecker.soft_lock(); + AZ_TEST_START_TRACE_SUPPRESSION; + concurrencyChecker.soft_lock(); + AZ_TEST_STOP_TRACE_SUPPRESSION(1); + } + + TEST_F(AllocatorsTestFixture, SoftUnlock_NotAlreadyLocked_Assert) + { + concurrency_checker concurrencyChecker; + concurrencyChecker.soft_lock(); + concurrencyChecker.soft_unlock(); + AZ_TEST_START_TRACE_SUPPRESSION; + concurrencyChecker.soft_unlock(); + AZ_TEST_STOP_TRACE_SUPPRESSION(1); + } + + TEST_F(AllocatorsTestFixture, SoftLockShared_NoContention_NoAsserts) + { + concurrency_checker concurrencyChecker; + // Multiple shared locks can be made at once, + // as long as they are all unlocked before the next soft_lock + concurrencyChecker.soft_lock_shared(); + concurrencyChecker.soft_lock_shared(); + concurrencyChecker.soft_unlock_shared(); + concurrencyChecker.soft_unlock_shared(); + + concurrencyChecker.soft_lock(); + concurrencyChecker.soft_unlock(); + + concurrencyChecker.soft_lock_shared(); + concurrencyChecker.soft_lock_shared(); + concurrencyChecker.soft_unlock_shared(); + concurrencyChecker.soft_unlock_shared(); + + concurrencyChecker.soft_lock(); + concurrencyChecker.soft_unlock(); + } + + TEST_F(AllocatorsTestFixture, SoftLockShared_SharedLockAfterSoftLock_Assert) + { + concurrency_checker concurrencyChecker; + + concurrencyChecker.soft_lock(); + AZ_TEST_START_TRACE_SUPPRESSION; + concurrencyChecker.soft_lock_shared(); + AZ_TEST_STOP_TRACE_SUPPRESSION(1); + } + + TEST_F(AllocatorsTestFixture, SoftUnlockShared_NotAlreadyLocked_Assert) + { + concurrency_checker concurrencyChecker; + concurrencyChecker.soft_lock_shared(); + concurrencyChecker.soft_unlock_shared(); + AZ_TEST_START_TRACE_SUPPRESSION; + concurrencyChecker.soft_unlock_shared(); + AZ_TEST_STOP_TRACE_SUPPRESSION(1); + } +} diff --git a/Code/Framework/AtomCore/Tests/atomcore_tests_files.cmake b/Code/Framework/AtomCore/Tests/atomcore_tests_files.cmake index 26efd09fe1..24d23cb852 100644 --- a/Code/Framework/AtomCore/Tests/atomcore_tests_files.cmake +++ b/Code/Framework/AtomCore/Tests/atomcore_tests_files.cmake @@ -11,6 +11,7 @@ set(FILES ArrayView.cpp + ConcurrencyCheckerTests.cpp InstanceDatabase.cpp JsonSerializationUtilsTests.cpp lru_cache.cpp