kadircet updated this revision to Diff 267157. kadircet marked 3 inline comments as done. kadircet added a comment.
- Add comment explaining why we block on ASTWorker::update rather than consumers of a preamble Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80293/new/ https://reviews.llvm.org/D80293 Files: clang-tools-extra/clangd/ClangdServer.cpp clang-tools-extra/clangd/ClangdServer.h clang-tools-extra/clangd/ParsedAST.cpp clang-tools-extra/clangd/ParsedAST.h clang-tools-extra/clangd/TUScheduler.cpp clang-tools-extra/clangd/TUScheduler.h clang-tools-extra/clangd/tool/ClangdMain.cpp clang-tools-extra/clangd/unittests/ClangdTests.cpp clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp +++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp @@ -21,14 +21,20 @@ #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 "llvm/ADT/StringExtras.h" +#include "llvm/ADT/StringMap.h" +#include "llvm/ADT/StringRef.h" #include "gmock/gmock.h" #include "gtest/gtest.h" #include <algorithm> #include <atomic> #include <chrono> #include <cstdint> +#include <memory> +#include <string> #include <utility> namespace clang { @@ -407,6 +413,7 @@ int TotalASTReads = 0; int TotalPreambleReads = 0; int TotalUpdates = 0; + llvm::StringMap<int> LatestDiagVersion; // Run TUScheduler and collect some stats. { @@ -441,15 +448,23 @@ auto Inputs = getInputs(File, Contents.str()); { WithContextValue WithNonce(NonceKey, ++Nonce); - Inputs.Version = std::to_string(Nonce); + Inputs.Version = std::to_string(UpdateI); updateWithDiags( S, File, Inputs, WantDiagnostics::Auto, - [File, Nonce, &Mut, &TotalUpdates](std::vector<Diag>) { + [File, Nonce, Version(Inputs.Version), &Mut, &TotalUpdates, + &LatestDiagVersion](std::vector<Diag>) { EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce)); std::lock_guard<std::mutex> Lock(Mut); ++TotalUpdates; EXPECT_EQ(File, *TUScheduler::getFileBeingProcessedInContext()); + // Make sure Diags are for a newer version. + auto It = LatestDiagVersion.try_emplace(File, -1); + const int PrevVersion = It.first->second; + int CurVersion; + ASSERT_TRUE(llvm::to_integer(Version, CurVersion, 10)); + EXPECT_LT(PrevVersion, CurVersion); + It.first->getValue() = CurVersion; }); } { @@ -494,7 +509,13 @@ } // TUScheduler destructor waits for all operations to finish. std::lock_guard<std::mutex> Lock(Mut); - EXPECT_EQ(TotalUpdates, FilesCount * UpdatesPerFile); + // Updates might get coalesced in preamble thread and result in dropping + // diagnostics for intermediate snapshots. + EXPECT_GE(TotalUpdates, FilesCount); + EXPECT_LE(TotalUpdates, FilesCount * UpdatesPerFile); + // We should receive diags for last update. + for (const auto &Entry : LatestDiagVersion) + EXPECT_EQ(Entry.second, UpdatesPerFile - 1); EXPECT_EQ(TotalASTReads, FilesCount * UpdatesPerFile); EXPECT_EQ(TotalPreambleReads, FilesCount * UpdatesPerFile); } @@ -972,6 +993,57 @@ EXPECT_NEAR(25, Compute({}), 0.01) << "no history -> max"; } +TEST_F(TUSchedulerTests, AsyncPreambleThread) { + // Blocks preamble thread while building preamble with \p BlockVersion until + // \p N is notified. + class BlockPreambleThread : public ParsingCallbacks { + public: + BlockPreambleThread(llvm::StringRef BlockVersion, Notification &N) + : BlockVersion(BlockVersion), N(N) {} + void onPreambleAST(PathRef Path, llvm::StringRef Version, ASTContext &Ctx, + std::shared_ptr<clang::Preprocessor> PP, + const CanonicalIncludes &) override { + if (Version == BlockVersion) + N.wait(); + } + + private: + llvm::StringRef BlockVersion; + Notification &N; + }; + + static constexpr llvm::StringLiteral InputsV0 = "v0"; + static constexpr llvm::StringLiteral InputsV1 = "v1"; + Notification Ready; + TUScheduler S(CDB, optsForTest(), + std::make_unique<BlockPreambleThread>(InputsV1, Ready)); + + Path File = testPath("foo.cpp"); + auto PI = getInputs(File, ""); + PI.Version = InputsV0.str(); + S.update(File, PI, WantDiagnostics::Auto); + S.blockUntilIdle(timeoutSeconds(10)); + + // Block preamble builds. + PI.Version = InputsV1.str(); + // 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<InputsAndAST> AST) { + ASSERT_TRUE(bool(AST)); + // Make sure preamble is built with stale inputs, but AST was built using + // new ones. + EXPECT_THAT(AST->AST.preambleVersion(), InputsV0); + EXPECT_THAT(AST->Inputs.Version, InputsV1.str()); + RunASTAction.notify(); + }); + RunASTAction.wait(); + Ready.notify(); +} + } // namespace } // namespace clangd } // namespace clang Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/ClangdTests.cpp +++ 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); Index: clang-tools-extra/clangd/tool/ClangdMain.cpp =================================================================== --- clang-tools-extra/clangd/tool/ClangdMain.cpp +++ clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -417,6 +417,15 @@ init(false), }; +opt<bool> AsyncPreamble{ + "async-preamble", + cat(Misc), + desc("Reuse even stale preambles, and rebuild them in the background. This " + "improves latency at the cost of accuracy."), + init(ClangdServer::Options().AsyncPreambleBuilds), + 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 Index: clang-tools-extra/clangd/TUScheduler.h =================================================================== --- clang-tools-extra/clangd/TUScheduler.h +++ 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, @@ -301,7 +305,7 @@ private: const GlobalCompilationDatabase &CDB; - const bool StorePreamblesInMemory; + const Options Opts; std::unique_ptr<ParsingCallbacks> Callbacks; // not nullptr Semaphore Barrier; llvm::StringMap<std::unique_ptr<FileData>> Files; @@ -310,7 +314,6 @@ // asynchronously. llvm::Optional<AsyncTaskRunner> PreambleTasks; llvm::Optional<AsyncTaskRunner> WorkerThreads; - DebouncePolicy UpdateDebounce; }; } // namespace clangd Index: clang-tools-extra/clangd/TUScheduler.cpp =================================================================== --- clang-tools-extra/clangd/TUScheduler.cpp +++ clang-tools-extra/clangd/TUScheduler.cpp @@ -239,10 +239,14 @@ return; } { - std::lock_guard<std::mutex> Lock(Mutex); - // If shutdown is issued, don't bother building. - if (Done) - return; + std::unique_lock<std::mutex> Lock(Mutex); + // If NextReq was requested with WantDiagnostics::Yes we cannot just drop + // that on the floor. Block until we start building it. This won't + // dead-lock as we are blocking the caller thread, while builds continue + // on preamble thread. + 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 @@ -359,8 +363,7 @@ friend class ASTWorkerHandle; ASTWorker(PathRef FileName, const GlobalCompilationDatabase &CDB, TUScheduler::ASTCache &LRUCache, Semaphore &Barrier, bool RunSync, - DebouncePolicy UpdateDebounce, bool StorePreamblesInMemory, - ParsingCallbacks &Callbacks); + const TUScheduler::Options &Opts, ParsingCallbacks &Callbacks); public: /// Create a new ASTWorker and return a handle to it. @@ -368,11 +371,12 @@ /// is null, all requests will be processed on the calling thread /// synchronously instead. \p Barrier is acquired when processing each /// request, it is used to limit the number of actively running threads. - static ASTWorkerHandle - create(PathRef FileName, const GlobalCompilationDatabase &CDB, - TUScheduler::ASTCache &IdleASTs, AsyncTaskRunner *Tasks, - Semaphore &Barrier, DebouncePolicy UpdateDebounce, - bool StorePreamblesInMemory, ParsingCallbacks &Callbacks); + static ASTWorkerHandle create(PathRef FileName, + const GlobalCompilationDatabase &CDB, + TUScheduler::ASTCache &IdleASTs, + AsyncTaskRunner *Tasks, Semaphore &Barrier, + const TUScheduler::Options &Opts, + ParsingCallbacks &Callbacks); ~ASTWorker(); void update(ParseInputs Inputs, WantDiagnostics); @@ -476,6 +480,7 @@ std::queue<Request> PreambleRequests; /* GUARDED_BY(Mutex) */ llvm::Optional<Request> CurrentRequest; /* GUARDED_BY(Mutex) */ mutable std::condition_variable RequestsCV; + Notification ReceivedPreamble; /// Guards the callback that publishes results of AST-related computations /// (diagnostics, highlightings) and file statuses. std::mutex PublishMu; @@ -535,14 +540,14 @@ std::shared_ptr<ASTWorker> Worker; }; -ASTWorkerHandle -ASTWorker::create(PathRef FileName, const GlobalCompilationDatabase &CDB, - TUScheduler::ASTCache &IdleASTs, AsyncTaskRunner *Tasks, - Semaphore &Barrier, DebouncePolicy UpdateDebounce, - bool StorePreamblesInMemory, ParsingCallbacks &Callbacks) { - std::shared_ptr<ASTWorker> Worker( - new ASTWorker(FileName, CDB, IdleASTs, Barrier, /*RunSync=*/!Tasks, - UpdateDebounce, StorePreamblesInMemory, Callbacks)); +ASTWorkerHandle ASTWorker::create(PathRef FileName, + const GlobalCompilationDatabase &CDB, + TUScheduler::ASTCache &IdleASTs, + AsyncTaskRunner *Tasks, Semaphore &Barrier, + const TUScheduler::Options &Opts, + ParsingCallbacks &Callbacks) { + std::shared_ptr<ASTWorker> Worker(new ASTWorker( + FileName, CDB, IdleASTs, Barrier, /*RunSync=*/!Tasks, Opts, Callbacks)); if (Tasks) { Tasks->runAsync("ASTWorker:" + llvm::sys::path::filename(FileName), [Worker]() { Worker->run(); }); @@ -555,15 +560,13 @@ ASTWorker::ASTWorker(PathRef FileName, const GlobalCompilationDatabase &CDB, TUScheduler::ASTCache &LRUCache, Semaphore &Barrier, - bool RunSync, DebouncePolicy UpdateDebounce, - bool StorePreamblesInMemory, ParsingCallbacks &Callbacks) - : IdleASTs(LRUCache), RunSync(RunSync), UpdateDebounce(UpdateDebounce), + bool RunSync, const TUScheduler::Options &Opts, + ParsingCallbacks &Callbacks) + : IdleASTs(LRUCache), RunSync(RunSync), UpdateDebounce(Opts.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) { + PreamblePeer(FileName, Callbacks, Opts.StorePreamblesInMemory, + RunSync || !Opts.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 +651,12 @@ 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. + // This isn't the natural place to block, rather where the preamble would be + // consumed. But that's too late, we'd be running on the worker thread with + // the PreambleTask scheduled and so we'd deadlock. + ReceivedPreamble.wait(); return; }; startTask(TaskName, std::move(Task), WantDiags, TUScheduler::NoInvalidation); @@ -771,6 +780,7 @@ }; if (RunSync) { Task(); + ReceivedPreamble.notify(); return; } { @@ -779,6 +789,7 @@ steady_clock::now(), Context::current().clone(), llvm::None, TUScheduler::NoInvalidation, nullptr}); } + ReceivedPreamble.notify(); RequestsCV.notify_all(); } @@ -915,6 +926,7 @@ Done = true; } // We are no longer going to build any preambles, let the waiters know that. + ReceivedPreamble.notify(); BuiltFirstPreamble.notify(); PreamblePeer.stop(); Status.stop(); @@ -1117,10 +1129,24 @@ } bool ASTWorker::blockUntilIdle(Deadline Timeout) const { - std::unique_lock<std::mutex> Lock(Mutex); - return wait(Lock, RequestsCV, Timeout, [&] { - return PreambleRequests.empty() && Requests.empty() && !CurrentRequest; - }); + auto WaitUntilASTWorkerIsIdle = [&] { + std::unique_lock<std::mutex> Lock(Mutex); + return wait(Lock, RequestsCV, Timeout, [&] { + return PreambleRequests.empty() && Requests.empty() && !CurrentRequest; + }); + }; + // Make sure ASTWorker has processed all requests, which might issue new + // updates to PreamblePeer. + WaitUntilASTWorkerIsIdle(); + // Now that ASTWorker processed all requests, ensure PreamblePeer has served + // all update requests. This might create new PreambleRequests for the + // ASTWorker. + PreamblePeer.blockUntilIdle(Timeout); + assert(Requests.empty() && + "No new normal tasks can be scheduled concurrently with " + "blockUntilIdle(): ASTWorker isn't threadsafe"); + // Finally make sure ASTWorker has processed all of the preamble updates. + return WaitUntilASTWorkerIsIdle(); } // Render a TUAction to a user-facing string representation. @@ -1178,13 +1204,12 @@ TUScheduler::TUScheduler(const GlobalCompilationDatabase &CDB, const Options &Opts, std::unique_ptr<ParsingCallbacks> Callbacks) - : CDB(CDB), StorePreamblesInMemory(Opts.StorePreamblesInMemory), + : CDB(CDB), Opts(Opts), Callbacks(Callbacks ? move(Callbacks) : std::make_unique<ParsingCallbacks>()), Barrier(Opts.AsyncThreadsCount), IdleASTs( - std::make_unique<ASTCache>(Opts.RetentionPolicy.MaxRetainedASTs)), - UpdateDebounce(Opts.UpdateDebounce) { + std::make_unique<ASTCache>(Opts.RetentionPolicy.MaxRetainedASTs)) { if (0 < Opts.AsyncThreadsCount) { PreambleTasks.emplace(); WorkerThreads.emplace(); @@ -1218,10 +1243,10 @@ 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, Opts, *Callbacks); FD = std::unique_ptr<FileData>( new FileData{Inputs.Contents, std::move(Worker)}); } else { Index: clang-tools-extra/clangd/ParsedAST.h =================================================================== --- clang-tools-extra/clangd/ParsedAST.h +++ clang-tools-extra/clangd/ParsedAST.h @@ -33,6 +33,9 @@ #include "clang/Tooling/CompilationDatabase.h" #include "clang/Tooling/Syntax/Tokens.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/None.h" +#include "llvm/ADT/Optional.h" +#include "llvm/ADT/StringRef.h" #include <memory> #include <string> #include <vector> @@ -102,6 +105,10 @@ /// Returns the version of the ParseInputs this AST was built from. llvm::StringRef version() const { return Version; } + /// Returns the version of the ParseInputs used to build Preamble part of this + /// AST. Might be None if no Preamble is used. + llvm::Optional<llvm::StringRef> preambleVersion() const; + private: ParsedAST(llvm::StringRef Version, std::shared_ptr<const PreambleData> Preamble, Index: clang-tools-extra/clangd/ParsedAST.cpp =================================================================== --- clang-tools-extra/clangd/ParsedAST.cpp +++ clang-tools-extra/clangd/ParsedAST.cpp @@ -551,5 +551,10 @@ assert(this->Action); } +llvm::Optional<llvm::StringRef> ParsedAST::preambleVersion() const { + if (!Preamble) + return llvm::None; + return llvm::StringRef(Preamble->Version); +} } // namespace clangd } // namespace clang Index: clang-tools-extra/clangd/ClangdServer.h =================================================================== --- clang-tools-extra/clangd/ClangdServer.h +++ clang-tools-extra/clangd/ClangdServer.h @@ -97,6 +97,9 @@ /// Cached preambles are potentially large. If false, store them on disk. bool StorePreamblesInMemory = true; + /// Reuse even stale preambles, and rebuild them in the background. + /// This improves latency at the cost of accuracy. + bool AsyncPreambleBuilds = false; /// If true, ClangdServer builds a dynamic in-memory index for symbols in /// opened files and uses the index to augment code completion results. Index: clang-tools-extra/clangd/ClangdServer.cpp =================================================================== --- clang-tools-extra/clangd/ClangdServer.cpp +++ 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; }
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits