[Lldb-commits] [PATCH] D105741: [trace] Add `thread trace export` command for Intel PT trace visualization

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 marked 2 inline comments as done.
jj10306 added inline comments.


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:272
+  ///     The map of block IDs to \a HTRBlock.
+  std::unordered_map<size_t, HTRBlock> const &GetBlockDefs() const;
+
----------------
wallace wrote:
> As this is const, the only way to use this unordered_map is to do look ups, then let's better hide the implementation detail that is an unordered map and create this method instead
> 
>   const HTRBlock &GetBlockById(size_t id);
> 
> that way we can later change unordered_map for anything else without affecting any callers
Makes sense. How should the case when an `id` that the layer doesn't contain is passed in? I would like to use `llvm::Optional` here but it's not possible to have an Optional reference (i.e. `Optional<& const HTRBlock>`). I was thinking about creating different methods:

`bool ContainsBlock(size_t id) const` for checking if the layer contains an ID
`HTRBlock const & GetBlockById(size_t id) const` for retrieving a reference to a block in the layer
but this still doesn't solve the issue of if a caller calls `GetBlockId` with an invalid ID.

Another option would be to change `GetBlockId` to return `Optional<HTRBlock>` instead of a reference, but this would result in an unnecessary copy, when all we really need is an const reference.


================
Comment at: lldb/include/lldb/Target/TraceHTR.h:339
+  ///     The file that the exported HTR data will be written to.
+  void Export(Stream &s, TraceExportFormat export_format, std::string outfile);
+
----------------
wallace wrote:
> better return an llvm::Error that will contain the error message, if any. The CommandObject that invokes this method can then get the error and dump it if necessary. So, don't pass any streams here
I based this method off of `TraceCursor::DumpInstructions` which takes in a Stream & to output its success/error messages to. This method (`TraceHTR::Export`) prints a message to the console on success (not only failure) so I'm not sure how the success message could be propagated back up to the caller. I agree that using `llvm::Error` here would make sense if we were only printing if an Error occurred, but since we print on success I'm not sure if we should use llvm::Error over passing the Stream.


================
Comment at: lldb/source/Target/TraceHTR.cpp:152
+      valid_cursor = cursor.Next();
+      s.Printf("%s\n", toString(std::move(err)).c_str());
+    } else {
----------------
wallace wrote:
> this Printf is too noisy. You don't really need that. Just remove it and instead of invoking cursor.GetError(), invoke cursor.IsError() after rebasing the code
Initially I wasn't planning on displaying the error, but then I was getting runtime errors because I wasn't doing anything with the error. As a result, I decided to display the error and looked at how `TraceCursor::DumpInstructions` does it: `s << toString(std::move(err));`.
I believe if an `isError()` method existed then I would not have this issue about not checking/using the error and I could just use the `isError()` method to check for the presence of an error without actually retrieving the error object.


================
Comment at: lldb/source/Target/TraceHTR.cpp:197
+HTRBlock IHTRLayer::MergeUnits(size_t start_unit_index, size_t num_units) {
+  assert(num_units > 0);
+  llvm::Optional<HTRBlockMetadata> merged_metadata = llvm::None;
----------------
wallace wrote:
> remove this assert, the code wouldn't break if num_units is 0
If `num_units` is 0 then the loop body would never execute and thus `merged_metadata` would be `None` at the time of return. This would be problematic since we return `*merged_metadata`.


================
Comment at: lldb/source/Target/TraceHTR.cpp:204-205
+    } else {
+      merged_metadata =
+          HTRBlockMetadata::MergeMetadata(*merged_metadata, current_metadata);
+    }
----------------
wallace wrote:
> this is very expensive. Let's suppose that num_units is N, then you'll invoke MergeMetadata N - 1 times, and each element of the first map will be copied N - 1 times, each one from the second map will be copied N - 2 times, etc. So you'll have N^2 copies, which is just too slow.
> 
> Instead, you have two options:
>   - when you merge A and B, you don't create a new map, you just copy the elements from B to A, effectively modifying A's map. That way after N merges, each element from each map will need to be copied only once. This might intuitively makes sense from an API standpoint, because when you merge blocks, you might expect the original blocks to go away
>   - remove the merge method that accepts only two blocks and do the merging here coping everything to the same single map
> 
> up to you
Ahh yep, I've been meaning to update this method to take the first approach you described.


================
Comment at: lldb/source/Target/TraceHTR.cpp:233
+
+void TraceHTR::Export(Stream &s, TraceExportFormat export_format,
+                      std::string outfile) {
----------------
wallace wrote:
> wallace wrote:
> > remove the TraceExportFormat enum. You don't really need it because there's one single case for now
> return an llvm::Error instead of receiving a Stream
See comment on this function's declaration in `TraceHTR.h`


================
Comment at: lldb/source/Target/TraceHTR.cpp:439-440
+
+      // Can't use ConstString here because no toJSON implementation exists for
+      // ConstString
+      llvm::Optional<std::string> most_freq_func =
----------------
wallace wrote:
> this comment doens't make sense, as you are instead passing display_name to the json object
I was trying to indicate that `GetMostFrequentlyCalledFunction` returns a `std::string` instead of `ConstString` because eventually the `ConsString` function name will need to be converted to `std::string` in order to be serialized, but I will remove this comment and just change the function to return `ConstString` and then do the conversion in the ternary that defines `display_name`.


================
Comment at: lldb/test/API/commands/trace/TestTraceExport.py:60
+                error=True)
+
----------------
wallace wrote:
> Add a new test in which you run this algorithm on a program with a few levels of function calls, and check that the output trace makes sense. Don't check for individual load addresses, but instead check for the appearance of the methods and blocks that you expect.
Yes, this makes sense. Is the way to do this by loading the JSON file's contents into memory in the test and doing assertions on that structure?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105741/new/

https://reviews.llvm.org/D105741