[PATCH] D88417: [clangd] Record memory usages after each notification

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG35871fde55ac: [clangd] Record memory usages after each 
notification (authored by kadircet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  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
@@ -17,6 +17,7 @@
 #include "TestFS.h"
 #include "TestTU.h"
 #include "URI.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "support/Threading.h"
 #include "clang/Config/config.h"
@@ -27,6 +28,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Allocator.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
@@ -48,6 +50,7 @@
 namespace {
 
 using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::Gt;
@@ -1236,6 +1239,21 @@
   EXPECT_FALSE(DiagConsumer.HadDiagsInLastCallback);
 }
 
+TEST(ClangdServer, MemoryUsageTest) {
+  MockFS FS;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  Server.addDocument(FooCpp, "");
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+
+  llvm::BumpPtrAllocator Alloc;
+  MemoryTree MT();
+  Server.profile(MT);
+  ASSERT_TRUE(MT.children().count("tuscheduler"));
+  EXPECT_TRUE(MT.child("tuscheduler").children().count(FooCpp));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -25,6 +25,7 @@
 #include "refactor/Tweak.h"
 #include "support/Cancellation.h"
 #include "support/Function.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
@@ -337,6 +338,9 @@
   LLVM_NODISCARD bool
   blockUntilIdleForTest(llvm::Optional TimeoutSeconds = 10);
 
+  /// Builds a nested representation of memory used by components.
+  void profile(MemoryTree ) const;
+
 private:
   void formatCode(PathRef File, llvm::StringRef Code,
   ArrayRef Ranges,
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -28,6 +28,7 @@
 #include "refactor/Tweak.h"
 #include "support/Logger.h"
 #include "support/Markup.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
 #include "clang/Format/Format.h"
@@ -826,5 +827,12 @@
   BackgroundIdx->blockUntilIdleForTest(TimeoutSeconds));
 }
 
+void ClangdServer::profile(MemoryTree ) const {
+  if (DynamicIdx)
+DynamicIdx->profile(MT.child("dynamic_index"));
+  if (BackgroundIdx)
+BackgroundIdx->profile(MT.child("background_index"));
+  WorkScheduler.profile(MT.child("tuscheduler"));
+}
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -17,11 +17,13 @@
 #include "Protocol.h"
 #include "Transport.h"
 #include "support/Context.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/JSON.h"
+#include 
 #include 
 
 namespace clang {
@@ -67,6 +69,9 @@
   /// \return Whether we shut down cleanly with a 'shutdown' -> 'exit' sequence.
   bool run();
 
+  /// Profiles resource-usage.
+  void profile(MemoryTree ) const;
+
 private:
   // Implement ClangdServer::Callbacks.
   void onDiagnosticsReady(PathRef File, llvm::StringRef Version,
@@ -160,6 +165,14 @@
   /// Sends a "publishDiagnostics" notification to the LSP client.
   void publishDiagnostics(const PublishDiagnosticsParams &);
 
+  /// Runs profiling and exports memory usage metrics if tracing is enabled and
+  /// profiling hasn't happened recently.
+  void maybeExportMemoryProfile();
+
+  /// Timepoint until which profiling is off. It is used to throttle profiling
+  /// requests.
+  

[PATCH] D88417: [clangd] Record memory usages after each notification

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 297568.
kadircet marked an inline comment as done.
kadircet added a comment.

- Update stale comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  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
@@ -17,6 +17,7 @@
 #include "TestFS.h"
 #include "TestTU.h"
 #include "URI.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "support/Threading.h"
 #include "clang/Config/config.h"
@@ -27,6 +28,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Allocator.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
@@ -48,6 +50,7 @@
 namespace {
 
 using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::Gt;
@@ -1236,6 +1239,21 @@
   EXPECT_FALSE(DiagConsumer.HadDiagsInLastCallback);
 }
 
+TEST(ClangdServer, MemoryUsageTest) {
+  MockFS FS;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  Server.addDocument(FooCpp, "");
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+
+  llvm::BumpPtrAllocator Alloc;
+  MemoryTree MT();
+  Server.profile(MT);
+  ASSERT_TRUE(MT.children().count("tuscheduler"));
+  EXPECT_TRUE(MT.child("tuscheduler").children().count(FooCpp));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -25,6 +25,7 @@
 #include "refactor/Tweak.h"
 #include "support/Cancellation.h"
 #include "support/Function.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
@@ -337,6 +338,9 @@
   LLVM_NODISCARD bool
   blockUntilIdleForTest(llvm::Optional TimeoutSeconds = 10);
 
+  /// Builds a nested representation of memory used by components.
+  void profile(MemoryTree ) const;
+
 private:
   void formatCode(PathRef File, llvm::StringRef Code,
   ArrayRef Ranges,
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -28,6 +28,7 @@
 #include "refactor/Tweak.h"
 #include "support/Logger.h"
 #include "support/Markup.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
 #include "clang/Format/Format.h"
@@ -826,5 +827,12 @@
   BackgroundIdx->blockUntilIdleForTest(TimeoutSeconds));
 }
 
+void ClangdServer::profile(MemoryTree ) const {
+  if (DynamicIdx)
+DynamicIdx->profile(MT.child("dynamic_index"));
+  if (BackgroundIdx)
+BackgroundIdx->profile(MT.child("background_index"));
+  WorkScheduler.profile(MT.child("tuscheduler"));
+}
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -17,11 +17,13 @@
 #include "Protocol.h"
 #include "Transport.h"
 #include "support/Context.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/JSON.h"
+#include 
 #include 
 
 namespace clang {
@@ -67,6 +69,9 @@
   /// \return Whether we shut down cleanly with a 'shutdown' -> 'exit' sequence.
   bool run();
 
+  /// Profiles resource-usage.
+  void profile(MemoryTree ) const;
+
 private:
   // Implement ClangdServer::Callbacks.
   void onDiagnosticsReady(PathRef File, llvm::StringRef Version,
@@ -160,6 +165,14 @@
   /// Sends a "publishDiagnostics" notification to the LSP client.
   void publishDiagnostics(const PublishDiagnosticsParams &);
 
+  /// Runs profiling and exports memory usage metrics if tracing is enabled and
+  /// profiling hasn't happened recently.
+  void maybeExportMemoryProfile();
+
+  /// Timepoint until which profiling is off. It is used to throttle profiling
+  /// requests.
+  std::chrono::steady_clock::time_point NextProfileTime;
+
   /// Since initialization of CDBs and ClangdServer 

[PATCH] D88417: [clangd] Record memory usages after each notification

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:72
 
+  /// Profiles resource-usage. No-op if there's no active tracer.
+  void profile(MemoryTree ) const;

drop the no-op part?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

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

[PATCH] D88417: [clangd] Record memory usages after each notification

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 297565.
kadircet marked 5 inline comments as done.
kadircet added a comment.

- Separate profiling and exporting into 2 functions
- Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  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
@@ -17,6 +17,7 @@
 #include "TestFS.h"
 #include "TestTU.h"
 #include "URI.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "support/Threading.h"
 #include "clang/Config/config.h"
@@ -27,6 +28,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Allocator.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
@@ -48,6 +50,7 @@
 namespace {
 
 using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::Gt;
@@ -1236,6 +1239,21 @@
   EXPECT_FALSE(DiagConsumer.HadDiagsInLastCallback);
 }
 
+TEST(ClangdServer, MemoryUsageTest) {
+  MockFS FS;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  Server.addDocument(FooCpp, "");
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+
+  llvm::BumpPtrAllocator Alloc;
+  MemoryTree MT();
+  Server.profile(MT);
+  ASSERT_TRUE(MT.children().count("tuscheduler"));
+  EXPECT_TRUE(MT.child("tuscheduler").children().count(FooCpp));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -25,6 +25,7 @@
 #include "refactor/Tweak.h"
 #include "support/Cancellation.h"
 #include "support/Function.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
@@ -337,6 +338,9 @@
   LLVM_NODISCARD bool
   blockUntilIdleForTest(llvm::Optional TimeoutSeconds = 10);
 
+  /// Builds a nested representation of memory used by components.
+  void profile(MemoryTree ) const;
+
 private:
   void formatCode(PathRef File, llvm::StringRef Code,
   ArrayRef Ranges,
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -28,6 +28,7 @@
 #include "refactor/Tweak.h"
 #include "support/Logger.h"
 #include "support/Markup.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
 #include "clang/Format/Format.h"
@@ -826,5 +827,12 @@
   BackgroundIdx->blockUntilIdleForTest(TimeoutSeconds));
 }
 
+void ClangdServer::profile(MemoryTree ) const {
+  if (DynamicIdx)
+DynamicIdx->profile(MT.child("dynamic_index"));
+  if (BackgroundIdx)
+BackgroundIdx->profile(MT.child("background_index"));
+  WorkScheduler.profile(MT.child("tuscheduler"));
+}
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -17,11 +17,13 @@
 #include "Protocol.h"
 #include "Transport.h"
 #include "support/Context.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/JSON.h"
+#include 
 #include 
 
 namespace clang {
@@ -67,6 +69,9 @@
   /// \return Whether we shut down cleanly with a 'shutdown' -> 'exit' sequence.
   bool run();
 
+  /// Profiles resource-usage. No-op if there's no active tracer.
+  void profile(MemoryTree ) const;
+
 private:
   // Implement ClangdServer::Callbacks.
   void onDiagnosticsReady(PathRef File, llvm::StringRef Version,
@@ -160,6 +165,14 @@
   /// Sends a "publishDiagnostics" notification to the LSP client.
   void publishDiagnostics(const PublishDiagnosticsParams &);
 
+  /// Runs profiling and exports memory usage metrics if tracing is enabled and
+  /// profiling hasn't happened recently.
+  void maybeExportMemoryProfile();
+
+  /// Timepoint until which profiling is off. It is used to throttle profiling
+  /// requests.
+  

[PATCH] D88417: [clangd] Record memory usages after each notification

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1234
 
+void ClangdLSPServer::profile(bool Detailed) {
+  if (!trace::enabled())

ClangdLSPServer does own things other than ClangdServer (e.g. the CDB), though 
we don't yet profile them.
Does it make sense to split this into ClangdLSPServer::profile (with the usual 
signature, and maybe public?) and ClangdLSPServer::maybeExportMemoryProfile()?



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1234
 
+void ClangdLSPServer::profile(bool Detailed) {
+  if (!trace::enabled())

sammccall wrote:
> ClangdLSPServer does own things other than ClangdServer (e.g. the CDB), 
> though we don't yet profile them.
> Does it make sense to split this into ClangdLSPServer::profile (with the 
> usual signature, and maybe public?) and 
> ClangdLSPServer::maybeExportMemoryProfile()?
Detailed is always false, drop the parameter? This can't be reused by code 
wanting to e.g. service a code action anyway.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1424
+
+  // Delay profiling for a minute, as serious allocations don't happen at
+  // startup.

As written this isn't clearly true or false (what exactly is "startup"?)
Maybe just "Delay first profile until we've finished warming up"?



Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:165
+  /// requests.
+  std::chrono::steady_clock::time_point NoProfileBefore;
+

or NextProfileTime? (to avoid the negation)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

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

[PATCH] D88417: [clangd] Record memory usages after each notification

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 297173.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Implement periodic profiling


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  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
@@ -17,6 +17,7 @@
 #include "TestFS.h"
 #include "TestTU.h"
 #include "URI.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "support/Threading.h"
 #include "clang/Config/config.h"
@@ -27,6 +28,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Allocator.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
@@ -48,6 +50,7 @@
 namespace {
 
 using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::Gt;
@@ -1236,6 +1239,21 @@
   EXPECT_FALSE(DiagConsumer.HadDiagsInLastCallback);
 }
 
+TEST(ClangdServer, MemoryUsageTest) {
+  MockFS FS;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  Server.addDocument(FooCpp, "");
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+
+  llvm::BumpPtrAllocator Alloc;
+  MemoryTree MT();
+  Server.profile(MT);
+  ASSERT_TRUE(MT.children().count("tuscheduler"));
+  EXPECT_TRUE(MT.child("tuscheduler").children().count(FooCpp));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -25,6 +25,7 @@
 #include "refactor/Tweak.h"
 #include "support/Cancellation.h"
 #include "support/Function.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
@@ -340,6 +341,9 @@
   LLVM_NODISCARD bool
   blockUntilIdleForTest(llvm::Optional TimeoutSeconds = 10);
 
+  /// Builds a nested representation of memory used by components.
+  void profile(MemoryTree ) const;
+
 private:
   void formatCode(PathRef File, llvm::StringRef Code,
   ArrayRef Ranges,
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -28,6 +28,7 @@
 #include "refactor/Tweak.h"
 #include "support/Logger.h"
 #include "support/Markup.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
 #include "clang/Format/Format.h"
@@ -824,5 +825,12 @@
   BackgroundIdx->blockUntilIdleForTest(TimeoutSeconds));
 }
 
+void ClangdServer::profile(MemoryTree ) const {
+  if (DynamicIdx)
+DynamicIdx->profile(MT.child("dynamic_index"));
+  if (BackgroundIdx)
+BackgroundIdx->profile(MT.child("background_index"));
+  WorkScheduler.profile(MT.child("tuscheduler"));
+}
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -22,6 +22,7 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/JSON.h"
+#include 
 #include 
 
 namespace clang {
@@ -156,6 +157,13 @@
   /// Sends a "publishDiagnostics" notification to the LSP client.
   void publishDiagnostics(const PublishDiagnosticsParams &);
 
+  /// Profiles resource-usage. No-op if there's no active tracer.
+  void profile(bool Detailed);
+
+  /// Timepoint until which profiling is off. It is used to throttle profiling
+  /// requests.
+  std::chrono::steady_clock::time_point NoProfileBefore;
+
   /// 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
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "ClangdLSPServer.h"
+#include 

[PATCH] D88417: [clangd] Record memory usages after each notification

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

> What about something like a 5 minute throttle, but have ClangdLSPServer's 
> constructor set the timestamp to now+1 minute? (Without profiling)

SGTM. Note that this means we can't easily test this in LSP layer anymore. 
(We've got couple of components depending on time, maybe it is time we have a 
"mock" clock?)




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:182
+Server.Server->profile(MT);
+trace::recordMemoryUsage(MT, "clangd_server");
 return true;

sammccall wrote:
> kadircet wrote:
> > sammccall wrote:
> > > (Sorry, I suspect we discussed this and I forgot)
> > > Is there a reason at this point to put knowledge of the core metric in 
> > > trace:: rather than define it here locally in ClangdLSPServer?
> > > (Sorry, I suspect we discussed this and I forgot)
> > 
> > Not really.
> > 
> > > Is there a reason at this point to put knowledge of the core metric in 
> > > trace:: rather than define it here locally in ClangdLSPServer?
> > 
> > `ClangdLSPServer` didnt feel like the appropriate place for that logic. 
> > Moreover other embedders of ClangdServer could benefit from traversal logic 
> > if it is defined in a lower level than ClangdLSPServer.
> (Sorry, I guess D88413 was really the place for this discussion)
> 
> > ClangdLSPServer didnt feel like the appropriate place for that logic.
> 
> To me, putting this in ClangdLSPServer feels like "this isn't part of the 
> central mission here, it could be split out for coherence/reuse".
> Whereas putting it in support/Trace feels like "this is a layering violation".
> 
> > other embedders of ClangdServer could benefit from traversal logic
> 
> If exporting memorytree->metric with the path as the label is something we 
> want to reuse, that could be a function in MemoryTree.h (taking the metric as 
> parameter) or we can include the generic traversal function you proposed 
> earlier. Even though they're both in Support, layering MemoryTree above 
> Tracer seems more appropriate than the other way around.
> My instinct is this is premature generalization and having a traversal in 
> each embedder is fine, though.
> If exporting memorytree->metric with the path as the label is something we 
> want to reuse, that could be a function in MemoryTree.h (taking the metric as 
> parameter) or we can include the generic traversal function you proposed 
> earlier. Even though they're both in Support, layering MemoryTree above 
> Tracer seems more appropriate than the other way around.

Added a `record` member to MemoryTree in D88413.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

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

[PATCH] D88417: [clangd] Record memory usages after each notification

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D88417#2319307 , @kadircet wrote:

> Bad news, I was testing this with remote-index, hence background-index was 
> turned off. Unfortunately traversing all of the slabs in `FileSymbols` takes 
> quite a while in this case (~15ms for LLVM).
>
> I don't think it is feasible to do this on every notification now, as this 
> implies an extra 15ms latency for interactive requests like code 
> completion/signature help due to the delay between didChange notification and 
> codeCompletion request.

Yeah, 15ms is a lot.
Staring at the code, I think this is our unfortunate index allocation strategy: 
for each source file in the project, we have one slab allocator for symbols, 
one for refs, one for relations...
The factor of 3 is sad but the factor of 1 is what's killing us :-)

So we should fix that someday (DBMS?) but for now, let's back off to measuring 
once every several minutes.

The main problem I see is that we're almost guaranteed to be "unlucky" with the 
sample that way.
e.g. delay = 5 min. On startup there's some notification (initialized? 
didOpen?) that happens before any of the serious allocation and resets the 
counter.
So we won't have a realistic memory usage until we hit the 5 minute mark and 
profile again. If many sessions are <5min then our averages are going to be way 
off.

I can't think of a notification that is ubiquitous but only fires after serious 
work has happened. (Well, publishDiagnostics, but that goes in the wrong 
direction and so runs on the wrong thread).

What about something like a 5 minute throttle, but have ClangdLSPServer's 
constructor set the timestamp to now+1 minute? (Without profiling)




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:152
+  trace::Span Tracer(Detailed ? "ProfileDetailed" : "ProfileBrief");
+  // Exit early if tracing is disabled.
+  if (!Tracer.Args)

I was about to complain that this doesn't work, but it actually does...

This wasn't the intended design of trace/span :-(
The idea was that `Args` would be null if the tracer dynamically wasn't 
interested in event details (e.g. an implementation like CSVMetricsTracer that 
doesn't record event payloads, or a sampling tracer).
In this case !Args would have false positives.

Do you mind adding an explicit trace::enabled() method to Trace.h instead, to 
be more explicit? I'd like to fix the Trace api.
(Else leave as-is and I can do this as a followup)



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:182
+Server.Server->profile(MT);
+trace::recordMemoryUsage(MT, "clangd_server");
 return true;

kadircet wrote:
> sammccall wrote:
> > (Sorry, I suspect we discussed this and I forgot)
> > Is there a reason at this point to put knowledge of the core metric in 
> > trace:: rather than define it here locally in ClangdLSPServer?
> > (Sorry, I suspect we discussed this and I forgot)
> 
> Not really.
> 
> > Is there a reason at this point to put knowledge of the core metric in 
> > trace:: rather than define it here locally in ClangdLSPServer?
> 
> `ClangdLSPServer` didnt feel like the appropriate place for that logic. 
> Moreover other embedders of ClangdServer could benefit from traversal logic 
> if it is defined in a lower level than ClangdLSPServer.
(Sorry, I guess D88413 was really the place for this discussion)

> ClangdLSPServer didnt feel like the appropriate place for that logic.

To me, putting this in ClangdLSPServer feels like "this isn't part of the 
central mission here, it could be split out for coherence/reuse".
Whereas putting it in support/Trace feels like "this is a layering violation".

> other embedders of ClangdServer could benefit from traversal logic

If exporting memorytree->metric with the path as the label is something we want 
to reuse, that could be a function in MemoryTree.h (taking the metric as 
parameter) or we can include the generic traversal function you proposed 
earlier. Even though they're both in Support, layering MemoryTree above Tracer 
seems more appropriate than the other way around.
My instinct is this is premature generalization and having a traversal in each 
embedder is fine, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

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

[PATCH] D88417: [clangd] Record memory usages after each notification

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet requested review of this revision.
kadircet added a comment.

Bad news, I was testing this with remote-index, hence background-index was 
turned off. Unfortunately traversing all of the slabs in `FileSymbols` takes 
quite a while in this case (~15ms for LLVM).

I don't think it is feasible to do this on every notification now, as this 
implies an extra 15ms latency for interactive requests like code 
completion/signature help due to the delay between didChange notification and 
codeCompletion request.

> We should watch the timing here carefully and consider guarding it - apart 
> from the minimum time interval we discussed, we could have a check whether 
> metric tracing is actually enabled in a meaningful way.

I've also added early exit for non-tracing case. But I think we should still 
change this to be periodic or once every N calls. WDYT?




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:182
+Server.Server->profile(MT);
+trace::recordMemoryUsage(MT, "clangd_server");
 return true;

sammccall wrote:
> (Sorry, I suspect we discussed this and I forgot)
> Is there a reason at this point to put knowledge of the core metric in 
> trace:: rather than define it here locally in ClangdLSPServer?
> (Sorry, I suspect we discussed this and I forgot)

Not really.

> Is there a reason at this point to put knowledge of the core metric in 
> trace:: rather than define it here locally in ClangdLSPServer?

`ClangdLSPServer` didnt feel like the appropriate place for that logic. 
Moreover other embedders of ClangdServer could benefit from traversal logic if 
it is defined in a lower level than ClangdLSPServer.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:168
   elog("Notification {0} before initialization", Method);
-else if (Method == "$/cancelRequest")
+  return true;
+}

sammccall wrote:
> this change is a bit puzzling - makes it look like there are some cases where 
> we specifically want/don't want to record. why?
it was to ensure we have a `ClangdServer` instance we can query for memory 
usage.

will revert as moving profiling into `happy case` makes it obselete.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

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

[PATCH] D88417: [clangd] Record memory usages after each notification

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 296955.
kadircet marked 4 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.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
@@ -17,6 +17,7 @@
 #include "TestFS.h"
 #include "TestTU.h"
 #include "URI.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "support/Threading.h"
 #include "clang/Config/config.h"
@@ -27,6 +28,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Allocator.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
@@ -48,6 +50,7 @@
 namespace {
 
 using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::Gt;
@@ -1236,6 +1239,21 @@
   EXPECT_FALSE(DiagConsumer.HadDiagsInLastCallback);
 }
 
+TEST(ClangdServer, MemoryUsageTest) {
+  MockFS FS;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  Server.addDocument(FooCpp, "");
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+
+  llvm::BumpPtrAllocator Alloc;
+  MemoryTree MT();
+  Server.profile(MT);
+  ASSERT_TRUE(MT.children().count("tuscheduler"));
+  EXPECT_TRUE(MT.child("tuscheduler").children().count(FooCpp));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -164,6 +164,17 @@
   stop();
   EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), testing::SizeIs(1));
 }
+
+TEST_F(LSPTest, RecordsMemoryUsage) {
+  trace::TestTracer Tracer;
+  auto  = start();
+  EXPECT_THAT(Tracer.takeMetric("memory_usage", "clangd_server"),
+  testing::SizeIs(0));
+  Client.notify("", {});
+  stop();
+  EXPECT_THAT(Tracer.takeMetric("memory_usage", "clangd_server"),
+  testing::SizeIs(1));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -25,6 +25,7 @@
 #include "refactor/Tweak.h"
 #include "support/Cancellation.h"
 #include "support/Function.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
@@ -340,6 +341,9 @@
   LLVM_NODISCARD bool
   blockUntilIdleForTest(llvm::Optional TimeoutSeconds = 10);
 
+  /// Builds a nested representation of memory used by components.
+  void profile(MemoryTree ) const;
+
 private:
   void formatCode(PathRef File, llvm::StringRef Code,
   ArrayRef Ranges,
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -28,6 +28,7 @@
 #include "refactor/Tweak.h"
 #include "support/Logger.h"
 #include "support/Markup.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
 #include "clang/Format/Format.h"
@@ -824,5 +825,12 @@
   BackgroundIdx->blockUntilIdleForTest(TimeoutSeconds));
 }
 
+void ClangdServer::profile(MemoryTree ) const {
+  if (DynamicIdx)
+DynamicIdx->profile(MT.child("dynamic_index"));
+  if (BackgroundIdx)
+BackgroundIdx->profile(MT.child("background_index"));
+  WorkScheduler.profile(MT.child("tuscheduler"));
+}
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "ClangdLSPServer.h"
+#include "ClangdServer.h"
 #include "CodeComplete.h"
 #include "Diagnostics.h"
 #include "DraftStore.h"
@@ -26,6 +27,7 @@
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator_range.h"
+#include 

[PATCH] D88417: [clangd] Record memory usages after each notification

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

this is at least going to take some important locks, hopefully only briefly.

We should watch the timing here carefully and consider guarding it - apart from 
the minimum time interval we discussed, we could have a check whether metric 
tracing is actually enabled in a meaningful way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

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

[PATCH] D88417: [clangd] Record memory usages after each notification

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:182
+Server.Server->profile(MT);
+trace::recordMemoryUsage(MT, "clangd_server");
 return true;

(Sorry, I suspect we discussed this and I forgot)
Is there a reason at this point to put knowledge of the core metric in trace:: 
rather than define it here locally in ClangdLSPServer?



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:168
   elog("Notification {0} before initialization", Method);
-else if (Method == "$/cancelRequest")
+  return true;
+}

this change is a bit puzzling - makes it look like there are some cases where 
we specifically want/don't want to record. why?



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:171
+if (Method == "$/cancelRequest")
   onCancel(std::move(Params));
 else if (auto Handler = Notifications.lookup(Method))

on the flip side processing cancellations as fast as possible seems like it 
might be important.

Maybe just move the recording of memory usage to the happy case? (Notification 
that we have a handler for, after the handler).



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:177
+
+// Record memory usage after each memory usage. This currently takes <1ms,
+// so it is safe to do frequently.

after each notification?



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:179
+// so it is safe to do frequently.
+trace::Span Tracer("RecordMemoryUsage");
+MemoryTree MT;

maybe move this into a tiny function? It's self-contained, a bit distracting 
from the fairly important core logic here, and we may well want to do it 
conditionally or in more/different places in future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

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

[PATCH] D88417: [clangd] Record memory usages after each notification

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 296696.
kadircet added a comment.

- Rebase and add tests for ClangdLSPServer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.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
@@ -17,6 +17,7 @@
 #include "TestFS.h"
 #include "TestTU.h"
 #include "URI.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "support/Threading.h"
 #include "clang/Config/config.h"
@@ -27,6 +28,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Allocator.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
@@ -48,6 +50,7 @@
 namespace {
 
 using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::Gt;
@@ -1236,6 +1239,21 @@
   EXPECT_FALSE(DiagConsumer.HadDiagsInLastCallback);
 }
 
+TEST(ClangdServer, MemoryUsageTest) {
+  MockFS FS;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  Server.addDocument(FooCpp, "");
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+
+  llvm::BumpPtrAllocator Alloc;
+  MemoryTree MT();
+  Server.profile(MT);
+  ASSERT_TRUE(MT.children().count("tuscheduler"));
+  EXPECT_TRUE(MT.child("tuscheduler").children().count(FooCpp));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -164,6 +164,17 @@
   stop();
   EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), testing::SizeIs(1));
 }
+
+TEST_F(LSPTest, RecordsMemoryUsage) {
+  trace::TestTracer Tracer;
+  auto  = start();
+  EXPECT_THAT(Tracer.takeMetric("memory_usage", "clangd_server"),
+  testing::SizeIs(0));
+  Client.notify("", {});
+  stop();
+  EXPECT_THAT(Tracer.takeMetric("memory_usage", "clangd_server"),
+  testing::SizeIs(1));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -25,6 +25,7 @@
 #include "refactor/Tweak.h"
 #include "support/Cancellation.h"
 #include "support/Function.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
@@ -337,6 +338,9 @@
   LLVM_NODISCARD bool
   blockUntilIdleForTest(llvm::Optional TimeoutSeconds = 10);
 
+  /// Builds a nested representation of memory used by components.
+  void profile(MemoryTree ) const;
+
 private:
   void formatCode(PathRef File, llvm::StringRef Code,
   ArrayRef Ranges,
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -28,6 +28,7 @@
 #include "refactor/Tweak.h"
 #include "support/Logger.h"
 #include "support/Markup.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
 #include "clang/Format/Format.h"
@@ -822,5 +823,12 @@
   BackgroundIdx->blockUntilIdleForTest(TimeoutSeconds));
 }
 
+void ClangdServer::profile(MemoryTree ) const {
+  if (DynamicIdx)
+DynamicIdx->profile(MT.child("dynamic_index"));
+  if (BackgroundIdx)
+BackgroundIdx->profile(MT.child("background_index"));
+  WorkScheduler.profile(MT.child("tuscheduler"));
+}
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -163,14 +163,23 @@
 log("<-- {0}", Method);
 if (Method == "exit")
   return false;
-if (!Server.Server)
+if (!Server.Server) {
   elog("Notification {0} before initialization", Method);
-else if (Method == "$/cancelRequest")
+  return true;
+}
+if (Method == "$/cancelRequest")
   onCancel(std::move(Params));
 else if (auto Handler = 

[PATCH] D88417: [clangd] Record memory usages after each notification

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 296115.
kadircet added a comment.

- Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  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
@@ -17,6 +17,7 @@
 #include "TestFS.h"
 #include "TestTU.h"
 #include "URI.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "support/Threading.h"
 #include "clang/Config/config.h"
@@ -27,6 +28,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Allocator.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
@@ -48,6 +50,7 @@
 namespace {
 
 using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::Gt;
@@ -1236,6 +1239,31 @@
   EXPECT_FALSE(DiagConsumer.HadDiagsInLastCallback);
 }
 
+TEST(ClangdServer, MemoryUsageTest) {
+  MockFS FS;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, ClangdServer::optsForTest());
+
+  std::vector SeenComponents;
+  auto CB = [](size_t Size, llvm::StringRef CompName) {
+SeenComponents.emplace_back(CompName.str());
+  };
+
+  llvm::BumpPtrAllocator Alloc;
+  MemoryTree MT();
+  Server.attachMemoryUsage(MT);
+  MT.traverseTree(CB, "clangd_server");
+  EXPECT_THAT(SeenComponents, Contains("clangd_server"));
+  SeenComponents.clear();
+
+  auto FooCpp = testPath("foo.cpp");
+  Server.addDocument(FooCpp, "");
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  Server.attachMemoryUsage(MT);
+  MT.traverseTree(CB, "clangd_server");
+  EXPECT_THAT(SeenComponents,
+  Contains("clangd_server.tuscheduler.preambles." + FooCpp));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -25,6 +25,7 @@
 #include "refactor/Tweak.h"
 #include "support/Cancellation.h"
 #include "support/Function.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
@@ -337,6 +338,9 @@
   LLVM_NODISCARD bool
   blockUntilIdleForTest(llvm::Optional TimeoutSeconds = 10);
 
+  /// Builds a nested representation of memory used by components.
+  void attachMemoryUsage(MemoryTree ) const;
+
 private:
   void formatCode(PathRef File, llvm::StringRef Code,
   ArrayRef Ranges,
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -28,6 +28,7 @@
 #include "refactor/Tweak.h"
 #include "support/Logger.h"
 #include "support/Markup.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
 #include "clang/Format/Format.h"
@@ -833,5 +834,12 @@
   BackgroundIdx->blockUntilIdleForTest(TimeoutSeconds));
 }
 
+void ClangdServer::attachMemoryUsage(MemoryTree ) const {
+  if (DynamicIdx)
+DynamicIdx->attachMemoryUsage(*MT.addChild("dynamic_index"));
+  if (BackgroundIdx)
+BackgroundIdx->attachMemoryUsage(*MT.addChild("background_index"));
+  WorkScheduler.attachMemoryUsage(*MT.addChild("tuscheduler"));
+}
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -163,14 +163,23 @@
 log("<-- {0}", Method);
 if (Method == "exit")
   return false;
-if (!Server.Server)
+if (!Server.Server) {
   elog("Notification {0} before initialization", Method);
-else if (Method == "$/cancelRequest")
+  return true;
+}
+if (Method == "$/cancelRequest")
   onCancel(std::move(Params));
 else if (auto Handler = Notifications.lookup(Method))
   Handler(std::move(Params));
 else
   log("unhandled notification {0}", Method);
+
+// Record memory usage after each memory usage. This currently takes <1ms,
+// so it is safe to do frequently.
+trace::Span Tracer("RecordMemoryUsage");
+MemoryTree MT;
+Server.Server->attachMemoryUsage(MT);
+MT.traverseTree(trace::recordMemoryUsage, "clangd_server");
 return true;
   }
 

[PATCH] D88417: [clangd] Record memory usages after each notification

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

Depends on D88415 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88417

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  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
@@ -48,6 +48,7 @@
 namespace {
 
 using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::Gt;
@@ -61,6 +62,16 @@
  Location{URIForFile::canonicalize(File, testRoot()), Range};
 }
 
+MATCHER_P(WithName, Name, "") {
+  if (arg.Name == Name)
+return true;
+  if (auto *Stream = result_listener->stream()) {
+llvm::raw_os_ostream OS(*Stream);
+OS << arg.Name;
+  }
+  return false;
+}
+
 bool diagsContainErrors(const std::vector ) {
   for (auto D : Diagnostics) {
 if (D.Severity == DiagnosticsEngine::Error ||
@@ -1236,6 +1247,35 @@
   EXPECT_FALSE(DiagConsumer.HadDiagsInLastCallback);
 }
 
+TEST(ClangdServer, MemoryUsageTest) {
+  MockFS FS;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, ClangdServer::optsForTest());
+
+  struct Component {
+std::string Name;
+bool Size;
+  };
+  std::vector SeenComponents;
+  auto CB = [](size_t Size, llvm::StringRef CompName) {
+Component C;
+C.Name = CompName.str();
+C.Size = Size;
+llvm::errs() << "Got: " << CompName << '\n';
+SeenComponents.emplace_back(std::move(C));
+  };
+
+  Server.getMemoryUsage().traverseTree(false, CB, "clangd_server");
+  EXPECT_THAT(SeenComponents, Contains(WithName("clangd_server")));
+
+  auto FooCpp = testPath("foo.cpp");
+  Server.addDocument(FooCpp, "");
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  Server.getMemoryUsage().traverseTree(false, CB, "clangd_server");
+  EXPECT_THAT(SeenComponents,
+  Contains(WithName(
+  "clangd_server.dynamic_index.preamble_symbols.foo.cpp")));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -25,6 +25,7 @@
 #include "refactor/Tweak.h"
 #include "support/Cancellation.h"
 #include "support/Function.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
@@ -337,6 +338,9 @@
   LLVM_NODISCARD bool
   blockUntilIdleForTest(llvm::Optional TimeoutSeconds = 10);
 
+  /// Builds a nested representation of memory used by components.
+  MemoryTree getMemoryUsage() const;
+
 private:
   void formatCode(PathRef File, llvm::StringRef Code,
   ArrayRef Ranges,
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -833,5 +833,14 @@
   BackgroundIdx->blockUntilIdleForTest(TimeoutSeconds));
 }
 
+MemoryTree ClangdServer::getMemoryUsage() const {
+  MemoryTree MT;
+  if (DynamicIdx)
+MT.addChild("dynamic_index", DynamicIdx->getMemoryUsage());
+  if (BackgroundIdx)
+MT.addChild("background_index", BackgroundIdx->getMemoryUsage());
+  MT.addChild("tuscheduler", WorkScheduler.getMemoryUsage());
+  return MT;
+}
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -170,14 +170,22 @@
 log("<-- {0}", Method);
 if (Method == "exit")
   return false;
-if (!Server.Server)
+if (!Server.Server) {
   elog("Notification {0} before initialization", Method);
-else if (Method == "$/cancelRequest")
+  return true;
+}
+if (Method == "$/cancelRequest")
   onCancel(std::move(Params));
 else if (auto Handler = Notifications.lookup(Method))
   Handler(std::move(Params));
 else
   log("unhandled notification {0}", Method);
+
+// Record memory usage after each memory usage. This currently takes <1ms,
+// so it is safe to do frequently.
+trace::Span Tracer("RecordMemoryUsage");
+Server.Server->getMemoryUsage().traverseTree(true, trace::recordMemoryUsage,
+ "clangd_server");
 return