Page MenuHomePhabricator

[clangd] Add symbol slab size to index memory consumption estimates
ClosedPublic

Authored by kbobyrev on Aug 31 2018, 6:14 AM.

Details

Summary

Currently, SymbolIndex::estimateMemoryUsage() returns the "overhead" estimate, i.e. the estimate of the Index data structure excluding SymbolSlab size. This patch propagates information about paired SymbolSlab size where necessary.

Diff Detail

Repository
rL LLVM

Event Timeline

kbobyrev created this revision.Aug 31 2018, 6:14 AM
kbobyrev planned changes to this revision.Aug 31 2018, 6:30 AM

I should use SymbolSlab.bytes() instead of SymbolSlab.size(). Either way, something seems to be wrong, because manually checking the PairedSlabSize made me suspicious.

kbobyrev updated this revision to Diff 163523.Aug 31 2018, 6:49 AM

Resolved the issues. Measurements show that static Dex index for LLVM takes ~140 MB, ~80 MB of which is the size of SymbolSlab.

This looks reasonable!
This is going to conflict with D51422, you might want to rebase.

clang-tools-extra/clangd/index/MemIndex.h
26 ↗(On Diff #163519)

could you make these names somewhat more abstract (e.g. BackingMemory rather than SlabSize)?
The index shouldn't know/care which configuration of slabs it's built from.

kbobyrev planned changes to this revision.Aug 31 2018, 8:49 AM

I'll rebase it on top of D51422 and address the last comment before proceeding to the review.

kadircet added inline comments.Aug 31 2018, 10:18 PM
clang-tools-extra/clangd/index/dex/DexIndex.cpp
184 ↗(On Diff #163523)

Why not use LookupTable.getMemorySize() directly? Is it intentionally for not counting non-used but allocated buckets? Same for the below two as well.

kadircet added inline comments.Aug 31 2018, 10:36 PM
clang-tools-extra/clangd/index/FileIndex.cpp
91 ↗(On Diff #163523)

Why not bytes here as well?

kbobyrev updated this revision to Diff 163786.Sep 4 2018, 4:52 AM
kbobyrev marked 3 inline comments as done.

Rebase on top of recent revisions.

kbobyrev removed a subscriber: ilya-biryukov.
ioeric added inline comments.Sep 7 2018, 1:32 AM
clang-tools-extra/clangd/index/FileIndex.cpp
144 ↗(On Diff #163786)

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.

clang-tools-extra/clangd/index/dex/DexIndex.h
44 ↗(On Diff #163786)

For this constructor, the index doesn't own the backing data. We should probably not include it in the estimation?

(same for MemIndex)

53 ↗(On Diff #163786)

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)

100 ↗(On Diff #163786)

nit: =0?

LG with a few nits

clang-tools-extra/clangd/index/FileIndex.cpp
121 ↗(On Diff #163786)

this isn't safe as it doesn't acquire the mutex

144 ↗(On Diff #163786)

(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.

clang-tools-extra/clangd/index/MemIndex.h
32 ↗(On Diff #163786)

I don't think defaulting this parameter makes sense, seems like an easy source of bugs
(and in 3 other cases)

70 ↗(On Diff #163786)

just // Size of memory retained by KeepAlive?
Don't mention Slab, this class doesn't know about it.

(similar in Dex)

ioeric added inline comments.Sep 7 2018, 1:57 AM
clang-tools-extra/clangd/index/FileIndex.cpp
144 ↗(On Diff #163786)

Yes, that's correct. Thanks for the clarification!

kbobyrev updated this revision to Diff 164369.Sep 7 2018, 2:05 AM
kbobyrev marked 9 inline comments as done.

Address a round of comments.

sammccall added inline comments.Sep 7 2018, 2:34 AM
clang-tools-extra/clangd/index/FileIndex.cpp
135 ↗(On Diff #164369)

this size should be calculated from the slabs above

kbobyrev updated this revision to Diff 164377.Sep 7 2018, 2:48 AM
kbobyrev marked an inline comment as done.
sammccall added inline comments.Sep 7 2018, 2:53 AM
clang-tools-extra/clangd/index/FileIndex.cpp
131 ↗(On Diff #164377)

also the refslabs and refsstorage

kbobyrev updated this revision to Diff 164383.Sep 7 2018, 3:31 AM
kbobyrev marked an inline comment as done.

Oh, I thought they're empty.

kbobyrev updated this revision to Diff 164643.Sep 10 2018, 3:30 AM

Sync with HEAD.

kbobyrev updated this revision to Diff 164644.Sep 10 2018, 3:35 AM

Remove accidental formatting changes.

ioeric accepted this revision.Sep 10 2018, 3:37 AM
ioeric added inline comments.
clang-tools-extra/clangd/index/Index.h
512 ↗(On Diff #164643)

nit: I'd avoid irrelevant changes.

clang-tools-extra/clangd/index/MemIndex.cpp
20 ↗(On Diff #164643)

Include Refs slab size?

76 ↗(On Diff #164643)

Include Refs size?

This revision is now accepted and ready to land.Sep 10 2018, 3:37 AM
kbobyrev updated this revision to Diff 164653.Sep 10 2018, 4:43 AM
kbobyrev marked 2 inline comments as done.
kbobyrev marked an inline comment as done.
kbobyrev added inline comments.
clang-tools-extra/clangd/index/Index.h
512 ↗(On Diff #164643)

This one is out of sync; fixed in the previous update :)

This revision was automatically updated to reflect the committed changes.