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

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

Hello Arnaldo :)

On 07/26/2017 01:17 AM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jul 26, 2017 at 12:53:28AM +0900, Taeung Song escreveu:
>> On 07/25/2017 11:42 PM, Arnaldo Carvalho de Melo wrote:
>>>> 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.
> 
> Ok, please continue your work from my perf/core branch that I just
> pushed, in it the latest patch is this one:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/commit/?h=perf/core&id=143e9656aec7c61b9b8e134da5abc5dfb6133cbf
> 
> Which is a chunk of what you done below. More comments below.
>   

Yes sir, :)
I fetched and checked it.

>>> 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:
> <SNIP>
>>>> +++ 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'.
> 
> Think that I am 7 years old :o) I'm still not understanding this
> logic...
> 

Humm.. first of all, we can check the 'width' variable in two function
disasm_line__print() and symbol__annotate_printf() like below:


1063 static int disasm_line__print(struct disasm_line *dl, struct symbol 
*sym, u64 start,
1064                       struct perf_evsel *evsel, u64 len, int 
min_pcnt, int printed,
1065                       int max_lines, struct disasm_line *queue)
1066 {

...
1167         else {
1168                 int width = 8;
1169
1170                 if (queue)
1171                         return -1;
1172
1173                 if (perf_evsel__is_group_event(evsel))
1174                         width *= evsel->nr_members;
1175
1176                 if (!*dl->line)
1177                         printf(" %*s:\n", width, " ");
1178                 else
1179                         printf(" %*s:   %s\n", width, " ", dl->line);


And,

1794 int symbol__annotate_printf(struct symbol *sym, struct map *map,
1795                             struct perf_evsel *evsel, bool full_paths,
1796                             int min_pcnt, int max_lines, int context)
1797 {
...

1809         int width = 8;
...
1823         if (perf_evsel__is_group_event(evsel))
1824                 width *= evsel->nr_members;
1825
1826         graph_dotted_len = printf(" %-*.*s|     Source code & 
Disassembly of %s for %s (%" PRIu64 " samples)\n",
1827                                   width, width, 
symbol_conf.show_total_period ? "Event count" : "Percent",
1828                                   d_filename, evsel_name, 
h->nr_samples);

As you can see, currently the 'width' variables are set as 8 letters
But I adjust the width as 12 letters for the first column " Event count"
and period value.

So I do witdh += 4 for 12 letters like below:

   $ perf annotate --stdio --show-total-period -i hex2u64
  Event count |  Source code & Disassembly of old for cycles:ppp (102 
samples)
---------------------------------------------------------------------------------
              :
              :
              :
              :  Disassembly of section .text:
              :
              :  0000000000400816 <get_cond_maxprice>:
              :  get_cond_maxprice():
      1950346 :    400816:       push   %rbp
       741848 :    400817:       mov    %rsp,%rbp

We don't need to adjust the 'width' for --show-total-period ?

>> 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 !
> 
> It is not a problem, you're making progress, thanks for taking into
> accoutn my comments.
> 
> The end result may be the same, but having a good patch granularity is
> fundamental for bisecting, also for maintainers to cherry-pick parts of
> your work that they agree on while making comments about parts that
> looks wrong or needing some more work.
>   

Thanks for your advice !!

   - Taeung

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