[Lldb-commits] [PATCH] D122093: [trace][intelpt] Added total memory usage by decoded trace

[walter erquinigo via Phabricator via lldb-commits]

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

Pretty nice! just some details i've noticed when reading your diff and good to go



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:121-126
+  size_t total = sizeof(m_raw_trace_size);
+
+  for (const IntelPTInstruction &ins : m_instructions)
+    total += ins.GetMemoryUsage();
+
+  return total;
----------------
you might be able to do something like

  // As calculating the size used by error instructions is very difficult, because errors
  // are dynamic objects, we just discard them from our calculations because errors
  // are supposed to be rare and not really contribute much to the total memory usage.
  size_t non_error_instructions = count(m_instructions.begin(), m_instructions.end(), [](const IntelPTInstruction& insn) {!insn.IsError();});

  return IntelPTInstruction::GetNonErrorMemoryUsage() * non_error_instructions + GetRawTraceSize();

notice that we need the actual `GetRawTraceSize()` and not `sizeof(m_raw_trace_size)`, because the latter will always give you 8 bytes on an x86_64 machine, regardless of how much data it is actually consuming.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:85-95
+  /// Get the size in bytes of decoded instruction.
+  /// This method is static because the size of \a IntelPTInstruction is not dynamic
+  ///
+  /// \return
+  ///   The size of the decoded instruction
+  static size_t GetMemoryUsage() {
+    return (
----------------
I made a mistake. I didn't notice that there's an m_error member, whose size is dynamic, because it might hold a string message.
However, the number of errors should be negligible compared to the total number of correct instructions, so, for global calculations, let's assume that all instructions are fine and they are not errors.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:128
 
   pt_insn m_pt_insn;
   llvm::Optional<uint64_t> m_timestamp;
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:166-170
+  /// Get the size in bytes of decoded Intel PT trace
+  ///
+  /// \return
+  ///   The size of the decoded traces.
+  size_t GetMemoryUsage() const;
----------------
We can put all the documentation in the \return section to avoid redundancy.

Let's also use the word Calculate instead of Get because Get is supposed to be used for cheap operations and Calculate for maybe not so trivial ones.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:114
   s.Printf("\n");
   s.Printf("  Raw trace size: %zu bytes\n", *raw_size);
   s.Printf("  Total number of instructions: %zu\n", 
----------------
print this in KB, as it will be readability. You can assume that this number will always be an integer because the raw trace size by definition in the kernel is a multiple of 4KB.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:117
     Decode(thread)->GetInstructions().size());
+  s.Printf("  Total memory usage: %zu bytes\n", 
+    Decode(thread)->GetMemoryUsage());
----------------
better print KB instead of bytes. you can use a double with two digits of precision


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122093