[PATCH] D39362: [XRay] Minimal tool to convert xray traces to Chrome's Trace Event Format.

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

Thanks, @kpw -- some comments below.

If you can share a sample output that other reviewers can load, that would be great too. :)



================
Comment at: tools/llvm-xray/stack-trie.h:25
+
+// TODO: Should this file instead be named trie-node.h?
+
----------------
This makes sense. Please feel free to rename.


================
Comment at: tools/llvm-xray/stack-trie.h:78
+      Node->Callees.push_back(
+          mergeTrieNodes(*(iter->second), *Callee, Node, NodeStore, MergeFn));
+      LeftCalleesByFuncId.erase(iter);
----------------
Have you thought about removing the recursion, and doing a depth-first traversal instead with an explicit stack?


================
Comment at: tools/llvm-xray/xray-converter.cc:41
+               clEnumValN(ConvertFormats::YAML, "yaml", "output in yaml"),
+               clEnumValN(ConvertFormats::CHROME_TRACE_EVENT, "chrome",
+                          "output in chrome trace event format")),
----------------
nit: The trace viewer project has been spun out of Chrome and is now called catapult (https://catapult.gsrc.io/tracing/README.md). You may want to use that name instead?


================
Comment at: tools/llvm-xray/xray-converter.cc:167
+SmallVector<StackTrieNode *, 4>
+FindSiblings(StackTrieNode *parent, int32_t FnId, uint32_t TId,
+             const DenseMap<uint32_t, SmallVector<StackTrieNode *, 4>>
----------------
nit: function naming should start with lowercase character? I keep getting confused about this. :)


================
Comment at: tools/llvm-xray/xray-converter.cc:172
+  if (parent == nullptr) {
+    SmallVector<StackTrieNode *, 4> Siblings{};
+    for (auto map_iter : StackRootsByThreadId) {
----------------
Maybe move this outside of the if, so that you can use a single container in either exit path?


================
Comment at: tools/llvm-xray/xray-converter.cc:175-177
+      if (map_iter.first == TId)
+        continue;
+      for (auto node_iter : map_iter.second) {
----------------
You can make this:

```
if (map_iter.first != TId)
  for (...)
    if (...)
      Siblings.push_back(...);
```


================
Comment at: tools/llvm-xray/xray-converter.cc:271-279
+      OS << "    ";
+      OS << "{ \"name\": \""
+         << (Symbolize ? FuncIdHelper.SymbolOrNumber(R.FuncId)
+                       : llvm::to_string(R.FuncId))
+         << "\", \"ph\": \"B\", "
+            "\"tid\": "
+         << R.TId << ", \"pid\": 1, "
----------------
Consider using `llvm::format(...)` more here, than the left-shift operator. That way you can better express the structure of the string you're building, potentially using a raw string literal to elide the character escapes. This might look like:

```
OS << llvm::format(R"(
{
  "name": "%0",
  "ph": "B",
  "tid": "%1",
  "pid": 1,
  "ts": %2,
  "sf": %3,
})", ...);
```


================
Comment at: tools/llvm-xray/xray-converter.cc:290-298
+        OS << "    ";
+        OS << "{ \"name\": \""
+           << (Symbolize ? FuncIdHelper.SymbolOrNumber(StackCursor->FuncId)
+                         : llvm::to_string(StackCursor->FuncId))
+           << "\", \"ph\": \"E\", "
+              "\"tid\": "
+           << R.TId << ", \"pid\": 1, "
----------------
Same advice for here.


================
Comment at: tools/llvm-xray/xray-converter.cc:314-327
+  for (auto map_iter : StacksByStackId) {
+    if (stack_frame_count++ == 0)
+      OS << "\n";
+    else
+      OS << ",\n";
+    OS << "    ";
+    OS << "\"" << map_iter.first << "\": { \"name\": \""
----------------
And here as well. You might even think about moving some of these out into smaller more focused helper functions.


================
Comment at: tools/llvm-xray/xray-stacks.cc:269
   for (auto duration : Left.TerminalDurations) {
-    Node->TerminalDurations.push_back(duration);
+    Data.TerminalDurations.push_back(duration);
   }
----------------
You can pre-reserve this vector too, to get maximum efficiency. Same for the other ones that follow.


https://reviews.llvm.org/D39362