From 054d7d65d2ea136a21090bf3e7672c4954aa0fca Mon Sep 17 00:00:00 2001 From: Jarek Kobus Date: Thu, 22 Sep 2022 13:48:54 +0200 Subject: [PATCH] UnifiedDiffEditor: Move showing diff into separate thread Before, when all the data was finished, we called showDiff() in main thread. This consisted of 2 parts: 1. Calculating some extra data and generating actual text for UnifiedDiffEditor out of input data. 2. Calling setPlainText() with generated text. For a really big diffs this could freeze the main thread for a couple of seconds. Like e.g. 05c35356abc31549c5db6eba31fb608c0365c2a0 (initial Creator import) - it contained 7 million characters, part 1. took about 500 ms and part 2. took about 2.5 seconds. This two tasks are now done in separate thread. However, since we can't call TextEditorWidget::setPlainText() directly from non-GUI thread, we create a separate TextDocument object in the worker thread, fill it with generated diff input and move the TextDocument object into the main thread as a result of async computation. In main thread we replace TextDocument object of the TextEditorWidget with the one generated in other thread. This replacement is very fast. Change-Id: I49a717ced1dc2d5b8946e0fd6bee244b25071f35 Reviewed-by: David Schulz Reviewed-by: Orgad Shaneh --- src/plugins/diffeditor/diffeditorplugin.cpp | 24 ++- src/plugins/diffeditor/diffeditorplugin.h | 12 +- .../diffeditor/diffeditorwidgetcontroller.cpp | 43 ++++-- .../diffeditor/diffeditorwidgetcontroller.h | 5 + .../diffeditor/unifieddiffeditorwidget.cpp | 145 +++++++++++++++--- .../diffeditor/unifieddiffeditorwidget.h | 16 +- 6 files changed, 206 insertions(+), 39 deletions(-) diff --git a/src/plugins/diffeditor/diffeditorplugin.cpp b/src/plugins/diffeditor/diffeditorplugin.cpp index 2b22500a2ba..7d97880357e 100644 --- a/src/plugins/diffeditor/diffeditorplugin.cpp +++ b/src/plugins/diffeditor/diffeditorplugin.cpp @@ -2,7 +2,6 @@ // SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0+ OR GPL-3.0 WITH Qt-GPL-exception-1.0 #include "diffeditorplugin.h" -#include "diffeditor.h" #include "diffeditorconstants.h" #include "diffeditorcontroller.h" #include "diffeditordocument.h" @@ -24,10 +23,10 @@ #include #include -#include "texteditor/texteditoractionhandler.h" #include #include +#include #include #include @@ -440,12 +439,14 @@ public: QAction *m_diffCurrentFileAction = nullptr; QAction *m_diffOpenFilesAction = nullptr; - DiffEditorFactory editorFactory; - DiffEditorServiceImpl service; + DiffEditorFactory m_editorFactory; + DiffEditorServiceImpl m_service; + FutureSynchronizer m_futureSynchronizer; }; DiffEditorPluginPrivate::DiffEditorPluginPrivate() { + m_futureSynchronizer.setCancelOnWait(true); //register actions ActionContainer *toolsContainer = ActionManager::actionContainer(Core::Constants::M_TOOLS); @@ -571,9 +572,17 @@ void DiffEditorPluginPrivate::diffExternalFiles() document->reload(); } +static DiffEditorPlugin *s_instance = nullptr; + +DiffEditorPlugin::DiffEditorPlugin() +{ + s_instance = this; +} + DiffEditorPlugin::~DiffEditorPlugin() { delete d; + s_instance = nullptr; } bool DiffEditorPlugin::initialize(const QStringList &arguments, QString *errorMessage) @@ -586,6 +595,13 @@ bool DiffEditorPlugin::initialize(const QStringList &arguments, QString *errorMe return true; } +void DiffEditorPlugin::addFuture(const QFuture &future) +{ + QTC_ASSERT(s_instance, return); + s_instance->d->m_futureSynchronizer.addFuture(future); +} + + } // namespace Internal } // namespace DiffEditor diff --git a/src/plugins/diffeditor/diffeditorplugin.h b/src/plugins/diffeditor/diffeditorplugin.h index 8c33b26c12e..0cf10548bad 100644 --- a/src/plugins/diffeditor/diffeditorplugin.h +++ b/src/plugins/diffeditor/diffeditorplugin.h @@ -8,6 +8,11 @@ #include #include +QT_BEGIN_NAMESPACE +template +class QFuture; +QT_END_NAMESPACE + namespace DiffEditor { namespace Internal { @@ -29,10 +34,15 @@ class DiffEditorPlugin final : public ExtensionSystem::IPlugin Q_PLUGIN_METADATA(IID "org.qt-project.Qt.QtCreatorPlugin" FILE "DiffEditor.json") public: - ~DiffEditorPlugin() final; + DiffEditorPlugin(); + ~DiffEditorPlugin(); bool initialize(const QStringList &arguments, QString *errorMessage) final; + template + static void addFuture(const QFuture &future) { addFuture(QFuture(future)); } + static void addFuture(const QFuture &future); + private: class DiffEditorPluginPrivate *d = nullptr; diff --git a/src/plugins/diffeditor/diffeditorwidgetcontroller.cpp b/src/plugins/diffeditor/diffeditorwidgetcontroller.cpp index 4a6545ef4b1..c631cb5a513 100644 --- a/src/plugins/diffeditor/diffeditorwidgetcontroller.cpp +++ b/src/plugins/diffeditor/diffeditorwidgetcontroller.cpp @@ -43,6 +43,23 @@ DiffEditorWidgetController::DiffEditorWidgetController(QWidget *diffEditorWidget connect(&m_timer, &QTimer::timeout, this, &DiffEditorWidgetController::showProgress); } +bool DiffEditorWidgetController::isInProgress() const +{ + return m_isBusyShowing || (m_document && m_document->state() == DiffEditorDocument::Reloading); +} + +void DiffEditorWidgetController::toggleProgress(bool wasInProgress) +{ + const bool inProgress = isInProgress(); + if (wasInProgress == inProgress) + return; + + if (inProgress) + scheduleShowProgress(); + else + hideProgress(); +} + void DiffEditorWidgetController::setDocument(DiffEditorDocument *document) { if (!m_progressIndicator) { @@ -59,25 +76,16 @@ void DiffEditorWidgetController::setDocument(DiffEditorDocument *document) disconnect(m_document, &IDocument::reloadFinished, this, &DiffEditorWidgetController::onDocumentReloadFinished); } - const bool wasRunning = m_document && m_document->state() == DiffEditorDocument::Reloading; + const bool wasInProgress = isInProgress(); m_document = document; - if (m_document) { connect(m_document, &IDocument::aboutToReload, this, &DiffEditorWidgetController::scheduleShowProgress); connect(m_document, &IDocument::reloadFinished, this, &DiffEditorWidgetController::onDocumentReloadFinished); updateCannotDecodeInfo(); } - const bool isRunning = m_document && m_document->state() == DiffEditorDocument::Reloading; - - if (wasRunning == isRunning) - return; - - if (isRunning) - scheduleShowProgress(); - else - hideProgress(); + toggleProgress(wasInProgress); } DiffEditorDocument *DiffEditorWidgetController::document() const @@ -85,6 +93,16 @@ DiffEditorDocument *DiffEditorWidgetController::document() const return m_document; } +void DiffEditorWidgetController::setBusyShowing(bool busy) +{ + if (m_isBusyShowing == busy) + return; + + const bool wasInProgress = isInProgress(); + m_isBusyShowing = busy; + toggleProgress(wasInProgress); +} + void DiffEditorWidgetController::scheduleShowProgress() { m_timer.start(); @@ -107,7 +125,8 @@ void DiffEditorWidgetController::hideProgress() void DiffEditorWidgetController::onDocumentReloadFinished() { updateCannotDecodeInfo(); - hideProgress(); + if (!isInProgress()) + hideProgress(); } void DiffEditorWidgetController::patch(bool revert, int fileIndex, int chunkIndex) diff --git a/src/plugins/diffeditor/diffeditorwidgetcontroller.h b/src/plugins/diffeditor/diffeditorwidgetcontroller.h index ab0d3269b9a..662e3462ae7 100644 --- a/src/plugins/diffeditor/diffeditorwidgetcontroller.h +++ b/src/plugins/diffeditor/diffeditorwidgetcontroller.h @@ -42,6 +42,7 @@ public: void addRevertAction(QMenu *menu, int fileIndex, int chunkIndex); void addExtraActions(QMenu *menu, int fileIndex, int chunkIndex, const ChunkSelection &selection); void updateCannotDecodeInfo(); + void setBusyShowing(bool busy); ChunkData chunkData(int fileIndex, int chunkIndex) const; @@ -56,6 +57,9 @@ public: QTextCharFormat m_rightCharFormat; private: + bool isInProgress() const; + void toggleProgress(bool wasInProgress); + void patch(bool revert, int fileIndex, int chunkIndex); void sendChunkToCodePaster(int fileIndex, int chunkIndex); bool chunkExists(int fileIndex, int chunkIndex) const; @@ -70,6 +74,7 @@ private: DiffEditorDocument *m_document = nullptr; + bool m_isBusyShowing = false; Utils::ProgressIndicator *m_progressIndicator = nullptr; QTimer m_timer; }; diff --git a/src/plugins/diffeditor/unifieddiffeditorwidget.cpp b/src/plugins/diffeditor/unifieddiffeditorwidget.cpp index 1347f44b61d..6c878b6f061 100644 --- a/src/plugins/diffeditor/unifieddiffeditorwidget.cpp +++ b/src/plugins/diffeditor/unifieddiffeditorwidget.cpp @@ -5,6 +5,7 @@ #include "diffeditorconstants.h" #include "diffeditordocument.h" +#include "diffeditorplugin.h" #include "diffutils.h" #include @@ -14,6 +15,7 @@ #include #include +#include #include #include @@ -21,6 +23,8 @@ #include #include +#include +#include #include using namespace Core; @@ -34,17 +38,18 @@ UnifiedDiffEditorWidget::UnifiedDiffEditorWidget(QWidget *parent) : SelectableTextEditorWidget("DiffEditor.UnifiedDiffEditor", parent) , m_controller(this) { + setReadOnly(true); + DisplaySettings settings = displaySettings(); settings.m_textWrapping = false; settings.m_displayLineNumbers = true; settings.m_markTextChanges = false; settings.m_highlightBlocks = false; SelectableTextEditorWidget::setDisplaySettings(settings); - - setReadOnly(true); connect(TextEditorSettings::instance(), &TextEditorSettings::displaySettingsChanged, this, &UnifiedDiffEditorWidget::setDisplaySettings); setDisplaySettings(TextEditorSettings::displaySettings()); + setCodeStyle(TextEditorSettings::codeStyle()); connect(TextEditorSettings::instance(), &TextEditorSettings::fontSettingsChanged, @@ -63,14 +68,20 @@ UnifiedDiffEditorWidget::UnifiedDiffEditorWidget(QWidget *parent) setCodeFoldingSupported(true); } +UnifiedDiffEditorWidget::~UnifiedDiffEditorWidget() +{ + if (m_watcher) { + m_watcher->cancel(); + DiffEditorPlugin::addFuture(m_watcher->future()); + } +} + void UnifiedDiffEditorWidget::setDocument(DiffEditorDocument *document) { + m_controller.setBusyShowing(true); m_controller.setDocument(document); clear(); - QList diffFileList; - if (document) - diffFileList = document->diffFiles(); - setDiff(diffFileList); + setDiff(document ? document->diffFiles() : QList()); } DiffEditorDocument *UnifiedDiffEditorWidget::diffDocument() const @@ -113,11 +124,15 @@ void UnifiedDiffEditorWidget::setFontSettings(const FontSettings &fontSettings) void UnifiedDiffEditorWidget::slotCursorPositionChangedInEditor() { + const int fileIndex = fileIndexForBlockNumber(textCursor().blockNumber()); + if (fileIndex < 0) + return; + if (m_controller.m_ignoreChanges.isLocked()) return; const GuardLocker locker(m_controller.m_ignoreChanges); - emit currentDiffFileIndexChanged(fileIndexForBlockNumber(textCursor().blockNumber())); + emit currentDiffFileIndexChanged(fileIndex); } void UnifiedDiffEditorWidget::mouseDoubleClickEvent(QMouseEvent *e) @@ -214,6 +229,12 @@ void UnifiedDiffEditorWidget::clear(const QString &message) { m_data = {}; setSelections({}); + if (m_watcher) { + m_watcher->cancel(); + DiffEditorPlugin::addFuture(m_watcher->future()); + m_watcher.reset(); + m_controller.setBusyShowing(false); + } const GuardLocker locker(m_controller.m_ignoreChanges); SelectableTextEditorWidget::clear(); @@ -279,7 +300,7 @@ void UnifiedDiffData::setChunkIndex(int startBlockNumber, int blockCount, int ch void UnifiedDiffEditorWidget::setDiff(const QList &diffFileList) { const GuardLocker locker(m_controller.m_ignoreChanges); - clear(); + clear(tr("Waiting for data...")); m_controller.m_contextFileData = diffFileList; showDiff(); } @@ -457,12 +478,28 @@ QString UnifiedDiffData::showChunk(const DiffEditorInput &input, const ChunkData return diffText; } -UnifiedDiffOutput UnifiedDiffData::showDiff(const DiffEditorInput &input) +static int interpolate(int x, int x1, int x2, int y1, int y2) +{ + if (x1 == x2) + return x1; + if (x == x1) + return y1; + if (x == x2) + return y2; + const int numerator = (y2 - y1) * x + x2 * y1 - x1 * y2; + const int denominator = x2 - x1; + return qRound((double)numerator / denominator); +} + +UnifiedDiffOutput UnifiedDiffData::showDiff(QFutureInterface &fi, int progressMin, + int progressMax, const DiffEditorInput &input) { UnifiedDiffOutput output; int blockNumber = 0; int charNumber = 0; + int i = 0; + const int count = input.m_contextFileData.size(); for (const FileData &fileData : qAsConst(input.m_contextFileData)) { const QString leftFileInfo = "--- " + fileData.leftFileInfo.fileName + '\n'; @@ -504,6 +541,9 @@ UnifiedDiffOutput UnifiedDiffData::showDiff(const DiffEditorInput &input) setChunkIndex(oldBlockNumber, blockNumber - oldBlockNumber, j); } } + fi.setProgressValue(interpolate(++i, 0, count, progressMin, progressMax)); + if (fi.isCanceled()) + return {}; } output.diffText.replace('\r', ' '); @@ -512,24 +552,87 @@ UnifiedDiffOutput UnifiedDiffData::showDiff(const DiffEditorInput &input) void UnifiedDiffEditorWidget::showDiff() { - const DiffEditorInput input = {&m_controller}; - const UnifiedDiffOutput output = m_data.showDiff(input); - - if (output.diffText.isEmpty()) { + if (m_controller.m_contextFileData.isEmpty()) { setPlainText(tr("No difference.")); return; } - { - const GuardLocker locker(m_controller.m_ignoreChanges); - setPlainText(output.diffText); + m_watcher.reset(new QFutureWatcher()); + m_controller.setBusyShowing(true); + connect(m_watcher.get(), &QFutureWatcherBase::finished, this, [this] { + if (m_watcher->isCanceled()) { + setPlainText(tr("Retrieving data failed.")); + } else { + const ShowResult result = m_watcher->result(); + m_data = result.diffData; + TextDocumentPtr doc(result.textDocument); + { + const GuardLocker locker(m_controller.m_ignoreChanges); + // TextDocument was living in no thread, so it's safe to pull it + doc->moveToThread(thread()); + setTextDocument(doc); - QTextBlock block = document()->firstBlock(); - for (int b = 0; block.isValid(); block = block.next(), ++b) - setFoldingIndent(block, output.foldingIndent.value(b, 3)); - } + setReadOnly(true); - setSelections(output.selections); + QTextBlock block = document()->firstBlock(); + for (int b = 0; block.isValid(); block = block.next(), ++b) + setFoldingIndent(block, result.foldingIndent.value(b, 3)); + } + // TODO: this could probably be done in thread, too + setSelections(result.selections); + } + m_watcher.release()->deleteLater(); + m_controller.setBusyShowing(false); + }); + + const DiffEditorInput input(&m_controller); + + auto getDocument = [=](QFutureInterface &futureInterface) { + auto cleanup = qScopeGuard([&futureInterface] { + if (futureInterface.isCanceled()) + futureInterface.reportCanceled(); + }); + const int progressMax = 100; + const int firstPartMax = 20; // showDiff is about 4 times quicker than filling document + futureInterface.setProgressRange(0, progressMax); + futureInterface.setProgressValue(0); + QFutureInterface fi = futureInterface; + UnifiedDiffData diffData; + const UnifiedDiffOutput output = diffData.showDiff(fi, 0, firstPartMax, input); + if (futureInterface.isCanceled()) + return; + + const ShowResult result = {TextDocumentPtr(new TextDocument("DiffEditor.UnifiedDiffEditor")), + diffData, output.foldingIndent, output.selections}; + // No need to store the change history + result.textDocument->document()->setUndoRedoEnabled(false); + + // We could do just: + // result.textDocument->setPlainText(output.diffText); + // but this would freeze the thread for couple of seconds without progress reporting + // and without checking for canceled. + const int diffSize = output.diffText.size(); + const int packageSize = 100000; + int currentPos = 0; + QTextCursor cursor(result.textDocument->document()); + while (currentPos < diffSize) { + const QString package = output.diffText.mid(currentPos, packageSize); + cursor.insertText(package); + currentPos += package.size(); + fi.setProgressValue(interpolate(currentPos, 0, diffSize, firstPartMax, progressMax)); + if (futureInterface.isCanceled()) + return; + } + + // If future was canceled, the destructor runs in this thread, so we can't move it + // to caller's thread. We push it to no thread (make object to have no thread affinity), + // and later, in the caller's thread, we pull it back to the caller's thread. + result.textDocument->moveToThread(nullptr); + futureInterface.reportResult(result); + }; + + m_watcher->setFuture(runAsync(getDocument)); + ProgressManager::addTask(m_watcher->future(), tr("Rendering diff"), "DiffEditor"); } int UnifiedDiffEditorWidget::blockNumberForFileIndex(int fileIndex) const diff --git a/src/plugins/diffeditor/unifieddiffeditorwidget.h b/src/plugins/diffeditor/unifieddiffeditorwidget.h index e90438332df..7237ef60bb0 100644 --- a/src/plugins/diffeditor/unifieddiffeditorwidget.h +++ b/src/plugins/diffeditor/unifieddiffeditorwidget.h @@ -6,6 +6,8 @@ #include "selectabletexteditorwidget.h" #include "diffeditorwidgetcontroller.h" +#include + namespace Core { class IContext; } namespace TextEditor { @@ -38,7 +40,8 @@ public: class UnifiedDiffData { public: - UnifiedDiffOutput showDiff(const DiffEditorInput &input); + UnifiedDiffOutput showDiff(QFutureInterface &fi, int progressMin, int progressMax, + const DiffEditorInput &input); // block number, visual line number, chunk row number QMap> m_leftLineNumbers; @@ -67,6 +70,7 @@ class UnifiedDiffEditorWidget final : public SelectableTextEditorWidget Q_OBJECT public: UnifiedDiffEditorWidget(QWidget *parent = nullptr); + ~UnifiedDiffEditorWidget(); void setDocument(DiffEditorDocument *document); DiffEditorDocument *diffDocument() const; @@ -110,6 +114,16 @@ private: UnifiedDiffData m_data; DiffEditorWidgetController m_controller; QByteArray m_state; + + struct ShowResult + { + QSharedPointer textDocument; + UnifiedDiffData diffData; + QHash foldingIndent; + QMap> selections; + }; + + std::unique_ptr> m_watcher; }; } // namespace Internal