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

[Keith via Phabricator via llvm-commits]

kpw added a comment.

Here are the revisions pre-landing. I'll probably land this tomorrow and follow up with a separate review of simple lit test cases for these conversions.



================
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:
> 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.
Went with trace_event, but kept the enum name.

Ugh. We have output-format and instr_map. :(


================
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) {
----------------
dberris wrote:
> 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.
Meh. I removed the braces and grouped the intermediate and terminal nodes. I think it's clear enough.

llvm::copy doesn't exist, although copy_if does. I could use std::copy and llvm::concat, but I'm not enamored with the clarity of some of the <algorithm> style code. It forces readers to either have knowledge of particular functions or grok more type signatures to see what's going on.


https://reviews.llvm.org/D39362