kadircet updated this revision to Diff 266201.
kadircet edited the summary of this revision.
kadircet added a comment.
- Rebase and update comments on block until idle.
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/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,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 <atomic>
#include <chrono>
#include <cstdint>
+#include <memory>
#include <utility>
namespace clang {
@@ -90,6 +92,24 @@
static Key<llvm::unique_function<void(PathRef File, std::vector<Diag>)>>
DiagsCallbackKey;
+ using PreambleCallback = llvm::unique_function<void()>;
+ static std::unique_ptr<ParsingCallbacks>
+ 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<clang::Preprocessor> PP,
+ const CanonicalIncludes &) override {
+ CB();
+ }
+
+ private:
+ PreambleCallback CB;
+ };
+ return std::make_unique<CapturePreamble>(std::move(CB));
+ }
+
/// A diagnostics callback that should be passed to TUScheduler when it's used
/// in updateWithDiags.
static std::unique_ptr<ParsingCallbacks> 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<Diag>) {
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<unsigned> 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<InputsAndAST> 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
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,14 @@
init(false),
};
+opt<bool> 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
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,
@@ -302,6 +306,7 @@
private:
const GlobalCompilationDatabase &CDB;
const bool StorePreamblesInMemory;
+ const bool AsyncPreambleBuilds;
std::unique_ptr<ParsingCallbacks> Callbacks; // not nullptr
Semaphore Barrier;
llvm::StringMap<std::unique_ptr<FileData>> Files;
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. 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<const PreambleData> 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<ASTWorker> Worker(
- new ASTWorker(FileName, CDB, IdleASTs, Barrier, /*RunSync=*/!Tasks,
- UpdateDebounce, StorePreamblesInMemory, Callbacks));
+ bool StorePreamblesInMemory, bool AsyncPreambleBuilds,
+ ParsingCallbacks &Callbacks) {
+ std::shared_ptr<ASTWorker> 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,21 @@
bool ASTWorker::blockUntilIdle(Deadline Timeout) const {
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.
+ if (!wait(Lock, RequestsCV, Timeout,
+ [&] { return Requests.empty() && !CurrentRequest; }))
+ return false;
+ Lock.unlock();
+ // Now that ASTWorker processed all requests, ensure PreamblePeer has served
+ // all update requests.
+ if (!PreamblePeer.blockUntilIdle(Timeout))
+ return false;
+ Lock.lock();
+ assert(Requests.empty() && "Received new requests during blockUntilIdle");
+ // Finally make sure ASTWorker has processed all of the preamble updates.
+ return wait(Lock, RequestsCV, Timeout,
+ [&] { return PreambleRequests.empty() && !CurrentRequest; });
}
// Render a TUAction to a user-facing string representation.
@@ -1179,6 +1205,7 @@
const Options &Opts,
std::unique_ptr<ParsingCallbacks> Callbacks)
: CDB(CDB), StorePreamblesInMemory(Opts.StorePreamblesInMemory),
+ AsyncPreambleBuilds(Opts.AsyncPreambleBuilds),
Callbacks(Callbacks ? move(Callbacks)
: std::make_unique<ParsingCallbacks>()),
Barrier(Opts.AsyncThreadsCount),
@@ -1218,10 +1245,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<FileData>(
new FileData{Inputs.Contents, std::move(Worker)});
} else {
Index: clang-tools-extra/clangd/ClangdServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdServer.h
+++ 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<bool(const Tweak &)> TweakFilter = [](const Tweak &T) {
return !T.hidden(); // only enable non-hidden tweaks.
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
