[clangd] Replaced WorkerRequest with std::function...

[ilya-biryukov]

Summary

And implemented a helper function to dump an AST of a file for
testing/debugging purposes.

[krasimir]

clangd/ClangdServer.cpp

85

Why are we taking it from the front and not from the back? Maybe at least add a comment about this behavior.

102

Why did this get out?

230

Maybe an explicit flush and no block?

clangd/ClangdUnitStore.h

56

Maybe make it less generic and put the implementation in the source file? It uses std::function anyways.

[krasimir]

clangd/ClangdUnitStore.h

56

Sorry, I didn't see the surroundings. Disregard this comment.

62

Why can't we use runOnUnitImpl here?

[ilya-biryukov]

Added more comments to addToEnd/addToFront, added explicit flush instead of relying on raw_ostream's destructor to do it implicitly.

[ilya-biryukov]

clangd/ClangdServer.cpp

85

Added comments here and at the addToEnd/addToFront methods.

230

Ok.

clangd/ClangdUnitStore.h

62

runOnUnitImpl does parsing/reparsing if the ASTFile is not there, which this function doesn't do specifically.

[ilya-biryukov]

clangd/ClangdServer.cpp

102

"The notifying thread does not need to hold the lock on the same mutex as the one held by the waiting thread(s); in fact doing so is a pessimization, since the notified thread would immediately block again, waiting for the notifying thread to release the lock"
(from: http://en.cppreference.com/w/cpp/thread/condition_variable/notify_one)

[krasimir]

Looks good.

clangd/ClangdServer.h

 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 
 #include "ClangdUnit.h"
 #include "Protocol.h"
 
 #include <condition_variable>
+#include <functional>
 #include <mutex>
 #include <string>
 #include <thread>
 #include <utility>
 
 namespace clang {
 class PCHContainerOperations;
 

 public:
   virtual ~DiagnosticsConsumer() = default;
 
   /// Called by ClangdServer when \p Diagnostics for \p File are ready.
   virtual void onDiagnosticsReady(PathRef File,
                                   std::vector<DiagWithFixIts> Diagnostics) = 0;
 };
 
-enum class WorkerRequestKind { ParseAndPublishDiagnostics, RemoveDocData };
-
-/// A request to the worker thread
-class WorkerRequest {
-public:
-  WorkerRequest() = default;
-  WorkerRequest(WorkerRequestKind Kind, Path File, DocVersion Version);
-
-  WorkerRequestKind Kind;
-  Path File;
-  DocVersion Version;
-};
-
 class ClangdServer;
 
 /// Handles running WorkerRequests of ClangdServer on a separate threads.
 /// Currently runs only one worker thread.
 class ClangdScheduler {
 public:
-  ClangdScheduler(ClangdServer &Server, bool RunSynchronously);
+  ClangdScheduler(bool RunSynchronously);
   ~ClangdScheduler();
 
-  /// Enqueue WorkerRequest to be run on a worker thread
-  void enqueue(ClangdServer &Server, WorkerRequest Request);
+  /// Add \p Request to the start of the queue. \p Request will be run on a
+  /// separate worker thread.
+  void addToFront(std::function<void()> Request);
+  /// Add \p Request to the end of the queue. \p Request will be run on a
+  /// separate worker thread.
+  void addToEnd(std::function<void()> Request);
 
 private:
   bool RunSynchronously;
   std::mutex Mutex;
   /// We run some tasks on a separate thread(parsing, ClangdUnit cleanup).
   /// This thread looks into RequestQueue to find requests to handle and
   /// terminates when Done is set to true.
   std::thread Worker;
   /// Setting Done to true will make the worker thread terminate.
   bool Done = false;
-  /// A LIFO queue of requests. Note that requests are discarded if the
-  /// `version` field is not equal to the one stored inside DraftStore.
+  /// A queue of requests.
   /// FIXME(krasimir): code completion should always have priority over parsing
   /// for diagnostics.
-  std::deque<WorkerRequest> RequestQueue;
+  std::deque<std::function<void()>> RequestQueue;
   /// Condition variable to wake up the worker thread.
   std::condition_variable RequestCV;
 };
 
 /// Provides API to manage ASTs for a collection of C++ files and request
 /// various language features(currently, only codeCompletion and async
 /// diagnostics for tracked files).
 class ClangdServer {
public:
   std::vector<tooling::Replacement> formatOnType(PathRef File, Position Pos);
 
   /// Gets current document contents for \p File. \p File must point to a
   /// currently tracked file.
   /// FIXME(ibiryukov): This function is here to allow offset-to-Position
   /// conversions in outside code, maybe there's a way to get rid of it.
   std::string getDocument(PathRef File);
 
-private:
-  friend class ClangdScheduler;
-
-  /// This function is called on a worker thread.
-  void handleRequest(WorkerRequest Request);
+  /// Only for testing purposes.
+  /// Waits until all requests to worker thread are finished and dumps AST for
+  /// \p File. \p File must be in the list of added documents.
+  std::string dumpAST(PathRef File);
 
+private:
   std::unique_ptr<GlobalCompilationDatabase> CDB;
   std::unique_ptr<DiagnosticsConsumer> DiagConsumer;
   DraftStore DraftMgr;
   ClangdUnitStore Units;
   std::shared_ptr<PCHContainerOperations> PCHs;
   // WorkScheduler has to be the last member, because its destructor has to be
   // called before all other members to stop the worker thread that references
   // ClangdServer
   ClangdScheduler WorkScheduler;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clangd/ClangdServer.cpp

 #include "ClangdServer.h"
 #include "clang/Format/Format.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include <future>
 
 using namespace clang;
 using namespace clang::clangd;
 
 namespace {
 
 std::vector<tooling::Replacement> formatCode(StringRef Code, StringRef Filename,
                                              ArrayRef<tooling::Range> Ranges) {
Position clangd::offsetToPosition(StringRef Code, size_t Offset) {
   StringRef JustBefore = Code.substr(0, Offset);
   // FIXME: \r\n
   // FIXME: UTF-8
   int Lines = JustBefore.count('\n');
   int Cols = JustBefore.size() - JustBefore.rfind('\n') - 1;
   return {Lines, Cols};
 }
 
-WorkerRequest::WorkerRequest(WorkerRequestKind Kind, Path File,
-                             DocVersion Version)
-    : Kind(Kind), File(File), Version(Version) {}
-
-ClangdScheduler::ClangdScheduler(ClangdServer &Server, bool RunSynchronously)
+ClangdScheduler::ClangdScheduler(bool RunSynchronously)
     : RunSynchronously(RunSynchronously) {
   if (RunSynchronously) {
     // Don't start the worker thread if we're running synchronously
     return;
   }
 
   // Initialize Worker in ctor body, rather than init list to avoid potentially
   // using not-yet-initialized members
-  Worker = std::thread([&Server, this]() {
+  Worker = std::thread([this]() {
     while (true) {
-      WorkerRequest Request;
+      std::function<void()> Request;
 
       // Pick request from the queue
       {
         std::unique_lock<std::mutex> Lock(Mutex);
         // Wait for more requests.
         RequestCV.wait(Lock, [this] { return !RequestQueue.empty() || Done; });
         if (Done)
           return;
 
         assert(!RequestQueue.empty() && "RequestQueue was empty");
 
-        Request = std::move(RequestQueue.back());
-        RequestQueue.pop_back();
-
-        // Skip outdated requests
-        if (Request.Version != Server.DraftMgr.getVersion(Request.File)) {
-          // FIXME(ibiryukov): Logging
-          // Output.log("Version for " + Twine(Request.File) +
-          //            " in request is outdated, skipping request\n");
-          continue;
-        }
+        Request = std::move(RequestQueue.front());
+        RequestQueue.pop_front();
       } // unlock Mutex
 
-      Server.handleRequest(std::move(Request));
+      Request();
     }
   });
 }
 
 ClangdScheduler::~ClangdScheduler() {
   if (RunSynchronously)
     return; // no worker thread is running in that case
 
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     // Wake up the worker thread
     Done = true;
-    RequestCV.notify_one();
   } // unlock Mutex
+  RequestCV.notify_one();
   Worker.join();
 }
 
-void ClangdScheduler::enqueue(ClangdServer &Server, WorkerRequest Request) {
+void ClangdScheduler::addToFront(std::function<void()> Request) {
+  if (RunSynchronously) {
+    Request();
+    return;
+  }
+
+  {
+    std::lock_guard<std::mutex> Lock(Mutex);
+    RequestQueue.push_front(Request);
+  }
+  RequestCV.notify_one();
+}
+
+void ClangdScheduler::addToEnd(std::function<void()> Request) {
   if (RunSynchronously) {
-    Server.handleRequest(Request);
+    Request();
     return;
   }
 
+  {
     std::lock_guard<std::mutex> Lock(Mutex);
     RequestQueue.push_back(Request);
+  }
   RequestCV.notify_one();
 }
 
 ClangdServer::ClangdServer(std::unique_ptr<GlobalCompilationDatabase> CDB,
                            std::unique_ptr<DiagnosticsConsumer> DiagConsumer,
                            bool RunSynchronously)
     : CDB(std::move(CDB)), DiagConsumer(std::move(DiagConsumer)),
       PCHs(std::make_shared<PCHContainerOperations>()),
-      WorkScheduler(*this, RunSynchronously) {}
+      WorkScheduler(RunSynchronously) {}
 
 void ClangdServer::addDocument(PathRef File, StringRef Contents) {
-  DocVersion NewVersion = DraftMgr.updateDraft(File, Contents);
-  WorkScheduler.enqueue(
-      *this, WorkerRequest(WorkerRequestKind::ParseAndPublishDiagnostics, File,
-                           NewVersion));
+  DocVersion Version = DraftMgr.updateDraft(File, Contents);
+  Path FileStr = File;
+  WorkScheduler.addToFront([this, FileStr, Version]() {
+    auto FileContents = DraftMgr.getDraft(FileStr);
+    if (FileContents.Version != Version)
+      return; // This request is outdated, do nothing
+
+    assert(FileContents.Draft &&
+           "No contents inside a file that was scheduled for reparse");
+    Units.runOnUnit(
+        FileStr, *FileContents.Draft, *CDB, PCHs, [&](ClangdUnit const &Unit) {
+          DiagConsumer->onDiagnosticsReady(FileStr, Unit.getLocalDiagnostics());
+        });
+  });
 }
 
 void ClangdServer::removeDocument(PathRef File) {
-  auto NewVersion = DraftMgr.removeDraft(File);
-  WorkScheduler.enqueue(
-      *this, WorkerRequest(WorkerRequestKind::RemoveDocData, File, NewVersion));
+  auto Version = DraftMgr.removeDraft(File);
+  Path FileStr = File;
+  WorkScheduler.addToFront([this, FileStr, Version]() {
+    if (Version != DraftMgr.getVersion(FileStr))
+      return; // This request is outdated, do nothing
+
+    Units.removeUnitIfPresent(FileStr);
+  });
 }
 
 std::vector<CompletionItem> ClangdServer::codeComplete(PathRef File,
                                                        Position Pos) {
   auto FileContents = DraftMgr.getDraft(File);
   assert(FileContents.Draft && "codeComplete is called for non-added document");
 
   std::vector<CompletionItem> Result;
   Units.runOnUnitWithoutReparse(
       File, *FileContents.Draft, *CDB, PCHs, [&](ClangdUnit &Unit) {
         Result = Unit.codeComplete(*FileContents.Draft, Pos);
       });
   return Result;
 }
+
 std::vector<tooling::Replacement> ClangdServer::formatRange(PathRef File,
                                                             Range Rng) {
   std::string Code = getDocument(File);
 
   size_t Begin = positionToOffset(Code, Rng.start);
   size_t Len = positionToOffset(Code, Rng.end) - Begin;
   return formatCode(Code, File, {tooling::Range(Begin, Len)});
 }

 }
 
 std::string ClangdServer::getDocument(PathRef File) {
   auto draft = DraftMgr.getDraft(File);
   assert(draft.Draft && "File is not tracked, cannot get contents");
   return *draft.Draft;
 }
 
-void ClangdServer::handleRequest(WorkerRequest Request) {
-  switch (Request.Kind) {
-  case WorkerRequestKind::ParseAndPublishDiagnostics: {
-    auto FileContents = DraftMgr.getDraft(Request.File);
-    if (FileContents.Version != Request.Version)
-      return; // This request is outdated, do nothing
+std::string ClangdServer::dumpAST(PathRef File) {
+  std::promise<std::string> DumpPromise;
+  auto DumpFuture = DumpPromise.get_future();
+  auto Version = DraftMgr.getVersion(File);
 
-    assert(FileContents.Draft &&
-           "No contents inside a file that was scheduled for reparse");
-    Units.runOnUnit(Request.File, *FileContents.Draft, *CDB, PCHs,
-                    [&](ClangdUnit const &Unit) {
-                      DiagConsumer->onDiagnosticsReady(
-                          Request.File, Unit.getLocalDiagnostics());
-                    });
-    break;
-  }
-  case WorkerRequestKind::RemoveDocData:
-    if (Request.Version != DraftMgr.getVersion(Request.File))
-      return; // This request is outdated, do nothing
+  WorkScheduler.addToEnd([this, &DumpPromise, File, Version]() {
+    assert(DraftMgr.getVersion(File) == Version && "Version has changed");
 
-    Units.removeUnitIfPresent(Request.File);
-    break;
+    Units.runOnExistingUnit(File, [&DumpPromise](ClangdUnit &Unit) {
+      std::string Result;
+      {
+        // We rely on ResultOS's destructor to flush the stream before we pass
+        // Result to set_value of DumpPromise.
+        llvm::raw_string_ostream ResultOS(Result);
+        Unit.dumpAST(ResultOS);
       }
+      DumpPromise.set_value(std::move(Result));
+    });
+  });
+  return DumpFuture.get();
 }

clangd/ClangdUnit.h

 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDUNIT_H
 
 #include "Protocol.h"
 #include "Path.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include <memory>
 
+namespace llvm {
+class raw_ostream;
+}
+
 namespace clang {
 class ASTUnit;
 class PCHContainerOperations;
 
 namespace tooling {
 struct CompileCommand;
 }
 
public:
   ///
   /// This function is thread-safe and returns completion items that own the
   /// data they contain.
   std::vector<CompletionItem> codeComplete(StringRef Contents, Position Pos);
   /// Returns diagnostics and corresponding FixIts for each diagnostic that are
   /// located in the current file.
   std::vector<DiagWithFixIts> getLocalDiagnostics() const;
 
+  /// For testing/debugging purposes. Note that this method deserializes all
+  /// unserialized Decls, so use with care.
+  void dumpAST(llvm::raw_ostream &OS) const;
+
 private:
   Path FileName;
   std::unique_ptr<ASTUnit> Unit;
   std::shared_ptr<PCHContainerOperations> PCHs;
 };
 
 } // namespace clangd
 } // namespace clang
 #endif

clangd/ClangdUnit.cpp

for (ASTUnit::stored_diag_iterator D = Unit->stored_diag_begin(),
     for (const FixItHint &Fix : D->getFixIts()) {
       FixItsForDiagnostic.push_back(clang::tooling::Replacement(
           Unit->getSourceManager(), Fix.RemoveRange, Fix.CodeToInsert));
     }
     Result.push_back({Diag, std::move(FixItsForDiagnostic)});
   }
   return Result;
 }
+
+void ClangdUnit::dumpAST(llvm::raw_ostream &OS) const {
+  Unit->getASTContext().getTranslationUnitDecl()->dump(OS, true);
+}

clangd/ClangdUnitStore.h

public:
   void runOnUnitWithoutReparse(PathRef File, StringRef FileContents,
                                GlobalCompilationDatabase &CDB,
                                std::shared_ptr<PCHContainerOperations> PCHs,
                                Func Action) {
     runOnUnitImpl(File, FileContents, CDB, PCHs, /*ReparseBeforeAction=*/false,
                   std::forward<Func>(Action));
   }
 
+  /// Run the specified \p Action on the ClangdUnit for \p File.
+  /// Unit for \p File should exist in the store.
+  template <class Func>
+  void runOnExistingUnit(PathRef File, Func Action) {
+    std::lock_guard<std::mutex> Lock(Mutex);
+
+    auto It = OpenedFiles.find(File);
+    assert(It != OpenedFiles.end() && "File is not in OpenedFiles");
+
+    Action(It->second);
+  }
+
   /// Remove ClangdUnit for \p File, if any
   void removeUnitIfPresent(PathRef File);
 
 private:
   /// Run specified \p Action on the ClangdUnit for \p File.
   template <class Func>
   void runOnUnitImpl(PathRef File, StringRef FileContents,
                      GlobalCompilationDatabase &CDB,