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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

I have a few comments but I think this is going in a direction that will work for both of us. Once this lands I can sign up for the work to untie the statistics form the target.



================
Comment at: lldb/include/lldb/Target/Statistics.h:23
+struct SuccessFailStats {
+  void Notify(bool success);
+
----------------
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. 


================
Comment at: lldb/include/lldb/Target/Statistics.h:28
+  uint32_t successes = 0;
+  uint32_t failures = 0;
+};
----------------
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
},
```



================
Comment at: lldb/include/lldb/Target/Statistics.h:31
+
+class TargetStats {
+public:
----------------
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.


================
Comment at: lldb/include/lldb/Target/Statistics.h:35
+
+  void SetLaunchOrAttachTime();
+  void SetFirstPrivateStopTime();
----------------
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?



================
Comment at: lldb/include/lldb/Target/Statistics.h:51
+  // and "statistics disable".
+  bool m_collecting_stats = false;
+  ElapsedTime::Duration create_time{0.0};
----------------
In line with my earlier comment about not tying the stats tot he target, this would be controlled by the global (singleton) statistic class. 


================
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;
----------------
I would abstract this behind a `TimeStats` or `TimeStats`, similar to SuccessFailStats, so that every type of statistic has its own type.  


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111686