kadircet created this revision.
Herald added subscribers: cfe-commits, usaxena95, jfb, arphaman, jkorous,
MaskRay, javed.absar, ilya-biryukov.
Herald added a project: clang.
kadircet added a parent revision: D76725: [clangd] Build ASTs only with fresh
preambles or after building a new preamble.
kadircet added a comment.
This requires patched-up ASTs and getting rid of consistent preamble needs
first.
Build preambles fully asynchronously.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D77063
Files:
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/unittests/ClangdTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
clang-tools-extra/clangd/unittests/XRefsTests.cpp
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -968,6 +968,7 @@
// Only preamble is built, and no AST is built in this request.
Server.addDocument(FooCpp, FooWithoutHeader.code(), "null",
WantDiagnostics::No);
+ ASSERT_TRUE(Server.blockUntilIdleForTest());
// We build AST here, and it should use the latest preamble rather than the
// stale one.
EXPECT_THAT(
@@ -979,6 +980,7 @@
// Both preamble and AST are built in this request.
Server.addDocument(FooCpp, FooWithoutHeader.code(), "null",
WantDiagnostics::Yes);
+ ASSERT_TRUE(Server.blockUntilIdleForTest());
// Use the AST being built in above request.
EXPECT_THAT(
cantFail(runLocateSymbolAt(Server, FooCpp, FooWithoutHeader.point())),
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -322,9 +322,11 @@
// Helper to schedule a named update and return a function to cancel it.
auto Update = [&](std::string ID) -> Canceler {
auto T = cancelableTask();
+ auto Inp = getInputs(Path, "//" + ID);
+ Inp.Version = ID;
WithContext C(std::move(T.first));
updateWithDiags(
- S, Path, "//" + ID, WantDiagnostics::Yes,
+ S, Path, std::move(Inp), WantDiagnostics::Yes,
[&, ID](std::vector<Diag> Diags) { DiagsSeen.push_back(ID); });
return std::move(T.second);
};
@@ -497,7 +499,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));
@@ -734,11 +736,15 @@
)cpp";
ParseInputs Inputs = getInputs(Source, SourceContents);
+ Inputs.Version = "MissingHeader";
+ std::atomic<int> UpdateCount(0);
// Update the source contents, which should trigger an initial build with
// the header file missing.
updateWithDiags(
- S, Source, Inputs, WantDiagnostics::Yes, [](std::vector<Diag> Diags) {
+ S, Source, Inputs, WantDiagnostics::Yes,
+ [&UpdateCount](std::vector<Diag> Diags) {
+ ++UpdateCount;
EXPECT_THAT(Diags,
ElementsAre(Field(&Diag::Message, "'foo.h' file not found"),
Field(&Diag::Message,
@@ -750,6 +756,7 @@
Files[Header] = "int a;";
Timestamps[Header] = time_t(1);
Inputs = getInputs(Source, SourceContents);
+ Inputs.Version = "WithHeader";
// The addition of the missing header file shouldn't trigger a rebuild since
// we don't track missing files.
@@ -761,11 +768,15 @@
// 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<Diag> Diags) { EXPECT_THAT(Diags, IsEmpty()); });
+ Inputs.Version = "ForceRebuild";
+ updateWithDiags(S, Source, Inputs, WantDiagnostics::Yes,
+ [&UpdateCount](std::vector<Diag> Diags) {
+ ++UpdateCount;
+ EXPECT_THAT(Diags, IsEmpty());
+ });
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+ EXPECT_EQ(UpdateCount, 2);
}
TEST_F(TUSchedulerTests, NoChangeDiags) {
TUScheduler S(CDB, optsForTest(), captureDiags());
@@ -849,27 +860,20 @@
ASSERT_TRUE(Server.blockUntilIdleForTest());
- EXPECT_THAT(CaptureTUStatus.allStatus(),
- ElementsAre(
- // Everything starts with ASTWorker starting to execute an
- // update
- TUState(PreambleAction::Idle, ASTAction::RunningAction),
- // We build the preamble
- TUState(PreambleAction::Building, ASTAction::RunningAction),
- // We built the preamble, and issued ast built on ASTWorker
- // thread. Preambleworker goes idle afterwards.
- TUState(PreambleAction::Idle, ASTAction::RunningAction),
- // Start task for building the ast, as a result of building
- // preamble, on astworker thread.
- TUState(PreambleAction::Idle, ASTAction::RunningAction),
- // AST build starts.
- TUState(PreambleAction::Idle, ASTAction::Building),
- // AST built finished successfully
- TUState(PreambleAction::Idle, ASTAction::Building),
- // Running go to def
- TUState(PreambleAction::Idle, ASTAction::RunningAction),
- // ASTWorker goes idle.
- TUState(PreambleAction::Idle, ASTAction::Idle)));
+ auto Statuses = CaptureTUStatus.allStatus();
+ const std::vector<PreambleAction> PreambleStatuses = {
+ PreambleAction::Idle, PreambleAction::Building, PreambleAction::Idle};
+
+ llvm::ArrayRef<PreambleAction> RemainingStatuses =
+ llvm::makeArrayRef(PreambleStatuses);
+ ASSERT_FALSE(Statuses.empty());
+ for (size_t I = 0, E = Statuses.size(); I != E; ++I) {
+ if (RemainingStatuses.front() != Statuses[I].PreambleActivity)
+ RemainingStatuses = RemainingStatuses.drop_front();
+ ASSERT_FALSE(RemainingStatuses.empty());
+ EXPECT_EQ(Statuses[I].PreambleActivity, RemainingStatuses.front());
+ }
+ EXPECT_EQ(RemainingStatuses.size(), 1U);
}
TEST_F(TUSchedulerTests, CommandLineErrors) {
Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -210,18 +210,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);
@@ -246,20 +246,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/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -230,6 +230,7 @@
/// will be built.
void update(const CompilerInvocation &CI, ParseInputs PI,
std::vector<Diag> CIDiags, WantDiagnostics WantDiags) {
+ dlog("Preamble worker queueing {0}", PI.Version);
// Make possibly expensive copy while not holding the lock.
Request Req = {std::make_unique<CompilerInvocation>(CI), std::move(PI),
std::move(CIDiags), std::move(WantDiags),
@@ -239,10 +240,14 @@
return;
}
{
- std::lock_guard<std::mutex> Lock(Mutex);
+ std::unique_lock<std::mutex> Lock(Mutex);
// If shutdown is issued, don't bother building.
if (Done)
return;
+ ReqCV.wait(Lock, [this] {
+ return !NextReq || NextReq->WantDiags != WantDiagnostics::Yes;
+ });
+ dlog("preamble worker queued {0}", Req.Inputs.Version);
NextReq = std::move(Req);
}
// Let the worker thread know there's a request, notify_one is safe as there
@@ -299,6 +304,7 @@
}
bool blockUntilIdle(Deadline Timeout) const {
+ dlog("blocking for preamble worker");
std::unique_lock<std::mutex> Lock(Mutex);
return wait(Lock, ReqCV, Timeout, [&] { return !NextReq && !CurrentReq; });
}
@@ -693,6 +699,8 @@
bool InputsAreTheSame =
std::tie(FileInputs->CompileCommand, FileInputs->Contents) ==
std::tie(Inputs.CompileCommand, Inputs.Contents);
+ if (InputsAreTheSame && RanASTCallback && !Inputs.ForceRebuild)
+ return;
std::string TaskName = llvm::formatv("Build AST ({0})", Inputs.Version);
Status.update([&](TUStatus &Status) {
@@ -831,10 +839,6 @@
IdleASTs.take(this);
PW.update(*Invocation, Inputs, std::move(CompilerInvocationDiags),
WantDiags);
- // FIXME: We block here to ensure any subsequent reads gets a fresh
- // preamble. Instead of blocking here, we should build patched-up ASTs on
- // reads or block only for picky reads.
- PW.blockUntilIdle(Deadline::infinity());
return;
}
vlog("ASTWorker: Reusing preamble version {0} for version {1} of {2}",
@@ -923,6 +927,8 @@
void ASTWorker::getCurrentPreamble(
llvm::unique_function<void(std::shared_ptr<const PreambleData>)> Callback) {
+ if (RunSync)
+ return Callback(getPossiblyStalePreamble());
// We could just call startTask() to throw the read on the queue, knowing
// it will run after any updates. But we know this task is cheap, so to
// improve latency we cheat: insert it on the queue after the last update.
@@ -930,23 +936,23 @@
auto LastUpdate =
std::find_if(Requests.rbegin(), Requests.rend(),
[](const Request &R) { return R.UpdateType.hasValue(); });
- // If there were no writes in the queue, and CurrentRequest is not a write,
- // the preamble is ready now.
- if (LastUpdate == Requests.rend() &&
- (!CurrentRequest || CurrentRequest->UpdateType.hasValue())) {
- Lock.unlock();
- return Callback(getPossiblyStalePreamble());
- }
- assert(!RunSync && "Running synchronously, but queue is non-empty!");
- Requests.insert(LastUpdate.base(),
- Request{[Callback = std::move(Callback), this]() mutable {
- Callback(getPossiblyStalePreamble());
- },
- "GetPreamble", steady_clock::now(),
- Context::current().clone(),
- /*UpdateType=*/None,
- /*InvalidationPolicy=*/TUScheduler::NoInvalidation,
- /*Invalidate=*/nullptr});
+ // FIXME: We should only serve stale preambles or build it here instead.
+ Requests.insert(
+ LastUpdate.base(),
+ Request{
+ [Callback = std::move(Callback), this]() mutable {
+ PW.blockUntilIdle(Deadline::infinity());
+ ReceivedPreambles.push(
+ {[Callback = std::move(Callback), this]() mutable {
+ Callback(getPossiblyStalePreamble());
+ },
+ "GetPreamble", steady_clock::now(), Context::current().clone(),
+ None, TUScheduler::NoInvalidation, nullptr});
+ },
+ "WaitForPreamble", steady_clock::now(), Context::current().clone(),
+ /*UpdateType=*/None,
+ /*InvalidationPolicy=*/TUScheduler::NoInvalidation,
+ /*Invalidate=*/nullptr});
Lock.unlock();
RequestsCV.notify_all();
}
@@ -996,6 +1002,7 @@
llvm::unique_function<void()> Task,
llvm::Optional<WantDiagnostics> UpdateType,
TUScheduler::ASTActionInvalidation Invalidation) {
+ dlog("ASTWorker queueing {0}", Name);
if (RunSync) {
assert(!Done && "running a task after stop()");
trace::Span Tracer(Name + ":" + llvm::sys::path::filename(FileName));
@@ -1188,9 +1195,18 @@
bool ASTWorker::blockUntilIdle(Deadline Timeout) const {
std::unique_lock<std::mutex> Lock(Mutex);
- return wait(Lock, RequestsCV, Timeout, [&] {
- return ReceivedPreambles.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 (!PW.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 ReceivedPreambles.empty() && !CurrentRequest; });
}
// Render a TUAction to a user-facing string representation.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
