[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