Re: [PATCH v2 1/2] perf stat: Fix trailing comma when there is no metric unit

From: James Clark
Date: Fri Nov 01 2024 - 07:10:01 EST




On 31/10/2024 4:17 am, Ian Rogers wrote:
On Wed, Oct 30, 2024 at 4:40 AM James Clark <james.clark@xxxxxxxxxx> wrote:

Now that printing metric-value and metric-unit is optional,
print_running_json() shouldn't add the comma in case it becomes
trailing.

Replace all manual json comma stuff with a json_out() function that uses
the existing os->first tracking and auto inserts a comma if it's needed.
Update the test to handle that two of the fields can be missing.

Thanks for the larger clean up!

This fixes the following test failure on Cortex A57 where the branch
misses metric is missing a required event:

$ perf test -vvv "json output"

106: perf stat JSON output linter:
--- start ---
test child forked, pid 665682
Checking json output: no args Test failed for input:
...
{"counter-value" : "3112.000000", "unit" : "",
"event" : "armv8_pmuv3_1/branch-misses/",
"event-runtime" : 20699340, "pcnt-running" : 100.00, }
...
json.decoder.JSONDecodeError: Expecting property name enclosed in
double quotes: line 12 column 144 (char 2109)
---- end(-1) ----
106: perf stat JSON output linter : FAILED!

Fixes: e1cc918b6cfd ("perf stat: Drop metric-unit if unit is NULL")
Signed-off-by: James Clark <james.clark@xxxxxxxxxx>
---
.../tests/shell/lib/perf_json_output_lint.py | 14 +-
tools/perf/util/stat-display.c | 177 ++++++++++--------
2 files changed, 104 insertions(+), 87 deletions(-)

diff --git a/tools/perf/tests/shell/lib/perf_json_output_lint.py b/tools/perf/tests/shell/lib/perf_json_output_lint.py
index 8ddb85586131..b066d721f897 100644
--- a/tools/perf/tests/shell/lib/perf_json_output_lint.py
+++ b/tools/perf/tests/shell/lib/perf_json_output_lint.py
@@ -69,16 +69,16 @@ def check_json_output(expected_items):
for item in json.loads(input):
if expected_items != -1:
count = len(item)
- if count != expected_items and count >= 1 and count <= 7 and 'metric-value' in item:
+ if count not in expected_items and count >= 1 and count <= 7 and 'metric-value' in item:
# Events that generate >1 metric may have isolated metric
# values and possibly other prefixes like interval, core,
# aggregate-number, or event-runtime/pcnt-running from multiplexing.
pass
- elif count != expected_items and count >= 1 and count <= 5 and 'metricgroup' in item:
+ elif count not in expected_items and count >= 1 and count <= 5 and 'metricgroup' in item:
pass
- elif count == expected_items + 1 and 'metric-threshold' in item:
+ elif count - 1 in expected_items and 'metric-threshold' in item:
pass
- elif count != expected_items:
+ elif count not in expected_items:
raise RuntimeError(f'wrong number of fields. counted {count} expected {expected_items}'
f' in \'{item}\'')
for key, value in item.items():
@@ -90,11 +90,11 @@ def check_json_output(expected_items):

try:
if args.no_args or args.system_wide or args.event:
- expected_items = 7
+ expected_items = [5, 7]
elif args.interval or args.per_thread or args.system_wide_no_aggr:
- expected_items = 8
+ expected_items = [6, 8]
elif args.per_core or args.per_socket or args.per_node or args.per_die or args.per_cluster or args.per_cache:
- expected_items = 9
+ expected_items = [7, 9]
else:
# If no option is specified, don't check the number of items.
expected_items = -1
diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 53dcdf07f5a2..a5d72f4a515c 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -114,23 +114,44 @@ static void print_running_csv(struct perf_stat_config *config, u64 run, u64 ena)
fprintf(config->output, "%s%" PRIu64 "%s%.2f",
config->csv_sep, run, config->csv_sep, enabled_percent);
}
+struct outstate {
+ FILE *fh;
+ bool newline;
+ bool first;

It'd be nice to have kernel-doc capturing the meaning of these
variables. newline and first, why would something be a newline but not
first? I know the lack of documentation is a pre-existing condition.
Pretty much every variable in the struct below confuses me and I need
to read the code to try to figure it out.

+ const char *prefix;

Prefix of what?

+ int nfields;

Is this the number of columns in CSV format? Why not a name like
csv_columns then? What is a field here?

+ int aggr_nr;
+ struct aggr_cpu_id id;

Something to do with aggregation, presumably for the current CSV line.
Why two of them?

+ struct evsel *evsel;
+ struct cgroup *cgrp;

Maybe the counter and cgroup being printed, but we usually pass those
as extra arguments. This loses me.

I was hoping the code here could be more like the perf list json:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-list.c?h=perf-tools-next#n357
which avoids the comma problem by printing everything in one go.
There's so much spaghetti code in stat-display and before we had tests
there were frequent breakages. Anyway, I don't expect a larger clean
up, just venting. If you could do the comments and clear up the
newline vs first it'd be great.

Thanks,
Ian


Yep I can do that.

Thanks for the review.