SERVER-16096: Use explicit masking in ResourceId instead of union/bitfields

Also pass ResourceId by value instead of by reference and use static const for
_numLockBuckets to avoid division.

8 files changed, 78 insertions, 72 deletions

diff --git a/src/mongo/db/concurrency/lock_mgr_defs.h b/src/mongo/db/concurrency/lock_mgr_defs.h
index 5c8c0992acc..a052e902b1c 100644
--- a/src/mongo/db/concurrency/lock_mgr_defs.h
+++ b/src/mongo/db/concurrency/lock_mgr_defs.h
@@ -29,6 +29,7 @@
 #pragma once
 
 #include <boost/static_assert.hpp>
+#include <boost/cstdint.hpp>
 #include <string>
 
 #include "mongo/base/string_data.h"
@@ -144,41 +145,37 @@ namespace mongo {
         ResourceTypesCount
     };
 
-    // We only use 3 bits for the resource type in the ResourceId hash
-    BOOST_STATIC_ASSERT(ResourceTypesCount <= 8);
-
     /**
      * Returns a human-readable name for the specified resource type.
      */
     const char* resourceTypeName(ResourceType resourceType);
 
-
     /**
      * Uniquely identifies a lockable resource.
      */
     class ResourceId {
+        // We only use 3 bits for the resource type in the ResourceId hash
+        enum {resourceTypeBits = 3};
+        BOOST_STATIC_ASSERT(ResourceTypesCount <= (1 << resourceTypeBits));
+
     public:
         ResourceId() : _fullHash(0) { }
         ResourceId(ResourceType type, const StringData& ns);
         ResourceId(ResourceType type, const std::string& ns);
         ResourceId(ResourceType type, uint64_t hashId);
 
-        bool isValid() const { return _type != RESOURCE_INVALID; }
+        bool isValid() const { return getType() != RESOURCE_INVALID; }
 
         operator uint64_t() const {
             return _fullHash;
         }
 
-        bool operator== (const ResourceId& other) const {
-            return _fullHash == other._fullHash;
-        }
-
         ResourceType getType() const {
-            return static_cast<ResourceType>(_type);
+            return static_cast<ResourceType>(_fullHash >> (64 - resourceTypeBits));
         }
 
         uint64_t getHashId() const {
-            return _hashId;
+            return _fullHash & (UINT64_MAX >> resourceTypeBits);
         }
 
         std::string toString() const;
@@ -186,27 +183,24 @@ namespace mongo {
     private:
 
         /**
-         * 64-bit hash of the resource
+         * The top 'resourceTypeBits' bits of '_fullHash' represent the resource type,
+         * while the remaining bits contain the bottom bits of the hashId. This avoids false
+         * conflicts between resources of different types, which is necessary to prevent deadlocks.
          */
-        union {
-            struct {
-                uint64_t     _type   : 3;
-                uint64_t     _hashId : 61;
-            };
+        uint64_t _fullHash;
 
-            uint64_t _fullHash;
-        };
+        static uint64_t fullHash(ResourceType type, uint64_t hashId);
 
 #ifdef _DEBUG
-        // Keep the complete namespace name for debugging purposes (TODO: this will be removed once
-        // we are confident in the robustness of the lock manager).
+        // Keep the complete namespace name for debugging purposes (TODO: this will be
+        // removed once we are confident in the robustness of the lock manager).
         std::string _nsCopy;
 #endif
     };
 
 #ifndef _DEBUG
-    // Treat the resource ids as 64-bit integers in release mode in order to ensure we do not
-    // spend too much time doing comparisons for hashing.
+    // Treat the resource ids as 64-bit integers in release mode in order to ensure we do
+    // not spend too much time doing comparisons for hashing.
     BOOST_STATIC_ASSERT(sizeof(ResourceId) == sizeof(uint64_t));
 #endif
 
diff --git a/src/mongo/db/concurrency/lock_mgr_new.cpp b/src/mongo/db/concurrency/lock_mgr_new.cpp
index 3940e1a90a9..518a05df4f3 100644
--- a/src/mongo/db/concurrency/lock_mgr_new.cpp
+++ b/src/mongo/db/concurrency/lock_mgr_new.cpp
@@ -230,7 +230,6 @@ namespace {
         // Id of the resource which this lock protects
         ResourceId resourceId;
 
-
         //
         // Granted queue
         //
@@ -363,9 +362,10 @@ namespace {
     // LockManager
     //
 
+    //  Have more buckets than CPUs to reduce contention on lock and caches
+    const unsigned LockManager::_numLockBuckets(128);
+
     LockManager::LockManager() {
-        //  Have more buckets than CPUs to reduce contention on lock and caches
-        _numLockBuckets = 128;
         _lockBuckets = new LockBucket[_numLockBuckets];
     }
 
@@ -705,7 +705,7 @@ namespace {
         invariant((lock->conflictModes == 0) ^ (lock->conflictList._front != NULL));
     }
 
-    LockManager::LockBucket* LockManager::_getBucket(const ResourceId& resId) const {
+    LockManager::LockBucket* LockManager::_getBucket(ResourceId resId) const {
         return &_lockBuckets[resId % _numLockBuckets];
     }
 
@@ -930,33 +930,32 @@ namespace {
 
     static const StringData::Hasher stringDataHashFunction = StringData::Hasher();
 
-    ResourceId::ResourceId(ResourceType type, const StringData& ns) {
-        _type = type;
-        _hashId = stringDataHashFunction(ns) % 0x1fffffffffffffffULL;
+    uint64_t ResourceId::fullHash(ResourceType type, uint64_t hashId) {
+        return (static_cast<uint64_t>(type) << (64 - resourceTypeBits))
+                + (hashId & (UINT64_MAX >> resourceTypeBits));
+    }
 
+    ResourceId::ResourceId(ResourceType type, const StringData& ns)
+        : _fullHash(fullHash(type, stringDataHashFunction(ns))) {
 #ifdef _DEBUG
         _nsCopy = ns.toString();
 #endif
     }
 
-    ResourceId::ResourceId(ResourceType type, const string& ns) {
-        _type = type;
-        _hashId = stringDataHashFunction(ns) % 0x1fffffffffffffffULL;
-
+    ResourceId::ResourceId(ResourceType type, const string& ns)
+        : _fullHash(fullHash(type, stringDataHashFunction(ns))) {
 #ifdef _DEBUG
         _nsCopy = ns;
 #endif
     }
 
-    ResourceId::ResourceId(ResourceType type, uint64_t hashId) {
-        _type = type;
-        _hashId = hashId;
-    }
+    ResourceId::ResourceId(ResourceType type, uint64_t hashId)
+        : _fullHash(fullHash(type, hashId)) { }
 
     string ResourceId::toString() const {
         StringBuilder ss;
-        ss << "{" << _fullHash << ": " << resourceTypeName(static_cast<ResourceType>(_type))
-           << ", " << _hashId;
+        ss << "{" << _fullHash << ": " << resourceTypeName(getType())
+           << ", " << getHashId();
 
 #ifdef _DEBUG
         ss << ", " << _nsCopy;
diff --git a/src/mongo/db/concurrency/lock_mgr_new.h b/src/mongo/db/concurrency/lock_mgr_new.h
index 03b15ab2ec3..325fae93e49 100644
--- a/src/mongo/db/concurrency/lock_mgr_new.h
+++ b/src/mongo/db/concurrency/lock_mgr_new.h
@@ -74,7 +74,7 @@ namespace mongo {
          * @resId ResourceId for which a lock operation was previously called.
          * @result Outcome of the lock operation.
          */
-        virtual void notify(const ResourceId& resId, LockResult result) = 0;
+        virtual void notify(ResourceId resId, LockResult result) = 0;
     };
 
 
@@ -267,7 +267,7 @@ namespace mongo {
          * Retrieves the bucket in which the particular resource must reside. There is no need to
          * hold a lock when calling this function.
          */
-        LockBucket* _getBucket(const ResourceId& resId) const;
+        LockBucket* _getBucket(ResourceId resId) const;
 
         /**
          * Prints the contents of a bucket to the log.
@@ -287,8 +287,7 @@ namespace mongo {
          */
         void _onLockModeChanged(LockHead* lock, bool checkConflictQueue);
 
-
-        unsigned _numLockBuckets;
+        static const unsigned _numLockBuckets;
         LockBucket* _lockBuckets;
     };
 
@@ -351,7 +350,7 @@ namespace mongo {
         typedef std::vector<LockerId> ConflictingOwnersList;
 
         struct Edges {
-            Edges(const ResourceId& resId) : resId(resId) { }
+            explicit Edges(ResourceId resId) : resId(resId) { }
 
             // Resource id indicating the lock node
             ResourceId resId;
diff --git a/src/mongo/db/concurrency/lock_mgr_new_test.cpp b/src/mongo/db/concurrency/lock_mgr_new_test.cpp
index 13abd20c8c6..75616bfb412 100644
--- a/src/mongo/db/concurrency/lock_mgr_new_test.cpp
+++ b/src/mongo/db/concurrency/lock_mgr_new_test.cpp
@@ -59,6 +59,21 @@ namespace mongo {
         ASSERT_EQUALS(resIdString, resIdStringData);
     }
 
+    TEST(ResourceId, Masking) {
+        const ResourceType maxRes = static_cast<ResourceType>(ResourceTypesCount - 1);
+        const uint64_t maxHash = (1ULL<<61) - 1; //  Only 61 bits usable for hash
+        ResourceType resources[3] = {maxRes, RESOURCE_GLOBAL, RESOURCE_DOCUMENT};
+        uint64_t hashes[3] = {maxHash, maxHash / 3, maxHash / 3 * 2};
+
+        //  The test below verifies that types/hashes are stored/retrieved unchanged
+        for (int h = 0; h < 3; h++) {
+            for (int r = 0; r < 3; r++) {
+                ResourceId id(resources[r], hashes[h]);
+                ASSERT_EQUALS(id.getHashId(), hashes[h]);
+                ASSERT_EQUALS(id.getType(), resources[r]);
+            }
+        }
+    }
 
     //
     // LockManager
diff --git a/src/mongo/db/concurrency/lock_mgr_test_help.h b/src/mongo/db/concurrency/lock_mgr_test_help.h
index 7149dad01c3..507e827a352 100644
--- a/src/mongo/db/concurrency/lock_mgr_test_help.h
+++ b/src/mongo/db/concurrency/lock_mgr_test_help.h
@@ -35,8 +35,7 @@ namespace mongo {
 
     class LockerForTests : public LockerImpl<false> {
     public:
-
-        LockerForTests(LockerId lockerId) : LockerImpl<false>(lockerId) {
+        explicit LockerForTests(LockerId lockerId) : LockerImpl<false>(lockerId) {
             lockGlobal(MODE_S);
         }
 
@@ -52,7 +51,7 @@ namespace mongo {
 
         }
 
-        virtual void notify(const ResourceId& resId, LockResult result) {
+        virtual void notify(ResourceId resId, LockResult result) {
             numNotifies++;
             lastResId = resId;
             lastResult = result;
@@ -68,7 +67,7 @@ namespace mongo {
 
     struct LockRequestCombo : public LockRequest, TrackingLockGrantNotification {
     public:
-        LockRequestCombo(Locker* locker) {
+        explicit LockRequestCombo (Locker* locker) {
             initNew(locker, this);
         }
     };
diff --git a/src/mongo/db/concurrency/lock_state.cpp b/src/mongo/db/concurrency/lock_state.cpp
index ca204b68a36..43692a0503d 100644
--- a/src/mongo/db/concurrency/lock_state.cpp
+++ b/src/mongo/db/concurrency/lock_state.cpp
@@ -83,9 +83,9 @@ namespace mongo {
          * delay releases for exclusive locks (locks that are for write operations) in order to
          * ensure that the data they protect is committed successfully.
          */
-        bool shouldDelayUnlock(const ResourceId& resId, LockMode mode) {
-            // Global and flush lock are not used to protect transactional resources and as such, they
-            // need to be acquired and released when requested.
+        bool shouldDelayUnlock(ResourceId resId, LockMode mode) {
+            // Global and flush lock are not used to protect transactional resources and as
+            // such, they need to be acquired and released when requested.
             if (resId == resourceIdGlobal) {
                 return false;
             }
@@ -275,7 +275,7 @@ namespace mongo {
         return _result;
     }
 
-    void CondVarLockGrantNotification::notify(const ResourceId& resId, LockResult result) {
+    void CondVarLockGrantNotification::notify(ResourceId resId, LockResult result) {
         boost::unique_lock<boost::mutex> lock(_mutex);
         invariant(_result == LOCK_INVALID);
         _result = result;
@@ -420,7 +420,7 @@ namespace mongo {
     }
 
     template<bool IsForMMAPV1>
-    LockResult LockerImpl<IsForMMAPV1>::lock(const ResourceId& resId,
+    LockResult LockerImpl<IsForMMAPV1>::lock(ResourceId resId,
                                              LockMode mode,
                                              unsigned timeoutMs,
                                              bool checkDeadlock) {
@@ -500,19 +500,19 @@ namespace mongo {
     }
 
     template<bool IsForMMAPV1>
-    void LockerImpl<IsForMMAPV1>::downgrade(const ResourceId& resId, LockMode newMode) {
+    void LockerImpl<IsForMMAPV1>::downgrade(ResourceId resId, LockMode newMode) {
         LockRequestsMap::Iterator it = _requests.find(resId);
         globalLockManager.downgrade(it.objAddr(), newMode);
     }
 
     template<bool IsForMMAPV1>
-    bool LockerImpl<IsForMMAPV1>::unlock(const ResourceId& resId) {
+    bool LockerImpl<IsForMMAPV1>::unlock(ResourceId resId) {
         LockRequestsMap::Iterator it = _requests.find(resId);
         return _unlockImpl(it);
     }
 
     template<bool IsForMMAPV1>
-    LockMode LockerImpl<IsForMMAPV1>::getLockMode(const ResourceId& resId) const {
+    LockMode LockerImpl<IsForMMAPV1>::getLockMode(ResourceId resId) const {
         scoped_spinlock scopedLock(_lock);
 
         const LockRequestsMap::ConstIterator it = _requests.find(resId);
@@ -522,7 +522,7 @@ namespace mongo {
     }
 
     template<bool IsForMMAPV1>
-    bool LockerImpl<IsForMMAPV1>::isLockHeldForMode(const ResourceId& resId, LockMode mode) const {
+    bool LockerImpl<IsForMMAPV1>::isLockHeldForMode(ResourceId resId, LockMode mode) const {
         return isModeCovered(mode, getLockMode(resId));
     }
 
@@ -647,7 +647,7 @@ namespace mongo {
 
         // Next, the non-global locks.
         for (LockRequestsMap::Iterator it = _requests.begin(); !it.finished(); it.next()) {
-            const ResourceId& resId = it.key();
+            const ResourceId resId = it.key();
 
             // We don't support saving and restoring document-level locks.
             invariant(RESOURCE_DATABASE == resId.getType() ||
@@ -683,7 +683,7 @@ namespace mongo {
     }
 
     template<bool IsForMMAPV1>
-    LockResult LockerImpl<IsForMMAPV1>::lockImpl(const ResourceId& resId, LockMode mode) {
+    LockResult LockerImpl<IsForMMAPV1>::lockImpl(ResourceId resId, LockMode mode) {
         LockRequest* request;
         bool isNew = true;
 
diff --git a/src/mongo/db/concurrency/lock_state.h b/src/mongo/db/concurrency/lock_state.h
index c24f368c2b3..ed4da505122 100644
--- a/src/mongo/db/concurrency/lock_state.h
+++ b/src/mongo/db/concurrency/lock_state.h
@@ -59,7 +59,7 @@ namespace mongo {
 
     private:
 
-        virtual void notify(const ResourceId& resId, LockResult result);
+        virtual void notify(ResourceId resId, LockResult result);
 
         // These two go together to implement the conditional variable pattern.
         boost::mutex _mutex;
@@ -104,17 +104,17 @@ namespace mongo {
 
         virtual bool inAWriteUnitOfWork() const { return _wuowNestingLevel > 0; }
 
-        virtual LockResult lock(const ResourceId& resId,
+        virtual LockResult lock(ResourceId resId,
                                 LockMode mode, 
                                 unsigned timeoutMs = UINT_MAX,
                                 bool checkDeadlock = false);
 
-        virtual void downgrade(const ResourceId& resId, LockMode newMode);
+        virtual void downgrade(ResourceId resId, LockMode newMode);
 
-        virtual bool unlock(const ResourceId& resId);
+        virtual bool unlock(ResourceId resId);
 
-        virtual LockMode getLockMode(const ResourceId& resId) const;
-        virtual bool isLockHeldForMode(const ResourceId& resId, LockMode mode) const;
+        virtual LockMode getLockMode(ResourceId resId) const;
+        virtual bool isLockHeldForMode(ResourceId resId, LockMode mode) const;
         virtual bool isDbLockedForMode(const StringData& dbName, LockMode mode) const;
         virtual bool isCollectionLockedForMode(const StringData& ns, LockMode mode) const;
 
@@ -133,7 +133,7 @@ namespace mongo {
          * NOTE: Must only be used to implement the actual lock call and for unit tests, because
          * it skips any internal consistency checks.
          */
-        LockResult lockImpl(const ResourceId& resId, LockMode mode);
+        LockResult lockImpl(ResourceId resId, LockMode mode);
 
     private:
 
diff --git a/src/mongo/db/concurrency/locker.h b/src/mongo/db/concurrency/locker.h
index c50e7eddcd3..db684c97f9c 100644
--- a/src/mongo/db/concurrency/locker.h
+++ b/src/mongo/db/concurrency/locker.h
@@ -128,7 +128,7 @@ namespace mongo {
          *
          * @return All LockResults except for LOCK_WAITING, because it blocks.
          */
-        virtual LockResult lock(const ResourceId& resId,
+        virtual LockResult lock(ResourceId resId,
                                 LockMode mode,
                                 unsigned timeoutMs = UINT_MAX,
                                 bool checkDeadlock = false) = 0;
@@ -136,7 +136,7 @@ namespace mongo {
         /**
          * Downgrades the specified resource's lock mode without changing the reference count.
          */
-        virtual void downgrade(const ResourceId& resId, LockMode newMode) = 0;
+        virtual void downgrade(ResourceId resId, LockMode newMode) = 0;
 
         /**
          * Releases a lock previously acquired through a lock call. It is an error to try to
@@ -145,7 +145,7 @@ namespace mongo {
          * @return true if the lock was actually released; false if only the reference count was 
          *              decremented, but the lock is still held.
          */
-        virtual bool unlock(const ResourceId& resId) = 0;
+        virtual bool unlock(ResourceId resId) = 0;
 
         /**
          * Retrieves the mode in which a lock is held or checks whether the lock held for a
@@ -154,8 +154,8 @@ namespace mongo {
          * For example isLockHeldForMode will return true for MODE_S, if MODE_X is already held,
          * because MODE_X covers MODE_S.
          */
-        virtual LockMode getLockMode(const ResourceId& resId) const = 0;
-        virtual bool isLockHeldForMode(const ResourceId& resId, LockMode mode) const = 0;
+        virtual LockMode getLockMode(ResourceId resId) const = 0;
+        virtual bool isLockHeldForMode(ResourceId resId, LockMode mode) const = 0;
 
         // These are shortcut methods for the above calls. They however check that the entire
         // hierarchy is properly locked and because of this they are very expensive to call.