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

From: Arnaldo Carvalho de Melo
Date: Wed Jul 26 2017 - 16:18:12 EST

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