* [PATCH] perf annotate: improve --stdio mode
@ 2021-02-21 12:46 Martin Liška
2021-02-23 19:47 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 10+ messages in thread
From: Martin Liška @ 2021-02-21 12:46 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-perf-users, Arnaldo Carvalho de Melo
The patch changes the output format in 2 ways:
- line number is displayed for all source lines (matching TUI mode)
- source locations for the hottest lines are printed
at the line end in order to preserve layout
Before:
0.00 : 405ef1: inc %r15
: tmpsd * (TD + tmpsd * TDD)));
0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
: tmpsd * (TC +
eff.c:1811 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8>
: TA + tmpsd * (TB +
0.35 : 405f06: vfmadd213sd 0x2b9b1(%rip),%xmm0,%xmm3 # 4318c0 <_IO_stdin_used+0x8c0>
: dumbo =
eff.c:1809 1.41 : 405f0f: vfmadd213sd 0x2b9b0(%rip),%xmm0,%xmm3 # 4318c8 <_IO_stdin_used+0x8c8>
: sumi -= sj * tmpsd * dij2i * dumbo;
eff.c:1813 2.58 : 405f18: vmulsd %xmm3,%xmm0,%xmm0
2.81 : 405f1c: vfnmadd213sd 0x30(%rsp),%xmm1,%xmm0
3.78 : 405f23: vmovsd %xmm0,0x30(%rsp)
: for (k = 0; k < lpears[i] + upears[i]; k++) {
eff.c:1761 0.90 : 405f29: cmp %r15d,%r12d
After:
0.00 : 405ef1: inc %r15
: 1812 tmpsd * (TD + tmpsd * TDD)));
0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
: 1811 tmpsd * (TC +
0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8> // eff.c:1811
: 1810 TA + tmpsd * (TB +
0.35 : 405f06: vfmadd213sd 0x2b9b1(%rip),%xmm0,%xmm3 # 4318c0 <_IO_stdin_used+0x8c0>
: 1809 dumbo =
1.41 : 405f0f: vfmadd213sd 0x2b9b0(%rip),%xmm0,%xmm3 # 4318c8 <_IO_stdin_used+0x8c8> // eff.c:1809
: 1813 sumi -= sj * tmpsd * dij2i * dumbo;
2.58 : 405f18: vmulsd %xmm3,%xmm0,%xmm0 // eff.c:1813
2.81 : 405f1c: vfnmadd213sd 0x30(%rsp),%xmm1,%xmm0
3.78 : 405f23: vmovsd %xmm0,0x30(%rsp)
: 1761 for (k = 0; k < lpears[i] + upears[i]; k++) {
Where e.g. '// eff.c:1811' shares the same color as the percentantage
at the line beginning.
Signed-off-by: Martin Liška <mliska@suse.cz>
---
tools/perf/util/annotate.c | 30 ++++++++++++++----------------
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index e60841b86d27..80542012ec1b 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1366,7 +1366,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
{
struct disasm_line *dl = container_of(al, struct disasm_line, al);
static const char *prev_line;
- static const char *prev_color;
if (al->offset != -1) {
double max_percent = 0.0;
@@ -1405,20 +1404,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
color = get_percent_color(max_percent);
- /*
- * Also color the filename and line if needed, with
- * the same color than the percentage. Don't print it
- * twice for close colored addr with the same filename:line
- */
- if (al->path) {
- if (!prev_line || strcmp(prev_line, al->path)
- || color != prev_color) {
- color_fprintf(stdout, color, " %s", al->path);
- prev_line = al->path;
- prev_color = color;
- }
- }
-
for (i = 0; i < nr_percent; i++) {
struct annotation_data *data = &al->data[i];
double percent;
@@ -1439,6 +1424,19 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
printf(" : ");
disasm_line__print(dl, start, addr_fmt_width);
+
+ /*
+ * Also color the filename and line if needed, with
+ * the same color than the percentage. Don't print it
+ * twice for close colored addr with the same filename:line
+ */
+ if (al->path) {
+ if (!prev_line || strcmp(prev_line, al->path)) {
+ color_fprintf(stdout, color, " // %s", al->path);
+ prev_line = al->path;
+ }
+ }
+
printf("\n");
} else if (max_lines && printed >= max_lines)
return 1;
@@ -1454,7 +1452,7 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
if (!*al->line)
printf(" %*s:\n", width, " ");
else
- printf(" %*s: %*s %s\n", width, " ", addr_fmt_width, " ", al->line);
+ printf(" %*s: %-*d %s\n", width, " ", addr_fmt_width, al->line_nr, al->line);
}
return 0;
--
2.30.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: improve --stdio mode
2021-02-21 12:46 [PATCH] perf annotate: improve --stdio mode Martin Liška
@ 2021-02-23 19:47 ` Arnaldo Carvalho de Melo
2021-02-26 9:24 ` Martin Liška
0 siblings, 1 reply; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-23 19:47 UTC (permalink / raw)
To: Martin Liška; +Cc: linux-kernel, linux-perf-users
Em Sun, Feb 21, 2021 at 01:46:36PM +0100, Martin Liška escreveu:
> The patch changes the output format in 2 ways:
> - line number is displayed for all source lines (matching TUI mode)
Are you aware of 'perf annotate --stdio2' ? If the goal is to make the
stdio mode better, doing it in that mode would be best, as it was done
to share as much code as possible, not just the looks, with the TUI
mode.
I kept --stdio around because changing the output in that way could
annoy people used to that format.
Please take a look at 'man perf-config' and see what can be configured
for both 'perf annotate --tui' and 'perf annotate --stdio2'.
Perhaps we can do something like:
perf config annotate.stdio=tui_like
And, for completeness have:
perf config annotate.stdio=classical
wdyt?
Looking at the other patches now.
- Arnaldo
> - source locations for the hottest lines are printed
> at the line end in order to preserve layout
>
> Before:
>
> 0.00 : 405ef1: inc %r15
> : tmpsd * (TD + tmpsd * TDD)));
> 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
> : tmpsd * (TC +
> eff.c:1811 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8>
> : TA + tmpsd * (TB +
> 0.35 : 405f06: vfmadd213sd 0x2b9b1(%rip),%xmm0,%xmm3 # 4318c0 <_IO_stdin_used+0x8c0>
> : dumbo =
> eff.c:1809 1.41 : 405f0f: vfmadd213sd 0x2b9b0(%rip),%xmm0,%xmm3 # 4318c8 <_IO_stdin_used+0x8c8>
> : sumi -= sj * tmpsd * dij2i * dumbo;
> eff.c:1813 2.58 : 405f18: vmulsd %xmm3,%xmm0,%xmm0
> 2.81 : 405f1c: vfnmadd213sd 0x30(%rsp),%xmm1,%xmm0
> 3.78 : 405f23: vmovsd %xmm0,0x30(%rsp)
> : for (k = 0; k < lpears[i] + upears[i]; k++) {
> eff.c:1761 0.90 : 405f29: cmp %r15d,%r12d
>
> After:
>
> 0.00 : 405ef1: inc %r15
> : 1812 tmpsd * (TD + tmpsd * TDD)));
> 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
> : 1811 tmpsd * (TC +
> 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8> // eff.c:1811
> : 1810 TA + tmpsd * (TB +
> 0.35 : 405f06: vfmadd213sd 0x2b9b1(%rip),%xmm0,%xmm3 # 4318c0 <_IO_stdin_used+0x8c0>
> : 1809 dumbo =
> 1.41 : 405f0f: vfmadd213sd 0x2b9b0(%rip),%xmm0,%xmm3 # 4318c8 <_IO_stdin_used+0x8c8> // eff.c:1809
> : 1813 sumi -= sj * tmpsd * dij2i * dumbo;
> 2.58 : 405f18: vmulsd %xmm3,%xmm0,%xmm0 // eff.c:1813
> 2.81 : 405f1c: vfnmadd213sd 0x30(%rsp),%xmm1,%xmm0
> 3.78 : 405f23: vmovsd %xmm0,0x30(%rsp)
> : 1761 for (k = 0; k < lpears[i] + upears[i]; k++) {
>
> Where e.g. '// eff.c:1811' shares the same color as the percentantage
> at the line beginning.
>
> Signed-off-by: Martin Liška <mliska@suse.cz>
> ---
> tools/perf/util/annotate.c | 30 ++++++++++++++----------------
> 1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index e60841b86d27..80542012ec1b 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1366,7 +1366,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> {
> struct disasm_line *dl = container_of(al, struct disasm_line, al);
> static const char *prev_line;
> - static const char *prev_color;
> if (al->offset != -1) {
> double max_percent = 0.0;
> @@ -1405,20 +1404,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> color = get_percent_color(max_percent);
> - /*
> - * Also color the filename and line if needed, with
> - * the same color than the percentage. Don't print it
> - * twice for close colored addr with the same filename:line
> - */
> - if (al->path) {
> - if (!prev_line || strcmp(prev_line, al->path)
> - || color != prev_color) {
> - color_fprintf(stdout, color, " %s", al->path);
> - prev_line = al->path;
> - prev_color = color;
> - }
> - }
> -
> for (i = 0; i < nr_percent; i++) {
> struct annotation_data *data = &al->data[i];
> double percent;
> @@ -1439,6 +1424,19 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> printf(" : ");
> disasm_line__print(dl, start, addr_fmt_width);
> +
> + /*
> + * Also color the filename and line if needed, with
> + * the same color than the percentage. Don't print it
> + * twice for close colored addr with the same filename:line
> + */
> + if (al->path) {
> + if (!prev_line || strcmp(prev_line, al->path)) {
> + color_fprintf(stdout, color, " // %s", al->path);
> + prev_line = al->path;
> + }
> + }
> +
> printf("\n");
> } else if (max_lines && printed >= max_lines)
> return 1;
> @@ -1454,7 +1452,7 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> if (!*al->line)
> printf(" %*s:\n", width, " ");
> else
> - printf(" %*s: %*s %s\n", width, " ", addr_fmt_width, " ", al->line);
> + printf(" %*s: %-*d %s\n", width, " ", addr_fmt_width, al->line_nr, al->line);
> }
> return 0;
> --
> 2.30.1
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: improve --stdio mode
2021-02-23 19:47 ` Arnaldo Carvalho de Melo
@ 2021-02-26 9:24 ` Martin Liška
2021-03-19 9:38 ` Martin Liška
2021-04-07 19:30 ` Arnaldo Carvalho de Melo
0 siblings, 2 replies; 10+ messages in thread
From: Martin Liška @ 2021-02-26 9:24 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: linux-kernel, linux-perf-users
On 2/23/21 8:47 PM, Arnaldo Carvalho de Melo wrote:
> Em Sun, Feb 21, 2021 at 01:46:36PM +0100, Martin Liška escreveu:
>> The patch changes the output format in 2 ways:
>> - line number is displayed for all source lines (matching TUI mode)
>
> Are you aware of 'perf annotate --stdio2' ? If the goal is to make the
> stdio mode better, doing it in that mode would be best, as it was done
> to share as much code as possible, not just the looks, with the TUI
> mode.
Yes, I'm aware of it. My motivation is to generate a HTML perf annotate report
and I see the following parts of --stdio2 not ideal:
- coloring is not available (--stdio-color=always does not work)
- 'Sorted summary for file ' is missing so one can't easily search for
hot spots in browser
- source line number are displayed, but not the source files
- there's a missing option for 'Toggle disassembler output/simplified view' which
is available in TUI mode
That said, the stdio2 annotation report is quite different and so handy for my use case.
>
> I kept --stdio around because changing the output in that way could
> annoy people used to that format.
Sure. But I think the current format provides quite broken visual layout:
0.00 : 405ef1: inc %r15
0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
eff.c:1811 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8>
: TA + tmpsd * (TB +
vs.
0.00 : 405ef1: inc %r15
0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8> // eff.c:1811
: 1810 TA + tmpsd * (TB +
I bet also the current users of --stdio mode would benefit from it.
What do you think?
Thanks,
Martin
>
> Please take a look at 'man perf-config' and see what can be configured
> for both 'perf annotate --tui' and 'perf annotate --stdio2'.
>
> Perhaps we can do something like:
>
> perf config annotate.stdio=tui_like
>
> And, for completeness have:
>
> perf config annotate.stdio=classical
>
> wdyt?
>
> Looking at the other patches now.
>
> - Arnaldo
>
>> - source locations for the hottest lines are printed
>> at the line end in order to preserve layout
>>
>> Before:
>>
>> 0.00 : 405ef1: inc %r15
>> : tmpsd * (TD + tmpsd * TDD)));
>> 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
>> : tmpsd * (TC +
>> eff.c:1811 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8>
>> : TA + tmpsd * (TB +
>> 0.35 : 405f06: vfmadd213sd 0x2b9b1(%rip),%xmm0,%xmm3 # 4318c0 <_IO_stdin_used+0x8c0>
>> : dumbo =
>> eff.c:1809 1.41 : 405f0f: vfmadd213sd 0x2b9b0(%rip),%xmm0,%xmm3 # 4318c8 <_IO_stdin_used+0x8c8>
>> : sumi -= sj * tmpsd * dij2i * dumbo;
>> eff.c:1813 2.58 : 405f18: vmulsd %xmm3,%xmm0,%xmm0
>> 2.81 : 405f1c: vfnmadd213sd 0x30(%rsp),%xmm1,%xmm0
>> 3.78 : 405f23: vmovsd %xmm0,0x30(%rsp)
>> : for (k = 0; k < lpears[i] + upears[i]; k++) {
>> eff.c:1761 0.90 : 405f29: cmp %r15d,%r12d
>>
>> After:
>>
>> 0.00 : 405ef1: inc %r15
>> : 1812 tmpsd * (TD + tmpsd * TDD)));
>> 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
>> : 1811 tmpsd * (TC +
>> 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8> // eff.c:1811
>> : 1810 TA + tmpsd * (TB +
>> 0.35 : 405f06: vfmadd213sd 0x2b9b1(%rip),%xmm0,%xmm3 # 4318c0 <_IO_stdin_used+0x8c0>
>> : 1809 dumbo =
>> 1.41 : 405f0f: vfmadd213sd 0x2b9b0(%rip),%xmm0,%xmm3 # 4318c8 <_IO_stdin_used+0x8c8> // eff.c:1809
>> : 1813 sumi -= sj * tmpsd * dij2i * dumbo;
>> 2.58 : 405f18: vmulsd %xmm3,%xmm0,%xmm0 // eff.c:1813
>> 2.81 : 405f1c: vfnmadd213sd 0x30(%rsp),%xmm1,%xmm0
>> 3.78 : 405f23: vmovsd %xmm0,0x30(%rsp)
>> : 1761 for (k = 0; k < lpears[i] + upears[i]; k++) {
>>
>> Where e.g. '// eff.c:1811' shares the same color as the percentantage
>> at the line beginning.
>>
>> Signed-off-by: Martin Liška <mliska@suse.cz>
>> ---
>> tools/perf/util/annotate.c | 30 ++++++++++++++----------------
>> 1 file changed, 14 insertions(+), 16 deletions(-)
>>
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index e60841b86d27..80542012ec1b 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -1366,7 +1366,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>> {
>> struct disasm_line *dl = container_of(al, struct disasm_line, al);
>> static const char *prev_line;
>> - static const char *prev_color;
>> if (al->offset != -1) {
>> double max_percent = 0.0;
>> @@ -1405,20 +1404,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>> color = get_percent_color(max_percent);
>> - /*
>> - * Also color the filename and line if needed, with
>> - * the same color than the percentage. Don't print it
>> - * twice for close colored addr with the same filename:line
>> - */
>> - if (al->path) {
>> - if (!prev_line || strcmp(prev_line, al->path)
>> - || color != prev_color) {
>> - color_fprintf(stdout, color, " %s", al->path);
>> - prev_line = al->path;
>> - prev_color = color;
>> - }
>> - }
>> -
>> for (i = 0; i < nr_percent; i++) {
>> struct annotation_data *data = &al->data[i];
>> double percent;
>> @@ -1439,6 +1424,19 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>> printf(" : ");
>> disasm_line__print(dl, start, addr_fmt_width);
>> +
>> + /*
>> + * Also color the filename and line if needed, with
>> + * the same color than the percentage. Don't print it
>> + * twice for close colored addr with the same filename:line
>> + */
>> + if (al->path) {
>> + if (!prev_line || strcmp(prev_line, al->path)) {
>> + color_fprintf(stdout, color, " // %s", al->path);
>> + prev_line = al->path;
>> + }
>> + }
>> +
>> printf("\n");
>> } else if (max_lines && printed >= max_lines)
>> return 1;
>> @@ -1454,7 +1452,7 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>> if (!*al->line)
>> printf(" %*s:\n", width, " ");
>> else
>> - printf(" %*s: %*s %s\n", width, " ", addr_fmt_width, " ", al->line);
>> + printf(" %*s: %-*d %s\n", width, " ", addr_fmt_width, al->line_nr, al->line);
>> }
>> return 0;
>> --
>> 2.30.1
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: improve --stdio mode
2021-02-26 9:24 ` Martin Liška
@ 2021-03-19 9:38 ` Martin Liška
2021-04-07 19:28 ` Arnaldo Carvalho de Melo
2021-04-07 19:30 ` Arnaldo Carvalho de Melo
1 sibling, 1 reply; 10+ messages in thread
From: Martin Liška @ 2021-03-19 9:38 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: linux-kernel, linux-perf-users
PING
On 2/26/21 10:24 AM, Martin Liška wrote:
> On 2/23/21 8:47 PM, Arnaldo Carvalho de Melo wrote:
>> Em Sun, Feb 21, 2021 at 01:46:36PM +0100, Martin Liška escreveu:
>>> The patch changes the output format in 2 ways:
>>> - line number is displayed for all source lines (matching TUI mode)
>>
>> Are you aware of 'perf annotate --stdio2' ? If the goal is to make the
>> stdio mode better, doing it in that mode would be best, as it was done
>> to share as much code as possible, not just the looks, with the TUI
>> mode.
>
> Yes, I'm aware of it. My motivation is to generate a HTML perf annotate report
> and I see the following parts of --stdio2 not ideal:
>
> - coloring is not available (--stdio-color=always does not work)
> - 'Sorted summary for file ' is missing so one can't easily search for
> hot spots in browser
> - source line number are displayed, but not the source files
> - there's a missing option for 'Toggle disassembler output/simplified view' which
> is available in TUI mode
>
> That said, the stdio2 annotation report is quite different and so handy for my use case.
>
>>
>> I kept --stdio around because changing the output in that way could
>> annoy people used to that format.
>
> Sure. But I think the current format provides quite broken visual layout:
>
> 0.00 : 405ef1: inc %r15
> 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
> eff.c:1811 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8>
> : TA + tmpsd * (TB +
>
> vs.
>
> 0.00 : 405ef1: inc %r15
> 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
> 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8> // eff.c:1811
> : 1810 TA + tmpsd * (TB +
>
> I bet also the current users of --stdio mode would benefit from it.
> What do you think?
>
> Thanks,
> Martin
>
>>
>> Please take a look at 'man perf-config' and see what can be configured
>> for both 'perf annotate --tui' and 'perf annotate --stdio2'.
>>
>> Perhaps we can do something like:
>>
>> perf config annotate.stdio=tui_like
>>
>> And, for completeness have:
>>
>> perf config annotate.stdio=classical
>>
>> wdyt?
>>
>> Looking at the other patches now.
>>
>> - Arnaldo
>>
>>> - source locations for the hottest lines are printed
>>> at the line end in order to preserve layout
>>>
>>> Before:
>>>
>>> 0.00 : 405ef1: inc %r15
>>> : tmpsd * (TD + tmpsd * TDD)));
>>> 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
>>> : tmpsd * (TC +
>>> eff.c:1811 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8>
>>> : TA + tmpsd * (TB +
>>> 0.35 : 405f06: vfmadd213sd 0x2b9b1(%rip),%xmm0,%xmm3 # 4318c0 <_IO_stdin_used+0x8c0>
>>> : dumbo =
>>> eff.c:1809 1.41 : 405f0f: vfmadd213sd 0x2b9b0(%rip),%xmm0,%xmm3 # 4318c8 <_IO_stdin_used+0x8c8>
>>> : sumi -= sj * tmpsd * dij2i * dumbo;
>>> eff.c:1813 2.58 : 405f18: vmulsd %xmm3,%xmm0,%xmm0
>>> 2.81 : 405f1c: vfnmadd213sd 0x30(%rsp),%xmm1,%xmm0
>>> 3.78 : 405f23: vmovsd %xmm0,0x30(%rsp)
>>> : for (k = 0; k < lpears[i] + upears[i]; k++) {
>>> eff.c:1761 0.90 : 405f29: cmp %r15d,%r12d
>>>
>>> After:
>>>
>>> 0.00 : 405ef1: inc %r15
>>> : 1812 tmpsd * (TD + tmpsd * TDD)));
>>> 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
>>> : 1811 tmpsd * (TC +
>>> 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8> // eff.c:1811
>>> : 1810 TA + tmpsd * (TB +
>>> 0.35 : 405f06: vfmadd213sd 0x2b9b1(%rip),%xmm0,%xmm3 # 4318c0 <_IO_stdin_used+0x8c0>
>>> : 1809 dumbo =
>>> 1.41 : 405f0f: vfmadd213sd 0x2b9b0(%rip),%xmm0,%xmm3 # 4318c8 <_IO_stdin_used+0x8c8> // eff.c:1809
>>> : 1813 sumi -= sj * tmpsd * dij2i * dumbo;
>>> 2.58 : 405f18: vmulsd %xmm3,%xmm0,%xmm0 // eff.c:1813
>>> 2.81 : 405f1c: vfnmadd213sd 0x30(%rsp),%xmm1,%xmm0
>>> 3.78 : 405f23: vmovsd %xmm0,0x30(%rsp)
>>> : 1761 for (k = 0; k < lpears[i] + upears[i]; k++) {
>>>
>>> Where e.g. '// eff.c:1811' shares the same color as the percentantage
>>> at the line beginning.
>>>
>>> Signed-off-by: Martin Liška <mliska@suse.cz>
>>> ---
>>> tools/perf/util/annotate.c | 30 ++++++++++++++----------------
>>> 1 file changed, 14 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>>> index e60841b86d27..80542012ec1b 100644
>>> --- a/tools/perf/util/annotate.c
>>> +++ b/tools/perf/util/annotate.c
>>> @@ -1366,7 +1366,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>>> {
>>> struct disasm_line *dl = container_of(al, struct disasm_line, al);
>>> static const char *prev_line;
>>> - static const char *prev_color;
>>> if (al->offset != -1) {
>>> double max_percent = 0.0;
>>> @@ -1405,20 +1404,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>>> color = get_percent_color(max_percent);
>>> - /*
>>> - * Also color the filename and line if needed, with
>>> - * the same color than the percentage. Don't print it
>>> - * twice for close colored addr with the same filename:line
>>> - */
>>> - if (al->path) {
>>> - if (!prev_line || strcmp(prev_line, al->path)
>>> - || color != prev_color) {
>>> - color_fprintf(stdout, color, " %s", al->path);
>>> - prev_line = al->path;
>>> - prev_color = color;
>>> - }
>>> - }
>>> -
>>> for (i = 0; i < nr_percent; i++) {
>>> struct annotation_data *data = &al->data[i];
>>> double percent;
>>> @@ -1439,6 +1424,19 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>>> printf(" : ");
>>> disasm_line__print(dl, start, addr_fmt_width);
>>> +
>>> + /*
>>> + * Also color the filename and line if needed, with
>>> + * the same color than the percentage. Don't print it
>>> + * twice for close colored addr with the same filename:line
>>> + */
>>> + if (al->path) {
>>> + if (!prev_line || strcmp(prev_line, al->path)) {
>>> + color_fprintf(stdout, color, " // %s", al->path);
>>> + prev_line = al->path;
>>> + }
>>> + }
>>> +
>>> printf("\n");
>>> } else if (max_lines && printed >= max_lines)
>>> return 1;
>>> @@ -1454,7 +1452,7 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>>> if (!*al->line)
>>> printf(" %*s:\n", width, " ");
>>> else
>>> - printf(" %*s: %*s %s\n", width, " ", addr_fmt_width, " ", al->line);
>>> + printf(" %*s: %-*d %s\n", width, " ", addr_fmt_width, al->line_nr, al->line);
>>> }
>>> return 0;
>>> --
>>> 2.30.1
>>>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: improve --stdio mode
2021-03-19 9:38 ` Martin Liška
@ 2021-04-07 19:28 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-04-07 19:28 UTC (permalink / raw)
To: Martin Liška; +Cc: linux-kernel, linux-perf-users
Em Fri, Mar 19, 2021 at 10:38:26AM +0100, Martin Liška escreveu:
> PING
Can you please try to refresh the patch? It isn't applying, probably my
bad for not having processed it already :-\
- Arnaldo
> On 2/26/21 10:24 AM, Martin Liška wrote:
> > On 2/23/21 8:47 PM, Arnaldo Carvalho de Melo wrote:
> > > Em Sun, Feb 21, 2021 at 01:46:36PM +0100, Martin Liška escreveu:
> > > > The patch changes the output format in 2 ways:
> > > > - line number is displayed for all source lines (matching TUI mode)
> > >
> > > Are you aware of 'perf annotate --stdio2' ? If the goal is to make the
> > > stdio mode better, doing it in that mode would be best, as it was done
> > > to share as much code as possible, not just the looks, with the TUI
> > > mode.
> >
> > Yes, I'm aware of it. My motivation is to generate a HTML perf annotate report
> > and I see the following parts of --stdio2 not ideal:
> >
> > - coloring is not available (--stdio-color=always does not work)
> > - 'Sorted summary for file ' is missing so one can't easily search for
> > hot spots in browser
> > - source line number are displayed, but not the source files
> > - there's a missing option for 'Toggle disassembler output/simplified view' which
> > is available in TUI mode
> >
> > That said, the stdio2 annotation report is quite different and so handy for my use case.
> >
> > >
> > > I kept --stdio around because changing the output in that way could
> > > annoy people used to that format.
> >
> > Sure. But I think the current format provides quite broken visual layout:
> >
> > 0.00 : 405ef1: inc %r15
> > 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
> > eff.c:1811 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8>
> > : TA + tmpsd * (TB +
> >
> > vs.
> >
> > 0.00 : 405ef1: inc %r15
> > 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
> > 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8> // eff.c:1811
> > : 1810 TA + tmpsd * (TB +
> >
> > I bet also the current users of --stdio mode would benefit from it.
> > What do you think?
> >
> > Thanks,
> > Martin
> >
> > >
> > > Please take a look at 'man perf-config' and see what can be configured
> > > for both 'perf annotate --tui' and 'perf annotate --stdio2'.
> > >
> > > Perhaps we can do something like:
> > >
> > > perf config annotate.stdio=tui_like
> > >
> > > And, for completeness have:
> > >
> > > perf config annotate.stdio=classical
> > >
> > > wdyt?
> > >
> > > Looking at the other patches now.
> > >
> > > - Arnaldo
> > >
> > > > - source locations for the hottest lines are printed
> > > > at the line end in order to preserve layout
> > > >
> > > > Before:
> > > >
> > > > 0.00 : 405ef1: inc %r15
> > > > : tmpsd * (TD + tmpsd * TDD)));
> > > > 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
> > > > : tmpsd * (TC +
> > > > eff.c:1811 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8>
> > > > : TA + tmpsd * (TB +
> > > > 0.35 : 405f06: vfmadd213sd 0x2b9b1(%rip),%xmm0,%xmm3 # 4318c0 <_IO_stdin_used+0x8c0>
> > > > : dumbo =
> > > > eff.c:1809 1.41 : 405f0f: vfmadd213sd 0x2b9b0(%rip),%xmm0,%xmm3 # 4318c8 <_IO_stdin_used+0x8c8>
> > > > : sumi -= sj * tmpsd * dij2i * dumbo;
> > > > eff.c:1813 2.58 : 405f18: vmulsd %xmm3,%xmm0,%xmm0
> > > > 2.81 : 405f1c: vfnmadd213sd 0x30(%rsp),%xmm1,%xmm0
> > > > 3.78 : 405f23: vmovsd %xmm0,0x30(%rsp)
> > > > : for (k = 0; k < lpears[i] + upears[i]; k++) {
> > > > eff.c:1761 0.90 : 405f29: cmp %r15d,%r12d
> > > >
> > > > After:
> > > >
> > > > 0.00 : 405ef1: inc %r15
> > > > : 1812 tmpsd * (TD + tmpsd * TDD)));
> > > > 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
> > > > : 1811 tmpsd * (TC +
> > > > 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8> // eff.c:1811
> > > > : 1810 TA + tmpsd * (TB +
> > > > 0.35 : 405f06: vfmadd213sd 0x2b9b1(%rip),%xmm0,%xmm3 # 4318c0 <_IO_stdin_used+0x8c0>
> > > > : 1809 dumbo =
> > > > 1.41 : 405f0f: vfmadd213sd 0x2b9b0(%rip),%xmm0,%xmm3 # 4318c8 <_IO_stdin_used+0x8c8> // eff.c:1809
> > > > : 1813 sumi -= sj * tmpsd * dij2i * dumbo;
> > > > 2.58 : 405f18: vmulsd %xmm3,%xmm0,%xmm0 // eff.c:1813
> > > > 2.81 : 405f1c: vfnmadd213sd 0x30(%rsp),%xmm1,%xmm0
> > > > 3.78 : 405f23: vmovsd %xmm0,0x30(%rsp)
> > > > : 1761 for (k = 0; k < lpears[i] + upears[i]; k++) {
> > > >
> > > > Where e.g. '// eff.c:1811' shares the same color as the percentantage
> > > > at the line beginning.
> > > >
> > > > Signed-off-by: Martin Liška <mliska@suse.cz>
> > > > ---
> > > > tools/perf/util/annotate.c | 30 ++++++++++++++----------------
> > > > 1 file changed, 14 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > > > index e60841b86d27..80542012ec1b 100644
> > > > --- a/tools/perf/util/annotate.c
> > > > +++ b/tools/perf/util/annotate.c
> > > > @@ -1366,7 +1366,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> > > > {
> > > > struct disasm_line *dl = container_of(al, struct disasm_line, al);
> > > > static const char *prev_line;
> > > > - static const char *prev_color;
> > > > if (al->offset != -1) {
> > > > double max_percent = 0.0;
> > > > @@ -1405,20 +1404,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> > > > color = get_percent_color(max_percent);
> > > > - /*
> > > > - * Also color the filename and line if needed, with
> > > > - * the same color than the percentage. Don't print it
> > > > - * twice for close colored addr with the same filename:line
> > > > - */
> > > > - if (al->path) {
> > > > - if (!prev_line || strcmp(prev_line, al->path)
> > > > - || color != prev_color) {
> > > > - color_fprintf(stdout, color, " %s", al->path);
> > > > - prev_line = al->path;
> > > > - prev_color = color;
> > > > - }
> > > > - }
> > > > -
> > > > for (i = 0; i < nr_percent; i++) {
> > > > struct annotation_data *data = &al->data[i];
> > > > double percent;
> > > > @@ -1439,6 +1424,19 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> > > > printf(" : ");
> > > > disasm_line__print(dl, start, addr_fmt_width);
> > > > +
> > > > + /*
> > > > + * Also color the filename and line if needed, with
> > > > + * the same color than the percentage. Don't print it
> > > > + * twice for close colored addr with the same filename:line
> > > > + */
> > > > + if (al->path) {
> > > > + if (!prev_line || strcmp(prev_line, al->path)) {
> > > > + color_fprintf(stdout, color, " // %s", al->path);
> > > > + prev_line = al->path;
> > > > + }
> > > > + }
> > > > +
> > > > printf("\n");
> > > > } else if (max_lines && printed >= max_lines)
> > > > return 1;
> > > > @@ -1454,7 +1452,7 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> > > > if (!*al->line)
> > > > printf(" %*s:\n", width, " ");
> > > > else
> > > > - printf(" %*s: %*s %s\n", width, " ", addr_fmt_width, " ", al->line);
> > > > + printf(" %*s: %-*d %s\n", width, " ", addr_fmt_width, al->line_nr, al->line);
> > > > }
> > > > return 0;
> > > > --
> > > > 2.30.1
> > > >
> > >
> >
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: improve --stdio mode
2021-02-26 9:24 ` Martin Liška
2021-03-19 9:38 ` Martin Liška
@ 2021-04-07 19:30 ` Arnaldo Carvalho de Melo
2021-04-07 20:25 ` Arnaldo Carvalho de Melo
1 sibling, 1 reply; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-04-07 19:30 UTC (permalink / raw)
To: Martin Liška; +Cc: linux-kernel, linux-perf-users
Em Fri, Feb 26, 2021 at 10:24:00AM +0100, Martin Liška escreveu:
> On 2/23/21 8:47 PM, Arnaldo Carvalho de Melo wrote:
> > Em Sun, Feb 21, 2021 at 01:46:36PM +0100, Martin Liška escreveu:
> > > The patch changes the output format in 2 ways:
> > > - line number is displayed for all source lines (matching TUI mode)
> >
> > Are you aware of 'perf annotate --stdio2' ? If the goal is to make the
> > stdio mode better, doing it in that mode would be best, as it was done
> > to share as much code as possible, not just the looks, with the TUI
> > mode.
>
> Yes, I'm aware of it. My motivation is to generate a HTML perf annotate report
> and I see the following parts of --stdio2 not ideal:
>
> - coloring is not available (--stdio-color=always does not work)
> - 'Sorted summary for file ' is missing so one can't easily search for
> hot spots in browser
> - source line number are displayed, but not the source files
> - there's a missing option for 'Toggle disassembler output/simplified view' which
> is available in TUI mode
>
> That said, the stdio2 annotation report is quite different and so handy for my use case.
I'll add these to my TODO list, all valid concerns. And being able to
directly generate some sort of HTML with CSS, etc also seems a great
feature to have.
- Arnaldo
> >
> > I kept --stdio around because changing the output in that way could
> > annoy people used to that format.
>
> Sure. But I think the current format provides quite broken visual layout:
>
> 0.00 : 405ef1: inc %r15
> 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
> eff.c:1811 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8>
> : TA + tmpsd * (TB +
>
> vs.
>
> 0.00 : 405ef1: inc %r15
> 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
> 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8> // eff.c:1811
> : 1810 TA + tmpsd * (TB +
>
> I bet also the current users of --stdio mode would benefit from it.
> What do you think?
Agreed, I tried applying but it bitrotted, it seems :-\
- Arnaldo
> Thanks,
> Martin
>
> >
> > Please take a look at 'man perf-config' and see what can be configured
> > for both 'perf annotate --tui' and 'perf annotate --stdio2'.
> >
> > Perhaps we can do something like:
> >
> > perf config annotate.stdio=tui_like
> >
> > And, for completeness have:
> >
> > perf config annotate.stdio=classical
> >
> > wdyt?
> >
> > Looking at the other patches now.
> >
> > - Arnaldo
> >
> > > - source locations for the hottest lines are printed
> > > at the line end in order to preserve layout
> > >
> > > Before:
> > >
> > > 0.00 : 405ef1: inc %r15
> > > : tmpsd * (TD + tmpsd * TDD)));
> > > 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
> > > : tmpsd * (TC +
> > > eff.c:1811 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8>
> > > : TA + tmpsd * (TB +
> > > 0.35 : 405f06: vfmadd213sd 0x2b9b1(%rip),%xmm0,%xmm3 # 4318c0 <_IO_stdin_used+0x8c0>
> > > : dumbo =
> > > eff.c:1809 1.41 : 405f0f: vfmadd213sd 0x2b9b0(%rip),%xmm0,%xmm3 # 4318c8 <_IO_stdin_used+0x8c8>
> > > : sumi -= sj * tmpsd * dij2i * dumbo;
> > > eff.c:1813 2.58 : 405f18: vmulsd %xmm3,%xmm0,%xmm0
> > > 2.81 : 405f1c: vfnmadd213sd 0x30(%rsp),%xmm1,%xmm0
> > > 3.78 : 405f23: vmovsd %xmm0,0x30(%rsp)
> > > : for (k = 0; k < lpears[i] + upears[i]; k++) {
> > > eff.c:1761 0.90 : 405f29: cmp %r15d,%r12d
> > >
> > > After:
> > >
> > > 0.00 : 405ef1: inc %r15
> > > : 1812 tmpsd * (TD + tmpsd * TDD)));
> > > 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
> > > : 1811 tmpsd * (TC +
> > > 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8> // eff.c:1811
> > > : 1810 TA + tmpsd * (TB +
> > > 0.35 : 405f06: vfmadd213sd 0x2b9b1(%rip),%xmm0,%xmm3 # 4318c0 <_IO_stdin_used+0x8c0>
> > > : 1809 dumbo =
> > > 1.41 : 405f0f: vfmadd213sd 0x2b9b0(%rip),%xmm0,%xmm3 # 4318c8 <_IO_stdin_used+0x8c8> // eff.c:1809
> > > : 1813 sumi -= sj * tmpsd * dij2i * dumbo;
> > > 2.58 : 405f18: vmulsd %xmm3,%xmm0,%xmm0 // eff.c:1813
> > > 2.81 : 405f1c: vfnmadd213sd 0x30(%rsp),%xmm1,%xmm0
> > > 3.78 : 405f23: vmovsd %xmm0,0x30(%rsp)
> > > : 1761 for (k = 0; k < lpears[i] + upears[i]; k++) {
> > >
> > > Where e.g. '// eff.c:1811' shares the same color as the percentantage
> > > at the line beginning.
> > >
> > > Signed-off-by: Martin Liška <mliska@suse.cz>
> > > ---
> > > tools/perf/util/annotate.c | 30 ++++++++++++++----------------
> > > 1 file changed, 14 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > > index e60841b86d27..80542012ec1b 100644
> > > --- a/tools/perf/util/annotate.c
> > > +++ b/tools/perf/util/annotate.c
> > > @@ -1366,7 +1366,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> > > {
> > > struct disasm_line *dl = container_of(al, struct disasm_line, al);
> > > static const char *prev_line;
> > > - static const char *prev_color;
> > > if (al->offset != -1) {
> > > double max_percent = 0.0;
> > > @@ -1405,20 +1404,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> > > color = get_percent_color(max_percent);
> > > - /*
> > > - * Also color the filename and line if needed, with
> > > - * the same color than the percentage. Don't print it
> > > - * twice for close colored addr with the same filename:line
> > > - */
> > > - if (al->path) {
> > > - if (!prev_line || strcmp(prev_line, al->path)
> > > - || color != prev_color) {
> > > - color_fprintf(stdout, color, " %s", al->path);
> > > - prev_line = al->path;
> > > - prev_color = color;
> > > - }
> > > - }
> > > -
> > > for (i = 0; i < nr_percent; i++) {
> > > struct annotation_data *data = &al->data[i];
> > > double percent;
> > > @@ -1439,6 +1424,19 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> > > printf(" : ");
> > > disasm_line__print(dl, start, addr_fmt_width);
> > > +
> > > + /*
> > > + * Also color the filename and line if needed, with
> > > + * the same color than the percentage. Don't print it
> > > + * twice for close colored addr with the same filename:line
> > > + */
> > > + if (al->path) {
> > > + if (!prev_line || strcmp(prev_line, al->path)) {
> > > + color_fprintf(stdout, color, " // %s", al->path);
> > > + prev_line = al->path;
> > > + }
> > > + }
> > > +
> > > printf("\n");
> > > } else if (max_lines && printed >= max_lines)
> > > return 1;
> > > @@ -1454,7 +1452,7 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> > > if (!*al->line)
> > > printf(" %*s:\n", width, " ");
> > > else
> > > - printf(" %*s: %*s %s\n", width, " ", addr_fmt_width, " ", al->line);
> > > + printf(" %*s: %-*d %s\n", width, " ", addr_fmt_width, al->line_nr, al->line);
> > > }
> > > return 0;
> > > --
> > > 2.30.1
> > >
> >
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: improve --stdio mode
2021-04-07 19:30 ` Arnaldo Carvalho de Melo
@ 2021-04-07 20:25 ` Arnaldo Carvalho de Melo
2021-04-08 10:08 ` Martin Liška
0 siblings, 1 reply; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-04-07 20:25 UTC (permalink / raw)
To: Martin Liška; +Cc: linux-kernel, linux-perf-users
Em Wed, Apr 07, 2021 at 04:30:46PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Feb 26, 2021 at 10:24:00AM +0100, Martin Liška escreveu:
> > On 2/23/21 8:47 PM, Arnaldo Carvalho de Melo wrote:
> > Sure. But I think the current format provides quite broken visual layout:
> >
> > 0.00 : 405ef1: inc %r15
> > 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
> > eff.c:1811 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8>
> > : TA + tmpsd * (TB +
> >
> > vs.
> >
> > 0.00 : 405ef1: inc %r15
> > 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
> > 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8> // eff.c:1811
> > : 1810 TA + tmpsd * (TB +
> >
> > I bet also the current users of --stdio mode would benefit from it.
> > What do you think?
> Agreed, I tried applying but it bitrotted, it seems :-\
I refreshed it, please check.
- Arnaldo
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 18eee25b4976bea8..abe1499a91645375 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1368,7 +1368,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
{
struct disasm_line *dl = container_of(al, struct disasm_line, al);
static const char *prev_line;
- static const char *prev_color;
if (al->offset != -1) {
double max_percent = 0.0;
@@ -1407,20 +1406,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
color = get_percent_color(max_percent);
- /*
- * Also color the filename and line if needed, with
- * the same color than the percentage. Don't print it
- * twice for close colored addr with the same filename:line
- */
- if (al->path) {
- if (!prev_line || strcmp(prev_line, al->path)
- || color != prev_color) {
- color_fprintf(stdout, color, " %s", al->path);
- prev_line = al->path;
- prev_color = color;
- }
- }
-
for (i = 0; i < nr_percent; i++) {
struct annotation_data *data = &al->data[i];
double percent;
@@ -1441,6 +1426,19 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
printf(" : ");
disasm_line__print(dl, start, addr_fmt_width);
+
+ /*
+ * Also color the filename and line if needed, with
+ * the same color than the percentage. Don't print it
+ * twice for close colored addr with the same filename:line
+ */
+ if (al->path) {
+ if (!prev_line || strcmp(prev_line, al->path)) {
+ color_fprintf(stdout, color, " // %s", al->path);
+ prev_line = al->path;
+ }
+ }
+
printf("\n");
} else if (max_lines && printed >= max_lines)
return 1;
@@ -1456,7 +1454,7 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
if (!*al->line)
printf(" %*s:\n", width, " ");
else
- printf(" %*s: %*s %s\n", width, " ", addr_fmt_width, " ", al->line);
+ printf(" %*s: %-*d %s\n", width, " ", addr_fmt_width, al->line_nr, al->line);
}
return 0;
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: improve --stdio mode
2021-04-07 20:25 ` Arnaldo Carvalho de Melo
@ 2021-04-08 10:08 ` Martin Liška
2021-04-19 7:39 ` Martin Liška
0 siblings, 1 reply; 10+ messages in thread
From: Martin Liška @ 2021-04-08 10:08 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: linux-kernel, linux-perf-users
On 4/7/21 10:25 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 07, 2021 at 04:30:46PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Fri, Feb 26, 2021 at 10:24:00AM +0100, Martin Liška escreveu:
>>> On 2/23/21 8:47 PM, Arnaldo Carvalho de Melo wrote:
>>> Sure. But I think the current format provides quite broken visual layout:
>>>
>>> 0.00 : 405ef1: inc %r15
>>> 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
>>> eff.c:1811 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8>
>>> : TA + tmpsd * (TB +
>>>
>>> vs.
>>>
>>> 0.00 : 405ef1: inc %r15
>>> 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
>>> 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8> // eff.c:1811
>>> : 1810 TA + tmpsd * (TB +
>>>
>>> I bet also the current users of --stdio mode would benefit from it.
>>> What do you think?
>
>> Agreed, I tried applying but it bitrotted, it seems :-\
>
> I refreshed it, please check.
Thanks! I've just tested the patch on top of acme/perf/core and it works as was planned.
I'm attaching 2 perf annotate snippets (perf annotate --stdio -l --stdio-color=always) before
and after the revision:
https://splichal.eu/tmp/perf-before.html
https://splichal.eu/tmp/perf-after.html
I hope it nicely describes that it's an improvement.
Cheers,
Martin
>
> - Arnaldo
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 18eee25b4976bea8..abe1499a91645375 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1368,7 +1368,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> {
> struct disasm_line *dl = container_of(al, struct disasm_line, al);
> static const char *prev_line;
> - static const char *prev_color;
>
> if (al->offset != -1) {
> double max_percent = 0.0;
> @@ -1407,20 +1406,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>
> color = get_percent_color(max_percent);
>
> - /*
> - * Also color the filename and line if needed, with
> - * the same color than the percentage. Don't print it
> - * twice for close colored addr with the same filename:line
> - */
> - if (al->path) {
> - if (!prev_line || strcmp(prev_line, al->path)
> - || color != prev_color) {
> - color_fprintf(stdout, color, " %s", al->path);
> - prev_line = al->path;
> - prev_color = color;
> - }
> - }
> -
> for (i = 0; i < nr_percent; i++) {
> struct annotation_data *data = &al->data[i];
> double percent;
> @@ -1441,6 +1426,19 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> printf(" : ");
>
> disasm_line__print(dl, start, addr_fmt_width);
> +
> + /*
> + * Also color the filename and line if needed, with
> + * the same color than the percentage. Don't print it
> + * twice for close colored addr with the same filename:line
> + */
> + if (al->path) {
> + if (!prev_line || strcmp(prev_line, al->path)) {
> + color_fprintf(stdout, color, " // %s", al->path);
> + prev_line = al->path;
> + }
> + }
> +
> printf("\n");
> } else if (max_lines && printed >= max_lines)
> return 1;
> @@ -1456,7 +1454,7 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> if (!*al->line)
> printf(" %*s:\n", width, " ");
> else
> - printf(" %*s: %*s %s\n", width, " ", addr_fmt_width, " ", al->line);
> + printf(" %*s: %-*d %s\n", width, " ", addr_fmt_width, al->line_nr, al->line);
> }
>
> return 0;
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: improve --stdio mode
2021-04-08 10:08 ` Martin Liška
@ 2021-04-19 7:39 ` Martin Liška
2021-04-19 17:12 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 10+ messages in thread
From: Martin Liška @ 2021-04-19 7:39 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: linux-kernel, linux-perf-users
@Arnaldo: May I please ping this?
Thanks,
Martin
On 4/8/21 12:08 PM, Martin Liška wrote:
> On 4/7/21 10:25 PM, Arnaldo Carvalho de Melo wrote:
>> Em Wed, Apr 07, 2021 at 04:30:46PM -0300, Arnaldo Carvalho de Melo escreveu:
>>> Em Fri, Feb 26, 2021 at 10:24:00AM +0100, Martin Liška escreveu:
>>>> On 2/23/21 8:47 PM, Arnaldo Carvalho de Melo wrote:
>>>> Sure. But I think the current format provides quite broken visual layout:
>>>>
>>>> 0.00 : 405ef1: inc %r15
>>>> 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
>>>> eff.c:1811 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8>
>>>> : TA + tmpsd * (TB +
>>>>
>>>> vs.
>>>>
>>>> 0.00 : 405ef1: inc %r15
>>>> 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
>>>> 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8> // eff.c:1811
>>>> : 1810 TA + tmpsd * (TB +
>>>>
>>>> I bet also the current users of --stdio mode would benefit from it.
>>>> What do you think?
>>
>>> Agreed, I tried applying but it bitrotted, it seems :-\
>>
>> I refreshed it, please check.
>
> Thanks! I've just tested the patch on top of acme/perf/core and it works as was planned.
> I'm attaching 2 perf annotate snippets (perf annotate --stdio -l --stdio-color=always) before
> and after the revision:
>
> https://splichal.eu/tmp/perf-before.html
> https://splichal.eu/tmp/perf-after.html
>
> I hope it nicely describes that it's an improvement.
>
> Cheers,
> Martin
>
>>
>> - Arnaldo
>>
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index 18eee25b4976bea8..abe1499a91645375 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -1368,7 +1368,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>> {
>> struct disasm_line *dl = container_of(al, struct disasm_line, al);
>> static const char *prev_line;
>> - static const char *prev_color;
>>
>> if (al->offset != -1) {
>> double max_percent = 0.0;
>> @@ -1407,20 +1406,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>>
>> color = get_percent_color(max_percent);
>>
>> - /*
>> - * Also color the filename and line if needed, with
>> - * the same color than the percentage. Don't print it
>> - * twice for close colored addr with the same filename:line
>> - */
>> - if (al->path) {
>> - if (!prev_line || strcmp(prev_line, al->path)
>> - || color != prev_color) {
>> - color_fprintf(stdout, color, " %s", al->path);
>> - prev_line = al->path;
>> - prev_color = color;
>> - }
>> - }
>> -
>> for (i = 0; i < nr_percent; i++) {
>> struct annotation_data *data = &al->data[i];
>> double percent;
>> @@ -1441,6 +1426,19 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>> printf(" : ");
>>
>> disasm_line__print(dl, start, addr_fmt_width);
>> +
>> + /*
>> + * Also color the filename and line if needed, with
>> + * the same color than the percentage. Don't print it
>> + * twice for close colored addr with the same filename:line
>> + */
>> + if (al->path) {
>> + if (!prev_line || strcmp(prev_line, al->path)) {
>> + color_fprintf(stdout, color, " // %s", al->path);
>> + prev_line = al->path;
>> + }
>> + }
>> +
>> printf("\n");
>> } else if (max_lines && printed >= max_lines)
>> return 1;
>> @@ -1456,7 +1454,7 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
>> if (!*al->line)
>> printf(" %*s:\n", width, " ");
>> else
>> - printf(" %*s: %*s %s\n", width, " ", addr_fmt_width, " ", al->line);
>> + printf(" %*s: %-*d %s\n", width, " ", addr_fmt_width, al->line_nr, al->line);
>> }
>>
>> return 0;
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf annotate: improve --stdio mode
2021-04-19 7:39 ` Martin Liška
@ 2021-04-19 17:12 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-04-19 17:12 UTC (permalink / raw)
To: Martin Liška; +Cc: linux-kernel, linux-perf-users
Em Mon, Apr 19, 2021 at 09:39:37AM +0200, Martin Liška escreveu:
> @Arnaldo: May I please ping this?
Applied the refreshed version,
- Arnaldo
> Thanks,
> Martin
>
> On 4/8/21 12:08 PM, Martin Liška wrote:
> > On 4/7/21 10:25 PM, Arnaldo Carvalho de Melo wrote:
> >> Em Wed, Apr 07, 2021 at 04:30:46PM -0300, Arnaldo Carvalho de Melo escreveu:
> >>> Em Fri, Feb 26, 2021 at 10:24:00AM +0100, Martin Liška escreveu:
> >>>> On 2/23/21 8:47 PM, Arnaldo Carvalho de Melo wrote:
> >>>> Sure. But I think the current format provides quite broken visual layout:
> >>>>
> >>>> 0.00 : 405ef1: inc %r15
> >>>> 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
> >>>> eff.c:1811 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8>
> >>>> : TA + tmpsd * (TB +
> >>>>
> >>>> vs.
> >>>>
> >>>> 0.00 : 405ef1: inc %r15
> >>>> 0.01 : 405ef4: vfmadd213sd 0x2b9b3(%rip),%xmm0,%xmm3 # 4318b0 <_IO_stdin_used+0x8b0>
> >>>> 0.67 : 405efd: vfmadd213sd 0x2b9b2(%rip),%xmm0,%xmm3 # 4318b8 <_IO_stdin_used+0x8b8> // eff.c:1811
> >>>> : 1810 TA + tmpsd * (TB +
> >>>>
> >>>> I bet also the current users of --stdio mode would benefit from it.
> >>>> What do you think?
> >>
> >>> Agreed, I tried applying but it bitrotted, it seems :-\
> >>
> >> I refreshed it, please check.
> >
> > Thanks! I've just tested the patch on top of acme/perf/core and it works as was planned.
> > I'm attaching 2 perf annotate snippets (perf annotate --stdio -l --stdio-color=always) before
> > and after the revision:
> >
> > https://splichal.eu/tmp/perf-before.html
> > https://splichal.eu/tmp/perf-after.html
> >
> > I hope it nicely describes that it's an improvement.
> >
> > Cheers,
> > Martin
> >
> >>
> >> - Arnaldo
> >>
> >> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> >> index 18eee25b4976bea8..abe1499a91645375 100644
> >> --- a/tools/perf/util/annotate.c
> >> +++ b/tools/perf/util/annotate.c
> >> @@ -1368,7 +1368,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> >> {
> >> struct disasm_line *dl = container_of(al, struct disasm_line, al);
> >> static const char *prev_line;
> >> - static const char *prev_color;
> >>
> >> if (al->offset != -1) {
> >> double max_percent = 0.0;
> >> @@ -1407,20 +1406,6 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> >>
> >> color = get_percent_color(max_percent);
> >>
> >> - /*
> >> - * Also color the filename and line if needed, with
> >> - * the same color than the percentage. Don't print it
> >> - * twice for close colored addr with the same filename:line
> >> - */
> >> - if (al->path) {
> >> - if (!prev_line || strcmp(prev_line, al->path)
> >> - || color != prev_color) {
> >> - color_fprintf(stdout, color, " %s", al->path);
> >> - prev_line = al->path;
> >> - prev_color = color;
> >> - }
> >> - }
> >> -
> >> for (i = 0; i < nr_percent; i++) {
> >> struct annotation_data *data = &al->data[i];
> >> double percent;
> >> @@ -1441,6 +1426,19 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> >> printf(" : ");
> >>
> >> disasm_line__print(dl, start, addr_fmt_width);
> >> +
> >> + /*
> >> + * Also color the filename and line if needed, with
> >> + * the same color than the percentage. Don't print it
> >> + * twice for close colored addr with the same filename:line
> >> + */
> >> + if (al->path) {
> >> + if (!prev_line || strcmp(prev_line, al->path)) {
> >> + color_fprintf(stdout, color, " // %s", al->path);
> >> + prev_line = al->path;
> >> + }
> >> + }
> >> +
> >> printf("\n");
> >> } else if (max_lines && printed >= max_lines)
> >> return 1;
> >> @@ -1456,7 +1454,7 @@ annotation_line__print(struct annotation_line *al, struct symbol *sym, u64 start
> >> if (!*al->line)
> >> printf(" %*s:\n", width, " ");
> >> else
> >> - printf(" %*s: %*s %s\n", width, " ", addr_fmt_width, " ", al->line);
> >> + printf(" %*s: %-*d %s\n", width, " ", addr_fmt_width, al->line_nr, al->line);
> >> }
> >>
> >> return 0;
> >>
> >
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-04-19 17:12 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-21 12:46 [PATCH] perf annotate: improve --stdio mode Martin Liška
2021-02-23 19:47 ` Arnaldo Carvalho de Melo
2021-02-26 9:24 ` Martin Liška
2021-03-19 9:38 ` Martin Liška
2021-04-07 19:28 ` Arnaldo Carvalho de Melo
2021-04-07 19:30 ` Arnaldo Carvalho de Melo
2021-04-07 20:25 ` Arnaldo Carvalho de Melo
2021-04-08 10:08 ` Martin Liška
2021-04-19 7:39 ` Martin Liška
2021-04-19 17:12 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).