[PATCH] D88411: [clangd] Introduce MemoryTrees
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 5 02:26:52 PDT 2020
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
More information about the cfe-commits
mailing list