From 30c95c937b8b25ee828eedf449327aab0f35bded Mon Sep 17 00:00:00 2001 From: Nikolai Kosjar Date: Wed, 10 Jan 2018 14:54:45 +0100 Subject: [PATCH] Clang: Take over jobs if document gets new project part We could loose jobs if e.g. the user switched to another parse context or shuffled project files between targets/products/project-parts while there were still jobs in the queue. Previously, changing the project part id of a document was a two step process: 1) Unregister document with old project part id 2) Register document with new project part id On 1), we have thrown the document processors (and thus the job queue) away. On 2), we have created a new document. Due to this separation the backend could not take over jobs to the new document (processor) - it could not know that these commands belong together. Now, we avoid the explicit unregister command. On a register command the backend has enough context to find out what to do, it can take over relevant jobs. Task-number: QTCREATORBUG-18856 Change-Id: Ib68a8e62140fcfdb2de58dcd2ae955b4c2e15166 Reviewed-by: Ivan Donchevskii --- .../clangeditordocumentprocessor.cpp | 1 - .../source/clangcodemodelserver.cpp | 82 +++++++++++++++---- .../source/clangcodemodelserver.h | 11 +++ .../source/clangdocumentprocessors.cpp | 22 +++-- .../unittest/clangcodemodelserver-test.cpp | 28 +++++++ 5 files changed, 122 insertions(+), 22 deletions(-) diff --git a/src/plugins/clangcodemodel/clangeditordocumentprocessor.cpp b/src/plugins/clangcodemodel/clangeditordocumentprocessor.cpp index c7294d57c66..cf0ca4b80d1 100644 --- a/src/plugins/clangcodemodel/clangeditordocumentprocessor.cpp +++ b/src/plugins/clangcodemodel/clangeditordocumentprocessor.cpp @@ -435,7 +435,6 @@ void ClangEditorDocumentProcessor::registerTranslationUnitForEditor(CppTools::Pr if (m_projectPart) { if (projectPart->id() == m_projectPart->id()) return; - unregisterTranslationUnitForEditor(); } m_communicator.registerTranslationUnitsForEditor( diff --git a/src/tools/clangbackend/source/clangcodemodelserver.cpp b/src/tools/clangbackend/source/clangcodemodelserver.cpp index 7e004e17d03..185f0f80966 100644 --- a/src/tools/clangbackend/source/clangcodemodelserver.cpp +++ b/src/tools/clangbackend/source/clangcodemodelserver.cpp @@ -84,21 +84,36 @@ void ClangCodeModelServer::end() QCoreApplication::exit(); } +static std::vector operator+(const std::vector &a, + const std::vector &b) +{ + std::vector result = a; + result.insert(result.end(), b.begin(), b.end()); + return result; +} + +// TODO: Rename to createOrUpdate... void ClangCodeModelServer::registerTranslationUnitsForEditor(const ClangBackEnd::RegisterTranslationUnitForEditorMessage &message) { qCDebug(serverLog) << "########## registerTranslationUnitsForEditor"; TIME_SCOPE_DURATION("ClangCodeModelServer::registerTranslationUnitsForEditor"); try { - auto createdDocuments = documents.create(message.fileContainers()); + DocumentResetInfos toReset; + QVector toCreate; + categorizeFileContainers(message.fileContainers(), toCreate, toReset); + + const std::vector createdDocuments = documents.create(toCreate); for (const auto &document : createdDocuments) documentProcessors().create(document); + const std::vector resetDocuments_ = resetDocuments(toReset); + unsavedFiles.createOrUpdate(message.fileContainers()); documents.setUsedByCurrentEditor(message.currentEditorFilePath()); documents.setVisibleInEditors(message.visibleEditorFilePaths()); - processSuspendResumeJobs(documents.documents()); - processInitialJobsForDocuments(createdDocuments); + processSuspendResumeJobs(documents.documents()); + processInitialJobsForDocuments(createdDocuments + resetDocuments_); } catch (const std::exception &exception) { qWarning() << "Error in ClangCodeModelServer::registerTranslationUnitsForEditor:" << exception.what(); } @@ -147,6 +162,14 @@ void ClangCodeModelServer::unregisterTranslationUnitsForEditor(const ClangBackEn } } +static DocumentResetInfos toDocumentResetInfos(const std::vector &documents) +{ + DocumentResetInfos infos; + for (const auto &d : documents) + infos.push_back(DocumentResetInfo{d, d.fileContainer()}); + return infos; +} + void ClangCodeModelServer::registerProjectPartsForEditor(const RegisterProjectPartsForEditorMessage &message) { qCDebug(serverLog) << "########## registerProjectPartsForEditor"; @@ -156,19 +179,7 @@ void ClangCodeModelServer::registerProjectPartsForEditor(const RegisterProjectPa projects.createOrUpdate(message.projectContainers()); std::vector affectedDocuments = documents.setDocumentsDirtyIfProjectPartChanged(); - for (Document &document : affectedDocuments) { - documents.remove({document.fileContainer()}); - - Document newDocument = *documents.create({document.fileContainer()}).begin(); - newDocument.setDirtyIfDependencyIsMet(document.filePath()); - newDocument.setIsUsedByCurrentEditor(document.isUsedByCurrentEditor()); - newDocument.setIsVisibleInEditor(document.isVisibleInEditor(), document.visibleTimePoint()); - newDocument.setResponsivenessIncreaseNeeded(document.isResponsivenessIncreased()); - - documentProcessors().reset(document, newDocument); - - QTC_CHECK(document.useCount() == 1); - } + resetDocuments(toDocumentResetInfos(affectedDocuments)); processJobsForDirtyAndVisibleDocuments(); } catch (const std::exception &exception) { @@ -419,6 +430,45 @@ void ClangCodeModelServer::processSuspendResumeJobs(const std::vector } } +void ClangCodeModelServer::categorizeFileContainers(const QVector &fileContainers, + QVector &toCreate, + DocumentResetInfos &toReset) const +{ + for (const FileContainer &fileContainer : fileContainers) { + const std::vector matching = documents.filtered([&](const Document &document) { + return document.filePath() == fileContainer.filePath(); + }); + if (matching.empty()) + toCreate.push_back(fileContainer); + else + toReset.push_back(DocumentResetInfo{*matching.begin(), fileContainer}); + } +} + +std::vector ClangCodeModelServer::resetDocuments(const DocumentResetInfos &infos) +{ + std::vector newDocuments; + + for (const DocumentResetInfo &info : infos) { + const Document &document = info.documentToRemove; + QTC_CHECK(document.filePath() == info.fileContainer.filePath()); + + documents.remove({document.fileContainer()}); + + Document newDocument = *documents.create({info.fileContainer}).begin(); + newDocument.setDirtyIfDependencyIsMet(document.filePath()); + newDocument.setIsUsedByCurrentEditor(document.isUsedByCurrentEditor()); + newDocument.setIsVisibleInEditor(document.isVisibleInEditor(), document.visibleTimePoint()); + newDocument.setResponsivenessIncreaseNeeded(document.isResponsivenessIncreased()); + + documentProcessors().reset(document, newDocument); + + newDocuments.push_back(newDocument); + } + + return newDocuments; +} + void ClangCodeModelServer::processInitialJobsForDocuments(const std::vector &documents) { for (const auto &document : documents) { diff --git a/src/tools/clangbackend/source/clangcodemodelserver.h b/src/tools/clangbackend/source/clangcodemodelserver.h index 98508f348c5..ee4ecd4847f 100644 --- a/src/tools/clangbackend/source/clangcodemodelserver.h +++ b/src/tools/clangbackend/source/clangcodemodelserver.h @@ -42,6 +42,12 @@ namespace ClangBackEnd { +struct DocumentResetInfo { + Document documentToRemove; + FileContainer fileContainer; +}; +using DocumentResetInfos = QVector; + class ClangCodeModelServer : public ClangCodeModelServerInterface, public IpcClientProvider { @@ -79,6 +85,11 @@ private: void processTimerForVisibleButNotCurrentDocuments(); void processSuspendResumeJobs(const std::vector &documents); + void categorizeFileContainers(const QVector &fileContainers, + QVector &toCreate, + DocumentResetInfos &toReset) const; + std::vector resetDocuments(const DocumentResetInfos &infos); + void addAndRunUpdateJobs(std::vector documents); private: diff --git a/src/tools/clangbackend/source/clangdocumentprocessors.cpp b/src/tools/clangbackend/source/clangdocumentprocessors.cpp index ef17dddee0e..8e5a51d0051 100644 --- a/src/tools/clangbackend/source/clangdocumentprocessors.cpp +++ b/src/tools/clangbackend/source/clangdocumentprocessors.cpp @@ -86,22 +86,34 @@ void DocumentProcessors::remove(const Document &document) throw DocumentProcessorDoesNotExist(document.filePath(), document.projectPart().id()); } +static JobRequests jobsToTakeOver(const JobRequests &jobsStillInQueue, + const Utf8String &updatedProjectPartId) +{ + JobRequests jobs = Utils::filtered(jobsStillInQueue, [](const JobRequest &job) { + return job.isTakeOverable(); + }); + + for (JobRequest &job : jobs) + job.projectPartId = updatedProjectPartId; + + return jobs; +} + void DocumentProcessors::reset(const Document &oldDocument, const Document &newDocument) { // Wait until the currently running jobs finish and remember the not yet // processed job requests for the new processor... - JobRequests jobsToTakeOver = processor(oldDocument).stop(); + const JobRequests jobsStillInQueue = processor(oldDocument).stop(); // ...but do not take over irrelevant ones. - jobsToTakeOver = Utils::filtered(jobsToTakeOver, [](const JobRequest &job){ - return job.isTakeOverable(); - }); + const JobRequests jobsForNewProcessor = jobsToTakeOver(jobsStillInQueue, + newDocument.projectPart().id()); // Remove current processor remove(oldDocument); // Create new processor and take over not yet processed jobs. DocumentProcessor newProcessor = create(newDocument); - for (const JobRequest &job : jobsToTakeOver) + for (const JobRequest &job : jobsForNewProcessor) newProcessor.addJob(job); } diff --git a/tests/unit/unittest/clangcodemodelserver-test.cpp b/tests/unit/unittest/clangcodemodelserver-test.cpp index edcbcff7ade..df4afa8004b 100644 --- a/tests/unit/unittest/clangcodemodelserver-test.cpp +++ b/tests/unit/unittest/clangcodemodelserver-test.cpp @@ -102,6 +102,7 @@ protected: bool waitUntilAllJobsFinished(int timeOutInMs = 10000); void registerProjectPart(); + void registerProjectPart(const Utf8String &projectPartId); void changeProjectPartArguments(); void registerProjectAndFile(const Utf8String &filePath, @@ -111,6 +112,8 @@ protected: void registerProjectAndFilesAndWaitForFinished(int expectedDocumentAnnotationsChangedMessages = 2); void registerFile(const Utf8String &filePath, int expectedDocumentAnnotationsChangedMessages = 1); + void registerFile(const Utf8String &filePath, const Utf8String &projectPartId, + int expectedDocumentAnnotationsChangedMessages = 1); void registerFiles(int expectedDocumentAnnotationsChangedMessages); void registerFileWithUnsavedContent(const Utf8String &filePath, const Utf8String &content); @@ -155,6 +158,7 @@ protected: ClangBackEnd::ClangCodeModelServer clangServer; const ClangBackEnd::Documents &documents = clangServer.documentsForTestOnly(); const Utf8String projectPartId = Utf8StringLiteral("pathToProjectPart.pro"); + const Utf8String projectPartId2 = Utf8StringLiteral("otherPathToProjectPart.pro"); const Utf8String filePathA = Utf8StringLiteral(TESTDATA_DIR"/complete_extractor_function.cpp"); const QString filePathAUnsavedVersion1 @@ -418,6 +422,18 @@ TEST_F(ClangCodeModelServerSlowTest, TakeOverJobsOnProjectPartChange) updateVisibilty(filePathC, filePathC); // Enable processing jobs } +TEST_F(ClangCodeModelServerSlowTest, TakeOverJobsOnProjectPartIdChange) +{ + registerProjectPart(projectPartId); + registerProjectPart(projectPartId2); + registerFile(filePathC, projectPartId, 0); + requestReferences(); + + expectReferences(); + + registerFile(filePathC, projectPartId2); // Here we do not want to loose the RequestReferences job +} + void ClangCodeModelServer::SetUp() { clangServer.setClient(&mockClangCodeModelClient); @@ -452,6 +468,13 @@ void ClangCodeModelServer::registerProjectAndFilesAndWaitForFinished( void ClangCodeModelServer::registerFile(const Utf8String &filePath, int expectedDocumentAnnotationsChangedMessages) +{ + registerFile(filePath, projectPartId, expectedDocumentAnnotationsChangedMessages); +} + +void ClangCodeModelServer::registerFile(const Utf8String &filePath, + const Utf8String &projectPartId, + int expectedDocumentAnnotationsChangedMessages) { const FileContainer fileContainer(filePath, projectPartId); const RegisterTranslationUnitForEditorMessage message({fileContainer}, filePath, {filePath}); @@ -683,6 +706,11 @@ void ClangCodeModelServer::unregisterFile(const Utf8String &filePath) } void ClangCodeModelServer::registerProjectPart() +{ + registerProjectPart(projectPartId); +} + +void ClangCodeModelServer::registerProjectPart(const Utf8String &projectPartId) { RegisterProjectPartsForEditorMessage message({ProjectPartContainer(projectPartId)});