Re: [PATCH 0/2] perf test: Fix JSON linter

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Fri, Jan 20, 2023 at 09:37:33AM -0800, Ian Rogers escreveu:
> On Fri, Jan 20, 2023 at 5:41 AM Michael Petlan <mpetlan@redhat.com> wrote:
> >
> > Hello all.
> >
> > We have observed issues with the 'perf stat JSON output linter' testcase
> > on systems that allow topdown metrics.
> >
> > =========================
> >
> > 1) The topdown metrics handling hardcodes printing more than one metric
> > per line (refer to util/stat-shadow.c):
> >
> >     if (retiring > 0.7 && heavy_ops > 0.1)
> >         color = PERF_COLOR_GREEN;
> >     print_metric(config, ctxp, color, "%8.1f%%", "Heavy Operations",
> >             heavy_ops * 100.);
> >     if (retiring > 0.7 && light_ops > 0.6)
> >         color = PERF_COLOR_GREEN;
> >     else
> >         color = NULL;
> >     print_metric(config, ctxp, color, "%8.1f%%", "Light Operations",
> >             light_ops * 100.);
> >
> > Thus, the test cannot rely on the fact that there will be only one
> > metric per line.
> 
> Right, these implicit metrics are also an issue with --metric-only. It
> isn't clear from the command line flags when implicit and listed
> metrics should be generated. The topdown events (Icelake+) made the
> implicit metric problem worse, and they likely didn't exist or weren't
> tested at the time of the json output.
> 
> > 2) Since the JSON printing engine thinks that one and only one metric
> > is printed, it always closes the line by adding '}'. This is not true,
> > so I have fixed the comma printing and adjusted the engine to be
> > capable of printing 1-N metrics and then close the line.
> 
> So the current printing code is messy. For example, there is a newline
> callback function, but that has little meaning for json where
> everything is on a newline. For CSV mode, adding a summary used to
> cause a column to suddenly appear on the left for the row containing
> the summary. I believe the right thing to do is a refactor and
> recently I did a similar refactor for 'perf list':
> https://lore.kernel.org/lkml/20221114210723.2749751-11-irogers@google.com/
> So it is strange that metrics are "shadows" of events, but whatever
> the point is to think about the printing API. In the change linked
> above you can see that everytime something is printed the print
> handling code is given all the information and the print handling code
> has state. Based on the state and callback's information, the print
> handling code can do nothing, properly format for CSV, json, etc. This
> gets completely away from the kind of madness of branches and
> whac-a-mole the code currently has.
> 
> I'm happy to look at doing the refactor to be similar to 'perf
> list'/builtin-list.c. Namhyung I thought might have gotten to it with
> his recent work on improving aggregation. If I hear silence then I'll
> assume that is a request I do it :-)

Yeah, go for it :-)
 
> Pragmatically in the short term we could land your changes, but I

Ok, I saw Namhyung's Ack and will apply both, we can go from there.

- Arnaldo

> worry they are more of the whac-a-mole and something somewhere else
> will break. Which is why we have a test, and so I'm not overly
> worried. I can download and test too.
> 
> Anyway, sorry for the issue and let me know what you think. Thanks,
> Ian
> 
> 
> > =========================
> >
> > On machines that don't support topdown, the problem can be shown by
> > doubling the following line in util/stat-shadow.c, so that the simple
> > 'insn per cycle' metric is printed twice:
> >
> >   print_metric(config, ctxp, NULL, "%7.2f ", "insn per cycle", ratio);
> >
> > The worst problem is that the JSON line is broken, such as:
> >
> > ... "pcnt-running" : 100.00, "metric-value" : 3.501931, "metric-unit"
> >     : "Heavy Operations"}"metric-value" : 14.007787, "metric-unit" : ...
> >                   here ^^^^^
> >
> > =========================
> >
> > The first patch solves the JSON output correctness, the second tries
> > to adjust the testcase to some extent, so that it should work on the
> > topdown-capable machines.
> >
> > However, it's highly possible that the testcase will have to be fixed
> > or maybe rewritten in future. First, it has quite naive evaluation of
> > what's expected/correct/etc. Second, it does no JSON format validation
> > at all. As a linter it should do so though.
> >
> >  ***
> >
> > For further discussion: What about removing the check for number of
> > expected elements in the line and just check for the keywords, such
> > as "metric-value", "counter-value", "event", etc. and for some values
> > that should be numeric. And, do proper JSON format sanity check?
> >
> >
> > Thank you for inputs!
> >
> > Michael
> >
> >
> >
> >
> > Michael Petlan (2):
> >   perf stat: Fix JSON metric printout for multiple metrics per line
> >   perf test: Fix JSON format linter test checks
> >
> >  .../tests/shell/lib/perf_json_output_lint.py  | 16 +++++------
> >  tools/perf/util/stat-display.c                | 28 +++++++++++--------
> >  2 files changed, 24 insertions(+), 20 deletions(-)
> >
> > --
> > 2.18.4
> >

-- 

- Arnaldo