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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
