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

[David Carrillo Cisneros via Phabricator via lldb-commits]

davidca added inline comments.
Herald added a project: All.



Comment at: lldb/docs/lldb-gdb-remote.txt:469
+//
+//tsc_rate: {
+//  kind: "perf",

why is all this called tsc rate? there is also offset in this conversion. What 
about something like tsc_conversion_params or tsc_conv



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:44
+/// TSC to nanosecond conversion.
+class TimestampCounterRate {
+  public:

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?



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:47
+virtual ~TimestampCounterRate() = default;
+/// Convert TSC value to nanoseconds. This represents the number of 
nanoseconds since
+/// the TSC register's reset.

wallace wrote:
> 
To be precise, it's number of nanoseconds since boot. You should also add that 
this nanoseconds clock is sched_clock 
https://www.kernel.org/doc/html/latest/timers/timekeeping.html

In fact, you may want to use sched_clock as the name for this clock, rather 
than Nanos as in the next lines.



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);

should this two be static factory methods?



Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:257
 
+  IntelPTManager::GetPerfTscRate(*m_mmap_meta);
+

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::OptionalGetTscRate()` 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.



Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:211
   static bool IsSupported();
+  static void GetPerfTscRate(perf_event_mmap_page &mmap_meta);
+

wallace wrote:
> don't forget to add documentation here
how is ti


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

https://reviews.llvm.org/D120595

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D122859: [trace] Show ideas for the main interfaces for new HTR

[David Carrillo Cisneros via Phabricator via lldb-commits]

davidca added inline comments.



Comment at: lldb/include/lldb/Target/TraceHTR.h:73
+  //   trace
+  struct HTRInMemoryBlock {
+

Just like in the original HTR design, if you remove this abstraction, and store 
indices as indices in each layer, then you don't have to create separate 
versions for in memory and in disk, as the layers become very easily 
serializable.



Comment at: lldb/include/lldb/Target/TraceHTR.h:118
+  // growing
+  std::vector m_blocks;
+};

This does not deduplicate blocks. If the same block appears multiple times (as 
it's common in loops), it will be stored multiple times, consuming more memory 
and making pattern detection and other post-processing more time consuming.

Note that this is the original motivation of the separation between BlockDefs 
and bids (the trace of block ids) in the original HTR doc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122859

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits