Clang: Improve file path caching

The old algorithm was comparing the string two times. One timer for smaller
and one for larger. And memcmp on a long string still costs. The new one
has a three state so it can compare smaller, greater and equal in one.
There is a reverse version too which has big advantage for file paths.

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

src/libs/clangsupport/clangsupport-lib.pri

@@ -173,6 +173,7 @@ HEADERS += \
     $$PWD/writemessageblock.h \
     $$PWD/ipcclientprovider.h \
     $$PWD/requestsourcerangesforquerymessage.h \
-    $$PWD/stringcachefwd.h
+    $$PWD/stringcachefwd.h \
+    $$PWD/stringcachealgorithms.h
 
 contains(QT_CONFIG, reduce_exports):CONFIG += hide_symbols

src/libs/clangsupport/stringcache.h

@@ -25,6 +25,7 @@
 
 #pragma once
 
+#include "stringcachealgorithms.h"
 #include "stringcachefwd.h"
 
 #include <utils/smallstringview.h>
@@ -63,23 +64,13 @@ public:
           id(id)
     {}
 
-    friend bool operator<(const StringCacheEntry &entry, Utils::SmallStringView stringView)
+    operator Utils::SmallStringView() const
     {
-        return entry.string < stringView;
-    }
-
-    friend bool operator<(Utils::SmallStringView stringView, const StringCacheEntry &entry)
-    {
-        return stringView < entry.string;
-    }
-
-    friend bool operator<(const StringCacheEntry &first, const StringCacheEntry &second)
-    {
-        return first.string < second.string;
+        return {string.data(), string.size()};
     }
 
     StringType string;
-    IndexType id;
+    uint id;
 };
 
 template <typename StringType, typename IndexType>
@@ -90,19 +81,15 @@ using FileCacheCacheEntries = std::vector<FileCacheCacheEntry>;
 
 template <typename StringType,
           typename IndexType,
-          typename Mutex>
+          typename Mutex,
+          typename Compare,
+          Compare compare = Utils::compare>
 class StringCache
 {
+    using CacheEntry = StringCacheEntry<StringType, IndexType>;
     using CacheEnties = StringCacheEntries<StringType, IndexType>;
     using const_iterator = typename CacheEnties::const_iterator;
-
-    class Found
-    {
-    public:
-        typename CacheEnties::const_iterator iterator;
-        bool wasFound;
-    };
-
+    using Found = ClangBackEnd::Found<const_iterator>;
 public:
     StringCache()
     {
@@ -119,7 +106,11 @@ public:
 
     void uncheckedPopulate(CacheEnties &&entries)
     {
-        std::sort(entries.begin(), entries.end());
+        std::sort(entries.begin(),
+                  entries.end(),
+                  [] (Utils::SmallStringView first, Utils::SmallStringView second) {
+            return compare(first, second) < 0;
+        });
 
         m_strings = std::move(entries);
         m_indices.resize(m_strings.size());
@@ -193,9 +184,7 @@ public:
 private:
     Found find(Utils::SmallStringView stringView)
     {
-        auto range = std::equal_range(m_strings.cbegin(), m_strings.cend(), stringView);
-
-        return {range.first, range.first != range.second};
+        return findInSorted(m_strings.cbegin(), m_strings.cend(), stringView, compare);
     }
 
     void incrementLargerOrEqualIndicesByOne(IndexType newIndex)
@@ -238,8 +227,6 @@ private:
     mutable Mutex m_mutex;
 };
 
-template <typename Mutex>
-using FilePathCache = StringCache<Utils::PathString, FilePathIndex, Mutex>;
 using FilePathIndices = std::vector<FilePathIndex>;
 
 } // namespace ClangBackEnd

src/libs/clangsupport/stringcachealgorithms.h

@@ -0,0 +1,72 @@
+/****************************************************************************
+**
+** Copyright (C) 2017 The Qt Company Ltd.
+** Contact: https://www.qt.io/licensing/
+**
+** This file is part of Qt Creator.
+**
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and The Qt Company. For licensing terms
+** and conditions see https://www.qt.io/terms-conditions. For further
+** information use the contact form at https://www.qt.io/contact-us.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 3 as published by the Free Software
+** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT
+** included in the packaging of this file. Please review the following
+** information to ensure the GNU General Public License requirements will
+** be met: https://www.gnu.org/licenses/gpl-3.0.html.
+**
+****************************************************************************/
+
+#pragma once
+
+#include <utils/smallstringio.h>
+
+#include <iterator>
+
+#include <QDebug>
+
+namespace ClangBackEnd {
+
+template <typename Iterator>
+class Found
+{
+public:
+    Iterator iterator;
+    bool wasFound;
+};
+
+template<typename ForwardIterator,
+         typename Type,
+         typename Compare>
+Found<ForwardIterator> findInSorted(ForwardIterator first, ForwardIterator last, const Type& value, Compare compare)
+{
+    ForwardIterator current;
+    using DifferenceType = typename std::iterator_traits<ForwardIterator>::difference_type;
+    DifferenceType count{std::distance(first, last)};
+    DifferenceType step;
+
+    while (count > 0) {
+        current = first;
+        step = count / 2;
+        std::advance(current, step);
+        auto comparison = compare(*current, value);
+        if (comparison < 0) {
+            first = ++current;
+            count -= step + 1;
+        } else if (comparison > 0) {
+            count = step;
+        } else {
+            return {current, true};
+        }
+    }
+
+    return {first, false};
+}
+
+} // namespace ClangBackEnd

src/libs/clangsupport/stringcachefwd.h

@@ -35,11 +35,17 @@ class NonLockingMutex;
 
 template <typename StringType,
           typename IndexType,
-          typename Mutex>
+          typename Mutex,
+          typename Compare,
+          Compare compare>
 class StringCache;
 
 template <typename Mutex = NonLockingMutex>
-using FilePathCache = StringCache<Utils::PathString, FilePathIndex, Mutex>;
+using FilePathCache = StringCache<Utils::PathString,
+                                  FilePathIndex,
+                                  Mutex,
+                                  decltype(&Utils::reverseCompare),
+                                  Utils::reverseCompare>;
 
 } // namespace ClangBackEnd
 

src/tools/clangpchmanagerbackend/source/clangpathwatcher.h

@@ -63,7 +63,11 @@ public:
 
 using WatcherEntries = std::vector<WatcherEntry>;
 
-using IdCache = StringCache<Utils::SmallString, FilePathIndex, NonLockingMutex>;
+using IdCache = StringCache<Utils::SmallString,
+                            FilePathIndex,
+                            NonLockingMutex,
+                            decltype(&Utils::compare),
+                            Utils::compare>;
 
 template <typename FileSystemWatcher,
           typename Timer>

tests/unit/unittest/pchcreator-test.cpp

@@ -65,7 +65,7 @@ using UnitTests::EndsWith;
 class PchCreator: public ::testing::Test
 {
 protected:
-    uint id(const Utils::PathString &path);
+    ClangBackEnd::FilePathIndex id(const Utils::PathString &path);
 
 protected:
     ClangBackEnd::FilePathCache<> filePathCache;
@@ -340,7 +340,7 @@ TEST_F(PchCreator, CreateProjectPartHeaderAndSourcesContent)
                             "#include \"" TESTDATA_DIR "/includecollector_main3.cpp\"\n"));
 }
 
-uint PchCreator::id(const Utils::PathString &path)
+ClangBackEnd::FilePathIndex PchCreator::id(const Utils::PathString &path)
 {
     return filePathCache.stringId(path);
 }

tests/unit/unittest/stringcache-test.cpp

@@ -35,16 +35,34 @@ using ClangBackEnd::StringCacheException;
 
 using uint64 = unsigned long long;
 
+using Utils::compare;
+using Utils::reverseCompare;
+using ClangBackEnd::findInSorted;
+
 using CacheEntries = ClangBackEnd::FileCacheCacheEntries;
 
 class StringCache : public testing::Test
 {
+protected:
+    void SetUp();
+
 protected:
     ClangBackEnd::FilePathCache<> cache;
     Utils::PathString filePath1{"/file/pathOne"};
     Utils::PathString filePath2{"/file/pathTwo"};
     Utils::PathString filePath3{"/file/pathThree"};
     Utils::PathString filePath4{"/file/pathFour"};
+    Utils::PathString filePath5{"/file/pathFife"};
+    Utils::PathStringVector filePaths{filePath1,
+                                      filePath2,
+                                      filePath3,
+                                      filePath4,
+                                      filePath5};
+    Utils::PathStringVector reverseFilePaths{filePath1,
+                                             filePath2,
+                                             filePath3,
+                                             filePath4,
+                                             filePath5};
 };
 
 TEST_F(StringCache, AddFilePath)
@@ -209,4 +227,95 @@ TEST_F(StringCache, MultipleEntries)
     ASSERT_THROW(cache.populate(std::move(entries)), StringCacheException);
 }
 
+TEST_F(StringCache, DontFindInSorted)
+{
+    auto found = findInSorted(filePaths.cbegin(), filePaths.cend(), "/file/pathFoo", compare);
+
+    ASSERT_FALSE(found.wasFound);
 }
+
+TEST_F(StringCache, FindInSortedOne)
+{
+    auto found = findInSorted(filePaths.cbegin(), filePaths.cend(), "/file/pathOne", compare);
+
+    ASSERT_TRUE(found.wasFound);
+}
+
+TEST_F(StringCache, FindInSortedTwo)
+{
+    auto found = findInSorted(filePaths.cbegin(), filePaths.cend(), "/file/pathTwo", compare);
+
+    ASSERT_TRUE(found.wasFound);
+}
+
+TEST_F(StringCache, FindInSortedTree)
+{
+    auto found = findInSorted(filePaths.cbegin(), filePaths.cend(), "/file/pathThree", compare);
+
+    ASSERT_TRUE(found.wasFound);
+}
+
+TEST_F(StringCache, FindInSortedFour)
+{
+    auto found = findInSorted(filePaths.cbegin(), filePaths.cend(), "/file/pathFour", compare);
+
+    ASSERT_TRUE(found.wasFound);
+}
+
+TEST_F(StringCache, FindInSortedFife)
+{
+    auto found = findInSorted(filePaths.cbegin(), filePaths.cend(), "/file/pathFife", compare);
+
+    ASSERT_TRUE(found.wasFound);
+}
+
+TEST_F(StringCache, DontFindInSortedReverse)
+{
+    auto found = findInSorted(reverseFilePaths.cbegin(), reverseFilePaths.cend(), "/file/pathFoo", reverseCompare);
+
+    ASSERT_FALSE(found.wasFound);
+}
+
+TEST_F(StringCache, FindInSortedOneReverse)
+{
+    auto found = findInSorted(reverseFilePaths.cbegin(), reverseFilePaths.cend(), "/file/pathOne", reverseCompare);
+
+    ASSERT_TRUE(found.wasFound);
+}
+
+TEST_F(StringCache, FindInSortedTwoReverse)
+{
+    auto found = findInSorted(reverseFilePaths.cbegin(), reverseFilePaths.cend(), "/file/pathTwo", reverseCompare);
+
+    ASSERT_TRUE(found.wasFound);
+}
+
+TEST_F(StringCache, FindInSortedTreeReverse)
+{
+    auto found = findInSorted(reverseFilePaths.cbegin(), reverseFilePaths.cend(), "/file/pathThree", reverseCompare);
+
+    ASSERT_TRUE(found.wasFound);
+}
+
+TEST_F(StringCache, FindInSortedFourReverse)
+{
+    auto found = findInSorted(reverseFilePaths.cbegin(), reverseFilePaths.cend(), "/file/pathFour", reverseCompare);
+
+    ASSERT_TRUE(found.wasFound);
+}
+
+TEST_F(StringCache, FindInSortedFifeReverse)
+{
+    auto found = findInSorted(reverseFilePaths.cbegin(), reverseFilePaths.cend(), "/file/pathFife", reverseCompare);
+
+    ASSERT_TRUE(found.wasFound);
+}
+
+
+void StringCache::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;});
+}
+}
+