Page MenuHomePhabricator

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

Authored by kpw on Oct 26 2017, 11:53 PM.

Details

Summary

Make use of Chrome Trace Event format's Duration events and stack frame dict to
produce Json files that chrome://tracing can visualize from xray function call
traces. Trace Event format is more robust and has several features like
argument logging, function categorization, multi process traces, etc. that we
can add as needed. Duration events cover an important base case.

Part of this change is rearranging the code so that the TrieNode data structure
can be used from multiple tools and can carry parameterized baggage on the
nodes. I put the actual behavior changes in llvm-xray convert exclusively.

Exploring the trace of instrumented llc was pretty nifty if overwhelming.
I can envision this being very useful for analyzing contention scenarios or
tuning parameters like batch sizes in a producer consumer queue. For more
targeted traces likemthis, let's talk about how we want to approach trace
pruning.

Diff Detail

Repository
rL LLVM

Event Timeline

kpw created this revision.Oct 26 2017, 11:53 PM
dberris edited edge metadata.Oct 27 2017, 3:24 PM

Thanks, @kpw -- some comments below.

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

tools/llvm-xray/stack-trie.h
25 ↗(On Diff #120544)

This makes sense. Please feel free to rename.

78 ↗(On Diff #120544)

Have you thought about removing the recursion, and doing a depth-first traversal instead with an explicit stack?

tools/llvm-xray/xray-converter.cc
41 ↗(On Diff #120544)

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?

167 ↗(On Diff #120544)

nit: function naming should start with lowercase character? I keep getting confused about this. :)

172 ↗(On Diff #120544)

Maybe move this outside of the if, so that you can use a single container in either exit path?

175–177 ↗(On Diff #120544)

You can make this:

if (map_iter.first != TId)
  for (...)
    if (...)
      Siblings.push_back(...);
271–279 ↗(On Diff #120544)

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,
})", ...);
290–298 ↗(On Diff #120544)

Same advice for here.

314–327 ↗(On Diff #120544)

And here as well. You might even think about moving some of these out into smaller more focused helper functions.

tools/llvm-xray/xray-stacks.cc
269 ↗(On Diff #120544)

You can pre-reserve this vector too, to get maximum efficiency. Same for the other ones that follow.

kpw added a comment.Oct 29 2017, 4:13 PM

Thanks for taking a look Dean. Here's a link to an example trace that can be loaded up at chrome://tracing. I'll respond to your other comments tomorrow.

https://storage.googleapis.com/wyssmanhostingus-central1/chrometraceviewer2.txt.gz

kpw updated this revision to Diff 121233.Nov 1 2017, 8:42 PM
kpw marked 7 inline comments as done.

Made some changes, particularly regarding string formatting.

kpw added inline comments.Nov 1 2017, 8:42 PM
tools/llvm-xray/stack-trie.h
78 ↗(On Diff #120544)

TODO attached.

tools/llvm-xray/xray-converter.cc
41 ↗(On Diff #120544)

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"

175–177 ↗(On Diff #120544)

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

271–279 ↗(On Diff #120544)

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.

dberris accepted this revision.Nov 1 2017, 10:44 PM

LGTM

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

Thanks, @kpw!

tools/llvm-xray/xray-converter.cc
188–193 ↗(On Diff #121233)

Braces here seem unnecessary too, and may be omitted. :)

222–231 ↗(On Diff #121233)

Since this is an early return, you don't need the else part of the if-else statement.

41 ↗(On Diff #120544)

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.

175–177 ↗(On Diff #120544)

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.

tools/llvm-xray/xray-stacks.cc
272–283 ↗(On Diff #121233)

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.

This revision is now accepted and ready to land.Nov 1 2017, 10:44 PM
kpw updated this revision to Diff 121385.Nov 2 2017, 2:41 PM
kpw marked 4 inline comments as done.
  • Phabricator comments.
kpw added a comment.Nov 2 2017, 2:41 PM

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.

tools/llvm-xray/xray-converter.cc
41 ↗(On Diff #120544)

Went with trace_event, but kept the enum name.

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

tools/llvm-xray/xray-stacks.cc
272–283 ↗(On Diff #121233)

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.

This revision was automatically updated to reflect the committed changes.
kpw marked an inline comment as done.