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

[James Clark <james.clark@linaro.org>]

On 31/10/2024 4:17 am, Ian Rogers wrote:
> On Wed, Oct 30, 2024 at 4:40 AM James Clark <james.clark@linaro.org> 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@linaro.org>
>> ---
>>   .../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.