kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay,
javed.absar, ilya-biryukov.
Herald added a project: clang.
kadircet added a parent revision: D76304: [clangd] Update TUStatus api to
accommodate preamble thread.
This is another step for out-of-order preamble builds. To keep the
diagnostic behavior same, we only build ASTs either with "reusable" preambles,
the ones that are fully applicable to a given ParseInput, or after building a
new preamble. Which is the same behaviour as what we do today.
ASTs built through preamble callbacks are not cached as they are built on a
different thread and ASTWorker heavily relies on being the only thread updating
cached ASTs. This results in possibly building some ASTs twice (when there's an
immediate read after a preamble built without any write in between).
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D76725
Files:
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/Preamble.h
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/unittests/FileIndexTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
clang-tools-extra/clangd/unittests/TestTU.cpp
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -66,8 +66,7 @@
auto CI = buildCompilerInvocation(Inputs, Diags);
assert(CI && "Failed to build compilation invocation.");
auto Preamble =
- buildPreamble(FullFilename, *CI,
- /*OldPreamble=*/nullptr, Inputs,
+ buildPreamble(FullFilename, *CI, Inputs,
/*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
auto AST =
buildAST(FullFilename, std::move(CI), Diags.take(), Inputs, Preamble);
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -570,6 +570,17 @@
auto Bar = testPath("bar.cpp");
auto Baz = testPath("baz.cpp");
+ // Build all of the files once, so that we've got their preambles ready.
+ updateWithCallback(S, Foo, SourceContents, WantDiagnostics::Yes,
+ [&BuiltASTCounter]() { ++BuiltASTCounter; });
+ updateWithCallback(S, Bar, SourceContents, WantDiagnostics::Yes,
+ [&BuiltASTCounter]() { ++BuiltASTCounter; });
+ updateWithCallback(S, Baz, SourceContents, WantDiagnostics::Yes,
+ [&BuiltASTCounter]() { ++BuiltASTCounter; });
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+ ASSERT_EQ(BuiltASTCounter.load(), 3);
+ BuiltASTCounter = 0;
+
// Build one file in advance. We will not access it later, so it will be the
// one that the cache will evict.
updateWithCallback(S, Foo, SourceContents, WantDiagnostics::Yes,
@@ -697,26 +708,29 @@
};
// Test that subsequent updates with the same inputs do not cause rebuilds.
- ASSERT_TRUE(DoUpdate(SourceContents));
- ASSERT_FALSE(DoUpdate(SourceContents));
+ ASSERT_TRUE(DoUpdate(SourceContents)); // Builds preamble
+ ASSERT_TRUE(DoUpdate(SourceContents)); // Builds AST
+ ASSERT_FALSE(DoUpdate(SourceContents)); // Reuses it
// Update to a header should cause a rebuild, though.
Timestamps[Header] = time_t(1);
- ASSERT_TRUE(DoUpdate(SourceContents));
- ASSERT_FALSE(DoUpdate(SourceContents));
+ ASSERT_TRUE(DoUpdate(SourceContents)); // Builds preamble
+ ASSERT_TRUE(DoUpdate(SourceContents)); // Builds AST
+ ASSERT_FALSE(DoUpdate(SourceContents)); // Reuses AST
// Update to the contents should cause a rebuild.
auto OtherSourceContents = R"cpp(
#include "foo.h"
int c = d;
)cpp";
- ASSERT_TRUE(DoUpdate(OtherSourceContents));
- ASSERT_FALSE(DoUpdate(OtherSourceContents));
+ ASSERT_TRUE(DoUpdate(OtherSourceContents)); // Builds AST, reusing preamble
+ ASSERT_FALSE(DoUpdate(OtherSourceContents)); // Reuses AST
// Update to the compile commands should also cause a rebuild.
CDB.ExtraClangFlags.push_back("-DSOMETHING");
- ASSERT_TRUE(DoUpdate(OtherSourceContents));
- ASSERT_FALSE(DoUpdate(OtherSourceContents));
+ ASSERT_TRUE(DoUpdate(SourceContents)); // Builds preamble
+ ASSERT_TRUE(DoUpdate(OtherSourceContents)); // Builds AST
+ ASSERT_FALSE(DoUpdate(OtherSourceContents)); // Reuses it
}
TEST_F(TUSchedulerTests, ForceRebuild) {
@@ -732,6 +746,10 @@
ParseInputs Inputs = getInputs(Source, SourceContents);
+ // Send an initial update to make sure we've got preamble ready.
+ S.update(Source, Inputs, WantDiagnostics::Yes);
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+
// Update the source contents, which should trigger an initial build with
// the header file missing.
updateWithDiags(
@@ -853,13 +871,14 @@
TUState(PreambleAction::Idle, ASTAction::RunningAction),
// We build the preamble
TUState(PreambleAction::Building, ASTAction::RunningAction),
+ // We start building the ast, as a result of building
+ // preamble.
+ TUState(PreambleAction::Building, ASTAction::Building),
+ // AST built finished successfully
+ TUState(PreambleAction::Building, ASTAction::Building),
// 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
+ // Running go to def
TUState(PreambleAction::Idle, ASTAction::RunningAction),
// both workers go idle
TUState(PreambleAction::Idle, ASTAction::Idle)));
Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/FileIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp
@@ -286,7 +286,7 @@
FileIndex Index;
bool IndexUpdated = false;
- buildPreamble(FooCpp, *CI, /*OldPreamble=*/nullptr, PI,
+ buildPreamble(FooCpp, *CI, PI,
/*StoreInMemory=*/true,
[&](ASTContext &Ctx, std::shared_ptr<Preprocessor> PP,
const CanonicalIncludes &CanonIncludes) {
@@ -424,7 +424,7 @@
}
TEST(FileIndexTest, MergeMainFileSymbols) {
- const char* CommonHeader = "void foo();";
+ const char *CommonHeader = "void foo();";
TestTU Header = TestTU::withCode(CommonHeader);
TestTU Cpp = TestTU::withCode("void foo() {}");
Cpp.Filename = "foo.cpp";
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -67,11 +67,13 @@
#include "llvm/Support/Path.h"
#include "llvm/Support/Threading.h"
#include <algorithm>
+#include <functional>
#include <memory>
#include <mutex>
#include <queue>
#include <string>
#include <thread>
+#include <utility>
namespace clang {
namespace clangd {
@@ -196,6 +198,9 @@
/// built synchronously in update() instead.
class PreambleThread {
public:
+ using PreambleCallback =
+ std::function<void(std::unique_ptr<CompilerInvocation>, ParseInputs,
+ std::shared_ptr<const PreambleData>)>;
PreambleThread(llvm::StringRef FileName, ParsingCallbacks &Callbacks,
bool StorePreambleInMemory, bool RunSync,
SynchronizedTUStatus &Status)
@@ -211,7 +216,7 @@
/// It isn't guaranteed that each requested version will be built. If there
/// are multiple update requests while building a preamble, only the last one
/// will be built.
- void update(CompilerInvocation *CI, ParseInputs PI) {
+ void update(CompilerInvocation *CI, ParseInputs PI, PreambleCallback PC) {
// If compiler invocation was broken, just fail out early.
if (!CI) {
// Make sure anyone waiting for the preamble gets notified it could not be
@@ -220,7 +225,8 @@
return;
}
// Make possibly expensive copy while not holding the lock.
- Request Req = {std::make_unique<CompilerInvocation>(*CI), std::move(PI)};
+ Request Req = {std::make_unique<CompilerInvocation>(*CI), std::move(PI),
+ std::move(PC), Context::current().clone()};
if (RunSync) {
build(std::move(Req));
return;
@@ -259,6 +265,7 @@
CurrentReq = std::move(*NextReq);
NextReq.reset();
}
+ WithContext Guard(std::move(CurrentReq->Ctx));
// Build the preamble and let the waiters know about it.
build(std::move(*CurrentReq));
bool IsEmpty = false;
@@ -305,6 +312,8 @@
struct Request {
std::unique_ptr<CompilerInvocation> CI;
ParseInputs Inputs;
+ PreambleCallback PC;
+ Context Ctx;
};
bool isDone() {
@@ -319,21 +328,32 @@
void build(Request Req) {
assert(Req.CI && "Got preamble request with null compiler invocation");
const ParseInputs &Inputs = Req.Inputs;
- std::shared_ptr<const PreambleData> OldPreamble =
- Inputs.ForceRebuild ? nullptr : latest();
+
+ if (LatestBuild) {
+ vlog("Rebuilding invalidated preamble for {0} version {1} "
+ "(previous was version {2})",
+ FileName, Inputs.Version, LatestBuild->Version);
+ } else {
+ vlog("Building first preamble for {0} version {1}", FileName,
+ Inputs.Version);
+ }
Status.update([&](TUStatus &Status) {
Status.PreambleActivity = PreambleAction::Building;
});
auto Preamble = clang::clangd::buildPreamble(
- FileName, std::move(*Req.CI), OldPreamble, Inputs, StoreInMemory,
+ FileName, *Req.CI, Inputs, StoreInMemory,
[this, Version(Inputs.Version)](
ASTContext &Ctx, std::shared_ptr<clang::Preprocessor> PP,
const CanonicalIncludes &CanonIncludes) {
Callbacks.onPreambleAST(FileName, Version, Ctx, std::move(PP),
CanonIncludes);
});
+ // We run preamble callbacks before updating latest build preamble to make
+ // sure ASTWorker doesn't see the fresh preamble until preambleworker
+ // finishes.
+ Req.PC(std::move(Req.CI), std::move(Req.Inputs), Preamble);
{
std::lock_guard<std::mutex> Lock(Mutex);
// LatestBuild might be the last reference to old preamble, do not trigger
@@ -623,19 +643,54 @@
std::vector<std::string> CC1Args;
std::unique_ptr<CompilerInvocation> Invocation = buildCompilerInvocation(
Inputs, CompilerInvocationDiagConsumer, &CC1Args);
- // This is true for now, as we always block until new preamble is build.
- // Once we start to block preambles out-of-order we need to make sure
- // OldPreamble refers to the preamble that was used to build last AST.
- auto OldPreamble = PW.latest();
- PW.update(Invocation.get(), Inputs);
- // FIXME make use of the stale preambles instead.
- PW.blockUntilIdle(Deadline::infinity());
- auto NewPreamble = PW.latest();
// Log cc1 args even (especially!) if creating invocation failed.
if (!CC1Args.empty())
vlog("Driver produced command: cc1 {0}", llvm::join(CC1Args, " "));
std::vector<Diag> CompilerInvocationDiags =
CompilerInvocationDiagConsumer.take();
+ auto Preamble = Inputs.ForceRebuild ? nullptr : PW.latest();
+ bool PreambleFresh =
+ Invocation && Preamble &&
+ isPreambleUsable(*Preamble, Inputs, FileName, *Invocation);
+ if (!PreambleFresh) {
+ PW.update(
+ Invocation.get(), Inputs,
+ [this, WantDiags, CompilerInvocationDiags, RunPublish,
+ TaskName](std::unique_ptr<CompilerInvocation> CI, ParseInputs PI,
+ std::shared_ptr<const PreambleData> Preamble) {
+ if (WantDiags == WantDiagnostics::No)
+ return;
+ Status.update([TaskName = std::move(TaskName)](TUStatus &Status) {
+ Status.ASTActivity.K = ASTAction::Building;
+ Status.ASTActivity.Name = TaskName;
+ });
+ if (auto AST =
+ buildAST(FileName, std::move(CI), CompilerInvocationDiags,
+ std::move(PI), std::move(Preamble))) {
+ Status.update([](TUStatus &Status) {
+ Status.Details.BuildFailed = false;
+ Status.Details.ReuseAST = false;
+ });
+ trace::Span Span("Running main AST callback");
+ Callbacks.onMainAST(FileName, *AST, RunPublish);
+ // Note that we are not caching the AST we've just built, or
+ // updating ASTWorker state saying that we've ran AST callbacks.
+ // This results in building and running callbacks on some updates
+ // possibly twice, but makes reasoning about ASTWorker internals
+ // easier, as they are manipulated by only a single thread.
+ } else {
+ Status.update([](TUStatus &Status) {
+ Status.Details.BuildFailed = true;
+ Status.Details.ReuseAST = false;
+ });
+ }
+ });
+ } else {
+ vlog("Reusing preamble version {0} for version {1} of {2}",
+ Preamble->Version, Inputs.Version, FileName);
+ }
+ // FIXME: make use of the stale preambles instead.
+ PW.blockUntilIdle(Deadline::infinity());
if (!Invocation) {
elog("Could not build CompilerInvocation for file {0}", FileName);
// Remove the old AST if it's still in cache.
@@ -646,16 +701,20 @@
return;
}
- bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble);
- // Before doing the expensive AST reparse, we want to release our reference
- // to the old preamble, so it can be freed if there are no other references
- // to it.
- OldPreamble.reset();
+ // Do not build AST with stale preambles, as diagnostics might contain false
+ // positives and symbol information might be misleading.
+ if (!PreambleFresh) {
+ // FIXME: We should build patched-up ASTs in case of stale preambles and
+ // keep using them.
+ IdleASTs.take(this);
+ return;
+ }
+
Status.update([&](TUStatus &Status) {
Status.ASTActivity.K = ASTAction::Building;
Status.ASTActivity.Name = std::move(TaskName);
});
- if (!CanReuseAST) {
+ if (!InputsAreTheSame) {
IdleASTs.take(this); // Remove the old AST if it's still in cache.
} else {
// We don't need to rebuild the AST, check if we need to run the callback.
@@ -697,7 +756,7 @@
if (!AST) {
llvm::Optional<ParsedAST> NewAST =
buildAST(FileName, std::move(Invocation), CompilerInvocationDiags,
- Inputs, NewPreamble);
+ Inputs, Preamble);
// buildAST fails.
Status.update([&](TUStatus &Status) {
Status.Details.ReuseAST = false;
Index: clang-tools-extra/clangd/Preamble.h
===================================================================
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -27,7 +27,9 @@
#include "Diagnostics.h"
#include "FS.h"
#include "Headers.h"
+#include "Path.h"
#include "index/CanonicalIncludes.h"
+#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/PrecompiledPreamble.h"
#include "clang/Tooling/CompilationDatabase.h"
@@ -72,17 +74,19 @@
const CanonicalIncludes &)>;
/// Build a preamble for the new inputs unless an old one can be reused.
-/// If \p OldPreamble can be reused, it is returned unchanged.
-/// If \p OldPreamble is null, always builds the preamble.
/// If \p PreambleCallback is set, it will be run on top of the AST while
-/// building the preamble. Note that if the old preamble was reused, no AST is
-/// built and, therefore, the callback will not be executed.
+/// building the preamble.
std::shared_ptr<const PreambleData>
buildPreamble(PathRef FileName, CompilerInvocation CI,
- std::shared_ptr<const PreambleData> OldPreamble,
const ParseInputs &Inputs, bool StoreInMemory,
PreambleParsedCallback PreambleCallback);
+/// Returns true if \p Preamble is reusable for \p Inputs. Note that it will
+/// return true when some missing headers are now available.
+/// FIXME: Should return more information about the delta between \p Preamble
+/// and \p Inputs, e.g. new headers.
+bool isPreambleUsable(const PreambleData &Preamble, const ParseInputs &Inputs,
+ PathRef FileName, const CompilerInvocation &CI);
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/Preamble.cpp
===================================================================
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -90,7 +90,6 @@
std::shared_ptr<const PreambleData>
buildPreamble(PathRef FileName, CompilerInvocation CI,
- std::shared_ptr<const PreambleData> OldPreamble,
const ParseInputs &Inputs, bool StoreInMemory,
PreambleParsedCallback PreambleCallback) {
// Note that we don't need to copy the input contents, preamble can live
@@ -100,23 +99,6 @@
auto Bounds =
ComputePreambleBounds(*CI.getLangOpts(), ContentsBuffer.get(), 0);
- if (OldPreamble &&
- compileCommandsAreEqual(Inputs.CompileCommand,
- OldPreamble->CompileCommand) &&
- OldPreamble->Preamble.CanReuse(CI, ContentsBuffer.get(), Bounds,
- Inputs.FS.get())) {
- vlog("Reusing preamble version {0} for version {1} of {2}",
- OldPreamble->Version, Inputs.Version, FileName);
- return OldPreamble;
- }
- if (OldPreamble)
- vlog("Rebuilding invalidated preamble for {0} version {1} "
- "(previous was version {2})",
- FileName, Inputs.Version, OldPreamble->Version);
- else
- vlog("Building first preamble for {0} verson {1}", FileName,
- Inputs.Version);
-
trace::Span Tracer("BuildPreamble");
SPAN_ATTACH(Tracer, "File", FileName);
StoreDiags PreambleDiagnostics;
@@ -168,5 +150,16 @@
}
}
+bool isPreambleUsable(const PreambleData &Preamble, const ParseInputs &Inputs,
+ PathRef FileName, const CompilerInvocation &CI) {
+ auto ContentsBuffer =
+ llvm::MemoryBuffer::getMemBuffer(Inputs.Contents, FileName);
+ auto Bounds =
+ ComputePreambleBounds(*CI.getLangOpts(), ContentsBuffer.get(), 0);
+ return compileCommandsAreEqual(Inputs.CompileCommand,
+ Preamble.CompileCommand) &&
+ Preamble.Preamble.CanReuse(CI, ContentsBuffer.get(), Bounds,
+ Inputs.FS.get());
+}
} // namespace clangd
} // namespace clang
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
