birkeh/o3de
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

ATOM-15358 : Culling concurrency checker fails in AtomSampleViewer (merge from 1.0->main)

Browse Source 
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.
This commit is contained in:
Tommy Walton
2021-05-18 10:53:41 -07:00
committed by GitHub
parent 343fc1999f
commit d0810892f1
4 changed files with 125 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Code/Framework/AtomCore/AtomCore/std/parallel/concurrency_checker.h
+22 -2
View File
@@ -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
};
Code/Framework/AtomCore/Tests/ArrayView.cpp
+2 -5
View File
@@ -293,15 +293,12 @@ namespace UnitTest
{
array_view<int> 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);
}
}
Code/Framework/AtomCore/Tests/ConcurrencyCheckerTests.cpp
+100
View File
@@ -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 <AtomCore/std/parallel/concurrency_checker.h>
#include <AzCore/UnitTest/TestTypes.h>
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);
}
}
Code/Framework/AtomCore/Tests/atomcore_tests_files.cmake
+1
View File
@@ -11,6 +11,7 @@
set(FILES
ArrayView.cpp
ConcurrencyCheckerTests.cpp
InstanceDatabase.cpp
JsonSerializationUtilsTests.cpp
lru_cache.cpp