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