kadircet marked an inline comment as done. kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/support/MemoryTree.cpp:28 + size_t ChildrenSize = 0; + for (const auto &C : Children) { + C.traverseTree(CollapseDetails, ---------------- sammccall wrote: > Here the detailed nodes are entirely collapsed. This isn't quite as powerful > as collapsing the detail *edges* only, which is worth considering. > > e.g. consider TUScheduler. The natural accounting of resources is (IMO) > ``` > clangd.TUScheduler.<filename>.AST > clangd.TUScheduler.<filename>.Preamble.InMemory > clangd.TUScheduler.<filename>.Preamble.Inputs > ``` > or something like that. > > Instead of aggregating to `clangd.TUScheduler` only, collapsing the > `<filename>` edges mean you get `clangd.TUScheduler.AST` etc. yes this was an option i hadn't considered. my only hesitation is this likely implies different paths with the same name. current api also doesn't protect against it, but they are a lot less likely as we collapse whole subtrees. I believe deleting the edge should also imply aggregating all the paths with the same name, e.g. `a.b.<deleted-1>.c, 5` and `a.b.<deleted-2>.c, 8` should end up being `a.b.c,13`. WDYT? ================ Comment at: clang-tools-extra/clangd/support/MemoryTree.h:22 +/// preserving the hierarchy. +struct MemoryTree { +public: ---------------- sammccall wrote: > (I'm wondering if there are other resources we'd like to count, like disk > size or so, but probably YAGNI and a concrete name is nice) i went with this one as i didn't want to call it `Tree` and couldn't find a nice generic name, maybe something around `NamedBytesTree`. ================ Comment at: clang-tools-extra/clangd/support/MemoryTree.h:25 + void addChild(llvm::StringRef Name, MemoryTree MT, bool IsDetail = false); + void addChild(llvm::StringRef Name, size_t Size); + ---------------- sammccall wrote: > do we support nodes with both usage and children? > This API makes such a construction awkward. > > vs something like addUsage(size_t) yes we do, I haven't added any because it wasn't needed and internal collapsing mechanism could update the size. i'll add an explicit `addUsage` too. ================ 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: > 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 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