sammccall updated this revision to Diff 135382.
sammccall marked an inline comment as done.
sammccall added a comment.
add param names to decls
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 WD,
+ 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 WD = 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 WD,
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
