From 0ab1eaa31c3602666742cd2a422e8db7e10da9dc Mon Sep 17 00:00:00 2001 From: Jarek Kobus Date: Thu, 13 Apr 2023 18:18:02 +0200 Subject: [PATCH] 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: Reviewed-by: Qt CI Bot Reviewed-by: Eike Ziller --- .../coreplugin/locator/ilocatorfilter.cpp | 111 ++++++++++++++---- .../coreplugin/locator/ilocatorfilter.h | 33 ++++-- src/plugins/cppeditor/cpplocatorfilter.cpp | 50 ++++---- src/plugins/languageclient/locatorfilter.cpp | 20 ++-- 4 files changed, 138 insertions(+), 76 deletions(-) diff --git a/src/plugins/coreplugin/locator/ilocatorfilter.cpp b/src/plugins/coreplugin/locator/ilocatorfilter.cpp index 9628fc49fde..bb4795fcc16 100644 --- a/src/plugins/coreplugin/locator/ilocatorfilter.cpp +++ b/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 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 &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 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 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 parallelTasks { ParallelLimit(d->m_parallelLimit) }; - const auto onGroupSetup = [this](const TreeStorage &storage) { - return [this, storage] { storage->input = d->m_storage.input; }; - }; - const auto onGroupDone = [filterStorage] - (const TreeStorage &storage, int index) { - return [filterStorage, storage, index] { + const auto onGroupSetup = [this, filterStorage](const TreeStorage &storage, + int index) { + return [this, filterStorage, storage, index] { OutputFilter *outputFilter = filterStorage->m_filter; QTC_ASSERT(outputFilter, return); - outputFilter->addOutputData(index, storage->output); + *storage = std::make_shared(d->m_input, index, + outputFilter->dataProvider()); }; }; + const auto onGroupDone = [](const TreeStorage &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)), - OnGroupDone(onGroupDone(storage, index)), - OnGroupError(onGroupDone(storage, index)), + OnGroupSetup(onGroupSetup(storage, index)), + OnGroupDone(onGroupDone(storage)), + 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, diff --git a/src/plugins/coreplugin/locator/ilocatorfilter.h b/src/plugins/coreplugin/locator/ilocatorfilter.h index e89bdc0885c..e2866d17519 100644 --- a/src/plugins/coreplugin/locator/ilocatorfilter.h +++ b/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; +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 &priv) { d = priv; } + void finalize() const; + std::shared_ptr d; +}; + class CORE_EXPORT LocatorMatcherTask final { public: - class Storage - { - public: - QString input; - LocatorFilterEntries output; - }; - // The main task. Initial data taken from storage.input field. - // Results reporting is done through the storage.output field. + // The main task. Initial data (searchTerm) should be taken from storage.input(). + // Results reporting is done via the storage.reportOutput(). 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; + Utils::Tasking::TreeStorage storage; }; using LocatorMatcherTasks = QList; @@ -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 allLocatorFilters(); diff --git a/src/plugins/cppeditor/cpplocatorfilter.cpp b/src/plugins/cppeditor/cpplocatorfilter.cpp index 13320c2ae62..0435f03c9ae 100644 --- a/src/plugins/cppeditor/cpplocatorfilter.cpp +++ b/src/plugins/cppeditor/cpplocatorfilter.cpp @@ -26,18 +26,19 @@ namespace CppEditor { using EntryFromIndex = std::function; -void matchesFor(QPromise &promise, const QString &entry, +void matchesFor(QPromise &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 QRegularExpression regexp = ILocatorFilter::createRegExp(entry); + const Qt::CaseSensitivity caseSensitivityForPrefix = ILocatorFilter::caseSensitivity(input); + 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 &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 &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 storage; + TreeStorage storage; - const auto onSetup = [=](AsyncTask &async) { + const auto onSetup = [=](AsyncTask &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 &async) { - if (async.isResultAvailable()) - storage->output = async.result(); - }; - return {Async(onSetup, onDone, onDone), storage}; + return {Async(onSetup), storage}; } LocatorMatcherTask cppAllSymbolsMatcher() @@ -200,10 +197,11 @@ LocatorFilterEntry::HighlightInfo highlightInfo(const QRegularExpressionMatch &m return LocatorFilterEntry::HighlightInfo(positions.starts, positions.lengths, dataType); } -void matchesForCurrentDocument(QPromise &promise, - const QString &entry, const FilePath ¤tFileName) +void matchesForCurrentDocument(QPromise &promise, const LocatorStorage &storage, + const FilePath ¤tFileName) { - 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 &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 storage; + TreeStorage storage; - const auto onSetup = [=](AsyncTask &async) { + const auto onSetup = [=](AsyncTask &async) { async.setFutureSynchronizer(Internal::CppEditorPlugin::futureSynchronizer()); - async.setConcurrentCallData(matchesForCurrentDocument, storage->input, currentFileName()); + async.setConcurrentCallData(matchesForCurrentDocument, *storage, currentFileName()); }; - const auto onDone = [storage](const AsyncTask &async) { - if (async.isResultAvailable()) - storage->output = async.result(); - }; - return {Async(onSetup, onDone, onDone), storage}; + return {Async(onSetup), storage}; } CppLocatorFilter::CppLocatorFilter() diff --git a/src/plugins/languageclient/locatorfilter.cpp b/src/plugins/languageclient/locatorfilter.cpp index a0229b11869..0fe00c23879 100644 --- a/src/plugins/languageclient/locatorfilter.cpp +++ b/src/plugins/languageclient/locatorfilter.cpp @@ -28,12 +28,14 @@ using namespace Utils; namespace LanguageClient { -void filterResults(QPromise &promise, Client *client, +void filterResults(QPromise &promise, const LocatorStorage &storage, Client *client, const QList &results, const QList &filter) { const auto doFilter = [&](const SymbolInformation &info) { return filter.contains(SymbolKind(info.kind())); }; + if (promise.isCanceled()) + return; const QList filteredResults = filter.isEmpty() ? results : Utils::filtered(results, doFilter); const auto generateEntry = [client](const SymbolInformation &info) { @@ -45,7 +47,7 @@ void filterResults(QPromise &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 storage; + TreeStorage storage; TreeStorage> 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 &async) { + const auto onFilterSetup = [=](AsyncTask &async) { const QList 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 &async) { - if (async.isResultAvailable()) - storage->output = async.result(); - }; const Group root { Storage(resultStorage), SymbolRequest(onQuerySetup, onQueryDone), - Async(onFilterSetup, onFilterDone) + Async(onFilterSetup) }; return {root, storage}; }