Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

[Taeung Song <treeze.taeung@gmail.com>]

Hi Arnaldo,

On 07/25/2017 11:42 PM, Arnaldo Carvalho de Melo wrote:
> 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 got it. will separate this patch.

> 
> 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@gmail.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@kdab.com>
>>      Cc: Namhyung Kim <namhyung@kernel.org>
>>      Cc: Jiri Olsa <jolsa@redhat.com>
>>      Signed-off-by: Taeung Song <treeze.taeung@gmail.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
> 

ok.

>>                           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?
> 

Yep, if you check the above code lines, like below:

   color_fprintf(stdout, color, " %11" PRIu64,
                 sample.period);

The above number of letters is 12
i.e. 12 = 1 (" ": white space) + 11 (digits of sample.period)

So, I used '4', because the 'width' variable is initialized as '8'.

Additionally this patch handle the width for group event like below:

   $ perf annotate --show-total-period -i group_events.data --stdio
  Event count                         |  Source code & Disassembly of 
old for cycles (529 samples)
-----------------------------------------------------------------------------------------------------
                                      :
                                      :
                                      :
                                      :  Disassembly of section .text:
                                      :
                                      :  0000000000400816 
<get_cond_maxprice>:
                                      :  get_cond_maxprice():
            0           0        7144 :    400816:       push   %rbp
      3480988           0        5709 :    400817:       mov    %rsp,%rbp
            0           0        7522 :    40081a:       mov 
%edi,-0x24(%rbp)


Sorry, I repeatedly failed to adjust a proper patch unit..
I'll remake this patches based on your comment,
and resend next patchset !

>>                   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?
> 

ditto.

>> +
>>           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
> 

ok.


Thanks,
Taeung

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