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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: tools/llvm-xray/xray-graph.cc:84-88
+    if (ThreadStack.size() == 0) {
+      ++(Graph[0][Record.FuncId].CallCount);
+    } else {
+      ++(Graph[ThreadStack.back()][Record.FuncId].CallCount);
+    }
----------------
could potentially use a conditional operator inside the [] since the expression is otherwise identical.

Also, the outer () aren't necessary - but you can keep them if you find they enhance readability.


================
Comment at: tools/llvm-xray/xray-graph.cc:89-91
+    if (VertexAttrs.count(Record.FuncId) == 0) {
+      VertexAttrs[Record.FuncId] = {FuncIdHelper.SymbolOrNumber(Record.FuncId)};
+    }
----------------
Consider dropping {} on single line blocks.


================
Comment at: tools/llvm-xray/xray-graph.cc:116-120
+  for (const auto &V : Graph)
+    for (const auto &E : V.second) {
+      OS << "F" << V.first << " -> "
+         << "F" << E.first << " label=\"" << E.second.CallCount << "\"];\n";
+    };
----------------
Inconsistent bracing (why does the inner loop have braces but the outer loop doesn't? - personally I'd probably drop them from both, but I could see an argument for adding them to both - just doesn't seem right to add them only to one and not the other)


================
Comment at: tools/llvm-xray/xray-graph.cc:120
+         << "F" << E.first << " label=\"" << E.second.CallCount << "\"];\n";
+    };
+
----------------
Extra semicolon


================
Comment at: tools/llvm-xray/xray-graph.cc:128
+       << "\"];\n";
+  };
+  OS << "}\n";
----------------
Extra semi


================
Comment at: tools/llvm-xray/xray-graph.cc:132-133
+
+namespace llvm {
+namespace xray {
+
----------------
Drop these (it's just a static variable in this scope anyway)


================
Comment at: tools/llvm-xray/xray-graph.cc:174-183
+  LogReader::LoaderFunction Loader;
+  switch (GraphInputFormat) {
+  case GraphInputFormats::RAW:
+    Loader = NaiveLogLoader;
+    break;
+  case GraphInputFormats::YAML:
+    Loader = YAMLLogLoader;
----------------
Should be some common utility for this, so every tool doesn't have to go through the same hoops (probably coalesce all the instrumentation map extractor stuff as well)


================
Comment at: tools/llvm-xray/xray-graph.h:30
+class GraphRenderer {
+private:
+  /// An inner struct for storing edge attributes for our graph. Here the
----------------
No need for the explicit 'private' as it's implied/the default here.


================
Comment at: tools/llvm-xray/xray-graph.h:41
+  /// An attached set of attributes.
+  std::unordered_map<int32_t, std::unordered_map<int32_t, EdgeAttribute>> Graph;
+
----------------
(Consider LLVM's data structures - can be more memory efficient than the allocation-per-node of standard containers.

Also a map of maps might be more efficient as a map of pair -> value, if it's equivalent for your use case)


================
Comment at: tools/llvm-xray/xray-graph.h:59
+  /// FIXME: Perhaps we can Build this into LatencyAccountant? or vise versa?
+  std::unordered_map<pid_t, std::vector<int32_t>> PerThreadFunctionStack;
+
----------------
(similarly, consider other data structures - but also maybe consider multimap rather than map to vector)


================
Comment at: tools/llvm-xray/xray-graph.h:68
+  explicit GraphRenderer(FuncIdConversionHelper &FuncIdHelper)
+      : FuncIdHelper(FuncIdHelper){};
+
----------------
Extra semicolon


https://reviews.llvm.org/D27243