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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris requested changes to this revision.
dberris added inline comments.
This revision now requires changes to proceed.


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


================
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.
----------------
So this is always a positive number?


https://reviews.llvm.org/D29320