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