Author: ibiryukov Date: Mon Jul 9 03:45:33 2018 New Revision: 336538 URL: http://llvm.org/viewvc/llvm-project?rev=336538&view=rev Log: [clangd] Wait for first preamble before code completion
Summary: To avoid doing extra work of processing headers in the preamble mutilple times in parallel. Reviewers: sammccall Reviewed By: sammccall Subscribers: javed.absar, ioeric, MaskRay, jkorous, cfe-commits Differential Revision: https://reviews.llvm.org/D48940 Modified: clang-tools-extra/trunk/clangd/TUScheduler.cpp clang-tools-extra/trunk/clangd/TUScheduler.h clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp Modified: clang-tools-extra/trunk/clangd/TUScheduler.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.cpp?rev=336538&r1=336537&r2=336538&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/TUScheduler.cpp (original) +++ clang-tools-extra/trunk/clangd/TUScheduler.cpp Mon Jul 9 03:45:33 2018 @@ -176,6 +176,12 @@ public: bool blockUntilIdle(Deadline Timeout) const; std::shared_ptr<const PreambleData> getPossiblyStalePreamble() const; + /// Wait for the first build of preamble to finish. Preamble itself can be + /// accessed via getPossibleStalePreamble(). Note that this function will + /// return after an unsuccessful build of the preamble too, i.e. result of + /// getPossiblyStalePreamble() can be null even after this function returns. + void waitForFirstPreamble() const; + std::size_t getUsedBytes() const; bool isASTCached() const; @@ -226,6 +232,8 @@ private: /// Guards members used by both TUScheduler and the worker thread. mutable std::mutex Mutex; std::shared_ptr<const PreambleData> LastBuiltPreamble; /* GUARDED_BY(Mutex) */ + /// Becomes ready when the first preamble build finishes. + Notification PreambleWasBuilt; /// Set to true to signal run() to finish processing. bool Done; /* GUARDED_BY(Mutex) */ std::deque<Request> Requests; /* GUARDED_BY(Mutex) */ @@ -329,6 +337,9 @@ void ASTWorker::update( buildCompilerInvocation(Inputs); if (!Invocation) { log("Could not build CompilerInvocation for file " + FileName); + // Make sure anyone waiting for the preamble gets notified it could not + // be built. + PreambleWasBuilt.notify(); return; } @@ -340,6 +351,8 @@ void ASTWorker::update( if (NewPreamble) LastBuiltPreamble = NewPreamble; } + PreambleWasBuilt.notify(); + // Build the AST for diagnostics. llvm::Optional<ParsedAST> AST = buildAST(FileName, std::move(Invocation), Inputs, NewPreamble, PCHs); @@ -392,6 +405,10 @@ ASTWorker::getPossiblyStalePreamble() co return LastBuiltPreamble; } +void ASTWorker::waitForFirstPreamble() const { + PreambleWasBuilt.wait(); +} + std::size_t ASTWorker::getUsedBytes() const { // Note that we don't report the size of ASTs currently used for processing // the in-flight requests. We used this information for debugging purposes @@ -655,6 +672,11 @@ void TUScheduler::runWithPreamble( std::string Contents, tooling::CompileCommand Command, Context Ctx, decltype(Action) Action) mutable { + // We don't want to be running preamble actions before the preamble was + // built for the first time. This avoids extra work of processing the + // preamble headers in parallel multiple times. + Worker->waitForFirstPreamble(); + std::lock_guard<Semaphore> BarrierLock(Barrier); WithContext Guard(std::move(Ctx)); trace::Span Tracer(Name); Modified: clang-tools-extra/trunk/clangd/TUScheduler.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.h?rev=336538&r1=336537&r2=336538&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/TUScheduler.h (original) +++ clang-tools-extra/trunk/clangd/TUScheduler.h Mon Jul 9 03:45:33 2018 @@ -101,6 +101,9 @@ public: /// - validate that the preamble is still valid, and only use it in this case /// - accept that preamble contents may be outdated, and try to avoid reading /// source code from headers. + /// If there's no preamble yet (because the file was just opened), we'll wait + /// for it to build. The preamble may still be null if it fails to build or is + /// empty. /// If an error occurs during processing, it is forwarded to the \p Action /// callback. void runWithPreamble(llvm::StringRef Name, PathRef File, Modified: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp?rev=336538&r1=336537&r2=336538&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp (original) +++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp Mon Jul 9 03:45:33 2018 @@ -19,6 +19,7 @@ namespace clang { namespace clangd { using ::testing::_; +using ::testing::Each; using ::testing::AnyOf; using ::testing::Pair; using ::testing::Pointee; @@ -299,5 +300,40 @@ TEST_F(TUSchedulerTests, EvictedAST) { UnorderedElementsAre(Foo, AnyOf(Bar, Baz))); } +TEST_F(TUSchedulerTests, RunWaitsForPreamble) { + // Testing strategy: we update the file and schedule a few preamble reads at + // the same time. All reads should get the same non-null preamble. + TUScheduler S( + /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, + PreambleParsedCallback(), + /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), + ASTRetentionPolicy()); + auto Foo = testPath("foo.cpp"); + auto NonEmptyPreamble = R"cpp( + #define FOO 1 + #define BAR 2 + + int main() {} + )cpp"; + constexpr int ReadsToSchedule = 10; + std::mutex PreamblesMut; + std::vector<const void *> Preambles(ReadsToSchedule, nullptr); + S.update(Foo, getInputs(Foo, NonEmptyPreamble), WantDiagnostics::Auto, + [](std::vector<Diag>) {}); + for (int I = 0; I < ReadsToSchedule; ++I) { + S.runWithPreamble( + "test", Foo, + [I, &PreamblesMut, &Preambles](llvm::Expected<InputsAndPreamble> IP) { + std::lock_guard<std::mutex> Lock(PreamblesMut); + Preambles[I] = cantFail(std::move(IP)).Preamble; + }); + } + ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); + // Check all actions got the same non-null preamble. + std::lock_guard<std::mutex> Lock(PreamblesMut); + ASSERT_NE(Preambles[0], nullptr); + ASSERT_THAT(Preambles, Each(Preambles[0])); +} + } // namespace clangd } // namespace clang _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
