Clang: Improve file path cache

Change-Id: I940dc63c0fc95fbb826ed7d19a1ac1e4db5a542c
Reviewed-by: Tim Jenssen <tim.jenssen@qt.io>

src/libs/clangsupport/filepathcache.h

@@ -161,6 +161,27 @@ public:
         return m_fileNameCache.string(filePathId.filePathId, fetchSoureNameAndDirectoryId).directoryId;
     }
 
+    void addFilePaths(FilePathViews &&filePaths)
+    {
+        auto directoryPaths = Utils::transform<std::vector<Utils::SmallStringView>>(
+            filePaths, [](FilePathView filePath) { return filePath.directory(); });
+
+        m_directoryPathCache.addStrings(std::move(directoryPaths),
+                                        [&](Utils::SmallStringView directoryPath) {
+                                            return m_filePathStorage.fetchDirectoryIdUnguarded(
+                                                directoryPath);
+                                        });
+
+        auto sourcePaths = Utils::transform<std::vector<FileNameView>>(filePaths, [&](FilePathView filePath) {
+            return FileNameView{filePath.name(), m_directoryPathCache.stringId(filePath.directory())};
+        });
+
+        m_fileNameCache.addStrings(std::move(sourcePaths), [&](FileNameView fileNameView) {
+            return m_filePathStorage.fetchSourceIdUnguarded(fileNameView.directoryId,
+                                                            fileNameView.fileName);
+        });
+    }
+
 private:
     mutable DirectoryPathCache m_directoryPathCache;
     mutable FileNameCache m_fileNameCache;

src/libs/clangsupport/filepathstorage.h

@@ -48,6 +48,16 @@ public:
         : m_statementFactory(statementFactory)
     {}
 
+    int fetchDirectoryIdUnguarded(Utils::SmallStringView directoryPath)
+    {
+        Utils::optional<int> optionalDirectoryId = readDirectoryId(directoryPath);
+
+        if (optionalDirectoryId)
+            return optionalDirectoryId.value();
+
+        return writeDirectoryId(directoryPath);
+    }
+
     int fetchDirectoryId(Utils::SmallStringView directoryPath)
     {
         try {
@@ -125,19 +135,22 @@ public:
         }
     }
 
+    int fetchSourceIdUnguarded(int directoryId, Utils::SmallStringView sourceName)
+    {
+        Utils::optional<int> optionalSourceId = readSourceId(directoryId, sourceName);
+
+        if (optionalSourceId)
+            return optionalSourceId.value();
+
+        return writeSourceId(directoryId, sourceName);
+    }
+
     int fetchSourceId(int directoryId, Utils::SmallStringView sourceName)
     {
         try {
             Sqlite::DeferredTransaction transaction{m_statementFactory.database};
 
-            Utils::optional<int> optionalSourceId = readSourceId(directoryId, sourceName);
+            int sourceId = fetchSourceIdUnguarded(directoryId, sourceName);
-
-            int sourceId = -1;
-
-            if (optionalSourceId)
-                sourceId = optionalSourceId.value();
-            else
-                sourceId = writeSourceId(directoryId, sourceName);
 
             transaction.commit();
 

src/libs/clangsupport/stringcache.h

@@ -26,6 +26,7 @@
 #pragma once
 
 #include "filepathstoragesources.h"
+#include "set_algorithm.h"
 #include "stringcachealgorithms.h"
 #include "stringcachefwd.h"
 
@@ -152,9 +153,7 @@ public:
 
     void uncheckedPopulate(CacheEntries &&entries)
     {
-        std::sort(entries.begin(),
+        std::sort(entries.begin(), entries.end(), [](StringViewType first, StringViewType second) {
-                  entries.end(),
-                  [] (StringViewType first, StringViewType second) {
             return compare(first, second) < 0;
         });
 
@@ -173,9 +172,59 @@ public:
 
         m_indices.resize(max_id, -1);
 
-        auto begin = m_strings.cbegin();
+        updateIndices();
-        for (auto current = begin; current != m_strings.end(); ++current)
+    }
-            m_indices[current->id] = std::distance(begin, current);
+
+    template<typename Function>
+    void addStrings(std::vector<StringViewType> &&strings, Function storageFunction)
+    {
+        auto less = [](StringViewType first, StringViewType second) {
+            return compare(first, second) < 0;
+        };
+
+        std::sort(strings.begin(), strings.end(), less);
+
+        strings.erase(std::unique(strings.begin(), strings.end()), strings.end());
+
+        CacheEntries newCacheEntries;
+        newCacheEntries.reserve(strings.size());
+
+        std::set_difference(strings.begin(),
+                            strings.end(),
+                            m_strings.begin(),
+                            m_strings.end(),
+                            make_iterator([&](StringViewType newString) {
+                                IndexType index = storageFunction(newString);
+                                newCacheEntries.emplace_back(newString, index);
+                            }),
+                            less);
+
+        if (newCacheEntries.size()) {
+            auto found = std::max_element(newCacheEntries.begin(),
+                                          newCacheEntries.end(),
+                                          [](const auto &first, const auto &second) {
+                                              return first.id < second.id;
+                                          });
+
+            int max_id = found->id + 1;
+
+            if (max_id > int(m_indices.size()))
+                m_indices.resize(max_id, -1);
+
+            CacheEntries mergedCacheEntries;
+            mergedCacheEntries.reserve(newCacheEntries.size() + m_strings.size());
+
+            std::merge(std::make_move_iterator(m_strings.begin()),
+                       std::make_move_iterator(m_strings.end()),
+                       std::make_move_iterator(newCacheEntries.begin()),
+                       std::make_move_iterator(newCacheEntries.end()),
+                       std::back_inserter(mergedCacheEntries),
+                       less);
+
+            m_strings = std::move(mergedCacheEntries);
+
+            updateIndices();
+        }
     }
 
     IndexType stringId(StringViewType stringView)
@@ -291,6 +340,13 @@ public:
     }
 
 private:
+    void updateIndices()
+    {
+        auto begin = m_strings.cbegin();
+        for (auto current = begin; current != m_strings.end(); ++current)
+            m_indices[current->id] = std::distance(begin, current);
+    }
+
     Found find(StringViewType stringView)
     {
         return findInSorted(m_strings.cbegin(), m_strings.cend(), stringView, compare);

src/libs/utils/smallstring.h

@@ -181,7 +181,7 @@ public:
     }
 
     BasicSmallString(BasicSmallString &&other) noexcept
-        : m_data(other.m_data)
+        : m_data(std::move(other.m_data))
     {
         other.m_data.reset();
     }
@@ -193,8 +193,8 @@ public:
 
         this->~BasicSmallString();
 
-        m_data = other.m_data;
+        m_data = std::move(other.m_data);
-        other.m_data = Internal::StringDataLayout<Size>();
+        other.m_data.reset();
 
         return *this;
     }

tests/unit/unittest/filepathcache-test.cpp

@@ -54,8 +54,7 @@ protected:
                 .WillByDefault(Return(63));
         ON_CALL(mockStorage, fetchSourceId(6, Eq("file.cpp")))
                 .WillByDefault(Return(72));
-        ON_CALL(mockStorage, fetchDirectoryPath(5))
+        ON_CALL(mockStorage, fetchDirectoryPath(5)).WillByDefault(Return(Utils::PathString("/path/to")));
-                .WillByDefault(Return(Utils::PathString("/path/to")));
         ON_CALL(mockStorage, fetchSourceNameAndDirectoryId(42))
             .WillByDefault(Return(SourceNameAndDirectoryId("file.cpp", 5)));
         ON_CALL(mockStorageFilled, fetchAllSources())
@@ -67,6 +66,10 @@ protected:
         ON_CALL(mockStorageFilled, fetchAllDirectories())
             .WillByDefault(Return(
                 std::vector<ClangBackEnd::Sources::Directory>({{"/path2/to", 6}, {"/path/to", 5}})));
+        ON_CALL(mockStorageFilled, fetchDirectoryIdUnguarded(Eq("/path3/to"))).WillByDefault(Return(7));
+        ON_CALL(mockStorageFilled, fetchSourceIdUnguarded(7, Eq("file.h"))).WillByDefault(Return(101));
+        ON_CALL(mockStorageFilled, fetchSourceIdUnguarded(6, Eq("file2.h"))).WillByDefault(Return(106));
+        ON_CALL(mockStorageFilled, fetchSourceIdUnguarded(5, Eq("file.h"))).WillByDefault(Return(99));
     }
 
 protected:
@@ -370,4 +373,38 @@ TEST_F(FilePathCache, GetFilePathInFilledCache)
 
     ASSERT_THAT(path, Eq("/path/to/file.cpp"));
 }
+
+TEST_F(FilePathCache, GetFileIdAfterAddFilePaths)
+{
+    Cache cacheFilled{mockStorageFilled};
+
+    cacheFilled.addFilePaths(
+        {"/path3/to/file.h", "/path/to/file.h", "/path2/to/file2.h", "/path/to/file.cpp"});
+
+    ASSERT_THAT(cacheFilled.filePath(101), Eq("/path3/to/file.h"));
+}
+
+TEST_F(FilePathCache, GetFileIdAfterAddFilePathsWhichWasAlreadyAdded)
+{
+    Cache cacheFilled{mockStorageFilled};
+
+    cacheFilled.addFilePaths({"/path3/to/file.h", "/path/to/file.h", "/path2/to/file2.h"});
+
+    ASSERT_THAT(cacheFilled.filePath(42), Eq("/path/to/file.cpp"));
+}
+
+TEST_F(FilePathCache, AddFilePathsCalls)
+{
+    Cache cacheFilled{mockStorageFilled};
+    InSequence s;
+
+    EXPECT_CALL(mockStorageFilled, fetchDirectoryIdUnguarded(Eq("/path3/to"))).WillOnce(Return(7));
+    EXPECT_CALL(mockStorageFilled, fetchSourceIdUnguarded(5, Eq("file.h"))).WillOnce(Return(99));
+    EXPECT_CALL(mockStorageFilled, fetchSourceIdUnguarded(6, Eq("file2.h"))).WillOnce(Return(106));
+    EXPECT_CALL(mockStorageFilled, fetchSourceIdUnguarded(7, Eq("file.h"))).WillOnce(Return(101));
+
+    cacheFilled.addFilePaths(
+        {"/path3/to/file.h", "/path/to/file.h", "/path2/to/file2.h", "/path/to/file.cpp"});
+}
+
 } // namespace

tests/unit/unittest/filepathstorage-test.cpp

@@ -599,4 +599,75 @@ TEST_F(FilePathStorage, FetchDirectoryIdCallsThrows)
     ASSERT_ANY_THROW(storage.fetchDirectoryId(41));
 }
 
+TEST_F(FilePathStorage, GetTheDirectoryIdBackAfterFetchingANewEntryFromDirectoriesUnguarded)
+{
+    auto directoryId = storage.fetchDirectoryIdUnguarded("/some/not/known/path");
+
+    ASSERT_THAT(directoryId, 12);
+}
+
+TEST_F(FilePathStorage, GetTheSourceIdBackAfterFetchingANewEntryFromSourcesUnguarded)
+{
+    auto sourceId = storage.fetchSourceIdUnguarded(5, "unknownfile.h");
+
+    ASSERT_THAT(sourceId, 12);
+}
+
+TEST_F(FilePathStorage, CallSelectForFetchingDirectoryIdForKnownPathUnguarded)
+{
+    InSequence s;
+
+    EXPECT_CALL(selectDirectoryIdFromDirectoriesByDirectoryPath,
+                valueReturnInt32(TypedEq<Utils::SmallStringView>("/path/to")));
+
+    storage.fetchDirectoryIdUnguarded("/path/to");
+}
+
+TEST_F(FilePathStorage, CallSelectForFetchingSourceIdForKnownPathUnguarded)
+{
+    InSequence s;
+
+    EXPECT_CALL(selectSourceIdFromSourcesByDirectoryIdAndSourceName,
+                valueReturnInt32(5, Eq("file.h")));
+
+    storage.fetchSourceIdUnguarded(5, "file.h");
+}
+
+TEST_F(FilePathStorage, CallNotWriteForFetchingDirectoryIdForKnownPathUnguarded)
+{
+    EXPECT_CALL(insertIntoDirectories, write(An<Utils::SmallStringView>())).Times(0);
+
+    storage.fetchDirectoryIdUnguarded("/path/to");
+}
+
+TEST_F(FilePathStorage, CallNotWriteForFetchingSoureIdForKnownEntryUnguarded)
+{
+    EXPECT_CALL(insertIntoSources, write(An<uint>(), An<Utils::SmallStringView>())).Times(0);
+
+    storage.fetchSourceIdUnguarded(5, "file.h");
+}
+
+TEST_F(FilePathStorage, CallSelectAndWriteForFetchingDirectoryIdForUnknownPathUnguarded)
+{
+    InSequence s;
+
+    EXPECT_CALL(selectDirectoryIdFromDirectoriesByDirectoryPath,
+                valueReturnInt32(TypedEq<Utils::SmallStringView>("/some/not/known/path")));
+    EXPECT_CALL(insertIntoDirectories, write(TypedEq<Utils::SmallStringView>("/some/not/known/path")));
+
+    storage.fetchDirectoryIdUnguarded("/some/not/known/path");
+}
+
+TEST_F(FilePathStorage, CallSelectAndWriteForFetchingSourceIdForUnknownEntryUnguarded)
+{
+    InSequence s;
+
+    EXPECT_CALL(selectSourceIdFromSourcesByDirectoryIdAndSourceName,
+                valueReturnInt32(5, Eq("unknownfile.h")));
+    EXPECT_CALL(insertIntoSources,
+                write(TypedEq<int>(5), TypedEq<Utils::SmallStringView>("unknownfile.h")));
+
+    storage.fetchSourceIdUnguarded(5, "unknownfile.h");
+}
+
 } // namespace

tests/unit/unittest/mockfilepathstorage.h

@@ -36,6 +36,8 @@ public:
                  int (Utils::SmallStringView directoryPath));
     MOCK_METHOD2(fetchSourceId,
                  int (int directoryId, Utils::SmallStringView sourceName));
+    MOCK_METHOD1(fetchDirectoryIdUnguarded, int(Utils::SmallStringView directoryPath));
+    MOCK_METHOD2(fetchSourceIdUnguarded, int(int directoryId, Utils::SmallStringView sourceName));
     MOCK_METHOD1(fetchDirectoryPath,
                  Utils::PathString (int directoryId));
     MOCK_METHOD1(fetchSourceNameAndDirectoryId,

tests/unit/unittest/stringcache-test.cpp

@@ -55,7 +55,21 @@ using CacheEntries = Cache::CacheEntries;
 class StringCache : public testing::Test
 {
 protected:
-    void SetUp();
+    void SetUp()
+    {
+        std::sort(filePaths.begin(), filePaths.end(), [](auto &f, auto &l) {
+            return compare(f, l) < 0;
+        });
+        std::sort(reverseFilePaths.begin(), reverseFilePaths.end(), [](auto &f, auto &l) {
+            return reverseCompare(f, l) < 0;
+        });
+
+        ON_CALL(mockStorage, fetchDirectoryId(Eq("foo"))).WillByDefault(Return(42));
+        ON_CALL(mockStorage, fetchDirectoryId(Eq("bar"))).WillByDefault(Return(43));
+        ON_CALL(mockStorage, fetchDirectoryId(Eq("poo"))).WillByDefault(Return(44));
+        ON_CALL(mockStorage, fetchDirectoryId(Eq("taa"))).WillByDefault(Return(45));
+        ON_CALL(mockStorage, fetchDirectoryPath(41)).WillByDefault(Return(Utils::PathString("bar")));
+    }
 
 protected:
     NiceMock<MockFilePathStorage> mockStorage;
@@ -490,15 +504,57 @@ TEST_F(StringCache, FetchDirectoryPathForUnknownIndex)
     ASSERT_THAT(string, Eq("bar"));
 }
 
-void StringCache::SetUp()
+TEST_F(StringCache, AddStringCalls)
 {
-    std::sort(filePaths.begin(), filePaths.end(), [] (auto &f, auto &l) { return compare(f, l) < 0;});
+    EXPECT_CALL(mockStorage, fetchDirectoryId(Eq("foo")));
-    std::sort(reverseFilePaths.begin(), reverseFilePaths.end(), [] (auto &f, auto &l) { return reverseCompare(f, l) < 0;});
+    EXPECT_CALL(mockStorage, fetchDirectoryId(Eq("bar")));
+    EXPECT_CALL(mockStorage, fetchDirectoryId(Eq("poo")));
 
-    ON_CALL(mockStorage, fetchDirectoryId(Eq("foo")))
+    cache.addStrings({"foo", "bar", "poo"}, mockStorageFetchDirectyId);
-            .WillByDefault(Return(42));
-    ON_CALL(mockStorage, fetchDirectoryPath(41))
-            .WillByDefault(Return(Utils::PathString("bar")));
-}
 }
 
+TEST_F(StringCache, AddStringCallsOnlyForNewStrings)
+{
+    cache.addStrings({"foo", "poo"}, mockStorageFetchDirectyId);
+
+    EXPECT_CALL(mockStorage, fetchDirectoryId(Eq("taa")));
+    EXPECT_CALL(mockStorage, fetchDirectoryId(Eq("bar")));
+
+    cache.addStrings({"foo", "bar", "poo", "taa"}, mockStorageFetchDirectyId);
+}
+
+TEST_F(StringCache, GetStringIdAfterAddingStrings)
+{
+    cache.addStrings({"foo", "bar", "poo", "taa"}, mockStorageFetchDirectyId);
+
+    ASSERT_THAT(cache.string(cache.stringId("taa")), Eq("taa"));
+}
+
+TEST_F(StringCache, GetStringAfterAddingStrings)
+{
+    cache.addStrings({"foo", "bar", "poo", "taa"}, mockStorageFetchDirectyId);
+
+    ASSERT_THAT(cache.string(cache.stringId("taa")), Eq("taa"));
+}
+
+TEST_F(StringCache, GetStringIdAfterAddingStringsMultipleTimes)
+{
+    cache.addStrings({"foo", "taa"}, mockStorageFetchDirectyId);
+
+    cache.addStrings({"foo", "bar", "poo", "taa"}, mockStorageFetchDirectyId);
+
+    ASSERT_THAT(cache.string(cache.stringId("taa")), Eq("taa"));
+}
+
+TEST_F(StringCache, GetStringIdAfterAddingTheSameStringsMultipleTimes)
+{
+    cache.addStrings({"foo", "taa", "poo", "taa", "bar", "taa"}, mockStorageFetchDirectyId);
+
+    ASSERT_THAT(cache.string(cache.stringId("taa")), Eq("taa"));
+}
+
+TEST_F(StringCache, AddingEmptyStrings)
+{
+    cache.addStrings({}, mockStorageFetchDirectyId);
+}
+} // namespace