Revert "Clang: Improve locking of string cache"

This reverts commit be939a80.
'shared_timed_mutex' is not available before Xcode 9.

Change-Id: I1ac6c2b3691d5b4f457c431e255629a526c48c3a
Reviewed-by: Eike Ziller <eike.ziller@qt.io>

src/libs/clangsupport/filepathcache.h

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

src/libs/clangsupport/stringcache.h

@@ -33,7 +33,7 @@
 #include <utils/smallstringfwd.h>
 
 #include <algorithm>
-#include <shared_mutex>
+#include <mutex>
 #include <vector>
 
 namespace ClangBackEnd {
@@ -54,8 +54,6 @@ public:
     NonLockingMutex& operator=(const NonLockingMutex&) = delete;
     void lock() {}
     void unlock() {}
-    void lock_shared() {}
-    void unlock_shared() {}
 };
 
 template <typename StringType, typename IndexType>
@@ -124,34 +122,23 @@ public:
 
     IndexType stringId(Utils::SmallStringView stringView)
     {
-        std::shared_lock<Mutex> sharedLock(m_mutex);
-        Found found = find(stringView);
-
-        if (found.wasFound)
-            return found.iterator->id;
-
-        sharedLock.unlock();
-        std::lock_guard<Mutex> exclusiveLock(m_mutex);
+        std::lock_guard<Mutex> lock(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;
+        return ungardedStringId(stringView);
     }
 
     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->stringId(string); });
+                       [&] (const auto &string) { return this->ungardedStringId(string); });
 
         return ids;
     }
@@ -163,7 +150,7 @@ public:
 
     StringType string(IndexType id) const
     {
-        std::shared_lock<Mutex> sharedLock(m_mutex);
+        std::lock_guard<Mutex> lock(m_mutex);
 
         return m_strings.at(m_indices.at(id)).string;
     }
@@ -171,25 +158,28 @@ public:
     template<typename Function>
     StringType string(IndexType id, Function storageFunction)
     {
-        std::shared_lock<Mutex> sharedLock(m_mutex);
-
-        if (IndexType(m_indices.size()) > id && m_indices.at(id) >= 0)
-            return m_strings.at(m_indices.at(id)).string;
-
+        std::lock_guard<Mutex> lock(m_mutex);
 
-        sharedLock.unlock();
-        std::lock_guard<Mutex> exclusiveLock(m_mutex);
         IndexType index;
 
-        StringType string{storageFunction(id)};
-        index = insertString(find(string).iterator, string, id);
+        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);
+            }
+        }
 
-        return m_strings[index].string;
+        return m_strings.at(index).string;
     }
 
     std::vector<StringType> strings(const std::vector<IndexType> &ids) const
     {
-        std::shared_lock<Mutex> sharedLock(m_mutex);
+        std::lock_guard<Mutex> lock(m_mutex);
 
         std::vector<StringType> strings;
         strings.reserve(ids.size());
@@ -210,21 +200,12 @@ public:
     template<typename Function>
     IndexType stringId(Utils::SmallStringView stringView, Function storageFunction)
     {
-        std::shared_lock<Mutex> sharedLock(m_mutex);
+        std::lock_guard<Mutex> lock(m_mutex);
 
         Found found = find(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;
-        }
+        if (!found.wasFound)
+            insertString(found.iterator, stringView, storageFunction(stringView));
 
         return found.iterator->id;
     }
@@ -235,6 +216,16 @@ 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,6 +34,4 @@ 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,110 +329,48 @@ TEST_F(StringCache, FindInSortedFifeReverse)
     ASSERT_TRUE(found.wasFound);
 }
 
-TEST_F(StringCache, StringIdIsReadAndWriteLockedForUnknownEntry)
+TEST_F(StringCache, StringIdIsLocked)
 {
-    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, StringIdWithStorageFunctionIsReadAndWriteLockedForUnknownEntry)
+TEST_F(StringCache, StringIdsIsLocked)
 {
-    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)
 {
-    auto id = cache.stringId("foo");
+    cache.stringId("foo");
 
-    EXPECT_CALL(mockMutex, lock_shared());
-    EXPECT_CALL(mockMutex, unlock_shared());
+    EXPECT_CALL(mockMutex, lock());
+    EXPECT_CALL(mockMutex, unlock());
 
-    cache.string(id);
+    cache.string(0);
 }
 
 TEST_F(StringCache, StringsIsLocked)
 {
-    auto ids = cache.stringIds({"foo", "bar"});
-
-    EXPECT_CALL(mockMutex, lock_shared());
-    EXPECT_CALL(mockMutex, unlock_shared());
-
-    cache.strings(ids);
-}
-
-TEST_F(StringCache, StringWithStorageFunctionIsReadAndWriteLockedForUnknownId)
-{
-    InSequence s;
+    cache.stringId("foo");
 
-    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.string(41, mockStorageFetchDirectyPath);
+    cache.strings({0});
 }
 
-TEST_F(StringCache, StringWithStorageFunctionIsReadLockedForKnownId)
+TEST_F(StringCache, StringIdWithStorageFunctionIsLocked)
 {
-    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);
+    EXPECT_CALL(mockMutex, lock());
+    EXPECT_CALL(mockMutex, unlock());
 
-    cache.string(41, mockStorageFetchDirectyPath);
+    cache.stringId("foo", mockStorageFetchDirectyId);
 }
 
 TEST_F(StringCache, StringIdWithStorageFunctionWhichHasNoEntryIsCallingStorageFunction)