From be939a80db584aecf94fc83305cd1529f4ddb51d Mon Sep 17 00:00:00 2001
From: Marco Bubke <marco.bubke@qt.io>
Date: Thu, 5 Oct 2017 17:36:44 +0200
Subject: [PATCH] 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: I2cc742d1b9cc15c162be40ab339fa8310640bc44
Reviewed-by: Tim Jenssen <tim.jenssen@qt.io>
---
src/libs/clangsupport/filepathcache.h | 4 +-
src/libs/clangsupport/stringcache.h | 77 +++++++++++----------
tests/unit/unittest/mockmutex.h | 2 +
tests/unit/unittest/stringcache-test.cpp | 86 ++++++++++++++++++++----
4 files changed, 121 insertions(+), 48 deletions(-)
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<Utils::PathString,
int,
- std::mutex,
+ std::shared_timed_mutex,
decltype(&Utils::reverseCompare),
Utils::reverseCompare>;
using FileNameCache = StringCache<Utils::SmallString,
int,
- std::mutex,
+ std::shared_timed_mutex,
decltype(&Utils::compare),
Utils::compare>;
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 <utils/smallstringfwd.h>
#include <algorithm>
-#include <mutex>
+#include <shared_mutex>
#include <vector>
namespace ClangBackEnd {
@@ -54,6 +54,8 @@ public:
NonLockingMutex& operator=(const NonLockingMutex&) = delete;
void lock() {}
void unlock() {}
+ void lock_shared() {}
+ void unlock_shared() {}
};
template <typename StringType, typename IndexType>
@@ -122,23 +124,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);
+
+ if (found.wasFound)
+ return found.iterator->id;
+
+ sharedLock.unlock();
+ std::lock_guard<Mutex> exclusiveLock(m_mutex);
- return ungardedStringId(stringView);
+ 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 +163,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 +171,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 +210,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 +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, 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)
--
GitLab