diff --git a/src/libs/clangsupport/filepathcache.h b/src/libs/clangsupport/filepathcache.h index 61e86809653..553aed61f7c 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 d3de7fe4ddd..c09fd199c13 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,6 +54,8 @@ public: NonLockingMutex& operator=(const NonLockingMutex&) = delete; void lock() {} void unlock() {} + void lock_shared() {} + void unlock_shared() {} }; template @@ -122,23 +124,34 @@ public: IndexType stringId(Utils::SmallStringView stringView) { - std::lock_guard lock(m_mutex); + std::shared_lock sharedLock(m_mutex); + Found found = find(stringView); - return ungardedStringId(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, IndexType(m_indices.size())); + found.iterator = m_strings.begin() + index;; + } + + return found.iterator->id; } 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->ungardedStringId(string); }); + [&] (const auto &string) { return this->stringId(string); }); return ids; } @@ -150,7 +163,7 @@ public: StringType string(IndexType id) const { - std::lock_guard lock(m_mutex); + std::shared_lock sharedLock(m_mutex); return m_strings.at(m_indices.at(id)).string; } @@ -158,28 +171,25 @@ public: template StringType string(IndexType id, Function storageFunction) { - std::lock_guard lock(m_mutex); + std::shared_lock sharedLock(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; - if (IndexType(m_indices.size()) <= id) { - StringType string{storageFunction(id)}; - index = insertString(find(string).iterator, string, id); - } else { - index = m_indices.at(id); + StringType string{storageFunction(id)}; + index = insertString(find(string).iterator, string, id); - if (index < 0) { - StringType string{storageFunction(id)}; - index = insertString(find(string).iterator, string, id); - } - } - - return m_strings.at(index).string; + return m_strings[index].string; } std::vector strings(const std::vector &ids) const { - std::lock_guard lock(m_mutex); + std::shared_lock sharedLock(m_mutex); std::vector strings; strings.reserve(ids.size()); @@ -200,12 +210,21 @@ public: template IndexType stringId(Utils::SmallStringView stringView, Function storageFunction) { - std::lock_guard lock(m_mutex); + std::shared_lock 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 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 +235,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); diff --git a/tests/unit/unittest/mockmutex.h b/tests/unit/unittest/mockmutex.h index cccb34e2ff2..f922210392a 100644 --- a/tests/unit/unittest/mockmutex.h +++ b/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 ()); }; diff --git a/tests/unit/unittest/stringcache-test.cpp b/tests/unit/unittest/stringcache-test.cpp index 1a20e473de6..8465321c34d 100644 --- a/tests/unit/unittest/stringcache-test.cpp +++ b/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, 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()); - EXPECT_CALL(mockMutex, unlock()); + 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)