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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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