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