[Lldb-commits] [PATCH] D110804: Add a new command "target metrics".

Thu Sep 30 14:55:09 PDT 2021

jingham added a comment.

In D110804#3034878 <https://reviews.llvm.org/D110804#3034878>, @clayborg wrote:

> I would like to get a consensus on if we should move this functionality to "statistics" or not. The reasons that I didn't do it were:
>
> - "statistics" as a top level method doesn't really do what I wanted here, which was "target statistics", or stats that are related only to the current target. I am not all that interested in overall stats over all targets

It seems confusing to have two ways to gather internal statistics (well three really because we also still have "log timers").  So it does seem good to gather at least the two obviously similar ones in one place.

We have loads of commands that operate on the "currently selected target" even though they aren't in the target hierarchy.  So having the target specific parts of the statistics gathering do that as well is in line with many other commands.  I don't think that's a problem.  It might even be nice to have a --target-id so that if I wanted statistics on several of the targets I have loaded, I could do it with one command rather than having to issue the command over and over.

On a side note, I'm not sure it's really correct to limit the statistics to only one target, at least for any symbol or debug info parsing information.  Since all the parsing goes on in the modules in the global shared cache, if you did have two targets running in one lldb session, then parsing module A will formally have to have been done on behalf of both targets, but will be triggered by only one or the other target.  If the target that triggered parsing of A is not the current target, will the time to parse that get dropped?  The work had to be done, and was only accounted to the other target by a timing accident.

> - I didn't want to have to make a human readable and JSON output

We have a pretty printer for StructuredData objects which are trivial to make from JSON, so a human-readable version should be easy to knock together.  That will do for the first pass, and since we seem to be leaning to JSON as a format for various data gathering, if we want to make the output better we could improve the pretty printer, and add header names to our JSON, etc.  But we can do that incrementally

> - the "statistics enable"/"statistics disable" didn't seem to make sense as they were coded in top of tree. Maybe enabling and disabling makes more sense when more info is gathered in hidden patches that have not been upstreamed?

If we are confident that gathering statistics really does have and always will have negligible performance impact, then we can drop the "enable" and "disable".  I don't know that we can promise we'll never want to gather something that's a bit costly.  And there are folks who run lldb in environments where they would rather we not spend time & memory on anything we don't absolutely have to.

So unless it's somehow prohibitively hard to disable gathering statistics, my feeling is we should offer that option.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110804/new/

https://reviews.llvm.org/D110804