Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples
From: Taeung Song
Date: Tue Jul 25 2017 - 11:53:38 EST
Hi Arnaldo,
On 07/25/2017 11:42 PM, Arnaldo Carvalho de Melo wrote:
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 got it. will separate this patch.
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.taeung@xxxxxxxxx>
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.wolff@xxxxxxxx>
Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
Signed-off-by: Taeung Song <treeze.taeung@xxxxxxxxx>
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
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'.
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 !
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