[PATCH] D38650: [XRay][tools] Updated stacks tool with flamegraph output.

[Dean Michael Berris via Phabricator via llvm-commits]

dberris accepted this revision.
dberris added a comment.
This revision is now accepted and ready to land.

LGTM with some nits and style issues.

Thanks, @kpw!

I suppose you're updating the documentation on how to generate the flame graphs too, at some point?



================
Comment at: tools/llvm-xray/xray-stacks.cc:336
+// Calculates how many times a function was invoked.
+// TODO(kpw): Hook up option to produce stacks
+template <>
----------------
In LLVM's style guide, we don't put names to TODO's (just // TODO:).


================
Comment at: tools/llvm-xray/xray-stacks.cc:594
+
+  // TODO(kpw): Add a format option when more than one are supported.
+  template <AggregationType AggType>
----------------
Same here (TODO:).


================
Comment at: tools/llvm-xray/xray-stacks.cc:599-600
+    for (const auto *N : RootValues) {
+      SmallVector<const TrieNode *, 16> S;
+      S.emplace_back(N);
+      while (!S.empty()) {
----------------
Maybe lift out of the loop and clear, instead of creating and destroying one every iteration?


================
Comment at: tools/llvm-xray/xray-stacks.cc:621
+      OS << "thread_" << ThreadId << ";";
+    std::stack<const TrieNode *> lineage{};
+    lineage.push(Node);
----------------
Do you actually need a std::stack, as opposed to just using a std::vector, or a SmallVector?


================
Comment at: tools/llvm-xray/xray-stacks.cc:738-742
+  if (!DumpAllStacks && StacksOutputFormat != HUMAN) {
+    return make_error<StringError>(
+        Twine("Can't specify a non-human format without -all-stacks."),
+        std::make_error_code(std::errc::invalid_argument));
+  }
----------------
nit: Enclosing curlies for single-statement bodies not required in LLVM style.


https://reviews.llvm.org/D38650