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

[David Blaikie via llvm-commits]

dblaikie added a comment.

There's some stylistic response - higher level, I think maybe 'auto' might be a bit overused in this code (when suggesting to use "size" rather than "distance" I had to go a long way to find out whether the object's type supported size). Also, maybe splitting out some chunks of this into separate functions not for reuse but just to give them names and make the high level algorithm more visible/legible, might be an idea.

Also - is there a nice way to split out the format writing handling (CSV v Text) into an orthogonal device ("OutputStreamer" that has two implementations, etc)?


================
Comment at: tools/xray/xray-fc-account.cc:78
@@ +77,3 @@
+
+  ~FileCloser() noexcept {
+    if (Fd != -1)
----------------
I don't think we can unconditionally use noexcept (because MSVC 2013) - there's an LLVM_NOEXCEPT macro used in some places, but I probably wouldn't bother with it at all unless there's a lot of value.

================
Comment at: tools/xray/xray-fc-account.cc:118-119
@@ +117,4 @@
+
+// FIXME: Make this available in a common header between compiler-rt and this
+// tool.
+struct XRaySledEntry {
----------------
Two comments like this - should there just be one comment at the start of the __xray namespace?

================
Comment at: tools/xray/xray-fc-account.cc:129
@@ +128,3 @@
+
+using FunctionAddressMap = std::unordered_map<int32_t, uint64_t>;
+
----------------
I think we're still using typedefs for things like this where a typedef is still usable (but I could be wrong - maybe check the LLVM style guide? and/or maybe we should revisit that choice?)

================
Comment at: tools/xray/xray-fc-account.cc:147
@@ +146,3 @@
+  StringRef Contents = "";
+  for (const auto &Section : ObjectFile->getBinary()->sections()) {
+    StringRef Name = "";
----------------
I wonder if this loop might be easier to read as something like:

  I = llvm::find_if(sections, [&](const Section &section) {
    ....
    return Name == "xray_instr_map";
  });
  if (I == sections.end())
    fail("Failed to find ... ");
  if (Section.getContents(Contents))
    ...

================
Comment at: tools/xray/xray-fc-account.cc:166-169
@@ +165,6 @@
+
+  // Treat Contents as though it were an array of __xray::XRaySledEntry
+  // elements.
+  const __xray::XRaySledEntry *Sleds =
+      reinterpret_cast<const __xray::XRaySledEntry *>(Contents.data());
+  size_t Entries = Contents.size() / sizeof(__xray::XRaySledEntry);
----------------
Is this not an aliasing violation? (memory might not be appropriately aligned, etc - I thought we had to memcpy into memory of the right type to preserve aliasing requirements)

================
Comment at: tools/xray/xray-fc-account.cc:170
@@ +169,3 @@
+      reinterpret_cast<const __xray::XRaySledEntry *>(Contents.data());
+  size_t Entries = Contents.size() / sizeof(__xray::XRaySledEntry);
+
----------------
Perhaps construct the Sleds+Entries variables as an ArrayRef<XRaySledEntry> ? (then you can use a range-for loop below)

================
Comment at: tools/xray/xray-fc-account.cc:178
@@ +177,3 @@
+  uint64_t CurFn = 0;
+  for (size_t I = 0; I < Entries; ++I) {
+    const auto &Sled = Sleds[I];
----------------
Probably 'i' rather than 'I'

================
Comment at: tools/xray/xray-fc-account.cc:251-254
@@ +250,6 @@
+
+  int Fd = open(Input.c_str(), O_RDONLY);
+  while (Fd == -1 && errno == EINTR) {
+    Fd = open(Input.c_str(), O_RDONLY);
+  }
+  if (Fd == -1)
----------------
do/while loop?

================
Comment at: tools/xray/xray-fc-account.cc:265-276
@@ +264,14 @@
+  bool ReadHeader = false;
+  while (auto ReadBytes = read(Fd, &Header, sizeof(__xray::XRayFileHeader))) {
+    if (ReadBytes == -1) {
+      if (errno == EINTR)
+        continue;
+      fail(Twine("Failed to read file '") + Input + "'; " + Twine(errno));
+    }
+    ReadHeader = true;
+    break;
+  }
+  if (!ReadHeader) {
+    fail(Twine("Failed to read file '") + Input + "'; " + Twine(errno));
+  }
+  __xray::XRayRecord Record;
----------------
Like the other loop (the section searching one) might this be more legible as something like:

  while ((ReadBytes = ...) <= 0) {
    ...
    if (ReadBytes == -1)
      ...
    if (ReadBytes == 0)
      fail(Twine("Failed to read file '") ...;
  }

That should remove the need for the "ReadHeader" flag, I think?

================
Comment at: tools/xray/xray-fc-account.cc:295
@@ +294,3 @@
+    if (Verbose) {
+      auto FuncAddr = FunctionAddresses.find(Record.FuncId);
+      outs() << "[tid=" << Record.TId << ",cpu=" << int32_t{Record.CPU} << "] "
----------------
I'd probably move this down to after the outs() line, so it's right next to its use. (general goal of decreasing the scope of variables where possible/reasonable - keeps the initialization near the use, so it's easier to read)

================
Comment at: tools/xray/xray-fc-account.cc:308
@@ +307,3 @@
+      // just add the correct number of indents. For Exit, we reduce it by 1.
+      for (size_t I = 0 + Record.Type; I < ThreadFuncStack.size(); I++) {
+        outs() << "  ";
----------------
I think we traditionally use 'i' for counters, 'I' for iterators.

================
Comment at: tools/xray/xray-fc-account.cc:349
@@ +348,3 @@
+            ThreadFuncStack.rbegin(), ThreadFuncStack.rend(),
+            [&Record](const std::pair<const int32_t, uint64_t> &Entry) {
+              return Entry.first == Record.FuncId;
----------------
I'd suggest just using [&] for any lambda used directly within the expression (or even within the scope) it's used (ie: any lambda not copied into something like a std::function, etc)

================
Comment at: tools/xray/xray-fc-account.cc:373-374
@@ +372,4 @@
+            auto Level = ThreadStack.second.size();
+            for (auto I = ThreadStack.second.rbegin();
+                 I != ThreadStack.second.rend(); I++) {
+              errs() << "#" << Level-- << "\t"
----------------
Consider using llvm::reverse and a range-for loop.

================
Comment at: tools/xray/xray-fc-account.cc:398-401
@@ +397,6 @@
+
+  if (OutputFormat == OutputFormats::TEXT && HasCyclesPerSec) {
+    outs() << "Cycle counter frequency = " << Header.CycleFrequency << "hz\n"
+           << "Durations below are in seconds.\n";
+  }
+
----------------
Inconsistent use of {} around single line (and multiline but single statement) blocks. (eg: 3 lines down, the if (OutputFormat) conditions don't have braces).

The LLVM style is to not put braces on single line statements. Some people still prefer braces on multiline blocks even if they contain only a single statement - so up to you which way you want to go there.

================
Comment at: tools/xray/xray-fc-account.cc:420
@@ +419,3 @@
+      auto MedianOff = std::distance(Timings.begin(), Timings.end()) / 2;
+      std::nth_element(Timings.begin(), Timings.begin() + (MedianOff),
+                       Timings.end());
----------------
Unnecessary extra parens around MedianOff

================
Comment at: tools/xray/xray-fc-account.cc:429
@@ +428,3 @@
+      auto Pct99Off =
+          std::floor(std::distance(Timings.begin(), Timings.end()) * 0.99);
+      std::nth_element(Timings.begin(), Timings.begin() + Pct99Off,
----------------
Prefer Timings.size() over std::distance(Timings.begin(), Timings.end()), probably? (repeat for similar instances)


https://reviews.llvm.org/D21987