sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, ioeric, jkorous-apple, klimek.
Through the C++ API, we support for a given snapshot version:
- Yes: make sure we generate diagnostics for exactly this version
- Auto: generate eventually-consistent diagnostics for at least this version
- No: don't generate diagnostics for this version
Eventually auto should be debounced for better UX.
Through LSP, we force diagnostics for initial load (bypassing future debouncing)
and all updates follow the "auto" policy.
This is complicated to implement under the CancellationFlag design, so
rewrote that part to just inspect the queue instead.
It turns out we never pass None to the diagnostics callback, so remove Optional
from the signature. The questionable behavior of not invoking the callback at
all if CppFile::rebuild fails is not changed.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43518
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
clangd/Threading.cpp
clangd/Threading.h
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===================================================================
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -50,7 +50,7 @@
auto Missing = testPath("missing.cpp");
Files[Missing] = "";
- S.update(Added, getInputs(Added, ""), ignoreUpdate);
+ S.update(Added, getInputs(Added, ""), WantDiagnostics::No, ignoreUpdate);
// Assert each operation for missing file is an error (even if it's available
// in VFS).
@@ -88,6 +88,37 @@
S.remove(Added);
}
+TEST_F(TUSchedulerTests, WantDiagnostics) {
+ std::atomic<int> CallbackCount(0);
+ {
+ TUScheduler S(getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ /*ASTParsedCallback=*/nullptr);
+ auto Path = testPath("foo.cpp");
+
+ // To avoid a racy test, don't allow tasks to actualy run on the worker
+ // thread until we've scheduled them all.
+ Notification Ready;
+ S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
+ [&](std::vector<DiagWithFixIts>) { Ready.wait(); });
+
+ S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
+ [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; });
+ S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto,
+ [&](std::vector<DiagWithFixIts> Diags) {
+ ADD_FAILURE() << "auto should have been cancelled by auto";
+ });
+ S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No,
+ [&](std::vector<DiagWithFixIts> Diags) {
+ ADD_FAILURE() << "no diags should not be called back";
+ });
+ S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
+ [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; });
+ Ready.notify();
+ }
+ EXPECT_EQ(2, CallbackCount);
+}
+
TEST_F(TUSchedulerTests, ManyUpdates) {
const int FilesCount = 3;
const int UpdatesPerFile = 10;
@@ -132,7 +163,7 @@
{
WithContextValue WithNonce(NonceKey, ++Nonce);
- S.update(File, Inputs,
+ S.update(File, Inputs, WantDiagnostics::Auto,
[Nonce, &Mut, &TotalUpdates](
llvm::Optional<std::vector<DiagWithFixIts>> Diags) {
EXPECT_THAT(Context::current().get(NonceKey),
Index: clangd/Threading.h
===================================================================
--- clangd/Threading.h
+++ clangd/Threading.h
@@ -13,7 +13,6 @@
#include "Context.h"
#include "Function.h"
#include "llvm/ADT/Twine.h"
-#include <atomic>
#include <cassert>
#include <condition_variable>
#include <memory>
@@ -23,24 +22,18 @@
namespace clang {
namespace clangd {
-/// A shared boolean flag indicating if the computation was cancelled.
-/// Once cancelled, cannot be returned to the previous state.
-class CancellationFlag {
+/// A threadsafe flag that is initially clear.
+class Notification {
public:
- CancellationFlag();
-
- void cancel() {
- assert(WasCancelled && "the object was moved");
- WasCancelled->store(true);
- }
-
- bool isCancelled() const {
- assert(WasCancelled && "the object was moved");
- return WasCancelled->load();
- }
+ // Sets the flag. No-op if already set.
+ void notify();
+ // Blocks until flag is set.
+ void wait() const;
private:
- std::shared_ptr<std::atomic<bool>> WasCancelled;
+ bool Notified = false;
+ mutable std::condition_variable CV;
+ mutable std::mutex Mu;
};
/// Limits the number of threads that can acquire the lock at the same time.
Index: clangd/Threading.cpp
===================================================================
--- clangd/Threading.cpp
+++ clangd/Threading.cpp
@@ -7,8 +7,18 @@
namespace clang {
namespace clangd {
-CancellationFlag::CancellationFlag()
- : WasCancelled(std::make_shared<std::atomic<bool>>(false)) {}
+void Notification::notify() {
+ {
+ std::lock_guard<std::mutex> Lock(Mu);
+ Notified = true;
+ }
+ CV.notify_all();
+}
+
+void Notification::wait() const {
+ std::unique_lock<std::mutex> Lock(Mu);
+ CV.wait(Lock, [this] { return Notified; });
+}
Semaphore::Semaphore(std::size_t MaxLocks) : FreeSlots(MaxLocks) {}
Index: clangd/TUScheduler.h
===================================================================
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -32,6 +32,14 @@
const PreambleData *Preamble;
};
+/// Determines whether diagnostics should be generated for a file snapshot.
+enum class WantDiagnostics {
+ Yes, /// Diagnostics must be generated for this snapshot.
+ No, /// Diagnostics must not be generated for this snapshot.
+ Auto, /// Diagnostics must be generated for this snapshot or a subsequent one,
+ /// within a bounded amount of time.
+};
+
/// Handles running tasks for ClangdServer and managing the resources (e.g.,
/// preambles and ASTs) for opened files.
/// TUScheduler is not thread-safe, only one thread should be providing updates
@@ -51,9 +59,8 @@
/// Schedule an update for \p File. Adds \p File to a list of tracked files if
/// \p File was not part of it before.
/// FIXME(ibiryukov): remove the callback from this function.
- void update(PathRef File, ParseInputs Inputs,
- UniqueFunction<void(llvm::Optional<std::vector<DiagWithFixIts>>)>
- OnUpdated);
+ void update(PathRef File, ParseInputs Inputs, WantDiagnostics,
+ UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated);
/// Remove \p File from the list of tracked files and schedule removal of its
/// resources.
Index: clangd/TUScheduler.cpp
===================================================================
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -82,9 +82,8 @@
Semaphore &Barrier, CppFile AST);
~ASTWorker();
- void update(ParseInputs Inputs,
- UniqueFunction<void(llvm::Optional<std::vector<DiagWithFixIts>>)>
- OnUpdated);
+ void update(ParseInputs Inputs, WantDiagnostics,
+ UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated);
void runWithAST(llvm::StringRef Name,
UniqueFunction<void(llvm::Expected<InputsAndAST>)> Action);
bool blockUntilIdle(Deadline Timeout) const;
@@ -101,12 +100,15 @@
void stop();
/// Adds a new task to the end of the request queue.
void startTask(llvm::StringRef Name, UniqueFunction<void()> Task,
- bool isUpdate, llvm::Optional<CancellationFlag> CF);
+ llvm::Optional<WantDiagnostics> UpdateType);
+ /// Should the first task in the queue be skipped instead of run?
+ bool shouldSkipHeadLocked();
struct Request {
UniqueFunction<void()> Action;
std::string Name;
Context Ctx;
+ llvm::Optional<WantDiagnostics> UpdateType;
};
std::string File;
@@ -123,10 +125,7 @@
std::size_t LastASTSize; /* GUARDED_BY(Mutex) */
// Set to true to signal run() to finish processing.
bool Done; /* GUARDED_BY(Mutex) */
- std::queue<Request> Requests; /* GUARDED_BY(Mutex) */
- // Only set when last request is an update. This allows us to cancel an update
- // that was never read, if a subsequent update comes in.
- llvm::Optional<CancellationFlag> LastUpdateCF; /* GUARDED_BY(Mutex) */
+ std::deque<Request> Requests; /* GUARDED_BY(Mutex) */
mutable std::condition_variable RequestsCV;
};
@@ -200,14 +199,9 @@
}
void ASTWorker::update(
- ParseInputs Inputs,
- UniqueFunction<void(llvm::Optional<std::vector<DiagWithFixIts>>)>
- OnUpdated) {
- auto Task = [=](CancellationFlag CF, decltype(OnUpdated) OnUpdated) mutable {
- if (CF.isCancelled()) {
- OnUpdated(llvm::None);
- return;
- }
+ ParseInputs Inputs, WantDiagnostics WantDiags,
+ UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated) {
+ auto Task = [=](decltype(OnUpdated) OnUpdated) mutable {
FileInputs = Inputs;
auto Diags = AST.rebuild(std::move(Inputs));
@@ -220,12 +214,11 @@
// We want to report the diagnostics even if this update was cancelled.
// It seems more useful than making the clients wait indefinitely if they
// spam us with updates.
- OnUpdated(std::move(Diags));
+ if (Diags && WantDiags != WantDiagnostics::No)
+ OnUpdated(std::move(*Diags));
};
- CancellationFlag UpdateCF;
- startTask("Update", BindWithForward(Task, UpdateCF, std::move(OnUpdated)),
- /*isUpdate=*/true, UpdateCF);
+ startTask("Update", BindWithForward(Task, std::move(OnUpdated)), WantDiags);
}
void ASTWorker::runWithAST(
@@ -247,7 +240,7 @@
};
startTask(Name, BindWithForward(Task, std::move(Action)),
- /*isUpdate=*/false, llvm::None);
+ /*UpdateType=*/llvm::None);
}
std::shared_ptr<const PreambleData>
@@ -271,10 +264,7 @@
}
void ASTWorker::startTask(llvm::StringRef Name, UniqueFunction<void()> Task,
- bool isUpdate, llvm::Optional<CancellationFlag> CF) {
- assert(isUpdate == CF.hasValue() &&
- "Only updates are expected to pass CancellationFlag");
-
+ llvm::Optional<WantDiagnostics> UpdateType) {
if (RunSync) {
assert(!Done && "running a task after stop()");
trace::Span Tracer(Name + ":" + llvm::sys::path::filename(File));
@@ -285,18 +275,9 @@
{
std::lock_guard<std::mutex> Lock(Mutex);
assert(!Done && "running a task after stop()");
- if (isUpdate) {
- if (!Requests.empty() && LastUpdateCF) {
- // There were no reads for the last unprocessed update, let's cancel it
- // to not waste time on it.
- LastUpdateCF->cancel();
- }
- LastUpdateCF = std::move(*CF);
- } else {
- LastUpdateCF = llvm::None;
- }
- Requests.push({std::move(Task), Name, Context::current().clone()});
- } // unlock Mutex.
+ Requests.push_back(
+ {std::move(Task), Name, Context::current().clone(), UpdateType});
+ }
RequestsCV.notify_all();
}
@@ -313,6 +294,9 @@
// Even when Done is true, we finish processing all pending requests
// before exiting the processing loop.
+ while (shouldSkipHeadLocked())
+ Requests.pop_front();
+ assert(!Requests.empty() && "skipped the whole queue");
Req = std::move(Requests.front());
// Leave it on the queue for now, so waiters don't see an empty queue.
} // unlock Mutex
@@ -326,12 +310,40 @@
{
std::lock_guard<std::mutex> Lock(Mutex);
- Requests.pop();
+ Requests.pop_front();
}
RequestsCV.notify_all();
}
}
+// Returns true if Requests.front() is a dead update that can be skipped.
+bool ASTWorker::shouldSkipHeadLocked() {
+ assert(!Requests.empty());
+ auto Next = Requests.begin();
+ auto UpdateType = Next->UpdateType;
+ if (!UpdateType) // Only skip updates.
+ return false;
+ ++Next;
+ // An update is live if its AST might still be read.
+ // That is, if it's not immediately followed by another update.
+ if (Next == Requests.end() || !Next->UpdateType)
+ return false;
+ // The other way an update can be live is if its diagnostics might be used.
+ switch (*UpdateType) {
+ case WantDiagnostics::Yes:
+ return false; // Always used.
+ case WantDiagnostics::No:
+ return true; // Always dead.
+ case WantDiagnostics::Auto:
+ // Used unless followed by an update that generates diagnostics.
+ for (; Next != Requests.end(); ++Next)
+ if (Next->UpdateType == WantDiagnostics::Yes ||
+ Next->UpdateType == WantDiagnostics::Auto)
+ return true; // Prefer later diagnostics.
+ return false;
+ }
+}
+
bool ASTWorker::blockUntilIdle(Deadline Timeout) const {
std::unique_lock<std::mutex> Lock(Mutex);
return wait(Lock, RequestsCV, Timeout, [&] { return Requests.empty(); });
@@ -389,9 +401,8 @@
}
void TUScheduler::update(
- PathRef File, ParseInputs Inputs,
- UniqueFunction<void(llvm::Optional<std::vector<DiagWithFixIts>>)>
- OnUpdated) {
+ PathRef File, ParseInputs Inputs, WantDiagnostics WantDiags,
+ UniqueFunction<void(std::vector<DiagWithFixIts>)> OnUpdated) {
std::unique_ptr<FileData> &FD = Files[File];
if (!FD) {
// Create a new worker to process the AST-related tasks.
@@ -402,7 +413,7 @@
} else {
FD->Inputs = Inputs;
}
- FD->Worker->update(std::move(Inputs), std::move(OnUpdated));
+ FD->Worker->update(std::move(Inputs), WantDiags, std::move(OnUpdated));
}
void TUScheduler::remove(PathRef File) {
Index: clangd/ClangdServer.h
===================================================================
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -150,7 +150,8 @@
/// \p File is already tracked. Also schedules parsing of the AST for it on a
/// separate thread. When the parsing is complete, DiagConsumer passed in
/// constructor will receive onDiagnosticsReady callback.
- void addDocument(PathRef File, StringRef Contents);
+ void addDocument(PathRef File, StringRef Contents,
+ WantDiagnostics = WantDiagnostics::Auto);
/// Remove \p File from list of tracked files, schedule a request to free
/// resources associated with it.
@@ -278,6 +279,7 @@
void
scheduleReparseAndDiags(PathRef File, VersionedDraft Contents,
+ WantDiagnostics,
Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS);
CompileArgsCache CompileArgs;
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -110,11 +110,12 @@
this->RootPath = NewRootPath;
}
-void ClangdServer::addDocument(PathRef File, StringRef Contents) {
+void ClangdServer::addDocument(PathRef File, StringRef Contents,
+ WantDiagnostics WantDiags) {
DocVersion Version = DraftMgr.updateDraft(File, Contents);
auto TaggedFS = FSProvider.getTaggedFileSystem(File);
scheduleReparseAndDiags(File, VersionedDraft{Version, Contents.str()},
- std::move(TaggedFS));
+ WantDiags, std::move(TaggedFS));
}
void ClangdServer::removeDocument(PathRef File) {
@@ -133,7 +134,8 @@
CompileArgs.invalidate(File);
auto TaggedFS = FSProvider.getTaggedFileSystem(File);
- scheduleReparseAndDiags(File, std::move(FileContents), std::move(TaggedFS));
+ scheduleReparseAndDiags(File, std::move(FileContents), WantDiagnostics::Yes,
+ std::move(TaggedFS));
}
void ClangdServer::codeComplete(
@@ -519,20 +521,16 @@
}
void ClangdServer::scheduleReparseAndDiags(
- PathRef File, VersionedDraft Contents,
+ PathRef File, VersionedDraft Contents, WantDiagnostics WantDiags,
Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS) {
tooling::CompileCommand Command = CompileArgs.getCompileCommand(File);
- using OptDiags = llvm::Optional<std::vector<DiagWithFixIts>>;
-
DocVersion Version = Contents.Version;
Path FileStr = File.str();
VFSTag Tag = std::move(TaggedFS.Tag);
- auto Callback = [this, Version, FileStr, Tag](OptDiags Diags) {
- if (!Diags)
- return; // A new reparse was requested before this one completed.
-
+ auto Callback = [this, Version, FileStr,
+ Tag](std::vector<DiagWithFixIts> Diags) {
// We need to serialize access to resulting diagnostics to avoid calling
// `onDiagnosticsReady` in the wrong order.
std::lock_guard<std::mutex> DiagsLock(DiagnosticsMutex);
@@ -546,14 +544,14 @@
LastReportedDiagsVersion = Version;
DiagConsumer.onDiagnosticsReady(
- FileStr, make_tagged(std::move(*Diags), std::move(Tag)));
+ FileStr, make_tagged(std::move(Diags), std::move(Tag)));
};
WorkScheduler.update(File,
ParseInputs{std::move(Command),
std::move(TaggedFS.Value),
std::move(*Contents.Draft)},
- std::move(Callback));
+ WantDiags, std::move(Callback));
}
void ClangdServer::onFileEvent(const DidChangeWatchedFilesParams &Params) {
Index: clangd/ClangdLSPServer.cpp
===================================================================
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -141,16 +141,17 @@
if (Params.metadata && !Params.metadata->extraFlags.empty())
CDB.setExtraFlagsForFile(Params.textDocument.uri.file(),
std::move(Params.metadata->extraFlags));
- Server.addDocument(Params.textDocument.uri.file(), Params.textDocument.text);
+ Server.addDocument(Params.textDocument.uri.file(), Params.textDocument.text,
+ WantDiagnostics::Yes);
}
void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) {
if (Params.contentChanges.size() != 1)
return replyError(ErrorCode::InvalidParams,
"can only apply one change at a time");
// We only support full syncing right now.
Server.addDocument(Params.textDocument.uri.file(),
- Params.contentChanges[0].text);
+ Params.contentChanges[0].text, WantDiagnostics::Auto);
}
void ClangdLSPServer::onFileEvent(DidChangeWatchedFilesParams &Params) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
