This revision was automatically updated to reflect the committed changes.
Closed by commit rL341076: [clangd] Run SignatureHelp using an up-to-date
preamble, waiting if needed. (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D51438
Files:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
Index: clang-tools-extra/trunk/clangd/TUScheduler.h
===================================================================
--- clang-tools-extra/trunk/clangd/TUScheduler.h
+++ clang-tools-extra/trunk/clangd/TUScheduler.h
@@ -119,19 +119,28 @@
void runWithAST(llvm::StringRef Name, PathRef File,
Callback<InputsAndAST> Action);
- /// Schedule an async read of the Preamble.
- /// The preamble may be stale, generated from an older version of the file.
- /// Reading from locations in the preamble may cause the files to be re-read.
- /// This gives callers two options:
- /// - validate that the preamble is still valid, and only use it in this case
- /// - accept that preamble contents may be outdated, and try to avoid reading
- /// source code from headers.
+ /// Controls whether preamble reads wait for the preamble to be up-to-date.
+ enum PreambleConsistency {
+ /// The preamble is generated from the current version of the file.
+ /// If the content was recently updated, we will wait until we have a
+ /// preamble that reflects that update.
+ /// This is the slowest option, and may be delayed by other tasks.
+ Consistent,
+ /// The preamble may be generated from an older version of the file.
+ /// Reading from locations in the preamble may cause files to be re-read.
+ /// This gives callers two options:
+ /// - validate that the preamble is still valid, and only use it if so
+ /// - accept that the preamble contents may be outdated, and try to avoid
+ /// reading source code from headers.
+ /// This is the fastest option, usually a preamble is available immediately.
+ Stale,
+ };
+ /// Schedule an async read of the preamble.
/// If there's no preamble yet (because the file was just opened), we'll wait
- /// for it to build. The preamble may still be null if it fails to build or is
- /// empty.
- /// If an error occurs during processing, it is forwarded to the \p Action
- /// callback.
+ /// for it to build. The result may be null if it fails to build or is empty.
+ /// If an error occurs, it is forwarded to the \p Action callback.
void runWithPreamble(llvm::StringRef Name, PathRef File,
+ PreambleConsistency Consistency,
Callback<InputsAndPreamble> Action);
/// Wait until there are no scheduled or running tasks.
Index: clang-tools-extra/trunk/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/trunk/clangd/TUScheduler.cpp
+++ clang-tools-extra/trunk/clangd/TUScheduler.cpp
@@ -183,6 +183,10 @@
bool blockUntilIdle(Deadline Timeout) const;
std::shared_ptr<const PreambleData> getPossiblyStalePreamble() const;
+ /// Obtain a preamble reflecting all updates so far. Threadsafe.
+ /// It may be delivered immediately, or later on the worker thread.
+ void getCurrentPreamble(
+ llvm::unique_function<void(std::shared_ptr<const PreambleData>)>);
/// Wait for the first build of preamble to finish. Preamble itself can be
/// accessed via getPossibleStalePreamble(). Note that this function will
/// return after an unsuccessful build of the preamble too, i.e. result of
@@ -464,6 +468,34 @@
return LastBuiltPreamble;
}
+void ASTWorker::getCurrentPreamble(
+ llvm::unique_function<void(std::shared_ptr<const PreambleData>)> Callback) {
+ // 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.
+ std::unique_lock<std::mutex> Lock(Mutex);
+ auto LastUpdate =
+ std::find_if(Requests.rbegin(), Requests.rend(),
+ [](const Request &R) { return R.UpdateType.hasValue(); });
+ // If there were no writes in the queue, the preamble is ready now.
+ if (LastUpdate == Requests.rend()) {
+ Lock.unlock();
+ return Callback(getPossiblyStalePreamble());
+ }
+ assert(!RunSync && "Running synchronously, but queue is non-empty!");
+ Requests.insert(LastUpdate.base(),
+ Request{Bind(
+ [this](decltype(Callback) Callback) {
+ Callback(getPossiblyStalePreamble());
+ },
+ std::move(Callback)),
+ "GetPreamble", steady_clock::now(),
+ Context::current().clone(),
+ /*UpdateType=*/llvm::None});
+ Lock.unlock();
+ RequestsCV.notify_all();
+}
+
void ASTWorker::waitForFirstPreamble() const {
PreambleWasBuilt.wait();
}
@@ -711,7 +743,7 @@
}
void TUScheduler::runWithPreamble(
- llvm::StringRef Name, PathRef File,
+ llvm::StringRef Name, PathRef File, PreambleConsistency Consistency,
llvm::unique_function<void(llvm::Expected<InputsAndPreamble>)> Action) {
auto It = Files.find(File);
if (It == Files.end()) {
@@ -731,31 +763,49 @@
return;
}
+ // Future is populated if the task needs a specific preamble.
+ std::future<std::shared_ptr<const PreambleData>> ConsistentPreamble;
+ if (Consistency == Consistent) {
+ std::promise<std::shared_ptr<const PreambleData>> Promise;
+ ConsistentPreamble = Promise.get_future();
+ It->second->Worker->getCurrentPreamble(Bind(
+ [](decltype(Promise) Promise,
+ std::shared_ptr<const PreambleData> Preamble) {
+ Promise.set_value(std::move(Preamble));
+ },
+ std::move(Promise)));
+ }
+
std::shared_ptr<const ASTWorker> Worker = It->second->Worker.lock();
auto Task = [Worker, this](std::string Name, std::string File,
std::string Contents,
tooling::CompileCommand Command, Context Ctx,
+ decltype(ConsistentPreamble) ConsistentPreamble,
decltype(Action) Action) mutable {
- // We don't want to be running preamble actions before the preamble was
- // built for the first time. This avoids extra work of processing the
- // preamble headers in parallel multiple times.
- Worker->waitForFirstPreamble();
+ std::shared_ptr<const PreambleData> Preamble;
+ if (ConsistentPreamble.valid()) {
+ Preamble = ConsistentPreamble.get();
+ } else {
+ // We don't want to be running preamble actions before the preamble was
+ // built for the first time. This avoids extra work of processing the
+ // preamble headers in parallel multiple times.
+ Worker->waitForFirstPreamble();
+ Preamble = Worker->getPossiblyStalePreamble();
+ }
std::lock_guard<Semaphore> BarrierLock(Barrier);
WithContext Guard(std::move(Ctx));
trace::Span Tracer(Name);
SPAN_ATTACH(Tracer, "file", File);
- std::shared_ptr<const PreambleData> Preamble =
- Worker->getPossiblyStalePreamble();
Action(InputsAndPreamble{Contents, Command, Preamble.get()});
};
PreambleTasks->runAsync(
"task:" + llvm::sys::path::filename(File),
Bind(Task, std::string(Name), std::string(File), It->second->Contents,
It->second->Command,
Context::current().derive(kFileBeingProcessed, File),
- std::move(Action)));
+ std::move(ConsistentPreamble), std::move(Action)));
}
std::vector<std::pair<Path, std::size_t>>
Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp
@@ -230,7 +230,8 @@
// is called as soon as results are available.
};
- WorkScheduler.runWithPreamble("CodeComplete", File,
+ // We use a potentially-stale preamble because latency is critical here.
+ WorkScheduler.runWithPreamble("CodeComplete", File, TUScheduler::Stale,
Bind(Task, File.str(), std::move(CB)));
return TH;
}
@@ -252,7 +253,11 @@
IP->Contents, Pos, FS, PCHs, Index));
};
- WorkScheduler.runWithPreamble("SignatureHelp", File,
+ // Unlike code completion, we wait for an up-to-date preamble here.
+ // Signature help is often triggered after code completion. If the code
+ // completion inserted a header to make the symbol available, then using
+ // the old preamble would yield useless results.
+ WorkScheduler.runWithPreamble("SignatureHelp", File, TUScheduler::Consistent,
Bind(Action, File.str(), std::move(CB)));
}
Index: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
===================================================================
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
@@ -22,6 +22,7 @@
using ::testing::_;
using ::testing::AnyOf;
using ::testing::Each;
+using ::testing::ElementsAre;
using ::testing::Pair;
using ::testing::Pointee;
using ::testing::UnorderedElementsAre;
@@ -63,7 +64,7 @@
ASSERT_FALSE(bool(AST));
ignoreError(AST.takeError());
});
- S.runWithPreamble("", Missing,
+ S.runWithPreamble("", Missing, TUScheduler::Stale,
[&](llvm::Expected<InputsAndPreamble> Preamble) {
ASSERT_FALSE(bool(Preamble));
ignoreError(Preamble.takeError());
@@ -75,20 +76,22 @@
S.runWithAST("", Added, [&](llvm::Expected<InputsAndAST> AST) {
EXPECT_TRUE(bool(AST));
});
- S.runWithPreamble("", Added, [&](llvm::Expected<InputsAndPreamble> Preamble) {
- EXPECT_TRUE(bool(Preamble));
- });
+ S.runWithPreamble("", Added, TUScheduler::Stale,
+ [&](llvm::Expected<InputsAndPreamble> Preamble) {
+ EXPECT_TRUE(bool(Preamble));
+ });
S.remove(Added);
// Assert that all operations fail after removing the file.
S.runWithAST("", Added, [&](llvm::Expected<InputsAndAST> AST) {
ASSERT_FALSE(bool(AST));
ignoreError(AST.takeError());
});
- S.runWithPreamble("", Added, [&](llvm::Expected<InputsAndPreamble> Preamble) {
- ASSERT_FALSE(bool(Preamble));
- ignoreError(Preamble.takeError());
- });
+ S.runWithPreamble("", Added, TUScheduler::Stale,
+ [&](llvm::Expected<InputsAndPreamble> Preamble) {
+ ASSERT_FALSE(bool(Preamble));
+ ignoreError(Preamble.takeError());
+ });
// remove() shouldn't crash on missing files.
S.remove(Added);
}
@@ -148,6 +151,64 @@
EXPECT_EQ(2, CallbackCount);
}
+static std::vector<std::string> includes(const PreambleData *Preamble) {
+ std::vector<std::string> Result;
+ if (Preamble)
+ for (const auto &Inclusion : Preamble->Includes.MainFileIncludes)
+ Result.push_back(Inclusion.Written);
+ return Result;
+}
+
+TEST_F(TUSchedulerTests, PreambleConsistency) {
+ std::atomic<int> CallbackCount(0);
+ {
+ Notification InconsistentReadDone; // Must live longest.
+ TUScheduler S(
+ getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+ noopParsingCallbacks(),
+ /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ ASTRetentionPolicy());
+ auto Path = testPath("foo.cpp");
+ // Schedule two updates (A, B) and two preamble reads (stale, consistent).
+ // The stale read should see A, and the consistent read should see B.
+ // (We recognize the preambles by their included files).
+ S.update(Path, getInputs(Path, "#include <A>"), WantDiagnostics::Yes,
+ [&](std::vector<Diag> Diags) {
+ // This callback runs in between the two preamble updates.
+
+ // This blocks update B, preventing it from winning the race
+ // against the stale read.
+ // If the first read was instead consistent, this would deadlock.
+ InconsistentReadDone.wait();
+ // This delays update B, preventing it from winning a race
+ // against the consistent read. The consistent read sees B
+ // only because it waits for it.
+ // If the second read was stale, it would usually see A.
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ });
+ S.update(Path, getInputs(Path, "#include <B>"), WantDiagnostics::Yes,
+ [&](std::vector<Diag> Diags) {});
+
+ S.runWithPreamble("StaleRead", Path, TUScheduler::Stale,
+ [&](llvm::Expected<InputsAndPreamble> Pre) {
+ ASSERT_TRUE(bool(Pre));
+ assert(bool(Pre));
+ EXPECT_THAT(includes(Pre->Preamble),
+ ElementsAre("<A>"));
+ InconsistentReadDone.notify();
+ ++CallbackCount;
+ });
+ S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent,
+ [&](llvm::Expected<InputsAndPreamble> Pre) {
+ ASSERT_TRUE(bool(Pre));
+ EXPECT_THAT(includes(Pre->Preamble),
+ ElementsAre("<B>"));
+ ++CallbackCount;
+ });
+ }
+ EXPECT_EQ(2, CallbackCount);
+}
+
TEST_F(TUSchedulerTests, ManyUpdates) {
const int FilesCount = 3;
const int UpdatesPerFile = 10;
@@ -229,7 +290,7 @@
{
WithContextValue WithNonce(NonceKey, ++Nonce);
S.runWithPreamble(
- "CheckPreamble", File,
+ "CheckPreamble", File, TUScheduler::Stale,
[File, Inputs, Nonce, &Mut, &TotalPreambleReads](
llvm::Expected<InputsAndPreamble> Preamble) {
EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
@@ -324,7 +385,7 @@
auto WithEmptyPreamble = R"cpp(int main() {})cpp";
S.update(Foo, getInputs(Foo, WithPreamble), WantDiagnostics::Auto,
[](std::vector<Diag>) {});
- S.runWithPreamble("getNonEmptyPreamble", Foo,
+ S.runWithPreamble("getNonEmptyPreamble", Foo, TUScheduler::Stale,
[&](llvm::Expected<InputsAndPreamble> Preamble) {
// We expect to get a non-empty preamble.
EXPECT_GT(cantFail(std::move(Preamble))
@@ -340,7 +401,7 @@
[](std::vector<Diag>) {});
// Wait for the preamble is being built.
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
- S.runWithPreamble("getEmptyPreamble", Foo,
+ S.runWithPreamble("getEmptyPreamble", Foo, TUScheduler::Stale,
[&](llvm::Expected<InputsAndPreamble> Preamble) {
// We expect to get an empty preamble.
EXPECT_EQ(cantFail(std::move(Preamble))
@@ -372,7 +433,7 @@
[](std::vector<Diag>) {});
for (int I = 0; I < ReadsToSchedule; ++I) {
S.runWithPreamble(
- "test", Foo,
+ "test", Foo, TUScheduler::Stale,
[I, &PreamblesMut, &Preambles](llvm::Expected<InputsAndPreamble> IP) {
std::lock_guard<std::mutex> Lock(PreamblesMut);
Preambles[I] = cantFail(std::move(IP)).Preamble;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
