[PATCH] D88411: [clangd] Introduce MemoryTrees

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/support/MemoryTree.h:22
+/// preserving the hierarchy.
+struct MemoryTree {
+public:
----------------
(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)


================
Comment at: clang-tools-extra/clangd/support/MemoryTree.h:24
+public:
+  void addChild(llvm::StringRef Name, MemoryTree MT, bool IsDetail = false);
+  void addChild(llvm::StringRef Name, size_t Size);
----------------
The concept of "detail" should be introduced somewhere, or just be something more concrete
(like IsFilename)

There's a few things about filenames that make erasing them sensible:
 - they have uniform semantics - we don't really know the difference between them
 - can be very many of them so the result is inherently hard to interpret
 - can be many/large so we don't want to copy them

Thinking about how to generalize these - I think the name detail is OK (if you want a general name) but we should mention some of it.


================
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);
+
----------------
do we support nodes with both usage and children?
This API makes such a construction awkward.

vs something like addUsage(size_t)


================
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*/,
----------------
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);
}
```


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88411