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

walter erquinigo via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Fri Apr 22 16:28:52 PDT 2022


wallace marked 13 inline comments as done.
wallace added inline comments.


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:265
 
+class TraceEventsUtils {
+public:
----------------
jj10306 wrote:
> nit: maybe this will change in the future to where this will have data and instance members, but if not, consider just using a namespace here to house the utility functions since the class isn't really providing any state?
great idea


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:75
+
+  /// \return \b true iff this struct holds a libipt error.
+  explicit operator bool() const;
----------------
jj10306 wrote:
> wallace wrote:
> > jj10306 wrote:
> > > 
> > oh, i wanted to mean if and only if. Or is that too mathematical and less colloquial? 
> Lol my first thought actually was "I wonder if he meant if and only if, so this isn't a typo".
> Up to you if you want to keep if or leave it as iff 
i'll use if and only if


================
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.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123982



More information about the lldb-commits mailing list