[PATCH] D51539: [clangd] Add symbol slab size to index memory consumption estimates
Eric Liu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 7 01:33:38 PDT 2018
ioeric added inline comments.
================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:144
+size_t FileIndex::estimateMemoryUsage() const {
+ return FSymbols.estimateMemoryUsage();
+}
----------------
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`.
================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:44
+ DexIndex(Range &&Symbols, llvm::ArrayRef<std::string> URISchemes,
+ size_t BackingMemory = 0)
+ : BackingMemory(BackingMemory), URISchemes(URISchemes) {
----------------
For this constructor, the index doesn't own the backing data. We should probably not include it in the estimation?
(same for MemIndex)
================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:53
DexIndex(Range &&Symbols, Payload &&BackingData,
- llvm::ArrayRef<std::string> URISchemes)
- : DexIndex(std::forward<Range>(Symbols), URISchemes) {
+ llvm::ArrayRef<std::string> URISchemes, size_t BackingMemory = 0)
+ : DexIndex(std::forward<Range>(Symbols), URISchemes, BackingMemory) {
----------------
nit: maybe call `BackingDataSize`? `BackingMemory` sounds like the actual memory. Maybe also put the parameter right after `BackingData`, as they are closely related?
(same for MemIndex)
================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.h:100
+ /// Slab.
+ size_t BackingMemory;
----------------
nit: `=0`?
https://reviews.llvm.org/D51539
More information about the cfe-commits
mailing list