LocatorMatcherTask: Refactor the storage class

Considering the case that a locator task is running
its task in a separate thread, at least 3 threads are
involved:
1. Main thread
2. Task's thread
3. OutputFilter's thread (the thread that removes
   duplicates from tasks' results)

State before this patch:

The task (2) was always delivering the results into
the main thread (1), and when main thread received
the result it was passed to the OutputFilter's thread (3).
The drawback is that we needlessly block the main thread
for intermediate handling.

State after this patch:

The task (2) is delivering the results directly to
the OutputFilter's thread (3), without blocking
the main thread (1).
Currently the common object shared between all 3
threads is OutputDataProvider.

Rename addOutput() into reportOutput().
Make ILocatorFilter::matchers() private.

Change-Id: Ie8ad7d82ae02825e232f65b82fe329e780f58626
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Eike Ziller <eike.ziller@qt.io>

src/plugins/coreplugin/locator/ilocatorfilter.cpp

@@ -93,13 +93,16 @@ public:
         , m_outputData(filterCount, {})
     {}
 
-    void addOutputData(int index, const LocatorFilterEntries &outputData)
+    void reportOutput(int index, const LocatorFilterEntries &outputData)
     {
         QTC_ASSERT(index >= 0, return);
 
         QMutexLocker locker(&m_mutex);
+        // It may happen that the task tree was canceled, while tasks are still running in other
+        // threads and are about to be canceled. In this case we just ignore the call.
+        if (m_state == State::Canceled)
+            return;
         QTC_ASSERT(index < m_filterCount, return);
-        QTC_ASSERT(m_state != State::Canceled, return);
         QTC_ASSERT(!m_outputData.at(index).has_value(), return);
 
         m_outputData[index] = outputData;
@@ -201,12 +204,12 @@ class OutputFilter : public QObject
 public:
     ~OutputFilter();
     void setFilterCount(int count);
-    // When last index is added it ends automatically (asynchronously)
-    void addOutputData(int index, const LocatorFilterEntries &outputData);
     void start();
 
     bool isRunning() const { return m_watcher.get(); }
 
+    std::shared_ptr<OutputDataProvider> dataProvider() const { return m_dataProvider; }
+
 signals:
     void serialOutputDataReady(const LocatorFilterEntries &serialOutputData);
     void done();
@@ -234,13 +237,6 @@ void OutputFilter::setFilterCount(int count)
     m_filterCount = count;
 }
 
-void OutputFilter::addOutputData(int index, const LocatorFilterEntries &outputData)
-{
-    QTC_ASSERT(isRunning(), return);
-
-    m_dataProvider->addOutputData(index, outputData);
-}
-
 void OutputFilter::start()
 {
     QTC_ASSERT(!m_watcher, return);
@@ -284,11 +280,72 @@ QTC_DECLARE_CUSTOM_TASK(Filter, Core::OutputFilterAdapter);
 
 namespace Core {
 
+class LocatorStoragePrivate
+{
+public:
+    LocatorStoragePrivate(const QString &input, int index,
+                          const std::shared_ptr<OutputDataProvider> &dataProvider)
+        : m_input(input)
+        , m_index(index)
+        , m_dataProvider(dataProvider)
+    {}
+
+    QString input() const { return m_input; }
+
+    void reportOutput(const LocatorFilterEntries &outputData)
+    {
+        QMutexLocker locker(&m_mutex);
+        QTC_ASSERT(m_dataProvider, return);
+        reportOutputImpl(outputData);
+    }
+
+    void finalize()
+    {
+        QMutexLocker locker(&m_mutex);
+        if (m_dataProvider)
+            reportOutputImpl({});
+    }
+
+private:
+    // Call me with mutex locked
+    void reportOutputImpl(const LocatorFilterEntries &outputData)
+    {
+        QTC_ASSERT(m_index >= 0, return);
+        m_dataProvider->reportOutput(m_index, outputData);
+        // Deliver results only once for all copies of the storage, drop ref afterwards
+        m_dataProvider.reset();
+    }
+
+    const QString m_input;
+    const int m_index = -1;
+    std::shared_ptr<OutputDataProvider> m_dataProvider;
+    QMutex m_mutex = {};
+};
+
+QString LocatorStorage::input() const
+{
+    QTC_ASSERT(d, return {});
+    return d->input();
+}
+
+void LocatorStorage::reportOutput(const LocatorFilterEntries &outputData) const
+{
+    QTC_ASSERT(d, return);
+    d->reportOutput(outputData);
+}
+
+void LocatorStorage::finalize() const
+{
+    QTC_ASSERT(d, return);
+    d->finalize();
+}
+
 class LocatorMatcherPrivate
 {
 public:
     LocatorMatcherTasks m_tasks;
-    LocatorMatcherTask::Storage m_storage;
+    QString m_input;
+    LocatorFilterEntries m_output;
     int m_parallelLimit = 0;
     std::unique_ptr<TaskTree> m_taskTree;
 };
@@ -305,7 +362,7 @@ void LocatorMatcher::setTasks(const LocatorMatcherTasks &tasks)
 
 void LocatorMatcher::setInputData(const QString &inputData)
 {
-    d->m_storage.input = inputData;
+    d->m_input = inputData;
 }
 
 void LocatorMatcher::setParallelLimit(int limit)
@@ -316,7 +373,7 @@ void LocatorMatcher::setParallelLimit(int limit)
 void LocatorMatcher::start()
 {
     QTC_ASSERT(!isRunning(), return);
-    d->m_storage.output = {};
+    d->m_output = {};
     d->m_taskTree.reset(new TaskTree);
 
     using namespace Tasking;
@@ -332,7 +389,7 @@ void LocatorMatcher::start()
         filter.setFilterCount(filterCount);
         connect(&filter, &OutputFilter::serialOutputDataReady,
                 this, [this](const LocatorFilterEntries &serialOutputData) {
-            d->m_storage.output += serialOutputData;
+            d->m_output += serialOutputData;
             emit serialOutputDataReady(serialOutputData);
         });
     };
@@ -343,27 +400,29 @@ void LocatorMatcher::start()
 
     QList<TaskItem> parallelTasks { ParallelLimit(d->m_parallelLimit) };
 
-    const auto onGroupSetup = [this](const TreeStorage<LocatorMatcherTask::Storage> &storage) {
+    const auto onGroupSetup = [this, filterStorage](const TreeStorage<LocatorStorage> &storage,
-        return [this, storage] { storage->input = d->m_storage.input; };
+                                                    int index) {
-    };
+        return [this, filterStorage, storage, index] {
-    const auto onGroupDone = [filterStorage]
-        (const TreeStorage<LocatorMatcherTask::Storage> &storage, int index) {
-        return [filterStorage, storage, index] {
             OutputFilter *outputFilter = filterStorage->m_filter;
             QTC_ASSERT(outputFilter, return);
-            outputFilter->addOutputData(index, storage->output);
+            *storage = std::make_shared<LocatorStoragePrivate>(d->m_input, index,
+                                                               outputFilter->dataProvider());
         };
     };
 
+    const auto onGroupDone = [](const TreeStorage<LocatorStorage> &storage) {
+        return [storage] { storage->finalize(); };
+    };
+
     int index = 0;
     for (const LocatorMatcherTask &task : std::as_const(d->m_tasks)) {
         const auto storage = task.storage;
         const Group group {
             optional,
             Storage(storage),
-            OnGroupSetup(onGroupSetup(storage)),
+            OnGroupSetup(onGroupSetup(storage, index)),
-            OnGroupDone(onGroupDone(storage, index)),
+            OnGroupDone(onGroupDone(storage)),
-            OnGroupError(onGroupDone(storage, index)),
+            OnGroupError(onGroupDone(storage)),
             task.task
         };
         parallelTasks << group;
@@ -408,7 +467,7 @@ bool LocatorMatcher::isRunning() const
 
 LocatorFilterEntries LocatorMatcher::outputData() const
 {
-    return d->m_storage.output;
+    return d->m_output;
 }
 
 LocatorFilterEntries LocatorMatcher::runBlocking(const LocatorMatcherTasks &tasks,

src/plugins/coreplugin/locator/ilocatorfilter.h

@@ -25,6 +25,7 @@ namespace Core {
 namespace Internal { class Locator; }
 
 class ILocatorFilter;
+class LocatorStoragePrivate;
 
 class AcceptResult
 {
@@ -114,22 +115,29 @@ public:
 
 using LocatorFilterEntries = QList<LocatorFilterEntry>;
 
+class CORE_EXPORT LocatorStorage final
+{
+public:
+    LocatorStorage() = default;
+    QString input() const;
+    void reportOutput(const LocatorFilterEntries &outputData) const;
+
+private:
+    friend class LocatorMatcher;
+    LocatorStorage(const std::shared_ptr<LocatorStoragePrivate> &priv) { d = priv; }
+    void finalize() const;
+    std::shared_ptr<LocatorStoragePrivate> d;
+};
+
 class CORE_EXPORT LocatorMatcherTask final
 {
 public:
-    class Storage
+    // The main task. Initial data (searchTerm) should be taken from storage.input().
-    {
+    // Results reporting is done via the storage.reportOutput().
-    public:
-        QString input;
-        LocatorFilterEntries output;
-    };
-    // The main task. Initial data taken from storage.input field.
-    // Results reporting is done through the storage.output field.
     Utils::Tasking::TaskItem task = Utils::Tasking::Group{};
-    // When setting up the task, take the input data from storage.input field.
+
-    // When task is done, report results by updating the storage.output field.
     // When constructing the task, don't place the storage inside the task above.
-    Utils::Tasking::TreeStorage<Storage> storage;
+    Utils::Tasking::TreeStorage<LocatorStorage> storage;
 };
 
 using LocatorMatcherTasks = QList<LocatorMatcherTask>;
@@ -267,6 +275,9 @@ protected:
     static bool isOldSetting(const QByteArray &state);
 
 private:
+    // TODO: Make pure virtual when all subclasses implement it.
+    virtual LocatorMatcherTasks matchers() { return {}; }
+
     friend class Internal::Locator;
     static const QList<ILocatorFilter *> allLocatorFilters();
 

src/plugins/cppeditor/cpplocatorfilter.cpp

@@ -26,18 +26,19 @@ namespace CppEditor {
 
 using EntryFromIndex = std::function<LocatorFilterEntry(const IndexItem::Ptr &)>;
 
-void matchesFor(QPromise<LocatorFilterEntries> &promise, const QString &entry,
+void matchesFor(QPromise<void> &promise, const LocatorStorage &storage,
                 IndexItem::ItemType wantedType, const EntryFromIndex &converter)
 {
+    const QString input = storage.input();
     LocatorFilterEntries entries[int(ILocatorFilter::MatchLevel::Count)];
-    const Qt::CaseSensitivity caseSensitivityForPrefix = ILocatorFilter::caseSensitivity(entry);
+    const Qt::CaseSensitivity caseSensitivityForPrefix = ILocatorFilter::caseSensitivity(input);
-    const QRegularExpression regexp = ILocatorFilter::createRegExp(entry);
+    const QRegularExpression regexp = ILocatorFilter::createRegExp(input);
     if (!regexp.isValid())
         return;
 
-    const bool hasColonColon = entry.contains("::");
+    const bool hasColonColon = input.contains("::");
     const QRegularExpression shortRegexp = hasColonColon
-            ? ILocatorFilter::createRegExp(entry.mid(entry.lastIndexOf("::") + 2)) : regexp;
+            ? ILocatorFilter::createRegExp(input.mid(input.lastIndexOf("::") + 2)) : regexp;
     CppLocatorData *locatorData = CppModelManager::instance()->locatorData();
     locatorData->filterAllFiles([&](const IndexItem::Ptr &info) {
         if (promise.isCanceled())
@@ -76,9 +77,9 @@ void matchesFor(QPromise<LocatorFilterEntries> &promise, const QString &entry,
 
                 if (matchInParameterList)
                     entries[int(ILocatorFilter::MatchLevel::Normal)].append(filterEntry);
-                else if (filterEntry.displayName.startsWith(entry, caseSensitivityForPrefix))
+                else if (filterEntry.displayName.startsWith(input, caseSensitivityForPrefix))
                     entries[int(ILocatorFilter::MatchLevel::Best)].append(filterEntry);
-                else if (filterEntry.displayName.contains(entry, caseSensitivityForPrefix))
+                else if (filterEntry.displayName.contains(input, caseSensitivityForPrefix))
                     entries[int(ILocatorFilter::MatchLevel::Better)].append(filterEntry);
                 else
                     entries[int(ILocatorFilter::MatchLevel::Good)].append(filterEntry);
@@ -95,7 +96,7 @@ void matchesFor(QPromise<LocatorFilterEntries> &promise, const QString &entry,
             Utils::sort(entry, LocatorFilterEntry::compareLexigraphically);
     }
 
-    promise.addResult(std::accumulate(std::begin(entries), std::end(entries),
+    storage.reportOutput(std::accumulate(std::begin(entries), std::end(entries),
                                       LocatorFilterEntries()));
 }
 
@@ -103,17 +104,13 @@ LocatorMatcherTask locatorMatcher(IndexItem::ItemType type, const EntryFromIndex
 {
     using namespace Tasking;
 
-    TreeStorage<LocatorMatcherTask::Storage> storage;
+    TreeStorage<LocatorStorage> storage;
 
-    const auto onSetup = [=](AsyncTask<LocatorFilterEntries> &async) {
+    const auto onSetup = [=](AsyncTask<void> &async) {
         async.setFutureSynchronizer(Internal::CppEditorPlugin::futureSynchronizer());
-        async.setConcurrentCallData(matchesFor, storage->input, type, converter);
+        async.setConcurrentCallData(matchesFor, *storage, type, converter);
     };
-    const auto onDone = [storage](const AsyncTask<LocatorFilterEntries> &async) {
+    return {Async<void>(onSetup), storage};
-        if (async.isResultAvailable())
-            storage->output = async.result();
-    };
-    return {Async<LocatorFilterEntries>(onSetup, onDone, onDone), storage};
 }
 
 LocatorMatcherTask cppAllSymbolsMatcher()
@@ -200,10 +197,11 @@ LocatorFilterEntry::HighlightInfo highlightInfo(const QRegularExpressionMatch &m
     return LocatorFilterEntry::HighlightInfo(positions.starts, positions.lengths, dataType);
 }
 
-void matchesForCurrentDocument(QPromise<LocatorFilterEntries> &promise,
+void matchesForCurrentDocument(QPromise<void> &promise, const LocatorStorage &storage,
-                               const QString &entry, const FilePath &currentFileName)
+                               const FilePath &currentFileName)
 {
-    const QRegularExpression regexp = FuzzyMatcher::createRegExp(entry, Qt::CaseInsensitive, false);
+    const QString input = storage.input();
+    const QRegularExpression regexp = FuzzyMatcher::createRegExp(input, Qt::CaseInsensitive, false);
     if (!regexp.isValid())
         return;
 
@@ -288,7 +286,7 @@ void matchesForCurrentDocument(QPromise<LocatorFilterEntries> &promise,
             }
         }
     }
-    promise.addResult(Utils::transform(betterEntries,
+    storage.reportOutput(Utils::transform(betterEntries,
                                        [](const Entry &entry) { return entry.entry; }));
 }
 
@@ -302,17 +300,13 @@ LocatorMatcherTask cppCurrentDocumentMatcher()
 {
     using namespace Tasking;
 
-    TreeStorage<LocatorMatcherTask::Storage> storage;
+    TreeStorage<LocatorStorage> storage;
 
-    const auto onSetup = [=](AsyncTask<LocatorFilterEntries> &async) {
+    const auto onSetup = [=](AsyncTask<void> &async) {
         async.setFutureSynchronizer(Internal::CppEditorPlugin::futureSynchronizer());
-        async.setConcurrentCallData(matchesForCurrentDocument, storage->input, currentFileName());
+        async.setConcurrentCallData(matchesForCurrentDocument, *storage, currentFileName());
     };
-    const auto onDone = [storage](const AsyncTask<LocatorFilterEntries> &async) {
+    return {Async<void>(onSetup), storage};
-        if (async.isResultAvailable())
-            storage->output = async.result();
-    };
-    return {Async<LocatorFilterEntries>(onSetup, onDone, onDone), storage};
 }
 
 CppLocatorFilter::CppLocatorFilter()

src/plugins/languageclient/locatorfilter.cpp

@@ -28,12 +28,14 @@ using namespace Utils;
 
 namespace LanguageClient {
 
-void filterResults(QPromise<LocatorFilterEntries> &promise, Client *client,
+void filterResults(QPromise<void> &promise, const LocatorStorage &storage, Client *client,
                    const QList<SymbolInformation> &results, const QList<SymbolKind> &filter)
 {
     const auto doFilter = [&](const SymbolInformation &info) {
         return filter.contains(SymbolKind(info.kind()));
     };
+    if (promise.isCanceled())
+        return;
     const QList<SymbolInformation> filteredResults = filter.isEmpty() ? results
         : Utils::filtered(results, doFilter);
     const auto generateEntry = [client](const SymbolInformation &info) {
@@ -45,7 +47,7 @@ void filterResults(QPromise<LocatorFilterEntries> &promise, Client *client,
         entry.linkForEditor = info.location().toLink(client->hostPathMapper());
         return entry;
     };
-    promise.addResult(Utils::transform(filteredResults, generateEntry));
+    storage.reportOutput(Utils::transform(filteredResults, generateEntry));
 }
 
 LocatorMatcherTask locatorMatcher(Client *client, int maxResultCount,
@@ -53,13 +55,13 @@ LocatorMatcherTask locatorMatcher(Client *client, int maxResultCount,
 {
     using namespace Tasking;
 
-    TreeStorage<LocatorMatcherTask::Storage> storage;
+    TreeStorage<LocatorStorage> storage;
     TreeStorage<QList<SymbolInformation>> resultStorage;
 
     const auto onQuerySetup = [=](WorkspaceSymbolRequestTask &request) {
         request.setClient(client);
         WorkspaceSymbolParams params;
-        params.setQuery(storage->input);
+        params.setQuery(storage->input());
         if (maxResultCount > 0)
             params.setLimit(maxResultCount);
         request.setParams(params);
@@ -71,23 +73,19 @@ LocatorMatcherTask locatorMatcher(Client *client, int maxResultCount,
             *resultStorage = result->toList();
     };
 
-    const auto onFilterSetup = [=](AsyncTask<LocatorFilterEntries> &async) {
+    const auto onFilterSetup = [=](AsyncTask<void> &async) {
         const QList<SymbolInformation> results = *resultStorage;
         if (results.isEmpty())
             return TaskAction::StopWithDone;
         async.setFutureSynchronizer(LanguageClientPlugin::futureSynchronizer());
-        async.setConcurrentCallData(filterResults, client, results, filter);
+        async.setConcurrentCallData(filterResults, *storage, client, results, filter);
         return TaskAction::Continue;
     };
-    const auto onFilterDone = [storage](const AsyncTask<LocatorFilterEntries> &async) {
-        if (async.isResultAvailable())
-            storage->output = async.result();
-    };
 
     const Group root {
         Storage(resultStorage),
         SymbolRequest(onQuerySetup, onQueryDone),
-        Async<LocatorFilterEntries>(onFilterSetup, onFilterDone)
+        Async<void>(onFilterSetup)
     };
     return {root, storage};
 }