[PATCH] D50441: [XRay] FDR Trace Dump Tool (FDR Trace Loading Refactor)

[Dean Michael Berris via Phabricator via llvm-commits]

dberris planned changes to this revision.
dberris added a comment.

I'll be breaking this up into smaller patches, please bear with me. I've also in the process added a few more abstractions, which makes the types cleaner to operate with -- for instance, I've moved the initialization of records into a visitor which is a friend of each of the record types. This way we isolate the logic of loading the data from a `DataExtractor` from the Record types. There will be more moving around of types and filenames, so please hold off on further review.



================
Comment at: llvm/include/llvm/XRay/BlockVerifier.h:25
+class BlockVerifier : public RecordVisitor {
+  enum class RecordKind : unsigned {
+    Unknown,
----------------
kpw wrote:
> If you forward declare this and move the TransitionTable to the cc file, you could use the .inc macro trick to automatically get two string from the list.
Unfortunately, it seems I can't really do that with C++11 enum classes.


================
Comment at: llvm/include/llvm/XRay/BlockVerifier.h:44-66
+  std::map<RecordKind, std::vector<RecordKind>> TransitionTable{
+      {RecordKind::Unknown, {RecordKind::BufferExtents, RecordKind::NewBuffer}},
+      {RecordKind::BufferExtents, {RecordKind::NewBuffer}},
+      {RecordKind::NewBuffer, {RecordKind::WallClockTime}},
+      {RecordKind::WallClockTime, {RecordKind::PIDEntry, RecordKind::NewCPUId}},
+      {RecordKind::PIDEntry, {RecordKind::NewCPUId}},
+      {RecordKind::NewCPUId,
----------------
kpw wrote:
> It's hard to tell whether this is right. I didn't notice any problems, but it is dense.
> 
> Is there a better way? Especially with regards to new record types?
I moved it to the implementation file, and added a few comments to represent the transitions. I'm not exactly sure if it's better, but worth the shot.


================
Comment at: llvm/include/llvm/XRay/FDRRecords.h:50
+  // appropriate function in the visitor to invoke, given its own type.
+  virtual Error apply(RecordVisitor &V) = 0;
+
----------------
kpw wrote:
> It kind of seems like this member function should be const qualified and the visit method should take a const reference to Record.
Unfortunately, that contract is much tighter and severely limits the kinds of visitors that could be implemented -- there may be some that end up needing to modify a record when it's visited (you can imagine a visitor that adjusts the deltas for `FunctionRecord` instances, or those that process the data associated with a custom event, etc.).


================
Comment at: llvm/include/llvm/XRay/FDRRecords.h:243-245
+  RecordTypes record_type() const { return Kind; }
+
+  int32_t functionId() const { return FuncId; }
----------------
kpw wrote:
> Mixed casing convention. You have like_this() and likeThat().
fixed -- using `camelCase()` more consistently for member functions.


================
Comment at: llvm/include/llvm/XRay/FDRRecords.h:257
+  // Support all specific kinds of records:
+  virtual Error visit(BufferExtents &) = 0;
+  virtual Error visit(WallclockRecord &) = 0;
----------------
kpw wrote:
> I think the Record references can and should be const.
Explained elsewhere, that limits the kinds of visitors that could be written. I rather keep it like this to allow visitor implementations to modify Record instances should they need to.


================
Comment at: llvm/lib/XRay/Common.h:1
+//===- Common.h - XRay Trace Loading Common Utilities ---------------------===//
+//
----------------
kpw wrote:
> This is almost never a good name and this file is doomed to become a miscellaneous catch all.
Good point. Renamed.


================
Comment at: llvm/lib/XRay/FDRRecords.cpp:1
+//===- FDRRecords.cpp -  XRay Flight Data Recorder Mode Records -----------===//
+//
----------------
kpw wrote:
> I'm going to only skim this and ask you whether you made any changes other than just copy and paste to the logic that lived elsewhere other than the state transition map and assigning to fields instead of structs directly.
This is mostly copy-paste, although the custom event loading implementation also reads the data from the log.


https://reviews.llvm.org/D50441