[PATCH] D51154: [clangd] Log memory usage of DexIndex and MemIndex

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/index/MemIndex.cpp:105
+size_t MemIndex::estimateMemoryUsage() {
+  size_t Bytes = Index.size() * sizeof(std::pair<SymbolID, const Symbol *>);
+  return Bytes / (1000 * 1000);
----------------
access to Index needs to be guarded by mutex


================
Comment at: clang-tools-extra/clangd/index/MemIndex.cpp:105
+size_t MemIndex::estimateMemoryUsage() {
+  size_t Bytes = Index.size() * sizeof(std::pair<SymbolID, const Symbol *>);
+  return Bytes / (1000 * 1000);
----------------
sammccall wrote:
> access to Index needs to be guarded by mutex
`Index.getMemorySize()` is a better estimate.


================
Comment at: clang-tools-extra/clangd/index/MemIndex.h:43
 private:
+  /// Returns estimate size of the index in megabytes.
+  size_t estimateMemoryUsage();
----------------
Can you return bytes here, and do lossy conversions at display time instead?

(Not sure the conversions are even worth it. e.g. it's may be interesting to compare even <1mb indexes)


================
Comment at: clang-tools-extra/clangd/index/MemIndex.h:44
+  /// Returns estimate size of the index in megabytes.
+  size_t estimateMemoryUsage();
+
----------------
I'd suggest making this part of the public interface, and implementing it for all (the implementations should be trivial).

This is useful e.g. for monitoring. We may want vim's :YcmDebugInfo to return this, etc.


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:178
+size_t DexIndex::estimateMemoryUsage() {
+  size_t Bytes = LookupTable.size() * sizeof(std::pair<SymbolID, const Symbol *>);
+  Bytes += SymbolQuality.size() * sizeof(std::pair<const Symbol *, float>);
----------------
again, LookupTable.getMemorySize() and others.


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:180
+  Bytes += SymbolQuality.size() * sizeof(std::pair<const Symbol *, float>);
+  Bytes += InvertedIndex.size() * sizeof(Token);
+  {
----------------
I think you're not counting the size of the actual symbols.
This is difficult to do precisely, but avoiding it seems misleading (we have "shared ownership" but it's basically exclusive). What's the plan here?


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:182
+  {
+    std::lock_guard<std::mutex> Lock(Mutex);
+
----------------
why is only the InvertedIndex access guarded by the mutex?


https://reviews.llvm.org/D51154