[PATCH] perf stat: Align CSV output for summary mode

[PATCH] perf stat: Align CSV output for summary mode

[Jin Yao]

perf-stat has supported the summary mode. But the summary
lines break the CSV output so it's hard for scripts to parse
the result.

Before:

  # perf stat -x, -I1000 --interval-count 1 --summary
       1.001323097,8013.48,msec,cpu-clock,8013483384,100.00,8.013,CPUs utilized
       1.001323097,270,,context-switches,8013513297,100.00,0.034,K/sec
       1.001323097,13,,cpu-migrations,8013530032,100.00,0.002,K/sec
       1.001323097,184,,page-faults,8013546992,100.00,0.023,K/sec
       1.001323097,20574191,,cycles,8013551506,100.00,0.003,GHz
       1.001323097,10562267,,instructions,8013564958,100.00,0.51,insn per cycle
       1.001323097,2019244,,branches,8013575673,100.00,0.252,M/sec
       1.001323097,106152,,branch-misses,8013585776,100.00,5.26,of all branches
  8013.48,msec,cpu-clock,8013483384,100.00,7.984,CPUs utilized
  270,,context-switches,8013513297,100.00,0.034,K/sec
  13,,cpu-migrations,8013530032,100.00,0.002,K/sec
  184,,page-faults,8013546992,100.00,0.023,K/sec
  20574191,,cycles,8013551506,100.00,0.003,GHz
  10562267,,instructions,8013564958,100.00,0.51,insn per cycle
  2019244,,branches,8013575673,100.00,0.252,M/sec
  106152,,branch-misses,8013585776,100.00,5.26,of all branches

The summary line loses the timestamp column, which breaks the
CVS output.

We add a column at the 'timestamp' position and it just says 'summary'
for the summary line.

After:

  # perf stat -x, -I1000 --interval-count 1 --summary
       1.001196053,8012.72,msec,cpu-clock,8012722903,100.00,8.013,CPUs utilized
       1.001196053,218,,context-switches,8012753271,100.00,0.027,K/sec
       1.001196053,9,,cpu-migrations,8012769767,100.00,0.001,K/sec
       1.001196053,0,,page-faults,8012786257,100.00,0.000,K/sec
       1.001196053,15004518,,cycles,8012790637,100.00,0.002,GHz
       1.001196053,7954691,,instructions,8012804027,100.00,0.53,insn per cycle
       1.001196053,1590259,,branches,8012814766,100.00,0.198,M/sec
       1.001196053,82601,,branch-misses,8012824365,100.00,5.19,of all branches
           summary,8012.72,msec,cpu-clock,8012722903,100.00,7.986,CPUs utilized
           summary,218,,context-switches,8012753271,100.00,0.027,K/sec
           summary,9,,cpu-migrations,8012769767,100.00,0.001,K/sec
           summary,0,,page-faults,8012786257,100.00,0.000,K/sec
           summary,15004518,,cycles,8012790637,100.00,0.002,GHz
           summary,7954691,,instructions,8012804027,100.00,0.53,insn per cycle
           summary,1590259,,branches,8012814766,100.00,0.198,M/sec
           summary,82601,,branch-misses,8012824365,100.00,5.19,of all branches

Now it's easy for script to analyse the summary lines.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/util/stat-display.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 7f09cdaf5b60..c4183d3e87a4 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -439,6 +439,10 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 		if (counter->cgrp)
 			os.nfields++;
 	}
+
+	if (config->csv_output && config->summary && !config->interval)
+		fprintf(config->output, "%16s%s", "summary", config->csv_sep);
+
 	if (run == 0 || ena == 0 || counter->counts->scaled == -1) {
 		if (config->metric_only) {
 			pm(config, &os, NULL, "", "", 0);
-- 
2.17.1

Re: [PATCH] perf stat: Align CSV output for summary mode

[Jiri Olsa]

On Tue, Mar 16, 2021 at 03:29:00PM +0800, Jin Yao wrote:
> perf-stat has supported the summary mode. But the summary
> lines break the CSV output so it's hard for scripts to parse
> the result.
> 
> Before:
> 
>   # perf stat -x, -I1000 --interval-count 1 --summary
>        1.001323097,8013.48,msec,cpu-clock,8013483384,100.00,8.013,CPUs utilized
>        1.001323097,270,,context-switches,8013513297,100.00,0.034,K/sec
>        1.001323097,13,,cpu-migrations,8013530032,100.00,0.002,K/sec
>        1.001323097,184,,page-faults,8013546992,100.00,0.023,K/sec
>        1.001323097,20574191,,cycles,8013551506,100.00,0.003,GHz
>        1.001323097,10562267,,instructions,8013564958,100.00,0.51,insn per cycle
>        1.001323097,2019244,,branches,8013575673,100.00,0.252,M/sec
>        1.001323097,106152,,branch-misses,8013585776,100.00,5.26,of all branches
>   8013.48,msec,cpu-clock,8013483384,100.00,7.984,CPUs utilized
>   270,,context-switches,8013513297,100.00,0.034,K/sec
>   13,,cpu-migrations,8013530032,100.00,0.002,K/sec
>   184,,page-faults,8013546992,100.00,0.023,K/sec
>   20574191,,cycles,8013551506,100.00,0.003,GHz
>   10562267,,instructions,8013564958,100.00,0.51,insn per cycle
>   2019244,,branches,8013575673,100.00,0.252,M/sec
>   106152,,branch-misses,8013585776,100.00,5.26,of all branches
> 
> The summary line loses the timestamp column, which breaks the
> CVS output.
> 
> We add a column at the 'timestamp' position and it just says 'summary'
> for the summary line.
> 
> After:
> 
>   # perf stat -x, -I1000 --interval-count 1 --summary

looks ok, but maybe make the option more related to CVS, like:

  --x-summary, --cvs-summary  ...? 

jirka




>        1.001196053,8012.72,msec,cpu-clock,8012722903,100.00,8.013,CPUs utilized
>        1.001196053,218,,context-switches,8012753271,100.00,0.027,K/sec
>        1.001196053,9,,cpu-migrations,8012769767,100.00,0.001,K/sec
>        1.001196053,0,,page-faults,8012786257,100.00,0.000,K/sec
>        1.001196053,15004518,,cycles,8012790637,100.00,0.002,GHz
>        1.001196053,7954691,,instructions,8012804027,100.00,0.53,insn per cycle
>        1.001196053,1590259,,branches,8012814766,100.00,0.198,M/sec
>        1.001196053,82601,,branch-misses,8012824365,100.00,5.19,of all branches
>            summary,8012.72,msec,cpu-clock,8012722903,100.00,7.986,CPUs utilized
>            summary,218,,context-switches,8012753271,100.00,0.027,K/sec
>            summary,9,,cpu-migrations,8012769767,100.00,0.001,K/sec
>            summary,0,,page-faults,8012786257,100.00,0.000,K/sec
>            summary,15004518,,cycles,8012790637,100.00,0.002,GHz
>            summary,7954691,,instructions,8012804027,100.00,0.53,insn per cycle
>            summary,1590259,,branches,8012814766,100.00,0.198,M/sec
>            summary,82601,,branch-misses,8012824365,100.00,5.19,of all branches
> 
> Now it's easy for script to analyse the summary lines.
> 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
>  tools/perf/util/stat-display.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 7f09cdaf5b60..c4183d3e87a4 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -439,6 +439,10 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
>  		if (counter->cgrp)
>  			os.nfields++;
>  	}
> +
> +	if (config->csv_output && config->summary && !config->interval)
> +		fprintf(config->output, "%16s%s", "summary", config->csv_sep);
> +
>  	if (run == 0 || ena == 0 || counter->counts->scaled == -1) {
>  		if (config->metric_only) {
>  			pm(config, &os, NULL, "", "", 0);
> -- 
> 2.17.1
> 

Re: [PATCH] perf stat: Align CSV output for summary mode

[Jiri Olsa]

On Tue, Mar 16, 2021 at 02:07:12PM +0100, Jiri Olsa wrote:
> On Tue, Mar 16, 2021 at 03:29:00PM +0800, Jin Yao wrote:
> > perf-stat has supported the summary mode. But the summary
> > lines break the CSV output so it's hard for scripts to parse
> > the result.
> > 
> > Before:
> > 
> >   # perf stat -x, -I1000 --interval-count 1 --summary
> >        1.001323097,8013.48,msec,cpu-clock,8013483384,100.00,8.013,CPUs utilized
> >        1.001323097,270,,context-switches,8013513297,100.00,0.034,K/sec
> >        1.001323097,13,,cpu-migrations,8013530032,100.00,0.002,K/sec
> >        1.001323097,184,,page-faults,8013546992,100.00,0.023,K/sec
> >        1.001323097,20574191,,cycles,8013551506,100.00,0.003,GHz
> >        1.001323097,10562267,,instructions,8013564958,100.00,0.51,insn per cycle
> >        1.001323097,2019244,,branches,8013575673,100.00,0.252,M/sec
> >        1.001323097,106152,,branch-misses,8013585776,100.00,5.26,of all branches
> >   8013.48,msec,cpu-clock,8013483384,100.00,7.984,CPUs utilized
> >   270,,context-switches,8013513297,100.00,0.034,K/sec
> >   13,,cpu-migrations,8013530032,100.00,0.002,K/sec
> >   184,,page-faults,8013546992,100.00,0.023,K/sec
> >   20574191,,cycles,8013551506,100.00,0.003,GHz
> >   10562267,,instructions,8013564958,100.00,0.51,insn per cycle
> >   2019244,,branches,8013575673,100.00,0.252,M/sec
> >   106152,,branch-misses,8013585776,100.00,5.26,of all branches
> > 
> > The summary line loses the timestamp column, which breaks the
> > CVS output.
> > 
> > We add a column at the 'timestamp' position and it just says 'summary'
> > for the summary line.
> > 
> > After:
> > 
> >   # perf stat -x, -I1000 --interval-count 1 --summary
> 
> looks ok, but maybe make the option more related to CVS, like:
> 
>   --x-summary, --cvs-summary  ...? 
> 

and we'll need man page update for that

jirka

Re: [PATCH] perf stat: Align CSV output for summary mode

[Andi Kleen]

> looks ok, but maybe make the option more related to CVS, like:
> 
>   --x-summary, --cvs-summary  ...? 

Actually I don't think it should be a new option. I doubt
anyone could parse the previous mess. So just make it default
with -x

-Andi

Re: [PATCH] perf stat: Align CSV output for summary mode

[Arnaldo Carvalho de Melo]

Em Tue, Mar 16, 2021 at 09:34:21AM -0700, Andi Kleen escreveu:
> > looks ok, but maybe make the option more related to CVS, like:
> > 
> >   --x-summary, --cvs-summary  ...? 
> 
> Actually I don't think it should be a new option. I doubt
> anyone could parse the previous mess. So just make it default
> with -x

In these cases I always fear that people are already parsing that mess
by considering the summary lines to be the ones not starting with
spaces, and now we go on and change it to be "better" by prefixing it
with "summary" and... break existing scripts.

Can we do this with a new option?

I.e. like --cvs-summary?

- Arnaldo

Re: [PATCH] perf stat: Align CSV output for summary mode

[Andi Kleen]

On Tue, Mar 16, 2021 at 04:05:13PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Mar 16, 2021 at 09:34:21AM -0700, Andi Kleen escreveu:
> > > looks ok, but maybe make the option more related to CVS, like:
> > > 
> > >   --x-summary, --cvs-summary  ...? 
> > 
> > Actually I don't think it should be a new option. I doubt
> > anyone could parse the previous mess. So just make it default
> > with -x
> 
> In these cases I always fear that people are already parsing that mess
> by considering the summary lines to be the ones not starting with
> spaces, and now we go on and change it to be "better" by prefixing it
> with "summary" and... break existing scripts.

I think it was just one version or so?

FWIW perf has broken CSV output several times, I added workarounds
to toplev every time. Having a broken version for a short time
shouldn't be too bad.

I actually had a workaround for this one, but it can parse either way.

> 
> Can we do this with a new option?
> 
> I.e. like --cvs-summary?

If you do it I would add an option for the old broken format
--i-want-broken-csv. But not  require the option forever
just to get sane output.

Or maybe only a perf config option.

-Andi

Re: [PATCH] perf stat: Align CSV output for summary mode

[Jiri Olsa]

On Tue, Mar 16, 2021 at 01:02:20PM -0700, Andi Kleen wrote:
> On Tue, Mar 16, 2021 at 04:05:13PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Mar 16, 2021 at 09:34:21AM -0700, Andi Kleen escreveu:
> > > > looks ok, but maybe make the option more related to CVS, like:
> > > > 
> > > >   --x-summary, --cvs-summary  ...? 
> > > 
> > > Actually I don't think it should be a new option. I doubt
> > > anyone could parse the previous mess. So just make it default
> > > with -x
> > 
> > In these cases I always fear that people are already parsing that mess
> > by considering the summary lines to be the ones not starting with
> > spaces, and now we go on and change it to be "better" by prefixing it
> > with "summary" and... break existing scripts.
> 
> I think it was just one version or so?
> 
> FWIW perf has broken CSV output several times, I added workarounds
> to toplev every time. Having a broken version for a short time
> shouldn't be too bad.
> 
> I actually had a workaround for this one, but it can parse either way.
> 
> > 
> > Can we do this with a new option?
> > 
> > I.e. like --cvs-summary?
> 
> If you do it I would add an option for the old broken format
> --i-want-broken-csv. But not  require the option forever
> just to get sane output.

I like that.. also we'll find out how many people are actually parsing that ;-)

jirka

> 
> Or maybe only a perf config option.
> 
> -Andi
> 

Re: [PATCH] perf stat: Align CSV output for summary mode

[Jin, Yao]

On 3/17/2021 5:55 AM, Jiri Olsa wrote:
> On Tue, Mar 16, 2021 at 01:02:20PM -0700, Andi Kleen wrote:
>> On Tue, Mar 16, 2021 at 04:05:13PM -0300, Arnaldo Carvalho de Melo wrote:
>>> Em Tue, Mar 16, 2021 at 09:34:21AM -0700, Andi Kleen escreveu:
>>>>> looks ok, but maybe make the option more related to CVS, like:
>>>>>
>>>>>    --x-summary, --cvs-summary  ...?
>>>>
>>>> Actually I don't think it should be a new option. I doubt
>>>> anyone could parse the previous mess. So just make it default
>>>> with -x
>>>
>>> In these cases I always fear that people are already parsing that mess
>>> by considering the summary lines to be the ones not starting with
>>> spaces, and now we go on and change it to be "better" by prefixing it
>>> with "summary" and... break existing scripts.
>>
>> I think it was just one version or so?
>>
>> FWIW perf has broken CSV output several times, I added workarounds
>> to toplev every time. Having a broken version for a short time
>> shouldn't be too bad.
>>
>> I actually had a workaround for this one, but it can parse either way.
>>
>>>
>>> Can we do this with a new option?
>>>
>>> I.e. like --cvs-summary?
>>
>> If you do it I would add an option for the old broken format
>> --i-want-broken-csv. But not  require the option forever
>> just to get sane output.
> 
> I like that.. also we'll find out how many people are actually parsing that ;-)
> 
> jirka
> 

Is it serious or just a joke? :)

Thanks
Jin Yao

>>
>> Or maybe only a perf config option.
>>
>> -Andi
>>
> 

Re: [PATCH] perf stat: Align CSV output for summary mode

[Andi Kleen]

> Is it serious or just a joke? :)

I would prefer to not be compatible (at least not until someone complains),
but if compatibility is required then yes opting in to the broken
format would be better. Perhaps not with that name.

And the option could be hidden in the perf config file instead
of being on the command line.

-Andi

Re: [PATCH] perf stat: Align CSV output for summary mode

[Jin, Yao]

On 3/17/2021 9:30 AM, Andi Kleen wrote:
>> Is it serious or just a joke? :)
> 
> I would prefer to not be compatible (at least not until someone complains),
> but if compatibility is required then yes opting in to the broken
> format would be better. Perhaps not with that name.
> 
> And the option could be hidden in the perf config file instead
> of being on the command line.
> 
> -Andi
> 

That makes sense, thanks Andi!

Thanks
Jin Yao