[PATCH] D86077: [clangd] Add a way for exporting memory usage metrics

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added inline comments.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:172
+    for (const auto &Elem : LRU)
+      TotalBytes += Elem.second->getUsedBytes();
+    return TotalBytes;
----------------
Any idea how expensive this is? I suppose TUScheduler::update() is rare enough that it's not a big deal?

I ask because of bad experience with estimating memory usage inside critical path on another project ;-)

Maybe we can add a span around this, so we can see if it's expensive in traces? Or maybe that will turn out to be more expensive than memory estimation? 


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1299
+
+  // Update memory usage metrics. Note that these are just estimates.
+  size_t ASTCacheBytes = IdleASTs->getUsedBytes();
----------------
Isn't FD->Worker->update() above async (with correct options)? If so, this is still reading the old data, right?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1311
+  }
+  trace::recordMemoryUsage(PreambleBytes, "preambles");
   return NewFile;
----------------
Would it make sense to collect the number of preambles and ASTs cached at this point as well? Exported under a different metric, but at the same time, to be able to join the data together?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86077