Clear taskId when unlocking

Signed-off-by: Alexander Damian <[email protected]>
bloomberg · May 18, 2020 · c334302 · c334302
1 parent 5bc27d3
commit c334302
Show file tree

Hide file tree

Showing 8 changed files with 87 additions and 64 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -53,7 +53,7 @@ if (NOT CMAKE_CXX_STANDARD)
 endif()
 #Set compile flags if CXXFLAGS environment variable is not set
 if (NOT CMAKE_CXX_FLAGS)
-    set(CMAKE_CXX_FLAGS "-Wall -Wextra -O0 -m${MODE} -std=c++${CMAKE_CXX_STANDARD} -ftemplate-backtrace-limit=0")
+    set(CMAKE_CXX_FLAGS "-Wall -Wextra -O0 -m${MODE} -std=c++${CMAKE_CXX_STANDARD} -ftemplate-backtrace-limit=0 -faligned-new")
 endif()
 if (QUANTUM_VERBOSE_MAKEFILE)
     message(STATUS "CMAKE_CXX_FLAGS = ${CMAKE_CXX_FLAGS}")

diff --git a/quantum/impl/quantum_mutex_impl.h b/quantum/impl/quantum_mutex_impl.h
@@ -69,8 +69,6 @@ void Mutex::lockImpl(ICoroSync::Ptr sync)
     {
         yield(sync);
     }
-    //mutex is locked
-    _taskId = local::taskId();
 }
 
 inline
@@ -89,6 +87,7 @@ void Mutex::unlock()
 {
     if (_taskId == local::taskId()) {
         _spinlock.unlock();
+        _taskId.clear();
     }
 }
 

diff --git a/quantum/impl/quantum_spinlock_traits_impl.h b/quantum/impl/quantum_spinlock_traits_impl.h
@@ -48,7 +48,7 @@ SpinLockTraits::sleepDuration() {
 
 inline SpinLockTraits::BackoffPolicy&
 SpinLockTraits::backoffPolicy() {
-    static BackoffPolicy backoffPolicy{QUANTUM_SPINLOCK_BACKOFF_POLICY};
+    static BackoffPolicy backoffPolicy = static_cast<BackoffPolicy>(QUANTUM_SPINLOCK_BACKOFF_POLICY);
     return backoffPolicy;
 }
 

diff --git a/quantum/impl/quantum_task_id_impl.h b/quantum/impl/quantum_task_id_impl.h
@@ -20,8 +20,8 @@
 //##############################################################################################
 #include <boost/functional/hash.hpp>
 #include <functional>
-#include <random>
 #include <limits>
+#include <atomic>
 
 namespace Bloomberg {
 namespace quantum {
@@ -123,14 +123,18 @@ bool TaskId::isCoroutine() const
     return _id < 0;
 }
 
+inline
+void TaskId::clear()
+{
+    _id = 0;
+    _threadId = std::thread::id();
+}
+
 inline
 ssize_t TaskId::generate()
 {
-    static std::random_device rd;
-    static std::mt19937_64 generator(rd());
-    //use only half the distribution
-    static std::uniform_int_distribution<size_t> distribution(1, std::numeric_limits<ssize_t>::max());
-    return distribution(generator);
+    static std::atomic<ssize_t> id;
+    return ++id;
 }
 
 inline

diff --git a/quantum/quantum_spinlock_traits.h b/quantum/quantum_spinlock_traits.h
@@ -51,10 +51,21 @@ struct SpinLockTraits {
         EqualStep = QUANTUM_BACKOFF_EQUALSTEP,     ///< Identical backoff amount
         Random = QUANTUM_BACKOFF_RANDOM            ///< Random backoff amount
     };
+
+    /// @brief Initial number of spins for the backoff
     static size_t &minSpins();
+
+    /// @brief The maximum number of spins to wait before yielding,
+    ///        as well as the backoff limit.
     static size_t &maxSpins();
+
+    /// @brief The number of yields before sleeping the thread
     static size_t &numYieldsBeforeSleep();
+
+    /// @brief The sleep duration (after there are no more yields)
     static std::chrono::microseconds &sleepDuration();
+
+    /// @brief Backoff policy while spinning
     static BackoffPolicy &backoffPolicy();
 };
 

diff --git a/quantum/quantum_task_id.h b/quantum/quantum_task_id.h
@@ -44,11 +44,11 @@ class TaskId
     size_t id() const;
     std::thread::id threadId() const;
     bool isCoroutine() const;
+    void clear();
 private:
     TaskId(CoroContextTag);
     TaskId(ThreadContextTag);
     void captureThreadId();
-
     static ssize_t generate();
 
     ssize_t         _id{0}; //negative values reserved for coroutines

diff --git a/quantum/util/impl/quantum_spinlock_util_impl.h b/quantum/util/impl/quantum_spinlock_util_impl.h
@@ -20,6 +20,7 @@
 //##############################################################################################
 
 #include <quantum/quantum_spinlock_traits.h>
+#include <random>
 
 //Arch macros: https://sourceforge.net/p/predef/wiki/Architectures
 #if defined(_MSC_VER) && \
@@ -79,7 +80,7 @@ void SpinLockUtil::lockShared(std::atomic_int &flag,
             newValue = oldValue + 1;
             pauseCPU();
         }
-        if (oldValue >= sharedValue)
+        if (oldValue < newValue)
         {
             //We obtained the lock
             reset();
@@ -101,7 +102,7 @@ void SpinLockUtil::pauseCPU()
       (defined(__i386__) || \
        defined(__ia64__) || \
        defined(__x86_64__))
-    __asm__ __volatile__( "pause":: : "memory" );
+    __asm__ __volatile__( "pause" ::: "memory" );
 #else
     __asm__ __volatile__( "rep;nop" ::: "memory" );
 #endif
@@ -122,6 +123,7 @@ void SpinLockUtil::yieldOrSleep()
         std::this_thread::yield();
     }
     else {
+        assert(SpinLockTraits::sleepDuration() >= std::chrono::microseconds::zero());
         std::this_thread::sleep_for(SpinLockTraits::sleepDuration());
     }
 }
@@ -132,38 +134,50 @@ void SpinLockUtil::backoff()
     using Distribution = std::uniform_int_distribution<size_t>;
     static thread_local std::mt19937_64 gen(std::random_device{}());
     static thread_local Distribution distribution;
-    if (numSpins() == 0) {
+    if (numSpins() == 0)
+    {
         //Initialize for the first time
+        assert(SpinLockTraits::minSpins() <= SpinLockTraits::maxSpins());
         if ((SpinLockTraits::backoffPolicy() == SpinLockTraits::BackoffPolicy::EqualStep) ||
-            (SpinLockTraits::backoffPolicy() == SpinLockTraits::BackoffPolicy::Random)) {
+            (SpinLockTraits::backoffPolicy() == SpinLockTraits::BackoffPolicy::Random))
+        {
             //Generate a number from the entire range
             numSpins() = distribution(gen, Distribution::param_type
                     {SpinLockTraits::minSpins(), SpinLockTraits::maxSpins()});
         }
-        else {
+        else
+        {
             //Generate a number below min and add it to the min.
             numSpins() = SpinLockTraits::minSpins() +
                     distribution(gen, Distribution::param_type
                     {0, SpinLockTraits::minSpins()});
         }
     }
-    else if (numSpins() < SpinLockTraits::maxSpins()) {
-        if (SpinLockTraits::backoffPolicy() == SpinLockTraits::BackoffPolicy::Linear) {
+    else if (numSpins() < SpinLockTraits::maxSpins())
+    {
+        if (SpinLockTraits::backoffPolicy() == SpinLockTraits::BackoffPolicy::Linear)
+        {
             numSpins() += numSpins();
-        } else if (SpinLockTraits::backoffPolicy() == SpinLockTraits::BackoffPolicy::Exponential) {
+        }
+        else if (SpinLockTraits::backoffPolicy() == SpinLockTraits::BackoffPolicy::Exponential)
+        {
             numSpins() *= 2;
-        } else if (SpinLockTraits::backoffPolicy() == SpinLockTraits::BackoffPolicy::Random) {
+        }
+        else if (SpinLockTraits::backoffPolicy() == SpinLockTraits::BackoffPolicy::Random)
+        {
             //Generate a new value each time
             numSpins() = distribution(gen, Distribution::param_type
                     {SpinLockTraits::minSpins(), SpinLockTraits::maxSpins()});
         }
         //Check that we don't exceed max spins
-        if (numSpins() > SpinLockTraits::maxSpins()) {
+        if (numSpins() > SpinLockTraits::maxSpins())
+        {
             numSpins() = SpinLockTraits::maxSpins();
         }
     }
     //Spin
-    for (size_t i = 0; i < numSpins(); ++i) {
+    for (size_t i = 0; i < numSpins(); ++i)
+    {
         pauseCPU();
     }
 }
@@ -173,12 +187,15 @@ void SpinLockUtil::spinWait(std::atomic_int& flag,
                             int spinValue)
 {
     size_t numIters = 0;
-    while (flag.load(std::memory_order_relaxed) == spinValue) {
-        if (numIters < SpinLockTraits::maxSpins()) {
+    while (flag.load(std::memory_order_relaxed) == spinValue)
+    {
+        if (numIters < SpinLockTraits::maxSpins())
+        {
             ++numIters;
             pauseCPU();
         }
-        else {
+        else
+        {
             //Yield or sleep the thread instead of spinning
             yieldOrSleep();
         }

diff --git a/tests/quantum_spinlocks_tests.cpp b/tests/quantum_spinlocks_tests.cpp
@@ -27,40 +27,42 @@ using namespace Bloomberg::quantum;
 #else
     int spins = 1000000;
 #endif
+    int val = 0;
+    int numThreads = 20;
+    int numLockAcquires = 100;
 
-SpinLock exclusiveLock;
-
-void runnable() {
+void runnable(SpinLock* exclusiveLock) {
     int locksTaken = 0;
-    while (locksTaken < 100) {
-        exclusiveLock.lock();
+    while (locksTaken < numLockAcquires) {
+        exclusiveLock->lock();
         locksTaken++;
+        val++;
         std::this_thread::sleep_for(std::chrono::microseconds(500));
-        exclusiveLock.unlock();
+        exclusiveLock->unlock();
     }
 }
 
-void runThreads()
+void runThreads(int num)
 {
+    SpinLock exclusiveLock;
     //Create 50 threads
     exclusiveLock.lock(); //lock it so that all threads block
-    int numThreads = 50;
     std::vector<std::shared_ptr<std::thread>> threads;
     auto start = std::chrono::high_resolution_clock::now();
     for (int i = 0; i < numThreads; ++i) {
-        threads.push_back(std::make_shared<std::thread>(runnable));
+        threads.push_back(std::make_shared<std::thread>(runnable, &exclusiveLock));
     }
     exclusiveLock.unlock(); //unlock to start contention
     for (auto& t : threads) {
         t->join();
     }
     auto end = std::chrono::high_resolution_clock::now();
-    std::cout << "total time: "
+    std::cout << "Total spin time " << num << ": "
             << std::chrono::duration_cast<std::chrono::milliseconds>(end-start).count()
             << "ms" << std::endl;
 }
 
-void setSettings(
+void spinlockSettings(
         size_t min,
         size_t max,
         std::chrono::microseconds sleepUs,
@@ -69,44 +71,20 @@ void setSettings(
         int enable)
 {
     if (enable == -1 || enable == num) {
+        val = 0;
         SpinLockTraits::minSpins() = min;
         SpinLockTraits::maxSpins() = max;
         SpinLockTraits::numYieldsBeforeSleep() = numYields;
         SpinLockTraits::sleepDuration() = sleepUs;
-        runThreads();
+        runThreads(num);
+        EXPECT_EQ(numThreads*numLockAcquires, val);
     }
 }
 
-TEST(Spinlock, Test)
-{
-    int enable = 2; //enable all tests
-    int i = 0;
-    std::cout << "Default settings " << i << std::endl;
-    setSettings(500, 200000, std::chrono::microseconds(100), 2, i, enable); //0
-    std::cout << "Settings " << ++i << std::endl;
-    setSettings(500, 20000, std::chrono::microseconds(100), 3, i, enable); //1
-    std::cout << "Settings " << ++i << std::endl;
-    setSettings(100, 5000, std::chrono::microseconds(200), 3, i, enable); //2
-    std::cout << "Settings " << ++i << std::endl;
-    setSettings(500, 200000, std::chrono::microseconds(500), 2, i, enable); //3
-    std::cout << "Settings " << ++i << std::endl;
-    setSettings(500, 200000, std::chrono::microseconds(1000), 2, i, enable); //4
-    std::cout << "Settings " << ++i << std::endl;
-    setSettings(500, 200000, std::chrono::microseconds(100), 5, i, enable); //5
-    std::cout << "Settings " << ++i << std::endl;
-    setSettings(500, 200000, std::chrono::microseconds(100), 200, i, enable); //6
-    std::cout << "Settings " << ++i << std::endl;
-    setSettings(1000, 200000, std::chrono::microseconds(100), 2, i, enable); //7
-    std::cout << "Settings " << ++i << std::endl;
-    setSettings(5000, 200000, std::chrono::microseconds(100), 2, i, enable); //8
-    std::cout << "Settings " << ++i << std::endl;
-    setSettings(0, 0, std::chrono::microseconds(10), 20000, i, enable); //9
-}
-
 TEST(Spinlock, Spinlock)
 {
     int num = spins;
-    int val = 0;
+    val = 0;
     SpinLock spin;
     std::thread t1([&, num]() mutable {
         while (num--) {
@@ -125,6 +103,20 @@ TEST(Spinlock, Spinlock)
     EXPECT_EQ(0, val);
 }
 
+TEST(Spinlock, HighContention)
+{
+    val = 0;
+    int enable = -1;
+    int i = 0;
+    spinlockSettings(500, 10000, std::chrono::microseconds(100), 2, i++, enable); //0
+    spinlockSettings(0, 20000, std::chrono::microseconds(100), 3, i++, enable); //1
+    spinlockSettings(100, 5000, std::chrono::microseconds(200), 3, i++, enable); //2
+    spinlockSettings(500, 200000, std::chrono::microseconds(0), 5, i++, enable); //3
+    spinlockSettings(500, 20000, std::chrono::microseconds(1000), 0, i++, enable); //4
+    spinlockSettings(500, 2000, std::chrono::microseconds(0), 0, i++, enable); //5
+    spinlockSettings(0, 0, std::chrono::microseconds(10), 2000, i++, enable); //6
+}
+
 TEST(ReadWriteSpinLock, LockReadMultipleTimes)
 {
     ReadWriteSpinLock spin;