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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
