This is an archive of the discontinued LLVM Phabricator instance.

Implemented color coding and Vertex labels in XRay Graph
ClosedPublic

Authored by varno on Jan 2 2017, 10:35 PM.

Details

Summary

A patch to enable the llvm-xray graph subcommand to color edges and
vertices based on statistics and to annotate vertices with statistics.

Depends on D27243

Event Timeline

varno updated this revision to Diff 82841.Jan 2 2017, 10:35 PM
varno retitled this revision from to Implemented color coding and Vertex labels in XRay Graph.
varno updated this object.
varno added reviewers: dblaikie, dberris.
varno added a subscriber: llvm-commits.
varno updated this revision to Diff 82966.Jan 3 2017, 4:04 PM
  • Fixing Test with changes added to handle color.
dblaikie edited edge metadata.Jan 9 2017, 1:21 PM

Needs test coverage for the color functionality, I think? (I don't see any colors being tested in the tests)

tools/llvm-xray/xray-graph.cc
264–270

Could use a member pointer & a helper function (possibly template) to maybe make these shorter/more reliable (by only having to mention the member name once - less chance of accidentally mismatching them).

310

Typo, 'function'

Also probably s/Normalise Statistics/normaliseStatistics/

Also, normalize rather than normalise (consistency with the rest of the codebase)

419–442

Could use another member pointer helper to handle this table, if you like.

dberris added inline comments.Jan 12 2017, 1:14 AM
tools/llvm-xray/xray-graph.cc
373–377

Probably missing a few punctuation marks. I think some structured fomulae might help with what exactly it's doing, like:

// Evaluates a polynomial given the provided range of coefficients in order.
//
//    p(x) = a[0]*x^0 + a[1]*x^1 + a[2]*x^2 + ... + a[n]*x^n
//
// We use Horner's Method for numerical stability for mapping to an 8-bit
// fixed point domain.
394–402

You probably want to document how you came up with these polynomials, even potentially citing some sources on how one might possibly start changing these.

405–407

Reading this, I think you're better off making polyEval take an llvm::ArrayRef instead (to make it easier to read).

444

Can you document why compare is returning the square root? You mentioned this offline in our discussion, but it would be good to document the reasons here.

varno updated this revision to Diff 84218.Jan 12 2017, 7:55 PM
varno edited edge metadata.
  • Clang format
varno marked 2 inline comments as done.Jan 12 2017, 7:56 PM

Fixed and addressed comments.

varno added inline comments.Jan 12 2017, 7:59 PM
tools/llvm-xray/xray-graph.cc
264–270

Discussion resulted in this comment being seen as not easy to do due to specifics.

dberris edited edge metadata.Jan 22 2017, 7:27 PM

In general I think this could do with a bit more documentation, about the semantics of what we're trying to do (as opposed to how we're doing it).

tools/llvm-xray/xray-graph.cc
170

Empty line after this?

171

This is a template, no need to be static (it's implicitly inline).

175

Style guide says identifiers ought to have an uppercase start letter.

184–187

Can we describe this in less round-about way?

We can spend a few words in documentation to indicate what the side-effects are and some detail of what the algorithm being implemented is.

192–193

Was the intent to use llvm::Expected<...>? In which case I'd prefer you change this now.

dberris requested changes to this revision.Jan 22 2017, 7:33 PM

Also, I think you'd want to rebase this given that llvm-xray graph has landed upstream.

This revision now requires changes to proceed.Jan 22 2017, 7:33 PM
varno updated this revision to Diff 85324.Jan 22 2017, 10:07 PM
varno edited edge metadata.
  • Clang format
  • Fixed tests after rebase
  • Addressed review comments
varno marked 5 inline comments as done.Jan 22 2017, 10:08 PM

Also has been rebased to master.

dberris requested changes to this revision.Jan 22 2017, 10:38 PM
dberris added inline comments.
tools/llvm-xray/xray-graph.cc
396

Empty line after this?

398

s/propper/proper/

409–410

Leave out the "I" and "Mathematica" parts -- explain the concepts involved, not the mechanics.

416–417

Instead of "subjectively provided a good result", say something like: "we used a 9-degree polynomial arbitrarily based on a fuzzy goodness of fit metric (using human judgment)".

430–431

Unnecessary now?

439–440

s/annother/another/

572

Do you need the equivalent of a "keep-going" flag here too (similar to what we have in llvm-xray account)?

This revision now requires changes to proceed.Jan 22 2017, 10:38 PM
varno updated this revision to Diff 85507.Jan 23 2017, 5:55 PM
varno edited edge metadata.
varno marked 6 inline comments as done.
  • Reply to comments
varno marked 6 inline comments as done.Jan 23 2017, 5:58 PM
dberris requested changes to this revision.Jan 23 2017, 9:37 PM
dberris added inline comments.
tools/llvm-xray/xray-graph.cc
572

Okay, now I think you need to report errors encountered that we're ignoring in case we do want to keep going. Accumulating the individual errors into a long list seems unnecessary to me.

580–586

This looks like it'd be better as:

if (auto E = GR.accountRecord(Record)) {
  ...
}
This revision now requires changes to proceed.Jan 23 2017, 9:37 PM
varno updated this revision to Diff 85644.Jan 24 2017, 3:25 PM
varno edited edge metadata.
  • Respond to comments
  • clang-format new
varno updated this revision to Diff 85645.Jan 24 2017, 3:28 PM
  • clang-format fix
varno marked an inline comment as done.Jan 24 2017, 3:29 PM
varno added inline comments.
tools/llvm-xray/xray-graph.cc
580–586

This does not quite work here.

dberris accepted this revision.Jan 24 2017, 3:54 PM

LGTM -- we can probably use some more user-facing documentation explaining a bit what this tool actually does (probably time to update docs/XRay.rst) but something to be done for later.

tools/llvm-xray/xray-graph.cc
581

In the future, comments like these really ought to expound more on what it's actually doing at a semantic level to help the reader of the code. A more informative comment as to "why" we're doing something might make more sense to the reader as that's not immediately obvious. Something like:

// Populate the graph with function call records that we
// encounter from the XRay trace. Any errors we encounter
// will be printed through standard error.
This revision is now accepted and ready to land.Jan 24 2017, 3:54 PM
This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.Jan 25 2017, 8:12 AM
llvm/trunk/tools/llvm-xray/xray-graph.cc
212–213 ↗(On Diff #85700)

Should probably be using llvm::errc - see the Support/errc.h - it defines portable constants we know work across all the compilers LLVM supports, etc.

401–403 ↗(On Diff #85700)

Usually don't bother with {} on single line scopes.

586–592 ↗(On Diff #85700)

Should accountRecord do this work/include this information in its Error (rather than callers doing so)?

llvm/trunk/tools/llvm-xray/xray-graph.h
135 ↗(On Diff #85700)

Const value return type? I'm assuming this should be a const ref return type? (we should really have a clang-tidy for that if we don't already, maybe even a straight-up compiler warning)