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

[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

Just a few more nits, please feel free to address and/or ignore. :)

Thanks, @kpw!



================
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")),
----------------
kpw wrote:
> dberris wrote:
> > 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?
> That document refers to the Catapult Trace-Viewer, which is "particularly good at viewing linux kernel traces (aka ftrace) and Chrome's trace_event format." Perhaps I should make the command line argument "trace_event"
I think it's fine for now to use Chrome on the name. We should still be able to maybe alias that (not sure if it's possible to do with the clEnumValN to bind multiple strings to the same value. Alternatively, you can say "JSON_TRACE_EVENT" to be more generic for the enum, and "trace-event" or "trace_event" for the string value. It's not super high-value to change this now, just something worth thinking about.


================
Comment at: tools/llvm-xray/xray-converter.cc:175-177
+      if (map_iter.first == TId)
+        continue;
+      for (auto node_iter : map_iter.second) {
----------------
kpw wrote:
> dberris wrote:
> > You can make this:
> > 
> > ```
> > if (map_iter.first != TId)
> >   for (...)
> >     if (...)
> >       Siblings.push_back(...);
> > ```
> What does the LLVM style guide have to say about not including braces.
> 
> A chain of several control flow elements without braces seems more crazy to me than a single statement if. I didn't have luck searching https://llvm.org/docs/CodingStandards.html
I don't think there's an explicit rule here, but that the convention has been to omit braces if they don't add value outside of just good indentation. In several reviews and patches, this has been suggested to reduce line-noise where the braces are not absolutely necessary.


================
Comment at: tools/llvm-xray/xray-converter.cc:188-193
+  for (auto *ParentSibling : parent->ExtraData.siblings) {
+    for (auto node_iter : ParentSibling->Callees) {
+      if (node_iter->FuncId == FnId)
+        Siblings.push_back(node_iter);
+    }
+  }
----------------
Braces here seem unnecessary too, and may be omitted. :)


================
Comment at: tools/llvm-xray/xray-converter.cc:222-231
+  } else {
+    unsigned stack_id = siblings[0]->ExtraData.id;
+    NodeStore.push_front({FuncId, Parent, {}, {stack_id, std::move(siblings)}});
+    StackTrieNode *CurrentStack = &NodeStore.front();
+    for (auto *sibling : CurrentStack->ExtraData.siblings) {
+      sibling->ExtraData.siblings.push_back(CurrentStack);
+    }
----------------
Since this is an early return, you don't need the else part of the if-else statement.


================
Comment at: tools/llvm-xray/xray-stacks.cc:272-283
   for (auto duration : Left.TerminalDurations) {
-    Node->TerminalDurations.push_back(duration);
+    Data.TerminalDurations.push_back(duration);
   }
   for (auto duration : Right.TerminalDurations) {
-    Node->TerminalDurations.push_back(duration);
+    Data.TerminalDurations.push_back(duration);
   }
   for (auto duration : Left.IntermediateDurations) {
----------------
You might also consider using algorithms here that deal with ranges to do the copies. The `llvm::copy` algorithm seems like a good candidate to better express intent here.


https://reviews.llvm.org/D39362