[PATCH] D21987: [XRay] Initial XRay Function Call Accounting Tool
Dean Michael Berris via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 4 21:38:22 PDT 2016
dberris marked 15 inline comments as done.
dberris added a comment.
In https://reviews.llvm.org/D21987#504924, @dblaikie wrote:
> 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.
Thanks -- yes, I think more refactoring for readability would be necessary here. Let me iterate on that and get back on some re-structuring.
In general though I think there are a few things being done here that ought to really be moved into independent units that might be tested in isolation. Do you have suggestions on how to do that with tools? i.e. having unit tests for parts of this implementation?
> 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)?
I was thinking about this, and yes there should be. Let me iterate on this one too and ping back when I have something cleaner.
================
Comment at: tools/xray/xray-fc-account.cc:78
@@ +77,3 @@
+
+ ~FileCloser() noexcept {
+ if (Fd != -1)
----------------
dblaikie wrote:
> 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.
I see -- removed the noexcept qualifier.
================
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 {
----------------
dblaikie wrote:
> Two comments like this - should there just be one comment at the start of the __xray namespace?
Done.
================
Comment at: tools/xray/xray-fc-account.cc:129
@@ +128,3 @@
+
+using FunctionAddressMap = std::unordered_map<int32_t, uint64_t>;
+
----------------
dblaikie wrote:
> 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?)
Done (this was force of habit, haven't checked the LLVM style guide -- happy either way :) ).
================
Comment at: tools/xray/xray-fc-account.cc:147
@@ +146,3 @@
+ StringRef Contents = "";
+ for (const auto &Section : ObjectFile->getBinary()->sections()) {
+ StringRef Name = "";
----------------
dblaikie wrote:
> I wonder if this loop might be easier to read as something like:
>
> I = llvm::find_if(sections, [&](const Section §ion) {
> ....
> return Name == "xray_instr_map";
> });
> if (I == sections.end())
> fail("Failed to find ... ");
> if (Section.getContents(Contents))
> ...
Thanks, yes, that's better.
================
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);
----------------
dblaikie wrote:
> 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)
Interesting. I'm not sure. Should I just do the memcpy into aligned memory first? I don't think it's possible to dynamically allocate aligned memory yet, is it?
================
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);
+
----------------
dblaikie wrote:
> Perhaps construct the Sleds+Entries variables as an ArrayRef<XRaySledEntry> ? (then you can use a range-for loop below)
Good idea, done.
================
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)
----------------
dblaikie wrote:
> do/while loop?
Done.
================
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;
----------------
dblaikie wrote:
> 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?
Fixed.
================
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} << "] "
----------------
dblaikie wrote:
> 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)
Done.
================
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() << " ";
----------------
dblaikie wrote:
> I think we traditionally use 'i' for counters, 'I' for iterators.
Oops, right, thanks -- done.
================
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;
----------------
dblaikie wrote:
> 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)
Cool, thanks. Done.
================
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"
----------------
dblaikie wrote:
> Consider using llvm::reverse and a range-for loop.
Nice. That's good to know. :)
================
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";
+ }
+
----------------
dblaikie wrote:
> 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.
Oops, right, thanks. :)
================
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());
----------------
dblaikie wrote:
> Unnecessary extra parens around MedianOff
Oops, yes, remnant of copy-pasting/editing. Fixed.
https://reviews.llvm.org/D21987
More information about the llvm-commits
mailing list