Add performance test to TSAN build (#1285)

* Fixed race condition in island splitter where it could potentially read mNumIterations before it was written. This could theoretically lead to an island not being updated during a step (this has never been observed though).
* TSAN build no longer uses custom allocators
* Specifying memory ordering explicitly for mNumActiveBodies (can be more relaxed than what they were)
* Suppress TSAN false positives for PhysicsSystem::JobFindCollisions, Profiler::NextFrame, Body::GetIndexInActiveBodiesInternal and Body::IsActive

.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

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

Jolt/Core/Profiler.cpp

@@ -60,6 +60,9 @@ uint64 Profiler::GetProcessorTicksPerSecond() const
 	return (ticks - mReferenceTick) * 1000000000ULL / std::chrono::duration_cast<std::chrono::nanoseconds>(time - mReferenceTime).count();
 }
 
+// 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);

Jolt/Jolt.cmake

@@ -521,8 +521,8 @@ endif()
 target_compile_definitions(Jolt PUBLIC "$<$<CONFIG:Debug>:_DEBUG>")
 target_compile_definitions(Jolt PUBLIC "$<$<CONFIG:Release,Distribution,ReleaseASAN,ReleaseUBSAN,ReleaseTSAN,ReleaseCoverage>:NDEBUG>")
 
-# ASAN should use the default allocators
-target_compile_definitions(Jolt PUBLIC "$<$<CONFIG:ReleaseASAN>:JPH_DISABLE_TEMP_ALLOCATOR;JPH_DISABLE_CUSTOM_ALLOCATOR>")
+# ASAN and TSAN should use the default allocators
+target_compile_definitions(Jolt PUBLIC "$<$<CONFIG:ReleaseASAN,ReleaseTSAN>:JPH_DISABLE_TEMP_ALLOCATOR;JPH_DISABLE_CUSTOM_ALLOCATOR>")
 
 # Setting floating point exceptions
 if (FLOATING_POINT_EXCEPTIONS_ENABLED AND "${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC")

Jolt/Physics/Body/Body.h

@@ -48,6 +48,8 @@ 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->mIndexInActiveBodies != cInactiveIndex; }
 
@@ -324,6 +326,11 @@ 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
 	uint32					GetIndexInActiveBodiesInternal() const							{ return mMotionProperties != nullptr? mMotionProperties->mIndexInActiveBodies : cInactiveIndex; }
 

Jolt/Physics/Body/BodyManager.cpp

@@ -498,10 +498,11 @@ void BodyManager::AddBodyToActiveBodies(Body &ioBody)
 	BodyID *active_bodies = mActiveBodies[type];
 
 	MotionProperties *mp = ioBody.mMotionProperties;
-	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
+	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
 	if (mp->GetMotionQuality() == EMotionQuality::LinearCast)
@@ -515,7 +516,7 @@ void BodyManager::RemoveBodyFromActiveBodies(Body &ioBody)
 	atomic<uint32> &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;
 	if (mp->mIndexInActiveBodies != last_body_index)
 	{
@@ -533,7 +534,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 +644,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 +1143,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();

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; }

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)

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