diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -150,6 +150,9 @@ /// Enable notification-based semantic highlighting. bool TheiaSemanticHighlighting = false; + /// Whether to run PreamblePeer asynchronously. + bool AsyncPreambleBuilds = false; + /// Returns true if the tweak should be enabled. std::function TweakFilter = [](const Tweak &T) { return !T.hidden(); // only enable non-hidden tweaks. diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -114,6 +114,7 @@ Opts.StorePreamblesInMemory = true; Opts.AsyncThreadsCount = 4; // Consistent! Opts.TheiaSemanticHighlighting = true; + Opts.AsyncPreambleBuilds = true; return Opts; } @@ -123,6 +124,7 @@ Opts.RetentionPolicy = RetentionPolicy; Opts.StorePreamblesInMemory = StorePreamblesInMemory; Opts.UpdateDebounce = UpdateDebounce; + Opts.AsyncPreambleBuilds = AsyncPreambleBuilds; return Opts; } diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h --- a/clang-tools-extra/clangd/TUScheduler.h +++ b/clang-tools-extra/clangd/TUScheduler.h @@ -191,6 +191,10 @@ /// Determines when to keep idle ASTs in memory for future use. ASTRetentionPolicy RetentionPolicy; + + /// Whether to run PreamblePeer asynchronously. No-op if AsyncThreadsCount + /// is 0. + bool AsyncPreambleBuilds = false; }; TUScheduler(const GlobalCompilationDatabase &CDB, const Options &Opts, @@ -302,6 +306,7 @@ private: const GlobalCompilationDatabase &CDB; const bool StorePreamblesInMemory; + const bool AsyncPreambleBuilds; std::unique_ptr Callbacks; // not nullptr Semaphore Barrier; llvm::StringMap> Files; diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -239,10 +239,14 @@ return; } { - std::lock_guard Lock(Mutex); - // If shutdown is issued, don't bother building. - if (Done) - return; + std::unique_lock Lock(Mutex); + // If NextReq was requested with WantDiagnostics::Yes we cannot just drop + // that on the floor. We block the request trying to override the update + // instead of the caller forcing diagnostics to don't block in cases where + // we can start serving the request quickly. + ReqCV.wait(Lock, [this] { + return !NextReq || NextReq->WantDiags != WantDiagnostics::Yes; + }); NextReq = std::move(Req); } // Let the worker thread know there's a request, notify_one is safe as there @@ -285,6 +289,7 @@ ReqCV.notify_all(); } dlog("Preamble worker for {0} stopped", FileName); + BuiltFirst.notify(); } /// Signals the run loop to exit. @@ -304,6 +309,8 @@ return wait(Lock, ReqCV, Timeout, [&] { return !NextReq && !CurrentReq; }); } + void waitForFirst() const { return BuiltFirst.wait(); } + private: /// Holds inputs required for building a preamble. CI is guaranteed to be /// non-null. @@ -333,6 +340,7 @@ mutable std::condition_variable ReqCV; /* GUARDED_BY(Mutex) */ // Accessed only by preamble thread. std::shared_ptr LatestBuild; + Notification BuiltFirst; const Path FileName; ParsingCallbacks &Callbacks; @@ -360,7 +368,7 @@ ASTWorker(PathRef FileName, const GlobalCompilationDatabase &CDB, TUScheduler::ASTCache &LRUCache, Semaphore &Barrier, bool RunSync, DebouncePolicy UpdateDebounce, bool StorePreamblesInMemory, - ParsingCallbacks &Callbacks); + bool AsyncPreambleBuilds, ParsingCallbacks &Callbacks); public: /// Create a new ASTWorker and return a handle to it. @@ -372,7 +380,8 @@ create(PathRef FileName, const GlobalCompilationDatabase &CDB, TUScheduler::ASTCache &IdleASTs, AsyncTaskRunner *Tasks, Semaphore &Barrier, DebouncePolicy UpdateDebounce, - bool StorePreamblesInMemory, ParsingCallbacks &Callbacks); + bool StorePreamblesInMemory, bool AsyncPreambleBuilds, + ParsingCallbacks &Callbacks); ~ASTWorker(); void update(ParseInputs Inputs, WantDiagnostics); @@ -539,10 +548,11 @@ ASTWorker::create(PathRef FileName, const GlobalCompilationDatabase &CDB, TUScheduler::ASTCache &IdleASTs, AsyncTaskRunner *Tasks, Semaphore &Barrier, DebouncePolicy UpdateDebounce, - bool StorePreamblesInMemory, ParsingCallbacks &Callbacks) { - std::shared_ptr Worker( - new ASTWorker(FileName, CDB, IdleASTs, Barrier, /*RunSync=*/!Tasks, - UpdateDebounce, StorePreamblesInMemory, Callbacks)); + bool StorePreamblesInMemory, bool AsyncPreambleBuilds, + ParsingCallbacks &Callbacks) { + std::shared_ptr Worker(new ASTWorker( + FileName, CDB, IdleASTs, Barrier, /*RunSync=*/!Tasks, UpdateDebounce, + StorePreamblesInMemory, AsyncPreambleBuilds, Callbacks)); if (Tasks) { Tasks->runAsync("ASTWorker:" + llvm::sys::path::filename(FileName), [Worker]() { Worker->run(); }); @@ -556,14 +566,13 @@ ASTWorker::ASTWorker(PathRef FileName, const GlobalCompilationDatabase &CDB, TUScheduler::ASTCache &LRUCache, Semaphore &Barrier, bool RunSync, DebouncePolicy UpdateDebounce, - bool StorePreamblesInMemory, ParsingCallbacks &Callbacks) + bool StorePreamblesInMemory, bool AsyncPreambleBuilds, + ParsingCallbacks &Callbacks) : IdleASTs(LRUCache), RunSync(RunSync), UpdateDebounce(UpdateDebounce), FileName(FileName), CDB(CDB), Callbacks(Callbacks), Barrier(Barrier), Done(false), Status(FileName, Callbacks), PreamblePeer(FileName, Callbacks, StorePreamblesInMemory, - // FIXME: Run PreamblePeer asynchronously once ast patching - // is available. - /*RunSync=*/true, Status, *this) { + RunSync || !AsyncPreambleBuilds, Status, *this) { // Set a fallback command because compile command can be accessed before // `Inputs` is initialized. Other fields are only used after initialization // from client inputs. @@ -648,6 +657,9 @@ PreamblePeer.update(std::move(Invocation), std::move(Inputs), std::move(CompilerInvocationDiags), WantDiags); + // Block until first preamble is ready. As patching an empty preamble would + // imply rebuilding it from scratch. + PreamblePeer.waitForFirst(); return; }; startTask(TaskName, std::move(Task), WantDiags, TUScheduler::NoInvalidation); @@ -709,6 +721,8 @@ ASTPeer.updatePreamble(std::move(Req.CI), std::move(Req.Inputs), LatestBuild, std::move(Req.CIDiags), std::move(Req.WantDiags)); + // Set it after notifying ASTPeer about the preamble to prevent any races. + BuiltFirst.notify(); }); if (!LatestBuild || Inputs.ForceRebuild) { @@ -1118,9 +1132,18 @@ bool ASTWorker::blockUntilIdle(Deadline Timeout) const { std::unique_lock Lock(Mutex); - return wait(Lock, RequestsCV, Timeout, [&] { - return PreambleRequests.empty() && Requests.empty() && !CurrentRequest; - }); + // Make sure ASTWorker has processed all requests. + if (!wait(Lock, RequestsCV, Timeout, + [&] { return Requests.empty() && !CurrentRequest; })) + return false; + Lock.unlock(); + if (!PreamblePeer.blockUntilIdle(Timeout)) + return false; + Lock.lock(); + assert(Requests.empty() && "Received new requests during blockUntilIdle"); + // Make sure ASTWorker has processed all preambles. + return wait(Lock, RequestsCV, Timeout, + [&] { return PreambleRequests.empty() && !CurrentRequest; }); } // Render a TUAction to a user-facing string representation. @@ -1179,6 +1202,7 @@ const Options &Opts, std::unique_ptr Callbacks) : CDB(CDB), StorePreamblesInMemory(Opts.StorePreamblesInMemory), + AsyncPreambleBuilds(Opts.AsyncPreambleBuilds), Callbacks(Callbacks ? move(Callbacks) : std::make_unique()), Barrier(Opts.AsyncThreadsCount), @@ -1218,10 +1242,11 @@ bool NewFile = FD == nullptr; if (!FD) { // Create a new worker to process the AST-related tasks. - ASTWorkerHandle Worker = ASTWorker::create( - File, CDB, *IdleASTs, - WorkerThreads ? WorkerThreads.getPointer() : nullptr, Barrier, - UpdateDebounce, StorePreamblesInMemory, *Callbacks); + ASTWorkerHandle Worker = + ASTWorker::create(File, CDB, *IdleASTs, + WorkerThreads ? WorkerThreads.getPointer() : nullptr, + Barrier, UpdateDebounce, StorePreamblesInMemory, + AsyncPreambleBuilds, *Callbacks); FD = std::unique_ptr( new FileData{Inputs.Contents, std::move(Worker)}); } else { diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp --- a/clang-tools-extra/clangd/tool/ClangdMain.cpp +++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -420,6 +420,14 @@ init(false), }; +opt AsyncPreambleBuilds{ + "async-preamble-builds", + cat(Misc), + desc("Enable async preamble builds."), + init(false), + Hidden, +}; + /// Supports a test URI scheme with relaxed constraints for lit tests. /// The path in a test URI will be combined with a platform-specific fake /// directory to form an absolute path. For example, test:///a.cpp is resolved diff --git a/clang-tools-extra/clangd/unittests/ClangdTests.cpp b/clang-tools-extra/clangd/unittests/ClangdTests.cpp --- a/clang-tools-extra/clangd/unittests/ClangdTests.cpp +++ b/clang-tools-extra/clangd/unittests/ClangdTests.cpp @@ -211,18 +211,18 @@ FS.Files[FooCpp] = SourceContents; Server.addDocument(FooCpp, SourceContents); - auto DumpParse1 = dumpASTWithoutMemoryLocs(Server, FooCpp); ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics"; + auto DumpParse1 = dumpASTWithoutMemoryLocs(Server, FooCpp); EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); Server.addDocument(FooCpp, ""); - auto DumpParseEmpty = dumpASTWithoutMemoryLocs(Server, FooCpp); ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics"; + auto DumpParseEmpty = dumpASTWithoutMemoryLocs(Server, FooCpp); EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); Server.addDocument(FooCpp, SourceContents); - auto DumpParse2 = dumpASTWithoutMemoryLocs(Server, FooCpp); ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics"; + auto DumpParse2 = dumpASTWithoutMemoryLocs(Server, FooCpp); EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); EXPECT_EQ(DumpParse1, DumpParse2); @@ -247,20 +247,20 @@ FS.Files[FooCpp] = SourceContents; Server.addDocument(FooCpp, SourceContents); - auto DumpParse1 = dumpASTWithoutMemoryLocs(Server, FooCpp); ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics"; + auto DumpParse1 = dumpASTWithoutMemoryLocs(Server, FooCpp); EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); FS.Files[FooH] = ""; Server.addDocument(FooCpp, SourceContents); - auto DumpParseDifferent = dumpASTWithoutMemoryLocs(Server, FooCpp); ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics"; + auto DumpParseDifferent = dumpASTWithoutMemoryLocs(Server, FooCpp); EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags()); FS.Files[FooH] = "int a;"; Server.addDocument(FooCpp, SourceContents); - auto DumpParse2 = dumpASTWithoutMemoryLocs(Server, FooCpp); ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics"; + auto DumpParse2 = dumpASTWithoutMemoryLocs(Server, FooCpp); EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); EXPECT_EQ(DumpParse1, DumpParse2); diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp --- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp +++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp @@ -21,6 +21,7 @@ #include "support/Threading.h" #include "clang/Basic/DiagnosticDriver.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/FunctionExtras.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/ScopeExit.h" #include "gmock/gmock.h" @@ -29,6 +30,7 @@ #include #include #include +#include #include namespace clang { @@ -90,6 +92,24 @@ static Key)>> DiagsCallbackKey; + using PreambleCallback = llvm::unique_function; + static std::unique_ptr + capturePreamble(PreambleCallback CB) { + class CapturePreamble : public ParsingCallbacks { + public: + CapturePreamble(PreambleCallback CB) : CB(std::move(CB)) {} + void onPreambleAST(PathRef Path, llvm::StringRef Version, ASTContext &Ctx, + std::shared_ptr PP, + const CanonicalIncludes &) override { + CB(); + } + + private: + PreambleCallback CB; + }; + return std::make_unique(std::move(CB)); + } + /// A diagnostics callback that should be passed to TUScheduler when it's used /// in updateWithDiags. static std::unique_ptr captureDiags() { @@ -443,7 +463,7 @@ WithContextValue WithNonce(NonceKey, ++Nonce); Inputs.Version = std::to_string(Nonce); updateWithDiags( - S, File, Inputs, WantDiagnostics::Auto, + S, File, Inputs, WantDiagnostics::Yes, [File, Nonce, &Mut, &TotalUpdates](std::vector) { EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce)); @@ -972,6 +992,40 @@ EXPECT_NEAR(25, Compute({}), 0.01) << "no history -> max"; } +TEST_F(TUSchedulerTests, AsyncPreambleThread) { + Notification Ready; + std::atomic PreambleBuildCount{0}; + TUScheduler S(CDB, optsForTest(), capturePreamble([&] { + if (PreambleBuildCount > 0) + Ready.wait(); + ++PreambleBuildCount; + })); + + Path File = testPath("foo.cpp"); + auto PI = getInputs(File, ""); + PI.Version = "initial"; + S.update(File, PI, WantDiagnostics::Auto); + S.blockUntilIdle(timeoutSeconds(10)); + + // Block preamble builds. + PI.Version = "blocking"; + // Issue second update which will block preamble thread. + S.update(File, PI, WantDiagnostics::Auto); + + Notification RunASTAction; + // Issue an AST read, which shouldn't be blocked and see latest version of the + // file. + S.runWithAST("test", File, [&](Expected AST) { + ASSERT_TRUE(bool(AST)); + EXPECT_THAT(AST->Inputs.Version, PI.Version); + RunASTAction.notify(); + }); + RunASTAction.wait(); + // Make sure second preamble hasn't been built yet. + EXPECT_THAT(PreambleBuildCount.load(), 1U); + Ready.notify(); +} + } // namespace } // namespace clangd } // namespace clang