[PATCH] D88414: [clangd] Introduce memory dumping to FileIndex, FileSymbols and BackgroundIndex
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 5 03:01:38 PDT 2020
sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/index/Background.cpp:419
+ IndexedSymbols.attachMemoryUsage(*MT.addChild("symbols"));
+ MT.addChild("index")->addUsage(estimateMemoryUsage());
+}
----------------
Hmm, we should be careful now, estimateMemoryUsage() is "total" while addUsage takes "self".
We're not overriding estimateMemoryUsage here to include IndexedSymbols, but this is probably just an oversight. Consider calling SwapIndex::estimateMemoryUsage() explicitly to guard/communicate against this.
In the long-run, we should put attachMemoryUsage on SymbolIndex and either remove estimateMemoryUsage() or make it non-virtual (`MemoryTree R; attachMemoryUsage(R); return R.total()`)
================
Comment at: clang-tools-extra/clangd/index/Background.h:177
+ void attachMemoryUsage(MemoryTree &MT) const;
+
----------------
As a conventional name for these, profile() or so might be slightly more evocative. And I think we should push this into progressively more places (that currently have ad-hoc "estimate" accessors) so the brevity is reasonable I think.
(If not, I'd even slightly prefer "record"/"measure" over "attach" as emphasizing the high-level operation rather than the data structure used can help readability)
================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:380
+ for (const auto &SymSlab : SymbolsSnapshot)
+ MT.addDetail(SymSlab.first())->addUsage(SymSlab.second->bytes());
+ for (const auto &RefSlab : RefsSnapshot)
----------------
addChild("symbols"/"refs"/"relations")?
This seems like really useful information
================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:457
+void FileIndex::attachMemoryUsage(MemoryTree &MT) const {
+ PreambleSymbols.attachMemoryUsage(*MT.addChild("preamble_symbols"));
+ MT.addChild("preamble_index")->addUsage(PreambleIndex.estimateMemoryUsage());
----------------
addChild("preamble").addChild("symbols")?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88414/new/
https://reviews.llvm.org/D88414
More information about the cfe-commits
mailing list