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

[Keith via Phabricator via llvm-commits]

kpw added inline comments.


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


================
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")),
----------------
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"


================
Comment at: tools/llvm-xray/xray-converter.cc:175-177
+      if (map_iter.first == TId)
+        continue;
+      for (auto node_iter : map_iter.second) {
----------------
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


================
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, "
----------------
dberris wrote:
> 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,
> })", ...);
> ```
It's a bit less pretty without the newlines, but I've put this into a function and used llvm::formatv.

I don't think llvm::format() allows positional wrangling.


https://reviews.llvm.org/D39362