[Lldb-commits] [PATCH] D111686: Modify "statistics dump" to dump JSON.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/include/lldb/Target/Statistics.h:23
+struct SuccessFailStats {
+  void Notify(bool success);
+
----------------
wallace wrote:
> JDevlieghere wrote:
> > I'd use an enum class with `Success` and `Failure` here instead of a bool that requires you to know what the argument is named to know if true means success of failure. 
> Or maybe just NotifySuccess(bool) to keep callers simple
I like the idea of adding:
```
void NotifySuccess();
void NotifyFailure();
```
JDevlieghere you ok with that?


================
Comment at: lldb/include/lldb/Target/Statistics.h:28
+  uint32_t successes = 0;
+  uint32_t failures = 0;
+};
----------------
JDevlieghere wrote:
> I would add a member for the name so that `ToJSON` can generate the whole JSON object. I imagine something like
> 
> ```
> SuccessFailStats s("frameVariable");
> s.Notify(SuccessFailStats::Success);
> s.ToJSON()
> ```
> 
> which generates
> 
> ```
> "frameVariable": {
>   "failures": 0,
>   "successes": 1
> },
> ```
> 
Good idea!


================
Comment at: lldb/include/lldb/Target/Statistics.h:31
+
+class TargetStats {
+public:
----------------
JDevlieghere wrote:
> The direction I outlined in the other patch is to move away from tying the statistics to the target. I think the class itself is fine, as long as we agree that the target stats would be held by an object spanning all debuggers.
Yeah, my main patch did a little of both. The main idea behind the classes in this file are they don't need to be the storage for the stats, but they can be if that makes things easier. The main classes (Target, Module, Breakpoint, etc) can just have member variables in the main classes themselves, but anything inside of these classes can/should be used in case we want to aggregate stats from any contained Stats classes. For example, I want to have a ModuleStats class (see https://reviews.llvm.org/D110804), this class doesn't not live inside of Module itself, but can be constructed from a Module. Then the TargetStats class can have a std::vector<ModuleStats> classes that it gathers when TargetStats is asked to collect statistics. This same methodology can then be applied to the debugger where we have DebuggerStats.

What do you think of this approach:
- Any class can define a XXXStats class in this file which is used to collect statistics at any given point
- Classes can use the XXXStats class as a member variable if it makes sense, or just add member variables to store any objects needed to calculate the stats later when the object is asked to collect stats (target.GatherStatistics(target_stats);).

This would mean we could make a DebuggerStats class that can have a std::vector<TargetStats>, and when the debugger.GatherStatistics(...) is called. Then either the DebuggerStats or TargetStats object can be asked to be converted to JSON when presenting to the user in the "statistics dump" command?


================
Comment at: lldb/include/lldb/Target/Statistics.h:35
+
+  void SetLaunchOrAttachTime();
+  void SetFirstPrivateStopTime();
----------------
JDevlieghere wrote:
> Why don't we have these return an `ElapsedTime`. Then you can do something like 
> 
> ```
> ElapsedTime elapsed_time = m_stats.GetCreateTime(); 
> ```
> 
> RVO should ensure there are no copies, but you can always delete the copy ctor to make sure. Maybe we should call it ScopedTime or something to make it more obvious that this is a RAII object?
> 
These are setting the time at which things start. If we follow suggestions above, SetLaunchOrAttachTime(), SetFirstPrivateStopTime(), and SetFirstPublicStopTime() can be remove and all of the llvm::Optional<std::chrono::steady_clock::time_point> member variables can be moved to Target class, and we can store only "llvm::Optional<ElapsedTime::Duration> launch_or_attach_time;" member variables in the TargetStats class, which would get set when statistics are gathered?


================
Comment at: lldb/include/lldb/Target/Statistics.h:51
+  // and "statistics disable".
+  bool m_collecting_stats = false;
+  ElapsedTime::Duration create_time{0.0};
----------------
JDevlieghere wrote:
> In line with my earlier comment about not tying the stats tot he target, this would be controlled by the global (singleton) statistic class. 
I put this here because of the existing SB API:
```
bool SBTarget::GetCollectingStats();
void SBTarget::SetCollectingStats(bool);
```
So you want debugger to have this? This m_collecting_stats can be moved to Debugger, or DebuggerStats? But that makes the above API useless or it becomes deprecated. We really don't need to set if we are collecting stats IMHO, since collection should be cheap. You ok with deprecating the "statistics enable"/"statistics disable" and the above APIs?




================
Comment at: lldb/include/lldb/Target/Statistics.h:53
+  ElapsedTime::Duration create_time{0.0};
+  llvm::Optional<std::chrono::steady_clock::time_point> launch_or_attach_time;
+  llvm::Optional<std::chrono::steady_clock::time_point> first_private_stop_time;
----------------
JDevlieghere wrote:
> I would abstract this behind a `TimeStats` or `TimeStats`, similar to SuccessFailStats, so that every type of statistic has its own type.  
I will make a ElapsedTime::TimePoint


================
Comment at: lldb/include/lldb/Target/Target.h:1466-1468
+  /// \param [in] modules
+  ///     If true, include an array of metrics for each module loaded in the
+  ///     target.
----------------
wallace wrote:
> this param is not present
I will remove it!


================
Comment at: lldb/include/lldb/Utility/ElapsedTime.h:34
+/// breakpoint each time a new shared library is loaded.
+class ElapsedTime {
+public:
----------------
JDevlieghere wrote:
> This seems simple enough and specific enough to the statistics that it can be part of that header?
Yes, good idea.


================
Comment at: lldb/include/lldb/lldb-forward.h:121
 class MemoryRegionInfos;
+struct StatsDumpOptions;
 class Module;
----------------
wallace wrote:
> move this to line ~277
I actually need to remove this for now as we don't have this class yet!


================
Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:761
 
+    def getShlibBuildArtifact(self, DYLIB_NAME):
+        """Return absolute path to a shared library artifact given the library
----------------
wallace wrote:
> is this actually used somewhere?
Not yet, I will remove it


================
Comment at: lldb/source/API/SBTarget.cpp:216-220
+  std::string json_text;
+  llvm::raw_string_ostream stream(json_text);
+  llvm::json::Value json = target_sp->ReportStatistics();
+  stream << json;
+  data.m_impl_up->SetObjectSP(StructuredData::ParseJSON(stream.str()));
----------------
wallace wrote:
> a simpler version
Will switch!


================
Comment at: lldb/source/Commands/CommandObjectExpression.cpp:662-668
     // Increment statistics to record this expression evaluation success.
-    target.IncrementStats(StatisticKind::ExpressionSuccessful);
+    target.GetStatistics().NotifyExprEval(true);
     return true;
   }
 
   // Increment statistics to record this expression evaluation failure.
+  target.GetStatistics().NotifyExprEval(false);
----------------
wallace wrote:
> actually this is not sufficient, we need to log as well in the SBAPI side, which is what lldb-vscode uses to evaluate expressions. So either move it to deeper point in the call chain or report this event from other locations
Gotcha, will move this.


================
Comment at: lldb/source/Commands/CommandObjectFrame.cpp:711
     bool res = result.Succeeded();
-    Target &target = GetSelectedOrDummyTarget();
-    if (res)
-      target.IncrementStats(StatisticKind::FrameVarSuccess);
-    else
-      target.IncrementStats(StatisticKind::FrameVarFailure);
+    GetSelectedOrDummyTarget().GetStatistics().NotifyFrameVar(res);
     return res;
----------------
wallace wrote:
> same here
"frame variable" takes optional args. If there are none, it dumps everything which will never fail, if it has args it can be one or more variable names or variable expressions. I can hook into a few more places for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111686