[PATCH] D21987: [XRay] Initial XRay Function Call Accounting Tool
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 6 09:30:47 PDT 2016
dblaikie added a comment.
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.
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).
& 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)
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.
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.
================
Comment at: tools/llvm-xray/CMakeLists.txt:3-6
@@ +2,6 @@
+ ${LLVM_TARGETS_TO_BUILD}
+ Support
+ DebugInfoDWARF
+ Symbolize
+ Object)
+
----------------
I think we usually keep these lists (including the source file lists below) sorted... though I see I've not done that consistently in the past either.
================
Comment at: tools/llvm-xray/xray-account.cc:27
@@ +26,3 @@
+ if (MM.first == 0 || MM.second == 0)
+ MM = std::make_pair(std::forward<U>(V), std::forward<U>(V));
+ else
----------------
Calling forward on the same object more than once seems problematic - if this was ever passed as an xvalue the pair would construct from the xvalue, then again from the xvalue but now with that value in a moved-from state.
(also does'nt seem like it needs to be a template - that might be overly generic at this point/given the current code)
================
Comment at: tools/llvm-xray/xray-account.cc:32
@@ +31,3 @@
+
+template <class T> T diff(T L, T R) { return std::max(L, R) - std::min(L, R); }
+
----------------
Does this need to be a template? Should the parameters be const ref? (might be easier to judge whether const ref is missing if it wasn't a template so we could see what sort of types it was for)
================
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";
+ });
+}
----------------
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)
================
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(); }
+};
----------------
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.
================
Comment at: tools/xray/xray-extract.cc:32-33
@@ +31,4 @@
+
+// Returns {"message", false} on errors; {"", true} otherwise.
+std::pair<StringRef, bool> LoadBinaryInstrELF(
+ StringRef Filename, std::deque<XRaySledEntry> &Sleds,
----------------
Should this be llvm::Error return instead?
================
Comment at: tools/xray/xray-extract.cc:37-38
@@ +36,4 @@
+ InstrumentationMapExtractor::FunctionAddressReverseMap &FunctionIds) {
+ if (Filename.empty())
+ llvm_unreachable("Provided Filename is empty.");
+
----------------
branch-to-unreachable should be an assert instead
https://reviews.llvm.org/D21987
More information about the llvm-commits
mailing list