Differential D24376
[XRay] Implement `llvm-xray convert` -- trace file conversion Authored by dberris on Sep 8 2016, 10:59 PM.
Details This is the second part of a multi-part change to define additional This change defines a conversion subcommand to take XRay log files, and Depends on D21987.
Diff Detail
Event TimelineComment Actions I'll be adding more tests and sample inputs for this tool, and probably use DataExtractor too for loading the records from the XRay log files. Please hold off on reviewing @dblaikie. Comment Actions Before I get into all the details - this seems similar to the extract tool, except it deals with the log rather than the instrumentation map, right? So pretty much all the same feedback as there (& I don't necessarily remember the original answers). We ended up with a one way conversion last time (to yaml, but not back again), should this one be different? If we only convert one way, is 'convert' the right name? Should be be more explicit about the difference between these two things, or just detect based on file magic & use one command name for both kinds of input files? (check if it's an ELF file then assume the user is trying to extract an instrumentation map, and otherwise assume the file's a log?)
Comment Actions A few of the inline comments from my last feedback are probably out of date/not relevant - I had them queued up from weeks/months ago. That said - taking a guess at it: Yes, I'd use DataExtractor here over splatting/memcpying structs around for the same reasons. Comment Actions The value of having a convert function is so that we can support other formats specifically for the xray log. Here are some future things we need to support:
For now, the most convenient thing we can do is create YAML files. The important part here is the log file loading library, that we use in the accounting implementation (stacked on top of this change). So the verb really is to "convert", as opposed to just "extract". Does this make sense? That said, I'll apply most of the changes in the 'extract' review here. Cheers Comment Actions
Comment Actions Ready for a look now @dblaikie, thanks in advance! PS. We no longer support round-tripping in this implementation.
Comment Actions Thanks @dblaikie, PTAL.
Comment Actions
Comment Actions (I was thinking more going the other direction: removing the dead code, rather than adding a use of it) What's the purpose of the binary/raw output format? (I think it was mentioned that the binary output format would be for testing - so we could write yaml, then generate raw files - then feed those into other tools for testing? But if the yaml input format is supported everywhere, what's the purpose of that? We'll want to have a couple of binary tests, but I imagine they would test checked in binary -> yaml output (if they test yaml -> binary -> yaml, then I'm not sure they achieve much because it's just roundtripping so we have as much code under test (binary -> yaml) as we do in the test harness (yaml -> binary), effectively) That's my mental model at least. (this commit seems to add several .bin test files - but I don't see where those are used?)
Comment Actions Other things going on concurrently (work on the FDR mode for the runtime library) is making me need to be able to turn versions of the binary log from one form to another. I've found that working with the binary versions is much more convenient from a tool perspective, and that this current simple format is more amenable to analysis than the condensed format I'm working on. So effectively, version 2 of the log format is turning out to have both non-fixed-size records and more interleaving happening. This tool, that's able to take that complex format into something simpler/different doesn't make sense for generating just YAML files.
Some of the tests are looking at symbolization of the function id's, and associating functions with debug info, etc. for coverage of the various modes by which the YAML output could be generated.
Comment Actions It seems Phabricator had been swallowing some of my comments (and not sending out email messages). This is ready for another look @dblaikie. Comment Actions I'm not sure I'm following here - could you describe this in more detail? My understanding was that tools would likely use these LLVM APIs for data handling (or they'd use YAML input) - so there wouldn't be any benefit to any particular format (except YAML) since these LLVM APIs could handle all the formats. This would make the binary format(s) fairly niche - just convenient for runtime emission, but not desirable for any other use. Where have I gone wrong here?
Not quite following, more practically: which tests are these files used in? Comment Actions Sure, there's a patch I've uploaded recently that has a lot more about this different format (see D27038).
That's one half of the story. The other half is the new log format (still binary) is not conducive to this simple approach of loading up records with full information. The FDR mode log format is substantially different, in that per-thread buffers are chunked up into fixed-sized blocks and each record might be a metadata record (16 bytes) or a function record (8 bytes). That log has all sorts of information consolidated in very different ways (e.g. the thread ID is stored once, as a metadata record at the beginning of a buffer, CPU id's only show up when the CPU is first encountered, we store TSC deltas instead of full TSCs in function records), and is not conducive to just "load in memory, read records one by one". So then, the conversion tool ought to be able to convert the FDR mode logs into this simpler log format that we already support in this tool -- then let all the other tools upcoming (accounting, timeline visualisation, etc.) just use the simpler log format. The closest analogy I can make for existing tools that do something similar would be 'dot' and friends.
convert-with-debug-syms.txt Comment Actions I'm still a bit confused though - why not just convert to the yaml format at that point? (& why convert at all - if we're talking about other tools built on top of LLVM's library (tools built outside of that would presumably only rely on the YAML?) would use a common parser/reader API that could handle all formats, right? Rather than having the user run a conversion tool, then the tool they want to use)
ah, hmm - I saw the .bin files (some of them) added in nthe last update - so I guess maybe they just hadn't been added previously, but the uses were already there. No worries. Comment Actions YAML is a little big and verbose -- the blow-up from the binary format to YAML is a factor of at least 10. In practical terms, dealing with the binary format in terms of storage and transport is a good option. YAML is convenient for human consumption, and if users ever build tools that use scripting languages or other things already deal with YAML (or JSON, or some other text format) then that's the convenient path to take. Maybe the solution here is to move the log reading part into something that's part of the LLVM library, but that still has the problem that reconstruction of a consistent timeline representing events on multiple threads is much more complex than one that just takes a linear "simple" binary file. Then the XRay log file that has non-fixed-size records and more complex encoding ought to be converted to the simplified form. So the compromise/trade-off being made here is that we localize the knowledge of dealing with the different XRay binary log formats to this tool, that can turn it into "the simplest XRay log format", that can be read by simpler libraries for consumption. Given that, and the plan of being able to convert these XRay log files into other non-XRay specific formats (Chrome trace viewer format which is JSON, or the perftools utilities, etc.) is a huge convenience. This convenience is not only for testing purposes but also for building and integrating with other existing tools that deal with all sorts of traces. Does this make sense? Comment Actions
I can agree with this.
Well, not entirely convenient. It would be nice to have a "dumping" output that produces an easy to consume by normal humans format that isn't the full yaml format for specifying.
I think that the format reading should definitely be a part of the llvm library and not the tool in particular. The tool should just wrap the library.
Being able to convert seems useful in general. You want to be able to write testcases in something human readable, but you want the default output to be binary for compactness. -eric Comment Actions Cool, thanks Eric -- D28345 does just the log reading library which depends on this change landing.
Good point. So I think "conversion" is a bit more loaded than just "dump", where "dump" really just makes the representation more human-readable (for debugging, or just turning machine-readable form to human-readable form). Will renaming this from 'convert' to 'dump' make that better?
Comment Actions
I don't think that'd be accurate - this is doing format conversions between semantic preserving representations intended for machine consumption. Our dumping tools are generally intended only for human consumption (or really hacky machine consumption in scripts for quick work sometimes (eg: some scripts I'm writing using objdump/dwarfdump for doing some object size analysis)).
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||