[PATCH] D88414: [clangd] Introduce memory dumping to FileIndex, FileSymbols and BackgroundIndex

[Sam McCall via Phabricator via cfe-commits]

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