[Lldb-commits] [PATCH] D123982: [trace][intel pt] Support events

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

Thanks for making those changes - lgtm!



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:258
   /// first one. Otherwise, this map will be empty.
   std::map<size_t, uint64_t> m_instruction_timestamps;
   /// This is the chronologically last TSC that has been added.
----------------
wallace wrote:
> jj10306 wrote:
> > wallace wrote:
> > > jj10306 wrote:
> > > > jj10306 wrote:
> > > > > I know this isn't related to these changes, but this should be updated to be consistent with the other `instruction_index -> XXX` maps in this class.
> > > > 
> > > in this case we can't do that, because when doing random accesses in the trace, we need to quickly find the most recent TSC for the new instruction, which is done with binary search using a map. This is impossible in a hash map like DenseMap
> > I know this is a pedantic discussion, but isn't lookup in hash tables amortized O(1) (assuming a good hash that doesn't take too long to execute or produce the same hash for different values) whereas in an ordered map it's O(logn)?
> > IIUC, you should only use an ordered map when you rely on the order property of it.
> > 
> > In any case, we should use `uint64_t` here instead of `size_t` to be consistent with the type for instruction index used in the other maps.
> > In any case, we should use uint64_t here instead of size_t to be consistent with the type for instruction index used in the other maps.
> 
> you are right with this one.
> 
> > IIUC, you should only use an ordered map when you rely on the order property of it.
> 
> yes, and that's what we use for the timestamps. As there are a small number of unique timestamps, we are keeping this map 'insn_index -> timestamp'. We need its order property because when we do GoToId(insn_id) we need to look for most recent entry in the map that has a timestamp, and we use that timestamp for our new instruction. We use map.lower_bound for that. With a hashmap we couldn't do that.
>We use map.lower_bound for that. With a hashmap we couldn't do that.
Ahhh yes, that makes sense - I forgot we were using that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123982