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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

Now had a look closely at the implementation. Please pardon the piecemeal review.



================
Comment at: tools/llvm-xray/xray-graph.cc:102
+namespace {
+
+template <class T> T diff(T L, T R) { return std::max(L, R) - std::min(L, R); }
----------------
Potentially spurious empty line here.


================
Comment at: tools/llvm-xray/xray-graph.cc:117
+bool GraphRenderer::accountRecord(const XRayRecord &Record) {
+
+  if (CurrentMaxTSC == 0)
----------------
Also potentially spurious empty line here.


================
Comment at: tools/llvm-xray/xray-graph.cc:142-148
+      while (ThreadStack.back().FuncId != Record.FuncId) {
+        uint64_t T = diff(ThreadStack.back().TSC, Record.TSC);
+        int32_t TopFuncId = ThreadStack.back().FuncId;
+        ThreadStack.pop_back();
+        assert(ThreadStack.size() != 0);
+        Graph[ThreadStack.back().FuncId][TopFuncId].Timings.push_back(T);
+      }
----------------
So I think in a future change which we talked about offline, we ought to make this and the `account` tool work with each other in some form of refactored implementation -- where the account tool can depend on the graph tool instead of duplicating a lot of this logic.

If you rebase against the `account` change (D24377) then the version used there now has less calls to .pop_back() on the vector, and also has a simpler loop as a result (i.e. you don't need the extra spillover logic, and the version there has less nesting and more straight-line code anyway.

The actionable comment right now I think would be a `//FIXME: Refactor this and the account subcommand to reduce code duplication`


================
Comment at: tools/llvm-xray/xray-graph.cc:150
+    }
+    uint64_t T = diff(ThreadStack.back().TSC, Record.TSC);
+    ThreadStack.pop_back();
----------------
Maybe use a name that isn't overloaded in this context -- `D` is a perfectly fine letter for something that indicates the delta between two numbers.


================
Comment at: tools/llvm-xray/xray-graph.cc:204-205
+  }
+}
+void GraphRenderer::calculateEdgeStatistics(void) {
+  for (auto &V : Graph) {
----------------
Empty line in between, and don't need void in the function arguments.


================
Comment at: tools/llvm-xray/xray-graph.cc:214
+
+void GraphRenderer::calculateVertexStatistics(void) {
+  DenseMap<int32_t, std::vector<EdgeAttribute *>> IncommingEdges;
----------------
No need for void in the function arguments.


================
Comment at: tools/llvm-xray/xray-graph.cc:215
+void GraphRenderer::calculateVertexStatistics(void) {
+  DenseMap<int32_t, std::vector<EdgeAttribute *>> IncommingEdges;
+  for (auto &V : Graph) {
----------------
Consider also using a `SmallVector` here, with potentially the same initial size as with the adjacency list we maintain.


================
Comment at: tools/llvm-xray/xray-graph.cc:221
+  }
+  std::vector<uint64_t> Timings;
+  for (auto &V : IncommingEdges) {
----------------
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. :)


================
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();
----------------
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.


================
Comment at: tools/llvm-xray/xray-graph.h:88
+  template <typename U, typename V>
+  void getStats(U begin, V end, GraphRenderer::TimeStat &S);
+
----------------
Why are the iterators not the same type?


================
Comment at: tools/llvm-xray/xray-graph.h:92
+  /// Graph
+  void calculateEdgeStatistics(void);
+
----------------
No need for `void` in the function arguments.


https://reviews.llvm.org/D27243