[PATCH] D88411: [clangd] Introduce MemoryTrees

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Looks good - some concerns that the traversal isn't flexible enough but we can address that later once it's used.



================
Comment at: clang-tools-extra/clangd/support/MemoryTree.cpp:10
+    std::string ComponentName) const {
+  CB(Size, ComponentName);
+  if (!ComponentName.empty())
----------------
If you're only going to pass one size, I'd think total (rather than self) is the most meaningful.
This is easier if you do postorder of course...


================
Comment at: clang-tools-extra/clangd/support/MemoryTree.cpp:11
+  CB(Size, ComponentName);
+  if (!ComponentName.empty())
+    ComponentName += '.';
----------------
if you pass ComponentName as a StringRef you can avoid lots of allocations by using the same underlying string (which will grow and shrink, but not reallocate much).
You'll need a recursion helper with a different signature though.


================
Comment at: clang-tools-extra/clangd/support/MemoryTree.h:33
+  /// If Alloc is nullptr, tree is in brief mode and will ignore detail edges.
+  MemoryTree(llvm::BumpPtrAllocator *Alloc = nullptr) : Alloc(Alloc) {}
+
----------------
nit: I'd call this parameter DetailAlloc to hint at what's stored there


================
Comment at: clang-tools-extra/clangd/support/MemoryTree.h:41
+  MemoryTree *addDetail(llvm::StringRef Name) {
+    return Alloc ? &createChild(llvm::StringSaver(*Alloc).save(Name)) : this;
+  }
----------------
nit: `Name.copy(*Alloc)` is a little more direct


================
Comment at: clang-tools-extra/clangd/support/MemoryTree.h:49
+  /// with dots(".").
+  void traverseTree(llvm::function_ref<void(size_t /*Size*/,
+                                            llvm::StringRef /*ComponentName*/)>
----------------
nit: need to be explicit whether this is self or subtree (or could pass both)


================
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*/,
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88411