LinuxLists.cc - [PATCH v4 5/9] perf annotate stdio: Support --show-nr-samples option

2017-07-27 16:16:27

Subject: [PATCH v4 5/9] perf annotate stdio: Support --show-nr-samples option

Add --show-nr-samples option to perf-annotate
so that it corresponds with perf-report.

Cc: Namhyung Kim <[email protected]>
Cc: Milian Wolff <[email protected]>
Cc: Jiri Olsa <[email protected]>
Signed-off-by: Taeung Song <[email protected]>
---
tools/perf/builtin-annotate.c | 2 ++
tools/perf/util/annotate.c | 6 +++++-
2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 6db782d..a8e6db2 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -447,6 +447,8 @@ int cmd_annotate(int argc, const char **argv)
"Show event group information together"),
OPT_BOOLEAN(0, "show-total-period", &symbol_conf.show_total_period,
"Show a column with the sum of periods"),
+ OPT_BOOLEAN('n', "show-nr-samples", &symbol_conf.show_nr_samples,
+ "Show a column with the number of samples"),
OPT_CALLBACK_DEFAULT(0, "stdio-color", NULL, "mode",
"'always' (default), 'never' or 'auto' only applicable to --stdio mode",
stdio__config_color, "always"),
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 5125c2b..7032bdc 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1144,6 +1144,9 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
if (symbol_conf.show_total_period)
color_fprintf(stdout, color, " %11" PRIu64,
sample.period);
+ else if (symbol_conf.show_nr_samples)
+ color_fprintf(stdout, color, " %7" PRIu64,
+ sample.nr_samples);
else
color_fprintf(stdout, color, " %7.2f", percent);
}
@@ -1824,7 +1827,8 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
width *= evsel->nr_members;

graph_dotted_len = printf(" %-*.*s| Source code & Disassembly of %s for %s (%" PRIu64 " samples)\n",
- width, width, symbol_conf.show_total_period ? "Event count" : "Percent",
+ width, width, symbol_conf.show_total_period ? "Event count" :
+ symbol_conf.show_nr_samples ? "Samples" : "Percent",
d_filename, evsel_name, h->nr_samples);

printf("%-*.*s----\n",
--
2.7.4

2017-07-28 16:26:22

by Arnaldo Carvalho de Melo

[permalink] [raw]

Subject: Re: [PATCH v4 5/9] perf annotate stdio: Support --show-nr-samples option

Em Fri, Jul 28, 2017 at 01:16:16AM +0900, Taeung Song escreveu:
> Add --show-nr-samples option to perf-annotate
> so that it corresponds with perf-report.

So this is not that intuitive, i.e. if one ask for:

perf annotate --show-total-period --show-nr-samples

then both should appear, no?

- Arnaldo

> Cc: Namhyung Kim <[email protected]>
> Cc: Milian Wolff <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Signed-off-by: Taeung Song <[email protected]>
> ---
> tools/perf/builtin-annotate.c | 2 ++
> tools/perf/util/annotate.c | 6 +++++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 6db782d..a8e6db2 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -447,6 +447,8 @@ int cmd_annotate(int argc, const char **argv)
> "Show event group information together"),
> OPT_BOOLEAN(0, "show-total-period", &symbol_conf.show_total_period,
> "Show a column with the sum of periods"),
> + OPT_BOOLEAN('n', "show-nr-samples", &symbol_conf.show_nr_samples,
> + "Show a column with the number of samples"),
> OPT_CALLBACK_DEFAULT(0, "stdio-color", NULL, "mode",
> "'always' (default), 'never' or 'auto' only applicable to --stdio mode",
> stdio__config_color, "always"),
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 5125c2b..7032bdc 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1144,6 +1144,9 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
> if (symbol_conf.show_total_period)
> color_fprintf(stdout, color, " %11" PRIu64,
> sample.period);
> + else if (symbol_conf.show_nr_samples)
> + color_fprintf(stdout, color, " %7" PRIu64,
> + sample.nr_samples);
> else
> color_fprintf(stdout, color, " %7.2f", percent);
> }
> @@ -1824,7 +1827,8 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
> width *= evsel->nr_members;
>
> graph_dotted_len = printf(" %-*.*s| Source code & Disassembly of %s for %s (%" PRIu64 " samples)\n",
> - width, width, symbol_conf.show_total_period ? "Event count" : "Percent",
> + width, width, symbol_conf.show_total_period ? "Event count" :
> + symbol_conf.show_nr_samples ? "Samples" : "Percent",
> d_filename, evsel_name, h->nr_samples);
>
> printf("%-*.*s----\n",
> --
> 2.7.4

2017-08-01 06:25:03

by Taeung Song

[permalink] [raw]

Subject: Re: [PATCH v4 5/9] perf annotate stdio: Support --show-nr-samples option

On 07/29/2017 01:26 AM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jul 28, 2017 at 01:16:16AM +0900, Taeung Song escreveu:
>> Add --show-nr-samples option to perf-annotate
>> so that it corresponds with perf-report.
>
> So this is not that intuitive, i.e. if one ask for:
>
> perf annotate --show-total-period --show-nr-samples
>
> then both should appear, no?
>
> - Arnaldo

I thought the users can use both --show-total-period and --show-nr-samples,
but perf-annotate can preferentially show the total period view IMHO.

What do you think about it ?
we need to prevent users from using both ?

Thanks,
Taeung

>
>> Cc: Namhyung Kim <[email protected]>
>> Cc: Milian Wolff <[email protected]>
>> Cc: Jiri Olsa <[email protected]>
>> Signed-off-by: Taeung Song <[email protected]>
>> ---
>> tools/perf/builtin-annotate.c | 2 ++
>> tools/perf/util/annotate.c | 6 +++++-
>> 2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
>> index 6db782d..a8e6db2 100644
>> --- a/tools/perf/builtin-annotate.c
>> +++ b/tools/perf/builtin-annotate.c
>> @@ -447,6 +447,8 @@ int cmd_annotate(int argc, const char **argv)
>> "Show event group information together"),
>> OPT_BOOLEAN(0, "show-total-period", &symbol_conf.show_total_period,
>> "Show a column with the sum of periods"),
>> + OPT_BOOLEAN('n', "show-nr-samples", &symbol_conf.show_nr_samples,
>> + "Show a column with the number of samples"),
>> OPT_CALLBACK_DEFAULT(0, "stdio-color", NULL, "mode",
>> "'always' (default), 'never' or 'auto' only applicable to --stdio mode",
>> stdio__config_color, "always"),
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index 5125c2b..7032bdc 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -1144,6 +1144,9 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>> if (symbol_conf.show_total_period)
>> color_fprintf(stdout, color, " %11" PRIu64,
>> sample.period);
>> + else if (symbol_conf.show_nr_samples)
>> + color_fprintf(stdout, color, " %7" PRIu64,
>> + sample.nr_samples);
>> else
>> color_fprintf(stdout, color, " %7.2f", percent);
>> }
>> @@ -1824,7 +1827,8 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
>> width *= evsel->nr_members;
>>
>> graph_dotted_len = printf(" %-*.*s| Source code & Disassembly of %s for %s (%" PRIu64 " samples)\n",
>> - width, width, symbol_conf.show_total_period ? "Event count" : "Percent",
>> + width, width, symbol_conf.show_total_period ? "Event count" :
>> + symbol_conf.show_nr_samples ? "Samples" : "Percent",
>> d_filename, evsel_name, h->nr_samples);
>>
>> printf("%-*.*s----\n",
>> --
>> 2.7.4

2017-08-01 06:26:24

by Taeung Song

[permalink] [raw]

Subject: Re: [PATCH v4 5/9] perf annotate stdio: Support --show-nr-samples option

2017-08-14 10:42:14

by Taeung Song

[permalink] [raw]

Subject: Re: [PATCH v4 5/9] perf annotate stdio: Support --show-nr-samples option

Hi Arnaldo,

On 08/01/2017 03:24 PM, Taeung Song wrote:
>
>
> On 07/29/2017 01:26 AM, Arnaldo Carvalho de Melo wrote:
>> Em Fri, Jul 28, 2017 at 01:16:16AM +0900, Taeung Song escreveu:
>>> Add --show-nr-samples option to perf-annotate
>>> so that it corresponds with perf-report.
>>
>> So this is not that intuitive, i.e. if one ask for:
>>
>> perf annotate --show-total-period --show-nr-samples
>>
>> then both should appear, no?
>>
>> - Arnaldo
>
> I thought the users can use both --show-total-period and --show-nr-samples,
> but perf-annotate can preferentially show the total period view IMHO.
>
> What do you think about it ?
> we need to prevent users from using both ?
>
> Thanks,
> Taeung

ping !

>
>>> Cc: Namhyung Kim <[email protected]>
>>> Cc: Milian Wolff <[email protected]>
>>> Cc: Jiri Olsa <[email protected]>
>>> Signed-off-by: Taeung Song <[email protected]>
>>> ---
>>> tools/perf/builtin-annotate.c | 2 ++
>>> tools/perf/util/annotate.c | 6 +++++-
>>> 2 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/perf/builtin-annotate.c
>>> b/tools/perf/builtin-annotate.c
>>> index 6db782d..a8e6db2 100644
>>> --- a/tools/perf/builtin-annotate.c
>>> +++ b/tools/perf/builtin-annotate.c
>>> @@ -447,6 +447,8 @@ int cmd_annotate(int argc, const char **argv)
>>> "Show event group information together"),
>>> OPT_BOOLEAN(0, "show-total-period",
>>> &symbol_conf.show_total_period,
>>> "Show a column with the sum of periods"),
>>> + OPT_BOOLEAN('n', "show-nr-samples", &symbol_conf.show_nr_samples,
>>> + "Show a column with the number of samples"),
>>> OPT_CALLBACK_DEFAULT(0, "stdio-color", NULL, "mode",
>>> "'always' (default), 'never' or 'auto' only
>>> applicable to --stdio mode",
>>> stdio__config_color, "always"),
>>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>>> index 5125c2b..7032bdc 100644
>>> --- a/tools/perf/util/annotate.c
>>> +++ b/tools/perf/util/annotate.c
>>> @@ -1144,6 +1144,9 @@ static int disasm_line__print(struct
>>> disasm_line *dl, struct symbol *sym, u64 st
>>> if (symbol_conf.show_total_period)
>>> color_fprintf(stdout, color, " %11" PRIu64,
>>> sample.period);
>>> + else if (symbol_conf.show_nr_samples)
>>> + color_fprintf(stdout, color, " %7" PRIu64,
>>> + sample.nr_samples);
>>> else
>>> color_fprintf(stdout, color, " %7.2f", percent);
>>> }
>>> @@ -1824,7 +1827,8 @@ int symbol__annotate_printf(struct symbol *sym,
>>> struct map *map,
>>> width *= evsel->nr_members;
>>> graph_dotted_len = printf(" %-*.*s| Source code &
>>> Disassembly of %s for %s (%" PRIu64 " samples)\n",
>>> - width, width, symbol_conf.show_total_period ?
>>> "Event count" : "Percent",
>>> + width, width, symbol_conf.show_total_period ?
>>> "Event count" :
>>> + symbol_conf.show_nr_samples ? "Samples" : "Percent",
>>> d_filename, evsel_name, h->nr_samples);
>>> printf("%-*.*s----\n",
>>> --
>>> 2.7.4

2017-08-14 16:31:56

by Arnaldo Carvalho de Melo

[permalink] [raw]

Subject: Re: [PATCH v4 5/9] perf annotate stdio: Support --show-nr-samples option

Em Mon, Aug 14, 2017 at 07:42:07PM +0900, Taeung Song escreveu:
> On 08/01/2017 03:24 PM, Taeung Song wrote:
> > On 07/29/2017 01:26 AM, Arnaldo Carvalho de Melo wrote:
> > > Em Fri, Jul 28, 2017 at 01:16:16AM +0900, Taeung Song escreveu:
> > > > Add --show-nr-samples option to perf-annotate
> > > > so that it corresponds with perf-report.
> > >
> > > So this is not that intuitive, i.e. if one ask for:
> > >
> > > perf annotate --show-total-period --show-nr-samples
> > >
> > > then both should appear, no?

> > I thought the users can use both --show-total-period and --show-nr-samples,
> > but perf-annotate can preferentially show the total period view IMHO.

That is inapropriate, if the user asks for both --show-total-period and
--show-nr-samples, then it either should show both or bail out telling
that that is not possible.

- Arnaldo

> > What do you think about it ?
> > we need to prevent users from using both ?
> >
> > Thanks,
> > Taeung
>
>
> ping !
>
> >
> > > > Cc: Namhyung Kim <[email protected]>
> > > > Cc: Milian Wolff <[email protected]>
> > > > Cc: Jiri Olsa <[email protected]>
> > > > Signed-off-by: Taeung Song <[email protected]>
> > > > ---
> > > > tools/perf/builtin-annotate.c | 2 ++
> > > > tools/perf/util/annotate.c | 6 +++++-
> > > > 2 files changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/perf/builtin-annotate.c
> > > > b/tools/perf/builtin-annotate.c
> > > > index 6db782d..a8e6db2 100644
> > > > --- a/tools/perf/builtin-annotate.c
> > > > +++ b/tools/perf/builtin-annotate.c
> > > > @@ -447,6 +447,8 @@ int cmd_annotate(int argc, const char **argv)
> > > > "Show event group information together"),
> > > > OPT_BOOLEAN(0, "show-total-period",
> > > > &symbol_conf.show_total_period,
> > > > "Show a column with the sum of periods"),
> > > > + OPT_BOOLEAN('n', "show-nr-samples", &symbol_conf.show_nr_samples,
> > > > + "Show a column with the number of samples"),
> > > > OPT_CALLBACK_DEFAULT(0, "stdio-color", NULL, "mode",
> > > > "'always' (default), 'never' or 'auto' only
> > > > applicable to --stdio mode",
> > > > stdio__config_color, "always"),
> > > > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > > > index 5125c2b..7032bdc 100644
> > > > --- a/tools/perf/util/annotate.c
> > > > +++ b/tools/perf/util/annotate.c
> > > > @@ -1144,6 +1144,9 @@ static int disasm_line__print(struct
> > > > disasm_line *dl, struct symbol *sym, u64 st
> > > > if (symbol_conf.show_total_period)
> > > > color_fprintf(stdout, color, " %11" PRIu64,
> > > > sample.period);
> > > > + else if (symbol_conf.show_nr_samples)
> > > > + color_fprintf(stdout, color, " %7" PRIu64,
> > > > + sample.nr_samples);
> > > > else
> > > > color_fprintf(stdout, color, " %7.2f", percent);
> > > > }
> > > > @@ -1824,7 +1827,8 @@ int symbol__annotate_printf(struct symbol
> > > > *sym, struct map *map,
> > > > width *= evsel->nr_members;
> > > > graph_dotted_len = printf(" %-*.*s| Source code &
> > > > Disassembly of %s for %s (%" PRIu64 " samples)\n",
> > > > - width, width, symbol_conf.show_total_period ?
> > > > "Event count" : "Percent",
> > > > + width, width, symbol_conf.show_total_period ?
> > > > "Event count" :
> > > > + symbol_conf.show_nr_samples ? "Samples" : "Percent",
> > > > d_filename, evsel_name, h->nr_samples);
> > > > printf("%-*.*s----\n",
> > > > --
> > > > 2.7.4

2017-08-14 16:40:06

by Taeung Song

[permalink] [raw]

Subject: Re: [PATCH v4 5/9] perf annotate stdio: Support --show-nr-samples option

On 08/15/2017 01:31 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Aug 14, 2017 at 07:42:07PM +0900, Taeung Song escreveu:
>> On 08/01/2017 03:24 PM, Taeung Song wrote:
>>> On 07/29/2017 01:26 AM, Arnaldo Carvalho de Melo wrote:
>>>> Em Fri, Jul 28, 2017 at 01:16:16AM +0900, Taeung Song escreveu:
>>>>> Add --show-nr-samples option to perf-annotate
>>>>> so that it corresponds with perf-report.
>>>>
>>>> So this is not that intuitive, i.e. if one ask for:
>>>>
>>>> perf annotate --show-total-period --show-nr-samples
>>>>
>>>> then both should appear, no?
>
>>> I thought the users can use both --show-total-period and --show-nr-samples,
>>> but perf-annotate can preferentially show the total period view IMHO.
>
> That is inapropriate, if the user asks for both --show-total-period and
> --show-nr-samples, then it either should show both or bail out telling
> that that is not possible.
>
> - Arnaldo
>

ok, got it, I'll send changed this patch based on your comment !

Thanks,
Taeung

>>> What do you think about it ?
>>> we need to prevent users from using both ?
>>>
>>> Thanks,
>>> Taeung
>>
>>
>> ping !
>>
>>>
>>>>> Cc: Namhyung Kim <[email protected]>
>>>>> Cc: Milian Wolff <[email protected]>
>>>>> Cc: Jiri Olsa <[email protected]>
>>>>> Signed-off-by: Taeung Song <[email protected]>
>>>>> ---
>>>>> tools/perf/builtin-annotate.c | 2 ++
>>>>> tools/perf/util/annotate.c | 6 +++++-
>>>>> 2 files changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/tools/perf/builtin-annotate.c
>>>>> b/tools/perf/builtin-annotate.c
>>>>> index 6db782d..a8e6db2 100644
>>>>> --- a/tools/perf/builtin-annotate.c
>>>>> +++ b/tools/perf/builtin-annotate.c
>>>>> @@ -447,6 +447,8 @@ int cmd_annotate(int argc, const char **argv)
>>>>> "Show event group information together"),
>>>>> OPT_BOOLEAN(0, "show-total-period",
>>>>> &symbol_conf.show_total_period,
>>>>> "Show a column with the sum of periods"),
>>>>> + OPT_BOOLEAN('n', "show-nr-samples", &symbol_conf.show_nr_samples,
>>>>> + "Show a column with the number of samples"),
>>>>> OPT_CALLBACK_DEFAULT(0, "stdio-color", NULL, "mode",
>>>>> "'always' (default), 'never' or 'auto' only
>>>>> applicable to --stdio mode",
>>>>> stdio__config_color, "always"),
>>>>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>>>>> index 5125c2b..7032bdc 100644
>>>>> --- a/tools/perf/util/annotate.c
>>>>> +++ b/tools/perf/util/annotate.c
>>>>> @@ -1144,6 +1144,9 @@ static int disasm_line__print(struct
>>>>> disasm_line *dl, struct symbol *sym, u64 st
>>>>> if (symbol_conf.show_total_period)
>>>>> color_fprintf(stdout, color, " %11" PRIu64,
>>>>> sample.period);
>>>>> + else if (symbol_conf.show_nr_samples)
>>>>> + color_fprintf(stdout, color, " %7" PRIu64,
>>>>> + sample.nr_samples);
>>>>> else
>>>>> color_fprintf(stdout, color, " %7.2f", percent);
>>>>> }
>>>>> @@ -1824,7 +1827,8 @@ int symbol__annotate_printf(struct symbol
>>>>> *sym, struct map *map,
>>>>> width *= evsel->nr_members;
>>>>> graph_dotted_len = printf(" %-*.*s| Source code &
>>>>> Disassembly of %s for %s (%" PRIu64 " samples)\n",
>>>>> - width, width, symbol_conf.show_total_period ?
>>>>> "Event count" : "Percent",
>>>>> + width, width, symbol_conf.show_total_period ?
>>>>> "Event count" :
>>>>> + symbol_conf.show_nr_samples ? "Samples" : "Percent",
>>>>> d_filename, evsel_name, h->nr_samples);
>>>>> printf("%-*.*s----\n",
>>>>> --
>>>>> 2.7.4