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