[PATCH] D29320: [XRay] A tool for Comparing xray function call graphs

Fri Feb 24 20:41:28 PST 2017

dberris requested changes to this revision.
dberris added a comment.
This revision now requires changes to proceed.

Lots of readability comments, and a couple of functionality questions.

================
Comment at: tools/llvm-xray/xray-graph-diff.cc:222-238
+  for (int i = 0; i < N; ++i)
+    for (const auto &V : G[i].get().vertices())
+      R.G[V.second.SymbolName].P[i] = &V;
+
+  for (int i = 0; i < N; ++i) {
+    for (const auto &E : G[i].get().edges()) {
+      auto SOrErr = G[i].get().at(E.first.first);
----------------
Looks like this is better merged:

```
for (auto i = 0; i < N; ++i) {
  const auto &G = G[i].get();
  for (const auto &V : G.vertices())
    ...
  for (const auto &E : G.edges())
    ...
```

================
Comment at: tools/llvm-xray/xray-graph-diff.cc:223
+  for (int i = 0; i < N; ++i)
+    for (const auto &V : G[i].get().vertices())
+      R.G[V.second.SymbolName].P[i] = &V;
----------------
For readability purposes, I'd suggest you alias `G[i].get()` to reduce the visual noise.

================
Comment at: tools/llvm-xray/xray-graph-diff.cc:228-229
+    for (const auto &E : G[i].get().edges()) {
+      auto SOrErr = G[i].get().at(E.first.first);
+      auto EOrErr = G[i].get().at(E.first.second);
+      if (!SOrErr)
----------------
What is "S" and "E" here? It would really help if you use better names here. It will make more sense if you do something like:

```
  auto &FromVertex = E.first.first;
  auto &ToVertex = E.first.second;
  auto FromVertexOrErr = G.at(FromVertex);
  auto ToVertexOrErr = G.at(ToVertex);
  ...
```

================
Comment at: tools/llvm-xray/xray-graph-diff.cc:236
+                                                  EOrErr->SymbolName};
+      R.G[I].P[i] = &E;
+    }
----------------
This is really unreadable. What is P?

================
Comment at: tools/llvm-xray/xray-graph-diff.cc:242
+  for (const auto &V : R.G.vertices()) {
+    R.IM[V.first] = i++;
+
----------------
This is really opaque -- what does `IM` stand for? Please pick a more meaningful name. It's also not clear why we're using a monotonic number here.

================
Comment at: tools/llvm-xray/xray-graph-diff.cc:244
+
+    if (V.second.P[0] == nullptr || V.second.P[1] == nullptr)
+      continue;
----------------
dberris wrote:
> So let me get this right -- if one of the vertex attribute's parents doesn't have it, then we move on. So we don't indicate that the vertex is only seen in one of the graphs?
I suggest using enum names instead of `0` and `1` here, to aid in readability. Something like:

```
enum { LEFT = 0, RIGHT = 1 };
auto &VAttr = V.second;
if (VAttr.Parent[LEFT] == nullptr || VAttr.Parent[RIGHT] == nullptr)
  continue;
```

================
Comment at: tools/llvm-xray/xray-graph-diff.cc:244-245
+
+    if (V.second.P[0] == nullptr || V.second.P[1] == nullptr)
+      continue;
+
----------------
So let me get this right -- if one of the vertex attribute's parents doesn't have it, then we move on. So we don't indicate that the vertex is only seen in one of the graphs?

================
Comment at: tools/llvm-xray/xray-graph-diff.cc:282-302
+  auto IP = maxDiff(MA.Count, MB.Count, A.Count, B.Count);
+  MA.Count = IP.first;
+  MB.Count = IP.second;
+  auto DP = maxDiff(MA.Min, MB.Min, A.Min, B.Min);
+  MA.Min = DP.first;
+  MB.Min = DP.second;
+  DP = maxDiff(MA.Median, MB.Median, A.Median, B.Median);
----------------
All this repetition seems unnecessary. It seems you might be able to do this with a loop:

```
auto MemPtrs[] = { &TimeStat::Count, &TimeStat::Min, &TimeStat::Median, ... };
for (auto Mem : MemPtrs) {
  auto Diff = maxDiff(MA.*Mem, MB.*Mem);
  MA.*Mem = Diff.first;
  MB.*Mem = Diff.second;
}
```

================
Comment at: tools/llvm-xray/xray-graph-diff.cc:324-327
+  double R = difference(EdgeAttr.P[0]->second.S.getDouble(T),
+                        EdgeAttr.P[1]->second.S.getDouble(T));
+  double MR = difference(G.EdgeMaxDiff.first.getDouble(T),
+                         G.EdgeMaxDiff.second.getDouble(T));
----------------
What is `R` and what is `MR` -- this code looks really hard to read. I can't even begin to explain what this is actually doing (what's `EdgeAttr.P[0]->second.S` ?).

================
Comment at: tools/llvm-xray/xray-graph-diff.cc:334
+    MR = 1.0;
+  assert(std::abs(R) <= std::abs(MR) && "Manumim should be no smaller");
+
----------------
What is Manumim? Did you mean Maximum?

================
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";
----------------
Why are these colours hard-coded, when you already have a ColorHelper available?

================
Comment at: tools/llvm-xray/xray-graph-diff.cc:369-372
+  case GraphDiffRenderer::StatType::NONE:
+    return "";
+  default:
+    return "";
----------------
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?

================
Comment at: tools/llvm-xray/xray-graph-diff.cc:382
+  case GraphDiffRenderer::StatType::NONE:
+    S << "label=\"" << truncateString(V.first, 40) << '\"';
+    break;
----------------
Looks like you might as well just use `formatv` here, as it's more confusing to read this as is.

================
Comment at: tools/llvm-xray/xray-graph-diff.cc:405
+       << "F" << IM[E.first.second] << " [tooltip=\""
+       << truncateString(E.first.first, 20) << " -> "
+       << truncateString(E.first.second, 20) << "\"" << getLabel(E, EdgeLabel)
----------------
Why are we truncating the tooltip? It seems it might be better to provide the whole string in the tooltip so in cases where we can keep them (in SVG) then the truncated names show up in the code, and on hover will show more information.

================
Comment at: tools/llvm-xray/xray-graph-diff.h:38-44
+  struct EdgeAttribute {
+    std::array<const GREdgeValueType *, N> P = {};
+  };
+
+  struct VertexAttribute {
+    std::array<const GRVertexValueType *, N> P = {};
+  };
----------------
Please use a better identifier than `P` -- Is this meant to indicate `Parent`? If so, just use that to make it easier to read. If not, then please use a more descriptive name.

https://reviews.llvm.org/D29320