[5/9] perf stat: Fix --metric-only --json output

  Currently it prints all metric headers for JSON output.  But actually it
skips some metrics with valid_only_metric().  So the output looks like:

  $ perf stat --metric-only --json true
  {"unit" : "CPUs utilized", "unit" : "/sec", "unit" : "/sec", "unit" : "/sec", "unit" : "GHz", "unit" : "insn per cycle", "unit" : "/sec", "unit" : "branch-misses of all branches"}
  {"metric-value" : "3.861"}{"metric-value" : "0.79"}{"metric-value" : "3.04"}

As you can see there are 8 units in the header but only 3 metric-values
are there.  It should skip the unused headers as well.  Also each unit
should be printed as a separate object like metric values.

With this patch:

  $ perf stat --metric-only --json true
  {"unit" : "GHz"}{"unit" : "insn per cycle"}{"unit" : "branch-misses of all branches"}
  {"metric-value" : "4.166"}{"metric-value" : "0.73"}{"metric-value" : "2.96"}

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)
  

Em Mon, Nov 07, 2022 at 01:33:10PM -0800, Namhyung Kim escreveu:
> Currently it prints all metric headers for JSON output.  But actually it
> skips some metrics with valid_only_metric().  So the output looks like:
> 
>   $ perf stat --metric-only --json true
>   {"unit" : "CPUs utilized", "unit" : "/sec", "unit" : "/sec", "unit" : "/sec", "unit" : "GHz", "unit" : "insn per cycle", "unit" : "/sec", "unit" : "branch-misses of all branches"}
>   {"metric-value" : "3.861"}{"metric-value" : "0.79"}{"metric-value" : "3.04"}
> 
> As you can see there are 8 units in the header but only 3 metric-values
> are there.  It should skip the unused headers as well.  Also each unit
> should be printed as a separate object like metric values.
> 
> With this patch:
> 
>   $ perf stat --metric-only --json true
>   {"unit" : "GHz"}{"unit" : "insn per cycle"}{"unit" : "branch-misses of all branches"}
>   {"metric-value" : "4.166"}{"metric-value" : "0.73"}{"metric-value" : "2.96"}

Can we get a Fixes tag for this one?

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/stat-display.c | 22 +++-------------------
>  1 file changed, 3 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 115477461224..515fb6db3d67 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -430,12 +430,12 @@ static void print_metric_header(struct perf_stat_config *config,
>  	    os->evsel->priv != os->evsel->evlist->selected->priv)
>  		return;
>  
> -	if (!valid_only_metric(unit) && !config->json_output)
> +	if (!valid_only_metric(unit))
>  		return;
>  	unit = fixunit(tbuf, os->evsel, unit);
>  
>  	if (config->json_output)
> -		fprintf(os->fh, "\"unit\" : \"%s\"", unit);
> +		fprintf(os->fh, "{\"unit\" : \"%s\"}", unit);
>  	else if (config->csv_output)
>  		fprintf(os->fh, "%s%s", unit, config->csv_sep);
>  	else
> @@ -847,10 +847,6 @@ static void print_metric_headers(struct perf_stat_config *config,
>  		.new_line = new_line_metric,
>  		.force_header = true,
>  	};
> -	bool first = true;
> -
> -	if (config->json_output && !config->interval)
> -		fprintf(config->output, "{");
>  
>  	if (prefix && !config->json_output)
>  		fprintf(config->output, "%s", prefix);
> @@ -871,18 +867,12 @@ static void print_metric_headers(struct perf_stat_config *config,
>  	evlist__for_each_entry(evlist, counter) {
>  		os.evsel = counter;
>  
> -		if (!first && config->json_output)
> -			fprintf(config->output, ", ");
> -		first = false;
> -
>  		perf_stat__print_shadow_stats(config, counter, 0,
>  					      0,
>  					      &out,
>  					      &config->metric_events,
>  					      &rt_stat);
>  	}
> -	if (config->json_output)
> -		fprintf(config->output, "}");
>  	fputc('\n', config->output);
>  }
>  
> @@ -954,14 +944,8 @@ static void print_interval(struct perf_stat_config *config,
>  		}
>  	}
>  
> -	if ((num_print_interval == 0 || config->interval_clear)
> -			 && metric_only && !config->json_output)
> +	if ((num_print_interval == 0 || config->interval_clear) && metric_only)
>  		print_metric_headers(config, evlist, " ", true);
> -	if ((num_print_interval == 0 || config->interval_clear)
> -			 && metric_only && config->json_output) {
> -		fprintf(output, "{");
> -		print_metric_headers(config, evlist, " ", true);
> -	}
>  	if (++num_print_interval == 25)
>  		num_print_interval = 0;
>  }
> -- 
> 2.38.1.431.g37b22c650d-goog
  

Hi Arnaldo,

On Tue, Nov 8, 2022 at 12:45 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Mon, Nov 07, 2022 at 01:33:10PM -0800, Namhyung Kim escreveu:
> > Currently it prints all metric headers for JSON output.  But actually it
> > skips some metrics with valid_only_metric().  So the output looks like:
> >
> >   $ perf stat --metric-only --json true
> >   {"unit" : "CPUs utilized", "unit" : "/sec", "unit" : "/sec", "unit" : "/sec", "unit" : "GHz", "unit" : "insn per cycle", "unit" : "/sec", "unit" : "branch-misses of all branches"}
> >   {"metric-value" : "3.861"}{"metric-value" : "0.79"}{"metric-value" : "3.04"}
> >
> > As you can see there are 8 units in the header but only 3 metric-values
> > are there.  It should skip the unused headers as well.  Also each unit
> > should be printed as a separate object like metric values.
> >
> > With this patch:
> >
> >   $ perf stat --metric-only --json true
> >   {"unit" : "GHz"}{"unit" : "insn per cycle"}{"unit" : "branch-misses of all branches"}
> >   {"metric-value" : "4.166"}{"metric-value" : "0.73"}{"metric-value" : "2.96"}
>
> Can we get a Fixes tag for this one?

I think this is it:

df936cadfb58 ("perf stat: Add JSON output option")

But this also depends on patch 4.  Do you want me to rebase
this not to depend on it?

Thanks,
Namhyung

>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/stat-display.c | 22 +++-------------------
> >  1 file changed, 3 insertions(+), 19 deletions(-)
> >
> > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> > index 115477461224..515fb6db3d67 100644
> > --- a/tools/perf/util/stat-display.c
> > +++ b/tools/perf/util/stat-display.c
> > @@ -430,12 +430,12 @@ static void print_metric_header(struct perf_stat_config *config,
> >           os->evsel->priv != os->evsel->evlist->selected->priv)
> >               return;
> >
> > -     if (!valid_only_metric(unit) && !config->json_output)
> > +     if (!valid_only_metric(unit))
> >               return;
> >       unit = fixunit(tbuf, os->evsel, unit);
> >
> >       if (config->json_output)
> > -             fprintf(os->fh, "\"unit\" : \"%s\"", unit);
> > +             fprintf(os->fh, "{\"unit\" : \"%s\"}", unit);
> >       else if (config->csv_output)
> >               fprintf(os->fh, "%s%s", unit, config->csv_sep);
> >       else
> > @@ -847,10 +847,6 @@ static void print_metric_headers(struct perf_stat_config *config,
> >               .new_line = new_line_metric,
> >               .force_header = true,
> >       };
> > -     bool first = true;
> > -
> > -     if (config->json_output && !config->interval)
> > -             fprintf(config->output, "{");
> >
> >       if (prefix && !config->json_output)
> >               fprintf(config->output, "%s", prefix);
> > @@ -871,18 +867,12 @@ static void print_metric_headers(struct perf_stat_config *config,
> >       evlist__for_each_entry(evlist, counter) {
> >               os.evsel = counter;
> >
> > -             if (!first && config->json_output)
> > -                     fprintf(config->output, ", ");
> > -             first = false;
> > -
> >               perf_stat__print_shadow_stats(config, counter, 0,
> >                                             0,
> >                                             &out,
> >                                             &config->metric_events,
> >                                             &rt_stat);
> >       }
> > -     if (config->json_output)
> > -             fprintf(config->output, "}");
> >       fputc('\n', config->output);
> >  }
> >
> > @@ -954,14 +944,8 @@ static void print_interval(struct perf_stat_config *config,
> >               }
> >       }
> >
> > -     if ((num_print_interval == 0 || config->interval_clear)
> > -                      && metric_only && !config->json_output)
> > +     if ((num_print_interval == 0 || config->interval_clear) && metric_only)
> >               print_metric_headers(config, evlist, " ", true);
> > -     if ((num_print_interval == 0 || config->interval_clear)
> > -                      && metric_only && config->json_output) {
> > -             fprintf(output, "{");
> > -             print_metric_headers(config, evlist, " ", true);
> > -     }
> >       if (++num_print_interval == 25)
> >               num_print_interval = 0;
> >  }
> > --
> > 2.38.1.431.g37b22c650d-goog
>
> --
>
> - Arnaldo
  

On Mon, Nov 7, 2022 at 1:33 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Currently it prints all metric headers for JSON output.  But actually it
> skips some metrics with valid_only_metric().  So the output looks like:
>
>   $ perf stat --metric-only --json true
>   {"unit" : "CPUs utilized", "unit" : "/sec", "unit" : "/sec", "unit" : "/sec", "unit" : "GHz", "unit" : "insn per cycle", "unit" : "/sec", "unit" : "branch-misses of all branches"}
>   {"metric-value" : "3.861"}{"metric-value" : "0.79"}{"metric-value" : "3.04"}
>
> As you can see there are 8 units in the header but only 3 metric-values
> are there.  It should skip the unused headers as well.  Also each unit
> should be printed as a separate object like metric values.
>
> With this patch:
>
>   $ perf stat --metric-only --json true
>   {"unit" : "GHz"}{"unit" : "insn per cycle"}{"unit" : "branch-misses of all branches"}
>   {"metric-value" : "4.166"}{"metric-value" : "0.73"}{"metric-value" : "2.96"}

It looks like the correct output would be to place the unit with the
metric-value, but perhaps that's something best done by deleting the
output code and starting again - the new output is wrong but better.
We could really do with functions like emit counter, emit metric and
have appropriate output for CSV, JSON, etc. A lot of the problems in
JSON and CSV outputting come from the unnatural way it is driven and
placing a large significance on newline characters. Putting the CSV
and JSON output to stdout also is problematic, as it naturally gets
interwoven with the monitored commands output. The dictionaries in the
new output should really be in a list, this is fixed in the test here:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/shell/lib/perf_json_output_lint.py?h=perf/core#n74

Thanks,
Ian

>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/stat-display.c | 22 +++-------------------
>  1 file changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 115477461224..515fb6db3d67 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -430,12 +430,12 @@ static void print_metric_header(struct perf_stat_config *config,
>             os->evsel->priv != os->evsel->evlist->selected->priv)
>                 return;
>
> -       if (!valid_only_metric(unit) && !config->json_output)
> +       if (!valid_only_metric(unit))
>                 return;
>         unit = fixunit(tbuf, os->evsel, unit);
>
>         if (config->json_output)
> -               fprintf(os->fh, "\"unit\" : \"%s\"", unit);
> +               fprintf(os->fh, "{\"unit\" : \"%s\"}", unit);
>         else if (config->csv_output)
>                 fprintf(os->fh, "%s%s", unit, config->csv_sep);
>         else
> @@ -847,10 +847,6 @@ static void print_metric_headers(struct perf_stat_config *config,
>                 .new_line = new_line_metric,
>                 .force_header = true,
>         };
> -       bool first = true;
> -
> -       if (config->json_output && !config->interval)
> -               fprintf(config->output, "{");
>
>         if (prefix && !config->json_output)
>                 fprintf(config->output, "%s", prefix);
> @@ -871,18 +867,12 @@ static void print_metric_headers(struct perf_stat_config *config,
>         evlist__for_each_entry(evlist, counter) {
>                 os.evsel = counter;
>
> -               if (!first && config->json_output)
> -                       fprintf(config->output, ", ");
> -               first = false;
> -
>                 perf_stat__print_shadow_stats(config, counter, 0,
>                                               0,
>                                               &out,
>                                               &config->metric_events,
>                                               &rt_stat);
>         }
> -       if (config->json_output)
> -               fprintf(config->output, "}");
>         fputc('\n', config->output);
>  }
>
> @@ -954,14 +944,8 @@ static void print_interval(struct perf_stat_config *config,
>                 }
>         }
>
> -       if ((num_print_interval == 0 || config->interval_clear)
> -                        && metric_only && !config->json_output)
> +       if ((num_print_interval == 0 || config->interval_clear) && metric_only)
>                 print_metric_headers(config, evlist, " ", true);
> -       if ((num_print_interval == 0 || config->interval_clear)
> -                        && metric_only && config->json_output) {
> -               fprintf(output, "{");
> -               print_metric_headers(config, evlist, " ", true);
> -       }
>         if (++num_print_interval == 25)
>                 num_print_interval = 0;
>  }
> --
> 2.38.1.431.g37b22c650d-goog
>
  

Em Tue, Nov 08, 2022 at 02:07:39PM -0800, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> On Tue, Nov 8, 2022 at 12:45 PM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Mon, Nov 07, 2022 at 01:33:10PM -0800, Namhyung Kim escreveu:
> > > Currently it prints all metric headers for JSON output.  But actually it
> > > skips some metrics with valid_only_metric().  So the output looks like:
> > >
> > >   $ perf stat --metric-only --json true
> > >   {"unit" : "CPUs utilized", "unit" : "/sec", "unit" : "/sec", "unit" : "/sec", "unit" : "GHz", "unit" : "insn per cycle", "unit" : "/sec", "unit" : "branch-misses of all branches"}
> > >   {"metric-value" : "3.861"}{"metric-value" : "0.79"}{"metric-value" : "3.04"}
> > >
> > > As you can see there are 8 units in the header but only 3 metric-values
> > > are there.  It should skip the unused headers as well.  Also each unit
> > > should be printed as a separate object like metric values.
> > >
> > > With this patch:
> > >
> > >   $ perf stat --metric-only --json true
> > >   {"unit" : "GHz"}{"unit" : "insn per cycle"}{"unit" : "branch-misses of all branches"}
> > >   {"metric-value" : "4.166"}{"metric-value" : "0.73"}{"metric-value" : "2.96"}
> >
> > Can we get a Fixes tag for this one?
> 
> I think this is it:
> 
> df936cadfb58 ("perf stat: Add JSON output option")

I'll add it
 
> But this also depends on patch 4.  Do you want me to rebase
> this not to depend on it?

Nope, I put the first one on perf/urgent, 2- on perf/core,

- Arnaldo
 
> Thanks,
> Namhyung
> 
> >
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > ---
> > >  tools/perf/util/stat-display.c | 22 +++-------------------
> > >  1 file changed, 3 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> > > index 115477461224..515fb6db3d67 100644
> > > --- a/tools/perf/util/stat-display.c
> > > +++ b/tools/perf/util/stat-display.c
> > > @@ -430,12 +430,12 @@ static void print_metric_header(struct perf_stat_config *config,
> > >           os->evsel->priv != os->evsel->evlist->selected->priv)
> > >               return;
> > >
> > > -     if (!valid_only_metric(unit) && !config->json_output)
> > > +     if (!valid_only_metric(unit))
> > >               return;
> > >       unit = fixunit(tbuf, os->evsel, unit);
> > >
> > >       if (config->json_output)
> > > -             fprintf(os->fh, "\"unit\" : \"%s\"", unit);
> > > +             fprintf(os->fh, "{\"unit\" : \"%s\"}", unit);
> > >       else if (config->csv_output)
> > >               fprintf(os->fh, "%s%s", unit, config->csv_sep);
> > >       else
> > > @@ -847,10 +847,6 @@ static void print_metric_headers(struct perf_stat_config *config,
> > >               .new_line = new_line_metric,
> > >               .force_header = true,
> > >       };
> > > -     bool first = true;
> > > -
> > > -     if (config->json_output && !config->interval)
> > > -             fprintf(config->output, "{");
> > >
> > >       if (prefix && !config->json_output)
> > >               fprintf(config->output, "%s", prefix);
> > > @@ -871,18 +867,12 @@ static void print_metric_headers(struct perf_stat_config *config,
> > >       evlist__for_each_entry(evlist, counter) {
> > >               os.evsel = counter;
> > >
> > > -             if (!first && config->json_output)
> > > -                     fprintf(config->output, ", ");
> > > -             first = false;
> > > -
> > >               perf_stat__print_shadow_stats(config, counter, 0,
> > >                                             0,
> > >                                             &out,
> > >                                             &config->metric_events,
> > >                                             &rt_stat);
> > >       }
> > > -     if (config->json_output)
> > > -             fprintf(config->output, "}");
> > >       fputc('\n', config->output);
> > >  }
> > >
> > > @@ -954,14 +944,8 @@ static void print_interval(struct perf_stat_config *config,
> > >               }
> > >       }
> > >
> > > -     if ((num_print_interval == 0 || config->interval_clear)
> > > -                      && metric_only && !config->json_output)
> > > +     if ((num_print_interval == 0 || config->interval_clear) && metric_only)
> > >               print_metric_headers(config, evlist, " ", true);
> > > -     if ((num_print_interval == 0 || config->interval_clear)
> > > -                      && metric_only && config->json_output) {
> > > -             fprintf(output, "{");
> > > -             print_metric_headers(config, evlist, " ", true);
> > > -     }
> > >       if (++num_print_interval == 25)
> > >               num_print_interval = 0;
> > >  }
> > > --
> > > 2.38.1.431.g37b22c650d-goog
> >
> > --
> >
> > - Arnaldo