This is an archive of the discontinued LLVM Phabricator instance.

[XRay] Implement the `llvm-xray account` subcommand
ClosedPublic

Authored by dberris on Sep 8 2016, 11:01 PM.

Details

Summary

This is the third of a multi-part change to implement subcommands for
the llvm-xray tool.

Here we define the account subcommand which does simple function call
accounting, generating basic statistics on function calls we find in an
XRay log/trace. We support text output and csv output for this
subcommand.

Part of this tool will later be turned into a library that could be used for
basic function call accounting.

Depends on D24376.

Diff Detail

Repository
rL LLVM

Event Timeline

dberris updated this revision to Diff 70788.Sep 8 2016, 11:01 PM
dberris retitled this revision from to [XRay] Implement the `llvm-xray account` subcommand.
dberris updated this object.
dberris added reviewers: dblaikie, echristo.
dberris added a subscriber: llvm-commits.
dberris updated this revision to Diff 74116.Oct 10 2016, 3:47 AM
  • Rebase and update implementation.
dberris updated this revision to Diff 75122.Oct 19 2016, 3:59 AM
  • Rebase and update implementation.
  • Use Error
dberris updated this revision to Diff 79270.Nov 24 2016, 7:19 PM
  • Make the number formatting nicer with formatv
dberris updated this revision to Diff 79280.Nov 25 2016, 12:59 AM
  • Sorting, Summing, Top N
dberris updated this revision to Diff 79368.Nov 27 2016, 8:46 PM
  • Rebase
  • Fix tests with format adjustment
varno added a subscriber: varno.Nov 30 2016, 7:08 PM

At line 182-195 the following code I think is simpler and results in the function performing the same work.

if (Parent == ThreadStack.rend())
  return false;
while (ThreadStack.back().FuncId != Record.FuncId) {
  const auto &Top = ThreadStack.back();
  recordLatency(Top.first, diff(Top.second, Record.TSC)):
  ThreadStack.pop_back();
}
dblaikie added inline comments.Dec 1 2016, 1:15 PM
tools/llvm-xray/xray-account.cc
174–203 ↗(On Diff #79368)

Might be good (maybe not) to invert this to reduce indentation.

if (first == funcid) {
  ...
  ThreadStack.pop_back();
}
if (!DeduceSiblingCalls)
  return false;
if (Parent == ThreadStack.rend())
  return false;
while (...) {
  ...
}
return true;
316 ↗(On Diff #79368)

extra semicolon?

324 ↗(On Diff #79368)

Probably wouldn't bother usind std::advance unless you're writing a generic algorithm or have a non-random iterator you need to stride forward.

Results.erase(Results.begin() + AccountTop.getValue(), Results.end());

seems fine.

332 ↗(On Diff #79368)

Text isn't an acronym, so probably shouldn't be capitalized here.

378–381 ↗(On Diff #79368)

Seems a bit weird to put a single static variable in a namespace - maybe just use 'using' decls for the namespace instead?

395 ↗(On Diff #79368)

No need for "Error(...)" here, this will do, I think:

return std::move(E);
414–421 ↗(On Diff #79368)

It'd be good if reading were refactored into some utility that would just read the input file in whatever format it was in without every tool having to have explicit support for format specifications.

433 ↗(On Diff #79368)

reduce indentation:

if (FCA.accountRecord(Record))
  continue;
...
dberris updated this revision to Diff 80006.Dec 1 2016, 5:48 PM
dberris marked 6 inline comments as done.
  • Address review comments

At line 182-195 the following code I think is simpler and results in the function performing the same work.

if (Parent == ThreadStack.rend())
  return false;
while (ThreadStack.back().FuncId != Record.FuncId) {
  const auto &Top = ThreadStack.back();
  recordLatency(Top.first, diff(Top.second, Record.TSC)):
  ThreadStack.pop_back();
}

Thanks -- after the refactoring suggested by @dblaikie though this became less of an issue.

tools/llvm-xray/xray-account.cc
316 ↗(On Diff #79368)

Apparently not, since if I leave this empty I get a warning that the default case is empty.

label at end of compound statement: expected statement

395 ↗(On Diff #79368)

Apparently no. :(

error: no viable conversion from returned value of type 'typename std::remove_reference<unique_ptr<StringError, default_delete<StringError> > &>::type' (aka 'std::unique_ptr<llvm::StringEr
ror, std::default_delete<llvm::StringError> >') to function return type 'llvm::Error'
dberris updated this revision to Diff 80376.Dec 5 2016, 7:34 PM
  • Reduce calls to .pop_back() for simpler code.
dblaikie accepted this revision.Jan 9 2017, 11:56 AM
dblaikie edited edge metadata.
dblaikie added inline comments.
tools/llvm-xray/xray-account.cc
34 ↗(On Diff #80376)

Is this necessary/useful?
Is this tested?

Also, I think flags tend to be - not _ separated.

39–42 ↗(On Diff #80376)

Test coverage?

47–69 ↗(On Diff #80376)

Having to specify both input files, as well as input and output formats every time seems a bit tedious. Could we detect formats using file magic & perhaps have some way of unifying the two files (the map and the log) in common/some cases?

219–223 ↗(On Diff #80376)

Might be easier written/read as:

for (auto &a : make_range(I, ThreadStack.end()))
  ...

?

221 ↗(On Diff #80376)

Capture all by reference for any lambda that doesn't escape its own scope (ie: if the lambda isn't put in a std::function or equivalent, feel free to [&] )

297–300 ↗(On Diff #80376)

Might be worth a helper function template that takes the non-type template argument to sort by.

399 ↗(On Diff #80376)

Can skip '()' here, possibly skip '-> Error' too, if you like (see below)

410 ↗(On Diff #80376)

Do you need the "-> Error" there? (I know the "if every return expression deduces to the same type it can be left implicit" was accepted as a DR to C++11 but not sure it's in all the compilers we support)

(same for the lambda being passed to the Unused variable's ctor too)

tools/llvm-xray/xray-log-reader.h
49 ↗(On Diff #80376)

Should this perhaps be called "LogInputFormat" (singular rather than plural)?

This revision is now accepted and ready to land.Jan 9 2017, 11:56 AM
dberris updated this revision to Diff 84069.Jan 11 2017, 7:59 PM
dberris marked 3 inline comments as done.
dberris updated this object.
dberris edited edge metadata.
  • Address review comments
dberris added inline comments.Jan 11 2017, 8:01 PM
tools/llvm-xray/xray-account.cc
34 ↗(On Diff #80376)

Is this necessary/useful?

Yep -- because we cannot assume that trace files will have perfect information (TSC drift, and unmatched exit records, just some of the errors we encounter) we'd like to allow users to continue despite imperfections.

Is this tested?

It wasn't but now should be. :)

Also, I think flags tend to be - not _ separated.

Done

39–42 ↗(On Diff #80376)

We have a test for this, which does tail call deduction.

47–69 ↗(On Diff #80376)

Changed to use the log file detection magic to at least remove the input format specification. I'll need to change the extract logic to also use file type detection magic (will do so in a different patch that makes the instrumentation map extraction a library in LLVM as well) which will greatly simplify this API.

dberris updated this revision to Diff 84076.Jan 11 2017, 11:44 PM
  • Commplete sorting support, add more tests
This revision was automatically updated to reflect the committed changes.
labath added a subscriber: labath.Jan 12 2017, 1:50 AM

Hi,

this is still failing to compile on windows/MSVC. http://lab.llvm.org:8011/builders/lldb-windows7-android/builds/1645/steps/ninja%20build%20local/logs/stdio

The first error seems to be about missing type pid_t.

Could you please look into that?