QmlDesigner: Cleanup storage cache

The cache was not using locking. It is now activated. If that will be a
performance problem we can add an extra path cache for the main thread.

Change-Id: I74565b4a8679a2c015ff651c3480362e924809a3
Reviewed-by: Thomas Hartmann <thomas.hartmann@qt.io>

src/libs/utils/set_algorithm.h

@@ -241,6 +241,71 @@ struct set_greedy_difference_functor
 
 inline constexpr set_greedy_difference_functor set_greedy_difference{};
 
+struct set_difference_functor
+{
+    template<std::input_iterator Iterator1,
+             std::sentinel_for<Iterator1> Sentinel1,
+             std::input_iterator Iterator2,
+             std::sentinel_for<Iterator2> Sentinel2,
+             std::invocable<std::iter_value_t<Iterator1> &> Callable,
+             typename Comp = std::ranges::less,
+             typename Projection1 = std::identity,
+             typename Projection2 = std::identity>
+        requires callmergeable<Iterator1, Iterator2, Comp, Projection1, Projection2>
+    constexpr void operator()(Iterator1 first1,
+                              Sentinel1 last1,
+                              Iterator2 first2,
+                              Sentinel2 last2,
+                              Callable &&callable,
+                              Comp comp = {},
+                              Projection1 proj1 = {},
+                              Projection2 proj2 = {}) const
+    {
+        while (first1 != last1 && first2 != last2) {
+            if (std::invoke(comp, std::invoke(proj1, *first1), std::invoke(proj2, *first2))) {
+                std::invoke(callable, *first1);
+                ++first1;
+            } else if (std::invoke(comp, std::invoke(proj2, *first2), std::invoke(proj1, *first1))) {
+                ++first2;
+            } else {
+                ++first1;
+                ++first2;
+            }
+        }
+
+        while (first1 != last1) {
+            std::invoke(callable, *first1);
+            ++first1;
+        }
+    }
+
+    template<std::ranges::input_range Range1,
+             std::ranges::input_range Range2,
+             std::invocable<std::iter_value_t<Range1> &> Callable,
+             typename Comp = std::ranges::less,
+             typename Projection1 = std::identity,
+             typename Projection2 = std::identity>
+        requires callmergeable<std::ranges::iterator_t<Range1>, std::ranges::iterator_t<Range2>, Comp, Projection1, Projection2>
+    constexpr void operator()(Range1 &&range1,
+                              Range2 &&range2,
+                              Callable &&callable,
+                              Comp comp = {},
+                              Projection1 proj1 = {},
+                              Projection2 proj2 = {}) const
+    {
+        (*this)(std::ranges::begin(range1),
+                std::ranges::end(range1),
+                std::ranges::begin(range2),
+                std::ranges::end(range2),
+                std::forward<Callable>(callable),
+                std::move(comp),
+                std::move(proj1),
+                std::move(proj2));
+    }
+};
+
+inline constexpr set_difference_functor set_difference{};
+
 struct set_has_common_element_functor
 {
     template<std::input_iterator Iterator1,

src/plugins/qmldesigner/libs/designercore/include/modelfwd.h

@@ -9,6 +9,8 @@
 #include <utils/smallstringview.h>
 #include <utils/span.h>
 
+#include <shared_mutex>
+
 namespace QmlDesigner {
 using PropertyName = QByteArray;
 using PropertyNameView = QByteArrayView;
@@ -40,7 +42,7 @@ constexpr bool useProjectStorage()
 #endif
 }
 class SourcePathStorage;
-using PathCache = SourcePathCache<SourcePathStorage, NonLockingMutex>;
+using PathCache = SourcePathCache<SourcePathStorage, std::shared_mutex>;
 
 #ifdef QDS_MODEL_USE_PROJECTSTORAGEINTERFACE
 using ProjectStorageType = ProjectStorageInterface;
@@ -48,7 +50,7 @@ class SourcePathCacheInterface;
 using PathCacheType = SourcePathCacheInterface;
 #else
 using ProjectStorageType = ProjectStorage;
-using PathCacheType = SourcePathCache<SourcePathStorage, NonLockingMutex>;
+using PathCacheType = SourcePathCache<SourcePathStorage, std::shared_mutex>;
 #endif
 
 struct ProjectStorageDependencies

src/plugins/qmldesigner/libs/designercore/projectstorage/projectstorage.h

@@ -284,10 +284,13 @@ private:
 
     friend ModuleStorageAdapter;
 
-    static bool moduleNameLess(ModuleView first, ModuleView second) noexcept
+    struct ModuleNameLess
     {
-        return first < second;
+        bool operator()(ModuleView first, ModuleView second) const noexcept
-    }
+        {
+            return first < second;
+        }
+    };
 
     class ModuleCacheEntry : public StorageCacheEntry<Storage::Module, ModuleView, ModuleId>
     {
@@ -314,7 +317,7 @@ private:
     using ModuleCacheEntries = std::vector<ModuleCacheEntry>;
 
     using ModuleCache
-        = StorageCache<Storage::Module, ModuleView, ModuleId, ModuleStorageAdapter, NonLockingMutex, moduleNameLess, ModuleCacheEntry>;
+        = StorageCache<Storage::Module, ModuleView, ModuleId, ModuleStorageAdapter, NonLockingMutex, ModuleNameLess, ModuleCacheEntry>;
 
     ModuleId fetchModuleId(Utils::SmallStringView moduleName, Storage::ModuleKind moduleKind);
 

src/plugins/qmldesigner/libs/designercore/sourcepathstorage/sourcepathcache.h

@@ -142,27 +142,38 @@ private:
         Storage &storage;
     };
 
-    static bool sourceLess(Utils::SmallStringView first, Utils::SmallStringView second) noexcept
+    struct SourceNameLess
     {
-        return std::lexicographical_compare(first.rbegin(),
+        bool operator()(Utils::SmallStringView first, Utils::SmallStringView second) const noexcept
-                                            first.rend(),
+        {
-                                            second.rbegin(),
+            return first < second;
-                                            second.rend());
+        }
-    }
+    };
+
+    struct SourceContextLess
+    {
+        bool operator()(Utils::SmallStringView first, Utils::SmallStringView second) const noexcept
+        {
+            return std::ranges::lexicographical_compare(first.rbegin(),
+                                                        first.rend(),
+                                                        second.rbegin(),
+                                                        second.rend());
+        }
+    };
 
     using SourceContextPathCache = StorageCache<Utils::PathString,
                                                 Utils::SmallStringView,
                                                 SourceContextId,
                                                 SourceContextStorageAdapter,
                                                 Mutex,
-                                                sourceLess,
+                                                SourceContextLess,
                                                 Cache::SourceContext>;
-    using SourceNameCache = StorageCache<Utils::PathString,
+    using SourceNameCache = StorageCache<Utils::SmallString,
                                          Utils::SmallStringView,
                                          SourceNameId,
                                          SourceNameStorageAdapter,
                                          Mutex,
-                                         sourceLess,
+                                         SourceNameLess,
                                          Cache::SourceName>;
 
 private:

src/plugins/qmldesigner/libs/designercore/sourcepathstorage/storagecache.h

@@ -33,7 +33,7 @@ template<typename Type,
          typename IndexType,
          typename Storage,
          typename Mutex,
-         bool (*compare)(ViewType, ViewType),
+         typename Compare,
          class CacheEntry = StorageCacheEntry<Type, ViewType, IndexType>>
 class StorageCache
 {
@@ -140,17 +140,11 @@ public:
     {
         m_entries = m_storage.fetchAll();
 
-        std::sort(m_entries.begin(), m_entries.end(), [](ViewType first, ViewType second) {
+        std::ranges::sort(m_entries, Compare{});
-            return compare(first, second);
-        });
 
         std::size_t max_id = 0;
 
-        auto found = std::max_element(m_entries.begin(),
+        auto found = std::ranges::max_element(m_entries, {}, &CacheEntry::id);
-                                      m_entries.end(),
-                                      [](const auto &first, const auto &second) {
-                                          return first.id < second.id;
-                                      });
 
         if (found != m_entries.end())
             max_id = static_cast<std::size_t>(found->id);
@@ -162,31 +156,25 @@ public:
 
     void add(std::vector<ViewType> &&views)
     {
-        auto less = [](ViewType first, ViewType second) { return compare(first, second); };
+        std::ranges::sort(views, Compare{});
 
-        std::sort(views.begin(), views.end(), less);
+        auto removed = std::ranges::unique(views.begin(), views.end());
-
+        views.erase(removed.begin(), removed.end());
-        views.erase(std::unique(views.begin(), views.end()), views.end());
 
         CacheEntries newCacheEntries;
         newCacheEntries.reserve(views.size());
 
-        std::set_difference(views.begin(),
+        Utils::set_difference(
-                            views.end(),
+            views,
-                            m_entries.begin(),
+            m_entries,
-                            m_entries.end(),
+            [&](ViewType newView) {
-                            Utils::make_iterator([&](ViewType newView) {
+                IndexType index = m_storage.fetchId(newView);
-                                IndexType index = m_storage.fetchId(newView);
+                newCacheEntries.emplace_back(newView, index);
-                                newCacheEntries.emplace_back(newView, index);
+            },
-                            }),
+            Compare{});
-                            less);
 
         if (newCacheEntries.size()) {
-            auto found = std::max_element(newCacheEntries.begin(),
+            auto found = std::ranges::max_element(newCacheEntries, {}, &CacheEntry::id);
-                                          newCacheEntries.end(),
-                                          [](const auto &first, const auto &second) {
-                                              return first.id < second.id;
-                                          });
 
             auto max_id = static_cast<std::size_t>(found->id);
 
@@ -196,12 +184,12 @@ public:
             CacheEntries mergedCacheEntries;
             mergedCacheEntries.reserve(newCacheEntries.size() + m_entries.size());
 
-            std::merge(std::make_move_iterator(m_entries.begin()),
+            std::ranges::merge(std::make_move_iterator(m_entries.begin()),
-                       std::make_move_iterator(m_entries.end()),
+                               std::make_move_iterator(m_entries.end()),
-                       std::make_move_iterator(newCacheEntries.begin()),
+                               std::make_move_iterator(newCacheEntries.begin()),
-                       std::make_move_iterator(newCacheEntries.end()),
+                               std::make_move_iterator(newCacheEntries.end()),
-                       std::back_inserter(mergedCacheEntries),
+                               std::back_inserter(mergedCacheEntries),
-                       less);
+                               Compare{});
 
             m_entries = std::move(mergedCacheEntries);
 
@@ -221,7 +209,7 @@ public:
         sharedLock.unlock();
         std::lock_guard<Mutex> exclusiveLock(m_mutex);
 
-        if (!std::is_base_of<NonLockingMutex, Mutex>::value)
+        if constexpr (!std::is_base_of_v<NonLockingMutex, Mutex>)
             found = find(view);
         if (found == m_entries.end())
             found = insertEntry(found, view, m_storage.fetchId(view));
@@ -235,7 +223,7 @@ public:
         std::vector<IndexType> ids;
         ids.reserve(values.size());
 
-        std::transform(values.begin(), values.end(), std::back_inserter(ids), [&](const auto &values) {
+        std::ranges::transform(values, std::back_inserter(ids), [&](const auto &values) {
             return this->id(values);
         });
 
@@ -252,9 +240,9 @@ public:
         std::shared_lock<Mutex> sharedLock(m_mutex);
 
         if (IndexType::create(static_cast<IndexDatabaseType>(m_indices.size()) + 1) > id) {
-            if (StorageCacheIndex indirectionIndex = m_indices.at(static_cast<std::size_t>(id) - 1);
+            if (StorageCacheIndex indirectionIndex = m_indices[static_cast<std::size_t>(id) - 1];
                 indirectionIndex.isValid()) {
-                return m_entries.at(static_cast<std::size_t>(indirectionIndex)).value;
+                return m_entries[static_cast<std::size_t>(indirectionIndex)].value;
             }
         }
 
@@ -276,9 +264,7 @@ public:
 
         for (IndexType id : ids) {
             values.emplace_back(
-                m_entries
+                m_entries[static_cast<std::size_t>(m_indices[static_cast<std::size_t>(id) - 1])].value);
-                    .at(static_cast<std::size_t>(m_indices.at(static_cast<std::size_t>(id) - 1)))
-                    .value);
         }
         return values;
     }
@@ -306,19 +292,10 @@ private:
     template<typename Entries>
     static auto find(Entries &&entries, ViewType view)
     {
-        auto begin = entries.begin();
+        auto found = std::ranges::lower_bound(entries, view, Compare{});
-        auto end = entries.end();
-        auto found = std::lower_bound(begin, end, view, compare);
 
-        if (found == entries.end()) {
+        if (found != entries.end() && *found == view)
-            return entries.end();
-        }
-
-        const auto &value = *found;
-
-        if (value == view) {
             return found;
-        }
 
         return entries.end();
     }
@@ -329,9 +306,8 @@ private:
 
         auto found = find(view);
 
-        if (found != m_entries.end()) {
+        if (found != m_entries.end())
             return found->id;
-        }
 
         return IndexType();
     }
@@ -341,9 +317,9 @@ private:
         std::shared_lock<Mutex> sharedLock(m_mutex);
 
         if (IndexType::create(static_cast<IndexDatabaseType>(m_indices.size()) + 1) > id) {
-            if (StorageCacheIndex indirectionIndex = m_indices.at(static_cast<std::size_t>(id) - 1);
+            if (StorageCacheIndex indirectionIndex = m_indices[static_cast<std::size_t>(id) - 1];
                 indirectionIndex.isValid()) {
-                return m_entries.at(static_cast<std::size_t>(indirectionIndex)).value;
+                return m_entries[static_cast<std::size_t>(indirectionIndex)].value;
             }
         }
 
@@ -377,7 +353,7 @@ private:
 
         auto indirectionIndex = static_cast<std::size_t>(id) - 1;
         ensureSize(indirectionIndex);
-        m_indices.at(indirectionIndex) = newIndirectionIndex;
+        m_indices[indirectionIndex] = newIndirectionIndex;
 
         return inserted;
     }

src/plugins/qmldesigner/libs/designercore/sourcepathstorage/storagecachefwd.h

@@ -9,12 +9,6 @@ namespace QmlDesigner {
 
 class NonLockingMutex;
 
-template<typename Type,
+template<typename Type, typename ViewType, typename IndexType, typename Storage, typename Mutex, typename Compare, typename CacheEntry>
-         typename ViewType,
-         typename IndexType,
-         typename Storage,
-         typename Mutex,
-         bool (*compare)(ViewType, ViewType),
-         typename CacheEntry>
 class StorageCache;
 } // namespace QmlDesigner

tests/unit/tests/unittests/sourcepathstorage/storagecache-test.cpp

@@ -30,9 +30,12 @@ public:
     ProjectStorageMock &storage;
 };
 
-auto less(Utils::SmallStringView first, Utils::SmallStringView second) -> bool
+struct Less
 {
-    return Utils::compare(first, second) < 0;
+    bool operator()(Utils::SmallStringView first, Utils::SmallStringView second) const
+    {
+        return Utils::compare(first, second) < 0;
+    }
 };
 
 using CacheWithMockLocking = QmlDesigner::StorageCache<Utils::PathString,
@@ -40,7 +43,7 @@ using CacheWithMockLocking = QmlDesigner::StorageCache<Utils::PathString,
                                                        SourceContextId,
                                                        StorageAdapter,
                                                        NiceMock<MockMutex>,
-                                                       less,
+                                                       Less,
                                                        QmlDesigner::Cache::SourceContext>;
 
 using CacheWithoutLocking = QmlDesigner::StorageCache<Utils::PathString,
@@ -48,7 +51,7 @@ using CacheWithoutLocking = QmlDesigner::StorageCache<Utils::PathString,
                                                       SourceContextId,
                                                       StorageAdapter,
                                                       NiceMock<MockMutexNonLocking>,
-                                                      less,
+                                                      Less,
                                                       QmlDesigner::Cache::SourceContext>;
 
 template<typename Cache>