[PATCH] D51539: [clangd] Add symbol slab size to index memory consumption estimates

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

LG with a few nits



================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:121
+size_t FileSymbols::estimateMemoryUsage() const {
+  size_t Result = 0;
+  for (const auto &FileAndSlab : FileToSlabs)
----------------
this isn't safe as it doesn't acquire the mutex


================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:144
+size_t FileIndex::estimateMemoryUsage() const {
+  return FSymbols.estimateMemoryUsage();
+}
----------------
ioeric wrote:
> This can be a bit tricky. Generally, the size of a `FileIndex` is the size of FSymbols plus the overhead of the underlying index, but there can be multiple snapshots of symbols in `SwapIndex`. I think we can treat them as burst and only consider the size of the latest snapshot. 
> 
> `FSymbols` and the index have shared ownership to the symbol corpus. We can make either of them bookkeep the size, but I think it might be more straightforward to let the index "own" the size. You could set the payload size when creating the index, and you wouldn't need to expose `estimateMemoryUsage` from `FSymbols`.
(I had trouble parsing this at first - If I understand right, you want the *memIndex* to own the size, and FileIndex to just inherit SwapIndex::estimateMemoryUsage which delegates to MemIndex::estimateMemoryUsage?)

I like this idea, it's simpler and also means one less place you need to lock, which is always nice.


================
Comment at: clang-tools-extra/clangd/index/MemIndex.h:32
+  MemIndex(SymbolRange &&Symbols, OccurrenceMap Occurrences,
+           size_t BackingMemory = 0)
+      : Occurrences(std::move(Occurrences)), BackingMemory(BackingMemory) {
----------------
I don't think defaulting this parameter makes sense, seems like an easy source of bugs
(and in 3 other cases)


================
Comment at: clang-tools-extra/clangd/index/MemIndex.h:70
   std::shared_ptr<void> KeepAlive; // poor man's move-only std::any
+  /// When paired with the SymbolSlab, the index owns underlying SymbolSlab and
+  /// keeps it alive. BackingMemory is the only way to know the size of that
----------------
just `// Size of memory retained by KeepAlive`?
Don't mention Slab, this class doesn't know about it.

(similar in Dex)


https://reviews.llvm.org/D51539