jj10306 requested changes to this revision.
jj10306 added inline comments.
This revision now requires changes to proceed.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:183
-DecodedThread::TscRange::TscRange(std::map<size_t, uint64_t>::const_iterator
it,
+DecodedThread::TscRange::TscRange(std::map<uint64_t, TSC>::const_iterator it,
const DecodedThread &decoded_thread)
----------------
What's purpose does the `TscRange` class serve? Specifically, why can't we take
the same approach we take for CPU events where we just store a map and the
chronologically last CPU ID on the `DecodedThread`:
```
// The cpu information is stored as a map. It maps `instruction index -> CPU`
// A CPU is associated with the next instructions that follow until the next
cpu is seen.
std::map<uint64_t, lldb::cpu_id_t> m_cpus;
/// This is the chronologically last CPU ID.
llvm::Optional<uint64_t> m_last_cpu = llvm::None;
```
we currently store this same information for TSCs but then we have this whole
additional `TscRange` notion that lives on a `TraceCursorIntelPT`
This is basically an extension of the conversation we had inline on
(https://reviews.llvm.org/D129340) about eventually moving TSCs to be the same
as CPUs. Perhaps now that we are making this change to treat TSCs like events
(just like CPU changes) it would be a good time to revisit the design and make
it simpler/consistent with CPU change events, if possible.
tl;dr iiuc the TSC lookup for an instruction is basically identical to the CPU
ID lookup for an instruction, so why should the way we do the lookup be
different for TSCs vs CPU IDs
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:191-197
+ TSC tsc_duration =
+ next_it == m_decoded_thread->m_timestamps.end()
+ ? (m_end_index + 1 - it->first) // for the last range, we assume each
+ // instruction took 1 TSC
+ : next_it->second - it->second;
+ m_tsc_duration_per_instruction =
+ static_cast<long double>(tsc_duration) / (m_end_index - m_it->first + 1);
----------------
I found this a little difficult to reason about.
Perhaps consistent use of the different iterator variables, minor renaming and
docs may make this a little more understandable. Feel free to take none or any
of the above suggestions 🙂
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:198-205
+ // duration_per_instruction will be greater than 1 if the PSBs are too spread
+ // out, e.g. when we missed instructions or there was a context switch in
+ // between. In this case, we change it to 1 to have a more real number. If
+ // duration_per_instruction is less than 1, then that means that multiple
+ // instructions were executed in the same tsc.
+ if (m_tsc_duration_per_instruction > 1)
+ m_tsc_duration_per_instruction = 1;
----------------
Doesn't this check essentially make it so `m_tsc_duration_per_instruction` is
either 0 or 1?
Where is the heuristic that every instruction should take <= 1 TSC to execute
coming from?
From a visualization perspective, how should we display instructions in a
TscRange where `m_tsc_duration_per_instruction` is 0? Doesn't this force us to
stack those instructions since will all have the same interpolated timestamp?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:275
/// first one. Otherwise, this map will be empty.
- std::map<uint64_t, uint64_t> m_timestamps;
+ std::map<uint64_t, TSC> m_timestamps;
/// This is the chronologically last TSC that has been added.
----------------
Now that we are introducing timestamps (ie wall clock from TSCs), should we
rename this to make it clear that these are TSC values and not timestamps?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130054/new/
https://reviews.llvm.org/D130054
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
