From 5e0b207a8959c9033ae5192b509966654a593301 Mon Sep 17 00:00:00 2001 From: Marco Bubke Date: Thu, 5 Oct 2017 17:36:44 +0200 Subject: 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 --- src/libs/clangsupport/filepathcache.h | 4 +- src/libs/clangsupport/stringcache.h | 110 +++++++++++++++++++++---------- tests/unit/unittest/mockmutex.h | 2 + tests/unit/unittest/stringcache-test.cpp | 86 ++++++++++++++++++++---- 4 files changed, 154 insertions(+), 48 deletions(-) diff --git a/src/libs/clangsupport/filepathcache.h b/src/libs/clangsupport/filepathcache.h index 61e8680965..157559a28b 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 d3de7fe4dd..cee4d05808 100644 --- a/src/libs/clangsupport/stringcache.h +++ b/src/libs/clangsupport/stringcache.h @@ -32,8 +32,10 @@ #include #include +#include + #include -#include +#include #include 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 @@ -122,23 +157,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 +196,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 +204,25 @@ public: template StringType string(IndexType id, Function storageFunction) { - std::lock_guard lock(m_mutex); + std::shared_lock 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 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 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 +243,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 +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); diff --git a/tests/unit/unittest/mockmutex.h b/tests/unit/unittest/mockmutex.h index cccb34e2ff..f922210392 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 1a20e473de..8465321c34 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, 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) -- cgit v1.2.1