[PATCH] D88411: [clangd] Introduce MemoryTrees

[Kadir Cetinkaya via Phabricator via cfe-commits]

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