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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:172
+    for (const auto &Elem : LRU)
+      TotalBytes += Elem.second->getUsedBytes();
+    return TotalBytes;
----------------
adamcz wrote:
> 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? 
> TUScheduler::update() is rare enough

actually it can potentially be triggered after every keystroke. I suppose it might make sense to move this out of the main thread completely. I suppose `runWithPreamble`s action might be a better place to record, which is still quite often btw (every signature help and code completion request goes through it), but runs on a separate thread.

i would expect `that will turn out to be more expensive than memory estimation` to be the case, will do some checks though.

Most of the calculations happening in there is just addition of some members. the source manager makes me afraid a little bit tho, as it seems to be going over all the buffers.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1299
+
+  // Update memory usage metrics. Note that these are just estimates.
+  size_t ASTCacheBytes = IdleASTs->getUsedBytes();
----------------
adamcz wrote:
> Isn't FD->Worker->update() above async (with correct options)? If so, this is still reading the old data, right?
yes that's true. i was aiming for this to be an estimate, rather than an accurate view.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1311
+  }
+  trace::recordMemoryUsage(PreambleBytes, "preambles");
   return NewFile;
----------------
adamcz wrote:
> 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?
yeah that definitely makes sense, but I would rather do that on a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86077