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 t
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL330043: [Command] Implement `statistics` command. (authored
by davide, committed by ).
Herald added a subscriber: llvm-com
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 for.
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 s
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 these
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
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/packages/Python/lldbsuite/test/functionalities/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 opera
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/source/Commands/CommandO
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. T
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);
+
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 comman
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.
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 needed
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 disabl
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 may
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 statisti
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 = '
18 matches
Mail list logo
