[PATCH] D112661: [clangd] reuse preambles from other files when possible

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

@nridge after rebasing this change for several months, I find it less and less 
useful. It seems both clangd and the IDE I'm using are making significant 
improvements that make this changes less and less impactful.
I closed this revision as I feel that it now passed the threshold where the 
annoyance of maintaining it is greater than the return I'm getting from it


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112661/new/

https://reviews.llvm.org/D112661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112661: [clangd] reuse preambles from other files when possible

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

@qchateau How should I interpret the abandoning of this patch -- did you run 
into a problem with the approach, or just don't have the time / interest to 
pursue it further?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112661/new/

https://reviews.llvm.org/D112661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112661: [clangd] reuse preambles from other files when possible

[Quentin Chateau via Phabricator via cfe-commits]

qchateau updated this revision to Diff 429478.
qchateau added a comment.

Small correction


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112661/new/

https://reviews.llvm.org/D112661

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -334,6 +334,13 @@
 init(getDefaultAsyncThreadsCount()),
 };
 
+opt ClosedPreambleCacheSize{
+"closed-preamble-cache-size",
+cat(Misc),
+desc("Number of preambles of closed files to keep in the cache."),
+init(1),
+};
+
 opt IndexFile{
 "index-file",
 cat(Misc),
@@ -879,6 +886,7 @@
 Opts.StaticIndex = PAI.get();
   }
   Opts.AsyncThreadsCount = WorkerThreadsCount;
+  Opts.ClosedPreambleCacheSize = ClosedPreambleCacheSize;
   Opts.FoldingRanges = FoldingRanges;
   Opts.MemoryCleanup = getMemoryCleanupFunction();
 
Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -190,6 +190,9 @@
 /// If 0, executes actions synchronously on the calling thread.
 unsigned AsyncThreadsCount = getDefaultAsyncThreadsCount();
 
+// Number of preambles of closed files to keep in the cache.
+unsigned ClosedPreambleCacheSize = 1;
+
 /// Cache (large) preamble data in RAM rather than temporary files on disk.
 bool StorePreamblesInMemory = false;
 
@@ -313,6 +316,8 @@
   class ASTCache;
   /// Tracks headers included by open files, to get known-good compile commands.
   class HeaderIncluderCache;
+  /// Store all known preambles
+  class PreambleStore;
 
   // The file being built/processed in the current thread. This is a hack in
   // order to get the file name into the index implementations. Do not depend on
@@ -336,6 +341,7 @@
   llvm::StringMap> Files;
   std::unique_ptr IdleASTs;
   std::unique_ptr HeaderIncluders;
+  std::unique_ptr Preambles;
   // None when running tasks synchronously and non-None when running tasks
   // asynchronously.
   llvm::Optional PreambleTasks;
Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -342,6 +342,65 @@
   }
 };
 
+class TUScheduler::PreambleStore {
+public:
+  struct Entry {
+std::shared_ptr Preamble;
+size_t Score;
+Path FileName;
+
+bool operator>(const Entry ) const { return Score > Other.Score; }
+  };
+
+  explicit PreambleStore(size_t ClosedPreamblesToKeep)
+  : ClosedPreamblesToKeep(ClosedPreamblesToKeep) {}
+
+  auto getAll() {
+std::unique_lock Lock(Mut);
+return Store;
+  }
+
+  void push(PathRef FileName, std::shared_ptr Preamble) {
+std::unique_lock Lock(Mut);
+auto It = llvm::find_if(
+Store, [&](const auto ) { return Item.FileName == FileName; });
+if (It == Store.end()) {
+  Store.push_back(Entry{std::move(Preamble), 0, FileName.str()});
+  popWorstPreambles();
+} else {
+  It->Preamble = std::move(Preamble);
+}
+vlog("Store contains {0} preambles", Store.size());
+  }
+
+  void hit(PathRef FileName) {
+std::unique_lock Lock(Mut);
+auto It = llvm::find_if(
+Store, [&](const auto ) { return Item.FileName == FileName; });
+if (It == Store.end()) {
+  return;
+}
+It->Score++;
+  }
+
+private:
+  void popWorstPreambles() {
+auto Begin = llvm::partition(
+Store, [](const auto ) { return Item.Preamble.use_count() > 1; });
+if (static_cast(std::distance(Begin, Store.end())) <=
+ClosedPreamblesToKeep) {
+  return;
+}
+auto Nth = Begin + ClosedPreamblesToKeep;
+std::nth_element(Begin, Nth, Store.end(), std::greater());
+Store.erase(Nth, Store.end());
+  }
+
+  std::mutex Mut;
+  std::vector Store;
+  size_t ClosedPreamblesToKeep;
+};
+
 namespace {
 
 bool isReliable(const tooling::CompileCommand ) {
@@ -541,7 +600,8 @@
   ASTWorker(PathRef FileName, const GlobalCompilationDatabase ,
 TUScheduler::ASTCache ,
 TUScheduler::HeaderIncluderCache ,
-Semaphore , bool RunSync, const TUScheduler::Options ,
+TUScheduler::PreambleStore , Semaphore ,
+bool RunSync, const TUScheduler::Options ,
 ParsingCallbacks );
 
 public:
@@ -554,8 +614,9 @@
   create(PathRef FileName, const GlobalCompilationDatabase ,
  TUScheduler::ASTCache ,
  TUScheduler::HeaderIncluderCache ,
- AsyncTaskRunner 

[PATCH] D112661: [clangd] reuse preambles from other files when possible

[Quentin Chateau via Phabricator via cfe-commits]

qchateau updated this revision to Diff 429470.
qchateau added a comment.
Herald added a project: All.

Resolve conflicts, fix formatting, simplify patch


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112661/new/

https://reviews.llvm.org/D112661

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -334,6 +334,13 @@
 init(getDefaultAsyncThreadsCount()),
 };
 
+opt ClosedPreambleCacheSize{
+"closed-preamble-cache-size",
+cat(Misc),
+desc("Number of preambles of closed files to keep in the cache."),
+init(1),
+};
+
 opt IndexFile{
 "index-file",
 cat(Misc),
@@ -879,6 +886,7 @@
 Opts.StaticIndex = PAI.get();
   }
   Opts.AsyncThreadsCount = WorkerThreadsCount;
+  Opts.ClosedPreambleCacheSize = ClosedPreambleCacheSize;
   Opts.FoldingRanges = FoldingRanges;
   Opts.MemoryCleanup = getMemoryCleanupFunction();
 
Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -190,6 +190,9 @@
 /// If 0, executes actions synchronously on the calling thread.
 unsigned AsyncThreadsCount = getDefaultAsyncThreadsCount();
 
+// Number of preambles of closed files to keep in the cache.
+unsigned ClosedPreambleCacheSize = 1;
+
 /// Cache (large) preamble data in RAM rather than temporary files on disk.
 bool StorePreamblesInMemory = false;
 
@@ -313,6 +316,8 @@
   class ASTCache;
   /// Tracks headers included by open files, to get known-good compile commands.
   class HeaderIncluderCache;
+  /// Store all known preambles
+  class PreambleStore;
 
   // The file being built/processed in the current thread. This is a hack in
   // order to get the file name into the index implementations. Do not depend on
@@ -336,6 +341,7 @@
   llvm::StringMap> Files;
   std::unique_ptr IdleASTs;
   std::unique_ptr HeaderIncluders;
+  std::unique_ptr Preambles;
   // None when running tasks synchronously and non-None when running tasks
   // asynchronously.
   llvm::Optional PreambleTasks;
Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -342,6 +342,65 @@
   }
 };
 
+class TUScheduler::PreambleStore {
+public:
+  struct Entry {
+std::shared_ptr Preamble;
+size_t Score;
+Path FileName;
+
+bool operator>(const Entry ) const { return Score > Other.Score; }
+  };
+
+  explicit PreambleStore(size_t ClosedPreamblesToKeep)
+  : ClosedPreamblesToKeep(ClosedPreamblesToKeep) {}
+
+  auto getAll() {
+std::unique_lock Lock(Mut);
+return Store;
+  }
+
+  void push(PathRef FileName, std::shared_ptr Preamble) {
+std::unique_lock Lock(Mut);
+auto It = llvm::find_if(
+Store, [&](const auto ) { return Item.FileName == FileName; });
+if (It == Store.end()) {
+  Store.push_back(Entry{std::move(Preamble), 0, FileName.str()});
+  popWorstPreambles();
+} else {
+  It->Preamble = std::move(Preamble);
+}
+vlog("Store contains {0} preambles", Store.size());
+  }
+
+  void hit(PathRef FileName) {
+std::unique_lock Lock(Mut);
+auto It = llvm::find_if(
+Store, [&](const auto ) { return Item.FileName == FileName; });
+if (It == Store.end()) {
+  return;
+}
+It->Score++;
+  }
+
+private:
+  void popWorstPreambles() {
+auto Begin = llvm::partition(
+Store, [](const auto ) { return Item.Preamble.use_count() > 1; });
+if (static_cast(std::distance(Begin, Store.end())) <=
+ClosedPreamblesToKeep) {
+  return;
+}
+auto Nth = Begin + ClosedPreamblesToKeep;
+std::nth_element(Begin, Nth, Store.end(), std::greater());
+Store.erase(Nth, Store.end());
+  }
+
+  std::mutex Mut;
+  std::vector Store;
+  size_t ClosedPreamblesToKeep;
+};
+
 namespace {
 
 bool isReliable(const tooling::CompileCommand ) {
@@ -541,7 +600,8 @@
   ASTWorker(PathRef FileName, const GlobalCompilationDatabase ,
 TUScheduler::ASTCache ,
 TUScheduler::HeaderIncluderCache ,
-Semaphore , bool RunSync, const TUScheduler::Options ,
+TUScheduler::PreambleStore , Semaphore ,
+bool RunSync, const TUScheduler::Options ,
 ParsingCallbacks );
 
 public:
@@ -554,8 +614,9 @@
   create(PathRef FileName, const GlobalCompilationDatabase ,
  TUScheduler::ASTCache ,
  

[PATCH] D112661: [clangd] reuse preambles from other files when possible

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

This is a revisit of D97417  which is a bit 
out of date and fell into oblivion.
I've been using a forked version of clangd including this change for months on 
my day-to-day work and it does significantly improve my experience.
Some numbers showing the kind of improvement you can expect from this change 
can be found in D97417 , they are still 
relevant.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112661/new/

https://reviews.llvm.org/D112661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112661: [clangd] reuse preambles from other files when possible

[Quentin Chateau via Phabricator via cfe-commits]

qchateau created this revision.
qchateau added a reviewer: sammccall.
Herald added subscribers: usaxena95, kadircet, arphaman, javed.absar.
qchateau requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Keep a store of the preambles of all opened filed, plus 
a configurable number of previously used preambles (default 1). 
When building the AST for a TU, if the preamble for this 
TU is not yet built, look through the stored premables 
to find the best compatible preamble and use this 
preamble to speed-up the AST build.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112661

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -329,6 +329,13 @@
 init(getDefaultAsyncThreadsCount()),
 };
 
+opt ClosedPreambleCacheSize{
+"closed-preamble-cache-size",
+cat(Misc),
+desc("Number of preambles of closed files to keep in the cache."),
+init(1),
+};
+
 opt IndexFile{
 "index-file",
 cat(Misc),
@@ -868,6 +875,7 @@
 Opts.StaticIndex = PAI.get();
   }
   Opts.AsyncThreadsCount = WorkerThreadsCount;
+  Opts.ClosedPreambleCacheSize = ClosedPreambleCacheSize;
   Opts.FoldingRanges = FoldingRanges;
   Opts.InlayHints = InlayHints;
   Opts.MemoryCleanup = getMemoryCleanupFunction();
Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -192,6 +192,9 @@
 /// If 0, executes actions synchronously on the calling thread.
 unsigned AsyncThreadsCount = getDefaultAsyncThreadsCount();
 
+// Number of preambles of closed files to keep in the cache.
+unsigned ClosedPreambleCacheSize = 1;
+
 /// Cache (large) preamble data in RAM rather than temporary files on disk.
 bool StorePreamblesInMemory = false;
 
@@ -315,6 +318,8 @@
   class ASTCache;
   /// Tracks headers included by open files, to get known-good compile commands.
   class HeaderIncluderCache;
+  /// Store all known preambles
+  class PreambleStore;
 
   // The file being built/processed in the current thread. This is a hack in
   // order to get the file name into the index implementations. Do not depend on
@@ -338,6 +343,7 @@
   llvm::StringMap> Files;
   std::unique_ptr IdleASTs;
   std::unique_ptr HeaderIncluders;
+  std::unique_ptr Preambles;
   // None when running tasks synchronously and non-None when running tasks
   // asynchronously.
   llvm::Optional PreambleTasks;
Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -307,6 +307,65 @@
   }
 };
 
+class TUScheduler::PreambleStore {
+public:
+  struct Entry {
+std::shared_ptr Preamble;
+size_t Score;
+Path FileName;
+
+bool operator>(const Entry ) const { return Score > Other.Score; }
+  };
+
+  explicit PreambleStore(size_t ClosedPreamblesToKeep): ClosedPreamblesToKeep(ClosedPreamblesToKeep) {}
+
+  auto getAll() {
+std::unique_lock Lock(Mut);
+return Store;
+  }
+
+  void push(PathRef FileName, std::shared_ptr Preamble) {
+std::unique_lock Lock(Mut);
+auto It = llvm::find_if(
+Store, [&](const auto ) { return Item.FileName == FileName; });
+if (It == Store.end()) {
+  Store.push_back(Entry{std::move(Preamble), 0, FileName.str()});
+  popWorstPreambles();
+}
+else {
+  It->Preamble = std::move(Preamble);
+}
+vlog("Store contains {0} preambles", Store.size());
+  }
+
+  void hit(PathRef FileName) {
+std::unique_lock Lock(Mut);
+auto It = llvm::find_if(
+Store, [&](const auto ) { return Item.FileName == FileName; });
+if (It == Store.end()) {
+  return;
+}
+It->Score++;
+  }
+
+private:
+  void popWorstPreambles() {
+auto Begin = llvm::partition(
+Store, [](const auto ) { return Item.Preamble.use_count() > 1; });
+if (static_cast(std::distance(Begin, Store.end())) <=
+ClosedPreamblesToKeep) {
+  return;
+}
+auto Nth = Begin + ClosedPreamblesToKeep;
+std::nth_element(Begin, Nth, Store.end(), std::greater());
+Store.erase(Nth, Store.end());
+  }
+
+  std::mutex Mut;
+  std::vector Store;
+  size_t ClosedPreamblesToKeep;
+};
+
 namespace {
 
 bool isReliable(const tooling::CompileCommand ) {
@@ -506,7 +565,8 @@
   ASTWorker(PathRef FileName, const GlobalCompilationDatabase ,