[PATCH] D34863: [XRay][tools] Function call stack based analysis tooling for XRay traces

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

Is this ready to land? What else is missing here?



================
Comment at: tools/llvm-xray/xray-stacks.cc:292
+  // We make sure all the nodes are accounted for in this list.
+  std::list<TrieNode> NodeStore;
+
----------------
nit: Do you need a doubly-linked list here? Or will a `std::forward_list` work better?


================
Comment at: tools/llvm-xray/xray-stacks.cc:393
+    }
+    }
+    return AccountRecordStatus::OK;
----------------
Do you need a case for tail exits? You might also want to log/ignore other kinds of records.


================
Comment at: tools/llvm-xray/xray-stacks.cc:426
+
+  /// Prints top stacks from looking at all the leafs and ignoring threads.
+  void printIgnoringThreads(raw_ostream &OS, FuncIdConversionHelper &FN) {
----------------
nit: leafs -> leaves?


================
Comment at: tools/llvm-xray/xray-stacks.cc:430
+    auto RootBackInserter = std::back_inserter(RootValues);
+    auto SecondFn = [](decltype(*Roots.begin()) value) { return value.second; };
+
----------------
Are you missing a reference somewhere in the signature? Otherwise you're making a copy in the call. Alternatively to this you can use an alias to make it a bit more readable:

```
using RootT = decltype(*Roots.begin());
auto SecondFn = [](const RootT &value) { return value.second; };
```


================
Comment at: tools/llvm-xray/xray-stacks.cc:432-434
+    std::for_each(
+        map_iterator(Roots.begin(), SecondFn),
+        map_iterator(Roots.end(), SecondFn),
----------------
Is this something you can turn into a `llvm::transform` instead?


================
Comment at: tools/llvm-xray/xray-stacks.cc:450
+      for (auto *node : RootNodeVector) {
+        auto iter = std::find_if(
+            RootValues.begin(), RootValues.end(),
----------------
Use `llvm::find_if` instead? That takes a range already.


https://reviews.llvm.org/D34863