[PATCH] D43648: [clangd] Debounce streams of updates.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL326546: [clangd] Debounce streams of updates. (authored by
sammccall, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D43648
Files:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/clangd/Threading.cpp
clang-tools-extra/trunk/clangd/Threading.h
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
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
@@ -42,7 +42,8 @@
TEST_F(TUSchedulerTests, MissingFiles) {
TUScheduler S(getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true,
-/*ASTParsedCallback=*/nullptr);
+/*ASTParsedCallback=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero());
auto Added = testPath("added.cpp");
Files[Added] = "";
@@ -94,9 +95,11 @@
// To avoid a racy test, don't allow tasks to actualy run on the worker
// thread until we've scheduled them all.
Notification Ready;
-TUScheduler S(getDefaultAsyncThreadsCount(),
- /*StorePreamblesInMemory=*/true,
- /*ASTParsedCallback=*/nullptr);
+TUScheduler S(
+getDefaultAsyncThreadsCount(),
+/*StorePreamblesInMemory=*/true,
+/*ASTParsedCallback=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero());
auto Path = testPath("foo.cpp");
S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
[&](std::vector) { Ready.wait(); });
@@ -118,6 +121,28 @@
EXPECT_EQ(2, CallbackCount);
}
+TEST_F(TUSchedulerTests, Debounce) {
+ std::atomic CallbackCount(0);
+ {
+TUScheduler S(getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ /*ASTParsedCallback=*/nullptr,
+ /*UpdateDebounce=*/std::chrono::milliseconds(50));
+auto Path = testPath("foo.cpp");
+S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) {
+ ADD_FAILURE() << "auto should have been debounced and canceled";
+ });
+std::this_thread::sleep_for(std::chrono::milliseconds(10));
+S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) { ++CallbackCount; });
+std::this_thread::sleep_for(std::chrono::milliseconds(60));
+S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) { ++CallbackCount; });
+ }
+ EXPECT_EQ(2, CallbackCount);
+}
+
TEST_F(TUSchedulerTests, ManyUpdates) {
const int FilesCount = 3;
const int UpdatesPerFile = 10;
@@ -131,7 +156,8 @@
{
TUScheduler S(getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true,
- /*ASTParsedCallback=*/nullptr);
+ /*ASTParsedCallback=*/nullptr,
+ /*UpdateDebounce=*/std::chrono::milliseconds(50));
std::vector Files;
for (int I = 0; I < FilesCount; ++I) {
Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
@@ -405,7 +405,7 @@
: Out(Out), CDB(std::move(CompileCommandsDir)), CCOpts(CCOpts),
Server(CDB, /*DiagConsumer=*/*this, FSProvider, AsyncThreadsCount,
StorePreamblesInMemory, BuildDynamicSymbolIndex, StaticIdx,
- ResourceDir) {}
+ ResourceDir, /*UpdateDebounce=*/std::chrono::milliseconds(500)) {}
bool ClangdLSPServer::run(std::istream , JSONStreamStyle InputStyle) {
assert(!IsDone && "Run was called before");
Index: clang-tools-extra/trunk/clangd/TUScheduler.h
===
--- clang-tools-extra/trunk/clangd/TUScheduler.h
+++ clang-tools-extra/trunk/clangd/TUScheduler.h
@@ -17,6 +17,7 @@
namespace clang {
namespace clangd {
+
/// Returns a number of a default async threads to use for TUScheduler.
/// Returned value is always >= 1 (i.e. will not cause requests to be processed
/// synchronously).
@@ -46,10 +47,12 @@
/// and scheduling tasks.
/// Callbacks are run on a threadpool and it's appropriate to do slow work in
/// them. Each task has a name, used for tracing (should be UpperCamelCase).
+/// FIXME(sammccall): pull out a scheduler options
[PATCH] D43648: [clangd] Debounce streams of updates.
ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43648: [clangd] Debounce streams of updates.
sammccall updated this revision to Diff 136045.
sammccall marked an inline comment as done.
sammccall added a comment.
review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43648
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
@@ -42,7 +42,8 @@
TEST_F(TUSchedulerTests, MissingFiles) {
TUScheduler S(getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true,
-/*ASTParsedCallback=*/nullptr);
+/*ASTParsedCallback=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero());
auto Added = testPath("added.cpp");
Files[Added] = "";
@@ -94,9 +95,11 @@
// To avoid a racy test, don't allow tasks to actualy run on the worker
// thread until we've scheduled them all.
Notification Ready;
-TUScheduler S(getDefaultAsyncThreadsCount(),
- /*StorePreamblesInMemory=*/true,
- /*ASTParsedCallback=*/nullptr);
+TUScheduler S(
+getDefaultAsyncThreadsCount(),
+/*StorePreamblesInMemory=*/true,
+/*ASTParsedCallback=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero());
auto Path = testPath("foo.cpp");
S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
[&](std::vector) { Ready.wait(); });
@@ -118,6 +121,28 @@
EXPECT_EQ(2, CallbackCount);
}
+TEST_F(TUSchedulerTests, Debounce) {
+ std::atomic CallbackCount(0);
+ {
+TUScheduler S(getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ /*ASTParsedCallback=*/nullptr,
+ /*UpdateDebounce=*/std::chrono::milliseconds(50));
+auto Path = testPath("foo.cpp");
+S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) {
+ ADD_FAILURE() << "auto should have been debounced and canceled";
+ });
+std::this_thread::sleep_for(std::chrono::milliseconds(10));
+S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) { ++CallbackCount; });
+std::this_thread::sleep_for(std::chrono::milliseconds(60));
+S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) { ++CallbackCount; });
+ }
+ EXPECT_EQ(2, CallbackCount);
+}
+
TEST_F(TUSchedulerTests, ManyUpdates) {
const int FilesCount = 3;
const int UpdatesPerFile = 10;
@@ -131,7 +156,8 @@
{
TUScheduler S(getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true,
- /*ASTParsedCallback=*/nullptr);
+ /*ASTParsedCallback=*/nullptr,
+ /*UpdateDebounce=*/std::chrono::milliseconds(50));
std::vector Files;
for (int I = 0; I < FilesCount; ++I) {
Index: clangd/Threading.h
===
--- clangd/Threading.h
+++ clangd/Threading.h
@@ -50,18 +50,50 @@
std::size_t FreeSlots;
};
-/// A point in time we may wait for, or None to wait forever.
+/// A point in time we can wait for.
+/// Can be zero (don't wait) or infinity (wait forever).
/// (Not time_point::max(), because many std::chrono implementations overflow).
-using Deadline = llvm::Optional;
-/// Makes a deadline from a timeout in seconds.
+class Deadline {
+public:
+ Deadline(std::chrono::steady_clock::time_point Time)
+ : Type(Finite), Time(Time) {}
+ static Deadline zero() { return Deadline(Zero); }
+ static Deadline infinity() { return Deadline(Infinite); }
+
+ std::chrono::steady_clock::time_point time() const {
+assert(Type == Finite);
+return Time;
+ }
+ bool expired() const {
+return (Type == Zero) ||
+ (Type == Finite && Time < std::chrono::steady_clock::now());
+ }
+ bool operator==(const Deadline ) const {
+return (Type == Other.Type) && (Type != Finite || Time == Other.Time);
+ }
+
+private:
+ enum Type { Zero, Infinite, Finite };
+
+ Deadline(enum Type Type) : Type(Type) {}
+ enum Type Type;
+ std::chrono::steady_clock::time_point Time;
+};
+
+/// Makes a deadline from a timeout in seconds. None means wait forever.
Deadline timeoutSeconds(llvm::Optional Seconds);
+/// Wait once on CV for the specified duration.
+void wait(std::unique_lock , std::condition_variable ,
+ Deadline D);
/// Waits on a condition variable until F() is true or D expires.
template
LLVM_NODISCARD bool wait(std::unique_lock ,
std::condition_variable , Deadline D, Func F) {
- if (D)
[PATCH] D43648: [clangd] Debounce streams of updates.
sammccall marked 4 inline comments as done.
sammccall added a comment.
Thanks for catching all those things!
Comment at: unittests/clangd/TUSchedulerTests.cpp:127
+ /*ASTParsedCallback=*/nullptr,
+ /*UpdateDebounce=*/std::chrono::milliseconds(50));
+auto Path = testPath("foo.cpp");
ilya-biryukov wrote:
> I wonder if the default debounce of `500ms` will make other tests (especially
> those that use `ClangdServer`) too slow?
> Maybe we should consider settings a smaller default (maybe even
> `Deadline::zero()`?) and having `500ms` set only by `ClangdLSPServer`?
Good point.
For now I've changed the ClangdServer default to 20ms, with a comment that this
is for tests (to make sure we're testing the "real" codepath).
After pulling out the options struct, I'd like to gave a function that returns
default options to be used in tests (in-memory preambles, fixed number of
threads, short debounce) and make
500ms the "real" default.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43648: [clangd] Debounce streams of updates.
ilya-biryukov added inline comments.
Comment at: clangd/TUScheduler.h:56
+ std::chrono::steady_clock::duration UpdateDebounce =
+ std::chrono::milliseconds(500));
~TUScheduler();
Maybe we could remove this default argument now that `ClangdServer` also has
the default debounce?
I guess the key thing that bothers me is having multiple places that have the
magical default constant of `500ms`, it would be nice to have it in one place.
Comment at: clangd/Threading.h:76
+private:
+ enum Type { Zero, Infinite, Finite } Type;
+ Deadline(enum Type Type) : Type(Type) {}
Maybe prefer grouping all the fields together? It seems we're giving that up to
avoid writing one extra instance of `enum Type`.
```
enum Type { Zero, Infinite, Finite };
Deadline(enum Type Type) : Type(Type) {}
enum Type Type;
std::chrono::steady_clock::time_point Time;
```
Comment at: clangd/Threading.h:83
Deadline timeoutSeconds(llvm::Optional Seconds);
+/// Wait once for on CV for the specified duration.
+void wait(std::unique_lock , std::condition_variable ,
s/Wait for on/Wait on/?
Comment at: unittests/clangd/TUSchedulerTests.cpp:127
+ /*ASTParsedCallback=*/nullptr,
+ /*UpdateDebounce=*/std::chrono::milliseconds(50));
+auto Path = testPath("foo.cpp");
I wonder if the default debounce of `500ms` will make other tests (especially
those that use `ClangdServer`) too slow?
Maybe we should consider settings a smaller default (maybe even
`Deadline::zero()`?) and having `500ms` set only by `ClangdLSPServer`?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43648: [clangd] Debounce streams of updates.
sammccall updated this revision to Diff 135661.
sammccall added a comment.
rebase
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43648
Files:
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
@@ -118,6 +118,28 @@
EXPECT_EQ(2, CallbackCount);
}
+TEST_F(TUSchedulerTests, Debounce) {
+ std::atomic CallbackCount(0);
+ {
+TUScheduler S(getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ /*ASTParsedCallback=*/nullptr,
+ /*UpdateDebounce=*/std::chrono::milliseconds(50));
+auto Path = testPath("foo.cpp");
+S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) {
+ ADD_FAILURE() << "auto should have been debounced and canceled";
+ });
+std::this_thread::sleep_for(std::chrono::milliseconds(10));
+S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) { ++CallbackCount; });
+std::this_thread::sleep_for(std::chrono::milliseconds(60));
+S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) { ++CallbackCount; });
+ }
+ EXPECT_EQ(2, CallbackCount);
+}
+
TEST_F(TUSchedulerTests, ManyUpdates) {
const int FilesCount = 3;
const int UpdatesPerFile = 10;
Index: clangd/Threading.h
===
--- clangd/Threading.h
+++ clangd/Threading.h
@@ -50,18 +50,48 @@
std::size_t FreeSlots;
};
-/// A point in time we may wait for, or None to wait forever.
+/// A point in time we can wait for.
+/// Can be zero (don't wait) or infinity (wait forever).
/// (Not time_point::max(), because many std::chrono implementations overflow).
-using Deadline = llvm::Optional;
-/// Makes a deadline from a timeout in seconds.
+class Deadline {
+public:
+ Deadline(std::chrono::steady_clock::time_point Time)
+ : Type(Finite), Time(Time) {}
+ static Deadline zero() { return Deadline(Zero); }
+ static Deadline infinity() { return Deadline(Infinite); }
+
+ std::chrono::steady_clock::time_point time() const {
+assert(Type == Finite);
+return Time;
+ }
+ bool expired() const {
+return (Type == Zero) ||
+ (Type == Finite && Time < std::chrono::steady_clock::now());
+ }
+ bool operator==(const Deadline ) const {
+return (Type == Other.Type) && (Type != Finite || Time == Other.Time);
+ }
+
+private:
+ enum Type { Zero, Infinite, Finite } Type;
+ Deadline(enum Type Type) : Type(Type) {}
+ std::chrono::steady_clock::time_point Time;
+};
+
+/// Makes a deadline from a timeout in seconds. None means wait forever.
Deadline timeoutSeconds(llvm::Optional Seconds);
+/// Wait once for on CV for the specified duration.
+void wait(std::unique_lock , std::condition_variable ,
+ Deadline D);
/// Waits on a condition variable until F() is true or D expires.
template
LLVM_NODISCARD bool wait(std::unique_lock ,
std::condition_variable , Deadline D, Func F) {
- if (D)
-return CV.wait_until(Lock, *D, F);
- CV.wait(Lock, F);
+ while (!F()) {
+if (D.expired())
+ return false;
+wait(Lock, CV, D);
+ }
return true;
}
@@ -73,7 +103,7 @@
/// Destructor waits for all pending tasks to finish.
~AsyncTaskRunner();
- void wait() const { (void) wait(llvm::None); }
+ void wait() const { (void)wait(Deadline::infinity()); }
LLVM_NODISCARD bool wait(Deadline D) const;
// The name is used for tracing and debugging (e.g. to name a spawned thread).
void runAsync(llvm::Twine Name, UniqueFunction Action);
Index: clangd/Threading.cpp
===
--- clangd/Threading.cpp
+++ clangd/Threading.cpp
@@ -76,10 +76,19 @@
Deadline timeoutSeconds(llvm::Optional Seconds) {
using namespace std::chrono;
if (!Seconds)
-return llvm::None;
+return Deadline::infinity();
return steady_clock::now() +
duration_cast(duration(*Seconds));
}
+void wait(std::unique_lock , std::condition_variable ,
+ Deadline D) {
+ if (D == Deadline::zero())
+return;
+ if (D == Deadline::infinity())
+return CV.wait(Lock);
+ CV.wait_until(Lock, D.time());
+}
+
} // namespace clangd
} // namespace clang
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -17,6 +17,7 @@
namespace clang {
namespace clangd {
+
/// Returns a number of a default async threads to use for TUScheduler.
/// Returned
[PATCH] D43648: [clangd] Debounce streams of updates.
sammccall updated this revision to Diff 135660.
sammccall marked 3 inline comments as done.
sammccall added a comment.
Make Deadline a real type with 0/finite/inf semantics.
Pull out another wait() overload.
Expose debounce delay option in clangdserver.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43648
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/CodeComplete.cpp
clangd/DraftStore.cpp
clangd/DraftStore.h
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/Headers.cpp
clangd/Protocol.cpp
clangd/Protocol.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
clangd/Threading.cpp
clangd/Threading.h
unittests/clangd/ClangdTests.cpp
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,58 @@
S.remove(Added);
}
+TEST_F(TUSchedulerTests, WantDiagnostics) {
+ std::atomic CallbackCount(0);
+ {
+// To avoid a racy test, don't allow tasks to actualy run on the worker
+// thread until we've scheduled them all.
+Notification Ready;
+TUScheduler S(getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ /*ASTParsedCallback=*/nullptr);
+auto Path = testPath("foo.cpp");
+S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
+ [&](std::vector) { Ready.wait(); });
+
+S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
+ [&](std::vector Diags) { ++CallbackCount; });
+S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) {
+ ADD_FAILURE() << "auto should have been cancelled by auto";
+ });
+S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No,
+ [&](std::vector Diags) {
+ ADD_FAILURE() << "no diags should not be called back";
+ });
+S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) { ++CallbackCount; });
+Ready.notify();
+ }
+ EXPECT_EQ(2, CallbackCount);
+}
+
+TEST_F(TUSchedulerTests, Debounce) {
+ std::atomic CallbackCount(0);
+ {
+TUScheduler S(getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ /*ASTParsedCallback=*/nullptr,
+ /*UpdateDebounce=*/std::chrono::milliseconds(50));
+auto Path = testPath("foo.cpp");
+S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) {
+ ADD_FAILURE() << "auto should have been debounced and canceled";
+ });
+std::this_thread::sleep_for(std::chrono::milliseconds(10));
+S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) { ++CallbackCount; });
+std::this_thread::sleep_for(std::chrono::milliseconds(60));
+S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) { ++CallbackCount; });
+ }
+ EXPECT_EQ(2, CallbackCount);
+}
+
TEST_F(TUSchedulerTests, ManyUpdates) {
const int FilesCount = 3;
const int UpdatesPerFile = 10;
@@ -132,7 +184,7 @@
{
WithContextValue WithNonce(NonceKey, ++Nonce);
- S.update(File, Inputs,
+ S.update(File, Inputs, WantDiagnostics::Auto,
[Nonce, , ](
llvm::Optional Diags) {
EXPECT_THAT(Context::current().get(NonceKey),
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -7,6 +7,7 @@
//
//===--===//
+#include "Annotations.h"
#include "ClangdLSPServer.h"
#include "ClangdServer.h"
#include "Matchers.h"
@@ -77,6 +78,43 @@
VFSTag LastVFSTag = VFSTag();
};
+/// For each file, record whether the last published diagnostics contained at
+/// least one error.
+class MultipleErrorCheckingDiagConsumer : public DiagnosticsConsumer {
+public:
+ void
+ onDiagnosticsReady(PathRef File,
+ Tagged Diagnostics) override {
+bool HadError = diagsContainErrors(Diagnostics.Value);
+
+
[PATCH] D43648: [clangd] Debounce streams of updates.
sammccall added inline comments.
Comment at: clangd/TUScheduler.cpp:324
+if (*Deadline)
+ RequestsCV.wait_until(Lock, **Deadline);
+else
ilya-biryukov wrote:
> It looks like if we unwrap `Optional` to `Deadline`, we could
> replace this code with `wait` helper from `Threading.h`.
> The tracing code (e.g. `if (!Requests.empty) { /*...*/}`) could be changed to
> log only when `*Deadline - steady_clock::now()` is positive.
> Will probably make the code simpler. WDYT?
Made Deadline a real type, and added a wait() overload without a condition.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43648: [clangd] Debounce streams of updates.
ilya-biryukov added inline comments.
Comment at: clangd/TUScheduler.cpp:140
+ // Time to wait after an update to see whether another update obsoletes it.
+ steady_clock::duration UpdateDebounce;
};
Maybe make it `const` and put beside `RunSync`? Both are scheduling options, so
it probably makes sense to group them together.
Comment at: clangd/TUScheduler.cpp:324
+if (*Deadline)
+ RequestsCV.wait_until(Lock, **Deadline);
+else
It looks like if we unwrap `Optional` to `Deadline`, we could replace
this code with `wait` helper from `Threading.h`.
The tracing code (e.g. `if (!Requests.empty) { /*...*/}`) could be changed to
log only when `*Deadline - steady_clock::now()` is positive.
Will probably make the code simpler. WDYT?
Comment at: clangd/TUScheduler.cpp:358
+ for (const auto : Requests)
+if (R.UpdateType == None || R.UpdateType == WantDiagnostics::Yes)
+ return None;
NIT: I tend to find multi-level nested statements easier to read with braces,
e.g.
```
for (const auto : Requests) {
if (Cond)
return None
}
```
But this is up to you.
Comment at: clangd/TUScheduler.h:55
+ ASTParsedCallback ASTCallback,
+ std::chrono::steady_clock::duration UpdateDebounce =
+ std::chrono::milliseconds(500));
As discussed offline, we want to expose the debounce parameter in
`ClangdServer`, as there are existing clients of the code that already send
updates with low frequency and it would be wasteful for them to do any extra
waits.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43648: [clangd] Debounce streams of updates.
sammccall created this revision.
sammccall added reviewers: hokein, ilya-biryukov.
Herald added subscribers: cfe-commits, ioeric, jkorous-apple, klimek.
Don't actually start building ASTs for new revisions until either:
- 500ms have passed since the last revision, or
- we actually need the revision for something (or to unblock the queue)
In practice, this avoids the "first keystroke results in diagnostics" problem.
This is kind of awkward to test, and the test is pretty bad.
It can be observed nicely by capturing a trace, though.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43648
Files:
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -118,6 +118,28 @@
EXPECT_EQ(2, CallbackCount);
}
+TEST_F(TUSchedulerTests, Debounce) {
+ std::atomic CallbackCount(0);
+ {
+TUScheduler S(getDefaultAsyncThreadsCount(),
+ /*StorePreamblesInMemory=*/true,
+ /*ASTParsedCallback=*/nullptr,
+ /*UpdateDebounce=*/std::chrono::milliseconds(50));
+auto Path = testPath("foo.cpp");
+S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) {
+ ADD_FAILURE() << "auto should have been debounced and canceled";
+ });
+std::this_thread::sleep_for(std::chrono::milliseconds(10));
+S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) { ++CallbackCount; });
+std::this_thread::sleep_for(std::chrono::milliseconds(60));
+S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
+ [&](std::vector Diags) { ++CallbackCount; });
+ }
+ EXPECT_EQ(2, CallbackCount);
+}
+
TEST_F(TUSchedulerTests, ManyUpdates) {
const int FilesCount = 3;
const int UpdatesPerFile = 10;
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -17,6 +17,7 @@
namespace clang {
namespace clangd {
+
/// Returns a number of a default async threads to use for TUScheduler.
/// Returned value is always >= 1 (i.e. will not cause requests to be processed
/// synchronously).
@@ -46,10 +47,13 @@
/// and scheduling tasks.
/// Callbacks are run on a threadpool and it's appropriate to do slow work in
/// them. Each task has a name, used for tracing (should be UpperCamelCase).
+/// FIXME(sammccall): pull out a scheduler options struct.
class TUScheduler {
public:
TUScheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory,
- ASTParsedCallback ASTCallback);
+ ASTParsedCallback ASTCallback,
+ std::chrono::steady_clock::duration UpdateDebounce =
+ std::chrono::milliseconds(500));
~TUScheduler();
/// Returns estimated memory usage for each of the currently open files.
@@ -101,6 +105,7 @@
// asynchronously.
llvm::Optional PreambleTasks;
llvm::Optional WorkerThreads;
+ std::chrono::steady_clock::duration UpdateDebounce;
};
} // namespace clangd
} // namespace clang
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -54,6 +54,7 @@
namespace clang {
namespace clangd {
+using std::chrono::steady_clock;
namespace {
class ASTWorkerHandle;
@@ -69,17 +70,18 @@
/// worker.
class ASTWorker {
friend class ASTWorkerHandle;
- ASTWorker(llvm::StringRef File, Semaphore , CppFile AST,
-bool RunSync);
+ ASTWorker(llvm::StringRef File, Semaphore , CppFile AST, bool RunSync,
+steady_clock::duration UpdateDebounce);
public:
/// Create a new ASTWorker and return a handle to it.
/// The processing thread is spawned using \p Tasks. However, when \p Tasks
/// is null, all requests will be processed on the calling thread
/// synchronously instead. \p Barrier is acquired when processing each
/// request, it is be used to limit the number of actively running threads.
static ASTWorkerHandle Create(llvm::StringRef File, AsyncTaskRunner *Tasks,
-Semaphore , CppFile AST);
+Semaphore , CppFile AST,
+steady_clock::duration UpdateDebounce);
~ASTWorker();
void update(ParseInputs Inputs, WantDiagnostics,
@@ -101,12 +103,19 @@
/// Adds a new task to the end of the request queue.
void startTask(llvm::StringRef Name, UniqueFunction Task,
llvm::Optional UpdateType);
+ /// Determines the next action to perform.
+ /// All actions that should never run are disarded.
+ /// If the next action is ready, returns None to indicate this.
+
