Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples
From: Taeung Song
Date: Wed Jul 26 2017 - 07:57:55 EST
Hello Arnaldo :)
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:
$ 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
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