[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:168
 
+  size_t getUsedBytes() {
+    size_t TotalBytes = 0;
----------------
nit: This is the same name as getUsedBytes(Key K). Maybe rename to getTotalUsedBytes()?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:172
+    for (const auto &Elem : LRU)
+      TotalBytes += Elem.second->getUsedBytes();
+    return TotalBytes;
----------------
kadircet wrote:
> 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.
+1 to moving call to this to runWithPreamble or something like that. If a request gets cancelled, for example, there's no need to update the memory usage.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1299
+
+  // Update memory usage metrics. Note that these are just estimates.
+  size_t ASTCacheBytes = IdleASTs->getUsedBytes();
----------------
kadircet wrote:
> 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.
I think this deserves a comment.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1309
+    // fileStats results include ast cache sizes too, subtract them.
+    PreambleBytes -= ASTCacheBytes;
+  }
----------------
So technically this is incorrect.
IdleASTs might contain AST for a file that is no longer tracked (e.g. had removeDocument() called on it). ASTCacheBytes will include it, but PreambleBytes will not, since it only asks for size of the tracked files. It's probably fine, considering how rare this will be and that these are all estimates, but I would recommend a comment, in case someone sees PreambleBytes be negative and wonders what's going on.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1311
+  }
+  trace::recordMemoryUsage(PreambleBytes, "preambles");
   return NewFile;
----------------
kadircet wrote:
> 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.
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86077