From 6b85032c95bee2e15ce7f239316de4d11e6ca08b Mon Sep 17 00:00:00 2001 From: Kadir Cetinkaya Date: Tue, 17 Mar 2020 19:08:23 +0100 Subject: [PATCH] [clangd] Update TUStatus api to accommodate preamble thread Summary: TUStatus api had a single thread in mind. This introudces a section action to represent state of the preamble thread. In the file status extension, we keep old behavior almost the same. We only prepend current task with a `parsing includes` if preamble thread is working. We omit the idle thread in the output unless both threads are idle. Reviewers: sammccall Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, usaxena95, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D76304 --- clang-tools-extra/clangd/ClangdLSPServer.cpp | 9 +- clang-tools-extra/clangd/TUScheduler.cpp | 199 +++++++++++++-------- clang-tools-extra/clangd/TUScheduler.h | 26 +-- .../clangd/unittests/TUSchedulerTests.cpp | 71 ++++---- 4 files changed, 184 insertions(+), 121 deletions(-) diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index 19476d7..eafe353 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -15,6 +15,7 @@ #include "Protocol.h" #include "SemanticHighlighting.h" #include "SourceCode.h" +#include "TUScheduler.h" #include "Trace.h" #include "URI.h" #include "refactor/Tweak.h" @@ -1368,7 +1369,8 @@ ClangdLSPServer::ClangdLSPServer( // clang-format on } -ClangdLSPServer::~ClangdLSPServer() { IsBeingDestroyed = true; +ClangdLSPServer::~ClangdLSPServer() { + IsBeingDestroyed = true; // Explicitly destroy ClangdServer first, blocking on threads it owns. // This ensures they don't access any other members. Server.reset(); @@ -1556,8 +1558,9 @@ void ClangdLSPServer::onFileUpdated(PathRef File, const TUStatus &Status) { // two statuses are running faster in practice, which leads the UI constantly // changing, and doesn't provide much value. We may want to emit status at a // reasonable time interval (e.g. 0.5s). - if (Status.Action.S == TUAction::BuildingFile || - Status.Action.S == TUAction::RunningAction) + if (Status.PreambleActivity == PreambleAction::Idle && + (Status.ASTActivity.K == ASTAction::Building || + Status.ASTActivity.K == ASTAction::RunningAction)) return; notify("textDocument/clangd.fileStatus", Status.render(File)); } diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp index 2341c34..2ca0ae8 100644 --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -57,15 +57,20 @@ #include "clang/Frontend/CompilerInvocation.h" #include "clang/Tooling/CompilationDatabase.h" #include "llvm/ADT/Optional.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/ScopeExit.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Errc.h" +#include "llvm/Support/ErrorHandling.h" #include "llvm/Support/Path.h" #include "llvm/Support/Threading.h" #include #include #include #include +#include #include namespace clang { @@ -152,6 +157,39 @@ private: }; namespace { +/// Threadsafe manager for updating a TUStatus and emitting it after each +/// update. +class SynchronizedTUStatus { +public: + SynchronizedTUStatus(PathRef FileName, ParsingCallbacks &Callbacks) + : FileName(FileName), Callbacks(Callbacks) {} + + void update(llvm::function_ref Mutator) { + std::lock_guard Lock(StatusMu); + Mutator(Status); + emitStatusLocked(); + } + + /// Prevents emitting of further updates. + void stop() { + std::lock_guard Lock(StatusMu); + CanPublish = false; + } + +private: + void emitStatusLocked() { + if (CanPublish) + Callbacks.onFileUpdated(FileName, Status); + } + + const Path FileName; + + std::mutex StatusMu; + TUStatus Status; + bool CanPublish = true; + ParsingCallbacks &Callbacks; +}; + /// Responsible for building and providing access to the preamble of a TU. /// Whenever the thread is idle and the preamble is outdated, it starts to build /// a fresh preamble from the latest inputs. If RunSync is true, preambles are @@ -159,9 +197,11 @@ namespace { class PreambleThread { public: PreambleThread(llvm::StringRef FileName, ParsingCallbacks &Callbacks, - bool StorePreambleInMemory, bool RunSync) + bool StorePreambleInMemory, bool RunSync, + SynchronizedTUStatus &Status) : FileName(FileName), Callbacks(Callbacks), - StoreInMemory(StorePreambleInMemory), RunSync(RunSync) {} + StoreInMemory(StorePreambleInMemory), RunSync(RunSync), Status(Status) { + } size_t getUsedBytes() const { auto Preamble = latest(); @@ -174,10 +214,6 @@ public: void update(CompilerInvocation *CI, ParseInputs PI) { // If compiler invocation was broken, just fail out early. if (!CI) { - TUStatus::BuildDetails Details; - Details.BuildFailed = true; - std::string TaskName = llvm::formatv("Update ({0})", PI.Version); - emitTUStatus({TUAction::BuildingPreamble, std::move(TaskName)}, &Details); // Make sure anyone waiting for the preamble gets notified it could not be // built. BuiltFirst.notify(); @@ -187,6 +223,9 @@ public: Request Req = {std::make_unique(*CI), std::move(PI)}; if (RunSync) { build(std::move(Req)); + Status.update([](TUStatus &Status) { + Status.PreambleActivity = PreambleAction::Idle; + }); return; } { @@ -225,9 +264,19 @@ public: } // Build the preamble and let the waiters know about it. build(std::move(*CurrentReq)); + bool IsEmpty = false; { std::lock_guard Lock(Mutex); CurrentReq.reset(); + IsEmpty = !NextReq.hasValue(); + } + if (IsEmpty) { + // We don't perform this above, before waiting for a request to make + // tests more deterministic. As there can be a race between this thread + // and client thread(clangdserver). + Status.update([](TUStatus &Status) { + Status.PreambleActivity = PreambleAction::Idle; + }); } ReqCV.notify_all(); } @@ -265,18 +314,6 @@ private: return Done; } - /// Updates the TUStatus and emits it. Only called in the worker thread. - void emitTUStatus(TUAction Action, - const TUStatus::BuildDetails *Details = nullptr) { - // Do not emit TU statuses when the worker is shutting down. - if (isDone()) - return; - TUStatus Status({std::move(Action), {}}); - if (Details) - Status.Details = *Details; - Callbacks.onFileUpdated(FileName, Status); - } - /// Builds a preamble for Req and caches it. Might re-use the latest built /// preamble if it is valid for Req. Also signals waiters about the build. /// FIXME: We shouldn't cache failed preambles, if we've got a successful @@ -287,8 +324,9 @@ private: std::shared_ptr OldPreamble = Inputs.ForceRebuild ? nullptr : latest(); - std::string TaskName = llvm::formatv("Update ({0})", Inputs.Version); - emitTUStatus({TUAction::BuildingPreamble, std::move(TaskName)}); + Status.update([&](TUStatus &Status) { + Status.PreambleActivity = PreambleAction::Building; + }); auto Preamble = clang::clangd::buildPreamble( FileName, std::move(*Req.CI), OldPreamble, Inputs, StoreInMemory, @@ -321,6 +359,8 @@ private: ParsingCallbacks &Callbacks; const bool StoreInMemory; const bool RunSync; + + SynchronizedTUStatus &Status; }; class ASTWorkerHandle; @@ -391,9 +431,6 @@ private: void startTask(llvm::StringRef Name, llvm::unique_function Task, llvm::Optional UpdateType, TUScheduler::ASTActionInvalidation); - /// Updates the TUStatus and emits it. Only called in the worker thread. - void emitTUStatus(TUAction FAction, - const TUStatus::BuildDetails *Detail = nullptr); /// Determines the next action to perform. /// All actions that should never run are discarded. @@ -427,8 +464,6 @@ private: const GlobalCompilationDatabase &CDB; /// Callback invoked when preamble or main file AST is built. ParsingCallbacks &Callbacks; - /// Only accessed by the worker thread. - TUStatus Status; Semaphore &Barrier; /// Whether the 'onMainAST' callback ran for the current FileInputs. @@ -458,6 +493,7 @@ private: // any results to the user. bool CanPublishResults = true; /* GUARDED_BY(PublishMu) */ + SynchronizedTUStatus Status; PreambleThread PW; }; @@ -526,12 +562,11 @@ ASTWorker::ASTWorker(PathRef FileName, const GlobalCompilationDatabase &CDB, bool RunSync, DebouncePolicy UpdateDebounce, bool StorePreamblesInMemory, ParsingCallbacks &Callbacks) : IdleASTs(LRUCache), RunSync(RunSync), UpdateDebounce(UpdateDebounce), - FileName(FileName), CDB(CDB), - Callbacks(Callbacks), Status{TUAction(TUAction::Idle, ""), - TUStatus::BuildDetails()}, - Barrier(Barrier), Done(false), + FileName(FileName), CDB(CDB), Callbacks(Callbacks), Barrier(Barrier), + Done(false), Status(FileName, Callbacks), // FIXME: Run preambleworker async. - PW(FileName, Callbacks, StorePreamblesInMemory, /*RunSync=*/true) { + PW(FileName, Callbacks, StorePreamblesInMemory, /*RunSync=*/true, + Status) { auto Inputs = std::make_shared(); // Set a fallback command because compile command can be accessed before // `Inputs` is initialized. Other fields are only used after initialization @@ -619,7 +654,10 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) { // to the old preamble, so it can be freed if there are no other references // to it. OldPreamble.reset(); - emitTUStatus({TUAction::BuildingFile, TaskName}); + Status.update([&](TUStatus &Status) { + Status.ASTActivity.K = ASTAction::Building; + Status.ASTActivity.Name = std::move(TaskName); + }); if (!CanReuseAST) { IdleASTs.take(this); // Remove the old AST if it's still in cache. } else { @@ -635,9 +673,11 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) { // current file at this point? log("Skipping rebuild of the AST for {0}, inputs are the same.", FileName); - TUStatus::BuildDetails Details; - Details.ReuseAST = true; - emitTUStatus({TUAction::BuildingFile, TaskName}, &Details); + + Status.update([](TUStatus &Status) { + Status.Details.ReuseAST = true; + Status.Details.BuildFailed = false; + }); return; } } @@ -661,17 +701,18 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) { llvm::Optional NewAST = buildAST(FileName, std::move(Invocation), CompilerInvocationDiags, Inputs, NewPreamble); + // buildAST fails. + Status.update([&](TUStatus &Status) { + Status.Details.ReuseAST = false; + Status.Details.BuildFailed = !NewAST.hasValue(); + }); AST = NewAST ? std::make_unique(std::move(*NewAST)) : nullptr; - if (!(*AST)) { // buildAST fails. - TUStatus::BuildDetails Details; - Details.BuildFailed = true; - emitTUStatus({TUAction::BuildingFile, TaskName}, &Details); - } } else { // We are reusing the AST. - TUStatus::BuildDetails Details; - Details.ReuseAST = true; - emitTUStatus({TUAction::BuildingFile, TaskName}, &Details); + Status.update([](TUStatus &Status) { + Status.Details.ReuseAST = true; + Status.Details.BuildFailed = false; + }); } // We want to report the diagnostics even if this update was cancelled. @@ -815,6 +856,7 @@ void ASTWorker::stop() { assert(!Done && "stop() called twice"); Done = true; } + Status.stop(); RequestsCV.notify_all(); } @@ -856,18 +898,6 @@ void ASTWorker::startTask(llvm::StringRef Name, RequestsCV.notify_all(); } -void ASTWorker::emitTUStatus(TUAction Action, - const TUStatus::BuildDetails *Details) { - Status.Action = std::move(Action); - if (Details) - Status.Details = *Details; - std::lock_guard Lock(PublishMu); - // Do not emit TU statuses when the ASTWorker is shutting down. - if (CanPublishResults) { - Callbacks.onFileUpdated(FileName, Status); - } -} - void ASTWorker::run() { auto _ = llvm::make_scope_exit([this] { PW.stop(); }); while (true) { @@ -891,7 +921,10 @@ void ASTWorker::run() { Tracer.emplace("Debounce"); SPAN_ATTACH(*Tracer, "next_request", Requests.front().Name); if (!(Wait == Deadline::infinity())) { - emitTUStatus({TUAction::Queued, Requests.front().Name}); + Status.update([&](TUStatus &Status) { + Status.ASTActivity.K = ASTAction::Queued; + Status.ASTActivity.Name = Requests.front().Name; + }); SPAN_ATTACH(*Tracer, "sleep_ms", std::chrono::duration_cast( Wait.time() - steady_clock::now()) @@ -910,12 +943,18 @@ void ASTWorker::run() { { std::unique_lock Lock(Barrier, std::try_to_lock); if (!Lock.owns_lock()) { - emitTUStatus({TUAction::Queued, CurrentRequest->Name}); + Status.update([&](TUStatus &Status) { + Status.ASTActivity.K = ASTAction::Queued; + Status.ASTActivity.Name = CurrentRequest->Name; + }); Lock.lock(); } WithContext Guard(std::move(CurrentRequest->Ctx)); trace::Span Tracer(CurrentRequest->Name); - emitTUStatus({TUAction::RunningAction, CurrentRequest->Name}); + Status.update([&](TUStatus &Status) { + Status.ASTActivity.K = ASTAction::RunningAction; + Status.ASTActivity.Name = CurrentRequest->Name; + }); CurrentRequest->Action(); } @@ -925,8 +964,12 @@ void ASTWorker::run() { CurrentRequest.reset(); IsEmpty = Requests.empty(); } - if (IsEmpty) - emitTUStatus({TUAction::Idle, /*Name*/ ""}); + if (IsEmpty) { + Status.update([&](TUStatus &Status) { + Status.ASTActivity.K = ASTAction::Idle; + Status.ASTActivity.Name = ""; + }); + } RequestsCV.notify_all(); } } @@ -1010,27 +1053,33 @@ bool ASTWorker::blockUntilIdle(Deadline Timeout) const { // TUAction represents clangd-internal states, we don't intend to expose them // to users (say C++ programmers) directly to avoid confusion, we use terms that // are familiar by C++ programmers. -std::string renderTUAction(const TUAction &Action) { - std::string Result; - llvm::raw_string_ostream OS(Result); - switch (Action.S) { - case TUAction::Queued: - OS << "file is queued"; +std::string renderTUAction(const PreambleAction PA, const ASTAction &AA) { + llvm::SmallVector Result; + switch (PA) { + case PreambleAction::Building: + Result.push_back("parsing includes"); break; - case TUAction::RunningAction: - OS << "running " << Action.Name; + case PreambleAction::Idle: + // We handle idle specially below. + break; + } + switch (AA.K) { + case ASTAction::Queued: + Result.push_back("file is queued"); break; - case TUAction::BuildingPreamble: - OS << "parsing includes"; + case ASTAction::RunningAction: + Result.push_back("running " + AA.Name); break; - case TUAction::BuildingFile: - OS << "parsing main file"; + case ASTAction::Building: + Result.push_back("parsing main file"); break; - case TUAction::Idle: - OS << "idle"; + case ASTAction::Idle: + // We handle idle specially below. break; } - return OS.str(); + if (Result.empty()) + return "idle"; + return llvm::join(Result, ","); } } // namespace @@ -1042,7 +1091,7 @@ unsigned getDefaultAsyncThreadsCount() { FileStatus TUStatus::render(PathRef File) const { FileStatus FStatus; FStatus.uri = URIForFile::canonicalize(File, /*TUPath=*/File); - FStatus.state = renderTUAction(Action); + FStatus.state = renderTUAction(PreambleActivity, ASTActivity); return FStatus; } diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h index 5cc7e02..b6b9e5c 100644 --- a/clang-tools-extra/clangd/TUScheduler.h +++ b/clang-tools-extra/clangd/TUScheduler.h @@ -83,18 +83,22 @@ struct DebouncePolicy { static DebouncePolicy fixed(clock::duration); }; -struct TUAction { - enum State { - Queued, // The TU is pending in the thread task queue to be built. - RunningAction, // Starting running actions on the TU. - BuildingPreamble, // The preamble of the TU is being built. - BuildingFile, // The TU is being built. It is only emitted when building - // the AST for diagnostics in write action (update). +enum class PreambleAction { + Idle, + Building, +}; + +struct ASTAction { + enum Kind { + Queued, // The action is pending in the thread task queue to be run. + RunningAction, // Started running actions on the TU. + Building, // The AST is being built. Idle, // Indicates the worker thread is idle, and ready to run any upcoming // actions. }; - TUAction(State S, llvm::StringRef Name) : S(S), Name(Name) {} - State S; + ASTAction() = default; + ASTAction(Kind K, llvm::StringRef Name) : K(K), Name(Name) {} + Kind K = ASTAction::Idle; /// The name of the action currently running, e.g. Update, GoToDef, Hover. /// Empty if we are in the idle state. std::string Name; @@ -111,7 +115,9 @@ struct TUStatus { /// Serialize this to an LSP file status item. FileStatus render(PathRef File) const; - TUAction Action; + PreambleAction PreambleActivity = PreambleAction::Idle; + ASTAction ASTActivity; + /// Stores status of the last build for the translation unit. BuildDetails Details; }; diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp index 3d68124..760e1c0 100644 --- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp +++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp @@ -24,6 +24,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" #include +#include #include #include @@ -40,13 +41,14 @@ using ::testing::IsEmpty; using ::testing::Pointee; using ::testing::UnorderedElementsAre; -MATCHER_P2(TUState, State, ActionName, "") { - if (arg.Action.S != State) { - *result_listener << "state is " << arg.Action.S; +MATCHER_P2(TUState, PreambleActivity, ASTActivity, "") { + if (arg.PreambleActivity != PreambleActivity) { + *result_listener << "preamblestate is " + << static_cast(arg.PreambleActivity); return false; } - if (arg.Action.Name != ActionName) { - *result_listener << "name is " << arg.Action.Name; + if (arg.ASTActivity.K != ASTActivity) { + *result_listener << "aststate is " << arg.ASTActivity.K; return false; } return true; @@ -732,14 +734,13 @@ TEST_F(TUSchedulerTests, ForceRebuild) { // Update the source contents, which should trigger an initial build with // the header file missing. - updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes, - [](std::vector Diags) { - EXPECT_THAT( - Diags, - ElementsAre( - Field(&Diag::Message, "'foo.h' file not found"), - Field(&Diag::Message, "use of undeclared identifier 'a'"))); - }); + updateWithDiags( + S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) { + EXPECT_THAT(Diags, + ElementsAre(Field(&Diag::Message, "'foo.h' file not found"), + Field(&Diag::Message, + "use of undeclared identifier 'a'"))); + }); // Add the header file. We need to recreate the inputs since we changed a // file from underneath the test FS. @@ -749,18 +750,17 @@ TEST_F(TUSchedulerTests, ForceRebuild) { // The addition of the missing header file shouldn't trigger a rebuild since // we don't track missing files. - updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes, - [](std::vector Diags) { - ADD_FAILURE() << "Did not expect diagnostics for missing header update"; - }); + updateWithDiags( + S, Source, Inputs, WantDiagnostics::Yes, [](std::vector Diags) { + ADD_FAILURE() << "Did not expect diagnostics for missing header update"; + }); // Forcing the reload should should cause a rebuild which no longer has any // errors. Inputs.ForceRebuild = true; - updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes, - [](std::vector Diags) { - EXPECT_THAT(Diags, IsEmpty()); - }); + updateWithDiags( + S, Source, Inputs, WantDiagnostics::Yes, + [](std::vector Diags) { EXPECT_THAT(Diags, IsEmpty()); }); ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); } @@ -848,14 +848,21 @@ TEST_F(TUSchedulerTests, TUStatus) { EXPECT_THAT(CaptureTUStatus.allStatus(), ElementsAre( - // Statuses of "Update" action. - TUState(TUAction::RunningAction, "Update (1)"), - TUState(TUAction::BuildingPreamble, "Update (1)"), - TUState(TUAction::BuildingFile, "Update (1)"), - - // Statuses of "Definitions" action - TUState(TUAction::RunningAction, "Definitions"), - TUState(TUAction::Idle, /*No action*/ ""))); + // Everything starts with ASTWorker starting to execute an + // update + TUState(PreambleAction::Idle, ASTAction::RunningAction), + // We build the preamble + TUState(PreambleAction::Building, ASTAction::RunningAction), + // Preamble worker goes idle + TUState(PreambleAction::Idle, ASTAction::RunningAction), + // We start building the ast + TUState(PreambleAction::Idle, ASTAction::Building), + // Built finished succesffully + TUState(PreambleAction::Idle, ASTAction::Building), + // Rnning go to def + TUState(PreambleAction::Idle, ASTAction::RunningAction), + // both workers go idle + TUState(PreambleAction::Idle, ASTAction::Idle))); } TEST_F(TUSchedulerTests, CommandLineErrors) { @@ -868,8 +875,7 @@ TEST_F(TUSchedulerTests, CommandLineErrors) { TUScheduler S(CDB, optsForTest(), captureDiags()); std::vector Diagnostics; updateWithDiags(S, testPath("foo.cpp"), "void test() {}", - WantDiagnostics::Yes, - [&](std::vector D) { + WantDiagnostics::Yes, [&](std::vector D) { Diagnostics = std::move(D); Ready.notify(); }); @@ -893,8 +899,7 @@ TEST_F(TUSchedulerTests, CommandLineWarnings) { TUScheduler S(CDB, optsForTest(), captureDiags()); std::vector Diagnostics; updateWithDiags(S, testPath("foo.cpp"), "void test() {}", - WantDiagnostics::Yes, - [&](std::vector D) { + WantDiagnostics::Yes, [&](std::vector D) { Diagnostics = std::move(D); Ready.notify(); }); -- 2.7.4