Clang: Improve locking of string cache

The string cache is only very seldom written but very often read. To
improve thread scaling it is faster to lock it only for write operations.
So we use a shared mutex which is locked in shared mode for read
operations and locked exclusively for write operations.

Change-Id: I7dfd4a02c5484683e4d0becd39269c0146126a96
Reviewed-by: Tim Jenssen <tim.jenssen@qt.io>

src/libs/clangsupport/filepathcache.h

@@ -67,12 +67,12 @@ class FilePathCache final : private FilePathCacheBase
 {
     using DirectoryPathCache = StringCache<Utils::PathString,
                                            int,
-                                           std::mutex,
+                                           SharedMutex,
                                            decltype(&Utils::reverseCompare),
                                            Utils::reverseCompare>;
     using FileNameCache = StringCache<Utils::SmallString,
                                       int,
-                                      std::mutex,
+                                      SharedMutex,
                                       decltype(&Utils::compare),
                                       Utils::compare>;
 public:

src/libs/clangsupport/stringcache.h

@@ -32,8 +32,10 @@
 #include <utils/smallstringview.h>
 #include <utils/smallstringfwd.h>
 
+#include <QReadWriteLock>
+
 #include <algorithm>
-#include <mutex>
+#include <shared_mutex>
 #include <vector>
 
 namespace ClangBackEnd {
@@ -54,6 +56,39 @@ public:
     NonLockingMutex& operator=(const NonLockingMutex&) = delete;
     void lock() {}
     void unlock() {}
+    void lock_shared() {}
+    void unlock_shared() {}
+};
+
+class SharedMutex
+{
+public:
+    SharedMutex() = default;
+
+    SharedMutex(const SharedMutex&) = delete;
+    SharedMutex& operator=(const SharedMutex&) = delete;
+
+    void lock()
+    {
+        m_mutex.lockForWrite();
+    }
+
+    void unlock()
+    {
+        m_mutex.unlock();
+    }
+
+    void lock_shared()
+    {
+        m_mutex.lockForRead();
+    }
+
+    void unlock_shared()
+    {
+        m_mutex.unlock();
+    }
+private:
+    QReadWriteLock m_mutex;
 };
 
 template <typename StringType, typename IndexType>
@@ -122,23 +157,34 @@ public:
 
     IndexType stringId(Utils::SmallStringView stringView)
     {
-        std::lock_guard<Mutex> lock(m_mutex);
+        std::shared_lock<Mutex> sharedLock(m_mutex);
+        Found found = find(stringView);
 
-        return ungardedStringId(stringView);
+        if (found.wasFound)
+            return found.iterator->id;
+
+        sharedLock.unlock();
+        std::lock_guard<Mutex> exclusiveLock(m_mutex);
+
+        found = find(stringView);
+        if (!found.wasFound) {
+            IndexType index = insertString(found.iterator, stringView, IndexType(m_indices.size()));
+            found.iterator = m_strings.begin() + index;;
+        }
+
+        return found.iterator->id;
     }
 
     template <typename Container>
     std::vector<IndexType> stringIds(const Container &strings)
     {
-        std::lock_guard<Mutex> lock(m_mutex);
-
         std::vector<IndexType> ids;
         ids.reserve(strings.size());
 
         std::transform(strings.begin(),
                        strings.end(),
                        std::back_inserter(ids),
-                       [&] (const auto &string) { return this->ungardedStringId(string); });
+                       [&] (const auto &string) { return this->stringId(string); });
 
         return ids;
     }
@@ -150,7 +196,7 @@ public:
 
     StringType string(IndexType id) const
     {
-        std::lock_guard<Mutex> lock(m_mutex);
+        std::shared_lock<Mutex> sharedLock(m_mutex);
 
         return m_strings.at(m_indices.at(id)).string;
     }
@@ -158,28 +204,25 @@ public:
     template<typename Function>
     StringType string(IndexType id, Function storageFunction)
     {
-        std::lock_guard<Mutex> lock(m_mutex);
+        std::shared_lock<Mutex> sharedLock(m_mutex);
 
-        IndexType index;
+        if (IndexType(m_indices.size()) > id && m_indices.at(id) >= 0)
+            return m_strings.at(m_indices.at(id)).string;
 
-        if (IndexType(m_indices.size()) <= id) {
-            StringType string{storageFunction(id)};
-            index = insertString(find(string).iterator, string, id);
-        } else {
-            index = m_indices.at(id);
 
-            if (index < 0) {
-                StringType string{storageFunction(id)};
-                index = insertString(find(string).iterator, string, id);
-            }
-        }
+        sharedLock.unlock();
+        std::lock_guard<Mutex> exclusiveLock(m_mutex);
+        IndexType index;
+
+        StringType string{storageFunction(id)};
+        index = insertString(find(string).iterator, string, id);
 
-        return m_strings.at(index).string;
+        return m_strings[index].string;
     }
 
     std::vector<StringType> strings(const std::vector<IndexType> &ids) const
     {
-        std::lock_guard<Mutex> lock(m_mutex);
+        std::shared_lock<Mutex> sharedLock(m_mutex);
 
         std::vector<StringType> strings;
         strings.reserve(ids.size());
@@ -200,12 +243,21 @@ public:
     template<typename Function>
     IndexType stringId(Utils::SmallStringView stringView, Function storageFunction)
     {
-        std::lock_guard<Mutex> lock(m_mutex);
+        std::shared_lock<Mutex> sharedLock(m_mutex);
 
         Found found = find(stringView);
 
-        if (!found.wasFound)
-            insertString(found.iterator, stringView, storageFunction(stringView));
+        if (found.wasFound)
+            return found.iterator->id;
+
+        sharedLock.unlock();
+        std::lock_guard<Mutex> exclusiveLock(m_mutex);
+
+        found = find(stringView);
+        if (!found.wasFound) {
+            IndexType index = insertString(found.iterator, stringView, storageFunction(stringView));
+            found.iterator = m_strings.begin() + index;
+        }
 
         return found.iterator->id;
     }
@@ -216,16 +268,6 @@ public:
     }
 
 private:
-    IndexType ungardedStringId(Utils::SmallStringView stringView)
-    {
-        Found found = find(stringView);
-
-        if (!found.wasFound)
-            insertString(found.iterator, stringView, IndexType(m_indices.size()));
-
-        return found.iterator->id;
-    }
-
     Found find(Utils::SmallStringView stringView)
     {
         return findInSorted(m_strings.cbegin(), m_strings.cend(), stringView, compare);

tests/unit/unittest/mockmutex.h

@@ -34,4 +34,6 @@ public:
 
     MOCK_METHOD0(lock, void ());
     MOCK_METHOD0(unlock, void ());
+    MOCK_METHOD0(lock_shared, void ());
+    MOCK_METHOD0(unlock_shared, void ());
 };

tests/unit/unittest/stringcache-test.cpp

@@ -329,48 +329,110 @@ TEST_F(StringCache, FindInSortedFifeReverse)
     ASSERT_TRUE(found.wasFound);
 }
 
-TEST_F(StringCache, StringIdIsLocked)
+TEST_F(StringCache, StringIdIsReadAndWriteLockedForUnknownEntry)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockMutex, lock_shared());
+    EXPECT_CALL(mockMutex, unlock_shared());
     EXPECT_CALL(mockMutex, lock());
     EXPECT_CALL(mockMutex, unlock());
 
     cache.stringId("foo");
 }
 
-TEST_F(StringCache, StringIdsIsLocked)
+TEST_F(StringCache, StringIdWithStorageFunctionIsReadAndWriteLockedForUnknownEntry)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockMutex, lock_shared());
+    EXPECT_CALL(mockMutex, unlock_shared());
     EXPECT_CALL(mockMutex, lock());
+    EXPECT_CALL(mockStorage, fetchDirectoryId(Eq("foo")));
     EXPECT_CALL(mockMutex, unlock());
 
+    cache.stringId("foo", mockStorageFetchDirectyId);
+}
+
+TEST_F(StringCache, StringIdWithStorageFunctionIsReadLockedForKnownEntry)
+{
+    InSequence s;
+    cache.stringId("foo", mockStorageFetchDirectyId);
+
+    EXPECT_CALL(mockMutex, lock_shared());
+    EXPECT_CALL(mockMutex, unlock_shared());
+    EXPECT_CALL(mockMutex, lock()).Times(0);
+    EXPECT_CALL(mockStorage, fetchDirectoryId(Eq("foo"))).Times(0);
+    EXPECT_CALL(mockMutex, unlock()).Times(0);
+
+    cache.stringId("foo", mockStorageFetchDirectyId);
+}
+
+TEST_F(StringCache, StringIdIsReadLockedForKnownEntry)
+{
+    cache.stringId("foo");
+
+    EXPECT_CALL(mockMutex, lock_shared());
+    EXPECT_CALL(mockMutex, unlock_shared());
+    EXPECT_CALL(mockMutex, lock()).Times(0);
+    EXPECT_CALL(mockMutex, unlock()).Times(0);
+
+    cache.stringId("foo");
+}
+
+TEST_F(StringCache, StringIdsIsLocked)
+{
+    EXPECT_CALL(mockMutex, lock_shared());
+    EXPECT_CALL(mockMutex, unlock_shared());
+
     cache.stringIds({"foo"});
 }
 
 TEST_F(StringCache, StringIsLocked)
 {
-    cache.stringId("foo");
+    auto id = cache.stringId("foo");
 
-    EXPECT_CALL(mockMutex, lock());
-    EXPECT_CALL(mockMutex, unlock());
+    EXPECT_CALL(mockMutex, lock_shared());
+    EXPECT_CALL(mockMutex, unlock_shared());
 
-    cache.string(0);
+    cache.string(id);
 }
 
 TEST_F(StringCache, StringsIsLocked)
 {
-    cache.stringId("foo");
+    auto ids = cache.stringIds({"foo", "bar"});
 
-    EXPECT_CALL(mockMutex, lock());
-    EXPECT_CALL(mockMutex, unlock());
+    EXPECT_CALL(mockMutex, lock_shared());
+    EXPECT_CALL(mockMutex, unlock_shared());
 
-    cache.strings({0});
+    cache.strings(ids);
 }
 
-TEST_F(StringCache, StringIdWithStorageFunctionIsLocked)
+TEST_F(StringCache, StringWithStorageFunctionIsReadAndWriteLockedForUnknownId)
 {
+    InSequence s;
+
+    EXPECT_CALL(mockMutex, lock_shared());
+    EXPECT_CALL(mockMutex, unlock_shared());
     EXPECT_CALL(mockMutex, lock());
+    EXPECT_CALL(mockStorage, fetchDirectoryPath(Eq(41)));
     EXPECT_CALL(mockMutex, unlock());
 
-    cache.stringId("foo", mockStorageFetchDirectyId);
+    cache.string(41, mockStorageFetchDirectyPath);
+}
+
+TEST_F(StringCache, StringWithStorageFunctionIsReadLockedForKnownId)
+{
+    InSequence s;
+    cache.string(41, mockStorageFetchDirectyPath);
+
+    EXPECT_CALL(mockMutex, lock_shared());
+    EXPECT_CALL(mockMutex, unlock_shared());
+    EXPECT_CALL(mockMutex, lock()).Times(0);
+    EXPECT_CALL(mockStorage, fetchDirectoryPath(Eq(41))).Times(0);
+    EXPECT_CALL(mockMutex, unlock()).Times(0);
+
+    cache.string(41, mockStorageFetchDirectyPath);
 }
 
 TEST_F(StringCache, StringIdWithStorageFunctionWhichHasNoEntryIsCallingStorageFunction)