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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
