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

[Alexis Shaw via Phabricator via llvm-commits]

varno added inline comments.


================
Comment at: tools/llvm-xray/xray-graph-diff.cc:369-372
+  case GraphDiffRenderer::StatType::NONE:
+    return "";
+  default:
+    return "";
----------------
dberris wrote:
> varno wrote:
> > 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.
> Why not?
> 
> User gave you two traces. We're showing them differences. Using a made-up relative difference as opposed to ((A - B / A) * 100%) seems wrong and not helpful. We want to make sure the magnitude of the difference is what we use to highlight the edge or the vertex. Any other scheme would be trying to be too clever, and be actively harmful because it wouldn't make sense to the user trying to make sense of the data. If you don't provide the relative difference then it's effectively useless -- I as a user wouldn't be able to tell how severe the difference is in relative terms which is the whole point of the diff tool.
> 
> What makes the current way better than using the relative difference for the stat chosen?
Hi Dean.

I Agree that for the textrual representation that RatioDiff is not the best answer, however for coloring the differences and determining what the largest difference is, this is just fine as both as the graph plotting ratioDiff vs (A-B)/A is monotonic. The properties of the chosen funciton Namely the symmetry allow us to compare the amount of change going positive and negative with each other, which is a useful property.

The reason that this is better is that the order of diff is mostly arbitrary, and so A/B - B;/A behaves like max(A,B)/min(A,B) away from where A and B are similar and properly when A, and B are close to each other then it behaves like (A-B)/A. However even here the behavior is better behaved. Also for choosing colors this is easier. the function (A-B)/ A just treats A, and B too differently to work well for coloring edges (I tried this first at google btw as it was the "obvious" answer).

However as a User in a textual representation I agree that we want to use a different metric, that is why I store both sides of the max and minimum statistics and not just the diff. There I am not sure whether we want to show percentage difference, absolute difference or just both numbers in the change. The decision here also will likely be different for edges and vertices, with edges probably wanting something simple like the percentage change or absolute  difference, while the vertices you may want to have both the old and new numbers plus an amount of change.


================
Comment at: tools/llvm-xray/xray-graph-diff.cc:265-266
+// which ranges onto [-infty, infty] with the sign corresponding to which number
+// is larger. If A is larger than B then this will be a positive number, if A
+// is less than B it will be a positive number. The function is such that if A
+// and B are swapped then the sidn of the comparrison function is reversed.
----------------
dberris wrote:
> So this is always a positive number?
Sorry,  bad comment will fix. 


https://reviews.llvm.org/D29320