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

[Alexis Shaw via Phabricator via llvm-commits]

varno marked 8 inline comments as done.
varno added inline comments.


================
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;
----------------
dberris wrote:
> dblaikie wrote:
> > 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)
> +1 -- if you rebase again to the latest of D24377 you can use the utility function that determines the supported loader function here.
Yeah there probably should be. Not yet though AFAIK.


================
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;
+
----------------
dberris wrote:
> dblaikie wrote:
> > (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)
> http://llvm.org/docs/ProgrammersManual.html is a good resource to look up available data structures -- for instance, for map-like containers:
> 
> http://llvm.org/docs/ProgrammersManual.html#map-like-containers-std-map-densemap-etc
> 
> You have a few choices you can work with.
Thinking more on this before I make changes.


================
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;
+
----------------
dblaikie wrote:
> (similarly, consider other data structures - but also maybe consider multimap rather than map to vector)
Thinking on this more before I make changes.


https://reviews.llvm.org/D27243