sammccall updated this revision to Diff 163121.
sammccall added a comment.
Finish a sentence
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51438
Files:
clangd/ClangdServer.cpp
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===================================================================
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -63,7 +63,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 +75,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);
}
@@ -229,7 +231,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 +326,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 +342,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 +374,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;
Index: clangd/TUScheduler.h
===================================================================
--- clangd/TUScheduler.h
+++ 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: clangd/TUScheduler.cpp
===================================================================
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -183,6 +183,11 @@
bool blockUntilIdle(Deadline Timeout) const;
std::shared_ptr<const PreambleData> getPossiblyStalePreamble() const;
+ /// Obtain a preamble reflecting all updates so far. It may be delivered
+ /// immediately, or later on the worker thread if it's not yet ready.
+ /// Threadsafe, but should not be called from 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 +469,36 @@
return LastBuiltPreamble;
}
+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.
+ 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());
+ }
+ 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 +746,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 +766,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: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ 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)));
}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
