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