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'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.tae...@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.wo...@kdab.com> > Cc: Namhyung Kim <namhy...@kernel.org> > Cc: Jiri Olsa <jo...@redhat.com> > Signed-off-by: Taeung Song <treeze.tae...@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 > 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? > 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? > + > 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 > printf("%-*.*s----\n", > graph_dotted_len, graph_dotted_len, graph_dotted_line); > - Arnaldo