[clangd] Debounce streams of updates.

Summary:
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.

Reviewers: hokein, ilya-biryukov

Subscribers: klimek, jkorous-apple, ioeric, cfe-commits

Differential Revision: https://reviews.llvm.org/D43648

llvm-svn: 326546

clang-tools-extra/clangd/ClangdLSPServer.cpp

@@ -405,7 +405,7 @@ ClangdLSPServer::ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount,
     : 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 &In, JSONStreamStyle InputStyle) {
   assert(!IsDone && "Run was called before");

clang-tools-extra/clangd/ClangdServer.cpp

@@ -76,7 +76,8 @@ ClangdServer::ClangdServer(GlobalCompilationDatabase &CDB,
                            unsigned AsyncThreadsCount,
                            bool StorePreamblesInMemory,
                            bool BuildDynamicSymbolIndex, SymbolIndex *StaticIdx,
-                           llvm::Optional<StringRef> ResourceDir)
+                           llvm::Optional<StringRef> ResourceDir,
+                           std::chrono::steady_clock::duration UpdateDebounce)
     : CompileArgs(CDB,
                   ResourceDir ? ResourceDir->str() : getStandardResourceDir()),
       DiagConsumer(DiagConsumer), FSProvider(FSProvider),
@@ -91,7 +92,8 @@ ClangdServer::ClangdServer(GlobalCompilationDatabase &CDB,
                     FileIdx
                         ? [this](PathRef Path,
                                  ParsedAST *AST) { FileIdx->update(Path, AST); }
-                        : ASTParsedCallback()) {
+                        : ASTParsedCallback(),
+                    UpdateDebounce) {
   if (FileIdx && StaticIdx) {
     MergedIndex = mergeIndex(FileIdx.get(), StaticIdx);
     Index = MergedIndex.get();

clang-tools-extra/clangd/ClangdServer.h

@@ -125,6 +125,8 @@ class ClangdServer {
   /// \p DiagConsumer. Note that a callback to \p DiagConsumer happens on a
   /// worker thread. Therefore, instances of \p DiagConsumer must properly
   /// synchronize access to shared state.
+  /// UpdateDebounce determines how long to wait after a new version of the file
+  /// before starting to compute diagnostics.
   ///
   /// \p StorePreamblesInMemory defines whether the Preambles generated by
   /// clangd are stored in-memory or on disk.
@@ -135,13 +137,17 @@ class ClangdServer {
   ///
   /// If \p StaticIdx is set, ClangdServer uses the index for global code
   /// completion.
+  /// FIXME(sammccall): pull out an options struct.
   ClangdServer(GlobalCompilationDatabase &CDB,
                DiagnosticsConsumer &DiagConsumer,
                FileSystemProvider &FSProvider, unsigned AsyncThreadsCount,
                bool StorePreamblesInMemory,
                bool BuildDynamicSymbolIndex = false,
                SymbolIndex *StaticIdx = nullptr,
-               llvm::Optional<StringRef> ResourceDir = llvm::None);
+               llvm::Optional<StringRef> ResourceDir = llvm::None,
+               /* Tiny default debounce, so tests hit the debounce logic */
+               std::chrono::steady_clock::duration UpdateDebounce =
+                   std::chrono::milliseconds(20));
 
   /// Set the root path of the workspace.
   void setRootPath(PathRef RootPath);

clang-tools-extra/clangd/TUScheduler.cpp

@@ -54,6 +54,7 @@
 
 namespace clang {
 namespace clangd {
+using std::chrono::steady_clock;
 namespace {
 class ASTWorkerHandle;
 
@@ -69,8 +70,8 @@ class ASTWorkerHandle;
 /// worker.
 class ASTWorker {
   friend class ASTWorkerHandle;
-  ASTWorker(llvm::StringRef File, Semaphore &Barrier, CppFile AST,
-            bool RunSync);
+  ASTWorker(llvm::StringRef File, Semaphore &Barrier, CppFile AST, bool RunSync,
+            steady_clock::duration UpdateDebounce);
 
 public:
   /// Create a new ASTWorker and return a handle to it.
@@ -79,7 +80,8 @@ class ASTWorker {
   /// 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 &Barrier, CppFile AST);
+                                Semaphore &Barrier, CppFile AST,
+                                steady_clock::duration UpdateDebounce);
   ~ASTWorker();
 
   void update(ParseInputs Inputs, WantDiagnostics,
@@ -101,18 +103,27 @@ class ASTWorker {
   /// Adds a new task to the end of the request queue.
   void startTask(llvm::StringRef Name, UniqueFunction<void()> Task,
                  llvm::Optional<WantDiagnostics> UpdateType);
+  /// Determines the next action to perform.
+  /// All actions that should never run are disarded.
+  /// Returns a deadline for the next action. If it's expired, run now.
+  /// scheduleLocked() is called again at the deadline, or if requests arrive.
+  Deadline scheduleLocked();
   /// Should the first task in the queue be skipped instead of run?
   bool shouldSkipHeadLocked() const;
 
   struct Request {
     UniqueFunction<void()> Action;
     std::string Name;
+    steady_clock::time_point AddTime;
     Context Ctx;
     llvm::Optional<WantDiagnostics> UpdateType;
   };
 
-  std::string File;
+  const std::string File;
   const bool RunSync;
+  // Time to wait after an update to see whether another update obsoletes it.
+  const steady_clock::duration UpdateDebounce;
+
   Semaphore &Barrier;
   // AST and FileInputs are only accessed on the processing thread from run().
   CppFile AST;
@@ -172,9 +183,10 @@ class ASTWorkerHandle {
 };
 
 ASTWorkerHandle ASTWorker::Create(llvm::StringRef File, AsyncTaskRunner *Tasks,
-                                  Semaphore &Barrier, CppFile AST) {
-  std::shared_ptr<ASTWorker> Worker(
-      new ASTWorker(File, Barrier, std::move(AST), /*RunSync=*/!Tasks));
+                                  Semaphore &Barrier, CppFile AST,
+                                  steady_clock::duration UpdateDebounce) {
+  std::shared_ptr<ASTWorker> Worker(new ASTWorker(
+      File, Barrier, std::move(AST), /*RunSync=*/!Tasks, UpdateDebounce));
   if (Tasks)
     Tasks->runAsync("worker:" + llvm::sys::path::filename(File),
                     [Worker]() { Worker->run(); });
@@ -183,9 +195,9 @@ ASTWorkerHandle ASTWorker::Create(llvm::StringRef File, AsyncTaskRunner *Tasks,
 }
 
 ASTWorker::ASTWorker(llvm::StringRef File, Semaphore &Barrier, CppFile AST,
-                     bool RunSync)
-    : File(File), RunSync(RunSync), Barrier(Barrier), AST(std::move(AST)),
-      Done(false) {
+                     bool RunSync, steady_clock::duration UpdateDebounce)
+    : File(File), RunSync(RunSync), UpdateDebounce(UpdateDebounce),
+      Barrier(Barrier), AST(std::move(AST)), Done(false) {
   if (RunSync)
     return;
 }
@@ -275,8 +287,8 @@ void ASTWorker::startTask(llvm::StringRef Name, UniqueFunction<void()> Task,
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     assert(!Done && "running a task after stop()");
-    Requests.push_back(
-        {std::move(Task), Name, Context::current().clone(), UpdateType});
+    Requests.push_back({std::move(Task), Name, steady_clock::now(),
+                        Context::current().clone(), UpdateType});
   }
   RequestsCV.notify_all();
 }
@@ -286,17 +298,31 @@ void ASTWorker::run() {
     Request Req;
     {
       std::unique_lock<std::mutex> Lock(Mutex);
-      RequestsCV.wait(Lock, [&]() { return Done || !Requests.empty(); });
-      if (Requests.empty()) {
-        assert(Done);
-        return;
+      for (auto Wait = scheduleLocked(); !Wait.expired();
+           Wait = scheduleLocked()) {
+        if (Done) {
+          if (Requests.empty())
+            return;
+          else     // Even though Done is set, finish pending requests.
+            break; // However, skip delays to shutdown fast.
+        }
+
+        // Tracing: we have a next request, attribute this sleep to it.
+        Optional<WithContext> Ctx;
+        Optional<trace::Span> Tracer;
+        if (!Requests.empty()) {
+          Ctx.emplace(Requests.front().Ctx.clone());
+          Tracer.emplace("Debounce");
+          SPAN_ATTACH(*Tracer, "next_request", Requests.front().Name);
+          if (!(Wait == Deadline::infinity()))
+            SPAN_ATTACH(*Tracer, "sleep_ms",
+                        std::chrono::duration_cast<std::chrono::milliseconds>(
+                            Wait.time() - steady_clock::now())
+                            .count());
+        }
+
+        wait(Lock, RequestsCV, Wait);
       }
-      // 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
@@ -316,6 +342,24 @@ void ASTWorker::run() {
   }
 }
 
+Deadline ASTWorker::scheduleLocked() {
+  if (Requests.empty())
+    return Deadline::infinity(); // Wait for new requests.
+  while (shouldSkipHeadLocked())
+    Requests.pop_front();
+  assert(!Requests.empty() && "skipped the whole queue");
+  // Some updates aren't dead yet, but never end up being used.
+  // e.g. the first keystroke is live until obsoleted by the second.
+  // We debounce "maybe-unused" writes, sleeping 500ms in case they become dead.
+  // But don't delay reads (including updates where diagnostics are needed).
+  for (const auto &R : Requests)
+    if (R.UpdateType == None || R.UpdateType == WantDiagnostics::Yes)
+      return Deadline::zero();
+  // Front request needs to be debounced, so determine when we're ready.
+  Deadline D(Requests.front().AddTime + UpdateDebounce);
+  return D;
+}
+
 // Returns true if Requests.front() is a dead update that can be skipped.
 bool ASTWorker::shouldSkipHeadLocked() const {
   assert(!Requests.empty());
@@ -370,10 +414,12 @@ struct TUScheduler::FileData {
 
 TUScheduler::TUScheduler(unsigned AsyncThreadsCount,
                          bool StorePreamblesInMemory,
-                         ASTParsedCallback ASTCallback)
+                         ASTParsedCallback ASTCallback,
+                         steady_clock::duration UpdateDebounce)
     : StorePreamblesInMemory(StorePreamblesInMemory),
       PCHOps(std::make_shared<PCHContainerOperations>()),
-      ASTCallback(std::move(ASTCallback)), Barrier(AsyncThreadsCount) {
+      ASTCallback(std::move(ASTCallback)), Barrier(AsyncThreadsCount),
+      UpdateDebounce(UpdateDebounce) {
   if (0 < AsyncThreadsCount) {
     PreambleTasks.emplace();
     WorkerThreads.emplace();
@@ -409,7 +455,8 @@ void TUScheduler::update(
     // Create a new worker to process the AST-related tasks.
     ASTWorkerHandle Worker = ASTWorker::Create(
         File, WorkerThreads ? WorkerThreads.getPointer() : nullptr, Barrier,
-        CppFile(File, StorePreamblesInMemory, PCHOps, ASTCallback));
+        CppFile(File, StorePreamblesInMemory, PCHOps, ASTCallback),
+        UpdateDebounce);
     FD = std::unique_ptr<FileData>(new FileData{Inputs, std::move(Worker)});
   } else {
     FD->Inputs = Inputs;

clang-tools-extra/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 @@ enum class WantDiagnostics {
 /// 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);
   ~TUScheduler();
 
   /// Returns estimated memory usage for each of the currently open files.
@@ -101,6 +104,7 @@ class TUScheduler {
   // asynchronously.
   llvm::Optional<AsyncTaskRunner> PreambleTasks;
   llvm::Optional<AsyncTaskRunner> WorkerThreads;
+  std::chrono::steady_clock::duration UpdateDebounce;
 };
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/Threading.cpp

@@ -76,10 +76,19 @@ void AsyncTaskRunner::runAsync(llvm::Twine Name,
 Deadline timeoutSeconds(llvm::Optional<double> Seconds) {
   using namespace std::chrono;
   if (!Seconds)
-    return llvm::None;
+    return Deadline::infinity();
   return steady_clock::now() +
          duration_cast<steady_clock::duration>(duration<double>(*Seconds));
 }
 
+void wait(std::unique_lock<std::mutex> &Lock, std::condition_variable &CV,
+          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

clang-tools-extra/clangd/Threading.h

@@ -50,18 +50,50 @@ class Semaphore {
   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<std::chrono::steady_clock::time_point>;
-/// 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 &Other) 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<double> Seconds);
+/// Wait once on CV for the specified duration.
+void wait(std::unique_lock<std::mutex> &Lock, std::condition_variable &CV,
+          Deadline D);
 /// Waits on a condition variable until F() is true or D expires.
 template <typename Func>
 LLVM_NODISCARD bool wait(std::unique_lock<std::mutex> &Lock,
                          std::condition_variable &CV, 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 +105,7 @@ class AsyncTaskRunner {
   /// 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<void()> Action);