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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: tools/llvm-xray/CMakeLists.txt:16
+  xray-registry.cc
+  xray-graph.cc)
 
----------------
nit: we try to somewhat keep this in lexicographical order.


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


================
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;
+
----------------
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.


https://reviews.llvm.org/D27243