[Lldb-commits] [PATCH] D120595: [WIP][trace][intelpt] Add TSC to Nanosecond conversion for IntelPT traces

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

Thanks @davidca and @labath for chiming in.
@jj10306, please rename IntelPTManager to IntelPTCollector for clarity.



================
Comment at: lldb/docs/lldb-gdb-remote.txt:469
+//
+//    tsc_rate: {
+//      kind: "perf",
----------------
davidca wrote:
> why is all this called tsc rate? there is also offset in this conversion. What about something like tsc_conversion_params or tsc_conv
tsc_conversion_params seems like a good idea


================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:44
+/// TSC to nanosecond conversion.
+class TimestampCounterRate {
+  public:
----------------
davidca wrote:
> nit, Intel documentation and kernel refers this as TSC, not TimestampCounter, in order to differentiate this awful name from everywhere else that "Timestamp" and "Counter" is used. Perhaps embrace TSC to make it easier to google info?
Ahh!! Then let's do what David says. Let's call it tsc everywhere


================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:74
+struct TraceIntelPTGetStateResponse : TraceGetStateResponse {
+  /// `nullptr` if no tsc conversion rate exists.
+  llvm::Optional<TimestampCounterRateSP> tsc_rate;
----------------
labath wrote:
> wallace wrote:
> > avoid using nullptr when you have an Optional. 
> Ideally this would be a plain `TimestampCounterRateSP` variable and nullptr would denote emptyness. Since (shared) pointers already have a natural empty state, it's best to use that. Adding an extra Optional layer just creates ambiguity.
Good idea


================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:78-80
+bool fromJSON(const llvm::json::Value &value, TimestampCounterRateSP &tsc_converter, llvm::json::Path path);
+
+bool fromJSON(const llvm::json::Value &value, TraceIntelPTGetStateResponse &packet, llvm::json::Path path);
----------------
davidca wrote:
> should this two be static factory methods?
That's just the pattern most json conversion utils follow. It helps with some templating stuff


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:257
 
+  IntelPTManager::GetPerfTscRate(*m_mmap_meta);
+
----------------
davidca wrote:
> wallace wrote:
> > sadly you can't put this here. If you do this, then GetState() will only work correctly if StartTrace() was invoked first. What if in the future we have a different flow in which lldb-server uses an additional tracing function besides StartTrace() and that new function forgets to set the tsc_rate?
> > This is called coupling and should be avoided.
> > 
> > What you should do instead is to create a new static function called `llvm::Optional<TimestampCounterRateSP >GetTscRate()` that will perform the perf_event request or reuse a previously saved value. It should work regardless of when it's invoked.
> 1) this is not a Get* method. This is populating a global. Get* are inexpensive methods that return a value.
> 
> 2) if the TSC conversion parameters will be stored in a global, why not to use the Singleton pattern?
> 
> 3) TSC parameters change over time. I really think that should be stored within IntelPTManager. Obtaining this parameters is a ~10 microseconds operation, so it's fine if they need to be read next time an IntelPTManager is created.
Can tsc frequency rates change even in modern cpus?. In this case, it's no brainer we need to calculate this number all the time.

I also agree with point number 1. 


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:211
   static bool IsSupported();
+  static void GetPerfTscRate(perf_event_mmap_page &mmap_meta);
+
----------------
davidca wrote:
> wallace wrote:
> > don't forget to add documentation here
> how is ti
@davidca, i think you didn't finish your comment


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

https://reviews.llvm.org/D120595