From cf7f015ae8d2c1c3caf883666c4175bf123c7f1b Mon Sep 17 00:00:00 2001 From: Jorrit Rouwe Date: Thu, 26 Sep 2024 21:05:02 +0200 Subject: [PATCH 1/9] Add performance test to TSAN build --- .github/workflows/build.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 1ab068566..b1aab74d3 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -54,6 +54,12 @@ jobs: - name: Unit Tests working-directory: ${{github.workspace}}/Build/Linux_ReleaseTSAN run: ctest --output-on-failure --verbose + - name: Test ConvexVsMesh + working-directory: ${{github.workspace}}/Build/Linux_ReleaseTSAN + run: ./PerformanceTest -q=LinearCast -t=max -s=ConvexVsMesh + - name: Test Ragdoll + working-directory: ${{github.workspace}}/Build/Linux_ReleaseTSAN + run: ./PerformanceTest -q=LinearCast -t=max -s=Ragdoll linux-clang-so: runs-on: ubuntu-latest From 0f377849a00133b15fdc02bba94262c3593e5ac8 Mon Sep 17 00:00:00 2001 From: Jorrit Rouwe Date: Sat, 28 Sep 2024 15:21:36 +0200 Subject: [PATCH 2/9] TSAN should also not use custom allocators --- Jolt/Jolt.cmake | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Jolt/Jolt.cmake b/Jolt/Jolt.cmake index 06b739285..c1277feca 100644 --- a/Jolt/Jolt.cmake +++ b/Jolt/Jolt.cmake @@ -521,8 +521,8 @@ endif() target_compile_definitions(Jolt PUBLIC "$<$:_DEBUG>") target_compile_definitions(Jolt PUBLIC "$<$:NDEBUG>") -# ASAN should use the default allocators -target_compile_definitions(Jolt PUBLIC "$<$:JPH_DISABLE_TEMP_ALLOCATOR;JPH_DISABLE_CUSTOM_ALLOCATOR>") +# ASAN and TSAN should use the default allocators +target_compile_definitions(Jolt PUBLIC "$<$:JPH_DISABLE_TEMP_ALLOCATOR;JPH_DISABLE_CUSTOM_ALLOCATOR>") # Setting floating point exceptions if (FLOATING_POINT_EXCEPTIONS_ENABLED AND "${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC") From ece5d24c205b87503065939a5ce6a662eb3f6ffe Mon Sep 17 00:00:00 2001 From: Jorrit Rouwe Date: Sat, 28 Sep 2024 15:22:17 +0200 Subject: [PATCH 3/9] Fixed race condition in island splitter --- Jolt/Physics/LargeIslandSplitter.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Jolt/Physics/LargeIslandSplitter.cpp b/Jolt/Physics/LargeIslandSplitter.cpp index 6ee1a5a11..50ab1c583 100644 --- a/Jolt/Physics/LargeIslandSplitter.cpp +++ b/Jolt/Physics/LargeIslandSplitter.cpp @@ -19,17 +19,20 @@ JPH_NAMESPACE_BEGIN LargeIslandSplitter::EStatus LargeIslandSplitter::Splits::FetchNextBatch(uint32 &outConstraintsBegin, uint32 &outConstraintsEnd, uint32 &outContactsBegin, uint32 &outContactsEnd, bool &outFirstIteration) { { - // First check if we can get a new batch (doing a relaxed read to avoid hammering an atomic with an atomic subtract) + // First check if we can get a new batch (doing a read to avoid hammering an atomic with an atomic subtract) // Note this also avoids overflowing the status counter if we're done but there's still one thread processing items - uint64 status = mStatus.load(memory_order_relaxed); - if (sGetIteration(status) >= mNumIterations) - return EStatus::AllBatchesDone; + uint64 status = mStatus.load(memory_order_acquire); // Check for special value that indicates that the splits are still being built // (note we do not check for this condition again below as we reset all splits before kicking off jobs that fetch batches of work) if (status == StatusItemMask) return EStatus::WaitingForBatch; + // Next check if all items have been processed. Note that we do this after checking if the job can be started + // as mNumIterations is not initialized until the split is started. + if (sGetIteration(status) >= mNumIterations) + return EStatus::AllBatchesDone; + uint item = sGetItem(status); uint split_index = sGetSplit(status); if (split_index == cNonParallelSplitIdx) From d024db7ec29a60c85bf5a7242c94238f405b1ee4 Mon Sep 17 00:00:00 2001 From: Jorrit Rouwe Date: Sat, 28 Sep 2024 15:30:27 +0200 Subject: [PATCH 4/9] Disable TSAN for Profiler::NextFrame --- Jolt/Core/Core.h | 7 +++++++ Jolt/Core/Profiler.cpp | 1 + 2 files changed, 8 insertions(+) diff --git a/Jolt/Core/Core.h b/Jolt/Core/Core.h index 52befc2b7..8396ff3e7 100644 --- a/Jolt/Core/Core.h +++ b/Jolt/Core/Core.h @@ -589,4 +589,11 @@ static_assert(sizeof(void *) == (JPH_CPU_ADDRESS_BITS == 64? 8 : 4), "Invalid si #endif #endif +// Attribute to disable Thread Sanitizer for a particular function +#ifdef JPH_TSAN_ENABLED + #define JPH_TSAN_NO_SANITIZE __attribute__((no_sanitize("thread"))) +#else + #define JPH_TSAN_NO_SANITIZE +#endif + JPH_NAMESPACE_END diff --git a/Jolt/Core/Profiler.cpp b/Jolt/Core/Profiler.cpp index 4b2b908c9..3d7f1bfac 100644 --- a/Jolt/Core/Profiler.cpp +++ b/Jolt/Core/Profiler.cpp @@ -60,6 +60,7 @@ uint64 Profiler::GetProcessorTicksPerSecond() const return (ticks - mReferenceTick) * 1000000000ULL / std::chrono::duration_cast(time - mReferenceTime).count(); } +JPH_TSAN_NO_SANITIZE // This function assumes that none of the threads are active while we're dumping the profile, otherwise there will be a race condition on mCurrentSample and the profile data void Profiler::NextFrame() { std::lock_guard lock(mLock); From 86f0b41f9a35cb560ae8a81076159cc6a50d6092 Mon Sep 17 00:00:00 2001 From: Jorrit Rouwe Date: Sun, 29 Sep 2024 10:33:39 +0200 Subject: [PATCH 5/9] Specifying memory ordering explicitly for mNumActiveBodies --- Jolt/Physics/Body/BodyManager.cpp | 8 ++++---- Jolt/Physics/Body/BodyManager.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Jolt/Physics/Body/BodyManager.cpp b/Jolt/Physics/Body/BodyManager.cpp index 440f95147..db11bae89 100644 --- a/Jolt/Physics/Body/BodyManager.cpp +++ b/Jolt/Physics/Body/BodyManager.cpp @@ -501,7 +501,7 @@ void BodyManager::AddBodyToActiveBodies(Body &ioBody) mp->mIndexInActiveBodies = num_active_bodies; JPH_ASSERT(num_active_bodies < GetMaxBodies()); active_bodies[num_active_bodies] = ioBody.GetID(); - num_active_bodies++; // Increment atomic after setting the body ID so that PhysicsSystem::JobFindCollisions (which doesn't lock the mActiveBodiesMutex) will only read valid IDs + num_active_bodies.fetch_add(1, memory_order_release); // Increment atomic after setting the body ID so that PhysicsSystem::JobFindCollisions (which doesn't lock the mActiveBodiesMutex) will only read valid IDs // Count CCD bodies if (mp->GetMotionQuality() == EMotionQuality::LinearCast) @@ -533,7 +533,7 @@ void BodyManager::RemoveBodyFromActiveBodies(Body &ioBody) mp->mIndexInActiveBodies = Body::cInactiveIndex; // Remove unused element from active bodies list - --num_active_bodies; + num_active_bodies.fetch_sub(1, memory_order_release); // Count CCD bodies if (mp->GetMotionQuality() == EMotionQuality::LinearCast) @@ -643,7 +643,7 @@ void BodyManager::GetActiveBodies(EBodyType inType, BodyIDVector &outBodyIDs) co UniqueLock lock(mActiveBodiesMutex JPH_IF_ENABLE_ASSERTS(, this, EPhysicsLockTypes::ActiveBodiesList)); const BodyID *active_bodies = mActiveBodies[(int)inType]; - outBodyIDs.assign(active_bodies, active_bodies + mNumActiveBodies[(int)inType]); + outBodyIDs.assign(active_bodies, active_bodies + mNumActiveBodies[(int)inType].load(memory_order_relaxed)); } void BodyManager::GetBodyIDs(BodyIDVector &outBodies) const @@ -1142,7 +1142,7 @@ void BodyManager::ValidateActiveBodyBounds() UniqueLock lock(mActiveBodiesMutex JPH_IF_ENABLE_ASSERTS(, this, EPhysicsLockTypes::ActiveBodiesList)); for (uint type = 0; type < cBodyTypeCount; ++type) - for (BodyID *id = mActiveBodies[type], *id_end = mActiveBodies[type] + mNumActiveBodies[type]; id < id_end; ++id) + for (BodyID *id = mActiveBodies[type], *id_end = mActiveBodies[type] + mNumActiveBodies[type].load(memory_order_relaxed); id < id_end; ++id) { const Body *body = mBodies[id->GetIndex()]; AABox cached = body->GetWorldSpaceBounds(); diff --git a/Jolt/Physics/Body/BodyManager.h b/Jolt/Physics/Body/BodyManager.h index fa1fb1117..518574707 100644 --- a/Jolt/Physics/Body/BodyManager.h +++ b/Jolt/Physics/Body/BodyManager.h @@ -117,7 +117,7 @@ class JPH_EXPORT BodyManager : public NonCopyable const BodyID * GetActiveBodiesUnsafe(EBodyType inType) const { return mActiveBodies[int(inType)]; } /// Get the number of active bodies. - uint32 GetNumActiveBodies(EBodyType inType) const { return mNumActiveBodies[int(inType)]; } + uint32 GetNumActiveBodies(EBodyType inType) const { return mNumActiveBodies[int(inType)].load(memory_order_acquire); } /// Get the number of active bodies that are using continuous collision detection uint32 GetNumActiveCCDBodies() const { return mNumActiveCCDBodies; } From 350fb7227bc46877f548b5bf35fdc7979c561013 Mon Sep 17 00:00:00 2001 From: Jorrit Rouwe Date: Sun, 29 Sep 2024 11:29:07 +0200 Subject: [PATCH 6/9] Fixed TSAN false positive TSAN detects a race between BodyManager::AddBodyToActiveBodies coming from PhysicsSystem::ProcessBodyPair and Body::GetIndexInActiveBodiesInternal, however we process the active bodies in batches using an atomic BodyManager::mNumActiveBodies with memory order acquire, so we know that the body is in a consistent state by the time we read the active index. Workaround is to use an atomic for mIndexInActiveBodies that we always read/write in relaxed mode. --- Jolt/Physics/Body/Body.h | 4 ++-- Jolt/Physics/Body/BodyManager.cpp | 17 +++++++++-------- Jolt/Physics/Body/MotionProperties.h | 4 ++-- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/Jolt/Physics/Body/Body.h b/Jolt/Physics/Body/Body.h index 5f7a18d56..a9d0e8331 100644 --- a/Jolt/Physics/Body/Body.h +++ b/Jolt/Physics/Body/Body.h @@ -49,7 +49,7 @@ class alignas(JPH_RVECTOR_ALIGNMENT) JPH_EXPORT_GCC_BUG_WORKAROUND Body : public inline bool IsSoftBody() const { return mBodyType == EBodyType::SoftBody; } /// If this body is currently actively simulating (true) or sleeping (false) - inline bool IsActive() const { return mMotionProperties != nullptr && mMotionProperties->mIndexInActiveBodies != cInactiveIndex; } + inline bool IsActive() const { return mMotionProperties != nullptr && mMotionProperties->GetIndexInActiveBodiesInternal() != cInactiveIndex; } /// Check if this body is static (not movable) inline bool IsStatic() const { return mMotionType == EMotionType::Static; } @@ -325,7 +325,7 @@ class alignas(JPH_RVECTOR_ALIGNMENT) JPH_EXPORT_GCC_BUG_WORKAROUND Body : public void SetShapeInternal(const Shape *inShape, bool inUpdateMassProperties); /// Access to the index in the BodyManager::mActiveBodies list - uint32 GetIndexInActiveBodiesInternal() const { return mMotionProperties != nullptr? mMotionProperties->mIndexInActiveBodies : cInactiveIndex; } + inline uint32 GetIndexInActiveBodiesInternal() const { return mMotionProperties != nullptr? mMotionProperties->GetIndexInActiveBodiesInternal() : cInactiveIndex; } /// Update eligibility for sleeping ECanSleep UpdateSleepStateInternal(float inDeltaTime, float inMaxMovement, float inTimeBeforeSleep); diff --git a/Jolt/Physics/Body/BodyManager.cpp b/Jolt/Physics/Body/BodyManager.cpp index db11bae89..529a2231e 100644 --- a/Jolt/Physics/Body/BodyManager.cpp +++ b/Jolt/Physics/Body/BodyManager.cpp @@ -498,7 +498,7 @@ void BodyManager::AddBodyToActiveBodies(Body &ioBody) BodyID *active_bodies = mActiveBodies[type]; MotionProperties *mp = ioBody.mMotionProperties; - mp->mIndexInActiveBodies = num_active_bodies; + mp->mIndexInActiveBodies.store(num_active_bodies, memory_order_relaxed); JPH_ASSERT(num_active_bodies < GetMaxBodies()); active_bodies[num_active_bodies] = ioBody.GetID(); num_active_bodies.fetch_add(1, memory_order_release); // Increment atomic after setting the body ID so that PhysicsSystem::JobFindCollisions (which doesn't lock the mActiveBodiesMutex) will only read valid IDs @@ -517,20 +517,21 @@ void BodyManager::RemoveBodyFromActiveBodies(Body &ioBody) uint32 last_body_index = num_active_bodies - 1; MotionProperties *mp = ioBody.mMotionProperties; - if (mp->mIndexInActiveBodies != last_body_index) + uint32 index_in_active_bodies = mp->mIndexInActiveBodies.load(memory_order_relaxed); + if (index_in_active_bodies != last_body_index) { // This is not the last body, use the last body to fill the hole BodyID last_body_id = active_bodies[last_body_index]; - active_bodies[mp->mIndexInActiveBodies] = last_body_id; + active_bodies[index_in_active_bodies] = last_body_id; // Update that body's index in the active list Body &last_body = *mBodies[last_body_id.GetIndex()]; - JPH_ASSERT(last_body.mMotionProperties->mIndexInActiveBodies == last_body_index); - last_body.mMotionProperties->mIndexInActiveBodies = mp->mIndexInActiveBodies; + JPH_ASSERT(last_body.mMotionProperties->mIndexInActiveBodies.load(memory_order_relaxed) == last_body_index); + last_body.mMotionProperties->mIndexInActiveBodies.store(index_in_active_bodies, memory_order_relaxed); } // Mark this body as no longer active - mp->mIndexInActiveBodies = Body::cInactiveIndex; + mp->mIndexInActiveBodies.store(Body::cInactiveIndex, memory_order_relaxed); // Remove unused element from active bodies list num_active_bodies.fetch_sub(1, memory_order_release); @@ -565,7 +566,7 @@ void BodyManager::ActivateBodies(const BodyID *inBodyIDs, int inNumber) body.ResetSleepTimer(); // Check if we're sleeping - if (body.mMotionProperties->mIndexInActiveBodies == Body::cInactiveIndex) + if (body.mMotionProperties->mIndexInActiveBodies.load(memory_order_relaxed) == Body::cInactiveIndex) { AddBodyToActiveBodies(body); @@ -597,7 +598,7 @@ void BodyManager::DeactivateBodies(const BodyID *inBodyIDs, int inNumber) JPH_ASSERT(body.IsInBroadPhase(), "Use BodyInterface::AddBody to add the body first!"); if (body.mMotionProperties != nullptr - && body.mMotionProperties->mIndexInActiveBodies != Body::cInactiveIndex) + && body.mMotionProperties->mIndexInActiveBodies.load(memory_order_relaxed) != Body::cInactiveIndex) { // Remove the body from the active bodies list RemoveBodyFromActiveBodies(body); diff --git a/Jolt/Physics/Body/MotionProperties.h b/Jolt/Physics/Body/MotionProperties.h index 6b5ec25de..181945ad7 100644 --- a/Jolt/Physics/Body/MotionProperties.h +++ b/Jolt/Physics/Body/MotionProperties.h @@ -205,7 +205,7 @@ class JPH_EXPORT MotionProperties void SetIslandIndexInternal(uint32 inIndex) { mIslandIndex = inIndex; } /// Access to the index in the active bodies array - uint32 GetIndexInActiveBodiesInternal() const { return mIndexInActiveBodies; } + inline uint32 GetIndexInActiveBodiesInternal() const { return mIndexInActiveBodies.load(memory_order_relaxed); } #ifdef JPH_DOUBLE_PRECISION inline DVec3 GetSleepTestOffset() const { return DVec3::sLoadDouble3Unsafe(mSleepTestOffset); } @@ -249,7 +249,7 @@ class JPH_EXPORT MotionProperties float mMaxLinearVelocity; ///< Maximum linear velocity that this body can reach (m/s) float mMaxAngularVelocity; ///< Maximum angular velocity that this body can reach (rad/s) float mGravityFactor; ///< Factor to multiply gravity with - uint32 mIndexInActiveBodies = cInactiveIndex; ///< If the body is active, this is the index in the active body list or cInactiveIndex if it is not active (note that there are 2 lists, one for rigid and one for soft bodies) + atomic mIndexInActiveBodies = cInactiveIndex; ///< If the body is active, this is the index in the active body list or cInactiveIndex if it is not active (note that there are 2 lists, one for rigid and one for soft bodies) uint32 mIslandIndex = cInactiveIndex; ///< Index of the island that this body is part of, when the body has not yet been updated or is not active this is cInactiveIndex // 1 byte aligned From 875df7293c2c4993b5cf861ad2b70d3904373752 Mon Sep 17 00:00:00 2001 From: Jorrit Rouwe Date: Sun, 29 Sep 2024 11:42:13 +0200 Subject: [PATCH 7/9] Suppress false positive in PhysicsSystem::JobFindCollisions --- Jolt/Physics/PhysicsSystem.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Jolt/Physics/PhysicsSystem.cpp b/Jolt/Physics/PhysicsSystem.cpp index 05501d549..c6478e039 100644 --- a/Jolt/Physics/PhysicsSystem.cpp +++ b/Jolt/Physics/PhysicsSystem.cpp @@ -830,6 +830,9 @@ static void sFinalizeContactAllocator(PhysicsUpdateContext::Step &ioStep, const ioStep.mContext->mErrors.fetch_or((uint32)inAllocator.mErrors, memory_order_relaxed); } +// Disable TSAN for this function. It detects a false positive race condition on mBodyPairs. +// We have written mBodyPairs before doing mWriteIdx++ and we check mWriteIdx before reading mBodyPairs, so this should be safe. +JPH_TSAN_NO_SANITIZE void PhysicsSystem::JobFindCollisions(PhysicsUpdateContext::Step *ioStep, int inJobIndex) { #ifdef JPH_ENABLE_ASSERTS From 6d67439927a0e52b42d40d379053133764b2dcd3 Mon Sep 17 00:00:00 2001 From: Jorrit Rouwe Date: Sun, 29 Sep 2024 11:47:09 +0200 Subject: [PATCH 8/9] Made comment better readable --- Jolt/Core/Profiler.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Jolt/Core/Profiler.cpp b/Jolt/Core/Profiler.cpp index 3d7f1bfac..39152b10b 100644 --- a/Jolt/Core/Profiler.cpp +++ b/Jolt/Core/Profiler.cpp @@ -60,7 +60,9 @@ uint64 Profiler::GetProcessorTicksPerSecond() const return (ticks - mReferenceTick) * 1000000000ULL / std::chrono::duration_cast(time - mReferenceTime).count(); } -JPH_TSAN_NO_SANITIZE // This function assumes that none of the threads are active while we're dumping the profile, otherwise there will be a race condition on mCurrentSample and the profile data +// This function assumes that none of the threads are active while we're dumping the profile, +// otherwise there will be a race condition on mCurrentSample and the profile data. +JPH_TSAN_NO_SANITIZE void Profiler::NextFrame() { std::lock_guard lock(mLock); From 26982c996893fd3e65276fdb90d89b8922f9e259 Mon Sep 17 00:00:00 2001 From: Jorrit Rouwe Date: Sun, 29 Sep 2024 17:57:12 +0200 Subject: [PATCH 9/9] Workaround for what appears to be a compiler bug in MSVC distribution build --- Jolt/Physics/Body/Body.h | 11 +++++++++-- Jolt/Physics/Body/BodyManager.cpp | 24 ++++++++++++------------ Jolt/Physics/Body/MotionProperties.h | 4 ++-- 3 files changed, 23 insertions(+), 16 deletions(-) diff --git a/Jolt/Physics/Body/Body.h b/Jolt/Physics/Body/Body.h index a9d0e8331..6f5c0d465 100644 --- a/Jolt/Physics/Body/Body.h +++ b/Jolt/Physics/Body/Body.h @@ -48,8 +48,10 @@ class alignas(JPH_RVECTOR_ALIGNMENT) JPH_EXPORT_GCC_BUG_WORKAROUND Body : public /// Check if this body is a soft body inline bool IsSoftBody() const { return mBodyType == EBodyType::SoftBody; } + // See comment at GetIndexInActiveBodiesInternal for reasoning why TSAN is disabled here + JPH_TSAN_NO_SANITIZE /// If this body is currently actively simulating (true) or sleeping (false) - inline bool IsActive() const { return mMotionProperties != nullptr && mMotionProperties->GetIndexInActiveBodiesInternal() != cInactiveIndex; } + inline bool IsActive() const { return mMotionProperties != nullptr && mMotionProperties->mIndexInActiveBodies != cInactiveIndex; } /// Check if this body is static (not movable) inline bool IsStatic() const { return mMotionType == EMotionType::Static; } @@ -324,8 +326,13 @@ class alignas(JPH_RVECTOR_ALIGNMENT) JPH_EXPORT_GCC_BUG_WORKAROUND Body : public /// @param inUpdateMassProperties When true, the mass and inertia tensor is recalculated void SetShapeInternal(const Shape *inShape, bool inUpdateMassProperties); + // TSAN detects a race between BodyManager::AddBodyToActiveBodies coming from PhysicsSystem::ProcessBodyPair and Body::GetIndexInActiveBodiesInternal coming from PhysicsSystem::ProcessBodyPair. + // When PhysicsSystem::ProcessBodyPair activates a body, it updates mIndexInActiveBodies and then updates BodyManager::mNumActiveBodies with release semantics. PhysicsSystem::ProcessBodyPair will + // then finish its loop of active bodies and at the end of the loop it will read BodyManager::mNumActiveBodies with acquire semantics to see if any bodies were activated during the loop. + // This means that changes to mIndexInActiveBodies must be visible to the thread, so TSANs report must be a false positive. We suppress the warning here. + JPH_TSAN_NO_SANITIZE /// Access to the index in the BodyManager::mActiveBodies list - inline uint32 GetIndexInActiveBodiesInternal() const { return mMotionProperties != nullptr? mMotionProperties->GetIndexInActiveBodiesInternal() : cInactiveIndex; } + uint32 GetIndexInActiveBodiesInternal() const { return mMotionProperties != nullptr? mMotionProperties->mIndexInActiveBodies : cInactiveIndex; } /// Update eligibility for sleeping ECanSleep UpdateSleepStateInternal(float inDeltaTime, float inMaxMovement, float inTimeBeforeSleep); diff --git a/Jolt/Physics/Body/BodyManager.cpp b/Jolt/Physics/Body/BodyManager.cpp index 529a2231e..a48e4b0f8 100644 --- a/Jolt/Physics/Body/BodyManager.cpp +++ b/Jolt/Physics/Body/BodyManager.cpp @@ -498,9 +498,10 @@ void BodyManager::AddBodyToActiveBodies(Body &ioBody) BodyID *active_bodies = mActiveBodies[type]; MotionProperties *mp = ioBody.mMotionProperties; - mp->mIndexInActiveBodies.store(num_active_bodies, memory_order_relaxed); - JPH_ASSERT(num_active_bodies < GetMaxBodies()); - active_bodies[num_active_bodies] = ioBody.GetID(); + uint32 num_active_bodies_val = num_active_bodies.load(memory_order_relaxed); + mp->mIndexInActiveBodies = num_active_bodies_val; + JPH_ASSERT(num_active_bodies_val < GetMaxBodies()); + active_bodies[num_active_bodies_val] = ioBody.GetID(); num_active_bodies.fetch_add(1, memory_order_release); // Increment atomic after setting the body ID so that PhysicsSystem::JobFindCollisions (which doesn't lock the mActiveBodiesMutex) will only read valid IDs // Count CCD bodies @@ -515,23 +516,22 @@ void BodyManager::RemoveBodyFromActiveBodies(Body &ioBody) atomic &num_active_bodies = mNumActiveBodies[type]; BodyID *active_bodies = mActiveBodies[type]; - uint32 last_body_index = num_active_bodies - 1; + uint32 last_body_index = num_active_bodies.load(memory_order_relaxed) - 1; MotionProperties *mp = ioBody.mMotionProperties; - uint32 index_in_active_bodies = mp->mIndexInActiveBodies.load(memory_order_relaxed); - if (index_in_active_bodies != last_body_index) + if (mp->mIndexInActiveBodies != last_body_index) { // This is not the last body, use the last body to fill the hole BodyID last_body_id = active_bodies[last_body_index]; - active_bodies[index_in_active_bodies] = last_body_id; + active_bodies[mp->mIndexInActiveBodies] = last_body_id; // Update that body's index in the active list Body &last_body = *mBodies[last_body_id.GetIndex()]; - JPH_ASSERT(last_body.mMotionProperties->mIndexInActiveBodies.load(memory_order_relaxed) == last_body_index); - last_body.mMotionProperties->mIndexInActiveBodies.store(index_in_active_bodies, memory_order_relaxed); + JPH_ASSERT(last_body.mMotionProperties->mIndexInActiveBodies == last_body_index); + last_body.mMotionProperties->mIndexInActiveBodies = mp->mIndexInActiveBodies; } // Mark this body as no longer active - mp->mIndexInActiveBodies.store(Body::cInactiveIndex, memory_order_relaxed); + mp->mIndexInActiveBodies = Body::cInactiveIndex; // Remove unused element from active bodies list num_active_bodies.fetch_sub(1, memory_order_release); @@ -566,7 +566,7 @@ void BodyManager::ActivateBodies(const BodyID *inBodyIDs, int inNumber) body.ResetSleepTimer(); // Check if we're sleeping - if (body.mMotionProperties->mIndexInActiveBodies.load(memory_order_relaxed) == Body::cInactiveIndex) + if (body.mMotionProperties->mIndexInActiveBodies == Body::cInactiveIndex) { AddBodyToActiveBodies(body); @@ -598,7 +598,7 @@ void BodyManager::DeactivateBodies(const BodyID *inBodyIDs, int inNumber) JPH_ASSERT(body.IsInBroadPhase(), "Use BodyInterface::AddBody to add the body first!"); if (body.mMotionProperties != nullptr - && body.mMotionProperties->mIndexInActiveBodies.load(memory_order_relaxed) != Body::cInactiveIndex) + && body.mMotionProperties->mIndexInActiveBodies != Body::cInactiveIndex) { // Remove the body from the active bodies list RemoveBodyFromActiveBodies(body); diff --git a/Jolt/Physics/Body/MotionProperties.h b/Jolt/Physics/Body/MotionProperties.h index 181945ad7..6b5ec25de 100644 --- a/Jolt/Physics/Body/MotionProperties.h +++ b/Jolt/Physics/Body/MotionProperties.h @@ -205,7 +205,7 @@ class JPH_EXPORT MotionProperties void SetIslandIndexInternal(uint32 inIndex) { mIslandIndex = inIndex; } /// Access to the index in the active bodies array - inline uint32 GetIndexInActiveBodiesInternal() const { return mIndexInActiveBodies.load(memory_order_relaxed); } + uint32 GetIndexInActiveBodiesInternal() const { return mIndexInActiveBodies; } #ifdef JPH_DOUBLE_PRECISION inline DVec3 GetSleepTestOffset() const { return DVec3::sLoadDouble3Unsafe(mSleepTestOffset); } @@ -249,7 +249,7 @@ class JPH_EXPORT MotionProperties float mMaxLinearVelocity; ///< Maximum linear velocity that this body can reach (m/s) float mMaxAngularVelocity; ///< Maximum angular velocity that this body can reach (rad/s) float mGravityFactor; ///< Factor to multiply gravity with - atomic mIndexInActiveBodies = cInactiveIndex; ///< If the body is active, this is the index in the active body list or cInactiveIndex if it is not active (note that there are 2 lists, one for rigid and one for soft bodies) + uint32 mIndexInActiveBodies = cInactiveIndex; ///< If the body is active, this is the index in the active body list or cInactiveIndex if it is not active (note that there are 2 lists, one for rigid and one for soft bodies) uint32 mIslandIndex = cInactiveIndex; ///< Index of the island that this body is part of, when the body has not yet been updated or is not active this is cInactiveIndex // 1 byte aligned