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

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:265
 
+class TraceEventsUtils {
+public:
----------------
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?


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123982