[PATCH] D88411: [clangd] Introduce MemoryTrees

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/support/MemoryTree.h:36
+  /// No copy of the \p Name.
+  MemoryTree *addChild(llvm::StringLiteral Name) { return &createChild(Name); }
+
----------------
sammccall wrote:
> sammccall wrote:
> > actually, why do these return pointers rather than references?
> reading call sites, `child()` might be both more fluent and more accurate than `addChild` - we're not calling it for the side effect and there may or may not be one.
> actually, why do these return pointers rather than references?

It is to make usage with `auto` safer, as it won't capture ref by default and make a copy. e.g. `auto child = MT.child(); child.addusage(1)` won't work as expected.

I suppose we can make the object non-copyable and prevent such misuse.


================
Comment at: clang-tools-extra/clangd/support/MemoryTree.h:41
+  MemoryTree *addDetail(llvm::StringRef Name) {
+    return Alloc ? &createChild(llvm::StringSaver(*Alloc).save(Name)) : this;
+  }
----------------
sammccall wrote:
> nit: `Name.copy(*Alloc)` is a little more direct
ah thanks, didn't know that!


================
Comment at: clang-tools-extra/clangd/support/MemoryTree.h:31
+  /// detail to root, and doesn't report any descendants.
+  void traverseTree(bool CollapseDetails,
+                    llvm::function_ref<void(size_t /*Size*/,
----------------
sammccall wrote:
> kadircet wrote:
> > sammccall wrote:
> > > sammccall wrote:
> > > > In earlier discussions we talked about avoiding the cost of *assembling* the detailed tree, not just summarizing it at output time.
> > > > 
> > > > This requires `CollapseDetails` to be passed into the profiling function, which in turn suggests making it part of a tree and passing a reference to the tree in. e.g. an API like
> > > > 
> > > > ```
> > > > class MemoryTree {
> > > >   MemoryTree(bool Detailed);
> > > >   MemoryTree* addChild(StringLiteral); // no copy
> > > >   MemoryTree* addDetail(StringRef); // returns `this` unless detailed
> > > >   void addUsage(size_t);
> > > > }
> > > > ```
> > > It's hard to evaluate a tree-traversal API without its usages... how will this be used?
> > you can find some in https://reviews.llvm.org/D88414
> I guess you mean D88417?
> 
> This works well when exporting all nodes somewhere keyed by dotted path, but it's pretty awkward+inefficient if you want to - totally hypothetical example here - dump the memory tree as a JSON structure over RPC.
> 
> So I think we need a more flexible API, at which point... maybe we should punt, expose `const DenseMap<StringRef, MemoryTree> &children() const`, and move the traverse implementation to the metrics exporter?
SGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88411/new/

https://reviews.llvm.org/D88411