[Lldb-commits] [PATCH] D122603: [intelpt] Refactor timestamps out of IntelPTInstruction

Wed Mar 30 18:56:22 PDT 2022

wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.

Some calculations are wrong, but overall this is good. We are very close!

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:112-113
+DecodedThread::CalculateTscRange(size_t insn_index) const {
+  if (m_instruction_timestamps.empty())
+    return None;
+
----------------
now that I think of this, you can delete this, because if the map is empty, this function will return in line 117

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:135-138
+    : m_thread_sp(thread_sp), m_last_tsc(None) {}

 DecodedThread::DecodedThread(ThreadSP thread_sp, Error &&error)
+    : m_thread_sp(thread_sp), m_last_tsc(None) {
----------------
undo these two lines

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:159
+DecodedThread::TscRange::TscRange(std::map<size_t, uint64_t>::const_iterator it,
+                                  const DecodedThread &ref)
+    : m_it(it), m_decoded_thread(ref) {
----------------
decoded_thread instead of ref

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:161-162
+    : m_it(it), m_decoded_thread(ref) {
+  m_start_index = it->first;
+  m_tsc = it->second;
+
----------------
delete

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:164-168
+  auto end = m_decoded_thread.m_instruction_timestamps.end();
+  if (it != end)
+    m_end_index = (++it--)->first - 1;
+  else
+    m_end_index = end->first;
----------------
seeing ++ and -- is very hard to read. I also prefer not to modify the `it` variable for cleanness. Also doing end->first might crash the program. I'm writing here a correct version

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:173
+
+size_t DecodedThread::TscRange::GetStart() const { return m_start_index; }
+
----------------

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:178
+bool DecodedThread::TscRange::InRange(size_t insn_index) {
+  if (insn_index < m_end_index && insn_index > m_start_index)
+    return true;
----------------
The comparison is not right. let's use <= in a specific order to make it easier to read

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:186
+    return None;
+  return TscRange(++m_it, m_decoded_thread);
+}
----------------
As m_it is valid, doing the comparison `m_it == m_decoded_thread.m_instruction_timestamps.end()` will always return false. Remember that .end() will return a fake iterator that points to no value.

Besides that, don't modify m_it. Let's better create a new iterator

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:190-192
+  if (m_it == m_decoded_thread.m_instruction_timestamps.end())
+    return None;
+  return TscRange(--m_it, m_decoded_thread);
----------------
Similarly, this has to be improved. I also like to put `--it` statements in their own line to make it easier to read.

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:142
+  /// \class TscRange
+  /// Class that represents the instruction range associated with a given TSC.
+  /// It provides efficient iteration to the previous or next TSC range in the
----------------

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:147-148
+  /// TSC timestamps are emitted by the decoder infrequently, which means
+  /// that each TSC covers a range of instruction indices, which we can use to
+  /// speed up TSC lookups.
+  class TscRange {
----------------

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:149
+  /// speed up TSC lookups.
+  class TscRange {
+  public:
----------------
Move this class to the beginning of the public section of DecodedThread for easier discoverability

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:151
+  public:
+    // Check if current TSC range covers given instruction index.
+    bool InRange(size_t insn_index);
----------------

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:167-173
+    TscRange &operator=(const TscRange &r) {
+      m_it = r.m_it;
+      m_tsc = r.m_tsc;
+      m_start_index = r.m_start_index;
+      m_end_index = r.m_end_index;
+      return *this;
+    }
----------------
let's better delete this. It adds some maintenance cost with little benefits

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:178
+
+    // Construct TscRange respecting bounds of timestamp map in thread
+    TscRange(std::map<size_t, uint64_t>::const_iterator it,
----------------
This comment is hard to follow. Let's just delete it because it's a private constructor

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:185-188
+    /// The TSC value
+    uint64_t m_tsc;
+    /// The smallest instruction index that has this TSC.
+    size_t m_start_index;
----------------
Let's just delete this, as we can get them directly from m_it without doing any operations

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:193
+
+  /// Construct a TSC range of an instruction by its index.
+  /// This operation is O(logn) and should be used sparingly.
----------------

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:232-236
+  /// This map contains the TSCs of the decoded instructions. It might be empty
+  /// if the trace doesn't contain TSCs. It maps `instruction index -> TSC`,
+  /// where `instruction index` is the first index at which the mapped TSC
+  /// appears. We use this representation because TSCs are sporadic and we can
+  /// think of it as ranges.
----------------
let's add another piece of information

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:236
+  /// appears. We use this representation because TSCs are sporadic and we can
+  /// think of it as ranges.
+  std::map<size_t, uint64_t> m_instruction_timestamps;
----------------

================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:239
+  /// This is the chronologically last TSC that has been added.
+  llvm::Optional<uint64_t> m_last_tsc;
+  // This variables stores the messages of all the error instructions in the
----------------
Setting to llvm::None here is equivalent to doing it from all the constructors

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:45-52
+    if (!m_current_tsc)
+      m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);
+    else if (!m_current_tsc->InRange(m_pos)) {
+      if (m_pos > m_current_tsc->GetEnd())
+        m_current_tsc = m_current_tsc->Next();
+      if (m_pos < m_current_tsc->GetStart())
+        m_current_tsc = m_current_tsc->Prev();
----------------
No need to do `m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);` because its value has already been calculated in the constructor. We can simplify this as well

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:102-103

-Optional<uint64_t> TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
+Optional<uint64_t>
+TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
+  if (!m_current_tsc)
----------------
are you using git clang-format? I'm curious why this line changed

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:104-110
+  if (!m_current_tsc)
+    return None;
+
   switch (counter_type) {
-    case lldb::eTraceCounterTSC:
-      return m_decoded_thread_sp->GetInstructions()[m_pos].GetTimestampCounter();
+  case lldb::eTraceCounterTSC:
+    return m_current_tsc->GetTsc();
   }
----------------

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h:45
   size_t m_pos;
+  /// Current instruction timestamp.
+  llvm::Optional<DecodedThread::TscRange> m_current_tsc;
----------------
/// Tsc range covering the current instruction.

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:124
+    s.Printf("  Average memory usage per instruction: %zu bytes\n",
+            (mem_used - *raw_size) / insn_len);
   return;
----------------
Instead of doing ` *raw_size`, better remove this number from CalculateApproximateMemoryUsage()

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122603