clayborg added inline comments.
================
Comment at: lldb/include/lldb/Target/Statistics.h:23
+struct SuccessFailStats {
+ void Notify(bool success);
+
----------------
wallace wrote:
> JDevlieghere wrote:
> > I'd use an enum class with `Success` and `Failure` here instead of a bool
> > that requires you to know what the argument is named to know if true means
> > success of failure.
> Or maybe just NotifySuccess(bool) to keep callers simple
I like the idea of adding:
```
void NotifySuccess();
void NotifyFailure();
```
JDevlieghere you ok with that?
================
Comment at: lldb/include/lldb/Target/Statistics.h:28
+ uint32_t successes = 0;
+ uint32_t failures = 0;
+};
----------------
JDevlieghere wrote:
> I would add a member for the name so that `ToJSON` can generate the whole
> JSON object. I imagine something like
>
> ```
> SuccessFailStats s("frameVariable");
> s.Notify(SuccessFailStats::Success);
> s.ToJSON()
> ```
>
> which generates
>
> ```
> "frameVariable": {
> "failures": 0,
> "successes": 1
> },
> ```
>
Good idea!
================
Comment at: lldb/include/lldb/Target/Statistics.h:31
+
+class TargetStats {
+public:
----------------
JDevlieghere wrote:
> The direction I outlined in the other patch is to move away from tying the
> statistics to the target. I think the class itself is fine, as long as we
> agree that the target stats would be held by an object spanning all debuggers.
Yeah, my main patch did a little of both. The main idea behind the classes in
this file are they don't need to be the storage for the stats, but they can be
if that makes things easier. The main classes (Target, Module, Breakpoint, etc)
can just have member variables in the main classes themselves, but anything
inside of these classes can/should be used in case we want to aggregate stats
from any contained Stats classes. For example, I want to have a ModuleStats
class (see https://reviews.llvm.org/D110804), this class doesn't not live
inside of Module itself, but can be constructed from a Module. Then the
TargetStats class can have a std::vector<ModuleStats> classes that it gathers
when TargetStats is asked to collect statistics. This same methodology can then
be applied to the debugger where we have DebuggerStats.
What do you think of this approach:
- Any class can define a XXXStats class in this file which is used to collect
statistics at any given point
- Classes can use the XXXStats class as a member variable if it makes sense, or
just add member variables to store any objects needed to calculate the stats
later when the object is asked to collect stats
(target.GatherStatistics(target_stats);).
This would mean we could make a DebuggerStats class that can have a
std::vector<TargetStats>, and when the debugger.GatherStatistics(...) is
called. Then either the DebuggerStats or TargetStats object can be asked to be
converted to JSON when presenting to the user in the "statistics dump" command?
================
Comment at: lldb/include/lldb/Target/Statistics.h:35
+
+ void SetLaunchOrAttachTime();
+ void SetFirstPrivateStopTime();
----------------
JDevlieghere wrote:
> Why don't we have these return an `ElapsedTime`. Then you can do something
> like
>
> ```
> ElapsedTime elapsed_time = m_stats.GetCreateTime();
> ```
>
> RVO should ensure there are no copies, but you can always delete the copy
> ctor to make sure. Maybe we should call it ScopedTime or something to make it
> more obvious that this is a RAII object?
>
These are setting the time at which things start. If we follow suggestions
above, SetLaunchOrAttachTime(), SetFirstPrivateStopTime(), and
SetFirstPublicStopTime() can be remove and all of the
llvm::Optional<std::chrono::steady_clock::time_point> member variables can be
moved to Target class, and we can store only
"llvm::Optional<ElapsedTime::Duration> launch_or_attach_time;" member variables
in the TargetStats class, which would get set when statistics are gathered?
================
Comment at: lldb/include/lldb/Target/Statistics.h:51
+ // and "statistics disable".
+ bool m_collecting_stats = false;
+ ElapsedTime::Duration create_time{0.0};
----------------
JDevlieghere wrote:
> In line with my earlier comment about not tying the stats tot he target, this
> would be controlled by the global (singleton) statistic class.
I put this here because of the existing SB API:
```
bool SBTarget::GetCollectingStats();
void SBTarget::SetCollectingStats(bool);
```
So you want debugger to have this? This m_collecting_stats can be moved to
Debugger, or DebuggerStats? But that makes the above API useless or it becomes
deprecated. We really don't need to set if we are collecting stats IMHO, since
collection should be cheap. You ok with deprecating the "statistics
enable"/"statistics disable" and the above APIs?
================
Comment at: lldb/include/lldb/Target/Statistics.h:53
+ ElapsedTime::Duration create_time{0.0};
+ llvm::Optional<std::chrono::steady_clock::time_point> launch_or_attach_time;
+ llvm::Optional<std::chrono::steady_clock::time_point>
first_private_stop_time;
----------------
JDevlieghere wrote:
> I would abstract this behind a `TimeStats` or `TimeStats`, similar to
> SuccessFailStats, so that every type of statistic has its own type.
I will make a ElapsedTime::TimePoint
================
Comment at: lldb/include/lldb/Target/Target.h:1466-1468
+ /// \param [in] modules
+ /// If true, include an array of metrics for each module loaded in the
+ /// target.
----------------
wallace wrote:
> this param is not present
I will remove it!
================
Comment at: lldb/include/lldb/Utility/ElapsedTime.h:34
+/// breakpoint each time a new shared library is loaded.
+class ElapsedTime {
+public:
----------------
JDevlieghere wrote:
> This seems simple enough and specific enough to the statistics that it can be
> part of that header?
Yes, good idea.
================
Comment at: lldb/include/lldb/lldb-forward.h:121
class MemoryRegionInfos;
+struct StatsDumpOptions;
class Module;
----------------
wallace wrote:
> move this to line ~277
I actually need to remove this for now as we don't have this class yet!
================
Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:761
+ def getShlibBuildArtifact(self, DYLIB_NAME):
+ """Return absolute path to a shared library artifact given the library
----------------
wallace wrote:
> is this actually used somewhere?
Not yet, I will remove it
================
Comment at: lldb/source/API/SBTarget.cpp:216-220
+ std::string json_text;
+ llvm::raw_string_ostream stream(json_text);
+ llvm::json::Value json = target_sp->ReportStatistics();
+ stream << json;
+ data.m_impl_up->SetObjectSP(StructuredData::ParseJSON(stream.str()));
----------------
wallace wrote:
> a simpler version
Will switch!
================
Comment at: lldb/source/Commands/CommandObjectExpression.cpp:662-668
// Increment statistics to record this expression evaluation success.
- target.IncrementStats(StatisticKind::ExpressionSuccessful);
+ target.GetStatistics().NotifyExprEval(true);
return true;
}
// Increment statistics to record this expression evaluation failure.
+ target.GetStatistics().NotifyExprEval(false);
----------------
wallace wrote:
> actually this is not sufficient, we need to log as well in the SBAPI side,
> which is what lldb-vscode uses to evaluate expressions. So either move it to
> deeper point in the call chain or report this event from other locations
Gotcha, will move this.
================
Comment at: lldb/source/Commands/CommandObjectFrame.cpp:711
bool res = result.Succeeded();
- Target &target = GetSelectedOrDummyTarget();
- if (res)
- target.IncrementStats(StatisticKind::FrameVarSuccess);
- else
- target.IncrementStats(StatisticKind::FrameVarFailure);
+ GetSelectedOrDummyTarget().GetStatistics().NotifyFrameVar(res);
return res;
----------------
wallace wrote:
> same here
"frame variable" takes optional args. If there are none, it dumps everything
which will never fail, if it has args it can be one or more variable names or
variable expressions. I can hook into a few more places for this.
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
