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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

I'm not super happy with the debugger stats as they are right here. Given that it only affects a subset of this patch and I don't want to block other work that depends on this, it's probably easier for me to create a patch on top of this rather than iterating on this one.

LGTM



================
Comment at: lldb/include/lldb/Target/Statistics.h:96
+};
+
+class GlobalStats {
----------------
clayborg wrote:
> JDevlieghere wrote:
> > Do we expect there to be something like `DebuggerStats`? I think it would be nice from a hierarchy perspective that Global Stats have a map of debugger -> to debuggerstat who then in turn hold on to a map of target -> target stats. That hierarchy would work really well for JSON (except they would be lists instead of maps). 
> I can change this to DebuggerStats as this is essentially what this was.
I do need something that sits about the debugger for the lldb_assert case.


================
Comment at: lldb/include/lldb/Target/Statistics.h:114
+  static bool g_collecting_stats;
+};
+
----------------
clayborg wrote:
> This can be expensive if you start locking a mutex just to increment a stat that is a counter and will make statistics slow down the debugger. I would rather rely on std::atomic or the locks already built into Target or Debugger if possible. If lldb_asserts are firing off at a high rate, we can't be spending thousands of instructions locking and unlocking mutexes.
Yeah, and atomic variable for a particular metric might do the trick. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111686