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

[Alexis Shaw via Phabricator via llvm-commits]

varno marked 12 inline comments as done.
varno added a comment.

I have fixed most of the comments, and replied to all of them, Tests incoming soon.



================
Comment at: tools/llvm-xray/xray-graph-diff.cc:369-372
+  case GraphDiffRenderer::StatType::NONE:
+    return "";
+  default:
+    return "";
----------------
dberris wrote:
> 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?
I am not convinced that this is the right statistic to show, or whether this is the right way to go about coloring edges.


================
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.
----------------
dberris wrote:
> 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%.
I have improved the comment. I chose this function because it is symmetrical (that is diff(A,B) = -diff(B,A)) and it behaves similarly to a 

Right/Left - 1 which you suggest. behaves well if Right == 0 but not if left == 0. it does not treat the left and right hand sides symmetrically, nor close to it away from 1.


https://reviews.llvm.org/D29320