[PATCH] D51539: [clangd] Add symbol slab size to index memory consumption estimates
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 7 01:53:22 PDT 2018
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
More information about the cfe-commits
mailing list