Re: [PATCH 2/2] perf stat: Also hide metric from JSON if units are an empty string

From: James Clark
Date: Wed Oct 30 2024 - 05:37:00 EST




On 30/10/2024 2:45 am, Ian Rogers wrote:
On Tue, Oct 29, 2024 at 5:42 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:

Hello,

On Fri, Oct 25, 2024 at 10:03:05AM +0100, James Clark wrote:
We decided to hide NULL metric units rather than showing it as "(null)",
but on hybrid systems if the process doesn't hit a PMU you get an empty
string metric unit instead. To make it consistent also remove empty
strings.

Note that metric-threshold is already hidden in this case without this
change.

Where a process only runs on cpu_core and never hits cpu_atom:
Before:
$ perf stat -j -- true
...
{"counter-value" : "<not counted>", "unit" : "", "event" : "cpu_atom/branch-misses/", "event-runtime" : 0, "pcnt-running" : 0.00, "metric-value" : "0.000000", "metric-unit" : ""}
{"counter-value" : "6326.000000", "unit" : "", "event" : "cpu_core/branch-misses/", "event-runtime" : 293786, "pcnt-running" : 100.00, "metric-value" : "3.553394", "metric-unit" : "of all branches", "metric-threshold" : "good"}
...

I guess you're talking about "metric-unit", not plain "unit", right?
Then please update the subject line to reduce the config.


Yep I'll update it.

Ian, can you please review?

It'd be nice to see the stack trace for when metric-unit is "" as I'm
not seeing the logic in stat-shadow.c. If we know the caller than it
seems logical the unit can be passed as NULL rather than "".

Thanks,
Ian



Here's the stack:

print_metric_json() (stat-display.c:516)
printout() (stat-display.c:912)
print_counter_aggrdata() (stat-display.c:1110)
print_counter() (stat-display.c:1224)
evlist__print_counters() (stat-display.c:1734)
print_counters() (builtin-stat.c:1016)
cmd_stat() (builtin-stat.c:2872)
run_builtin() (perf/perf.c:351)
handle_internal_command() (perf.c:404)
run_argv() (perf.c:448)
main() (perf.c:560)

The empty string is from printout():

pm(config, os, METRIC_THRESHOLD_UNKNOWN, /*format=*/NULL, /*unit=*/"",
/*val=*/0);

Changing it to NULL seems to work, so is probably a bit neater. I can do that if you think that makes sense?

There's another one for --metric-only that I didn't run into, but it could also make sense to change:

if (config->metric_only) {
pm(config, os, METRIC_THRESHOLD_UNKNOWN, "", "", 0);

Thanks,
Namhyung


After:
...
{"counter-value" : "<not counted>", "unit" : "", "event" : "cpu_atom/branch-misses/", "event-runtime" : 0, "pcnt-running" : 0.00}
{"counter-value" : "5778.000000", "unit" : "", "event" : "cpu_core/branch-misses/", "event-runtime" : 282240, "pcnt-running" : 100.00, "metric-value" : "3.226797", "metric-unit" : "of all branches", "metric-threshold" : "good"}
...

Signed-off-by: James Clark <james.clark@xxxxxxxxxx>
---
tools/perf/util/stat-display.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index a5d72f4a515c..9b7fd985a42a 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -506,7 +506,7 @@ static void print_metric_json(struct perf_stat_config *config __maybe_unused,
struct outstate *os = ctx;
FILE *out = os->fh;

- if (unit) {
+ if (unit && strlen(unit)) {
json_out(os, "\"metric-value\" : \"%f\", \"metric-unit\" : \"%s\"", val, unit);
if (thresh != METRIC_THRESHOLD_UNKNOWN) {
json_out(os, "\"metric-threshold\" : \"%s\"",
--
2.34.1