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

[Alexis Shaw via Phabricator via llvm-commits]

varno marked 11 inline comments as done.
varno added inline comments.


================
Comment at: tools/llvm-xray/xray-graph-diff.cc:244-245
+
+    if (V.second.P[0] == nullptr || V.second.P[1] == nullptr)
+      continue;
+
----------------
dberris wrote:
> 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;
> ```
That the Vertex is in only one of the graphs is encoded in the fact that the corresponding pointer is null. So Yeah We do indicate this, i.e. if there is no link the 0th or 1st vertex does not exist in the corresponding graph.


================
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);
----------------
dberris wrote:
> 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;
> }
> ```
Can't quite do this due to type issues but I have fixed this most of the way there.


================
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";
----------------
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.


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


https://reviews.llvm.org/D29320