From 4ac7b805b7c4aa7af0a82d6187e3bd6ac0cf38cd Mon Sep 17 00:00:00 2001 From: Kadir Cetinkaya Date: Thu, 2 Apr 2020 16:35:15 +0200 Subject: [PATCH] [clangd] Get rid of ASTWorker::getCurrentFileInputs Summary: FileInputs are only written by ASTWorker thread, therefore it is safe to read them without the lock inside that thread. It can still be read by other threads through ASTWorker::getCurrentCompileCommand though. This patch also gets rid of the smart pointer wrapping FileInputs as there is never mutliple owners. Reviewers: sammccall Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, usaxena95, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D77309 --- clang-tools-extra/clangd/TUScheduler.cpp | 40 +++++++++++--------------------- 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp index bf847c1..be4d52e 100644 --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -423,10 +423,6 @@ private: Deadline scheduleLocked(); /// Should the first task in the queue be skipped instead of run? bool shouldSkipHeadLocked() const; - /// This is private because `FileInputs.FS` is not thread-safe and thus not - /// safe to share. Callers should make sure not to expose `FS` via a public - /// interface. - std::shared_ptr getCurrentFileInputs() const; struct Request { llvm::unique_function Action; @@ -458,9 +454,9 @@ private: /// Guards members used by both TUScheduler and the worker thread. mutable std::mutex Mutex; /// File inputs, currently being used by the worker. - /// Inputs are written and read by the worker thread, compile command can also - /// be consumed by clients of ASTWorker. - std::shared_ptr FileInputs; /* GUARDED_BY(Mutex) */ + /// Writes and reads from unknown threads are locked. Reads from the worker + /// thread are not locked, as it's the only writer. + ParseInputs FileInputs; /* GUARDED_BY(Mutex) */ /// Times of recent AST rebuilds, used for UpdateDebounce computation. llvm::SmallVector RebuildTimes; /* GUARDED_BY(Mutex) */ @@ -556,12 +552,10 @@ ASTWorker::ASTWorker(PathRef FileName, const GlobalCompilationDatabase &CDB, // FIXME: Run PreamblePeer asynchronously once ast patching // is available. /*RunSync=*/true, Status, *this) { - 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 // from client inputs. - Inputs->CompileCommand = CDB.getFallbackCommand(FileName); - FileInputs = std::move(Inputs); + FileInputs.CompileCommand = CDB.getFallbackCommand(FileName); } ASTWorker::~ASTWorker() { @@ -590,7 +584,7 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) { Inputs.CompileCommand = CDB.getFallbackCommand(FileName); bool InputsAreTheSame = - std::tie(FileInputs->CompileCommand, FileInputs->Contents) == + std::tie(FileInputs.CompileCommand, FileInputs.Contents) == std::tie(Inputs.CompileCommand, Inputs.Contents); // Cached AST is invalidated. if (!InputsAreTheSame) { @@ -601,7 +595,7 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) { // Update current inputs so that subsequent reads can see them. { std::lock_guard Lock(Mutex); - FileInputs = std::make_shared(Inputs); + FileInputs = Inputs; } log("ASTWorker building file {0} version {1} with command {2}\n[{3}]\n{4}", @@ -655,21 +649,20 @@ void ASTWorker::runWithAST( if (isCancelled()) return Action(llvm::make_error()); llvm::Optional> AST = IdleASTs.take(this); - auto CurrentInputs = getCurrentFileInputs(); if (!AST) { StoreDiags CompilerInvocationDiagConsumer; - std::unique_ptr Invocation = buildCompilerInvocation( - *CurrentInputs, CompilerInvocationDiagConsumer); + std::unique_ptr Invocation = + buildCompilerInvocation(FileInputs, CompilerInvocationDiagConsumer); // Try rebuilding the AST. vlog("ASTWorker rebuilding evicted AST to run {0}: {1} version {2}", Name, - FileName, CurrentInputs->Version); + FileName, FileInputs.Version); // FIXME: We might need to build a patched ast once preamble thread starts // running async. Currently getPossiblyStalePreamble below will always // return a compatible preamble as ASTWorker::update blocks. llvm::Optional NewAST = Invocation ? buildAST(FileName, std::move(Invocation), CompilerInvocationDiagConsumer.take(), - *CurrentInputs, getPossiblyStalePreamble()) + FileInputs, getPossiblyStalePreamble()) : None; AST = NewAST ? std::make_unique(std::move(*NewAST)) : nullptr; } @@ -681,8 +674,8 @@ void ASTWorker::runWithAST( return Action(llvm::make_error( "invalid AST", llvm::errc::invalid_argument)); vlog("ASTWorker running {0} on version {2} of {1}", Name, FileName, - CurrentInputs->Version); - Action(InputsAndAST{*CurrentInputs, **AST}); + FileInputs.Version); + Action(InputsAndAST{FileInputs, **AST}); }; startTask(Name, std::move(Task), /*UpdateType=*/None, Invalidation); } @@ -782,7 +775,7 @@ void ASTWorker::generateDiagnostics( } // Used to check whether we can update AST cache. bool InputsAreLatest = - std::tie(FileInputs->CompileCommand, FileInputs->Contents) == + std::tie(FileInputs.CompileCommand, FileInputs.Contents) == std::tie(Inputs.CompileCommand, Inputs.Contents); // Take a shortcut and don't report the diagnostics, since they should be the // same. All the clients should handle the lack of OnUpdated() call anyway to @@ -899,14 +892,9 @@ void ASTWorker::getCurrentPreamble( void ASTWorker::waitForFirstPreamble() const { BuiltFirstPreamble.wait(); } -std::shared_ptr ASTWorker::getCurrentFileInputs() const { - std::unique_lock Lock(Mutex); - return FileInputs; -} - tooling::CompileCommand ASTWorker::getCurrentCompileCommand() const { std::unique_lock Lock(Mutex); - return FileInputs->CompileCommand; + return FileInputs.CompileCommand; } std::size_t ASTWorker::getUsedBytes() const { -- 2.7.4