[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

davide added a comment. In https://reviews.llvm.org/D45547#1066044, @xiaobai wrote: > I really like this idea! It will be very helpful for @sas and I. I'd like to > +1 creating a separate `stats dump` subcommand instead of dumping stats on > `stats disable`. Thanks! If you has ideas or want

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

davide added inline comments. Comment at: lldb/source/Commands/CommandObjectExpression.cpp:626 + Target *target = m_interpreter.GetExecutionContext().GetTargetPtr(); + if (!target) jingham wrote: > This is what CommandObject::GetSelectedOrDummy target is

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

jingham added a comment. I made a few comments all to the same effect, you can use CommandObject::GetSelectedOrDummyTarget to get the target that the command is operating on, or you can get is straight from m_exe_ctx if you want to make sure that you are getting a real target. In the case of

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

davide added inline comments. Comment at: lldb/source/Commands/CommandObjectStats.cpp:37 + +if (target->GetCollectingStats() == true) { + result.AppendError("stats already enabled"); xiaobai wrote: > nit: You can drop the `== true` Thanks, I'll fix

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

davide added a comment. In https://reviews.llvm.org/D45547#1066348, @jingham wrote: > Timers seemed like they would be really useful for collection of data about > operations in lldb, but for most things I think they end up being hard to use > because actual wall-clock time is so variable from

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

davide updated this revision to Diff 142301. https://reviews.llvm.org/D45547 Files: lldb/include/lldb/Target/Target.h lldb/include/lldb/lldb-private-enumerations.h lldb/packages/Python/lldbsuite/test/functionalities/stats/Makefile

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

jingham added a comment. Timers seemed like they would be really useful for collection of data about operations in lldb, but for most things I think they end up being hard to use because actual wall-clock time is so variable from run to run, and especially for disk and inter-process heavy

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

xiaobai added subscribers: sas, xiaobai. xiaobai added a comment. I really like this idea! It will be very helpful for @sas and I. I'd like to +1 creating a separate `stats dump` subcommand instead of dumping stats on `stats disable`. Comment at:

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

davide added a comment. I'm under the impression that we should either merge `log timers` with `stats` or just remove log timers altogether and start from scratch. From what I hear from Jim, it was really useful for a few people, so maybe a fresh start would be a better way of handling things.

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

davide added inline comments. Comment at: lldb/source/Commands/CommandObjectStats.cpp:43-46 +target->RegisterStats(StatisticKind::ExpressionSuccessful); +target->RegisterStats(StatisticKind::ExpressionFailure); +target->RegisterStats(StatisticKind::FrameVarSuccess);

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

labath added a comment. In https://reviews.llvm.org/D45547#1065661, @davide wrote: > I prefer having it as a top level command rather than part of log. If you > think about it LLVM does the same distinction and it worked quite well in > practice. > We plan to collect more metrics to the

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

davide added a comment. I prefer having it as a top level command rather than part of log. If you think about it LLVM does the same distinction and it worked quite well in practice. We plan to collect more metrics to the command so I'd very much like to have this living as a separate entity.

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

labath added a comment. I don't see myself using this command personally, but I agree that splitting the "disable stats collection" and "dump accumulated stats" into two actions seems a better choice (maybe disable could do a final auto-dump though). I also think the header/footer is not

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

davide added a comment. In https://reviews.llvm.org/D45547#1065054, @jasonmolenda wrote: > Ah, no you couldn't set up a command alias like that. Still, if the full > name was statistics, you could type 'stat' and it would match. 'stats' > wouldn't, though. > > I do think decoupling the

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

jasonmolenda added a comment. Ah, no you couldn't set up a command alias like that. Still, if the full name was statistics, you could type 'stat' and it would match. 'stats' wouldn't, though. I do think decoupling the disabling action from the dumping action would be an improvement. You

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

jasonmolenda added a comment. My two cents, Why print the '### Start STATISTICS dump ###' header/footer when printing the results? I don't think we demarcate result output like that anywhere else in lldb. I don't think it adds anything for the user. I would probably name the command

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

davide created this revision. davide added reviewers: jingham, friss, JDevlieghere, aprantl, labath, clayborg. This allows us to collect useful metrics about lldb debugging sessions. I thought that an example would be better than a thousand words: Process 19705 stopped * thread #1, queue =