[Lldb-commits] [PATCH] D130054: [trace][intel pt] Introduce wall clock time for each trace item

[Jakob Johnson via Phabricator via lldb-commits]

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