SERVER-40004 Modified compareAndSwap to return boolean

9 files changed, 61 insertions, 41 deletions

diff --git a/src/mongo/db/operation_context.cpp b/src/mongo/db/operation_context.cpp
index 706d07ef2ca..4690f4e38a2 100644
--- a/src/mongo/db/operation_context.cpp
+++ b/src/mongo/db/operation_context.cpp
@@ -317,7 +317,7 @@ void OperationContext::markKilled(ErrorCodes::Error killCode) {
         log() << "operation was interrupted because a client disconnected";
     }
 
-    if (_killCode.compareAndSwap(ErrorCodes::OK, killCode) == ErrorCodes::OK) {
+    if (auto status = ErrorCodes::OK; _killCode.compareAndSwap(&status, killCode)) {
         _baton->notify();
     }
 }
diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_session_cache.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_session_cache.cpp
index fb33870e114..fb41f7c2b3b 100644
--- a/src/mongo/db/storage/wiredtiger/wiredtiger_session_cache.cpp
+++ b/src/mongo/db/storage/wiredtiger/wiredtiger_session_cache.cpp
@@ -215,16 +215,9 @@ WiredTigerSessionCache::~WiredTigerSessionCache() {
 }
 
 void WiredTigerSessionCache::shuttingDown() {
-    uint32_t actual = _shuttingDown.load();
-    uint32_t expected;
-
     // Try to atomically set _shuttingDown flag, but just return if another thread was first.
-    do {
-        expected = actual;
-        actual = _shuttingDown.compareAndSwap(expected, expected | kShuttingDownMask);
-        if (actual & kShuttingDownMask)
-            return;
-    } while (actual != expected);
+    if (_shuttingDown.fetchAndBitOr(kShuttingDownMask) & kShuttingDownMask)
+        return;
 
     // Spin as long as there are threads in releaseSession
     while (_shuttingDown.load() != kShuttingDownMask) {
diff --git a/src/mongo/platform/atomic_word.h b/src/mongo/platform/atomic_word.h
index 30a597edcd1..d97854c1d5e 100644
--- a/src/mongo/platform/atomic_word.h
+++ b/src/mongo/platform/atomic_word.h
@@ -114,13 +114,14 @@ public:
     /**
      * Atomic compare and swap.
      *
-     * If this value equals "expected", sets this to "newValue".
-     * Always returns the original of this.
+     * If this value equals the value at "expected", sets this value to "newValue".
+     * Otherwise, sets the storage at "expected" to this value.
+     *
+     * Returns true if swap successful, false otherwise
      */
-    WordType compareAndSwap(WordType expected, WordType newValue) {
+    bool compareAndSwap(WordType* expected, WordType newValue) {
         // NOTE: Subtle: compare_exchange mutates its first argument.
-        _value.compare_exchange_strong(expected, newValue);
-        return expected;
+        return _value.compare_exchange_strong(*expected, newValue);
     }
 
 protected:
diff --git a/src/mongo/platform/atomic_word_test.cpp b/src/mongo/platform/atomic_word_test.cpp
index 212d34f2d13..7f55057a7c6 100644
--- a/src/mongo/platform/atomic_word_test.cpp
+++ b/src/mongo/platform/atomic_word_test.cpp
@@ -38,6 +38,26 @@
 namespace mongo {
 namespace {
 
+/*
+ * Modified CAS for AtomicWord to return boolean based on whether or not the swap occurred.
+ * This helper function mimics the old implementation, which returned the original value of
+ * expected.
+ */
+template <typename WordType>
+WordType testAtomicWordCompareAndSwap(AtomicWord<WordType>& word,
+                                      WordType expected,
+                                      WordType desired) {
+    auto prevWord = word.loadRelaxed();
+    auto didSwap = word.compareAndSwap(&expected, desired);
+    ASSERT_EQUALS(expected, prevWord);
+    if (didSwap) {
+        ASSERT_EQUALS(word.load(), desired);
+    } else {
+        ASSERT_EQUALS(word.load(), prevWord);
+    }
+    return expected;
+}
+
 template <typename _AtomicWordType>
 void testAtomicWordBasicOperations() {
     typedef typename _AtomicWordType::WordType WordType;
@@ -51,16 +71,16 @@ void testAtomicWordBasicOperations() {
     ASSERT_EQUALS(WordType(1), w.swap(2));
     ASSERT_EQUALS(WordType(2), w.load());
 
-    ASSERT_EQUALS(WordType(2), w.compareAndSwap(0, 1));
+    ASSERT_EQUALS(WordType(2), testAtomicWordCompareAndSwap<WordType>(w, 0, 1));
     ASSERT_EQUALS(WordType(2), w.load());
-    ASSERT_EQUALS(WordType(2), w.compareAndSwap(2, 1));
+    ASSERT_EQUALS(WordType(2), testAtomicWordCompareAndSwap<WordType>(w, 2, 1));
     ASSERT_EQUALS(WordType(1), w.load());
 
     ASSERT_EQUALS(WordType(1), w.fetchAndAdd(14));
     ASSERT_EQUALS(WordType(17), w.addAndFetch(2));
     ASSERT_EQUALS(WordType(16), w.subtractAndFetch(1));
     ASSERT_EQUALS(WordType(16), w.fetchAndSubtract(1));
-    ASSERT_EQUALS(WordType(15), w.compareAndSwap(15, 0));
+    ASSERT_EQUALS(WordType(15), testAtomicWordCompareAndSwap<WordType>(w, 15, 0));
     ASSERT_EQUALS(WordType(0), w.load());
 }
 
@@ -92,9 +112,9 @@ TEST(AtomicWordTests, BasicOperationsEnum) {
     MONGO_STATIC_ASSERT(sizeof(AtomicWord<TestEnum>) == sizeof(TestEnum));
     AtomicWord<TestEnum> w;
     ASSERT_EQUALS(E0, w.load());
-    ASSERT_EQUALS(E0, w.compareAndSwap(E0, E1));
+    ASSERT_EQUALS(E0, testAtomicWordCompareAndSwap(w, E0, E1));
     ASSERT_EQUALS(E1, w.load());
-    ASSERT_EQUALS(E1, w.compareAndSwap(E0, E2));
+    ASSERT_EQUALS(E1, testAtomicWordCompareAndSwap(w, E0, E2));
     ASSERT_EQUALS(E1, w.load());
 }
 
@@ -104,8 +124,9 @@ TEST(AtomicWordTests, BasicOperationsUnsigned32Bit) {
     testAtomicWordBitOperations<AtomicWord<unsigned>>();
 
     AtomicWord<unsigned> w(0xdeadbeef);
-    ASSERT_EQUALS(WordType(0xdeadbeef), w.compareAndSwap(0, 1));
-    ASSERT_EQUALS(WordType(0xdeadbeef), w.compareAndSwap(0xdeadbeef, 0xcafe1234));
+    ASSERT_EQUALS(WordType(0xdeadbeef), testAtomicWordCompareAndSwap<WordType>(w, 0, 1));
+    ASSERT_EQUALS(WordType(0xdeadbeef),
+                  testAtomicWordCompareAndSwap<WordType>(w, 0xdeadbeef, 0xcafe1234));
     ASSERT_EQUALS(WordType(0xcafe1234), w.fetchAndAdd(0xf000));
     ASSERT_EQUALS(WordType(0xcaff0234), w.swap(0));
     ASSERT_EQUALS(WordType(0), w.load());
@@ -117,9 +138,10 @@ TEST(AtomicWordTests, BasicOperationsUnsigned64Bit) {
     testAtomicWordBitOperations<AtomicWord<unsigned long long>>();
 
     AtomicWord<unsigned long long> w(0xdeadbeefcafe1234ULL);
-    ASSERT_EQUALS(WordType(0xdeadbeefcafe1234ULL), w.compareAndSwap(0, 1));
-    ASSERT_EQUALS(WordType(0xdeadbeefcafe1234ULL),
-                  w.compareAndSwap(0xdeadbeefcafe1234ULL, 0xfedcba9876543210ULL));
+    ASSERT_EQUALS(WordType(0xdeadbeefcafe1234ULL), testAtomicWordCompareAndSwap<WordType>(w, 0, 1));
+    ASSERT_EQUALS(
+        WordType(0xdeadbeefcafe1234ULL),
+        testAtomicWordCompareAndSwap<WordType>(w, 0xdeadbeefcafe1234ULL, 0xfedcba9876543210ULL));
     ASSERT_EQUALS(WordType(0xfedcba9876543210ULL), w.fetchAndAdd(0xf0000000ULL));
     ASSERT_EQUALS(WordType(0xfedcba9966543210ULL), w.swap(0));
     ASSERT_EQUALS(WordType(0), w.load());
@@ -130,8 +152,9 @@ TEST(AtomicWordTests, BasicOperationsSigned32Bit) {
     testAtomicWordBasicOperations<AtomicWord<int>>();
 
     AtomicWord<int> w(0xdeadbeef);
-    ASSERT_EQUALS(WordType(0xdeadbeef), w.compareAndSwap(0, 1));
-    ASSERT_EQUALS(WordType(0xdeadbeef), w.compareAndSwap(0xdeadbeef, 0xcafe1234));
+    ASSERT_EQUALS(WordType(0xdeadbeef), testAtomicWordCompareAndSwap<WordType>(w, 0, 1));
+    ASSERT_EQUALS(WordType(0xdeadbeef),
+                  testAtomicWordCompareAndSwap<WordType>(w, 0xdeadbeef, 0xcafe1234));
     ASSERT_EQUALS(WordType(0xcafe1234), w.fetchAndAdd(0xf000));
     ASSERT_EQUALS(WordType(0xcaff0234), w.swap(0));
     ASSERT_EQUALS(WordType(0), w.load());
@@ -142,9 +165,10 @@ TEST(AtomicWordTests, BasicOperationsSigned64Bit) {
     testAtomicWordBasicOperations<AtomicWord<long long>>();
 
     AtomicWord<long long> w(0xdeadbeefcafe1234ULL);
-    ASSERT_EQUALS(WordType(0xdeadbeefcafe1234LL), w.compareAndSwap(0, 1));
-    ASSERT_EQUALS(WordType(0xdeadbeefcafe1234LL),
-                  w.compareAndSwap(0xdeadbeefcafe1234LL, 0xfedcba9876543210LL));
+    ASSERT_EQUALS(WordType(0xdeadbeefcafe1234LL), testAtomicWordCompareAndSwap<WordType>(w, 0, 1));
+    ASSERT_EQUALS(
+        WordType(0xdeadbeefcafe1234LL),
+        testAtomicWordCompareAndSwap<WordType>(w, 0xdeadbeefcafe1234LL, 0xfedcba9876543210LL));
     ASSERT_EQUALS(WordType(0xfedcba9876543210LL), w.fetchAndAdd(0xf0000000LL));
     ASSERT_EQUALS(WordType(0xfedcba9966543210LL), w.swap(0));
     ASSERT_EQUALS(WordType(0), w.load());
@@ -163,13 +187,13 @@ TEST(AtomicWordTests, BasicOperationsFloat) {
     ASSERT_EQUALS(WordType(1), w.swap(2));
     ASSERT_EQUALS(WordType(2), w.load());
 
-    ASSERT_EQUALS(WordType(2), w.compareAndSwap(0, 1));
+    ASSERT_EQUALS(WordType(2), testAtomicWordCompareAndSwap<WordType>(w, 0, 1));
     ASSERT_EQUALS(WordType(2), w.load());
-    ASSERT_EQUALS(WordType(2), w.compareAndSwap(2, 1));
+    ASSERT_EQUALS(WordType(2), testAtomicWordCompareAndSwap<WordType>(w, 2, 1));
     ASSERT_EQUALS(WordType(1), w.load());
 
     w.store(15);
-    ASSERT_EQUALS(WordType(15), w.compareAndSwap(15, 0));
+    ASSERT_EQUALS(WordType(15), testAtomicWordCompareAndSwap<WordType>(w, 15, 0));
     ASSERT_EQUALS(WordType(0), w.load());
 }
 
diff --git a/src/mongo/transport/service_executor_adaptive.cpp b/src/mongo/transport/service_executor_adaptive.cpp
index 9b9d3e9c734..a62f057e7b3 100644
--- a/src/mongo/transport/service_executor_adaptive.cpp
+++ b/src/mongo/transport/service_executor_adaptive.cpp
@@ -602,8 +602,7 @@ void ServiceExecutorAdaptive::_workerThreadRoutine(
                 terminateThread = pctExecuting <= _config->idlePctThreshold();
             }
         } while (terminateThread &&
-                 _threadsRunning.compareAndSwap(runningThreads, runningThreads - 1) !=
-                     runningThreads);
+                 !_threadsRunning.compareAndSwap(&runningThreads, runningThreads - 1));
         if (terminateThread) {
             log() << "Thread was only executing tasks " << pctExecuting << "% over the last "
                   << runTime << ". Exiting thread.";
diff --git a/src/mongo/transport/service_state_machine.cpp b/src/mongo/transport/service_state_machine.cpp
index 8e751d9327c..c59f31ca362 100644
--- a/src/mongo/transport/service_state_machine.cpp
+++ b/src/mongo/transport/service_state_machine.cpp
@@ -176,7 +176,8 @@ class ServiceStateMachine::ThreadGuard {
 
 public:
     explicit ThreadGuard(ServiceStateMachine* ssm) : _ssm{ssm} {
-        auto owned = _ssm->_owned.compareAndSwap(Ownership::kUnowned, Ownership::kOwned);
+        auto owned = Ownership::kUnowned;
+        _ssm->_owned.compareAndSwap(&owned, Ownership::kOwned);
         if (owned == Ownership::kStatic) {
             dassert(haveClient());
             dassert(Client::getCurrent() == _ssm->_dbClientPtr);
diff --git a/src/mongo/transport/session.cpp b/src/mongo/transport/session.cpp
index c9036cbcabd..0bbc4bb5eba 100644
--- a/src/mongo/transport/session.cpp
+++ b/src/mongo/transport/session.cpp
@@ -62,7 +62,7 @@ void Session::mutateTags(const stdx::function<TagMask(TagMask)>& mutateFunc) {
 
         // Any change to the session tags automatically clears kPending status.
         newValue &= ~kPending;
-    } while (_tags.compareAndSwap(oldValue, newValue) != oldValue);
+    } while (!_tags.compareAndSwap(&oldValue, newValue));
 }
 
 Session::TagMask Session::getTags() const {
diff --git a/src/mongo/util/background_thread_clock_source.cpp b/src/mongo/util/background_thread_clock_source.cpp
index 1ca4eb74de9..e7612a66037 100644
--- a/src/mongo/util/background_thread_clock_source.cpp
+++ b/src/mongo/util/background_thread_clock_source.cpp
@@ -94,8 +94,10 @@ void BackgroundThreadClockSource::_updateClock() {
 // This will be called at most once per _granularity per thread. In common cases it will only be
 // called by a single thread per _granularity.
 void BackgroundThreadClockSource::_updateClockAndWakeTimerIfNeeded() {
+    uint8_t expected = kTimerWillPause;
+    _state.compareAndSwap(&expected, kReaderHasRead);
     // Try to go from TimerWillPause to ReaderHasRead.
-    if (_state.compareAndSwap(kTimerWillPause, kReaderHasRead) != kTimerPaused) {
+    if (expected != kTimerPaused) {
         // There are three possible states _state could have been in before this cas:
         //
         // kTimerWillPause - In this case, we've transitioned to kReaderHasRead, telling the timer
diff --git a/src/mongo/util/time_support.cpp b/src/mongo/util/time_support.cpp
index 90225bbbb43..f3c1fee7a33 100644
--- a/src/mongo/util/time_support.cpp
+++ b/src/mongo/util/time_support.cpp
@@ -66,8 +66,8 @@ namespace mongo {
 AtomicWord<long long> Date_t::lastNowVal;
 
 Date_t Date_t::now() {
-    int64_t curTime = curTimeMillis64();
-    int64_t oldLastNow = lastNowVal.loadRelaxed();
+    decltype(lastNowVal)::WordType curTime = curTimeMillis64();
+    auto oldLastNow = lastNowVal.loadRelaxed();
 
     // If curTime is different than old last now, unconditionally try to cas it to the new value.
     // This is an optimization to avoid performing stores for multiple clock reads in the same
@@ -80,7 +80,7 @@ Date_t Date_t::now() {
         // which case it's likely their time is also recent.  It's important that we don't loop so
         // that we avoid forcing time backwards if we have multiple callers at a millisecond
         // boundary.
-        lastNowVal.compareAndSwap(oldLastNow, curTime);
+        lastNowVal.compareAndSwap(&oldLastNow, curTime);
     }
 
     return fromMillisSinceEpoch(curTime);