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 <[email protected]>
---
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
> 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
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 <[email protected]>
> ---
> 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
>
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
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
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
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
>
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
>>
>
> 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
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