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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Apr 12 08:28:23 PDT 2018

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 command so I'd very much like to have this living as a separate entity.

I agree that "logging" and "statistics" sound like two different entities. Though in that case, maybe it's the "log timers" command that is misplaced. "performance timers" sound more like "statistics" than "logging" to me. Maybe eventually these two things should be merged even? I don't know what's the right solution here, and I don't care much about it even, though I would be sad if the choice we make now prevents some future unification due to backwards compatibility issues.

Comment at: lldb/packages/Python/lldbsuite/test/functionalities/stats/main.c:12
+  //%self.expect("stats enable")
+  //%self.expect("frame var", substrs=['27'])
+  //%self.expect("stats disable", substrs=['frame var successes : 1', 'frame var failures : 0'])
davide wrote:
> labath wrote:
> > Maybe test for failure as well (I guess trying to print an non-existing variable should do the trick)?
> Testing for failure here is a little tricky because the expression parser reports a failure only when it fails internally (and not when succeeds but returns an error because clangAST can't find the variable). Eventually we could make it more fine grained but for now I just would like to get the number of failures resolving variable due to expression parsing logic failing (and not because typos).
Makes sense, thanks for the explanation.

Comment at: lldb/source/Commands/CommandObjectStats.cpp:43-46
+    target->RegisterStats(StatisticKind::ExpressionSuccessful);
+    target->RegisterStats(StatisticKind::ExpressionFailure);
+    target->RegisterStats(StatisticKind::FrameVarSuccess);
+    target->RegisterStats(StatisticKind::FrameVarFailure);
davide wrote:
> labath wrote:
> > Are you planning to have some kind of decentralized method of registering statistics?
> > If not, then I don't see a need for explicitly registering the statistic kinds here, since the single source of truth about the available kinds can be StatisticKind enum, and Target can just get the list from there automatically when need (when stats are enabled?)
> > 
> > This way we could even simplify the code by avoiding the "statistic is not registered but someone still called IncrementStats" state altogether) and the stats map could be a simple array with NumStatistics elements.
> No. I originally thought to make it that way but I have to say that maybe it's easier to have a centralized mechanism. I'm still unsure whether this should be a vector or a map, I'll think about it more.
llvm manual recommends <http://llvm.org/docs/ProgrammersManual.html#map-like-containers-std-map-densemap-etc> vector as a first choice data structure for a map. :P


More information about the lldb-commits mailing list