[PATCH] D21987: [XRay] Initial XRay Function Call Accounting Tool

[Dean Michael Berris via llvm-commits]

dberris planned changes to this revision.
dberris marked 2 inline comments as done.
dberris added a comment.

In https://reviews.llvm.org/D21987#534750, @dblaikie wrote:

> It's sort of a bit hard to see the forest for the trees reviewing this - I know it's a pain to do smaller reviews that might end up with large refactorings that'll have knock-on effects to later patches, but I think that might work better. Stacking up lots of work unreviewed gets a bit tricky.


I agree. I'll break this up into multiple smaller changes, and update accordingly.

> Still some outstanding design issues to discuss re: file formats, conversions, portability, etc. But once that's done.

> 

> I'm still concerned about reading/writing the binary format using memory mapping/splatting - I think the tools should be portable (in the same way that LLVM/Clang are - we don't build cross compilers, we just build tools that can read/write from/to any format on any platform), but even that's an implementation detail to a degree (unless making the tools that generic removes the need for the separate portable format/conversion tools).


That's a tricky one. The problem with _not_ doing this memory-dump format is the cost of doing it in the XRay runtime. We try to be as cheap as possible there (in compiler-rt) so that we don't spend too much time just formatting the log when the process is writing it down to disk. The tradeoff we've made here consciously is that any conversion should happen externally -- i.e. the tool should be able to read this format, and then turn it into something more manageable offline. That's a very conscious trade-off here because we can't afford to use too much time or too much memory on the compiler-rt implementation.

> & not sure about YAML+JSON. If you've a strong/unavoidable need for JSON, it's probably best to just implement that. (maybe it's best to just do JSON anyway if you just have a vague preference - as I said in the design review thread, I'm not sure how strong the binding to YAML is in the project)


Not strong, and if anything, I might just drop JSON. :)

> Higher level design/coding style stuff:

> 

> Might be worth pulling out each of the subcommands into their own utility function/file along with the command line arguments. Though I can see an argument (har har) to be made for keeping all the arguments in the same place - but maybe subcommand arguments make sense to be grouped together along with the functionality, separate from the main file.


I thought about that, then it started to feel over-engineered -- since I'd have to figure out how to dispatch anyway from the main file, I still need to know which command to call based on which subcommand, this seemed like the more prudent thing to do.

> Basic stylistic stuff: Commenting and header file layout looks fine. Oh, we don't seem to put the trailing _ in our header file names, and other headers in tools use "LLVM_TOOLS_FOO_H" rather than just "FOO_H", so that's probably a good idea to avoid naming conflicts.


Ah, right. Let me fix those.


================
Comment at: tools/llvm-xray/xray-account.cc:94-169
@@ +93,77 @@
+
+namespace {
+
+// We consolidate the data into a struct which we can output in various forms.
+struct ResultRow {
+  uint64_t Count;
+  double Min;
+  double Median;
+  double Pct90;
+  double Pct99;
+  double Max;
+  std::string DebugInfo;
+  std::string Function;
+};
+
+ResultRow getStats(std::vector<uint64_t> &Timings) {
+  ResultRow R;
+  std::nth_element(Timings.begin(), Timings.end(), Timings.end());
+  R.Min = Timings.front();
+  auto MedianOff = Timings.size() / 2;
+  std::nth_element(Timings.begin(), Timings.begin() + MedianOff, Timings.end());
+  R.Median = Timings[MedianOff];
+  auto Pct90Off = std::floor(Timings.size() * 0.9);
+  std::nth_element(Timings.begin(), Timings.begin() + Pct90Off, Timings.end());
+  R.Pct90 = Timings[Pct90Off];
+  auto Pct99Off = std::floor(Timings.size() * 0.99);
+  std::nth_element(Timings.begin(), Timings.begin() + Pct90Off, Timings.end());
+  R.Pct99 = Timings[Pct99Off];
+  R.Max = *std::max_element(Timings.begin(), Timings.end());
+  R.Count = Timings.size();
+  return R;
+}
+
+} // namespace
+
+template <class F>
+void LatencyAccountant::exportStats(const XRayFileHeader &Header, F Fn) const {
+  for (auto FT : FunctionLatencies) {
+    const auto &FuncId = FT.first;
+    auto &Timings = FT.second;
+    ResultRow Row = getStats(Timings);
+    if (Header.CycleFrequency) {
+      double CycleFrequency = Header.CycleFrequency;
+      Row.Min /= CycleFrequency;
+      Row.Median /= CycleFrequency;
+      Row.Pct90 /= CycleFrequency;
+      Row.Pct99 /= CycleFrequency;
+      Row.Max /= CycleFrequency;
+    }
+
+    Row.Function = FuncIdHelper.SymbolOrNumber(FuncId);
+    Row.DebugInfo = FuncIdHelper.FileLineAndColumn(FuncId);
+    Fn(FuncId, Timings.size(), Row);
+  }
+}
+
+void LatencyAccountant::exportStatsAsTEXT(raw_ostream &OS,
+                                          const XRayFileHeader &Header) const {
+  OS << "Functions with latencies: " << FunctionLatencies.size() << "\n";
+  OS << "funcid\t\tcount\t\t[min, median, 90%ile, 99%ile, "
+        "max]\tdebug\t\tfunction\n";
+  exportStats(Header, [&](int32_t FuncId, size_t Count, const ResultRow &Row) {
+    OS << FuncId << "\t\t" << Count << "\t\t[" << Row.Min << ", " << Row.Median
+       << ", " << Row.Pct90 << ", " << Row.Pct99 << ", " << Row.Max << "]  "
+       << Row.DebugInfo << "  " << Row.Function << "\n";
+  });
+}
+
+void LatencyAccountant::exportStatsAsCSV(raw_ostream &OS,
+                                         const XRayFileHeader &Header) const {
+  OS << "funcid,count,min,median,90%ile,99%ile,max,debug,function\n";
+  exportStats(Header, [&](int32_t FuncId, size_t Count, const ResultRow &Row) {
+    OS << FuncId << ',' << Count << ',' << Row.Min << ',' << Row.Median << ','
+       << Row.Pct90 << ',' << Row.Pct99 << ',' << Row.Max << ",\""
+       << Row.DebugInfo << "\",\"" << Row.Function << "\"\n";
+  });
+}
----------------
dblaikie wrote:
> This seems like more than is necessary - I know we were talking in the early versions about how to abstract out the different formats, but what I meant was whether we could abstract out the whole file format, rather than this one part of it. (eg: by having a "HumanWriter" and a "CSVWriter" that just get handed the data, etc). Probably doesn't need a whole record type of its own, and could just pass the relevant parameters to the function as arguments (though a struct doesn't hurt, I suppoes... *shrug)
Right -- I'm ambivalent about coming up with some generic writer interface here.

One the one hand, the stats generated by this one sub-command follow a very specific format, defined for this particular set of data. I don't see how we can abstract out a writer for CSV and for pretty-printed data, since we'd have different information anyway. For example, the text output currently just has auto-formatted values -- I am thinking of potentially using a formatting ostream here to fix column widths, etc. (very hard to do with a generic implementation).

On the other hand, if we could figure out a way of generating pretty-printed console I/O for arbitrary data, then that sounds like a bigger library than something that just does the simple thing. :)

================
Comment at: tools/llvm-xray/xray-log-reader.h:65-73
@@ +64,11 @@
+  const XRayFileHeader &getFileHeader() const { return FileHeader; };
+  iterator begin() { return Records.begin(); }
+  iterator end() { return Records.end(); }
+  citerator cbegin() { return Records.cbegin(); }
+  citerator cend() { return Records.cend(); }
+  citerator begin() const { return Records.begin(); }
+  citerator end() const { return Records.end(); }
+  citerator cbegin() const { return Records.begin(); }
+  citerator cend() const { return Records.end(); }
+  size_t size() const { return Records.size(); }
+};
----------------
dblaikie wrote:
> Wouldn't bother about implementing every variant of (c)(begin|end)(const) - just what you need. It's not like you're implementing the whole Container requirements anyway.
Believe it or not, it seemed like I needed all of these. :/


https://reviews.llvm.org/D21987