This revision was automatically updated to reflect the committed changes.
Closed by commit rL341800: [clangd] Add symbol slab size to index memory
consumption estimates (authored by omtcyfz, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
kbobyrev marked an inline comment as done.
kbobyrev added inline comments.
Comment at: clang-tools-extra/clangd/index/Index.h:512
llvm::function_ref) const override;
+
void lookup(const LookupRequest &,
ioeric wrote:
> nit: I'd avoid
kbobyrev updated this revision to Diff 164653.
kbobyrev marked 2 inline comments as done.
https://reviews.llvm.org/D51539
Files:
clang-tools-extra/clangd/index/FileIndex.cpp
clang-tools-extra/clangd/index/MemIndex.cpp
clang-tools-extra/clangd/index/MemIndex.h
ioeric accepted this revision.
ioeric added inline comments.
This revision is now accepted and ready to land.
Comment at: clang-tools-extra/clangd/index/Index.h:512
llvm::function_ref) const override;
+
void lookup(const LookupRequest &,
kbobyrev updated this revision to Diff 164644.
kbobyrev added a comment.
Remove accidental formatting changes.
https://reviews.llvm.org/D51539
Files:
clang-tools-extra/clangd/index/FileIndex.cpp
clang-tools-extra/clangd/index/MemIndex.cpp
clang-tools-extra/clangd/index/MemIndex.h
kbobyrev updated this revision to Diff 164643.
kbobyrev added a comment.
Sync with HEAD.
https://reviews.llvm.org/D51539
Files:
clang-tools-extra/clangd/index/FileIndex.cpp
clang-tools-extra/clangd/index/Index.cpp
clang-tools-extra/clangd/index/Index.h
kbobyrev updated this revision to Diff 164383.
kbobyrev marked an inline comment as done.
kbobyrev added a comment.
Oh, I thought they're empty.
https://reviews.llvm.org/D51539
Files:
clang-tools-extra/clangd/index/FileIndex.cpp
clang-tools-extra/clangd/index/Index.cpp
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:131
+ size_t StorageSize = 0;
+ for (const auto : SymbolSlabs)
+StorageSize += Slab->bytes();
also the refslabs and refsstorage
https://reviews.llvm.org/D51539
kbobyrev updated this revision to Diff 164377.
kbobyrev marked an inline comment as done.
https://reviews.llvm.org/D51539
Files:
clang-tools-extra/clangd/index/FileIndex.cpp
clang-tools-extra/clangd/index/Index.cpp
clang-tools-extra/clangd/index/Index.h
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:135
+ std::move(RefsStorage)),
+ /*BackingDataSize=*/0);
}
this size should be calculated from the slabs above
https://reviews.llvm.org/D51539
kbobyrev updated this revision to Diff 164369.
kbobyrev marked 9 inline comments as done.
kbobyrev added a comment.
Address a round of comments.
https://reviews.llvm.org/D51539
Files:
clang-tools-extra/clangd/index/FileIndex.cpp
clang-tools-extra/clangd/index/Index.cpp
ioeric added inline comments.
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:144
+size_t FileIndex::estimateMemoryUsage() const {
+ return FSymbols.estimateMemoryUsage();
+}
sammccall wrote:
> ioeric wrote:
> > This can be a bit tricky. Generally, the
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 : FileToSlabs)
this isn't safe as it doesn't acquire the
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
kbobyrev updated this revision to Diff 163786.
kbobyrev marked 3 inline comments as done.
kbobyrev added a comment.
Rebase on top of recent revisions.
https://reviews.llvm.org/D51539
Files:
clang-tools-extra/clangd/index/FileIndex.cpp
clang-tools-extra/clangd/index/FileIndex.h
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:91
+ for (const auto : FileToSlabs)
+Result += P.second->size();
+ return Result;
Why not `bytes` here as well?
https://reviews.llvm.org/D51539
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:184
+ size_t Bytes = PairedSlabSize;
+ Bytes += LookupTable.size() * sizeof(std::pair);
Bytes += SymbolQuality.size() * sizeof(std::pair);
Why not use
kbobyrev planned changes to this revision.
kbobyrev added a comment.
I'll rebase it on top of https://reviews.llvm.org/D51422 and address the last
comment before proceeding to the review.
https://reviews.llvm.org/D51539
___
cfe-commits mailing
sammccall added a comment.
This looks reasonable!
This is going to conflict with https://reviews.llvm.org/D51422, you might want
to rebase.
Comment at: clang-tools-extra/clangd/index/MemIndex.h:26
+ void build(std::shared_ptr> Symbols,
+ size_t SlabSize=0);
kbobyrev updated this revision to Diff 163523.
kbobyrev added a comment.
Resolved the issues. Measurements show that static Dex index for LLVM takes
~140 MB, ~80 MB of which is the size of `SymbolSlab`.
https://reviews.llvm.org/D51539
Files:
clang-tools-extra/clangd/index/FileIndex.cpp
kbobyrev planned changes to this revision.
kbobyrev added a comment.
I should use `SymbolSlab.bytes()` instead of `SymbolSlab.size()`. Either way,
something seems to be wrong, because manually checking the `PairedSlabSize`
made me suspicious.
https://reviews.llvm.org/D51539
kbobyrev created this revision.
kbobyrev added reviewers: ioeric, sammccall.
kbobyrev added a project: clang-tools-extra.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay.
Currently, `SymbolIndex::estimateMemoryUsage()` returns the "overhead"
estimate, i.e. the estimate of the
22 matches
Mail list logo
