[PATCH] D27243: Initial work on the XRay Graph tool.
Dean Michael Berris via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 6 19:57:08 PST 2016
dberris added a comment.
Some style comments.
As for tests I think this is an OK set for first tests, but it'd be good to look at error conditions and making sure we're handling those appropriately.
================
Comment at: tools/llvm-xray/xray-graph.cc:121
+ VertexAttrs[Record.FuncId].SymbolName = FuncIdHelper.SymbolOrNumber(Record.FuncId);
+ ThreadStack.push_back({Record.FuncId, Record.TSC});
+ break;
----------------
Can you use `.emplace_back(...)` instead here?
================
Comment at: tools/llvm-xray/xray-graph.cc:155-156
+ return true;
+}
+template <typename U, typename V>
+void GraphRenderer::getStats(U begin, V end, GraphRenderer::TimeStat &S){
----------------
Please add an empty line between the end of function definitions, and the start of the next function.
================
Comment at: tools/llvm-xray/xray-graph.h:27-28
+namespace llvm {
+namespace xray {
+/// A class encapsulating the logic related to analyzing XRay traces, producting
+/// Graphs from them and then exporting those graphs for review.
----------------
Please add an empty line between last non-comment and first comment lines.
================
Comment at: tools/llvm-xray/xray-graph.h:75
+ /// FIXME: Perhaps we can Build this into LatencyAccountant? or vise versa?
+ DenseMap<pid_t, std::vector<FunctionAttr>> PerThreadFunctionStack;
+
----------------
Can you use a `SmallVector<...>` instead of a `std::vector<...>`?
================
Comment at: tools/llvm-xray/xray-graph.h:115-124
+ /// Calculates latency statistics for each edge and stores the data in the
+ /// Graph
+ void calculateEdgeStatistics(void);
+
+ ///Calculates latency statistics for each vertex and stores the data in the
+ /// Graph
+ void calculateVertexStatistics(void);
----------------
Can this not happen incrementally, as we're adding the records, or on demand when we export? i.e. why do they need to be public functions?
================
Comment at: tools/llvm-xray/xray-graph.h:126-127
+private:
+ /// A private function to facilitate the output of edge labels;
+ void outputEdgeInfo(const TimeStat &S, StatType T, raw_ostream &OS) const;
+};
----------------
Does this need to be part of the class? Could this not be a function in the implementation?
https://reviews.llvm.org/D27243
More information about the llvm-commits
mailing list