diff --git a/src/libs/clangsupport/filepathcache.h b/src/libs/clangsupport/filepathcache.h index 553aed61f7c..61e86809653 100644 --- a/src/libs/clangsupport/filepathcache.h +++ b/src/libs/clangsupport/filepathcache.h @@ -67,12 +67,12 @@ class FilePathCache final : private FilePathCacheBase { using DirectoryPathCache = StringCache; using FileNameCache = StringCache; public: diff --git a/src/libs/clangsupport/stringcache.h b/src/libs/clangsupport/stringcache.h index c09fd199c13..d3de7fe4ddd 100644 --- a/src/libs/clangsupport/stringcache.h +++ b/src/libs/clangsupport/stringcache.h @@ -33,7 +33,7 @@ #include #include -#include +#include #include namespace ClangBackEnd { @@ -54,8 +54,6 @@ public: NonLockingMutex& operator=(const NonLockingMutex&) = delete; void lock() {} void unlock() {} - void lock_shared() {} - void unlock_shared() {} }; template @@ -124,34 +122,23 @@ public: IndexType stringId(Utils::SmallStringView stringView) { - std::shared_lock sharedLock(m_mutex); - Found found = find(stringView); + std::lock_guard lock(m_mutex); - if (found.wasFound) - return found.iterator->id; - - sharedLock.unlock(); - std::lock_guard 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; + return ungardedStringId(stringView); } template std::vector stringIds(const Container &strings) { + std::lock_guard lock(m_mutex); + std::vector 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 sharedLock(m_mutex); + std::lock_guard lock(m_mutex); return m_strings.at(m_indices.at(id)).string; } @@ -171,25 +158,28 @@ public: template StringType string(IndexType id, Function storageFunction) { - std::shared_lock sharedLock(m_mutex); + std::lock_guard lock(m_mutex); - if (IndexType(m_indices.size()) > id && m_indices.at(id) >= 0) - return m_strings.at(m_indices.at(id)).string; - - - sharedLock.unlock(); - std::lock_guard 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); - return m_strings[index].string; + if (index < 0) { + StringType string{storageFunction(id)}; + index = insertString(find(string).iterator, string, id); + } + } + + return m_strings.at(index).string; } std::vector strings(const std::vector &ids) const { - std::shared_lock sharedLock(m_mutex); + std::lock_guard lock(m_mutex); std::vector strings; strings.reserve(ids.size()); @@ -210,21 +200,12 @@ public: template IndexType stringId(Utils::SmallStringView stringView, Function storageFunction) { - std::shared_lock sharedLock(m_mutex); + std::lock_guard lock(m_mutex); Found found = find(stringView); - if (found.wasFound) - return found.iterator->id; - - sharedLock.unlock(); - std::lock_guard 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); diff --git a/tests/unit/unittest/mockmutex.h b/tests/unit/unittest/mockmutex.h index f922210392a..cccb34e2ff2 100644 --- a/tests/unit/unittest/mockmutex.h +++ b/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 ()); }; diff --git a/tests/unit/unittest/stringcache-test.cpp b/tests/unit/unittest/stringcache-test.cpp index 8465321c34d..1a20e473de6 100644 --- a/tests/unit/unittest/stringcache-test.cpp +++ b/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) -{ - 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()); + EXPECT_CALL(mockMutex, lock()); + EXPECT_CALL(mockMutex, unlock()); 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"}); + cache.stringId("foo"); - EXPECT_CALL(mockMutex, lock_shared()); - EXPECT_CALL(mockMutex, unlock_shared()); - - cache.strings(ids); -} - -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.string(41, mockStorageFetchDirectyPath); + cache.strings({0}); } -TEST_F(StringCache, StringWithStorageFunctionIsReadLockedForKnownId) +TEST_F(StringCache, StringIdWithStorageFunctionIsLocked) { - InSequence s; - cache.string(41, mockStorageFetchDirectyPath); + EXPECT_CALL(mockMutex, lock()); + EXPECT_CALL(mockMutex, unlock()); - 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); + cache.stringId("foo", mockStorageFetchDirectyId); } TEST_F(StringCache, StringIdWithStorageFunctionWhichHasNoEntryIsCallingStorageFunction)