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

[Davide Italiano via Phabricator via lldb-commits]

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.



================
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'])
----------------
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).


================
Comment at: lldb/source/Commands/CommandObjectStats.cpp:31-35
+    if (!target) {
+      result.AppendError("stats command needs a target");
+      result.SetStatus(eReturnStatusFailed);
+      return false;
+    }
----------------
labath wrote:
> I think we have some way (should involve `eCommandRequiresTarget`) to avoid the need for checking this.
I'll check this out, thanks!


================
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);
----------------
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.


https://reviews.llvm.org/D45547