[PATCH] D29320: [XRay] A tool for Comparing xray function call graphs
Dean Michael Berris via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 8 00:23:35 PST 2017
dberris requested changes to this revision.
dberris added a comment.
This revision now requires changes to proceed.
More readability comments. In general all the escaped strings and broken up stream operations don't read well. A better option is using formatv(...) and consolidating those into format strings that are specified as raw string literals would make all the output generation code much more readable.
================
Comment at: tools/llvm-xray/xray-graph-diff.cc:342-345
+ if (V.second.P[0] == nullptr)
+ return "green";
+ if (V.second.P[1] == nullptr)
+ return "red";
----------------
varno wrote:
> dberris wrote:
> > Why are these colours hard-coded, when you already have a ColorHelper available?
> These are different colors not on the scale for if the vertex or edge does not exist (as opposed to encoding some difference. They should not come from the ColorHelper but are specified separately.
I can reverse that question too -- why isn't the choice for what colour to use for when something is missing from one side not in the ColorHelper? It seems weird that these are the only colours hard-coded when I'd expect as a reader/maintainer of this that this information fits really well in the ColorHelper utility.
Something like a categorisation amongst "in both sets", "in set A", "in set B", "in neither". We could just use hard-coded colours to indicate those four states in the ColorHelper, and centralise where all the Colors are defined.
================
Comment at: tools/llvm-xray/xray-graph-diff.cc:369-372
+ case GraphDiffRenderer::StatType::NONE:
+ return "";
+ default:
+ return "";
----------------
varno wrote:
> dberris wrote:
> > What is this meant to do? Did you mean to provide stringified labels? Why isn't this just always returning the empty string? Is this supposed to also provide the value of the difference, in relative terms as a string appropriately coloured?
> I am not sure what to do here in labeling edges with differences, so I have a TODO here, If you can tell me what you think I should put there I will change this.
How about providing the relative differences? Something like "+10%" or "-2.3%" in the same colour as the edge/vertex?
================
Comment at: tools/llvm-xray/xray-graph-diff.cc:226
+ for (const auto &V : G.vertices())
+ R.G[V.second.SymbolName].CorrVertexPtr[i] = &V;
+ for (const auto &E : G.edges()) {
----------------
What is `V.second`? Is this the vertex attribute? Aliasing this will make it much easier to read:
```
const auto& VAttr = V.second;
```
================
Comment at: tools/llvm-xray/xray-graph-diff.cc:242-244
+ for (const auto &V : R.G.vertices()) {
+ R.updateVertexDiff(V.second);
+ }
----------------
Single line body; prefer no {}'s.
================
Comment at: tools/llvm-xray/xray-graph-diff.cc:253-255
+// This function calculates a value related to the difference of two numbers
+// which ranges onto [-infty, infty] with the sign corresponding to which number
+// is larger.
----------------
As documented, it doesn't say what a "negative" value means -- so for values 1 and 2, it will result in:
(1/2) (0.5) - (2) = -1.5
I presume that the sign implies says the value on the left is larger? Or is that in reverse? It would be better if the comment actually stated this.
Alternatively, if you used a relative computation instead and yielded the percentage difference in one direction, and can use that as a label too. That would look something like:
-(Left - Right / Left)
If Left == 0, then it's +100%. If Right is 0 then it's -100%.
================
Comment at: tools/llvm-xray/xray-graph-diff.cc:256
+// is larger.
+static double difference(double A, double B) { return A / B - B / A; }
+static std::pair<double, double> maxDiff(double AA, double AB, double BA,
----------------
Reading this now, I think 'difference' is a little misleading a name for what it does. It seems to be a scaled relative difference, because it's not just A - B (as the name "difference" would suggest). So one of: `scaledRelativeDiff(...)` or `ratioDiff(...)` would make this less confusing as it says what it means.
================
Comment at: tools/llvm-xray/xray-graph-diff.cc:281-282
+ auto DP = maxDiff(MA.*Mem, MB.*Mem, A.*Mem, B.*Mem);
+ MA.*Mem = DP.first;
+ MB.*Mem = DP.second;
+ }
----------------
Alternative way of spelling this:
```
std::tie(MA.*Mem, MB.*Mem) = DP;
```
Or just:
```
std::tie(MA.*Mem, MB.*Mem) =
maxDiff(MA.*Mem, MB.*Mem, A.*Mem, B.*Mem);
```
This might also benefit from better names. Mem isn't the best name, but "Stat" might be more indicative of what it actually is. DoubleMemPtrs looks like it doesn't need to be given a name, the for loop could just have it there. Also, `MA` and `MB` don't mean much in terms of names. So rewriting that to be more succinct:
```
for (auto Stat : {&TimeStat::Min, ...})
std::tie(StatLeft.*Stat, StatRight.*Stat) =
maxDiff(StatLeft.*Stat, StatRight.*Stat, NewStatLeft.*Stat, NewStatRight.*Stat);
```
================
Comment at: tools/llvm-xray/xray-graph-diff.cc:385
+ case GraphDiffRenderer::StatType::NONE:
+ return formatv("label=\"{0}\"", truncateString(V.first, 40).str());
+ default:
----------------
Use raw string literals to avoid the problem of having to escape quotes.
================
Comment at: tools/llvm-xray/xray-graph-diff.cc:385
+ case GraphDiffRenderer::StatType::NONE:
+ return formatv("label=\"{0}\"", truncateString(V.first, 40).str());
+ default:
----------------
dberris wrote:
> Use raw string literals to avoid the problem of having to escape quotes.
Can 40 not be hard-coded here? Consider a flag to control string length truncation.
================
Comment at: tools/llvm-xray/xray-graph-diff.cc:412-415
+ OS << "F" << VertexNo[E.first.first] << " -> "
+ << "F" << VertexNo[E.first.second] << " [tooltip=\"" << E.first.first
+ << " -> " << E.first.second << "\"" << getLabel(E, EdgeLabel)
+ << " color=\"" << getColor(E, G, H, EdgeColor) << "\"]\n";
----------------
This is completely unreadable. Consider using the combination of formatv(...) and raw string literals instead. Also, please use aliases to make things like `E.first.first` say something a bit more readable. Something like:
```
const auto &SourceVertex = E.first.first;
const auto &DestinationVertex = E.first.second;
```
================
Comment at: tools/llvm-xray/xray-graph-diff.cc:437-481
+ GraphRenderer::Factory F1;
+ GraphRenderer::Factory F2;
+
+ F1.KeepGoing = ifSpecified(GraphDiffKeepGoing1, GraphDiffKeepGoing1A,
+ GraphDiffKeepGoing);
+ F2.KeepGoing = ifSpecified(GraphDiffKeepGoing2, GraphDiffKeepGoing2A,
+ GraphDiffKeepGoing);
----------------
Some of this duplication seems a little unnecessary. How about using control flow to reduce the repetition?
```
struct GraphData {
bool KeepGoing;
bool DeduceSiblings;
string InstrMapFile;
string Input;
GraphRenderer::Factory Factory;
Trace Trace;
GraphRenderer Renderer;
Graph Graph;
};
std::array<GraphData, 2> Graphs {
{ isSpecified(KeepGoing1), ... },
{ isSpecified(KeepGoing2), ...},
};
for (auto &Graph : Graphs) {
Graph.Factory.KeepGoing = Graph.KeepGoing;
...
auto TraceOrError = loadTraceFile(Graph.Input, true);
if (!TraceOrError) ...
Graph.Trace = std::move(*TraceOrError);
...
}
================
Comment at: tools/llvm-xray/xray-graph.cc:374-402
+double GraphRenderer::TimeStat::getDouble(StatType T) const {
double retval = 0;
switch (T) {
case GraphRenderer::StatType::COUNT:
- retval = static_cast<double>(Count) / static_cast<double>(O.Count);
+ retval = static_cast<double>(Count);
break;
case GraphRenderer::StatType::MIN:
----------------
Now that I read this, I'm more convinced of a mapping between the enum and data member pointers.
https://reviews.llvm.org/D29320
More information about the llvm-commits
mailing list