[PATCH] D27243: Initial work on the XRay Graph tool.

[Dean Michael Berris via Phabricator via llvm-commits]

dberris accepted this revision.
dberris added a comment.
This revision is now accepted and ready to land.

>From a style perspective and implementation I think this is mostly OK -- waiting on @dblaikie to have a look.



================
Comment at: tools/llvm-xray/xray-graph.cc:221-228
+  std::vector<uint64_t> Timings;
+  for (auto &V : IncommingEdges) {
+    for (auto &P : V.second) {
+      Timings.insert(Timings.begin(), P->Timings.begin(), P->Timings.end());
+    }
+    getStats(Timings.begin(), Timings.end(), VertexAttrs[V.first].S);
+    Timings.clear();
----------------
varno wrote:
> dberris wrote:
> > dberris wrote:
> > > Is there no way to do this in a single pass, in the same loop above? I could imagine building a map that contains a vertex as the key, then accumulating statistics as you go traversing the graph (or even just storing the data anyway like you do here for the timings), and another pass to collapse that data.
> > It's thoroughly confusing too that you're using Timings here, but there's a data member named Timings as well. :)
> Really not, the way here has linear space and uses the required memory for each section. If we did not care about memory I could store the accumulation data for both edges and vertices. but otherwise ???
Right, so the alternate body of the function seems to be more straight-forward, and the amount of memory being used isn't that bad. You can even be smart about this and have an upper bound on the elements in the vector, and as you insert you keep a relative order and track median, and other percentiles incrementally. That's not as critical as the functionality though, and we can tune this as we go along late anyway if we encounter really big graphs in practice that would cause this to be a huge problem (either as implemented or even in the alternative version).

I'm happy either way if you choose to stick with the current implementation, but if you do consider just removing the alternate implementation you have here `#ifdef 0`'d out.


https://reviews.llvm.org/D27243