Use shared locks in physics system when using multithreaded bullet

apps/openmw/mwphysics/mtphysics.cpp

@@ -3,6 +3,7 @@
 #include <optional>
 #include <shared_mutex>
 #include <mutex>
+#include <stdexcept>
 
 #include <BulletCollision/BroadphaseCollision/btDbvtBroadphase.h>
 #include <BulletCollision/CollisionShapes/btCollisionShape.h>
@@ -36,11 +37,11 @@
 namespace
 {
     template <class Mutex>
-    std::optional<std::unique_lock<Mutex>> makeExclusiveLock(Mutex& mutex, unsigned threadCount)
+    std::optional<std::unique_lock<Mutex>> makeExclusiveLock(Mutex& mutex, MWPhysics::LockingPolicy lockingPolicy)
     {
-        if (threadCount > 0)
+        if (lockingPolicy == MWPhysics::LockingPolicy::NoLocks)
-            return std::unique_lock(mutex);
+            return {};
-        return {};
+        return std::unique_lock(mutex);
     }
 
     /// @brief A scoped lock that is either exclusive or inexistent depending on configuration
@@ -48,22 +49,21 @@ namespace
     class MaybeExclusiveLock
     {
         public:
-            /// @param mutex a mutex
+            explicit MaybeExclusiveLock(Mutex& mutex, MWPhysics::LockingPolicy lockingPolicy)
-            /// @param threadCount decide wether the excluse lock will be taken
+                : mImpl(makeExclusiveLock(mutex, lockingPolicy))
-            explicit MaybeExclusiveLock(Mutex& mutex, unsigned threadCount)
+            {
-                : mImpl(makeExclusiveLock(mutex, threadCount))
+            }
-            {}
 
         private:
             std::optional<std::unique_lock<Mutex>> mImpl;
     };
 
     template <class Mutex>
-    std::optional<std::shared_lock<Mutex>> makeSharedLock(Mutex& mutex, unsigned threadCount)
+    std::optional<std::shared_lock<Mutex>> makeSharedLock(Mutex& mutex, MWPhysics::LockingPolicy lockingPolicy)
     {
-        if (threadCount > 0)
+        if (lockingPolicy == MWPhysics::LockingPolicy::NoLocks)
-            return std::shared_lock(mutex);
+            return {};
-        return {};
+        return std::shared_lock(mutex);
     }
 
     /// @brief A scoped lock that is either shared or inexistent depending on configuration
@@ -71,24 +71,31 @@ namespace
     class MaybeSharedLock
     {
         public:
-            /// @param mutex a shared mutex
+            explicit MaybeSharedLock(Mutex& mutex, MWPhysics::LockingPolicy lockingPolicy)
-            /// @param threadCount decide wether the shared lock will be taken
+                : mImpl(makeSharedLock(mutex, lockingPolicy))
-            explicit MaybeSharedLock(Mutex& mutex, unsigned threadCount)
+            {
-                : mImpl(makeSharedLock(mutex, threadCount))
+            }
-            {}
 
         private:
             std::optional<std::shared_lock<Mutex>> mImpl;
     };
 
     template <class Mutex>
-    std::variant<std::monostate, std::unique_lock<Mutex>, std::shared_lock<Mutex>> makeLock(Mutex& mutex, unsigned threadCount)
+    std::variant<std::monostate, std::unique_lock<Mutex>, std::shared_lock<Mutex>> makeLock(
+        Mutex& mutex, MWPhysics::LockingPolicy lockingPolicy)
     {
-        if (threadCount > 1)
+        switch (lockingPolicy)
-            return std::shared_lock(mutex);
+        {
-        if (threadCount == 1)
+            case MWPhysics::LockingPolicy::NoLocks:
-            return std::unique_lock(mutex);
+                return std::monostate{};
-        return std::monostate {};
+            case MWPhysics::LockingPolicy::ExclusiveLocksOnly:
+                return std::unique_lock(mutex);
+            case MWPhysics::LockingPolicy::AllowSharedLocks:
+                return std::shared_lock(mutex);
+        };
+
+        throw std::runtime_error("Unsupported LockingPolicy: "
+            + std::to_string(static_cast<std::underlying_type_t<MWPhysics::LockingPolicy>>(lockingPolicy)));
     }
 
     /// @brief A scoped lock that is either shared, exclusive or inexistent depending on configuration
@@ -96,10 +103,10 @@ namespace
     class MaybeLock
     {
         public:
-            /// @param mutex a shared mutex
+            explicit MaybeLock(Mutex& mutex, MWPhysics::LockingPolicy lockingPolicy)
-            /// @param threadCount decide wether the lock will be shared, exclusive or inexistent
+                : mImpl(makeLock(mutex, lockingPolicy))
-            explicit MaybeLock(Mutex& mutex, unsigned threadCount)
+            {
-                : mImpl(makeLock(mutex, threadCount)) {}
+            }
 
         private:
             std::variant<std::monostate, std::unique_lock<Mutex>, std::shared_lock<Mutex>> mImpl;
@@ -132,7 +139,7 @@ namespace
         {
             const Impl& mImpl;
             std::shared_mutex& mCollisionWorldMutex;
-            const unsigned mNumThreads;
+            const MWPhysics::LockingPolicy mLockingPolicy;
 
             template <class Ptr, class FrameData>
             void operator()(MWPhysics::SimulationImpl<Ptr, FrameData>& sim) const
@@ -144,7 +151,7 @@ namespace
                 // Locked shared_ptr has to be destructed after releasing mCollisionWorldMutex to avoid
                 // possible deadlock. Ptr destructor also acquires mCollisionWorldMutex.
                 const std::pair arg(std::move(ptr), frameData);
-                const Lock<std::shared_mutex> lock(mCollisionWorldMutex, mNumThreads);
+                const Lock<std::shared_mutex> lock(mCollisionWorldMutex, mLockingPolicy);
                 mImpl(arg);
             }
         };
@@ -284,23 +291,43 @@ namespace
             }
         };
     }
+}
 
-    namespace Config
+namespace MWPhysics
+{
+    namespace
     {
-        /// @return either the number of thread as configured by the user, or 1 if Bullet doesn't support multithreading and user requested more than 1 background threads
+        int getMaxBulletSupportedThreads()
-        unsigned computeNumThreads()
         {
-            int wantedThread = Settings::Manager::getInt("async num threads", "Physics");
-
             auto broad = std::make_unique<btDbvtBroadphase>();
-            auto maxSupportedThreads = broad->m_rayTestStacks.size();
+            return broad->m_rayTestStacks.size();
-            auto threadSafeBullet = (maxSupportedThreads > 1);
+        }
-            if (!threadSafeBullet && wantedThread > 1)
+
+        LockingPolicy detectLockingPolicy()
+        {
+            if (Settings::Manager::getInt("async num threads", "Physics") < 1)
+                return LockingPolicy::NoLocks;
+            if (getMaxBulletSupportedThreads() > 1)
+                return LockingPolicy::AllowSharedLocks;
+            Log(Debug::Warning) << "Bullet was not compiled with multithreading support, 1 async thread will be used";
+            return LockingPolicy::ExclusiveLocksOnly;
+        }
+
+        unsigned getNumThreads(LockingPolicy lockingPolicy)
+        {
+            switch (lockingPolicy)
             {
-                Log(Debug::Warning) << "Bullet was not compiled with multithreading support, 1 async thread will be used";
+                case LockingPolicy::NoLocks:
-                return 1;
+                    return 0;
+                case LockingPolicy::ExclusiveLocksOnly:
+                    return 1;
+                case LockingPolicy::AllowSharedLocks:
+                    return static_cast<unsigned>(std::max(
+                        getMaxBulletSupportedThreads(), Settings::Manager::getInt("async num threads", "Physics")));
             }
-            return static_cast<unsigned>(std::max(0, wantedThread));
+
+            throw std::runtime_error("Unsupported LockingPolicy: "
+                + std::to_string(static_cast<std::underlying_type_t<LockingPolicy>>(lockingPolicy)));
         }
     }
 }
@@ -313,7 +340,8 @@ namespace MWPhysics
           , mTimeAccum(0.f)
           , mCollisionWorld(collisionWorld)
           , mDebugDrawer(debugDrawer)
-          , mNumThreads(Config::computeNumThreads())
+          , mLockingPolicy(detectLockingPolicy())
+          , mNumThreads(getNumThreads(mLockingPolicy))
           , mNumJobs(0)
           , mRemainingSteps(0)
           , mLOSCacheExpiry(Settings::Manager::getInt("lineofsight keep inactive cache", "Physics"))
@@ -354,7 +382,7 @@ namespace MWPhysics
     {
         waitForWorkers();
         {
-            MaybeExclusiveLock lock(mSimulationMutex, mNumThreads);
+            MaybeExclusiveLock lock(mSimulationMutex, mLockingPolicy);
             mQuit = true;
             mNumJobs = 0;
             mRemainingSteps = 0;
@@ -417,7 +445,7 @@ namespace MWPhysics
         // This function run in the main thread.
         // While the mSimulationMutex is held, background physics threads can't run.
 
-        MaybeExclusiveLock lock(mSimulationMutex, mNumThreads);
+        MaybeExclusiveLock lock(mSimulationMutex, mLockingPolicy);
 
         double timeStart = mTimer->tick();
 
@@ -475,7 +503,7 @@ namespace MWPhysics
     void PhysicsTaskScheduler::resetSimulation(const ActorMap& actors)
     {
         waitForWorkers();
-        MaybeExclusiveLock lock(mSimulationMutex, mNumThreads);
+        MaybeExclusiveLock lock(mSimulationMutex, mLockingPolicy);
         mBudget.reset(mDefaultPhysicsDt);
         mAsyncBudget.reset(0.0f);
         mSimulations.clear();
@@ -488,25 +516,25 @@ namespace MWPhysics
 
     void PhysicsTaskScheduler::rayTest(const btVector3& rayFromWorld, const btVector3& rayToWorld, btCollisionWorld::RayResultCallback& resultCallback) const
     {
-        MaybeLock lock(mCollisionWorldMutex, mNumThreads);
+        MaybeLock lock(mCollisionWorldMutex, mLockingPolicy);
         mCollisionWorld->rayTest(rayFromWorld, rayToWorld, resultCallback);
     }
 
     void PhysicsTaskScheduler::convexSweepTest(const btConvexShape* castShape, const btTransform& from, const btTransform& to, btCollisionWorld::ConvexResultCallback& resultCallback) const
     {
-        MaybeLock lock(mCollisionWorldMutex, mNumThreads);
+        MaybeLock lock(mCollisionWorldMutex, mLockingPolicy);
         mCollisionWorld->convexSweepTest(castShape, from, to, resultCallback);
     }
 
     void PhysicsTaskScheduler::contactTest(btCollisionObject* colObj, btCollisionWorld::ContactResultCallback& resultCallback)
     {
-        MaybeSharedLock lock(mCollisionWorldMutex, mNumThreads);
+        MaybeSharedLock lock(mCollisionWorldMutex, mLockingPolicy);
         ContactTestWrapper::contactTest(mCollisionWorld, colObj, resultCallback);
     }
 
     std::optional<btVector3> PhysicsTaskScheduler::getHitPoint(const btTransform& from, btCollisionObject* target)
     {
-        MaybeLock lock(mCollisionWorldMutex, mNumThreads);
+        MaybeLock lock(mCollisionWorldMutex, mLockingPolicy);
         // target the collision object's world origin, this should be the center of the collision object
         btTransform rayTo;
         rayTo.setIdentity();
@@ -523,33 +551,33 @@ namespace MWPhysics
 
     void PhysicsTaskScheduler::aabbTest(const btVector3& aabbMin, const btVector3& aabbMax, btBroadphaseAabbCallback& callback)
     {
-        MaybeSharedLock lock(mCollisionWorldMutex, mNumThreads);
+        MaybeSharedLock lock(mCollisionWorldMutex, mLockingPolicy);
         mCollisionWorld->getBroadphase()->aabbTest(aabbMin, aabbMax, callback);
     }
 
     void PhysicsTaskScheduler::getAabb(const btCollisionObject* obj, btVector3& min, btVector3& max)
     {
-        MaybeSharedLock lock(mCollisionWorldMutex, mNumThreads);
+        MaybeSharedLock lock(mCollisionWorldMutex, mLockingPolicy);
         obj->getCollisionShape()->getAabb(obj->getWorldTransform(), min, max);
     }
 
     void PhysicsTaskScheduler::setCollisionFilterMask(btCollisionObject* collisionObject, int collisionFilterMask)
     {
-        MaybeExclusiveLock lock(mCollisionWorldMutex, mNumThreads);
+        MaybeExclusiveLock lock(mCollisionWorldMutex, mLockingPolicy);
         collisionObject->getBroadphaseHandle()->m_collisionFilterMask = collisionFilterMask;
     }
 
     void PhysicsTaskScheduler::addCollisionObject(btCollisionObject* collisionObject, int collisionFilterGroup, int collisionFilterMask)
     {
         mCollisionObjects.insert(collisionObject);
-        MaybeExclusiveLock lock(mCollisionWorldMutex, mNumThreads);
+        MaybeExclusiveLock lock(mCollisionWorldMutex, mLockingPolicy);
         mCollisionWorld->addCollisionObject(collisionObject, collisionFilterGroup, collisionFilterMask);
     }
 
     void PhysicsTaskScheduler::removeCollisionObject(btCollisionObject* collisionObject)
     {
         mCollisionObjects.erase(collisionObject);
-        MaybeExclusiveLock lock(mCollisionWorldMutex, mNumThreads);
+        MaybeExclusiveLock lock(mCollisionWorldMutex, mLockingPolicy);
         mCollisionWorld->removeCollisionObject(collisionObject);
     }
 
@@ -561,14 +589,14 @@ namespace MWPhysics
         }
         else
         {
-            MaybeExclusiveLock lock(mUpdateAabbMutex, mNumThreads);
+            MaybeExclusiveLock lock(mUpdateAabbMutex, mLockingPolicy);
             mUpdateAabb.insert(ptr);
         }
     }
 
     bool PhysicsTaskScheduler::getLineOfSight(const std::shared_ptr<Actor>& actor1, const std::shared_ptr<Actor>& actor2)
     {
-        MaybeExclusiveLock lock(mLOSCacheMutex, mNumThreads);
+        MaybeExclusiveLock lock(mLOSCacheMutex, mLockingPolicy);
 
         auto req = LOSRequest(actor1, actor2);
         auto result = std::find(mLOSCache.begin(), mLOSCache.end(), req);
@@ -584,7 +612,7 @@ namespace MWPhysics
 
     void PhysicsTaskScheduler::refreshLOSCache()
     {
-        MaybeSharedLock lock(mLOSCacheMutex, mNumThreads);
+        MaybeSharedLock lock(mLOSCacheMutex, mLockingPolicy);
         int job = 0;
         int numLOS = mLOSCache.size();
         while ((job = mNextLOS.fetch_add(1, std::memory_order_relaxed)) < numLOS)
@@ -603,7 +631,7 @@ namespace MWPhysics
 
     void PhysicsTaskScheduler::updateAabbs()
     {
-        MaybeExclusiveLock lock(mUpdateAabbMutex, mNumThreads);
+        MaybeExclusiveLock lock(mUpdateAabbMutex, mLockingPolicy);
         std::for_each(mUpdateAabb.begin(), mUpdateAabb.end(),
             [this](const std::weak_ptr<PtrHolder>& ptr)
             {
@@ -616,7 +644,7 @@ namespace MWPhysics
 
     void PhysicsTaskScheduler::updatePtrAabb(const std::shared_ptr<PtrHolder>& ptr)
     {
-        MaybeExclusiveLock lock(mCollisionWorldMutex, mNumThreads);
+        MaybeExclusiveLock lock(mCollisionWorldMutex, mLockingPolicy);
         if (const auto actor = std::dynamic_pointer_cast<Actor>(ptr))
         {
             actor->updateCollisionObjectPosition();
@@ -653,7 +681,7 @@ namespace MWPhysics
     void PhysicsTaskScheduler::updateActorsPositions()
     {
         const Visitors::UpdatePosition impl{mCollisionWorld};
-        const Visitors::WithLockedPtr<Visitors::UpdatePosition, MaybeExclusiveLock> vis{impl, mCollisionWorldMutex, mNumThreads};
+        const Visitors::WithLockedPtr<Visitors::UpdatePosition, MaybeExclusiveLock> vis{impl, mCollisionWorldMutex, mLockingPolicy};
         for (Simulation& sim : mSimulations)
             std::visit(vis, sim);
     }
@@ -667,7 +695,7 @@ namespace MWPhysics
         resultCallback.m_collisionFilterGroup = CollisionType_AnyPhysical;
         resultCallback.m_collisionFilterMask = CollisionType_World|CollisionType_HeightMap|CollisionType_Door;
 
-        MaybeLock lockColWorld(mCollisionWorldMutex, mNumThreads);
+        MaybeLock lockColWorld(mCollisionWorldMutex, mLockingPolicy);
         mCollisionWorld->rayTest(pos1, pos2, resultCallback);
 
         return !resultCallback.hasHit();
@@ -680,7 +708,7 @@ namespace MWPhysics
             mPreStepBarrier->wait([this] { afterPreStep(); });
             int job = 0;
             const Visitors::Move impl{mPhysicsDt, mCollisionWorld, *mWorldFrameData};
-            const Visitors::WithLockedPtr<Visitors::Move, MaybeLock> vis{impl, mCollisionWorldMutex, mNumThreads};
+            const Visitors::WithLockedPtr<Visitors::Move, MaybeLock> vis{impl, mCollisionWorldMutex, mLockingPolicy};
             while ((job = mNextJob.fetch_add(1, std::memory_order_relaxed)) < mNumJobs)
                 std::visit(vis, mSimulations[job]);
 
@@ -708,7 +736,7 @@ namespace MWPhysics
 
     void PhysicsTaskScheduler::debugDraw()
     {
-        MaybeSharedLock lock(mCollisionWorldMutex, mNumThreads);
+        MaybeSharedLock lock(mCollisionWorldMutex, mLockingPolicy);
         mDebugDrawer->step();
     }
 
@@ -734,7 +762,7 @@ namespace MWPhysics
         if (!mRemainingSteps)
             return;
         const Visitors::PreStep impl{mCollisionWorld};
-        const Visitors::WithLockedPtr<Visitors::PreStep, MaybeExclusiveLock> vis{impl, mCollisionWorldMutex, mNumThreads};
+        const Visitors::WithLockedPtr<Visitors::PreStep, MaybeExclusiveLock> vis{impl, mCollisionWorldMutex, mLockingPolicy};
         for (auto& sim : mSimulations)
             std::visit(vis, sim);
     }
@@ -752,7 +780,7 @@ namespace MWPhysics
     void PhysicsTaskScheduler::afterPostSim()
     {
         {
-            MaybeExclusiveLock lock(mLOSCacheMutex, mNumThreads);
+            MaybeExclusiveLock lock(mLOSCacheMutex, mLockingPolicy);
             mLOSCache.erase(
                     std::remove_if(mLOSCache.begin(), mLOSCache.end(),
                         [](const LOSRequest& req) { return req.mStale; }),

apps/openmw/mwphysics/mtphysics.hpp

@@ -29,6 +29,13 @@ namespace MWRender
 
 namespace MWPhysics
 {
+    enum class LockingPolicy
+    {
+        NoLocks,
+        ExclusiveLocksOnly,
+        AllowSharedLocks,
+    };
+
     class PhysicsTaskScheduler
     {
         public:
@@ -92,6 +99,7 @@ namespace MWPhysics
             std::unique_ptr<Misc::Barrier> mPostStepBarrier;
             std::unique_ptr<Misc::Barrier> mPostSimBarrier;
 
+            LockingPolicy mLockingPolicy;
             unsigned mNumThreads;
             int mNumJobs;
             int mRemainingSteps;