[PATCH] D93829: [clangd] Support outgoing calls in call hierarchy

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

I'm interested but I don't have enough free time these days. So if anyone wants 
to take it from there, feel free to reuse my commits (or not).
If I find myself with some free time and nobody else is working on this, I'll 
give it a shot


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93829

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

[PATCH] D134384: [clangd] Add support for HeaderInsertion in .clangd config file

[Quentin Chateau via Phabricator via cfe-commits]

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

- clangd: rename HeaderInsertion and IncludeInsertion, read value from Config


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134384

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeComplete.h
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp

Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -218,6 +218,20 @@
   EXPECT_THAT(Results[0].Completion.AllScopes, testing::Eq(llvm::None));
 }
 
+TEST(ParseYAML, InsertIncludes) {
+  CapturedDiags Diags;
+  Annotations YAML(R"yaml(
+Completion:
+  InsertIncludes: Never
+  )yaml");
+  auto Results =
+  Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_EQ(Results.size(), 1u);
+  EXPECT_THAT(Results[0].Completion.InsertIncludes,
+  llvm::ValueIs(val("Never")));
+}
+
 TEST(ParseYAML, ShowAKA) {
   CapturedDiags Diags;
   Annotations YAML(R"yaml(
Index: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "CodeComplete.h"
 #include "Config.h"
 #include "ConfigFragment.h"
 #include "ConfigTesting.h"
@@ -537,6 +538,23 @@
   EXPECT_TRUE(Conf.Completion.AllScopes);
 }
 
+TEST_F(ConfigCompileTests, InsertIncludes) {
+  // Defaults to IWYU.
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Completion.InsertIncludes, Config::InsertIncludesPolicy::IWYU);
+
+  Frag = {};
+  Frag.Completion.InsertIncludes = std::string("IWYU");
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Completion.InsertIncludes, Config::InsertIncludesPolicy::IWYU);
+
+  Frag = {};
+  Frag.Completion.InsertIncludes = std::string("Never");
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Completion.InsertIncludes,
+Config::InsertIncludesPolicy::NeverInsert);
+}
+
 TEST_F(ConfigCompileTests, Style) {
   Frag = {};
   Frag.Style.FullyQualifiedNamespaces.push_back(std::string("foo"));
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -11,6 +11,7 @@
 #include "ClangdServer.h"
 #include "CodeComplete.h"
 #include "Compiler.h"
+#include "Config.h"
 #include "Matchers.h"
 #include "Protocol.h"
 #include "Quality.h"
@@ -742,7 +743,7 @@
   EXPECT_TRUE(Results.HasMore);
 }
 
-TEST(CompletionTest, IncludeInsertionPreprocessorIntegrationTests) {
+TEST(CompletionTest, InsertIncludesPolicyPreprocessorIntegrationTests) {
   TestTU TU;
   TU.ExtraArgs.push_back("-I" + testPath("sub"));
   TU.AdditionalFiles["sub/bar.h"] = "";
@@ -757,12 +758,15 @@
   auto Results = completions(TU, Test.point(), {Sym});
   EXPECT_THAT(Results.Completions,
   ElementsAre(AllOf(named("X"), insertInclude("\"bar.h\"";
-  // Can be disabled via option.
-  CodeCompleteOptions NoInsertion;
-  NoInsertion.InsertIncludes = CodeCompleteOptions::NeverInsert;
-  Results = completions(TU, Test.point(), {Sym}, NoInsertion);
-  EXPECT_THAT(Results.Completions,
-  ElementsAre(AllOf(named("X"), Not(insertInclude();
+  // Can be disabled via config.
+  {
+Config Cfg;
+Cfg.Completion.InsertIncludes = Config::InsertIncludesPolicy::NeverInsert;
+WithContextValue WithCfg(Config::Key, std::move(Cfg));
+Results = completions(TU, Test.point(), {Sym});
+EXPECT_THAT(Results.Completions,
+ElementsAre(AllOf(named("X"), Not(insertInclude();
+  }
   // Duplicate based on inclusions in preamble.
   Test = Annotations(R"cpp(
   #include "sub/bar.h"  // not shortest, so should only match resolved.
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -249,19 +249,19 @@
 init(CodeCompleteOptions().EnableFunctionArgSnippets),
 };
 
-opt HeaderInsertion{
+opt HeaderInsertion{
 

[PATCH] D134384: [clangd] Add support for HeaderInsertion in .clangd config file

[Quentin Chateau via Phabricator via cfe-commits]

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

Add ability to configure header insertion when accepting code
completion from the .clangd config file.
The command line option always take precedence.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134384

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp

Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -218,6 +218,20 @@
   EXPECT_THAT(Results[0].Completion.AllScopes, testing::Eq(llvm::None));
 }
 
+TEST(ParseYAML, HeaderInsertion) {
+  CapturedDiags Diags;
+  Annotations YAML(R"yaml(
+Completion:
+  HeaderInsertion: Never
+  )yaml");
+  auto Results =
+  Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+  ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+  ASSERT_EQ(Results.size(), 1u);
+  EXPECT_THAT(Results[0].Completion.HeaderInsertion,
+  llvm::ValueIs(val("Never")));
+}
+
 TEST(ParseYAML, ShowAKA) {
   CapturedDiags Diags;
   Annotations YAML(R"yaml(
Index: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "CodeComplete.h"
 #include "Config.h"
 #include "ConfigFragment.h"
 #include "ConfigTesting.h"
@@ -537,6 +538,25 @@
   EXPECT_TRUE(Conf.Completion.AllScopes);
 }
 
+TEST_F(ConfigCompileTests, HeaderInsertion) {
+  // Defaults to IWYU.
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Completion.InsertIncludes,
+CodeCompleteOptions::IncludeInsertion::IWYU);
+
+  Frag = {};
+  Frag.Completion.HeaderInsertion = std::string("IWYU");
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Completion.InsertIncludes,
+CodeCompleteOptions::IncludeInsertion::IWYU);
+
+  Frag = {};
+  Frag.Completion.HeaderInsertion = std::string("Never");
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Completion.InsertIncludes,
+CodeCompleteOptions::IncludeInsertion::NeverInsert);
+}
+
 TEST_F(ConfigCompileTests, Style) {
   Frag = {};
   Frag.Style.FullyQualifiedNamespaces.push_back(std::string("foo"));
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -697,6 +697,8 @@
 C.Index.Background = *BGPolicy;
   if (AllScopesCompletion.getNumOccurrences())
 C.Completion.AllScopes = AllScopesCompletion;
+  if (HeaderInsertion.getNumOccurrences())
+C.Completion.InsertIncludes = HeaderInsertion;
   return true;
 };
   }
@@ -904,7 +906,6 @@
   if (CompletionStyle.getNumOccurrences())
 Opts.CodeComplete.BundleOverloads = CompletionStyle != Detailed;
   Opts.CodeComplete.ShowOrigins = ShowOrigins;
-  Opts.CodeComplete.InsertIncludes = HeaderInsertion;
   if (!HeaderInsertionDecorators) {
 Opts.CodeComplete.IncludeIndicator.Insert.clear();
 Opts.CodeComplete.IncludeIndicator.NoInsert.clear();
Index: clang-tools-extra/clangd/ConfigYAML.cpp
===
--- clang-tools-extra/clangd/ConfigYAML.cpp
+++ clang-tools-extra/clangd/ConfigYAML.cpp
@@ -218,6 +218,10 @@
   if (auto AllScopes = boolValue(N, "AllScopes"))
 F.AllScopes = *AllScopes;
 });
+Dict.handle("HeaderInsertion", [&](Node ) {
+  if (auto HeaderInsertion = scalarValue(N, "HeaderInsertion"))
+F.HeaderInsertion = *HeaderInsertion;
+});
 Dict.parse(N);
   }
 
Index: clang-tools-extra/clangd/ConfigFragment.h
===
--- clang-tools-extra/clangd/ConfigFragment.h
+++ clang-tools-extra/clangd/ConfigFragment.h
@@ -284,6 +284,12 @@
 /// Whether code completion should include suggestions from scopes that are
 /// not visible. The required scope prefix will be inserted.
 llvm::Optional> AllScopes;
+/// Add #include directives when accepting code completions.
+///
+/// Valid values are:
+/// - Never
+/// - IWYU
+

[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

[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 ,
 

[PATCH] D97417: [clangd] use a compatible preamble for the first AST built

[Quentin Chateau via Phabricator via cfe-commits]

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

- Rebase
- Add cli argument to set the cache size
- Reduce default cache size to 1 (to allow fast open/close/reopen)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97417

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
@@ -327,6 +327,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),
@@ -846,6 +853,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
@@ -96,6 +96,62 @@
 
 namespace {
 class ASTWorker;
+
+bool compileCommandsAreSimilar(const tooling::CompileCommand ,
+   const tooling::CompileCommand ) {
+  auto LIt = LHS.CommandLine.begin();
+  auto RIt = RHS.CommandLine.begin();
+
+  auto SkipSpecificArg = [](auto It, auto End, const std::string ) {
+while (It != End) {
+  if (*It == "-o" || *It == "-x") {
+// Erase -o and its argument
+// Erase -x and its argument: it would prevent using header file
+// preamble for a source file
+It = std::min(It + 2, End);
+  } else if (*It == "-c" || It->find("-W") == 0 || *It == "-pedantic") {
+// We don't care about those flags
+It++;
+  } else if (It->find(Filename) != std::string::npos) {
+// The file we're compiling appears in this argument, drop it to make it
+// generic
+It++;
+  } else {
+break;
+  }
+}
+return It;
+  };
+
+  // Iterate the command line, skipping arguments that are specific to the
+  // file being compiled and that would not influence the premable
+  // compatiblity. Stop when one iterate reach the or when an argument differs
+  auto LEnd = LHS.CommandLine.end();
+  auto REnd = RHS.CommandLine.end();
+  while (true) {
+LIt = SkipSpecificArg(LIt, LEnd, LHS.Filename);
+RIt = SkipSpecificArg(RIt, REnd, RHS.Filename);
+
+if (LIt == LEnd || RIt == REnd) {
+  break;
+}
+
+if (*LIt != *RIt) {
+  break;
+}
+
+LIt++;
+RIt++;
+  }
+
+  // If both iterator get to the end at the same time, the CL are compatible
+  bool Compatible = LIt == LEnd && RIt == REnd;
+  if (Compatible) {
+vlog("{0} and {1} are compatible", LHS.Filename, RHS.Filename);
+  }
+  return Compatible;
+}
+
 } // namespace
 
 static clang::clangd::Key kFileBeingProcessed;
@@ -301,6 +357,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 

[PATCH] D97417: [clangd] use a compatible preamble for the first AST built

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.
Herald added a project: clang-tools-extra.

Have you guys been giving some thoughts to that patch ? I've been using it in 
my daily work since I submitted the patch, and I'd not go back


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97417

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

[PATCH] D97983: [clangd] strictly respect formatting range

[Quentin Chateau via Phabricator via cfe-commits]

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

fix tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97983

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/test/formatting.test


Index: clang-tools-extra/clangd/test/formatting.test
===
--- clang-tools-extra/clangd/test/formatting.test
+++ clang-tools-extra/clangd/test/formatting.test
@@ -8,19 +8,6 @@
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": [
 # CHECK-NEXT:{
-# CHECK-NEXT:  "newText": "\n  ",
-# CHECK-NEXT:  "range": {
-# CHECK-NEXT:"end": {
-# CHECK-NEXT:  "character": 4,
-# CHECK-NEXT:  "line": 1
-# CHECK-NEXT:},
-# CHECK-NEXT:"start": {
-# CHECK-NEXT:  "character": 19,
-# CHECK-NEXT:  "line": 0
-# CHECK-NEXT:}
-# CHECK-NEXT:  }
-# CHECK-NEXT:},
-# CHECK-NEXT:{
 # CHECK-NEXT:  "newText": " ",
 # CHECK-NEXT:  "range": {
 # CHECK-NEXT:"end": {
@@ -45,19 +32,6 @@
 # CHECK-NEXT:  "line": 1
 # CHECK-NEXT:}
 # CHECK-NEXT:  }
-# CHECK-NEXT:},
-# CHECK-NEXT:{
-# CHECK-NEXT:  "newText": "\n  ",
-# CHECK-NEXT:  "range": {
-# CHECK-NEXT:"end": {
-# CHECK-NEXT:  "character": 4,
-# CHECK-NEXT:  "line": 2
-# CHECK-NEXT:},
-# CHECK-NEXT:"start": {
-# CHECK-NEXT:  "character": 12,
-# CHECK-NEXT:  "line": 1
-# CHECK-NEXT:}
-# CHECK-NEXT:  }
 # CHECK-NEXT:}
 # CHECK-NEXT:  ]
 ---
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -423,10 +423,24 @@
 if (!Changed)
   return CB(Changed.takeError());
 
-CB(IncludeReplaces.merge(format::reformat(
+auto Replacements = IncludeReplaces.merge(format::reformat(
 Style, *Changed,
 tooling::calculateRangesAfterReplacements(IncludeReplaces, Ranges),
-File)));
+File));
+clang::tooling::Replacements ReplacementsInRange;
+for (const auto  : Replacements) {
+  tooling::Range ReplRange{
+  Repl.getOffset(),
+  std::max(Repl.getLength(),
+ Repl.getReplacementText().size()),
+  };
+  if (ReplRange.overlapsWith(Ranges.front())) {
+if (auto Err = ReplacementsInRange.add(Repl)) {
+  CB(std::move(Err));
+}
+  }
+}
+CB(ReplacementsInRange);
   };
   WorkScheduler->runQuick("Format", File, std::move(Action));
 }


Index: clang-tools-extra/clangd/test/formatting.test
===
--- clang-tools-extra/clangd/test/formatting.test
+++ clang-tools-extra/clangd/test/formatting.test
@@ -8,19 +8,6 @@
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": [
 # CHECK-NEXT:{
-# CHECK-NEXT:  "newText": "\n  ",
-# CHECK-NEXT:  "range": {
-# CHECK-NEXT:"end": {
-# CHECK-NEXT:  "character": 4,
-# CHECK-NEXT:  "line": 1
-# CHECK-NEXT:},
-# CHECK-NEXT:"start": {
-# CHECK-NEXT:  "character": 19,
-# CHECK-NEXT:  "line": 0
-# CHECK-NEXT:}
-# CHECK-NEXT:  }
-# CHECK-NEXT:},
-# CHECK-NEXT:{
 # CHECK-NEXT:  "newText": " ",
 # CHECK-NEXT:  "range": {
 # CHECK-NEXT:"end": {
@@ -45,19 +32,6 @@
 # CHECK-NEXT:  "line": 1
 # CHECK-NEXT:}
 # CHECK-NEXT:  }
-# CHECK-NEXT:},
-# CHECK-NEXT:{
-# CHECK-NEXT:  "newText": "\n  ",
-# CHECK-NEXT:  "range": {
-# CHECK-NEXT:"end": {
-# CHECK-NEXT:  "character": 4,
-# CHECK-NEXT:  "line": 2
-# CHECK-NEXT:},
-# CHECK-NEXT:"start": {
-# CHECK-NEXT:  "character": 12,
-# CHECK-NEXT:  "line": 1
-# CHECK-NEXT:}
-# CHECK-NEXT:  }
 # CHECK-NEXT:}
 # CHECK-NEXT:  ]
 ---
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -423,10 +423,24 @@
 if (!Changed)
   return CB(Changed.takeError());
 
-CB(IncludeReplaces.merge(format::reformat(
+auto Replacements = IncludeReplaces.merge(format::reformat(
 Style, *Changed,
 tooling::calculateRangesAfterReplacements(IncludeReplaces, Ranges),
-File)));
+File));
+clang::tooling::Replacements ReplacementsInRange;
+for (const auto  : Replacements) {
+  tooling::Range ReplRange{
+  Repl.getOffset(),
+  std::max(Repl.getLength(),
+ Repl.getReplacementText().size()),
+  };
+  if (ReplRange.overlapsWith(Ranges.front())) {
+if 

[PATCH] D97983: [clangd] strictly respect formatting range

[Quentin Chateau via Phabricator via cfe-commits]

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

Clang format will often format a range of code wider then the requested format 
range. Returning a replacement outside of the requested range can mess with 
clients. Keeping the formatting in range of what the client asks gives more 
expected results and will limit the impact of partial formatting in diffs.

A specific example: a problem often occurs when using VSCode in "format 
modified lines" mode. The client will ask clangd to format two non-continugous 
lines, separately. Clangd replies to both requests with replacements for both 
lines, effectively providing four replacements in total. VSCode will either 
apply replacements twice (messing up the results) or simply error out and skip 
the formatting.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D97983

Files:
  clang-tools-extra/clangd/ClangdServer.cpp


Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -423,10 +423,24 @@
 if (!Changed)
   return CB(Changed.takeError());
 
-CB(IncludeReplaces.merge(format::reformat(
+auto Replacements = IncludeReplaces.merge(format::reformat(
 Style, *Changed,
 tooling::calculateRangesAfterReplacements(IncludeReplaces, Ranges),
-File)));
+File));
+clang::tooling::Replacements ReplacementsInRange;
+for (const auto  : Replacements) {
+  tooling::Range ReplRange{
+  Repl.getOffset(),
+  std::max(Repl.getLength(),
+ Repl.getReplacementText().size()),
+  };
+  if (ReplRange.overlapsWith(Ranges.front())) {
+if (auto Err = ReplacementsInRange.add(Repl)) {
+  CB(std::move(Err));
+}
+  }
+}
+CB(ReplacementsInRange);
   };
   WorkScheduler->runQuick("Format", File, std::move(Action));
 }


Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -423,10 +423,24 @@
 if (!Changed)
   return CB(Changed.takeError());
 
-CB(IncludeReplaces.merge(format::reformat(
+auto Replacements = IncludeReplaces.merge(format::reformat(
 Style, *Changed,
 tooling::calculateRangesAfterReplacements(IncludeReplaces, Ranges),
-File)));
+File));
+clang::tooling::Replacements ReplacementsInRange;
+for (const auto  : Replacements) {
+  tooling::Range ReplRange{
+  Repl.getOffset(),
+  std::max(Repl.getLength(),
+ Repl.getReplacementText().size()),
+  };
+  if (ReplRange.overlapsWith(Ranges.front())) {
+if (auto Err = ReplacementsInRange.add(Repl)) {
+  CB(std::move(Err));
+}
+  }
+}
+CB(ReplacementsInRange);
   };
   WorkScheduler->runQuick("Format", File, std::move(Action));
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97417: [clangd] use a compatible preamble for the first AST built

[Quentin Chateau via Phabricator via cfe-commits]

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

fix bad arc diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97417

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

Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -312,6 +312,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
@@ -335,6 +337,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
@@ -96,6 +96,62 @@
 
 namespace {
 class ASTWorker;
+
+bool compileCommandsAreSimilar(const tooling::CompileCommand ,
+   const tooling::CompileCommand ) {
+  auto LIt = LHS.CommandLine.begin();
+  auto RIt = RHS.CommandLine.begin();
+
+  auto SkipSpecificArg = [](auto It, auto End, const std::string ) {
+while (It != End) {
+  if (*It == "-o" || *It == "-x") {
+// Erase -o and its argument
+// Erase -x and its argument: it would prevent using header file
+// preamble for a source file
+It = std::min(It + 2, End);
+  } else if (*It == "-c" || It->find("-W") == 0 || *It == "-pedantic") {
+// We don't care about those flags
+It++;
+  } else if (It->find(Filename) != std::string::npos) {
+// The file we're compiling appears in this argument, drop it to make it
+// generic
+It++;
+  } else {
+break;
+  }
+}
+return It;
+  };
+
+  // Iterate the command line, skipping arguments that are specific to the
+  // file being compiled and that would not influence the premable
+  // compatiblity. Stop when one iterate reach the or when an argument differs
+  auto LEnd = LHS.CommandLine.end();
+  auto REnd = RHS.CommandLine.end();
+  while (true) {
+LIt = SkipSpecificArg(LIt, LEnd, LHS.Filename);
+RIt = SkipSpecificArg(RIt, REnd, RHS.Filename);
+
+if (LIt == LEnd || RIt == REnd) {
+  break;
+}
+
+if (*LIt != *RIt) {
+  break;
+}
+
+LIt++;
+RIt++;
+  }
+
+  // If both iterator get to the end at the same time, the CL are compatible
+  bool Compatible = LIt == LEnd && RIt == REnd;
+  if (Compatible) {
+vlog("{0} and {1} are compatible", LHS.Filename, RHS.Filename);
+  }
+  return Compatible;
+}
+
 } // namespace
 
 static clang::clangd::Key kFileBeingProcessed;
@@ -301,6 +357,59 @@
   }
 };
 
+class TUScheduler::PreambleStore {
+public:
+  struct Entry {
+std::shared_ptr Preamble;
+size_t Score;
+Path FileName;
+
+bool operator>(const Entry ) const { return Score > Other.Score; }
+  };
+
+  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();
+}
+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() {
+constexpr int ClosedPreamblesToKeep = 5;
+auto Begin = llvm::partition(
+Store, [](const auto ) { return Item.Preamble.use_count() > 1; });
+if (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;
+};
+
 namespace {
 
 bool isReliable(const tooling::CompileCommand ) {
@@ -500,7 +609,8 @@
   ASTWorker(PathRef FileName, const GlobalCompilationDatabase ,
 TUScheduler::ASTCache ,
 TUScheduler::HeaderIncluderCache ,
-Semaphore , bool 

[PATCH] D97417: [clangd] use a compatible preamble for the first AST built

[Quentin Chateau via Phabricator via cfe-commits]

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

rebase on main, fix formatting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97417

Files:
  clang-tools-extra/clangd/TUScheduler.cpp


Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -609,8 +609,8 @@
   ASTWorker(PathRef FileName, const GlobalCompilationDatabase ,
 TUScheduler::ASTCache ,
 TUScheduler::HeaderIncluderCache ,
-TUScheduler::PreambleStore ,
-Semaphore , bool RunSync, const TUScheduler::Options ,
+TUScheduler::PreambleStore , Semaphore ,
+bool RunSync, const TUScheduler::Options ,
 ParsingCallbacks );
 
 public:
@@ -623,9 +623,9 @@
   create(PathRef FileName, const GlobalCompilationDatabase ,
  TUScheduler::ASTCache ,
  TUScheduler::HeaderIncluderCache ,
- TUScheduler::PreambleStore ,
- AsyncTaskRunner *Tasks, Semaphore ,
- const TUScheduler::Options , ParsingCallbacks );
+ TUScheduler::PreambleStore , AsyncTaskRunner *Tasks,
+ Semaphore , const TUScheduler::Options ,
+ ParsingCallbacks );
   ~ASTWorker();
 
   void update(ParseInputs Inputs, WantDiagnostics, bool ContentChanged);
@@ -844,11 +844,11 @@
 ASTWorker::ASTWorker(PathRef FileName, const GlobalCompilationDatabase ,
  TUScheduler::ASTCache ,
  TUScheduler::HeaderIncluderCache ,
-TUScheduler::PreambleStore ,
- Semaphore , bool RunSync,
- const TUScheduler::Options ,
+ TUScheduler::PreambleStore , Semaphore ,
+ bool RunSync, const TUScheduler::Options ,
  ParsingCallbacks )
-: IdleASTs(LRUCache), HeaderIncluders(HeaderIncluders), 
Preambles(Preambles), RunSync(RunSync),
+: IdleASTs(LRUCache), HeaderIncluders(HeaderIncluders),
+  Preambles(Preambles), RunSync(RunSync),
   UpdateDebounce(Opts.UpdateDebounce), FileName(FileName),
   ContextProvider(Opts.ContextProvider), CDB(CDB), Callbacks(Callbacks),
   Barrier(Barrier), Done(false), Status(FileName, Callbacks),


Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -609,8 +609,8 @@
   ASTWorker(PathRef FileName, const GlobalCompilationDatabase ,
 TUScheduler::ASTCache ,
 TUScheduler::HeaderIncluderCache ,
-TUScheduler::PreambleStore ,
-Semaphore , bool RunSync, const TUScheduler::Options ,
+TUScheduler::PreambleStore , Semaphore ,
+bool RunSync, const TUScheduler::Options ,
 ParsingCallbacks );
 
 public:
@@ -623,9 +623,9 @@
   create(PathRef FileName, const GlobalCompilationDatabase ,
  TUScheduler::ASTCache ,
  TUScheduler::HeaderIncluderCache ,
- TUScheduler::PreambleStore ,
- AsyncTaskRunner *Tasks, Semaphore ,
- const TUScheduler::Options , ParsingCallbacks );
+ TUScheduler::PreambleStore , AsyncTaskRunner *Tasks,
+ Semaphore , const TUScheduler::Options ,
+ ParsingCallbacks );
   ~ASTWorker();
 
   void update(ParseInputs Inputs, WantDiagnostics, bool ContentChanged);
@@ -844,11 +844,11 @@
 ASTWorker::ASTWorker(PathRef FileName, const GlobalCompilationDatabase ,
  TUScheduler::ASTCache ,
  TUScheduler::HeaderIncluderCache ,
-TUScheduler::PreambleStore ,
- Semaphore , bool RunSync,
- const TUScheduler::Options ,
+ TUScheduler::PreambleStore , Semaphore ,
+ bool RunSync, const TUScheduler::Options ,
  ParsingCallbacks )
-: IdleASTs(LRUCache), HeaderIncluders(HeaderIncluders), Preambles(Preambles), RunSync(RunSync),
+: IdleASTs(LRUCache), HeaderIncluders(HeaderIncluders),
+  Preambles(Preambles), RunSync(RunSync),
   UpdateDebounce(Opts.UpdateDebounce), FileName(FileName),
   ContextProvider(Opts.ContextProvider), CDB(CDB), Callbacks(Callbacks),
   Barrier(Barrier), Done(false), Status(FileName, Callbacks),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97417: [clangd] use a compatible preamble for the first AST built

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

I am much more afraid of providing bad results than I am afraid of degrading 
performance. I mean, eventually the "real" preamble is built, and the results 
are just as correct as before, but it may yield incorrect results until we 
invalidate the AST.
That is especially the case as I'm not very experienced in compiler design and 
I don't have a deep understanding of what happens behind the curtains when 
using a PCH.
There are probably cases where we degrade perf, but overall my experience with 
this patch is overwhelmingly positive, especially for "long coding sessions".

> I am afraid this can happen more than twice

I'm not sure I follow, I see these different situations (in TUScheduler):

- runWithPreamble
  - PreambleConsistency::Stale -> will wait for a "real" preamble, no change in 
behavior
  - PreambleConsistency::StaleOrAbsent
- a preamble is available, we'll use it, no change in behavior
- the preamble is not yet built, we'll use a compatible preamble instead -> 
this means instant partial completion !
- runWithAST
  - a preamble is available, we'll use it, no change in behavior
  - the preamble is not yet built, we'll use a compatible preamble to start 
building the AST straight away -> here there is a risk of double work, if the 
compatible preamble is useless, we'll start building the AST *while* we're also 
building the preamble

But I don't see how we can do worse than building the AST and the preamble at 
the same time ?

> could you give a bit more information on average preamble build latency in 
> the files you are testing this with

Extracts from VSCode logs, between the file being opened and the semantic token 
request completion.
Also worth noting that we have partial completion with 0 delay (with the 
compatible preamble)

Without the patch:

  I[22:40:57.317] ASTWorker building file 
/home/quentin/dev/llvm-project/clang-tools-extra/clangd/ParsedAST.h version 1 
with command inferred from 
/home/quentin/dev/llvm-project/clang-tools-extra/clangd/ParsedAST.cpp
  I[22:41:07.893] --> reply:textDocument/semanticTokens/full(1) 9921 ms
  
  I[22:41:16.526] ASTWorker building file 
/home/quentin/dev/llvm-project/clang-tools-extra/clangd/ParsedAST.cpp version 1 
with command
  I[22:41:38.401] --> reply:textDocument/semanticTokens/full(12) 21877 ms
  
  I[22:41:53.861] ASTWorker building file 
/home/quentin/dev/llvm-project/clang-tools-extra/clangd/IncludeFixer.h version 
1 with command inferred from 
/home/quentin/dev/llvm-project/clang-tools-extra/clangd/IncludeFixer.cpp
  I[22:42:06.006] --> reply:textDocument/semanticTokens/full(23) 12144 ms
  
  I[22:42:08.619] ASTWorker building file 
/home/quentin/dev/llvm-project/clang-tools-extra/clangd/IncludeFixer.cpp 
version 1 with command
  I[22:42:21.920] --> reply:textDocument/semanticTokens/full(28) 13278 ms

With the patch

  I[22:36:32.828] ASTWorker building file 
/home/quentin/dev/llvm-project/clang-tools-extra/clangd/ParsedAST.h version 1 
with command inferred from 
/home/quentin/dev/llvm-project/clang-tools-extra/clangd/ParsedAST.cpp
  I[22:36:43.211] --> reply:textDocument/semanticTokens/full(3) 9675 ms
  
  I[22:36:52.331] ASTWorker building file 
/home/quentin/dev/llvm-project/clang-tools-extra/clangd/ParsedAST.cpp version 1 
with command
  I[22:36:58.206] --> reply:textDocument/semanticTokens/full(9) 5881 ms
  
  I[22:37:13.474] ASTWorker building file 
/home/quentin/dev/llvm-project/clang-tools-extra/clangd/IncludeFixer.h version 
1 with command inferred from 
/home/quentin/dev/llvm-project/clang-tools-extra/clangd/IncludeFixer.cpp
  I[22:37:15.840] --> reply:textDocument/semanticTokens/full(15) 2351 ms
  
  I[22:37:23.939] ASTWorker building file 
/home/quentin/dev/llvm-project/clang-tools-extra/clangd/IncludeFixer.cpp 
version 1 with command
  I[22:37:25.030] --> reply:textDocument/semanticTokens/full(20) 1089 ms


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97417

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

[PATCH] D97417: [clangd] use a compatible preamble for the first AST built

[Quentin Chateau via Phabricator via cfe-commits]

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

- [clangd] make more compile commands compatible
- [clangd] ignore incompatible preamble

  I improved command line compatibility detection (faster & matches more files) 
and I had to blacklist some preambles. Namely, when the file we're building the 
AST for appears in the include tree of a preamble, we can't use it, otherwise 
symbols are missing from the AST. I feel like I understand why, but I have no 
idea how to fix it ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97417

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

Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -310,6 +310,8 @@
   /// Responsible for retaining and rebuilding idle ASTs. An implementation is
   /// an LRU cache.
   class ASTCache;
+  /// 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
@@ -332,6 +334,7 @@
   Semaphore QuickRunBarrier;
   llvm::StringMap> Files;
   std::unique_ptr IdleASTs;
+  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
@@ -94,6 +94,62 @@
 
 namespace {
 class ASTWorker;
+
+bool compileCommandsAreSimilar(const tooling::CompileCommand ,
+   const tooling::CompileCommand ) {
+  auto LIt = LHS.CommandLine.begin();
+  auto RIt = RHS.CommandLine.begin();
+
+  auto SkipSpecificArg = [](auto It, auto End, const std::string ) {
+while (It != End) {
+  if (*It == "-o" || *It == "-x") {
+// Erase -o and its argument
+// Erase -x and its argument: it would prevent using header file
+// preamble for a source file
+It = std::min(It + 2, End);
+  } else if (*It == "-c" || It->find("-W") == 0 || *It == "-pedantic") {
+// We don't care about those flags
+It++;
+  } else if (It->find(Filename) != std::string::npos) {
+// The file we're compiling appears in this argument, drop it to make it
+// generic
+It++;
+  } else {
+break;
+  }
+}
+return It;
+  };
+
+  // Iterate the command line, skipping arguments that are specific to the
+  // file being compiled and that would not influence the premable
+  // compatiblity. Stop when one iterate reach the or when an argument differs
+  auto LEnd = LHS.CommandLine.end();
+  auto REnd = RHS.CommandLine.end();
+  while (true) {
+LIt = SkipSpecificArg(LIt, LEnd, LHS.Filename);
+RIt = SkipSpecificArg(RIt, REnd, RHS.Filename);
+
+if (LIt == LEnd || RIt == REnd) {
+  break;
+}
+
+if (*LIt != *RIt) {
+  break;
+}
+
+LIt++;
+RIt++;
+  }
+
+  // If both iterator get to the end at the same time, the CL are compatible
+  bool Compatible = LIt == LEnd && RIt == REnd;
+  if (Compatible) {
+vlog("{0} and {1} are compatible", LHS.Filename, RHS.Filename);
+  }
+  return Compatible;
+}
+
 } // namespace
 
 static clang::clangd::Key kFileBeingProcessed;
@@ -179,6 +235,59 @@
   std::vector LRU; /* GUARDED_BY(Mut) */
 };
 
+class TUScheduler::PreambleStore {
+public:
+  struct Entry {
+std::shared_ptr Preamble;
+size_t Score;
+Path FileName;
+
+bool operator>(const Entry ) const { return Score > Other.Score; }
+  };
+
+  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();
+}
+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() {
+constexpr int ClosedPreamblesToKeep = 5;
+auto Begin = llvm::partition(
+Store, [](const auto ) { return Item.Preamble.use_count() > 1; });
+if (std::distance(Begin, Store.end()) <= ClosedPreamblesToKeep) {
+  return;
+}
+auto Nth = Begin + 

[PATCH] D97417: [clangd] use a compatible preamble for the first AST built

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

The hardcoded 5 premables can indeed be changed, I did not want to waste time 
on coding a configuration logic at such an early stage.

Well indeed do some extra work if we elect a compatible but almost useless 
preamble. We'll basically do the work twice (but at least we do it concurrently 
\o/). I can look into adding a heuristic layer to detect almost useless 
preambles and reject them. Though I'm not sure how I could do this as a single 
header can contain 99% of the preamble through include chains

Anyway that's the current state of this patch: it's already pretty cool (it 
does speed things up noticeably) but there are probably tons of flaws or 
potential improvements and I'd like to receive comments from another POV


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97417

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

[PATCH] D97417: [clangd] use a compatible preamble for the first AST built

[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.

Keep a store of the preambles of all opened filed, plus up to
5 previously used preambles. 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.

-

This patch is starting to look like something acceptable, so
I'm sending it for review. At this point I'd like to get some 
feedback on this before going further.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D97417

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

Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -310,6 +310,8 @@
   /// Responsible for retaining and rebuilding idle ASTs. An implementation is
   /// an LRU cache.
   class ASTCache;
+  /// 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
@@ -332,6 +334,7 @@
   Semaphore QuickRunBarrier;
   llvm::StringMap> Files;
   std::unique_ptr IdleASTs;
+  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
@@ -94,6 +94,21 @@
 
 namespace {
 class ASTWorker;
+
+bool compileCommandsAreSimilar(const tooling::CompileCommand ,
+   const tooling::CompileCommand ) {
+  std::vector LHSCL(LHS.CommandLine);
+  auto LHSBasename = llvm::sys::path::filename(LHS.Filename).str();
+  auto RHSBasename = llvm::sys::path::filename(RHS.Filename).str();
+  for (auto  : LHSCL) {
+for (auto Pos = Arg.find(LHSBasename); Pos != std::string::npos;
+ Pos = Arg.find(LHSBasename, Pos + 1)) {
+  Arg.replace(Pos, LHSBasename.size(), RHSBasename);
+}
+  }
+  return llvm::makeArrayRef(LHSCL).equals(RHS.CommandLine);
+}
+
 } // namespace
 
 static clang::clangd::Key kFileBeingProcessed;
@@ -179,6 +194,59 @@
   std::vector LRU; /* GUARDED_BY(Mut) */
 };
 
+class TUScheduler::PreambleStore {
+public:
+  struct Entry {
+std::shared_ptr Preamble;
+size_t Score;
+Path FileName;
+
+bool operator>(const Entry ) const { return Score > Other.Score; }
+  };
+
+  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();
+}
+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() {
+constexpr int ClosedPreamblesToKeep = 5;
+auto Begin = llvm::partition(
+Store, [](const auto ) { return Item.Preamble.use_count() > 1; });
+if (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;
+};
+
 namespace {
 /// Threadsafe manager for updating a TUStatus and emitting it after each
 /// update.
@@ -368,8 +436,10 @@
 class ASTWorker {
   friend class ASTWorkerHandle;
   ASTWorker(PathRef FileName, const GlobalCompilationDatabase ,
-TUScheduler::ASTCache , Semaphore , bool RunSync,
-const TUScheduler::Options , ParsingCallbacks );
+TUScheduler::ASTCache ,
+TUScheduler::PreambleStore , Semaphore ,
+bool RunSync, const TUScheduler::Options ,
+ParsingCallbacks );
 
 public:
   /// Create a new ASTWorker and return a handle to it.
@@ -377,12 +447,11 @@
   /// is null, all requests will be processed on the calling thread
   /// synchronously instead. \p Barrier is acquired when processing each
   /// request, it is used to limit 

[PATCH] D94875: [clangd] ignore parallelism level for quick tasks

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

I applied your suggestion as I agree with both. I chose to use 
`Opts.AsyncThreadsCount` instead of a hard-coded constant. This way the 
"formatting speed" will grow as the user allow more parallelism, which IMO 
makes sense.

I agree the implementation had drawbacks but this is the ever-ending problem of 
non-preemptable tasks. You can land this if this looks good to you.

Commit email: quentin.chat...@gmail.com


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94875

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

[PATCH] D94875: [clangd] ignore parallelism level for quick tasks

[Quentin Chateau via Phabricator via cfe-commits]

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

- [clangd] fix nits


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94875

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


Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -244,6 +244,13 @@
   void run(llvm::StringRef Name, llvm::StringRef Path,
llvm::unique_function Action);
 
+  /// Similar to run, except the task is expected to be quick.
+  /// This function will not honor AsyncThreadsCount (except
+  /// if threading is disabled with AsyncThreadsCount=0)
+  /// It is intended to run quick tasks that need to run ASAP
+  void runQuick(llvm::StringRef Name, llvm::StringRef Path,
+llvm::unique_function Action);
+
   /// Defines how a runWithAST action is implicitly cancelled by other actions.
   enum ASTActionInvalidation {
 /// The request will run unless explicitly cancelled.
@@ -316,10 +323,14 @@
   void profile(MemoryTree ) const;
 
 private:
+  void runWithSemaphore(llvm::StringRef Name, llvm::StringRef Path,
+llvm::unique_function Action, Semaphore );
+
   const GlobalCompilationDatabase 
   Options Opts;
   std::unique_ptr Callbacks; // not nullptr
   Semaphore Barrier;
+  Semaphore QuickRunBarrier;
   llvm::StringMap> Files;
   std::unique_ptr IdleASTs;
   // None when running tasks synchronously and non-None when running tasks
Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -1248,7 +1248,7 @@
 : CDB(CDB), Opts(Opts),
   Callbacks(Callbacks ? move(Callbacks)
   : std::make_unique()),
-  Barrier(Opts.AsyncThreadsCount),
+  Barrier(Opts.AsyncThreadsCount), QuickRunBarrier(Opts.AsyncThreadsCount),
   IdleASTs(
   std::make_unique(Opts.RetentionPolicy.MaxRetainedASTs)) {
   // Avoid null checks everywhere.
@@ -1322,14 +1322,27 @@
 
 void TUScheduler::run(llvm::StringRef Name, llvm::StringRef Path,
   llvm::unique_function Action) {
+  runWithSemaphore(Name, Path, std::move(Action), Barrier);
+}
+
+void TUScheduler::runQuick(llvm::StringRef Name, llvm::StringRef Path,
+   llvm::unique_function Action) {
+  // Use QuickRunBarrier to serialize quick tasks: we are ignoring
+  // the parallelism level set by the user, don't abuse it
+  runWithSemaphore(Name, Path, std::move(Action), QuickRunBarrier);
+}
+
+void TUScheduler::runWithSemaphore(llvm::StringRef Name, llvm::StringRef Path,
+   llvm::unique_function Action,
+   Semaphore ) {
   if (!PreambleTasks) {
 WithContext WithProvidedContext(Opts.ContextProvider(Path));
 return Action();
   }
-  PreambleTasks->runAsync(Name, [this, Ctx = Context::current().clone(),
+  PreambleTasks->runAsync(Name, [this, , Ctx = Context::current().clone(),
  Path(Path.str()),
  Action = std::move(Action)]() mutable {
-std::lock_guard BarrierLock(Barrier);
+std::lock_guard BarrierLock(Sem);
 WithContext WC(std::move(Ctx));
 WithContext WithProvidedContext(Opts.ContextProvider(Path));
 Action();
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -351,7 +351,7 @@
   Result.push_back(replacementToEdit(Code, R));
 return CB(Result);
   };
-  WorkScheduler.run("FormatOnType", File, std::move(Action));
+  WorkScheduler.runQuick("FormatOnType", File, std::move(Action));
 }
 
 void ClangdServer::prepareRename(PathRef File, Position Pos,
@@ -579,7 +579,7 @@
 tooling::calculateRangesAfterReplacements(IncludeReplaces, Ranges),
 File)));
   };
-  WorkScheduler.run("Format", File, std::move(Action));
+  WorkScheduler.runQuick("Format", File, std::move(Action));
 }
 
 void ClangdServer::findDocumentHighlights(


Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -244,6 +244,13 @@
   void run(llvm::StringRef Name, llvm::StringRef Path,
llvm::unique_function Action);
 
+  /// Similar to run, except the task is expected to be quick.
+  /// This function will not honor AsyncThreadsCount (except
+  /// if threading is disabled with AsyncThreadsCount=0)
+  /// It is intended to run quick tasks 

[PATCH] D94875: [clangd] ignore parallelism level for quick tasks

[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.

This allows quick tasks without dependencies that
need to run fast to run ASAP. This is mostly useful
for code formatting.



This fixes something that's been annoying me:

- Open your IDE workspace and its 20 open files
- Clangd spends 5 minutes parsing it all
- In the meantime you start to work
- Save a file, trigger format-on-save, which hangs because clangd is busy
- You're stuck waiting until clangd is done parsing your files before the 
formatting and save takes place


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D94875

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


Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -244,6 +244,13 @@
   void run(llvm::StringRef Name, llvm::StringRef Path,
llvm::unique_function Action);
 
+  /// Similar to run, except the task is expected to be quick.
+  /// This function will not honor AsyncThreadsCount (except
+  /// if threading is disabled with AsyncThreadsCount=0)
+  /// It is intended to run quick tasks that need to run ASAP
+  void quickRun(llvm::StringRef Name, llvm::StringRef Path,
+llvm::unique_function Action);
+
   /// Defines how a runWithAST action is implicitly cancelled by other actions.
   enum ASTActionInvalidation {
 /// The request will run unless explicitly cancelled.
@@ -316,10 +323,14 @@
   void profile(MemoryTree ) const;
 
 private:
+  void runWithSemaphore(llvm::StringRef Name, llvm::StringRef Path,
+llvm::unique_function Action, Semaphore );
+
   const GlobalCompilationDatabase 
   Options Opts;
   std::unique_ptr Callbacks; // not nullptr
   Semaphore Barrier;
+  Semaphore QuickRunBarrier;
   llvm::StringMap> Files;
   std::unique_ptr IdleASTs;
   // None when running tasks synchronously and non-None when running tasks
Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -1248,7 +1248,7 @@
 : CDB(CDB), Opts(Opts),
   Callbacks(Callbacks ? move(Callbacks)
   : std::make_unique()),
-  Barrier(Opts.AsyncThreadsCount),
+  Barrier(Opts.AsyncThreadsCount), QuickRunBarrier(1),
   IdleASTs(
   std::make_unique(Opts.RetentionPolicy.MaxRetainedASTs)) {
   // Avoid null checks everywhere.
@@ -1322,14 +1322,27 @@
 
 void TUScheduler::run(llvm::StringRef Name, llvm::StringRef Path,
   llvm::unique_function Action) {
+  runWithSemaphore(Name, Path, std::move(Action), Barrier);
+}
+
+void TUScheduler::quickRun(llvm::StringRef Name, llvm::StringRef Path,
+   llvm::unique_function Action) {
+  // Use QuickRunBarrier to serialize quick tasks: we are ignoring
+  // the parallelism level set by the user, don't abuse it
+  runWithSemaphore(Name, Path, std::move(Action), QuickRunBarrier);
+}
+
+void TUScheduler::runWithSemaphore(llvm::StringRef Name, llvm::StringRef Path,
+   llvm::unique_function Action,
+   Semaphore ) {
   if (!PreambleTasks) {
 WithContext WithProvidedContext(Opts.ContextProvider(Path));
 return Action();
   }
-  PreambleTasks->runAsync(Name, [this, Ctx = Context::current().clone(),
+  PreambleTasks->runAsync(Name, [this, , Ctx = Context::current().clone(),
  Path(Path.str()),
  Action = std::move(Action)]() mutable {
-std::lock_guard BarrierLock(Barrier);
+std::lock_guard BarrierLock(Sem);
 WithContext WC(std::move(Ctx));
 WithContext WithProvidedContext(Opts.ContextProvider(Path));
 Action();
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -351,7 +351,7 @@
   Result.push_back(replacementToEdit(Code, R));
 return CB(Result);
   };
-  WorkScheduler.run("FormatOnType", File, std::move(Action));
+  WorkScheduler.quickRun("FormatOnType", File, std::move(Action));
 }
 
 void ClangdServer::prepareRename(PathRef File, Position Pos,
@@ -579,7 +579,7 @@
 tooling::calculateRangesAfterReplacements(IncludeReplaces, Ranges),
 File)));
   };
-  WorkScheduler.run("Format", File, std::move(Action));
+  WorkScheduler.quickRun("Format", File, 

[PATCH] D93829: [clangd] Support outgoing calls in call hierarchy

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

You're right, I probably got carried away for the sake of completeness. 
Allocating extra memory for a feature nobody is going to use is definitely not 
worth it. Though maybe we can take advantage of what's already been done and if 
we remove the `RevRefs` map, and pay the full price of iterating all refs when 
`refersTo` is called, we still have this feature. Although slower, it shouldn't 
take more than a few 100's of milliseconds even on a big project. Which means 
the feature is usable, at least for most small to medium sized projects, at no 
extra cost for people who do not use it.

Would this be acceptable ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93829

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

[PATCH] D93452: [clangd] Trim memory periodically

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

If you look back at my original comment, you'll notice I originally trimmed the 
memory after `FileSymbols::buildIndex` which seemed to be the "end of the peak" 
memory usage, the goal was to counteract exactly what you describe: a huge 
memory usage when clangd warms-up and parses a lot of files in parallel.
I wonder if we can find a middle ground. The solution we finally adopted also 
has the advantage of being less intrusive, which is definitely great 
considering the nature of the issue


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452

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

[PATCH] D93873: [clangd] Cache preambles of closed files

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

Hey, don't worry about the delay, I won't have as much time on my hands anymore 
anyway.

- we can definitely make this opt-in
- MB instead of # is an idea, it's probably closer to what the user want to 
configure - if he does - but it would also probably give a worse default value. 
Your call !
- I mean why not, but this diff was more about bringing the feature in than 
making it perfect. The heuristic to decide how to evict preambles can be made 
very complicated if we want to optimize it
- we can do as you suggest, it related to the previous point
- Ah ! What would you recommend ? No forget it, I'll just make it opt-it for now

I've also tried to keep the ASTWorkers alive in a different branch. The result 
is very different: ASTWorkers always use RAM so we can't keep as many, but 
keeping them alive makes is even better (15s with no cache, 2s with preamble 
cache, virtually 0s with ASTWorker cache). I'd say both features are nice but 
they give different results for a different cost.
Also it would be great to stop the ASTWorker threads when they are just in 
cache, but the class needs a rework to be able to stop/restart it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93873

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

[PATCH] D93829: [clangd] Support outgoing calls in call hierarchy

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added inline comments.



Comment at: clang-tools-extra/clangd/XRefs.cpp:1843
+auto Kind = Callee.SymInfo.Kind;
+if (Kind != SK::Function && Kind != SK::InstanceMethod &&
+Kind != SK::ClassMethod && Kind != SK::StaticMethod &&

nridge wrote:
> If memory usage of `Dex::RevRefs` becomes a concern, we could consider only 
> storing the references that would pass this filter in the first place. That 
> would trade off time spent building the reverse lookup (we'd have to do 
> symbol lookups or keep around extra state to track the symbol kinds) for 
> space savings.
I've used tried this patch with the whole llvm project indexed. If I remember 
correctly it's something in the like of 100MB, around 5-10% même usage of 
clangd. As such it's not very high but it's definitely not negligible, 
especially for a feature that's not used very often.



Comment at: clang-tools-extra/clangd/index/Index.h:85
 
+struct RefersToResult {
+  /// The source location where the symbol is named.

nridge wrote:
> As the protocol wants the outgoing calls grouped by symbol, we could consider 
> storing them (and having the `SymbolIndex` API expose them) in that way.
> 
> The main motivation would be space savings on the storage side 
> (`Dex::RevRefs`), as in the case of multiple calls to the same function we 
> only need to store the target `SymbolID` once.
> 
> However, it may not be worth doing this, as having a large number of outgoing 
> calls to the same target inside a given function is likely to be rare, and 
> vectors have their own overhead. (It's also not clear to what extent the 
> details of the LSP protocol should dictate the shape of the `SymbolIndex` 
> API.)
I indeed believe that I'd use more memory than it would save, but I did not try 
it.
Anyway I think the difference would be minimal, and I'd say we should choose 
what gives the "best" API



Comment at: clang-tools-extra/clangd/index/Index.h:133
+  virtual bool
+  refersTo(const RefsRequest ,
+   llvm::function_ref Callback) const = 
0;

nridge wrote:
> Perhaps we should have a distinct request structure, for future extensibility?
Sure, that makes sense



Comment at: clang-tools-extra/clangd/index/MemIndex.cpp:97
+  Req.Limit.getValueOr(std::numeric_limits::max());
+  for (const auto  : Refs) {
+for (const auto  : Pair.second) {

nridge wrote:
> Looping over all refs seems expensive even for `MemIndex`. Perhaps we should 
> have a reverse lookup table similar to `RevRefs` here as well?
That's pretty fast even on my laptop IIRC, and only used for outgoing calls 
atm. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93829

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

[PATCH] D93873: [clangd] Cache preambles of closed files

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

New year's ping ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93873

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

[PATCH] D94265: [clangd] Search compiler in PATH for system include extraction If the compiler is specified by its name, search it in the system PATH instead of the command directory: this is what th

[Quentin Chateau via Phabricator via cfe-commits]

qchateau abandoned this revision.
qchateau added a comment.

Ah ! It's been done already !
https://reviews.llvm.org/D93600


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94265

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

[PATCH] D94265: [clangd] Search compiler in PATH for system include extraction If the compiler is specified by its name, search it in the system PATH instead of the command directory: this is what th

[Quentin Chateau via Phabricator via cfe-commits]

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D94265

Files:
  clang-tools-extra/clangd/QueryDriverDatabase.cpp


Index: clang-tools-extra/clangd/QueryDriverDatabase.cpp
===
--- clang-tools-extra/clangd/QueryDriverDatabase.cpp
+++ clang-tools-extra/clangd/QueryDriverDatabase.cpp
@@ -136,24 +136,21 @@
 }
 
 llvm::Optional
-extractSystemIncludesAndTarget(PathRef Driver, llvm::StringRef Lang,
+extractSystemIncludesAndTarget(llvm::StringRef Driver, llvm::StringRef Lang,
llvm::ArrayRef CommandLine,
const llvm::Regex ) {
   trace::Span Tracer("Extract system includes and target");
   SPAN_ATTACH(Tracer, "driver", Driver);
   SPAN_ATTACH(Tracer, "lang", Lang);
 
-  if (!QueryDriverRegex.match(Driver)) {
-vlog("System include extraction: not allowed driver {0}", Driver);
+  auto DriverPath = llvm::sys::findProgramByName(Driver);
+  if (auto Error = DriverPath.getError()) {
+elog("System include extraction: {0} - {1}", Error.message(), Driver);
 return llvm::None;
   }
 
-  if (!llvm::sys::fs::exists(Driver)) {
-elog("System include extraction: {0} does not exist.", Driver);
-return llvm::None;
-  }
-  if (!llvm::sys::fs::can_execute(Driver)) {
-elog("System include extraction: {0} is not executable.", Driver);
+  if (!QueryDriverRegex.match(*DriverPath)) {
+vlog("System include extraction: not allowed driver {0}", *DriverPath);
 return llvm::None;
   }
 
@@ -171,8 +168,8 @@
   llvm::Optional Redirects[] = {
   {""}, {""}, llvm::StringRef(StdErrPath)};
 
-  llvm::SmallVector Args = {Driver, "-E", "-x",
- Lang,   "-",  "-v"};
+  llvm::SmallVector Args = {*DriverPath, "-E", "-x",
+ Lang,"-",  "-v"};
 
   // These flags will be preserved
   const llvm::StringRef FlagsToPreserve[] = {
@@ -203,8 +200,8 @@
 }
   }
 
-  if (int RC = llvm::sys::ExecuteAndWait(Driver, Args, /*Env=*/llvm::None,
- Redirects)) {
+  if (int RC = llvm::sys::ExecuteAndWait(*DriverPath, Args,
+ /*Env=*/llvm::None, Redirects)) {
 elog("System include extraction: driver execution failed with return code: 
"
  "{0}. Args: ['{1}']",
  llvm::to_string(RC), llvm::join(Args, "', '"));
@@ -224,7 +221,7 @@
 return llvm::None;
   log("System includes extractor: successfully executed {0}\n\tgot includes: "
   "\"{1}\"\n\tgot target: \"{2}\"",
-  Driver, llvm::join(Info->SystemIncludes, ", "), Info->Target);
+  *DriverPath, llvm::join(Info->SystemIncludes, ", "), Info->Target);
   return Info;
 }
 
@@ -332,7 +329,6 @@
 }
 
 llvm::SmallString<128> Driver(Cmd->CommandLine.front());
-llvm::sys::fs::make_absolute(Cmd->Directory, Driver);
 
 if (auto Info =
 QueriedDrivers.get(/*Key=*/(Driver + ":" + Lang).str(), [&] {


Index: clang-tools-extra/clangd/QueryDriverDatabase.cpp
===
--- clang-tools-extra/clangd/QueryDriverDatabase.cpp
+++ clang-tools-extra/clangd/QueryDriverDatabase.cpp
@@ -136,24 +136,21 @@
 }
 
 llvm::Optional
-extractSystemIncludesAndTarget(PathRef Driver, llvm::StringRef Lang,
+extractSystemIncludesAndTarget(llvm::StringRef Driver, llvm::StringRef Lang,
llvm::ArrayRef CommandLine,
const llvm::Regex ) {
   trace::Span Tracer("Extract system includes and target");
   SPAN_ATTACH(Tracer, "driver", Driver);
   SPAN_ATTACH(Tracer, "lang", Lang);
 
-  if (!QueryDriverRegex.match(Driver)) {
-vlog("System include extraction: not allowed driver {0}", Driver);
+  auto DriverPath = llvm::sys::findProgramByName(Driver);
+  if (auto Error = DriverPath.getError()) {
+elog("System include extraction: {0} - {1}", Error.message(), Driver);
 return llvm::None;
   }
 
-  if (!llvm::sys::fs::exists(Driver)) {
-elog("System include extraction: {0} does not exist.", Driver);
-return llvm::None;
-  }
-  if (!llvm::sys::fs::can_execute(Driver)) {
-elog("System include extraction: {0} is not executable.", Driver);
+  if (!QueryDriverRegex.match(*DriverPath)) {
+vlog("System include extraction: not allowed driver {0}", *DriverPath);
 return llvm::None;
   }
 
@@ -171,8 +168,8 @@
   llvm::Optional Redirects[] = {
   {""}, {""}, llvm::StringRef(StdErrPath)};
 
-  llvm::SmallVector Args = {Driver, "-E", "-x",
- Lang,   "-",  "-v"};
+  llvm::SmallVector Args = {*DriverPath, "-E", "-x",
+  

[PATCH] D93873: [clangd] Cache preambles of closed files

[Quentin Chateau via Phabricator via cfe-commits]

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

- Fix keep preamble command line option
- Fix tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93873

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/test/memory_tree.test
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp

Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -1256,7 +1256,7 @@
   MemoryTree MT();
   Server.profile(MT);
   ASSERT_TRUE(MT.children().count("tuscheduler"));
-  EXPECT_TRUE(MT.child("tuscheduler").children().count(FooCpp));
+  EXPECT_TRUE(MT.child("tuscheduler").child("files").children().count(FooCpp));
 }
 } // namespace
 } // namespace clangd
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -347,6 +347,16 @@
 init(getDefaultAsyncThreadsCount()),
 };
 
+// Magic value to know when the user does not specify a value
+constexpr size_t DefaultKeepPreambleValue = std::numeric_limits::max();
+constexpr size_t DefaultKeepPreambleMemory = 10;
+constexpr size_t DefaultKeepPreambleDisk = 1000;
+opt KeepPreambles{
+"keep-preambles", cat(Misc),
+desc("Number of preambles of closed files that clangd will keep in cache.\n"
+ "Note that preambles may be stored in memory or in disk."),
+init(DefaultKeepPreambleValue)};
+
 opt IndexFile{
 "index-file",
 cat(Misc),
@@ -821,6 +831,13 @@
 Opts.StaticIndex = PAI.get();
   }
   Opts.AsyncThreadsCount = WorkerThreadsCount;
+
+  if (KeepPreambles == DefaultKeepPreambleValue) // User did not specify a value
+Opts.KeepPreambles = Opts.StorePreamblesInMemory ? DefaultKeepPreambleMemory
+ : DefaultKeepPreambleDisk;
+  else
+Opts.KeepPreambles = KeepPreambles;
+
   Opts.BuildRecoveryAST = RecoveryAST;
   Opts.PreserveRecoveryASTType = RecoveryASTType;
   Opts.FoldingRanges = FoldingRanges;
Index: clang-tools-extra/clangd/test/memory_tree.test
===
--- clang-tools-extra/clangd/test/memory_tree.test
+++ clang-tools-extra/clangd/test/memory_tree.test
@@ -57,20 +57,32 @@
 # CHECK-NEXT: }
 # CHECK-NEXT:   },
 # CHECK-NEXT:   "tuscheduler": {
-# CHECK-NEXT: "{{.*}}main.cpp": {
+# CHECK-NEXT: "_self": {{[0-9]+}},
+# CHECK-NEXT: "_total": {{[0-9]+}},
+# CHECK-NEXT: "cache": {
 # CHECK-NEXT:   "_self": {{[0-9]+}},
 # CHECK-NEXT:   "_total": {{[0-9]+}},
-# CHECK-NEXT:   "ast": {
-# CHECK-NEXT: "_self": {{[0-9]+}},
-# CHECK-NEXT: "_total": {{[0-9]+}}
-# CHECK-NEXT:   },
-# CHECK-NEXT:   "preamble": {
+# CHECK-NEXT:   "containers": {
 # CHECK-NEXT: "_self": {{[0-9]+}},
 # CHECK-NEXT: "_total": {{[0-9]+}}
 # CHECK-NEXT:   }
 # CHECK-NEXT: },
-# CHECK-NEXT: "_self": {{[0-9]+}},
-# CHECK-NEXT: "_total": {{[0-9]+}}
+# CHECK-NEXT: "files": {
+# CHECK-NEXT:   "{{.*}}main.cpp": {
+# CHECK-NEXT: "_self": {{[0-9]+}},
+# CHECK-NEXT: "_total": {{[0-9]+}},
+# CHECK-NEXT: "ast": {
+# CHECK-NEXT:   "_self": {{[0-9]+}},
+# CHECK-NEXT:   "_total": {{[0-9]+}}
+# CHECK-NEXT: },
+# CHECK-NEXT: "preamble": {
+# CHECK-NEXT:   "_self": {{[0-9]+}},
+# CHECK-NEXT:   "_total": {{[0-9]+}}
+# CHECK-NEXT: }
+# CHECK-NEXT:   },
+# CHECK-NEXT:   "_self": {{[0-9]+}},
+# CHECK-NEXT:   "_total": {{[0-9]+}}
+# CHECK-NEXT: }
 # CHECK-NEXT:   }
 # CHECK-NEXT: }
 # CHECK-NEXT:   }
Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -197,6 +197,10 @@
 /// No-op if AsyncThreadsCount is 0.
 bool AsyncPreambleBuilds = true;
 
+// The number of preambles that will be retained even after the file is
+// closed
+size_t KeepPreambles = 0;
+
 /// Used to create a context that wraps each single operation.
 /// Typically to inject per-file configuration.
 /// If the path is empty, context sholud be "generic".
@@ -305,6 +309,9 @@
   /// an LRU cache.
   class ASTCache;
 
+  /// Responsible for retaining preambles.
+  

[PATCH] D93873: [clangd] Cache preambles of closed files

[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.

When a file is closed, push its preamble to a LRU cache
When a file is opened, try to get the preamble from the LRU cache.

By default store 10 preambles if they are stored on
memory and 1000 if they are stored on disk. That value
can be modified with --keep-preambles=N


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D93873

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

Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -1256,7 +1256,7 @@
   MemoryTree MT();
   Server.profile(MT);
   ASSERT_TRUE(MT.children().count("tuscheduler"));
-  EXPECT_TRUE(MT.child("tuscheduler").children().count(FooCpp));
+  EXPECT_TRUE(MT.child("tuscheduler").child("files").children().count(FooCpp));
 }
 } // namespace
 } // namespace clangd
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -347,6 +347,15 @@
 init(getDefaultAsyncThreadsCount()),
 };
 
+constexpr size_t DefaultKeepPreambleMemory = 10;
+constexpr size_t DefaultKeepPreambleDisk = 1000;
+opt> KeepPreambles{
+"keep-preambles",
+cat(Misc),
+desc("Number of preambles of closed files that clangd will keep in cache.\n"
+ "Note that preambles may be stored in memory or in disk.")
+};
+
 opt IndexFile{
 "index-file",
 cat(Misc),
@@ -821,6 +830,10 @@
 Opts.StaticIndex = PAI.get();
   }
   Opts.AsyncThreadsCount = WorkerThreadsCount;
+  Opts.KeepPreambles = KeepPreambles.getValueOr(
+Opts.StorePreamblesInMemory ? DefaultKeepPreambleMemory :
+DefaultKeepPreambleDisk
+  );
   Opts.BuildRecoveryAST = RecoveryAST;
   Opts.PreserveRecoveryASTType = RecoveryASTType;
   Opts.FoldingRanges = FoldingRanges;
Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -197,6 +197,10 @@
 /// No-op if AsyncThreadsCount is 0.
 bool AsyncPreambleBuilds = true;
 
+// The number of preambles that will be retained even after the file is
+// closed
+size_t KeepPreambles = 0;
+
 /// Used to create a context that wraps each single operation.
 /// Typically to inject per-file configuration.
 /// If the path is empty, context sholud be "generic".
@@ -305,6 +309,9 @@
   /// an LRU cache.
   class ASTCache;
 
+  /// Responsible for retaining preambles.
+  class PreambleCache;
+
   // 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
   // this inside clangd.
@@ -321,6 +328,7 @@
   std::unique_ptr Callbacks; // not nullptr
   Semaphore Barrier;
   llvm::StringMap> Files;
+  std::unique_ptr CachedPreambles;
   std::unique_ptr IdleASTs;
   // None when running tasks synchronously and non-None when running tasks
   // asynchronously.
Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -179,6 +179,91 @@
   std::vector LRU; /* GUARDED_BY(Mut) */
 };
 
+/// LRU cache with amortized O(1) put and take
+/// Preambles can be stored on disk so we may want to store a high
+/// number of entries
+class TUScheduler::PreambleCache {
+public:
+  PreambleCache(size_t MaxSize, bool StorePreamblesInMemory)
+  : MaxSize(MaxSize), StorePreamblesInMemory(StorePreamblesInMemory) {
+vlog("TUScheduler will cache {0} preambles", MaxSize);
+  }
+
+  /// Get the preamble associated with a \p Key, removing
+  /// it from the cache
+  std::shared_ptr take(llvm::StringRef Key) {
+auto It = Data.find(Key);
+if (It == Data.end())
+  return nullptr;
+auto Result = std::move(It->second);
+
+// Remove the key from all internal data structures
+auto KeyToLRUIt = KeyToLRU.find(Key);
+assert(KeyToLRUIt != KeyToLRU.end() && "Key is missing");
+auto LRUIt = KeyToLRUIt->second;
+Data.erase(It);
+KeyToLRU.erase(KeyToLRUIt);
+LRU.erase(LRUIt);
+
+return Result;
+  }
+
+  /// Add a \p Preamble associated with a \p Key, the preamble must
+  /// not 

[PATCH] D93829: [clangd] Support outgoing calls in call hierarchy

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

Note: Build fails due to clang-tidy warnings in tests. I chose to keep the 
naming consistent with what's already in place rather than fix the clang-tidy 
warning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93829

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

[PATCH] D93829: [clangd] Support outgoing calls in call hierarchy

[Quentin Chateau via Phabricator via cfe-commits]

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

- Smaller reversed refs memory usage
- Fix Dex::estimateMemoryUsage


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93829

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/MemIndex.cpp
  clang-tools-extra/clangd/index/MemIndex.h
  clang-tools-extra/clangd/index/Merge.cpp
  clang-tools-extra/clangd/index/Merge.h
  clang-tools-extra/clangd/index/ProjectAware.cpp
  clang-tools-extra/clangd/index/dex/Dex.cpp
  clang-tools-extra/clangd/index/dex/Dex.h
  clang-tools-extra/clangd/test/type-hierarchy.test
  clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1225,6 +1225,12 @@
   return true; // has more references
 }
 
+bool refersTo(const RefsRequest ,
+  llvm::function_ref Callback)
+const override {
+  return false;
+}
+
 bool fuzzyFind(
 const FuzzyFindRequest ,
 llvm::function_ref Callback) const override {
@@ -1281,6 +1287,12 @@
   return false;
 }
 
+bool refersTo(const RefsRequest ,
+  llvm::function_ref Callback)
+const override {
+  return false;
+}
+
 bool fuzzyFind(const FuzzyFindRequest &,
llvm::function_ref) const override {
   return false;
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -1345,6 +1345,12 @@
 return false;
   }
 
+  bool
+  refersTo(const RefsRequest &,
+   llvm::function_ref) const override {
+return false;
+  }
+
   void relations(const RelationsRequest &,
  llvm::function_ref)
   const override {}
Index: clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
===
--- clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
+++ clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
@@ -37,17 +37,27 @@
 
 // Helpers for matching call hierarchy data structures.
 MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithDetail, N, "") { return arg.detail == N; }
 MATCHER_P(WithSelectionRange, R, "") { return arg.selectionRange == R; }
 
 template 
 ::testing::Matcher From(ItemMatcher M) {
   return Field(::from, M);
 }
+template 
+::testing::Matcher To(ItemMatcher M) {
+  return Field(::to, M);
+}
 template 
-::testing::Matcher FromRanges(RangeMatchers... M) {
+::testing::Matcher IFromRanges(RangeMatchers... M) {
   return Field(::fromRanges,
UnorderedElementsAre(M...));
 }
+template 
+::testing::Matcher OFromRanges(RangeMatchers... M) {
+  return Field(::fromRanges,
+   UnorderedElementsAre(M...));
+}
 
 TEST(CallHierarchy, IncomingOneFile) {
   Annotations Source(R"cpp(
@@ -72,22 +82,25 @@
   prepareCallHierarchy(AST, Source.point(), testPath(TU.Filename));
   ASSERT_THAT(Items, ElementsAre(WithName("callee")));
   auto IncomingLevel1 = incomingCalls(Items[0], Index.get());
-  ASSERT_THAT(IncomingLevel1,
-  ElementsAre(AllOf(From(WithName("caller1")),
-FromRanges(Source.range("Callee");
+  ASSERT_THAT(
+  IncomingLevel1,
+  ElementsAre(AllOf(From(AllOf(WithName("caller1"), WithDetail("caller1"))),
+IFromRanges(Source.range("Callee");
 
   auto IncomingLevel2 = incomingCalls(IncomingLevel1[0].from, Index.get());
-  ASSERT_THAT(IncomingLevel2,
-  ElementsAre(AllOf(From(WithName("caller2")),
-FromRanges(Source.range("Caller1A"),
-   Source.range("Caller1B"))),
-  AllOf(From(WithName("caller3")),
-FromRanges(Source.range("Caller1C");
+  ASSERT_THAT(
+  IncomingLevel2,
+  ElementsAre(AllOf(From(AllOf(WithName("caller2"), WithDetail("caller2"))),
+IFromRanges(Source.range("Caller1A"),
+Source.range("Caller1B"))),
+  AllOf(From(AllOf(WithName("caller3"), WithDetail("caller3"))),
+

[PATCH] D93829: [clangd] Support outgoing calls in call hierarchy

[Quentin Chateau via Phabricator via cfe-commits]

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

- Fix tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93829

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/MemIndex.cpp
  clang-tools-extra/clangd/index/MemIndex.h
  clang-tools-extra/clangd/index/Merge.cpp
  clang-tools-extra/clangd/index/Merge.h
  clang-tools-extra/clangd/index/ProjectAware.cpp
  clang-tools-extra/clangd/index/dex/Dex.cpp
  clang-tools-extra/clangd/index/dex/Dex.h
  clang-tools-extra/clangd/test/type-hierarchy.test
  clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1225,6 +1225,12 @@
   return true; // has more references
 }
 
+bool refersTo(const RefsRequest ,
+  llvm::function_ref Callback)
+const override {
+  return false;
+}
+
 bool fuzzyFind(
 const FuzzyFindRequest ,
 llvm::function_ref Callback) const override {
@@ -1281,6 +1287,12 @@
   return false;
 }
 
+bool refersTo(const RefsRequest ,
+  llvm::function_ref Callback)
+const override {
+  return false;
+}
+
 bool fuzzyFind(const FuzzyFindRequest &,
llvm::function_ref) const override {
   return false;
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -1345,6 +1345,12 @@
 return false;
   }
 
+  bool
+  refersTo(const RefsRequest &,
+   llvm::function_ref) const override {
+return false;
+  }
+
   void relations(const RelationsRequest &,
  llvm::function_ref)
   const override {}
Index: clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
===
--- clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
+++ clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
@@ -37,17 +37,27 @@
 
 // Helpers for matching call hierarchy data structures.
 MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithDetail, N, "") { return arg.detail == N; }
 MATCHER_P(WithSelectionRange, R, "") { return arg.selectionRange == R; }
 
 template 
 ::testing::Matcher From(ItemMatcher M) {
   return Field(::from, M);
 }
+template 
+::testing::Matcher To(ItemMatcher M) {
+  return Field(::to, M);
+}
 template 
-::testing::Matcher FromRanges(RangeMatchers... M) {
+::testing::Matcher IFromRanges(RangeMatchers... M) {
   return Field(::fromRanges,
UnorderedElementsAre(M...));
 }
+template 
+::testing::Matcher OFromRanges(RangeMatchers... M) {
+  return Field(::fromRanges,
+   UnorderedElementsAre(M...));
+}
 
 TEST(CallHierarchy, IncomingOneFile) {
   Annotations Source(R"cpp(
@@ -72,22 +82,25 @@
   prepareCallHierarchy(AST, Source.point(), testPath(TU.Filename));
   ASSERT_THAT(Items, ElementsAre(WithName("callee")));
   auto IncomingLevel1 = incomingCalls(Items[0], Index.get());
-  ASSERT_THAT(IncomingLevel1,
-  ElementsAre(AllOf(From(WithName("caller1")),
-FromRanges(Source.range("Callee");
+  ASSERT_THAT(
+  IncomingLevel1,
+  ElementsAre(AllOf(From(AllOf(WithName("caller1"), WithDetail("caller1"))),
+IFromRanges(Source.range("Callee");
 
   auto IncomingLevel2 = incomingCalls(IncomingLevel1[0].from, Index.get());
-  ASSERT_THAT(IncomingLevel2,
-  ElementsAre(AllOf(From(WithName("caller2")),
-FromRanges(Source.range("Caller1A"),
-   Source.range("Caller1B"))),
-  AllOf(From(WithName("caller3")),
-FromRanges(Source.range("Caller1C");
+  ASSERT_THAT(
+  IncomingLevel2,
+  ElementsAre(AllOf(From(AllOf(WithName("caller2"), WithDetail("caller2"))),
+IFromRanges(Source.range("Caller1A"),
+Source.range("Caller1B"))),
+  AllOf(From(AllOf(WithName("caller3"), WithDetail("caller3"))),
+IFromRanges(Source.range("Caller1C");
 
   auto IncomingLevel3 = 

[PATCH] D93829: [clangd] Support outgoing calls in call hierarchy

[Quentin Chateau via Phabricator via cfe-commits]

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

The implementation is very close the the incoming
calls implementation. The results of the outgoing
calls are expected to be the exact symmetry of the
incoming calls.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D93829

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/Index.cpp
  clang-tools-extra/clangd/index/Index.h
  clang-tools-extra/clangd/index/MemIndex.cpp
  clang-tools-extra/clangd/index/MemIndex.h
  clang-tools-extra/clangd/index/Merge.cpp
  clang-tools-extra/clangd/index/Merge.h
  clang-tools-extra/clangd/index/ProjectAware.cpp
  clang-tools-extra/clangd/index/dex/Dex.cpp
  clang-tools-extra/clangd/index/dex/Dex.h
  clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1225,6 +1225,12 @@
   return true; // has more references
 }
 
+bool refersTo(const RefsRequest ,
+  llvm::function_ref Callback)
+const override {
+  return false;
+}
+
 bool fuzzyFind(
 const FuzzyFindRequest ,
 llvm::function_ref Callback) const override {
@@ -1281,6 +1287,12 @@
   return false;
 }
 
+bool refersTo(const RefsRequest ,
+  llvm::function_ref Callback)
+const override {
+  return false;
+}
+
 bool fuzzyFind(const FuzzyFindRequest &,
llvm::function_ref) const override {
   return false;
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -1345,6 +1345,12 @@
 return false;
   }
 
+  bool
+  refersTo(const RefsRequest &,
+   llvm::function_ref) const override {
+return false;
+  }
+
   void relations(const RelationsRequest &,
  llvm::function_ref)
   const override {}
Index: clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
===
--- clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
+++ clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
@@ -37,17 +37,27 @@
 
 // Helpers for matching call hierarchy data structures.
 MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithDetail, N, "") { return arg.detail == N; }
 MATCHER_P(WithSelectionRange, R, "") { return arg.selectionRange == R; }
 
 template 
 ::testing::Matcher From(ItemMatcher M) {
   return Field(::from, M);
 }
+template 
+::testing::Matcher To(ItemMatcher M) {
+  return Field(::to, M);
+}
 template 
-::testing::Matcher FromRanges(RangeMatchers... M) {
+::testing::Matcher IFromRanges(RangeMatchers... M) {
   return Field(::fromRanges,
UnorderedElementsAre(M...));
 }
+template 
+::testing::Matcher OFromRanges(RangeMatchers... M) {
+  return Field(::fromRanges,
+   UnorderedElementsAre(M...));
+}
 
 TEST(CallHierarchy, IncomingOneFile) {
   Annotations Source(R"cpp(
@@ -72,22 +82,25 @@
   prepareCallHierarchy(AST, Source.point(), testPath(TU.Filename));
   ASSERT_THAT(Items, ElementsAre(WithName("callee")));
   auto IncomingLevel1 = incomingCalls(Items[0], Index.get());
-  ASSERT_THAT(IncomingLevel1,
-  ElementsAre(AllOf(From(WithName("caller1")),
-FromRanges(Source.range("Callee");
+  ASSERT_THAT(
+  IncomingLevel1,
+  ElementsAre(AllOf(From(AllOf(WithName("caller1"), WithDetail("caller1"))),
+IFromRanges(Source.range("Callee");
 
   auto IncomingLevel2 = incomingCalls(IncomingLevel1[0].from, Index.get());
-  ASSERT_THAT(IncomingLevel2,
-  ElementsAre(AllOf(From(WithName("caller2")),
-FromRanges(Source.range("Caller1A"),
-   Source.range("Caller1B"))),
-  AllOf(From(WithName("caller3")),
-FromRanges(Source.range("Caller1C");
+  ASSERT_THAT(
+  IncomingLevel2,
+  ElementsAre(AllOf(From(AllOf(WithName("caller2"), WithDetail("caller2"))),
+IFromRanges(Source.range("Caller1A"),
+   

[PATCH] D93726: [clangd] Use atomics instead of locks to track periodic memory trimming

[Quentin Chateau via Phabricator via cfe-commits]

qchateau accepted this revision.
qchateau added a comment.
This revision is now accepted and ready to land.

LGTM

Small nits:

- `operator()` is not `const` but `Next` is `mutable`, seems to me you intended 
to have `operator()` as `const`
- memory orders for the atomic operations can probably be downgraded to 
`std::memory_order_acquire`/`std::memory_order_acq_rel`. I think the first load 
can even be `relaxed` but that I'm always careful with these


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93726

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

[PATCH] D93452: [clangd] Trim memory periodically

[Quentin Chateau via Phabricator via cfe-commits]

qchateau updated this revision to Diff 313215.
qchateau marked 3 inline comments as done.
qchateau added a comment.

The log is back, I renamed the CMake option and the command line option and 
reduced the number of preprocessor directives.
I chose not to modify the files for the `gn` build system, I'd rather not break 
it.

I did not use the CMke option as the default value for the command line option: 
IMO this CMake option is only useful if you encounter build problems related to 
malloc_trim, so malloc_trim must not be part of the code when you disable it 
through CMake.

If this all makes sense and I did not make a mistake in this update, you can 
land this. Let me know if there are other small details you'd like me to change.

Email: quentin.chat...@gmail.com


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/Features.inc.in
  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
@@ -50,6 +50,10 @@
 #include 
 #endif
 
+#ifdef __GLIBC__
+#include 
+#endif
+
 namespace clang {
 namespace clangd {
 
@@ -497,6 +501,29 @@
 init(ClangdServer::Options().CollectMainFileRefs),
 };
 
+#if defined(__GLIBC__) && CLANGD_MALLOC_TRIM
+opt EnableMallocTrim{
+"malloc-trim",
+cat(Misc),
+desc("Release memory periodically via malloc_trim(3)."),
+init(true),
+};
+
+std::function getMemoryCleanupFunction() {
+  if (!EnableMallocTrim)
+return nullptr;
+  // Leave a few MB at the top of the heap: it is insignificant
+  // and will most likely be needed by the main thread
+  constexpr size_t MallocTrimPad = 20'000'000;
+  return []() {
+if (malloc_trim(MallocTrimPad))
+  vlog("Released memory via malloc_trim");
+  };
+}
+#else
+std::function getMemoryCleanupFunction() { return nullptr; }
+#endif
+
 #if CLANGD_ENABLE_REMOTE
 opt RemoteIndexAddress{
 "remote-index-address",
@@ -797,6 +824,7 @@
   Opts.BuildRecoveryAST = RecoveryAST;
   Opts.PreserveRecoveryASTType = RecoveryASTType;
   Opts.FoldingRanges = FoldingRanges;
+  Opts.MemoryCleanup = getMemoryCleanupFunction();
 
   Opts.CodeComplete.IncludeIneligibleResults = IncludeIneligibleResults;
   Opts.CodeComplete.Limit = LimitResults;
Index: clang-tools-extra/clangd/Features.inc.in
===
--- clang-tools-extra/clangd/Features.inc.in
+++ clang-tools-extra/clangd/Features.inc.in
@@ -1,2 +1,3 @@
 #define CLANGD_BUILD_XPC @CLANGD_BUILD_XPC@
 #define CLANGD_ENABLE_REMOTE @CLANGD_ENABLE_REMOTE@
+#define CLANGD_MALLOC_TRIM @CLANGD_MALLOC_TRIM@
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -48,6 +48,9 @@
 llvm::Optional CompileCommandsDir;
 /// The offset-encoding to use, or None to negotiate it over LSP.
 llvm::Optional Encoding;
+/// If set, periodically called to release memory.
+/// Consider malloc_trim(3)
+std::function MemoryCleanup = nullptr;
 
 /// Per-feature options. Generally ClangdServer lets these vary
 /// per-request, but LSP allows limited/no customizations.
@@ -184,10 +187,18 @@
   /// profiling hasn't happened recently.
   void maybeExportMemoryProfile();
 
+  /// Run the MemoryCleanup callback if it's time.
+  /// This method is thread safe.
+  void maybeCleanupMemory();
+
   /// Timepoint until which profiling is off. It is used to throttle profiling
   /// requests.
   std::chrono::steady_clock::time_point NextProfileTime;
 
+  /// Next time we want to call the MemoryCleanup callback.
+  std::mutex NextMemoryCleanupTimeMutex;
+  std::chrono::steady_clock::time_point NextMemoryCleanupTime;
+
   /// Since initialization of CDBs and ClangdServer is done lazily, the
   /// following context captures the one used while creating ClangdLSPServer and
   /// passes it to above mentioned object instances to make sure they share the
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -178,6 +178,7 @@
 } else if (auto Handler = Notifications.lookup(Method)) {
   Handler(std::move(Params));
   Server.maybeExportMemoryProfile();
+  Server.maybeCleanupMemory();
 } else {
   log("unhandled notification {0}", Method);
 }
@@ -453,6 +454,7 @@
 
 void ClangdLSPServer::notify(llvm::StringRef Method, llvm::json::Value Params) {
   

[PATCH] D93546: [clangd][NFC] Improve clangd status messages

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

Sure !
Email: quentin.chat...@gmail.com

I guess I'll need to ask for commit access


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93546

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

[PATCH] D93546: [clangd][NFC] Improve clangd status messages

[Quentin Chateau via Phabricator via cfe-commits]

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

- dont capitalize first letter of status


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93546

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/TUScheduler.cpp


Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -1206,7 +1206,7 @@
   }
   if (Result.empty())
 return "idle";
-  return llvm::join(Result, ",");
+  return llvm::join(Result, ", ");
 }
 
 } // namespace
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -621,7 +621,7 @@
 File));
   };
 
-  WorkScheduler.runWithAST("Type Hierarchy", File, std::move(Action));
+  WorkScheduler.runWithAST("TypeHierarchy", File, std::move(Action));
 }
 
 void ClangdServer::resolveTypeHierarchy(
@@ -642,7 +642,7 @@
   return CB(InpAST.takeError());
 CB(clangd::prepareCallHierarchy(InpAST->AST, Pos, File));
   };
-  WorkScheduler.runWithAST("Call Hierarchy", File, std::move(Action));
+  WorkScheduler.runWithAST("CallHierarchy", File, std::move(Action));
 }
 
 void ClangdServer::incomingCalls(
@@ -678,7 +678,7 @@
   return CB(InpAST.takeError());
 CB(clangd::getDocumentSymbols(InpAST->AST));
   };
-  WorkScheduler.runWithAST("documentSymbols", File, std::move(Action),
+  WorkScheduler.runWithAST("DocumentSymbols", File, std::move(Action),
TUScheduler::InvalidateOnUpdate);
 }
 
@@ -690,7 +690,7 @@
   return CB(InpAST.takeError());
 CB(clangd::getFoldingRanges(InpAST->AST));
   };
-  WorkScheduler.runWithAST("foldingRanges", File, std::move(Action),
+  WorkScheduler.runWithAST("FoldingRanges", File, std::move(Action),
TUScheduler::InvalidateOnUpdate);
 }
 


Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -1206,7 +1206,7 @@
   }
   if (Result.empty())
 return "idle";
-  return llvm::join(Result, ",");
+  return llvm::join(Result, ", ");
 }
 
 } // namespace
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -621,7 +621,7 @@
 File));
   };
 
-  WorkScheduler.runWithAST("Type Hierarchy", File, std::move(Action));
+  WorkScheduler.runWithAST("TypeHierarchy", File, std::move(Action));
 }
 
 void ClangdServer::resolveTypeHierarchy(
@@ -642,7 +642,7 @@
   return CB(InpAST.takeError());
 CB(clangd::prepareCallHierarchy(InpAST->AST, Pos, File));
   };
-  WorkScheduler.runWithAST("Call Hierarchy", File, std::move(Action));
+  WorkScheduler.runWithAST("CallHierarchy", File, std::move(Action));
 }
 
 void ClangdServer::incomingCalls(
@@ -678,7 +678,7 @@
   return CB(InpAST.takeError());
 CB(clangd::getDocumentSymbols(InpAST->AST));
   };
-  WorkScheduler.runWithAST("documentSymbols", File, std::move(Action),
+  WorkScheduler.runWithAST("DocumentSymbols", File, std::move(Action),
TUScheduler::InvalidateOnUpdate);
 }
 
@@ -690,7 +690,7 @@
   return CB(InpAST.takeError());
 CB(clangd::getFoldingRanges(InpAST->AST));
   };
-  WorkScheduler.runWithAST("foldingRanges", File, std::move(Action),
+  WorkScheduler.runWithAST("FoldingRanges", File, std::move(Action),
TUScheduler::InvalidateOnUpdate);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D93452: [clangd] Trim memory periodically

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

Here is the latest version, I believe it should mostly look like what you 
expect. Let me know if there is anything that you'd like me to change, it's 
totally fine to have a few more iterations on this diff.

I added a `CLANGD_ENABLE_MEMORY_CLEANUP` to make sure people can work around 
some weird linking issues with `malloc_trim` if necessary. I've noticed options 
such as `CLANGD_ENABLE_REMOTE` also appear in the files below, but as I have no 
idea what they are for, I did not add `CLANGD_ENABLE_MEMORY_CLEANUP` to them. 
Let me know if I should do anything concerning these files.

- clang-tools-extra/clangd/test/lit.site.cfg.py.in
- llvm/utils/gn/secondary/clang-tools-extra/clangd/BUILD.gn
- llvm/utils/gn/secondary/clang-tools-extra/clangd/test/BUILD.gn


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452

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

[PATCH] D93452: [clangd] Trim memory periodically

[Quentin Chateau via Phabricator via cfe-commits]

qchateau updated this revision to Diff 313000.
qchateau marked 16 inline comments as done.
qchateau added a comment.



- Move platform specific code to ClangdMain
- Generic memory cleanup callback in ClangdLSPServer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/Features.inc.in
  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
@@ -50,6 +50,10 @@
 #include 
 #endif
 
+#ifdef __GLIBC__
+#include 
+#endif
+
 namespace clang {
 namespace clangd {
 
@@ -497,6 +501,15 @@
 init(ClangdServer::Options().CollectMainFileRefs),
 };
 
+#if CLANGD_ENABLE_MEMORY_CLEANUP
+opt EnableMemoryCleanup{
+"memory-cleanup",
+cat(Misc),
+desc("Perform periodic memory cleanup."),
+init(true),
+};
+#endif // CLANGD_ENABLE_MEMORY_CLEANUP
+
 #if CLANGD_ENABLE_REMOTE
 opt RemoteIndexAddress{
 "remote-index-address",
@@ -577,6 +590,20 @@
   }
   llvm_unreachable("Invalid ExternalIndexKind.");
 }
+
+#if CLANGD_ENABLE_MEMORY_CLEANUP
+std::function getMemoryCleanupFunction() {
+#ifdef __GLIBC__
+  // Leave a few MB at the top of the heap: it is insignificant
+  // and will most likely be needed by the main thread
+  constexpr size_t MallocTrimPad = 20'000'000;
+  return []() { malloc_trim(MallocTrimPad); };
+#else
+  return {};
+#endif
+}
+#endif // CLANGD_ENABLE_MEMORY_CLEANUP
+
 } // namespace
 } // namespace clangd
 } // namespace clang
@@ -797,6 +824,10 @@
   Opts.BuildRecoveryAST = RecoveryAST;
   Opts.PreserveRecoveryASTType = RecoveryASTType;
   Opts.FoldingRanges = FoldingRanges;
+#if CLANGD_ENABLE_MEMORY_CLEANUP
+  if (EnableMemoryCleanup)
+Opts.MemoryCleanup = getMemoryCleanupFunction();
+#endif // CLANGD_ENABLE_MEMORY_CLEANUP
 
   Opts.CodeComplete.IncludeIneligibleResults = IncludeIneligibleResults;
   Opts.CodeComplete.Limit = LimitResults;
Index: clang-tools-extra/clangd/Features.inc.in
===
--- clang-tools-extra/clangd/Features.inc.in
+++ clang-tools-extra/clangd/Features.inc.in
@@ -1,2 +1,3 @@
 #define CLANGD_BUILD_XPC @CLANGD_BUILD_XPC@
 #define CLANGD_ENABLE_REMOTE @CLANGD_ENABLE_REMOTE@
+#define CLANGD_ENABLE_MEMORY_CLEANUP @CLANGD_ENABLE_MEMORY_CLEANUP@
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -48,6 +48,9 @@
 llvm::Optional CompileCommandsDir;
 /// The offset-encoding to use, or None to negotiate it over LSP.
 llvm::Optional Encoding;
+/// If set, periodically called to release memory.
+/// Consider malloc_trim(3)
+std::function MemoryCleanup = nullptr;
 
 /// Per-feature options. Generally ClangdServer lets these vary
 /// per-request, but LSP allows limited/no customizations.
@@ -184,10 +187,18 @@
   /// profiling hasn't happened recently.
   void maybeExportMemoryProfile();
 
+  /// Run the MemoryCleanup callback if it's time.
+  /// This method is thread safe.
+  void maybeCleanupMemory();
+
   /// Timepoint until which profiling is off. It is used to throttle profiling
   /// requests.
   std::chrono::steady_clock::time_point NextProfileTime;
 
+  /// Next time we want to call the MemoryCleanup callback.
+  std::mutex NextMemoryCleanupTimeMutex;
+  std::chrono::steady_clock::time_point NextMemoryCleanupTime;
+
   /// Since initialization of CDBs and ClangdServer is done lazily, the
   /// following context captures the one used while creating ClangdLSPServer and
   /// passes it to above mentioned object instances to make sure they share the
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -178,6 +178,7 @@
 } else if (auto Handler = Notifications.lookup(Method)) {
   Handler(std::move(Params));
   Server.maybeExportMemoryProfile();
+  Server.maybeCleanupMemory();
 } else {
   log("unhandled notification {0}", Method);
 }
@@ -453,6 +454,7 @@
 
 void ClangdLSPServer::notify(llvm::StringRef Method, llvm::json::Value Params) {
   log("--> {0}", Method);
+  maybeCleanupMemory();
   std::lock_guard Lock(TranspWriter);
   Transp.notify(Method, std::move(Params));
 }
@@ -1295,6 +1297,27 @@
   NextProfileTime = Now + ProfileInterval;
 }
 
+void ClangdLSPServer::maybeCleanupMemory() {
+  // Memory cleanup is probably expensive, throttle it
+  static constexpr auto 

[PATCH] D93452: [clangd] Trim memory periodically

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

> Sorry to mess you around like this, but I think I might have jumped the gun 
> in asking this to be abstracted away in `Process.h`.

No problem, there is not much code anyway. The thought process behind the 
"right" solution is by far the most important.

> So I think my favorite option may be back to:
>
> - Have ClangdLSPServer::Options take a nullable `std::function` to 
> abstract the (optional) platform-specific operation
> - Have ClangdMain contain platform #ifdefs and define+inject the wrapper that 
> calls `malloc_trim`
> - Optionally: have a clangd or cmake flag to disable this even if glibc was 
> detected. This provides a workaround for other allocators if we need it.

I'll go back to this then, and add all the other changes you suggested

---

In the meantime, I've done some testing with tcmalloc and jemalloc.

`jemalloc` does very good in this situation, the RSS goes down as soon as the 
memory is not required anymore:

- RSS goes down when the file is done parsing
- RSS goes down as soon as I close the file in VSCode

`tcmalloc` does fine, but is not ideal, the RSS does not grow to an 
unreasonable size, but does not go down when we release resources. It basically 
plateaus at the maximum usage (meaning what you use when parsing N files, N 
depending on the parallelism level). I'm not an expert on `tcmalloc` but from 
what I found it's supposed to release resources over time, which I failed to 
observe. `tcmalloc` does have an extension to actively release memory 
(`MallocExtension::instance()->ReleaseFreeMemory()`) which works as expected, 
bringing the RSS down. That being said, I'm not sure we need to do something 
about it, clangd will indeed waste a significant amount of RAM in this case, 
but [1] the usage is still reasonable and bounded and [2] it seems known that 
`tcmalloc` is a greedy allocator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452

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

[PATCH] D93452: [clangd] Trim memory periodically

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

> Interestingly clangd held rock solid at 1.0GB recorded memory usage 
> throughout.

What do you mean exactly ? Using the built in memory usage measurement ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452

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

[PATCH] D93452: [clangd] Trim memory periodically

[Quentin Chateau via Phabricator via cfe-commits]

qchateau planned changes to this revision.
qchateau marked 2 inline comments as done.
qchateau added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:52
+// Malloc trim minimal period
+std::chrono::seconds MemoryCleanupPeriod = std::chrono::seconds(60);
 

sammccall wrote:
> I don't particularly think the interval needs to be set through options 
> (memory profiling interval isn't), we can just hardcode this.
> 
> On the other hand, doing this at all should be optional, and I think we 
> should enable it from ClangdMain - this is a weird thing for a library to be 
> doing by default, and as it stands we'll be doing it in unit tests and stuff.
> 
> I think my favorite variant would be
> ```
> /// If set, periodically called to release memory.
> /// Consider llvm::sys::Process::ReleaseHeapMemory;
> std::function MemoryCleanup = nullptr;
> ```
Sounds good, do you think [1] we should throttle the calls to `MemoryCleanup` 
or [2] throttle the calls to `malloc_trim` from within the callback ?

>I don't particularly think the interval needs to be set through options 
>(memory profiling interval isn't), we can just hardcode this.
>
>On the other hand, doing this at all should be optional, and I think we should 
>enable it from ClangdMain - this is a weird thing for a library to be doing by 
>default, and as it stands we'll be doing it in unit tests and stuff.

I like the idea of configuring the `MemoryCleanup` function from ClangdMain, 
but it's as easy to have [3] an int option that configures the interval as it 
is to have [4] a boolean one to just disable it.

I actually think both points are related:
- If we choose [2], then I'd go for [3] because it is more configurable and 
adds no code complexity.
- If we choose [1], then I agree we can go for [4] to avoid an extra option



Comment at: llvm/lib/Support/Unix/Process.inc:122
+#if defined(HAVE_MALLOC_TRIM)
+  return malloc_trim(Pad);
+#else

sammccall wrote:
> MaskRay wrote:
> > If the user uses jemalloc/tcmalloc with glibc, malloc_trim may exist while 
> > the function may do nothing.
> True. Unless I'm missing something, it won't be completely free - it'll still 
> acquire glibc-malloc locks and walk over whatever arenas exist.
> 
> @MaskRay is there anything sensible to do about this? We can introduce a 
> cmake variable or something you're supposed to set if you're using another 
> allocator, but:
>  - that seems fragile
>  - fundamentally doesn't support switching allocators with LD_PRELOAD, I guess
> 
I'm not worried about LD_PRELOAD, worst case it adds a little bit of extra 
runtime cost (calling malloc_trim although it's useless because we preloaded 
tcmalloc).

But I agree there is an issue here: we detect which cleanup function to use 
depending on the headers (in cmake), but it should actually depend on what we 
link. Not sure if this can be done properly :/
For now we only want to cleanup on glibc malloc, but I'll investigate with 
tcmalloc and jemalloc, maybe they do not work so well either after all. Then 
the problem would be even worse: I'd be tempted to call a similar function for 
these libraries as well, and although the developer may have tcmalloc in its 
system include, it does not mean he is linking it -> undefined symbol

Overall this approach of calling `malloc_trim` is looking grim to me because it 
depends on what's linked.

Either we can continue in this way, or try to find another way around this 
issue:
- Encourage the developer to link another malloc implementation, but then it's 
only useful if the various distributions do it
- Continue to dig on why glibc malloc performs so bad with clangd and try to 
improve things in clangd code
- Something else ?




Comment at: llvm/lib/Support/Unix/Process.inc:124
+#else
+#warning Cannot get malloc info on this platform
+  return false;

sammccall wrote:
> this is expected, we don't want a compiler warning here
Same for windows I guess ? If I understand correctly your logic is "if it's not 
available that's because we don't need it: don't warn"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452

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

[PATCH] D93452: [clangd] Trim memory periodically

[Quentin Chateau via Phabricator via cfe-commits]

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

- Fix clang-tidy errors


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/index/FileIndex.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  llvm/cmake/config-ix.cmake
  llvm/include/llvm/Config/config.h.cmake
  llvm/include/llvm/Support/Process.h
  llvm/lib/Support/Unix/Process.inc
  llvm/lib/Support/Windows/Process.inc

Index: llvm/lib/Support/Windows/Process.inc
===
--- llvm/lib/Support/Windows/Process.inc
+++ llvm/lib/Support/Windows/Process.inc
@@ -81,6 +81,11 @@
   return size;
 }
 
+bool Process::mallocTrim(size_t Pad) {
+#warning Cannot get malloc info on this platform
+  return false;
+}
+
 void Process::GetTimeUsage(TimePoint<> , std::chrono::nanoseconds _time,
std::chrono::nanoseconds _time) {
   elapsed = std::chrono::system_clock::now();;
Index: llvm/lib/Support/Unix/Process.inc
===
--- llvm/lib/Support/Unix/Process.inc
+++ llvm/lib/Support/Unix/Process.inc
@@ -31,7 +31,7 @@
 #if HAVE_SIGNAL_H
 #include 
 #endif
-#if defined(HAVE_MALLINFO)
+#if defined(HAVE_MALLINFO) || defined(HAVE_MALLOC_TRIM)
 #include 
 #endif
 #if defined(HAVE_MALLCTL)
@@ -117,6 +117,15 @@
 #endif
 }
 
+bool Process::mallocTrim(size_t Pad) {
+#if defined(HAVE_MALLOC_TRIM)
+  return malloc_trim(Pad);
+#else
+#warning Cannot get malloc info on this platform
+  return false;
+#endif
+}
+
 void Process::GetTimeUsage(TimePoint<> , std::chrono::nanoseconds _time,
std::chrono::nanoseconds _time) {
   elapsed = std::chrono::system_clock::now();
Index: llvm/include/llvm/Support/Process.h
===
--- llvm/include/llvm/Support/Process.h
+++ llvm/include/llvm/Support/Process.h
@@ -75,6 +75,12 @@
   /// allocated space.
   static size_t GetMallocUsage();
 
+  /// Attempts to free unused memory from the heap.
+  /// This function is basically a call to malloc_trim(3).
+  /// \param Pad Free space to leave at the top of the heap
+  /// \returns true if memory was actually released
+  static bool mallocTrim(size_t Pad);
+
   /// This static function will set \p user_time to the amount of CPU time
   /// spent in user (non-kernel) mode and \p sys_time to the amount of CPU
   /// time spent in system (kernel) mode.  If the operating system does not
Index: llvm/include/llvm/Config/config.h.cmake
===
--- llvm/include/llvm/Config/config.h.cmake
+++ llvm/include/llvm/Config/config.h.cmake
@@ -136,6 +136,9 @@
 /* Define to 1 if you have the `mallinfo' function. */
 #cmakedefine HAVE_MALLINFO ${HAVE_MALLINFO}
 
+/* Define to 1 if you have the `mallinfo' function. */
+#cmakedefine HAVE_MALLOC_TRIM ${HAVE_MALLOC_TRIM}
+
 /* Define to 1 if you have the  header file. */
 #cmakedefine HAVE_MALLOC_MALLOC_H ${HAVE_MALLOC_MALLOC_H}
 
Index: llvm/cmake/config-ix.cmake
===
--- llvm/cmake/config-ix.cmake
+++ llvm/cmake/config-ix.cmake
@@ -232,6 +232,7 @@
 set(CMAKE_REQUIRED_DEFINITIONS "")
 check_symbol_exists(mallctl malloc_np.h HAVE_MALLCTL)
 check_symbol_exists(mallinfo malloc.h HAVE_MALLINFO)
+check_symbol_exists(malloc_trim malloc.h HAVE_MALLOC_TRIM)
 check_symbol_exists(malloc_zone_statistics malloc/malloc.h
 HAVE_MALLOC_ZONE_STATISTICS)
 check_symbol_exists(getrlimit "sys/types.h;sys/time.h;sys/resource.h" HAVE_GETRLIMIT)
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -497,6 +497,14 @@
 init(ClangdServer::Options().CollectMainFileRefs),
 };
 
+opt MemoryCleanupPeriod{
+"memory-cleanup-period",
+cat(Misc),
+desc("Period at which clangd will actively try to perform a "
+ "memory cleanup (in seconds)."),
+init(ClangdLSPServer::Options().MemoryCleanupPeriod.count()),
+};
+
 #if CLANGD_ENABLE_REMOTE
 opt RemoteIndexAddress{
 "remote-index-address",
@@ -797,6 +805,7 @@
   Opts.BuildRecoveryAST = RecoveryAST;
   Opts.PreserveRecoveryASTType = RecoveryASTType;
   Opts.FoldingRanges = FoldingRanges;
+  Opts.MemoryCleanupPeriod = std::chrono::seconds(MemoryCleanupPeriod);
 
   Opts.CodeComplete.IncludeIneligibleResults = IncludeIneligibleResults;
   Opts.CodeComplete.Limit = LimitResults;
Index: clang-tools-extra/clangd/index/FileIndex.cpp
===
--- clang-tools-extra/clangd/index/FileIndex.cpp
+++ 

[PATCH] D93553: [clangd] Make our printing policies for Hover more consistent, especially tags

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

Looks good to me, it removes a lot of duplication and corner cases.

I'd have printed the tag for reference and pointers as well (`class Foo*` 
instead of `Foo*`). I don't think `Foo*` is much more understandable than 
`Foo`, so we decide that printing `class Foo` is easier to understand for the 
user, we should print `class Foo*` as well. 
But I agree it's even less idiomatic, so I won't argue with your choice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93553

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

[PATCH] D93546: [clangd][NFC] Improve clangd status messages

[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.

clangd actions have various naming schemes, the most
common being PascalCase. This commit applies PascalCase
to all clangd actions, and fix the status rendering
in `renderTUAction` to look more consistent.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D93546

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/TUScheduler.cpp


Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -1184,7 +1184,7 @@
   llvm::SmallVector Result;
   switch (PA) {
   case PreambleAction::Building:
-Result.push_back("parsing includes");
+Result.push_back("Parsing includes");
 break;
   case PreambleAction::Idle:
 // We handle idle specially below.
@@ -1192,13 +1192,13 @@
   }
   switch (AA.K) {
   case ASTAction::Queued:
-Result.push_back("file is queued");
+Result.push_back("File is queued");
 break;
   case ASTAction::RunningAction:
-Result.push_back("running " + AA.Name);
+Result.push_back("Running " + AA.Name);
 break;
   case ASTAction::Building:
-Result.push_back("parsing main file");
+Result.push_back("Parsing main file");
 break;
   case ASTAction::Idle:
 // We handle idle specially below.
@@ -1206,7 +1206,7 @@
   }
   if (Result.empty())
 return "idle";
-  return llvm::join(Result, ",");
+  return llvm::join(Result, ", ");
 }
 
 } // namespace
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -621,7 +621,7 @@
 File));
   };
 
-  WorkScheduler.runWithAST("Type Hierarchy", File, std::move(Action));
+  WorkScheduler.runWithAST("TypeHierarchy", File, std::move(Action));
 }
 
 void ClangdServer::resolveTypeHierarchy(
@@ -642,7 +642,7 @@
   return CB(InpAST.takeError());
 CB(clangd::prepareCallHierarchy(InpAST->AST, Pos, File));
   };
-  WorkScheduler.runWithAST("Call Hierarchy", File, std::move(Action));
+  WorkScheduler.runWithAST("CallHierarchy", File, std::move(Action));
 }
 
 void ClangdServer::incomingCalls(
@@ -678,7 +678,7 @@
   return CB(InpAST.takeError());
 CB(clangd::getDocumentSymbols(InpAST->AST));
   };
-  WorkScheduler.runWithAST("documentSymbols", File, std::move(Action),
+  WorkScheduler.runWithAST("DocumentSymbols", File, std::move(Action),
TUScheduler::InvalidateOnUpdate);
 }
 
@@ -690,7 +690,7 @@
   return CB(InpAST.takeError());
 CB(clangd::getFoldingRanges(InpAST->AST));
   };
-  WorkScheduler.runWithAST("foldingRanges", File, std::move(Action),
+  WorkScheduler.runWithAST("FoldingRanges", File, std::move(Action),
TUScheduler::InvalidateOnUpdate);
 }
 


Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -1184,7 +1184,7 @@
   llvm::SmallVector Result;
   switch (PA) {
   case PreambleAction::Building:
-Result.push_back("parsing includes");
+Result.push_back("Parsing includes");
 break;
   case PreambleAction::Idle:
 // We handle idle specially below.
@@ -1192,13 +1192,13 @@
   }
   switch (AA.K) {
   case ASTAction::Queued:
-Result.push_back("file is queued");
+Result.push_back("File is queued");
 break;
   case ASTAction::RunningAction:
-Result.push_back("running " + AA.Name);
+Result.push_back("Running " + AA.Name);
 break;
   case ASTAction::Building:
-Result.push_back("parsing main file");
+Result.push_back("Parsing main file");
 break;
   case ASTAction::Idle:
 // We handle idle specially below.
@@ -1206,7 +1206,7 @@
   }
   if (Result.empty())
 return "idle";
-  return llvm::join(Result, ",");
+  return llvm::join(Result, ", ");
 }
 
 } // namespace
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -621,7 +621,7 @@
 File));
   };
 
-  WorkScheduler.runWithAST("Type Hierarchy", File, std::move(Action));
+  WorkScheduler.runWithAST("TypeHierarchy", File, std::move(Action));
 }
 
 void ClangdServer::resolveTypeHierarchy(
@@ -642,7 +642,7 @@
   return CB(InpAST.takeError());
 CB(clangd::prepareCallHierarchy(InpAST->AST, Pos, File));
   };
-  

[PATCH] D93227: [clangd] Smarter hover on auto and decltype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:353
 if (auto *AT = D->getType()->getContainedAutoType()) {
-  if (!AT->getDeducedType().isNull())
-DeducedType = AT->getDeducedType();
+  DeducedType = AT->desugar();
 }

sammccall wrote:
> I added a comment to getDeducedType() - I think the intent here is that we 
> return the AutoType itself if it's not deduced, right?
It is already the case for return types in `VisitFunctionDecl`, but not for 
types visited in in `VisitDeclaratorDecl`. I simply made it consistent. One 
could argue `getDeducedType` should return `None` when the type is not deduced, 
and I'd not argue against that. Though I have t admit this behavior makes 
things simpler for the hover feature.



Comment at: clang-tools-extra/clangd/Hover.cpp:591
+   ASTContext ,
+   const SymbolIndex *Index) {
+  QualType OriginThisType = CTE->getType()->getPointeeType();

sammccall wrote:
> I dropped this unused param
Good catch



Comment at: clang-tools-extra/clangd/Hover.cpp:618
+  } else {
+CXXRecordDecl *D = QT->getAsCXXRecordDecl();
+if (D && D->isLambda())

sammccall wrote:
> sammccall wrote:
> > qchateau wrote:
> > > sammccall wrote:
> > > > You've rewritten this logic compared to the old 
> > > > `getHoverContents(QualType)`, and I think there are regressions:
> > > >  - We've dropped class documentation (see e.g. 
> > > > ac3f9e48421712168884d59cbfe8b294dd76a19b, this is visible in the tests)
> > > >  - we're no longer using printName() to print tag-decls, which I expect 
> > > > changes e.g. the printing of anyonymous structs which is special-cased 
> > > > in that function (we have tests for this but probably not in 
> > > > combination with auto)
> > > >  - we're no longer using the printing-policy to print non-tag types, 
> > > > which will lead to some random changes
> > > > 
> > > > I don't see a reason for these changes, can we revert them?
> > > - I can re-add class documentation, but should it work when `auto` is a 
> > > pointer or a ref ? In that case, I'll need something like your 
> > > `unwrapType` of https://reviews.llvm.org/D93314
> > > - `printName` will print `C` instead of `class C` even if I hack around 
> > > and give it the right `PrintingPolicy`. The problem is that IDE (at least 
> > > VSCode) does not know what `C` is and cannot color it, which looks a nice 
> > > less nice. Up to you !
> > > - I can re-add the printing policy for non-tag types, and add it for 
> > > tag-types if we chose not to use `printName`.
> > > should it work when auto is a pointer or a ref?
> > 
> > I think no need for now, I just want to avoid the regression.
> > (Nice to have, but at least in the codebases I work in, `auto` is much more 
> > common than `decltype(auto)` so it's rarely a reference, and pointers are 
> > idiomatically written `auto*`)
> > 
> > > printName will print C instead of class C even if I hack around and give 
> > > it the right PrintingPolicy.
> > 
> > Yeah... it's not supposed to, though the extra clarity is valuable here. 
> > You could just hack around this and prepend the TagTypeKind yourself :-)
> Hmm, it looks like this wasn't done. It's now printing e.g. `class 
> vector` which seems more confusing than either `class vector` 
> or `vector` (even if we lose some highlighting). I'll send a followup 
> with proposed changes here.
We can probably use `printDefinition` for tag types and keep `getAsString` for 
non tag types. That would also make the definition close to what we get when 
hovering on an explicit type (as opposed to 'auto'). We'd probably need to set 
the scopes in `HoverInfo` as well, `printDefinition` does not print them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93227

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

[PATCH] D93452: [clangd] Trim memory after buildINdex

[Quentin Chateau via Phabricator via cfe-commits]

qchateau updated this revision to Diff 312781.
qchateau added a comment.
Herald added subscribers: llvm-commits, dexonsmith, hiraditya.
Herald added a project: LLVM.

- Add MallocTrim to llvm::sys::Process
- Implement malloc trimming in ClangdLSPServer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/index/FileIndex.cpp
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  llvm/cmake/config-ix.cmake
  llvm/include/llvm/Config/config.h.cmake
  llvm/include/llvm/Support/Process.h
  llvm/lib/Support/Unix/Process.inc
  llvm/lib/Support/Windows/Process.inc

Index: llvm/lib/Support/Windows/Process.inc
===
--- llvm/lib/Support/Windows/Process.inc
+++ llvm/lib/Support/Windows/Process.inc
@@ -81,6 +81,11 @@
   return size;
 }
 
+bool Process::MallocTrim(size_t Pad) {
+#warning Cannot get malloc info on this platform
+  return false;
+}
+
 void Process::GetTimeUsage(TimePoint<> , std::chrono::nanoseconds _time,
std::chrono::nanoseconds _time) {
   elapsed = std::chrono::system_clock::now();;
Index: llvm/lib/Support/Unix/Process.inc
===
--- llvm/lib/Support/Unix/Process.inc
+++ llvm/lib/Support/Unix/Process.inc
@@ -31,7 +31,7 @@
 #if HAVE_SIGNAL_H
 #include 
 #endif
-#if defined(HAVE_MALLINFO)
+#if defined(HAVE_MALLINFO) || defined(HAVE_MALLOC_TRIM)
 #include 
 #endif
 #if defined(HAVE_MALLCTL)
@@ -117,6 +117,15 @@
 #endif
 }
 
+bool Process::MallocTrim(size_t Pad) {
+#if defined(HAVE_MALLOC_TRIM)
+  return malloc_trim(Pad);
+#else
+#warning Cannot get malloc info on this platform
+  return false;
+#endif
+}
+
 void Process::GetTimeUsage(TimePoint<> , std::chrono::nanoseconds _time,
std::chrono::nanoseconds _time) {
   elapsed = std::chrono::system_clock::now();
Index: llvm/include/llvm/Support/Process.h
===
--- llvm/include/llvm/Support/Process.h
+++ llvm/include/llvm/Support/Process.h
@@ -75,6 +75,12 @@
   /// allocated space.
   static size_t GetMallocUsage();
 
+  /// Attempts to free unused memory from the heap.
+  /// This function is basically a call to malloc_trim(3).
+  /// \param Pad Free space to leave at the top of the heap
+  /// \returns true if memory was actually released
+  static bool MallocTrim(size_t Pad);
+
   /// This static function will set \p user_time to the amount of CPU time
   /// spent in user (non-kernel) mode and \p sys_time to the amount of CPU
   /// time spent in system (kernel) mode.  If the operating system does not
Index: llvm/include/llvm/Config/config.h.cmake
===
--- llvm/include/llvm/Config/config.h.cmake
+++ llvm/include/llvm/Config/config.h.cmake
@@ -136,6 +136,9 @@
 /* Define to 1 if you have the `mallinfo' function. */
 #cmakedefine HAVE_MALLINFO ${HAVE_MALLINFO}
 
+/* Define to 1 if you have the `mallinfo' function. */
+#cmakedefine HAVE_MALLOC_TRIM ${HAVE_MALLOC_TRIM}
+
 /* Define to 1 if you have the  header file. */
 #cmakedefine HAVE_MALLOC_MALLOC_H ${HAVE_MALLOC_MALLOC_H}
 
Index: llvm/cmake/config-ix.cmake
===
--- llvm/cmake/config-ix.cmake
+++ llvm/cmake/config-ix.cmake
@@ -232,6 +232,7 @@
 set(CMAKE_REQUIRED_DEFINITIONS "")
 check_symbol_exists(mallctl malloc_np.h HAVE_MALLCTL)
 check_symbol_exists(mallinfo malloc.h HAVE_MALLINFO)
+check_symbol_exists(malloc_trim malloc.h HAVE_MALLOC_TRIM)
 check_symbol_exists(malloc_zone_statistics malloc/malloc.h
 HAVE_MALLOC_ZONE_STATISTICS)
 check_symbol_exists(getrlimit "sys/types.h;sys/time.h;sys/resource.h" HAVE_GETRLIMIT)
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -497,6 +497,14 @@
 init(ClangdServer::Options().CollectMainFileRefs),
 };
 
+opt MemoryCleanupPeriod{
+"memory-cleanup-period",
+cat(Misc),
+desc("Period at which clangd will actively try to perform a "
+ "memory cleanup (in seconds)."),
+init(ClangdLSPServer::Options().MemoryCleanupPeriod.count()),
+};
+
 #if CLANGD_ENABLE_REMOTE
 opt RemoteIndexAddress{
 "remote-index-address",
@@ -797,6 +805,7 @@
   Opts.BuildRecoveryAST = RecoveryAST;
   Opts.PreserveRecoveryASTType = RecoveryASTType;
   Opts.FoldingRanges = FoldingRanges;
+  Opts.MemoryCleanupPeriod = std::chrono::seconds(MemoryCleanupPeriod);
 
   Opts.CodeComplete.IncludeIneligibleResults = IncludeIneligibleResults;
   Opts.CodeComplete.Limit = LimitResults;
Index: 

[PATCH] D93227: [clangd] Smarter hover on auto and decltype

[Quentin Chateau via Phabricator via cfe-commits]

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

Fix ExpandAutoType to not expand undeduced auto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93227

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -379,6 +379,30 @@
  HI.Definition = "class X {}";
}},
 
+  // auto on structured bindings
+  {R"cpp(
+void foo() {
+  struct S { int x; float y; };
+  [[au^to]] [x, y] = S();
+}
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "struct S";
+   }},
+  // undeduced auto
+  {R"cpp(
+template
+void foo() {
+  [[au^to]] x = T{};
+}
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "/* not deduced */";
+   }},
   // auto on lambda
   {R"cpp(
 void foo() {
@@ -386,8 +410,9 @@
 }
 )cpp",
[](HoverInfo ) {
- HI.Name = "(lambda)";
- HI.Kind = index::SymbolKind::Class;
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "class(lambda)";
}},
   // auto on template instantiation
   {R"cpp(
@@ -397,8 +422,9 @@
 }
 )cpp",
[](HoverInfo ) {
- HI.Name = "Foo";
- HI.Kind = index::SymbolKind::Class;
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "class Foo";
}},
   // auto on specialized template
   {R"cpp(
@@ -409,8 +435,9 @@
 }
 )cpp",
[](HoverInfo ) {
- HI.Name = "Foo";
- HI.Kind = index::SymbolKind::Class;
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "class Foo";
}},
 
   // macro
@@ -582,8 +609,9 @@
   }
   )cpp",
   [](HoverInfo ) {
-HI.Name = "Foo";
-HI.Kind = index::SymbolKind::Class;
+HI.Name = "auto";
+HI.Kind = index::SymbolKind::TypeAlias;
+HI.Definition = "class Foo";
   }},
   {// Falls back to primary template, when the type is not instantiated.
R"cpp(
@@ -955,20 +983,11 @@
   llvm::StringRef Tests[] = {
   "^int main() {}",
   "void foo() {^}",
-  R"cpp(// structured binding. Not supported yet
-struct Bar {};
-void foo() {
-  Bar a[2];
-  ^auto [x,y] = a;
-}
-  )cpp",
-  R"cpp(// Template auto parameter. Nothing (Not useful).
-template
-void func() {
-}
-void foo() {
-   func<1>();
-}
+  // FIXME: "decltype(auto)" should be a single hover
+  "decltype(au^to) x = 0;",
+  // FIXME: not supported yet
+  R"cpp(// Lambda auto parameter
+auto lamb = [](a^uto){};
   )cpp",
   R"cpp(// non-named decls don't get hover. Don't crash!
 ^static_assert(1, "");
@@ -1545,9 +1564,9 @@
 }
   )cpp",
   [](HoverInfo ) {
-HI.Name = "int";
-// FIXME: Should be Builtin/Integral.
-HI.Kind = index::SymbolKind::Unknown;
+HI.Name = "auto";
+HI.Kind = index::SymbolKind::TypeAlias;
+HI.Definition = "int";
   }},
   {
   R"cpp(// Simple initialization with const auto
@@ -1555,14 +1574,22 @@
   const ^[[auto]] i = 1;
 }
   )cpp",
-  [](HoverInfo ) { HI.Name = "int"; }},
+  [](HoverInfo ) {
+HI.Name = "auto";
+HI.Kind = index::SymbolKind::TypeAlias;
+HI.Definition = "int";
+  }},
   {
   R"cpp(// Simple initialization with const auto&
 void foo() {
   const ^[[auto]]& i = 1;
 }
   )cpp",
-  [](HoverInfo ) { HI.Name = "int"; }},
+  [](HoverInfo ) {
+HI.Name = "auto";
+HI.Kind = index::SymbolKind::TypeAlias;
+HI.Definition = "int";
+  }},
   {
   R"cpp(// Simple initialization with auto&
 void foo() {
@@ -1570,7 +1597,11 @@
   ^[[auto]]& i = x;
 }
   )cpp",
-  [](HoverInfo ) { HI.Name = "int"; }},
+  [](HoverInfo ) {
+HI.Name 

[PATCH] D93227: [clangd] Smarter hover on auto and decltype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

You can land this if it is still fine after my final update.

email: quentin.chat...@gmail.com

Thanks !




Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2452
 // In namespace ns
 ret_type foo(params) {})",
   },

I've also changed this raw string to a normal string (and another one line 
2729) because they include whitespace at the end of the line. Git complains 
about it and my editor automatically trims trailing whitespace. I assume I'm 
not the only one using this setting and it annoyed me more than it should.



Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:999
+  "decltype(au^to) x = 0;",
+  R"cpp(// Lambda auto parameter. Nothing (Not useful).
+auto lamb = [](a^uto){};

sammccall wrote:
> qchateau wrote:
> > sammccall wrote:
> > > (not convinced this is fundamentally not useful - the fact that it's a 
> > > template parameter means it's probably worth having a hover card for it 
> > > at some point. But I agree with suppressing it for now)
> > As a user I'd prefer the hover to work over the whole `decltype(auto)` 
> > expression. But that does not seem quite compatible with the way tokens are 
> > parsed.
> > 
> > Are you suggesting I remove the test case or should I add a `FIXME` comment 
> > ?
> Sorry, I think we're talking about different examples.
> 
> (I agree decltype(auto) should ideally be a single thing and we should 
> support hover on all of it, no need to address in this patch, FIXME would be 
> nice).
> 
> But I was talking about the lambda auto parameter, where your comment says 
> "not useful". Anyway, nothing to do here either except maybe soften the 
> comment to "not supported at the moment".
Ahah yes my bad, I copy pasted the comment x)
I fixed it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93227

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

[PATCH] D93227: [clangd] Smarter hover on auto and decltype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau updated this revision to Diff 312747.
qchateau marked 5 inline comments as done.
qchateau added a comment.

- Verify documentation in hover test on 'auto'
- Fixed comments
- Converted raw string to string to avoid trailing whitespace


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93227

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -379,6 +379,30 @@
  HI.Definition = "class X {}";
}},
 
+  // auto on structured bindings
+  {R"cpp(
+void foo() {
+  struct S { int x; float y; };
+  [[au^to]] [x, y] = S();
+}
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "struct S";
+   }},
+  // undeduced auto
+  {R"cpp(
+template
+void foo() {
+  [[au^to]] x = T{};
+}
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "/* not deduced */";
+   }},
   // auto on lambda
   {R"cpp(
 void foo() {
@@ -386,8 +410,9 @@
 }
 )cpp",
[](HoverInfo ) {
- HI.Name = "(lambda)";
- HI.Kind = index::SymbolKind::Class;
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "class(lambda)";
}},
   // auto on template instantiation
   {R"cpp(
@@ -397,8 +422,9 @@
 }
 )cpp",
[](HoverInfo ) {
- HI.Name = "Foo";
- HI.Kind = index::SymbolKind::Class;
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "class Foo";
}},
   // auto on specialized template
   {R"cpp(
@@ -409,8 +435,9 @@
 }
 )cpp",
[](HoverInfo ) {
- HI.Name = "Foo";
- HI.Kind = index::SymbolKind::Class;
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "class Foo";
}},
 
   // macro
@@ -582,8 +609,9 @@
   }
   )cpp",
   [](HoverInfo ) {
-HI.Name = "Foo";
-HI.Kind = index::SymbolKind::Class;
+HI.Name = "auto";
+HI.Kind = index::SymbolKind::TypeAlias;
+HI.Definition = "class Foo";
   }},
   {// Falls back to primary template, when the type is not instantiated.
R"cpp(
@@ -955,20 +983,11 @@
   llvm::StringRef Tests[] = {
   "^int main() {}",
   "void foo() {^}",
-  R"cpp(// structured binding. Not supported yet
-struct Bar {};
-void foo() {
-  Bar a[2];
-  ^auto [x,y] = a;
-}
-  )cpp",
-  R"cpp(// Template auto parameter. Nothing (Not useful).
-template
-void func() {
-}
-void foo() {
-   func<1>();
-}
+  // FIXME: "decltype(auto)" should be a single hover
+  "decltype(au^to) x = 0;",
+  // FIXME: not supported yet
+  R"cpp(// Lambda auto parameter
+auto lamb = [](a^uto){};
   )cpp",
   R"cpp(// non-named decls don't get hover. Don't crash!
 ^static_assert(1, "");
@@ -1545,9 +1564,9 @@
 }
   )cpp",
   [](HoverInfo ) {
-HI.Name = "int";
-// FIXME: Should be Builtin/Integral.
-HI.Kind = index::SymbolKind::Unknown;
+HI.Name = "auto";
+HI.Kind = index::SymbolKind::TypeAlias;
+HI.Definition = "int";
   }},
   {
   R"cpp(// Simple initialization with const auto
@@ -1555,14 +1574,22 @@
   const ^[[auto]] i = 1;
 }
   )cpp",
-  [](HoverInfo ) { HI.Name = "int"; }},
+  [](HoverInfo ) {
+HI.Name = "auto";
+HI.Kind = index::SymbolKind::TypeAlias;
+HI.Definition = "int";
+  }},
   {
   R"cpp(// Simple initialization with const auto&
 void foo() {
   const ^[[auto]]& i = 1;
 }
   )cpp",
-  [](HoverInfo ) { HI.Name = "int"; }},
+  [](HoverInfo ) {
+HI.Name = "auto";
+HI.Kind = index::SymbolKind::TypeAlias;
+HI.Definition = "int";
+  }},
   {
   R"cpp(// Simple initialization with auto&
 void foo() {
@@ -1570,7 +1597,11 @@
   ^[[auto]]& i = x;
 }
   )cpp",
-  [](HoverInfo ) { HI.Name = 

[PATCH] D93227: [clangd] Smarter hover on auto and decltype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:618
+  } else {
+CXXRecordDecl *D = QT->getAsCXXRecordDecl();
+if (D && D->isLambda())

sammccall wrote:
> You've rewritten this logic compared to the old `getHoverContents(QualType)`, 
> and I think there are regressions:
>  - We've dropped class documentation (see e.g. 
> ac3f9e48421712168884d59cbfe8b294dd76a19b, this is visible in the tests)
>  - we're no longer using printName() to print tag-decls, which I expect 
> changes e.g. the printing of anyonymous structs which is special-cased in 
> that function (we have tests for this but probably not in combination with 
> auto)
>  - we're no longer using the printing-policy to print non-tag types, which 
> will lead to some random changes
> 
> I don't see a reason for these changes, can we revert them?
- I can re-add class documentation, but should it work when `auto` is a pointer 
or a ref ? In that case, I'll need something like your `unwrapType` of 
https://reviews.llvm.org/D93314
- `printName` will print `C` instead of `class C` even if I hack around and 
give it the right `PrintingPolicy`. The problem is that IDE (at least VSCode) 
does not know what `C` is and cannot color it, which looks a nice less nice. Up 
to you !
- I can re-add the printing policy for non-tag types, and add it for tag-types 
if we chose not to use `printName`.



Comment at: clang-tools-extra/clangd/Hover.cpp:631
+// getDeducedType are handled.
+class ExtraAutoTypeHoverVisitor
+: public RecursiveASTVisitor {

sammccall wrote:
> This functionality (reporting the `auto` type in structured bindings, and not 
> reporting non-deduced uses of auto) belongs in the getDeducedType() helper 
> rather than as a layer on top.
> (Especially because it has to be done via an AST traversal rather than 
> SelectionTree)
> 
> I'd suggest leaving it out of this patch to get this the original change 
> landed quickly - this seems like a mostly-unrelated enhancement. But up to 
> you.
I'll remove this. I'll leave out the case of structured bindings for arrays 
(which is really weird and specific), but I guess I'll fix`getDeducedType` to 
return the undeduced `QualType` instead of `None` (which it already does but 
only for return types).



Comment at: clang-tools-extra/clangd/Hover.cpp:907
+
+  HI->Name = tok::getTokenName(Tok.kind());
+  HighlightRange = Tok.range(SM).toCharRange(SM);

sammccall wrote:
> decltype(auto) and decltype(expr) are fairly different things and ultimately 
> we should be displaying them differently I think ("decltype(auto)" vs 
> "decltype(...)").
> 
> Unfortunately it's awkward because our getDeducedType helper handles both at 
> the moment (and so is misnamed, because decltype(expr) isn't deduced at all).
> 
> Can you add `// FIXME: distinguish decltype(auto) vs decltype(expr)` and I'll 
> do some refactoring later?
Sure, I'll add the comment. I'll leave that refactoring to you, I'm not quite 
sure how you intent to achieve it.



Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:999
+  "decltype(au^to) x = 0;",
+  R"cpp(// Lambda auto parameter. Nothing (Not useful).
+auto lamb = [](a^uto){};

sammccall wrote:
> (not convinced this is fundamentally not useful - the fact that it's a 
> template parameter means it's probably worth having a hover card for it at 
> some point. But I agree with suppressing it for now)
As a user I'd prefer the hover to work over the whole `decltype(auto)` 
expression. But that does not seem quite compatible with the way tokens are 
parsed.

Are you suggesting I remove the test case or should I add a `FIXME` comment ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93227

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

[PATCH] D93227: [clangd] Smarter hover on auto and decltype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau updated this revision to Diff 312545.
qchateau marked 3 inline comments as done.
qchateau added a comment.

I updated this diff according to your review. There are a few things that may 
still need change but I need your input, see my other comments :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93227

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -379,6 +379,30 @@
  HI.Definition = "class X {}";
}},
 
+  // auto on structured bindings
+  {R"cpp(
+void foo() {
+  struct S { int x; float y; };
+  [[au^to]] [x, y] = S();
+}
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "struct S";
+   }},
+  // undeduced auto
+  {R"cpp(
+template
+void foo() {
+  [[au^to]] x = T{};
+}
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "/* not deduced */";
+   }},
   // auto on lambda
   {R"cpp(
 void foo() {
@@ -386,8 +410,9 @@
 }
 )cpp",
[](HoverInfo ) {
- HI.Name = "(lambda)";
- HI.Kind = index::SymbolKind::Class;
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "class(lambda)";
}},
   // auto on template instantiation
   {R"cpp(
@@ -397,8 +422,9 @@
 }
 )cpp",
[](HoverInfo ) {
- HI.Name = "Foo";
- HI.Kind = index::SymbolKind::Class;
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "class Foo";
}},
   // auto on specialized template
   {R"cpp(
@@ -409,8 +435,9 @@
 }
 )cpp",
[](HoverInfo ) {
- HI.Name = "Foo";
- HI.Kind = index::SymbolKind::Class;
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "class Foo";
}},
 
   // macro
@@ -582,8 +609,9 @@
   }
   )cpp",
   [](HoverInfo ) {
-HI.Name = "Foo";
-HI.Kind = index::SymbolKind::Class;
+HI.Name = "auto";
+HI.Kind = index::SymbolKind::TypeAlias;
+HI.Definition = "class Foo";
   }},
   {// Falls back to primary template, when the type is not instantiated.
R"cpp(
@@ -955,20 +983,9 @@
   llvm::StringRef Tests[] = {
   "^int main() {}",
   "void foo() {^}",
-  R"cpp(// structured binding. Not supported yet
-struct Bar {};
-void foo() {
-  Bar a[2];
-  ^auto [x,y] = a;
-}
-  )cpp",
-  R"cpp(// Template auto parameter. Nothing (Not useful).
-template
-void func() {
-}
-void foo() {
-   func<1>();
-}
+  "decltype(au^to) x = 0;",
+  R"cpp(// Lambda auto parameter. Nothing (Not useful).
+auto lamb = [](a^uto){};
   )cpp",
   R"cpp(// non-named decls don't get hover. Don't crash!
 ^static_assert(1, "");
@@ -1545,9 +1562,9 @@
 }
   )cpp",
   [](HoverInfo ) {
-HI.Name = "int";
-// FIXME: Should be Builtin/Integral.
-HI.Kind = index::SymbolKind::Unknown;
+HI.Name = "auto";
+HI.Kind = index::SymbolKind::TypeAlias;
+HI.Definition = "int";
   }},
   {
   R"cpp(// Simple initialization with const auto
@@ -1555,14 +1572,22 @@
   const ^[[auto]] i = 1;
 }
   )cpp",
-  [](HoverInfo ) { HI.Name = "int"; }},
+  [](HoverInfo ) {
+HI.Name = "auto";
+HI.Kind = index::SymbolKind::TypeAlias;
+HI.Definition = "int";
+  }},
   {
   R"cpp(// Simple initialization with const auto&
 void foo() {
   const ^[[auto]]& i = 1;
 }
   )cpp",
-  [](HoverInfo ) { HI.Name = "int"; }},
+  [](HoverInfo ) {
+HI.Name = "auto";
+HI.Kind = index::SymbolKind::TypeAlias;
+HI.Definition = "int";
+  }},
   {
   R"cpp(// Simple initialization with auto&
 void foo() {
@@ -1570,7 +1595,11 @@
   ^[[auto]]& i = x;
 }
   )cpp",
-  [](HoverInfo ) { HI.Name = "int"; }},
+  [](HoverInfo ) {
+

[PATCH] D93452: [clangd] Trim memory after buildINdex

[Quentin Chateau via Phabricator via cfe-commits]

qchateau planned changes to this revision.
qchateau added a comment.

> Periodic malloc_trim seems like it solves a real problem, one I don't 
> personally fully understand but we may never do - I feel convinced we should 
> use it anyway. It probably has some overhead, who knows how much.

It does indeed look like facing the problem from the wrong side (fight the end 
result instead of finding the root cause) but at that point it does solve an 
annoying problem. As I investigated this, I found that replacing `DenseMap` and 
`StringMap` wtih `std::map` mitigates the issue. I think allocating huge chunks 
of contiguous memory temporarily make this issue worse. Not very useful, but 
hey, I'm just sharing what I observed. As I previously said, resolving this 
problem by changing containers or allocators is a lot of work and is not 
guaranteed to actually solve the issue.

> It's process-wide (not per-thread, and certainly not per-program-module) so 
> the fact that slapping it in FileSymbols::buildIndex solves the problem only 
> proves that function is called periodically :-)
> This is a system-level function, I think we should probably be calling it 
> periodically from something top-level like ClangdMain or ClangdLSPServer.
> (We could almost slip it in with ClangdLSPServer::maybeExportMemoryProfile(), 
> except we don't want to link it to incoming notifications as memory can 
> probably grow while silently background indexing)

It can indeed be called somewhere else, I tried a few different placed before 
submitting this diff. That can be change easily, it's not a problem. I'd also 
like to see it at higher level: my first implementation called it periodically, 
inspired from `maybeExportMemoryProfile`, but I got worried by the background 
index filling the RAM while the user is not in front of his computer, 
`onBackgroundIndexProgress` should address this issue.

> I think we should pass a nonzero `pad` to malloc_trim of several MB. It only 
> affects the main thread, but the main thread is constantly allocating small 
> short-lived objects (e.g. JSON encoding/decoding) and cutting it to the bone 
> only to have it sbrk again seems pointless.

Good point


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452

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

[PATCH] D93452: [clangd] Trim memory after buildINdex

[Quentin Chateau via Phabricator via cfe-commits]

qchateau updated this revision to Diff 312500.
qchateau added a comment.
Herald added a subscriber: mgorny.

Add macro to use malloc_trim only if available


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/index/FileIndex.cpp


Index: clang-tools-extra/clangd/index/FileIndex.cpp
===
--- clang-tools-extra/clangd/index/FileIndex.cpp
+++ clang-tools-extra/clangd/index/FileIndex.cpp
@@ -32,6 +32,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -41,10 +42,21 @@
 #include 
 #include 
 
+#ifdef HAVE_MALLOC_TRIM
+#include 
+#endif // HAVE_MALLOC_TRIM
+
 namespace clang {
 namespace clangd {
 namespace {
 
+void trimMemory() {
+#ifdef HAVE_MALLOC_TRIM
+  if (malloc_trim(0))
+vlog("Trimmed memory");
+#endif // HAVE_MALLOC_TRIM
+}
+
 SlabTuple indexSymbols(ASTContext , std::shared_ptr PP,
llvm::ArrayRef DeclsToIndex,
const MainFileMacros *MacroRefsToIndex,
@@ -263,6 +275,10 @@
 std::unique_ptr
 FileSymbols::buildIndex(IndexType Type, DuplicateHandling DuplicateHandle,
 size_t *Version) {
+  // Building the index often leaves a lof of freed but unreleased memory
+  // (at the OS level): manually trim the memory after the index is built
+  auto _ = llvm::make_scope_exit(trimMemory);
+
   std::vector> SymbolSlabs;
   std::vector> RefSlabs;
   std::vector> RelationSlabs;
Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -154,6 +154,13 @@
   clangdSupport
   )
 
+check_symbol_exists(malloc_trim malloc.h HAVE_MALLOC_TRIM)
+if (HAVE_MALLOC_TRIM)
+  target_compile_definitions(obj.clangDaemon
+PRIVATE HAVE_MALLOC_TRIM
+)
+endif ()
+
 add_subdirectory(refactor/tweaks)
 if (${CMAKE_SYSTEM_NAME} STREQUAL "Linux")
   # FIXME: Make fuzzer not use linux-specific APIs, build it everywhere.


Index: clang-tools-extra/clangd/index/FileIndex.cpp
===
--- clang-tools-extra/clangd/index/FileIndex.cpp
+++ clang-tools-extra/clangd/index/FileIndex.cpp
@@ -32,6 +32,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -41,10 +42,21 @@
 #include 
 #include 
 
+#ifdef HAVE_MALLOC_TRIM
+#include 
+#endif // HAVE_MALLOC_TRIM
+
 namespace clang {
 namespace clangd {
 namespace {
 
+void trimMemory() {
+#ifdef HAVE_MALLOC_TRIM
+  if (malloc_trim(0))
+vlog("Trimmed memory");
+#endif // HAVE_MALLOC_TRIM
+}
+
 SlabTuple indexSymbols(ASTContext , std::shared_ptr PP,
llvm::ArrayRef DeclsToIndex,
const MainFileMacros *MacroRefsToIndex,
@@ -263,6 +275,10 @@
 std::unique_ptr
 FileSymbols::buildIndex(IndexType Type, DuplicateHandling DuplicateHandle,
 size_t *Version) {
+  // Building the index often leaves a lof of freed but unreleased memory
+  // (at the OS level): manually trim the memory after the index is built
+  auto _ = llvm::make_scope_exit(trimMemory);
+
   std::vector> SymbolSlabs;
   std::vector> RefSlabs;
   std::vector> RelationSlabs;
Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -154,6 +154,13 @@
   clangdSupport
   )
 
+check_symbol_exists(malloc_trim malloc.h HAVE_MALLOC_TRIM)
+if (HAVE_MALLOC_TRIM)
+  target_compile_definitions(obj.clangDaemon
+PRIVATE HAVE_MALLOC_TRIM
+)
+endif ()
+
 add_subdirectory(refactor/tweaks)
 if (${CMAKE_SYSTEM_NAME} STREQUAL "Linux")
   # FIXME: Make fuzzer not use linux-specific APIs, build it everywhere.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D93452: [clangd] Trim memory after buildINdex

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

> Side note is this memory failing to free behaviour observed on windows?

No idea, I only develop on linux. Looking at the the github issues, it seems 
people that had the issue were also using the linux version...so we can't 
conclude anything about windows




Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:51
+void trimMemory() {
+  if (malloc_trim(0))
+vlog("Trimmed memory");

njames93 wrote:
> It may be helpful to log how many times this call fails to free any memory.
From what I observed, it (almost?) never fails to free memory, although 
sometimes it's only a few kB.
Anyway, are you suggesting to keep track of how many time we call `malloc_trim` 
and how many times it fails ? Or simply logging when it fails ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93452

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

[PATCH] D93452: [clangd] Trim memory after buildINdex

[Quentin Chateau via Phabricator via cfe-commits]

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

This diff addresses the issue of the ever increasing memory usage of clangd. 
The key to understand what happens is to use `malloc_stats()`: malloc arenas 
keep getting bigger, although the actual memory used does not. It seems some 
operations while bulding the indices (both dynamic and background) create this 
problem. Specifically, 'FileSymbols::update' and 'FileSymbols::buildIndex' seem 
especially affected.

This diff adds a call to `malloc_trim(0)` at the end of 
`FileSymbols::buildIndex`.

For reference:
https://github.com/clangd/clangd/issues/251
https://github.com/clangd/clangd/issues/115



I'm not sure how much I'm allowed to use GNU extensions but this diff has been 
a game changer for me and my 8GB RAM laptop.

In any case, I think I've properly diagnosed to issue. Once trimmed, the actual 
memory usage of the process is approximately what clangd reports with the 
"memoryUsage" feature.

The solution is either what I suggest here (use `malloc_trim`) or rework a ton 
of code, change containers and allocators and hope the problem won't happen 
again in the future as we add features.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D93452

Files:
  clang-tools-extra/clangd/index/FileIndex.cpp


Index: clang-tools-extra/clangd/index/FileIndex.cpp
===
--- clang-tools-extra/clangd/index/FileIndex.cpp
+++ clang-tools-extra/clangd/index/FileIndex.cpp
@@ -32,6 +32,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -40,11 +41,17 @@
 #include 
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
 namespace {
 
+void trimMemory() {
+  if (malloc_trim(0))
+vlog("Trimmed memory");
+}
+
 SlabTuple indexSymbols(ASTContext , std::shared_ptr PP,
llvm::ArrayRef DeclsToIndex,
const MainFileMacros *MacroRefsToIndex,
@@ -263,6 +270,10 @@
 std::unique_ptr
 FileSymbols::buildIndex(IndexType Type, DuplicateHandling DuplicateHandle,
 size_t *Version) {
+  // Building the index often leaves a lof of freed but unreleased memory
+  // (at the OS level): manually trim the memory after the index is built
+  auto _ = llvm::make_scope_exit(trimMemory);
+
   std::vector> SymbolSlabs;
   std::vector> RefSlabs;
   std::vector> RelationSlabs;


Index: clang-tools-extra/clangd/index/FileIndex.cpp
===
--- clang-tools-extra/clangd/index/FileIndex.cpp
+++ clang-tools-extra/clangd/index/FileIndex.cpp
@@ -32,6 +32,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -40,11 +41,17 @@
 #include 
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
 namespace {
 
+void trimMemory() {
+  if (malloc_trim(0))
+vlog("Trimmed memory");
+}
+
 SlabTuple indexSymbols(ASTContext , std::shared_ptr PP,
llvm::ArrayRef DeclsToIndex,
const MainFileMacros *MacroRefsToIndex,
@@ -263,6 +270,10 @@
 std::unique_ptr
 FileSymbols::buildIndex(IndexType Type, DuplicateHandling DuplicateHandle,
 size_t *Version) {
+  // Building the index often leaves a lof of freed but unreleased memory
+  // (at the OS level): manually trim the memory after the index is built
+  auto _ = llvm::make_scope_exit(trimMemory);
+
   std::vector> SymbolSlabs;
   std::vector> RefSlabs;
   std::vector> RelationSlabs;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D93227: [clangd] Smarter hover on auto and decltype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

> We can't easily add new SymbolKinds (it's an enum we don't own, which we 
> should fix, but that's a yak-shave). But `TypeAlias` almost fits these roles 
> for auto.

Yes I originally wanted to have "deduced-type `auto`" but I realized it would 
not be easy. I did not think of `TypeAlias`, let's use this until we can do 
better.

> (There are other cases: function parameter types `void foo(auto X)` and NTTP 
> types `template  class Y` but we can punt on those)

For now I disabled them (instead of displaying nonsense as we did before). 
Anyway as a user, I have zero useful information to get when hovering on these 
word, I prefer nothing rather than a useless pop-up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93227

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

[PATCH] D93227: [clangd] Smarter hover on auto and decltype

[Quentin Chateau via Phabricator via cfe-commits]

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

- Rebase on master after D92041 
- Remove the usage of the "Documentation" field
- Use the `TypeAlias` Kind on auto and decltype
- Move code related to hover on `this` in a new function
- Update hover on `this` to be consistent with this change


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93227

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -379,6 +379,42 @@
  HI.Definition = "class X {}";
}},
 
+  // auto on structured bindings
+  {R"cpp(
+void foo() {
+  int arr[2];
+  [[au^to]] [x, y] = arr;
+}
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "int[2]";
+   }},
+  // auto on structured bindings
+  {R"cpp(
+void foo() {
+  struct S { int x; float y; };
+  [[au^to]] [x, y] = S();
+}
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "struct S";
+   }},
+  // undeduced auto
+  {R"cpp(
+template
+void foo() {
+  [[au^to]] x = T{};
+}
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "/* not deduced */";
+   }},
   // auto on lambda
   {R"cpp(
 void foo() {
@@ -386,8 +422,9 @@
 }
 )cpp",
[](HoverInfo ) {
- HI.Name = "(lambda)";
- HI.Kind = index::SymbolKind::Class;
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "(lambda)";
}},
   // auto on template instantiation
   {R"cpp(
@@ -397,8 +434,9 @@
 }
 )cpp",
[](HoverInfo ) {
- HI.Name = "Foo";
- HI.Kind = index::SymbolKind::Class;
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "class Foo";
}},
   // auto on specialized template
   {R"cpp(
@@ -409,8 +447,9 @@
 }
 )cpp",
[](HoverInfo ) {
- HI.Name = "Foo";
- HI.Kind = index::SymbolKind::Class;
+ HI.Name = "auto";
+ HI.Kind = index::SymbolKind::TypeAlias;
+ HI.Definition = "class Foo";
}},
 
   // macro
@@ -582,8 +621,9 @@
   }
   )cpp",
   [](HoverInfo ) {
-HI.Name = "Foo";
-HI.Kind = index::SymbolKind::Class;
+HI.Name = "auto";
+HI.Kind = index::SymbolKind::TypeAlias;
+HI.Definition = "class Foo";
   }},
   {// Falls back to primary template, when the type is not instantiated.
R"cpp(
@@ -955,12 +995,9 @@
   llvm::StringRef Tests[] = {
   "^int main() {}",
   "void foo() {^}",
-  R"cpp(// structured binding. Not supported yet
-struct Bar {};
-void foo() {
-  Bar a[2];
-  ^auto [x,y] = a;
-}
+  "decltype(au^to) x = 0;",
+  R"cpp(// Lambda auto parameter. Nothing (Not useful).
+auto lamb = [](a^uto){};
   )cpp",
   R"cpp(// Template auto parameter. Nothing (Not useful).
 template
@@ -1545,9 +1582,9 @@
 }
   )cpp",
   [](HoverInfo ) {
-HI.Name = "int";
-// FIXME: Should be Builtin/Integral.
-HI.Kind = index::SymbolKind::Unknown;
+HI.Name = "auto";
+HI.Kind = index::SymbolKind::TypeAlias;
+HI.Definition = "int";
   }},
   {
   R"cpp(// Simple initialization with const auto
@@ -1555,14 +1592,22 @@
   const ^[[auto]] i = 1;
 }
   )cpp",
-  [](HoverInfo ) { HI.Name = "int"; }},
+  [](HoverInfo ) {
+HI.Name = "auto";
+HI.Kind = index::SymbolKind::TypeAlias;
+HI.Definition = "int";
+  }},
   {
   R"cpp(// Simple initialization with const auto&
 void foo() {
   const ^[[auto]]& i = 1;
 }
   )cpp",
-  [](HoverInfo ) { HI.Name = "int"; }},
+  [](HoverInfo ) {
+HI.Name = "auto";
+HI.Kind = index::SymbolKind::TypeAlias;
+HI.Definition = "int";
+  }},
   {
   R"cpp(// Simple initialization with auto&
 void foo() {
@@ -1570,7 +1615,11 @@
   

[PATCH] D93314: [clangd] go-to-definition on auto unwraps array/pointer types

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

> So I'm leaning towards defining these behaviors for **pointers and 
> containers** rather than **templates** per se. There's a nice symmetry with 
> the raw pointer and array types that this patch.
> (We have some precedent and code for detecting smart pointers, see 
> `getPointeeType()` in FindTarget.cpp.)

If you think it's fine to have specific code using heuristics to try to find 
what the user expects, then **pointers and containers** sounds like a good 
target.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93314

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

[PATCH] D93314: [clangd] go-to-definition on auto unwraps array/pointer types

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

Ah good catch, I agree this is the behavior we want.

Concerning cases like `unique_ptr`, the only generic behavior I can think of 
would be to suggest multiple `LocatedSymbol` when `auto` is deduced to a 
template class: the template class itself, and all its template type parameters.

  class A {};
  class B {};
  
  template
  class C {};
  
  auto x = C();

go-to-definition on `auto` could suggest jumping to `C`, `A` or `B`.

In a way, when you explicitly have `C x;` you can go-to-def to either 
`A`, `B` or `C` in one click, it is simply made easier because all these types 
are visible in the code and you make the choice by choosing on where you click.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93314

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

[PATCH] D93227: [clangd] Smarter hover on auto and decltype

[Quentin Chateau via Phabricator via cfe-commits]

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

Only show the keyword as the hover "Name".

Show whether the type is deduced or undeduced as
the hover "Documentation".

Show the deduced type (if any) as the "Definition".

Don't show any hover information for:

- the "auto" word of "decltype(auto)"
- "auto" in lambda parameters
- "auto" in template arguments


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D93227

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -379,6 +379,41 @@
  HI.Definition = "class X {}";
}},
 
+  // auto on structured bindings
+  {R"cpp(
+void foo() {
+  int arr[2];
+  [[au^to]] [x, y] = arr;
+}
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "auto";
+ HI.Documentation = "Deduced type";
+ HI.Definition = "int[2]";
+   }},
+  // auto on structured bindings
+  {R"cpp(
+void foo() {
+  struct S { int x; float y; };
+  [[au^to]] [x, y] = S();
+}
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "auto";
+ HI.Documentation = "Deduced type";
+ HI.Definition = "struct S";
+   }},
+  // undeduced auto
+  {R"cpp(
+template
+void foo() {
+  [[au^to]] x = T{};
+}
+)cpp",
+   [](HoverInfo ) {
+ HI.Name = "auto";
+ HI.Documentation = "Undeduced type";
+   }},
   // auto on lambda
   {R"cpp(
 void foo() {
@@ -386,8 +421,9 @@
 }
 )cpp",
[](HoverInfo ) {
- HI.Name = "(lambda)";
- HI.Kind = index::SymbolKind::Class;
+ HI.Name = "auto";
+ HI.Documentation = "Deduced type";
+ HI.Definition = "(lambda)";
}},
   // auto on template instantiation
   {R"cpp(
@@ -397,8 +433,9 @@
 }
 )cpp",
[](HoverInfo ) {
- HI.Name = "Foo";
- HI.Kind = index::SymbolKind::Class;
+ HI.Name = "auto";
+ HI.Documentation = "Deduced type";
+ HI.Definition = "class Foo";
}},
   // auto on specialized template
   {R"cpp(
@@ -409,8 +446,9 @@
 }
 )cpp",
[](HoverInfo ) {
- HI.Name = "Foo";
- HI.Kind = index::SymbolKind::Class;
+ HI.Name = "auto";
+ HI.Documentation = "Deduced type";
+ HI.Definition = "class Foo";
}},
 
   // macro
@@ -582,8 +620,9 @@
   }
   )cpp",
   [](HoverInfo ) {
-HI.Name = "Foo";
-HI.Kind = index::SymbolKind::Class;
+HI.Name = "auto";
+HI.Documentation = "Deduced type";
+HI.Definition = "class Foo";
   }},
   {// Falls back to primary template, when the type is not instantiated.
R"cpp(
@@ -955,12 +994,9 @@
   llvm::StringRef Tests[] = {
   "^int main() {}",
   "void foo() {^}",
-  R"cpp(// structured binding. Not supported yet
-struct Bar {};
-void foo() {
-  Bar a[2];
-  ^auto [x,y] = a;
-}
+  "decltype(au^to) x = 0;",
+  R"cpp(// Lambda auto parameter. Nothing (Not useful).
+auto lamb = [](a^uto){};
   )cpp",
   R"cpp(// Template auto parameter. Nothing (Not useful).
 template
@@ -1545,9 +1581,9 @@
 }
   )cpp",
   [](HoverInfo ) {
-HI.Name = "int";
-// FIXME: Should be Builtin/Integral.
-HI.Kind = index::SymbolKind::Unknown;
+HI.Name = "auto";
+HI.Documentation = "Deduced type";
+HI.Definition = "int";
   }},
   {
   R"cpp(// Simple initialization with const auto
@@ -1555,14 +1591,22 @@
   const ^[[auto]] i = 1;
 }
   )cpp",
-  [](HoverInfo ) { HI.Name = "int"; }},
+  [](HoverInfo ) {
+HI.Name = "auto";
+HI.Documentation = "Deduced type";
+HI.Definition = "int";
+  }},
   {
   R"cpp(// Simple initialization with const auto&
 void foo() {
   const ^[[auto]]& i = 1;
 }
   )cpp",
-  [](HoverInfo ) { HI.Name = "int"; }},
+  [](HoverInfo ) {
+HI.Name = "auto";
+HI.Documentation = "Deduced type";
+HI.Definition = "int";
+  }},
   {
   R"cpp(// Simple initialization with auto&
 void foo() {
@@ -1570,7 

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

I've updated the patch with your latest comments.

Please commit this patch for me, using "quentin.chat...@gmail.com", I'll 
probably ask for commit access after a few successful reviews


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977

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

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau updated this revision to Diff 311247.
qchateau marked an inline comment as done.
qchateau added a comment.

- Reintroduce test with a FIXME comment
- locateSymbolForType returns a vector instead of an optional


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977

Files:
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/ASTTests.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -624,6 +624,134 @@
 struct Fo^o {};
   )cpp",
 
+  R"cpp(// auto builtin type (not supported)
+^auto x = 42;
+  )cpp",
+
+  R"cpp(// auto on lambda
+auto x = [[[]]]{};
+^auto y = x;
+  )cpp",
+
+  R"cpp(// auto on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+^auto x = ns1::S1{};
+  )cpp",
+
+  R"cpp(// decltype on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+ns1::S1 i;
+^decltype(i) j;
+  )cpp",
+
+  R"cpp(// decltype(auto) on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+ns1::S1 i;
+ns1::S1& j = i;
+^decltype(auto) k = j;
+  )cpp",
+
+  R"cpp(// auto on template class
+template class [[Foo]] {};
+
+^auto x = Foo();
+  )cpp",
+
+  R"cpp(// auto on template class with forward declared class
+template class [[Foo]] {};
+class X;
+
+^auto x = Foo();
+  )cpp",
+
+  R"cpp(// auto on specialized template class
+template class Foo {};
+template<> class [[Foo]] {};
+
+^auto x = Foo();
+  )cpp",
+
+  R"cpp(// auto on initializer list.
+namespace std
+{
+  template
+  class [[initializer_list]] {};
+}
+
+^auto i = {1,2};
+  )cpp",
+
+  R"cpp(// auto function return with trailing type
+struct [[Bar]] {};
+^auto test() -> decltype(Bar()) {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// decltype in trailing return type
+struct [[Bar]] {};
+auto test() -> ^decltype(Bar()) {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// auto in function return
+struct [[Bar]] {};
+^auto test() {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// auto& in function return
+struct [[Bar]] {};
+^auto& test() {
+  static Bar x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// auto* in function return
+struct [[Bar]] {};
+^auto* test() {
+  Bar* x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// const auto& in function return
+struct [[Bar]] {};
+const ^auto& test() {
+  static Bar x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// decltype(auto) in function return
+struct [[Bar]] {};
+^decltype(auto) test() {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// decltype of function with trailing return type.
+struct [[Bar]] {};
+auto test() -> decltype(Bar()) {
+  return Bar();
+}
+void foo() {
+  ^decltype(test()) i = test();
+}
+  )cpp",
+
   R"cpp(// Override specifier jumps to overridden method
 class Y { virtual void $decl[[a]]() = 0; };
 class X : Y { void a() ^override {} };
Index: clang-tools-extra/clangd/unittests/ASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ASTTests.cpp
@@ -26,23 +26,168 @@
 namespace clangd {
 namespace {
 
-TEST(GetDeducedType, KwAutoExpansion) {
+TEST(GetDeducedType, KwAutoKwDecltypeExpansion) {
   struct Test {
 StringRef AnnotatedCode;
 const char *DeducedType;
   } Tests[] = {
   {"^auto i = 0;", "int"},
   {"^auto f(){ return 1;};", "int"},
+  {
+  R"cpp( // auto on struct in a namespace
+  namespace ns1 { struct S {}; }
+  ^auto v = ns1::S{};
+  )cpp",
+  "struct ns1::S",
+  },
+  {
+  R"cpp( // decltype on struct
+  namespace ns1 { struct S {}; }
+  ns1::S i;
+  ^decltype(i) j;
+  )cpp",
+  "ns1::S",
+  },
+  {
+  R"cpp(// decltype(auto) on struct&
+namespace ns1 {
+struct S {};
+} // namespace ns1
+
+ns1::S i;
+ns1::S& j = i;
+^decltype(auto) k = j;
+  )cpp",
+  "struct ns1::S &",
+  

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau marked 3 inline comments as done.
qchateau added inline comments.



Comment at: clang-tools-extra/clangd/XRefs.cpp:689
+const NamedDecl *D = Deduced->getTypePtr()->getAsTagDecl();
+if (!D)
+  continue;

sammccall wrote:
> there are many other types we could resolve to a decl: TypedefType, 
> TemplateTypeParmtype, ...
> If we're only going to handle tag types, it deserves a FIXME comment.
> 
> But we do have  a helper for this. Can you try calling 
> `targetDecl(DynTypedNode::create(Deduced), DeclRelation::TemplatePattern | 
> DeclRelation::Alias)` instead of `Deduced->getTypePtr()->getAsTagDecl()`?
> 
> That should handle more cases, at a minimum, this case sholud pass:
> ```
> using [[T]] = int;
> T x;
> ^auto y = x;
> ```
> 
> (I see you have a test case that aliases backed by tag types are resolved to 
> the underlying tag decl, this change would offer the alias instead, which is 
> more consistent with our current go-to-definition. You could also offer both 
> by passing `DeclRelation::TemplatePattern | DeclRelation::Alias | 
> DeclRelation::Underlying`... but I think we should value consistency here)
`locateSymbolForType` uses `targetDecl`. Thanks for the tip, that's exactly 
what I was looking for.

I also agree with you that go-to-definition should go to the alias instead of 
the underlying type for consistency, but using 
`targetDecl(DynTypedNode::create(Deduced), DeclRelation::TemplatePattern | 
DeclRelation::Alias)` is not enough to resolve this consistency issue: 
`getDeducedType` already returns the underlying type. My current knowledge of 
the clangd code is nou sufficient to understand why, but the root of the 
problem seem to lie in the `DeducedTypeVisitor` class.

I removed the test that tested the undesirable behavior, should I open a bug 
report for `getDeducedType` that should not "go through" aliases ? In which 
case, what's the right place for that ? Github ?



Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:669
+
+  R"cpp(// auto on template class with forward declared class
+template class [[Foo]] {};

sammccall wrote:
> There's a scalability problem with testing every corner of the C++ language 
> (there are lots!) with every feature. Generally I prefer to cover some 
> important/weird/bugfix cases here, but to cover as much as possible with 
> unittests of the underlying functions.
> 
> In this case, that would mean
>  - moving tests that are about `auto` in different contexts to ASTTests.cpp 
> (current test coverage there is poor).
>  - (assuming we can use `targetDecl()`) moving tests that are about which 
> decl a type should resolve to to `FindTargetTests.cpp` - but current coverage 
> there is pretty good so we can probably just drop many of them.
I added the relevant tests to `ASTTests.cpp` but I did not remove the tests 
here for the moment. I've always had the habit to keep tests that are already 
written, even if redundant.

If you'd like me to remove some of them, I'd say the only tests that are not 
redundant with the ones in `ASTTests.cpp` are the ones that test template 
specializations. Shoud I keep only these ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977

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

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Quentin Chateau via Phabricator via cfe-commits]

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

- Remove the patches affecting hover
- Add tests in ASTTests.cpp to test the behavior of getDeducedType for auto and 
decltype
- Add missing comment
- Create a locateSymbolForType function (which uses targetDecl)
- Remove the unit tests that were testing undesirable behaviors


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977

Files:
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/ASTTests.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -624,6 +624,134 @@
 struct Fo^o {};
   )cpp",
 
+  R"cpp(// auto builtin type (not supported)
+^auto x = 42;
+  )cpp",
+
+  R"cpp(// auto on lambda
+auto x = [[[]]]{};
+^auto y = x;
+  )cpp",
+
+  R"cpp(// auto on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+^auto x = ns1::S1{};
+  )cpp",
+
+  R"cpp(// decltype on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+ns1::S1 i;
+^decltype(i) j;
+  )cpp",
+
+  R"cpp(// decltype(auto) on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+ns1::S1 i;
+ns1::S1& j = i;
+^decltype(auto) k = j;
+  )cpp",
+
+  R"cpp(// auto on template class
+template class [[Foo]] {};
+
+^auto x = Foo();
+  )cpp",
+
+  R"cpp(// auto on template class with forward declared class
+template class [[Foo]] {};
+class X;
+
+^auto x = Foo();
+  )cpp",
+
+  R"cpp(// auto on specialized template class
+template class Foo {};
+template<> class [[Foo]] {};
+
+^auto x = Foo();
+  )cpp",
+
+  R"cpp(// auto on initializer list.
+namespace std
+{
+  template
+  class [[initializer_list]] {};
+}
+
+^auto i = {1,2};
+  )cpp",
+
+  R"cpp(// auto function return with trailing type
+struct [[Bar]] {};
+^auto test() -> decltype(Bar()) {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// decltype in trailing return type
+struct [[Bar]] {};
+auto test() -> ^decltype(Bar()) {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// auto in function return
+struct [[Bar]] {};
+^auto test() {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// auto& in function return
+struct [[Bar]] {};
+^auto& test() {
+  static Bar x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// auto* in function return
+struct [[Bar]] {};
+^auto* test() {
+  Bar* x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// const auto& in function return
+struct [[Bar]] {};
+const ^auto& test() {
+  static Bar x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// decltype(auto) in function return
+struct [[Bar]] {};
+^decltype(auto) test() {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// decltype of function with trailing return type.
+struct [[Bar]] {};
+auto test() -> decltype(Bar()) {
+  return Bar();
+}
+void foo() {
+  ^decltype(test()) i = test();
+}
+  )cpp",
+
   R"cpp(// Override specifier jumps to overridden method
 class Y { virtual void $decl[[a]]() = 0; };
 class X : Y { void a() ^override {} };
Index: clang-tools-extra/clangd/unittests/ASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ASTTests.cpp
@@ -26,23 +26,159 @@
 namespace clangd {
 namespace {
 
-TEST(GetDeducedType, KwAutoExpansion) {
+TEST(GetDeducedType, KwAutoKwDecltypeExpansion) {
   struct Test {
 StringRef AnnotatedCode;
 const char *DeducedType;
   } Tests[] = {
   {"^auto i = 0;", "int"},
   {"^auto f(){ return 1;};", "int"},
+  {
+  R"cpp( // auto on struct in a namespace
+  namespace ns1 { struct S {}; }
+  ^auto v = ns1::S{};
+  )cpp",
+  "struct ns1::S",
+  },
+  {
+  R"cpp( // decltype on struct
+  namespace ns1 { struct S {}; }
+  ns1::S i;
+  ^decltype(i) j;
+  )cpp",
+  "ns1::S",
+  },
+  {
+  R"cpp(// decltype(auto) on struct&
+namespace ns1 {
+struct S {};
+} // namespace ns1
+
+  

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

Not sure what's wrong with the unit test "x64 windows > 
LLVM.CodeGen/XCore::threads.ll", I don't see how it's related to my patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977

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

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Quentin Chateau via Phabricator via cfe-commits]

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

Fix unittests and behavior

I've fixed the failing unittests, added some more
to test the new behavior, and fixed some bugs in
the new code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -624,6 +624,140 @@
 struct Fo^o {};
   )cpp",
 
+  R"cpp(// auto builtin type (not supported)
+au^to x = 42;
+  )cpp",
+
+  R"cpp(// auto on lambda
+auto x = [[[]]]{};
+au^to y = x;
+  )cpp",
+
+  R"cpp(// auto on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+au^to x = ns1::S1{};
+  )cpp",
+
+  R"cpp(// decltype on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+ns1::S1 i;
+decl^type(i) j;
+  )cpp",
+
+  R"cpp(// decltype(auto) on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+ns1::S1 i;
+ns1::S1& j = i;
+decl^type(auto) k = j;
+  )cpp",
+
+  R"cpp(// auto on template class
+template class [[Foo]] {};
+
+au^to x = Foo();
+  )cpp",
+
+  R"cpp(// auto on template class with forward declared class
+template class [[Foo]] {};
+class X;
+
+au^to x = Foo();
+  )cpp",
+
+  R"cpp(// auto on specialized template class
+template class Foo {};
+template<> class [[Foo]] {};
+
+au^to x = Foo();
+  )cpp",
+
+  R"cpp(// auto on initializer list.
+namespace std
+{
+  template
+  class [[initializer_list]] {};
+}
+
+au^to i = {1,2};
+  )cpp",
+
+  R"cpp(// auto function return with trailing type
+struct [[Bar]] {};
+au^to test() -> decltype(Bar()) {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// decltype in trailing return type
+struct [[Bar]] {};
+auto test() -> decl^type(Bar()) {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// auto in function return
+struct [[Bar]] {};
+au^to test() {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// auto& in function return
+struct [[Bar]] {};
+au^to& test() {
+  static Bar x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// auto* in function return
+struct [[Bar]] {};
+au^to* test() {
+  Bar* x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// const auto& in function return
+struct [[Bar]] {};
+const au^to& test() {
+  static Bar x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// decltype(auto) in function return
+struct [[Bar]] {};
+decl^type(auto) test() {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// decltype of function with trailing return type.
+struct [[Bar]] {};
+auto test() -> decltype(Bar()) {
+  return Bar();
+}
+void foo() {
+  decl^type(test()) i = test();
+}
+  )cpp",
+
+  R"cpp(// auto on alias
+struct [[cls]] {};
+typedef cls cls_type;
+au^to y = cls_type();
+  )cpp",
+
   R"cpp(// Override specifier jumps to overridden method
 class Y { virtual void $decl[[a]]() = 0; };
 class X : Y { void a() ^override {} };
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -388,6 +388,9 @@
[](HoverInfo ) {
  HI.Name = "(lambda)";
  HI.Kind = index::SymbolKind::Class;
+ HI.NamespaceScope = "";
+ HI.LocalScope = "foo::";
+ HI.Definition = "class {}";
}},
   // auto on template instantiation
   {R"cpp(
@@ -399,6 +402,8 @@
[](HoverInfo ) {
  HI.Name = "Foo";
  HI.Kind = index::SymbolKind::Class;
+ HI.NamespaceScope = "";
+ HI.Definition = "template <> class Foo {}";
}},
   // auto on specialized template
   {R"cpp(
@@ -411,6 +416,8 @@
[](HoverInfo ) {
  HI.Name = "Foo";
  HI.Kind = index::SymbolKind::Class;
+ HI.NamespaceScope = "";
+ HI.Definition = "template <> class Foo {}";
}},
 
   // macro
@@ -584,6 +591,8 

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Quentin Chateau via Phabricator via cfe-commits]

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

Make the hover content on auto and decltype
look like the hover content we would get on the
deduced type.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92977

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/XRefs.cpp


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -667,21 +667,44 @@
 return {};
   }
 
-  const syntax::Token *TouchedIdentifier =
-  syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens());
-  if (TouchedIdentifier)
-if (auto Macro =
-locateMacroReferent(*TouchedIdentifier, AST, *MainFilePath))
-  // Don't look at the AST or index if we have a macro result.
-  // (We'd just return declarations referenced from the macro's
-  // expansion.)
-  return {*std::move(Macro)};
-
-  ASTNodeKind NodeKind;
-  auto ASTResults = locateASTReferent(*CurLoc, TouchedIdentifier, AST,
-  *MainFilePath, Index, );
-  if (!ASTResults.empty())
-return ASTResults;
+  auto TokensTouchingCursor =
+  syntax::spelledTokensTouching(*CurLoc, AST.getTokens());
+  for (const auto  : TokensTouchingCursor) {
+if (Tok.kind() == tok::identifier) {
+  if (auto Macro = locateMacroReferent(Tok, AST, *MainFilePath))
+// Don't look at the AST or index if we have a macro result.
+// (We'd just return declarations referenced from the macro's
+// expansion.)
+return {*std::move(Macro)};
+
+  ASTNodeKind NodeKind;
+  auto ASTResults = locateASTReferent(*CurLoc, , AST, *MainFilePath,
+  Index, );
+  if (!ASTResults.empty())
+return ASTResults;
+} else if (Tok.kind() == tok::kw_auto || Tok.kind() == tok::kw_decltype) {
+  if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) {
+const NamedDecl *D = Deduced->getTypePtr()->getAsTagDecl();
+if (!D)
+  continue;
+
+D = getPreferredDecl(D);
+auto Loc = makeLocation(AST.getASTContext(), nameLocation(*D, SM),
+*MainFilePath);
+if (!Loc)
+  continue;
+
+LocatedSymbol LocSym;
+LocSym.Name = printName(AST.getASTContext(), *D);
+LocSym.PreferredDeclaration = *Loc;
+if (const NamedDecl *Def = getDefinition(D))
+  LocSym.Definition = makeLocation(
+  AST.getASTContext(), nameLocation(*Def, SM), *MainFilePath);
+
+return {std::move(LocSym)};
+  }
+}
+  }
 
   // If the cursor can't be resolved directly, try fallback strategies.
   auto Word =
@@ -695,7 +718,7 @@
 Word->Text);
 return {*std::move(Macro)};
   }
-  ASTResults =
+  auto ASTResults =
   locateASTReferent(NearbyIdent->location(), NearbyIdent, AST,
 *MainFilePath, Index, /*NodeKind=*/nullptr);
   if (!ASTResults.empty()) {
@@ -708,6 +731,7 @@
   }
 }
 // No nearby word, or it didn't refer to anything either. Try the index.
+ASTNodeKind NodeKind;
 auto TextualResults =
 locateSymbolTextually(*Word, AST, Index, *MainFilePath, NodeKind);
 if (!TextualResults.empty())
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -556,12 +556,7 @@
   HoverInfo HI;
 
   if (const auto *D = T->getAsTagDecl()) {
-HI.Name = printName(ASTCtx, *D);
-HI.Kind = index::getSymbolInfo(D).Kind;
-
-const auto *CommentD = getDeclForComment(D);
-HI.Documentation = getDeclComment(ASTCtx, *CommentD);
-enhanceFromIndex(HI, *CommentD, Index);
+return getHoverContents(D, Index);
   } else {
 // Builtin types
 auto Policy = printingPolicyForDecls(ASTCtx.getPrintingPolicy());


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -667,21 +667,44 @@
 return {};
   }
 
-  const syntax::Token *TouchedIdentifier =
-  syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens());
-  if (TouchedIdentifier)
-if (auto Macro =
-locateMacroReferent(*TouchedIdentifier, AST, *MainFilePath))
-  // Don't look at the AST or index if we have a macro result.
-  // (We'd just return declarations referenced from the macro's
-  // expansion.)
-  return {*std::move(Macro)};
-
-  ASTNodeKind NodeKind;
-  auto ASTResults = locateASTReferent(*CurLoc, TouchedIdentifier, AST,
-