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

[James Clark]

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.