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