Message-ID: <20170725144252.GC11990@kernel.org>
Date:   Tue, 25 Jul 2017 11:42:52 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Taeung Song <treeze.taeung@...il.com>
Cc:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>,
        linux-kernel@...r.kernel.org, Namhyung Kim <namhyung@...nel.org>,
        Milian Wolff <milian.wolff@...b.com>,
        Jiri Olsa <jolsa@...hat.com>
Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period
 option showing number of samples

Em Tue, Jul 25, 2017 at 06:28:42AM +0900, Taeung Song escreveu:
> On 07/25/2017 02:37 AM, Arnaldo Carvalho de Melo wrote:
> > The one above, look further down, from 896bccd3cb8d to 0321d0281cbb,
> > there are some more missing, but the ones below should be ready for
> > sending to Ingo, which I plan to do today.
> 
> I didn't see them..
> Thank you for adjusted patchset.
> I checked them a while ago.
> 
> Would you add Reported-by: Namhyung Kim
> to the commit 0321d0281cbbb404ea73f9e1869ec8db42e8ddfd ?

Sure, will do.
 
> And we can handle other patches later,
> but how about additionally fixing the header
> on the annotate stdio browser ?
> (If you don't want to handle additional patches anymore today,
> I'll send them with next patchset.)
> 
>   "Percent" -> "Event Count"
> 
> Because I think it would be better to show the proper first column.

Of course, I was coming to that, trying to figure out if what was left
was just one or even more separate patches, see below

> Moreover there is the below case that is not aligned due to big period
> values.

So, that "moreover" means its not just one patch, but at least two, i.e.
when one selects show-total-period we better have more space for that
column, right?

I'll break the patch below accordingly.

And even then, there is one question left, see below

> perf annotate --stdio -i milian.data --show-total-period
>  Percent |      Source code & Disassembly of test for cycles:ppp (1442
> samples)
> -------------------------------------------------------------------------------
>          :
>          :
>          :
>          :      Disassembly of section .text:
> ...
>        0 :        40089d:       pxor   %xmm1,%xmm1
>  27288350 :       4008a1:       cvtsi2sd %rsi,%xmm1
>        0 :        4008a6:       pxor   %xmm5,%xmm5
> 
> 
> So, I made a patch like below:
> 
> commit 6b96b9947e83474bd6e6fd09f93c390536bb435b
> Author: Taeung Song <treeze.taeung@...il.com>
> Date:   Tue Jul 25 06:17:59 2017 +0900
> 
>     perf annotate: Show the proper header when using --show-total-period
> 
>     Currently a first column is only "Percent",
>     so fix it to show correct column name based on given options.
>     (e.g. if using --show-total-period, show "Event count" as a first
> column)
> 
>     Reported-by: Milian Wolff <milian.wolff@...b.com>
>     Cc: Namhyung Kim <namhyung@...nel.org>
>     Cc: Jiri Olsa <jolsa@...hat.com>
>     Signed-off-by: Taeung Song <treeze.taeung@...il.com>
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 004072f..0224595 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1142,7 +1142,7 @@ static int disasm_line__print(struct disasm_line *dl,
> struct symbol *sym, u64 st
>                          color = get_percent_color(percent);
> 
>                          if (symbol_conf.show_total_period)
> -                                color_fprintf(stdout, color, " %7" PRIu64,
> +                                color_fprintf(stdout, color, " %11" PRIu64,
>                                                sample.period);

this part will be in a separate patch, i.e. something like:

  [PATCH] Widen "Period" column when using --show-total-period

>                          else
>                                  color_fprintf(stdout, color, " %7.2f",
> percent);
> @@ -1173,6 +1173,10 @@ static int disasm_line__print(struct disasm_line *dl,
> struct symbol *sym, u64 st
>                  if (perf_evsel__is_group_event(evsel))
>                          width *= evsel->nr_members;
> 
> +                if (symbol_conf.show_total_period)
> +                        width += perf_evsel__is_group_event(evsel) ?
> +                                4 * evsel->nr_members : 4;
> +

But what about this one? What is that '4' for? Not obvious at first
sight, can you elaborate on the need for this specific one?

>                  if (!*dl->line)
>                          printf(" %*s:\n", width, " ");
>                  else
> @@ -1823,8 +1827,14 @@ int symbol__annotate_printf(struct symbol *sym,
> struct map *map,
>          if (perf_evsel__is_group_event(evsel))
>                  width *= evsel->nr_members;
> 
> +        if (symbol_conf.show_total_period)
> +                width += perf_evsel__is_group_event(evsel) ?
> +                        4 * evsel->nr_members : 4;

What about this one?

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

this one will be in a separate patch, with the title you chose:

   [PATCH] perf annotate: Show the proper header when using --show-total-period

>          printf("%-*.*s----\n",
>                 graph_dotted_len, graph_dotted_len, graph_dotted_line);
> 

- Arnaldo

Powered by blists - more mailing lists