Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751825AbdG0PPF (ORCPT <rfc822;w@1wt.eu>);
        Thu, 27 Jul 2017 11:15:05 -0400
Received: from mail-pf0-f193.google.com ([209.85.192.193]:34292 "EHLO
        mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751766AbdG0PPC (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 27 Jul 2017 11:15:02 -0400
Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period
 option showing number of samples
To: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: linux-kernel@vger.kernel.org, Namhyung Kim <namhyung@kernel.org>,
        Milian Wolff <milian.wolff@kdab.com>, Jiri Olsa <jolsa@redhat.com>
References: <20170720191934.GD4134@kernel.org>
 <51c3c55a-d43d-427f-53fb-56b3ab275dd9@gmail.com>
 <20170721112420.GE4134@kernel.org>
 <f8fa9e2d-f55f-512c-4637-eefc4dd646c9@gmail.com>
 <20170724173725.GT4134@kernel.org>
 <c3c902e7-95bc-16d4-366f-12eb034c5c8d@gmail.com>
 <20170725144252.GC11990@kernel.org>
 <f687b1ab-6bc2-e901-fcc0-19472199160d@gmail.com>
 <20170725161721.GA23846@kernel.org>
 <240f3e20-f76e-19e5-dcb4-4f36cbbbb999@gmail.com>
 <20170726201755.GA16224@kernel.org>
From: Taeung Song <treeze.taeung@gmail.com>
Message-ID: <0cec5900-8840-7814-b973-f8aaf06621fa@gmail.com>
Date: Fri, 28 Jul 2017 00:14:55 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <20170726201755.GA16224@kernel.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 8792
Lines: 231



On 07/27/2017 05:17 AM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jul 26, 2017 at 08:57:13PM +0900, Taeung Song escreveu:
>> 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:
> 
> Why not fix the initialization of width? I.e.:
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index c2b4b00166ed..cc0bf0c1489b 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1165,7 +1165,7 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>   	} else if (max_lines && printed >= max_lines)
>   		return 1;
>   	else {
> -		int width = 8;
> +		int width = symbol_conf.show_total_period ? 12 : 8;
>   
>   		if (queue)
>   			return -1;
> @@ -1806,7 +1806,7 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
>   	int printed = 2, queue_len = 0;
>   	int more = 0;
>   	u64 len;
> -	int width = 8;
> +	int width = symbol_conf.show_total_period ? 12 : 8;
>   	int graph_dotted_len;
>   
>   	filename = strdup(dso->long_name);
> 
> -----------------
> 
> the s/7/11/ case is ok, as it is always branching on
> symbol_conf.show_total_period.


Understood !! :)

Thanks,
Taeung

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