[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