[Lldb-commits] [PATCH] D130054: [trace][intel pt] Introduce wall clock time for each trace item

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added inline comments.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:288
 
+    decoded_thread.NotifyTsc(execution.thread_execution.GetLowestKnownTSC());
     decoded_thread.NotifyCPU(execution.thread_execution.cpu_id);
----------------
if we have both a start and end TSC, shouldn't we emit the start here (like you're doing) and then after decoding all the instructions we should also emit the the end TSC?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:100-103
+    // This is the last TSC range, so we have to extrapolate. In this case, we
+    // assume that each instruction took one TSC, which is what an instruction
+    // would take if no parallelism is achieved and the frequency multiplier
+    // is 1.
----------------
Is this correct?
I don't see an issue with choosing a 1 TSC per instruction as a heuristic in this case, but we should be cautious not to explain this heuristic as being tied to any "expected" performance of the hardware because there are way too many variables that influence IPC like differences in workload, microarchitecture (speculative execution/branch prediction implementation, vector units, super scalar, etc), etc not to mention the difference b/t TSC and CPU cycles (like you mentioned).


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:105
+    return m_tsc_conversion->ToNanosFloat(lowest_nanos,
+                                          range->tsc + items_since_last_tsc);
+  }
----------------
Won't our interpolation calculation be messed up since `items_since_last_tsc` will be counting all trace errors, events, etc and not just the instructions/events that we want to assign a timestamp to and thus should be part of this calculation?

I'm thinking of a case like below :
i - instruction, e - error

`tsc, cpu_change, i, e, e, e, tsc`
 in this case the items_since last_tsc would be 5 even though there is only 1 instruction.

i guess this brings up a more general question around how timestamps should be assigned. For example, in this case I see a couple different options:
1. What we are doing now where we assign all events timestamps
Pro: easy implementation
Con: visualization may be weird bc timestamps are being influenced by events (ie errors) that users of the visualization don't care or know about
2. Ignore errors but treat all instructions and events the same (ie instructions and events get timestamps)
3. Ignore errors and treat instructions and events differently (ie instructions are the only things that contribute to `items_since_last_tsc`) and then events are simply assigned the same timestamp as an instruction

We can discuss this in more depth offline.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130054