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

[walter erquinigo via Phabricator via lldb-commits]

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

almost there! Mostly cosmetic changes needed



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:94-98
+  m_instructions.emplace_back(insn);
+  if (!m_last_tsc || *m_last_tsc != tsc) {
+    m_instruction_timestamps.emplace(m_instructions.size() - 1, tsc);
+    m_last_tsc = tsc;
+  }
----------------
We need to handle a special case. It might happen that the first instruction is an error, which won't have a TSC, and the second instruction is an actual instruction, and from that point on you'll always have TSCs. For this case, we can assume that the TSC of the error instruction at the beginning of the trace is the same as the first valid TSC.


================
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;
+
----------------
delete these two lines. The rest of the code will work well without it


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



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:104
   // When adding new members to this class, make sure to update
-  // IntelPTInstruction::GetNonErrorMemoryUsage() if needed.
+  // IntelPTInstruction::GetMemoryUsage() if needed.
   pt_insn m_pt_insn;
----------------
+1


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:140
+    /// Get the smallest instruction index that has this TSC.
+    size_t GetStart() const;
+    /// Get the largest instruction index that has this TSC.
----------------
Let's be more verbose with the names to improve readability


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:142
+    /// Get the largest instruction index that has this TSC.
+    size_t GetEnd() const;
+
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:148
+    TscRange(std::map<size_t, uint64_t>::const_iterator it,
+             const DecodedThread *decoded_thread);
+
----------------
let's receive a reference here and then convert it to pointer, so that we minimize the number of places with pointers. Also, if later we decide to use a shared_ptr instead of a pointer, we can do it inside of the constructor without changing this line 


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:150
+
+    /// The current range
+    std::map<size_t, uint64_t>::const_iterator m_it;
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:181-182
 
+  /// Construct the TSC range that covers the given instruction index.
+  /// This operation is O(logn) and should be used sparingly.
+  llvm::Optional<TscRange> CalculateTscRange(size_t insn_index) const;
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:67-82
   switch (origin) {
   case TraceCursor::SeekType::Set:
     m_pos = fitPosToBounds(offset);
+    m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);
     return m_pos;
   case TraceCursor::SeekType::End:
     m_pos = fitPosToBounds(offset + last_index);
----------------
we can simplify this so that we only invoke CalculateTscRange once


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h:46
+  /// Tsc range covering the current instruction.
+  llvm::Optional<DecodedThread::TscRange> m_current_tsc;
 };
----------------
rename it to `m_tsc_range`. The word current is very redundant in this case


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:123-124
+  if (insn_len != 0)
+    s.Printf("  Average memory usage per instruction: %zu bytes\n",
+             mem_used / insn_len);
   return;
----------------
Use doubles, as the average might not be a whole number




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122603