[clangd] Record memory usages after each notification

[kadircet]

Summary

Depends on D88415

[kadircet]

• Rebase

[kadircet]

• Rebase and add tests for ClangdLSPServer

[sammccall]

clang-tools-extra/clangd/ClangdLSPServer.cpp

171

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

172

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

179

after each notification?

181

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.

184

(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?

[sammccall]

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.

[kadircet]

• Address comments

[kadircet]

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?

clang-tools-extra/clangd/ClangdLSPServer.cpp

171

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.

184

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

[sammccall]

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 10000 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)

clang-tools-extra/clangd/ClangdLSPServer.cpp

153

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)

184

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

[kadircet]

• Implement periodic profiling

[kadircet]

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?)

clang-tools-extra/clangd/ClangdLSPServer.cpp

184

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.

[sammccall]

clang-tools-extra/clangd/ClangdLSPServer.cpp

1242

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()?

1242

Detailed is always false, drop the parameter? This can't be reused by code wanting to e.g. service a code action anyway.

1432

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

clang-tools-extra/clangd/ClangdLSPServer.h

173

or NextProfileTime? (to avoid the negation)

[kadircet]

• Separate profiling and exporting into 2 functions
• Rebase

[sammccall]

clang-tools-extra/clangd/ClangdLSPServer.h

72

drop the no-op part?

[kadircet]

• Update stale comment

clang-tools-extra/clangd/ClangdLSPServer.h

 #include "ClangdServer.h"
 #include "DraftStore.h"
 #include "Features.inc"
 #include "FindSymbols.h"
 #include "GlobalCompilationDatabase.h"
 #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 <chrono>
 #include <memory>
 
 namespace clang {
 namespace clangd {
 
 class SymbolIndex;
 
 /// This class exposes ClangdServer's capabilities via Language Server Protocol.
public:
   ~ClangdLSPServer();
 
   /// Run LSP server loop, communicating with the Transport provided in the
   /// constructor. This method must not be executed more than once.
   ///
   /// \return Whether we shut down cleanly with a 'shutdown' -> 'exit' sequence.
   bool run();
 
+  /// Profiles resource-usage.
+  void profile(MemoryTree &MT) const;
+
 private:
   // Implement ClangdServer::Callbacks.
   void onDiagnosticsReady(PathRef File, llvm::StringRef Version,
                           std::vector<Diag> Diagnostics) override;
   void onFileUpdated(PathRef File, const TUStatus &Status) override;
   void
   onHighlightingsReady(PathRef File, llvm::StringRef Version,
                        std::vector<HighlightingToken> Highlightings) override;
private:
 
   /// Sends a "publishSemanticHighlighting" notification to the LSP client.
   void
   publishTheiaSemanticHighlighting(const TheiaSemanticHighlightingParams &);
 
   /// 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 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
   /// same state.
   Context BackgroundContext;
 
   /// Used to indicate that the 'shutdown' request was received from the
   /// Language Server client.

clang-tools-extra/clangd/ClangdLSPServer.cpp

 //===--- ClangdLSPServer.cpp - LSP server ------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangdLSPServer.h"
+#include "ClangdServer.h"
 #include "CodeComplete.h"
 #include "Diagnostics.h"
 #include "DraftStore.h"
 #include "GlobalCompilationDatabase.h"
 #include "Protocol.h"
 #include "SemanticHighlighting.h"
 #include "SourceCode.h"
 #include "TUScheduler.h"
 #include "URI.h"
 #include "refactor/Tweak.h"
 #include "support/Context.h"
+#include "support/MemoryTree.h"
 #include "support/Trace.h"
 #include "clang/Basic/Version.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator_range.h"
+#include "llvm/Support/Allocator.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SHA1.h"
 #include "llvm/Support/ScopedPrinter.h"
+#include <chrono>
 #include <cstddef>
 #include <memory>
 #include <mutex>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
llvm::Error validateEdits(const DraftStore &DraftMgr, const FileEdits &FE) {
   }
   if (!InvalidFileCount)
     return llvm::Error::success();
   if (InvalidFileCount == 1)
     return error("File must be saved first: {0}", LastInvalidFile);
   return error("Files must be saved first: {0} (and {1} others)",
                LastInvalidFile, InvalidFileCount - 1);
 }
-
 } // namespace
 
 // MessageHandler dispatches incoming LSP messages.
 // It handles cross-cutting concerns:
 //  - serializes/deserializes protocol objects to JSON
 //  - logging of inbound messages
 //  - cancellation handling
 //  - basic call tracing
 // MessageHandler ensures that initialize() is called before any other handler.
 class ClangdLSPServer::MessageHandler : public Transport::MessageHandler {
 public:
   MessageHandler(ClangdLSPServer &Server) : Server(Server) {}
 
   bool onNotify(llvm::StringRef Method, llvm::json::Value Params) override {
     WithContext HandlerContext(handlerContext());
     log("<-- {0}", Method);
     if (Method == "exit")
       return false;
-    if (!Server.Server)
+    if (!Server.Server) {
       elog("Notification {0} before initialization", Method);
-    else if (Method == "$/cancelRequest")
+    } else if (Method == "$/cancelRequest") {
       onCancel(std::move(Params));
-    else if (auto Handler = Notifications.lookup(Method))
+    } else if (auto Handler = Notifications.lookup(Method)) {
       Handler(std::move(Params));
-    else
+      Server.maybeExportMemoryProfile();
+    } else {
       log("unhandled notification {0}", Method);
+    }
     return true;
   }
 
   bool onCall(llvm::StringRef Method, llvm::json::Value Params,
               llvm::json::Value ID) override {
     WithContext HandlerContext(handlerContext());
     // Calls can be canceled by the client. Add cancellation context.
     WithContext WithCancel(cancelableRequestContext(ID));
     trace::Span Tracer(Method, LSPLatency);
     SPAN_ATTACH(Tracer, "Params", Params);
     ReplyOnce Reply(ID, Method, &Server, Tracer.Args);
     log("<-- {0}({1})", Method, ID);
     if (!Server.Server && Method != "initialize") {
       elog("Call {0} before initialization.", Method);
void ClangdLSPServer::publishTheiaSemanticHighlighting(
   notify("textDocument/semanticHighlighting", Params);
 }
 
 void ClangdLSPServer::publishDiagnostics(
     const PublishDiagnosticsParams &Params) {
   notify("textDocument/publishDiagnostics", Params);
 }
 
+void ClangdLSPServer::maybeExportMemoryProfile() {
+  if (!trace::enabled())
+    return;
+  // Profiling might be expensive, so we throttle it to happen once every 5
+  // minutes.
+  static constexpr auto ProfileInterval = std::chrono::minutes(5);
+  auto Now = std::chrono::steady_clock::now();
+  if (Now < NextProfileTime)
+    return;
+
+  static constexpr trace::Metric MemoryUsage(
+      "memory_usage", trace::Metric::Value, "component_name");
+  trace::Span Tracer("ProfileBrief");
+  MemoryTree MT;
+  profile(MT);
+  record(MT, "clangd_lsp_server", MemoryUsage);
+  NextProfileTime = Now + ProfileInterval;
+}
+
 // FIXME: This function needs to be properly tested.
 void ClangdLSPServer::onChangeConfiguration(
     const DidChangeConfigurationParams &Params) {
   applyConfiguration(Params.settings);
 }
 
 void ClangdLSPServer::onReference(const ReferenceParams &Params,
                                   Callback<std::vector<Location>> Reply) {
ClangdLSPServer::ClangdLSPServer(class Transport &Transp,
   MsgHandler->bind("typeHierarchy/resolve", &ClangdLSPServer::onResolveTypeHierarchy);
   MsgHandler->bind("textDocument/selectionRange", &ClangdLSPServer::onSelectionRange);
   MsgHandler->bind("textDocument/documentLink", &ClangdLSPServer::onDocumentLink);
   MsgHandler->bind("textDocument/semanticTokens/full", &ClangdLSPServer::onSemanticTokens);
   MsgHandler->bind("textDocument/semanticTokens/full/delta", &ClangdLSPServer::onSemanticTokensDelta);
   if (Opts.FoldingRanges)
     MsgHandler->bind("textDocument/foldingRange", &ClangdLSPServer::onFoldingRange);
   // clang-format on
+
+  // Delay first profile until we've finished warming up.
+  NextProfileTime = std::chrono::steady_clock::now() + std::chrono::minutes(1);
 }
 
 ClangdLSPServer::~ClangdLSPServer() {
   IsBeingDestroyed = true;
   // Explicitly destroy ClangdServer first, blocking on threads it owns.
   // This ensures they don't access any other members.
   Server.reset();
 }
 
 bool ClangdLSPServer::run() {
   // Run the Language Server loop.
   bool CleanExit = true;
   if (auto Err = Transp.loop(*MsgHandler)) {
     elog("Transport error: {0}", std::move(Err));
     CleanExit = false;
   }
 
   return CleanExit && ShutdownRequestReceived;
 }
 
+void ClangdLSPServer::profile(MemoryTree &MT) const {
+  if (Server)
+    Server->profile(MT.child("clangd_server"));
+}
+
 std::vector<Fix> ClangdLSPServer::getFixes(llvm::StringRef File,
                                            const clangd::Diagnostic &D) {
   std::lock_guard<std::mutex> Lock(FixItsMutex);
   auto DiagToFixItsIter = FixItsMap.find(File);
   if (DiagToFixItsIter == FixItsMap.end())
     return {};
 
   const auto &DiagToFixItsMap = DiagToFixItsIter->second;

clang-tools-extra/clangd/ClangdServer.h

 #include "XRefs.h"
 #include "index/Background.h"
 #include "index/FileIndex.h"
 #include "index/Index.h"
 #include "refactor/Rename.h"
 #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"
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include <functional>
public:
   /// FIXME: those metrics might be useful too, we should add them.
   llvm::StringMap<TUScheduler::FileStats> fileStats() const;
 
   // Blocks the main thread until the server is idle. Only for use in tests.
   // Returns false if the timeout expires.
   LLVM_NODISCARD bool
   blockUntilIdleForTest(llvm::Optional<double> TimeoutSeconds = 10);
 
+  /// Builds a nested representation of memory used by components.
+  void profile(MemoryTree &MT) const;
+
 private:
   void formatCode(PathRef File, llvm::StringRef Code,
                   ArrayRef<tooling::Range> Ranges,
                   Callback<tooling::Replacements> CB);
 
   /// Derives a context for a task processing the specified source file.
   /// This includes the current configuration (see Options::ConfigProvider).
   /// The empty string means no particular file is the target.

clang-tools-extra/clangd/ClangdServer.cpp

 #include "XRefs.h"
 #include "index/CanonicalIncludes.h"
 #include "index/FileIndex.h"
 #include "index/Merge.h"
 #include "refactor/Rename.h"
 #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"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"

 
 LLVM_NODISCARD bool
 ClangdServer::blockUntilIdleForTest(llvm::Optional<double> TimeoutSeconds) {
   return WorkScheduler.blockUntilIdle(timeoutSeconds(TimeoutSeconds)) &&
          (!BackgroundIdx ||
           BackgroundIdx->blockUntilIdleForTest(TimeoutSeconds));
 }
 
+void ClangdServer::profile(MemoryTree &MT) 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

clang-tools-extra/clangd/unittests/ClangdTests.cpp

 #include "CodeComplete.h"
 #include "ConfigFragment.h"
 #include "GlobalCompilationDatabase.h"
 #include "Matchers.h"
 #include "SyncAPI.h"
 #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"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #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"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <algorithm>
 #include <chrono>
 #include <iostream>
 #include <random>
 #include <string>
 #include <thread>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
 namespace {
 
 using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::Gt;
 using ::testing::IsEmpty;
 using ::testing::Pair;
 using ::testing::SizeIs;
 using ::testing::UnorderedElementsAre;
 
void foo() {
       Foo y = std::move(x);
       Foo z = x;
     })cpp";
   Server.addDocument(testPath("foo.h"), SourceContents);
   ASSERT_TRUE(Server.blockUntilIdleForTest());
   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(&Alloc);
+  Server.profile(MT);
+  ASSERT_TRUE(MT.children().count("tuscheduler"));
+  EXPECT_TRUE(MT.child("tuscheduler").children().count(FooCpp));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang