[GIT PULL 0/5] perf/annotate fixes and improvements

[GIT PULL 0/5] perf/annotate fixes and improvements

[Arnaldo Carvalho de Melo]

Hi Ingo,

	Please consider pulling,

- Arnaldo
The following changes since commit 38b31bd0cefbb0e69a182d9a94b09a7e648549dc:

  perf annotate browser: Don't draw jump connectors for out of function jumps (2012-04-25 14:18:42 -0300)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux perf/annotate

for you to fetch changes up to 0822cc80d9aee026b1ebe43c02dc01e0a0227864:

  perf annotate browser: Don't display 0.00 percentages (2012-04-27 17:13:53 -0300)

----------------------------------------------------------------
Perf annotate improvements and fixes:

. Current output:

avtab_search_node
               push   %rbp
               mov    %rsp,%rbp
             → callq  mcount
               movzwl 0x6(%rsi),%edx
               and    $0x7fff,%dx
               test   %rdi,%rdi
       ┌─────↓ jne    20
       │  17:  xor    %eax,%eax
       │  19:  leaveq
       │     ← retq
       │       nopl   0x0(%rax,%rax,1)
       └─→20:  mov    (%rdi),%rax
               test   %rax,%rax
             ↑ je     17
               movzwl (%rsi),%ecx
               movzwl 0x2(%rsi),%r9d
               movzwl 0x4(%rsi),%r8d
               movzwl %cx,%esi
               movzwl %r9w,%r10d
               shl    $0x9,%esi
               lea    (%rsi,%r10,4),%esi
               lea    (%r8,%rsi,1),%esi
               and    0x10(%rdi),%si
               movzwl %si,%esi
               mov    (%rax,%rsi,8),%rax
  1.63         test   %rax,%rax
             ↑ je     19
               nopw   0x0(%rax,%rax,1)
  4.88    60:  cmp    %cx,(%rax)
             ↓ jne    7e
               cmp    %r9w,0x2(%rax)
             ↓ jne    7e
               cmp    %r8w,0x4(%rax)
             ↓ jne    79
               test   %dx,0x6(%rax)
             ↑ jne    19
          79:  cmp    %r8w,0x4(%rax)
 86.99    7e:↑ ja     17
  3.25         mov    0x10(%rax),%rax
  3.25         test   %rax,%rax
             ↑ jne    60
               leaveq
             ← retq

. Changes:

	- Don't show the big vertical line.

        - Add an arrow to the right before call instructions

        - Scrap bogus loop detection and instead start showing
          arrows from jump (fwd or back) instructions to its targets
          when cursor is on jump instruction. Press 'j' to toggle this.

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

----------------------------------------------------------------
Arnaldo Carvalho de Melo (5):
      perf annotate browser: Add a right arrow before call instructions
      perf ui browser: Add method to draw up/down arrow line
      perf annotate browser: Show current jump, back or forward
      perf annotate browser: Remove the vertical line after the percentages
      perf annotate browser: Don't display 0.00 percentages

 tools/perf/ui/browser.c           |   54 ++++++++++++++++++++++++++++--
 tools/perf/ui/browser.h           |    4 +--
 tools/perf/ui/browsers/annotate.c |   66 ++++++++++++++++++-------------------
 3 files changed, 86 insertions(+), 38 deletions(-)

[PATCH 1/5] perf annotate browser: Add a right arrow before call instructions

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

The counterpart of 'ret' instructions.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/n/tip-jlz2ldaquaow0rqi2vr4b91l@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/annotate.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 077380b..4778172 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -117,6 +117,9 @@ static void annotate_browser__write(struct ui_browser *self, void *entry, int ro
 				ui_browser__write_graph(self, fwd ? SLSMG_DARROW_CHAR :
 								    SLSMG_UARROW_CHAR);
 				SLsmg_write_char(' ');
+			} else if (ins__is_call(dl->ins)) {
+				ui_browser__write_graph(self, SLSMG_RARROW_CHAR);
+				SLsmg_write_char(' ');
 			} else {
 				slsmg_write_nstring(" ", 2);
 			}
-- 
1.7.9.2.358.g22243

[PATCH 2/5] perf ui browser: Add method to draw up/down arrow line

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

It figures out the direction and draws downwards arrows too if that is
the case.

Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/n/tip-tg329nr7q4dg9d0tl3o0wywg@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browser.c |   54 +++++++++++++++++++++++++++++++++++++++++++++--
 tools/perf/ui/browser.h |    4 ++--
 2 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
index 32ac116..f4b2530 100644
--- a/tools/perf/ui/browser.c
+++ b/tools/perf/ui/browser.c
@@ -600,8 +600,9 @@ void ui_browser__write_graph(struct ui_browser *browser __used, int graph)
 	SLsmg_set_char_set(0);
 }
 
-void __ui_browser__line_arrow_up(struct ui_browser *browser, unsigned int column,
-				 u64 start, u64 end, int start_width)
+static void __ui_browser__line_arrow_up(struct ui_browser *browser,
+					unsigned int column,
+					u64 start, u64 end, int start_width)
 {
 	unsigned int row, end_row;
 
@@ -639,6 +640,55 @@ out:
 	SLsmg_set_char_set(0);
 }
 
+static void __ui_browser__line_arrow_down(struct ui_browser *browser,
+					  unsigned int column,
+					  u64 start, u64 end, int start_width)
+{
+	unsigned int row, end_row;
+
+	SLsmg_set_char_set(1);
+
+	if (start >= browser->top_idx) {
+		row = start - browser->top_idx;
+		ui_browser__gotorc(browser, row, column);
+		SLsmg_write_char(SLSMG_ULCORN_CHAR);
+		ui_browser__gotorc(browser, row, column + 1);
+		SLsmg_draw_hline(start_width);
+
+		if (row++ == 0)
+			goto out;
+	} else
+		row = 0;
+
+	if (end >= browser->top_idx + browser->height)
+		end_row = browser->height - 1;
+	else
+		end_row = end - browser->top_idx;;
+
+	ui_browser__gotorc(browser, row, column);
+	SLsmg_draw_vline(end_row - row + 1);
+
+	ui_browser__gotorc(browser, end_row, column);
+	if (end < browser->top_idx + browser->height) {
+		SLsmg_write_char(SLSMG_LLCORN_CHAR);
+		ui_browser__gotorc(browser, end_row, column + 1);
+		SLsmg_write_char(SLSMG_HLINE_CHAR);
+		ui_browser__gotorc(browser, end_row, column + 2);
+		SLsmg_write_char(SLSMG_RARROW_CHAR);
+	}
+out:
+	SLsmg_set_char_set(0);
+}
+
+void __ui_browser__line_arrow(struct ui_browser *browser, unsigned int column,
+			      u64 start, u64 end, int start_width)
+{
+	if (start > end)
+		__ui_browser__line_arrow_up(browser, column, start, end, start_width);
+	else
+		__ui_browser__line_arrow_down(browser, column, start, end, start_width);
+}
+
 void ui_browser__init(void)
 {
 	int i = 0;
diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
index 2f226cb..059764b 100644
--- a/tools/perf/ui/browser.h
+++ b/tools/perf/ui/browser.h
@@ -38,8 +38,8 @@ void ui_browser__reset_index(struct ui_browser *self);
 
 void ui_browser__gotorc(struct ui_browser *self, int y, int x);
 void ui_browser__write_graph(struct ui_browser *browser, int graph);
-void __ui_browser__line_arrow_up(struct ui_browser *browser, unsigned int column,
-				 u64 start, u64 end, int start_width);
+void __ui_browser__line_arrow(struct ui_browser *browser, unsigned int column,
+			      u64 start, u64 end, int start_width);
 void __ui_browser__show_title(struct ui_browser *browser, const char *title);
 void ui_browser__show_title(struct ui_browser *browser, const char *title);
 int ui_browser__show(struct ui_browser *self, const char *title,
-- 
1.7.9.2.358.g22243

[PATCH 3/5] perf annotate browser: Show current jump, back or forward

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

Instead of trying to show the current loop by naively looking for the
next backward jump, just use 'j' to toggle showing arrows connecting
jump with its target.

And do it for forward jumps as well.

Loop detection requires more code to follow the flow control, etc.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/n/tip-soahcn1lz2u4wxj31ch0594j@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/annotate.c |   50 +++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 27 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 4778172..d203daf 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -30,6 +30,7 @@ struct annotate_browser {
 	int		    nr_entries;
 	bool		    hide_src_code;
 	bool		    use_offset;
+	bool		    jump_arrows;
 	bool		    searching_backwards;
 	u8		    offset_width;
 	char		    search_bf[128];
@@ -144,56 +145,47 @@ static void annotate_browser__write(struct ui_browser *self, void *entry, int ro
 		ab->selection = dl;
 }
 
-static void annotate_browser__draw_current_loop(struct ui_browser *browser)
+static void annotate_browser__draw_current_jump(struct ui_browser *browser)
 {
 	struct annotate_browser *ab = container_of(browser, struct annotate_browser, b);
-	struct map_symbol *ms = browser->priv;
-	struct symbol *sym = ms->sym;
-	struct annotation *notes = symbol__annotation(sym);
-	struct disasm_line *cursor = ab->selection, *pos = cursor, *target;
-	struct browser_disasm_line *bcursor = disasm_line__browser(cursor),
-				   *btarget, *bpos;
+	struct disasm_line *cursor = ab->selection, *target;
+	struct browser_disasm_line *btarget, *bcursor;
 	unsigned int from, to, start_width = 2;
 
-	list_for_each_entry_from(pos, &notes->src->source, node) {
-		if (!pos->ins || !ins__is_jump(pos->ins) ||
-		    !disasm_line__has_offset(pos))
-			continue;
-
-		target = ab->offsets[pos->ops.target.offset];
-		if (!target)
-			continue;
+	if (!cursor->ins || !ins__is_jump(cursor->ins) ||
+	    !disasm_line__has_offset(cursor))
+		return;
 
-		btarget = disasm_line__browser(target);
-		if (btarget->idx <= bcursor->idx)
-			goto found;
-	}
+	target = ab->offsets[cursor->ops.target.offset];
+	if (!target)
+		return;
 
-	return;
+	bcursor = disasm_line__browser(cursor);
+	btarget = disasm_line__browser(target);
 
-found:
-	bpos = disasm_line__browser(pos);
 	if (ab->hide_src_code) {
-		from = bpos->idx_asm;
+		from = bcursor->idx_asm;
 		to = btarget->idx_asm;
 	} else {
-		from = (u64)bpos->idx;
+		from = (u64)bcursor->idx;
 		to = (u64)btarget->idx;
 	}
 
 	ui_browser__set_color(browser, HE_COLORSET_CODE);
 
-	if (!bpos->jump_target)
+	if (!bcursor->jump_target)
 		start_width += ab->offset_width + 1;
 
-	__ui_browser__line_arrow_up(browser, 10, from, to, start_width);
+	__ui_browser__line_arrow(browser, 10, from, to, start_width);
 }
 
 static unsigned int annotate_browser__refresh(struct ui_browser *browser)
 {
+	struct annotate_browser *ab = container_of(browser, struct annotate_browser, b);
 	int ret = ui_browser__list_head_refresh(browser);
 
-	annotate_browser__draw_current_loop(browser);
+	if (ab->jump_arrows)
+		annotate_browser__draw_current_jump(browser);
 
 	return ret;
 }
@@ -628,6 +620,9 @@ static int annotate_browser__run(struct annotate_browser *self, int evidx,
 		case 'o':
 			self->use_offset = !self->use_offset;
 			continue;
+		case 'j':
+			self->jump_arrows = !self->jump_arrows;
+			continue;
 		case '/':
 			if (annotate_browser__search(self, delay_secs)) {
 show_help:
@@ -739,6 +734,7 @@ int symbol__tui_annotate(struct symbol *sym, struct map *map, int evidx,
 			.use_navkeypressed = true,
 		},
 		.use_offset = true,
+		.jump_arrows = true,
 	};
 	int ret = -1;
 
-- 
1.7.9.2.358.g22243

[PATCH 4/5] perf annotate browser: Remove the vertical line after the percentages

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

It is confusing when used with jump -> target lines.

Requested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/n/tip-xeiyfsxptwtmlvowledg6wpy@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/annotate.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index d203daf..a90680b 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -72,7 +72,6 @@ static void annotate_browser__write(struct ui_browser *self, void *entry, int ro
 		slsmg_write_nstring(" ", 9);
 	}
 
-	ui_browser__write_graph(self, SLSMG_VLINE_CHAR);
 	SLsmg_write_char(' ');
 
 	/* The scroll bar isn't being used */
@@ -83,9 +82,9 @@ static void annotate_browser__write(struct ui_browser *self, void *entry, int ro
 		ui_browser__set_color(self, HE_COLORSET_CODE);
 
 	if (!*dl->line)
-		slsmg_write_nstring(" ", width - 10);
+		slsmg_write_nstring(" ", width - 9);
 	else if (dl->offset == -1)
-		slsmg_write_nstring(dl->line, width - 10);
+		slsmg_write_nstring(dl->line, width - 9);
 	else {
 		char bf[256];
 		u64 addr = dl->offset;
@@ -138,7 +137,7 @@ static void annotate_browser__write(struct ui_browser *self, void *entry, int ro
 			scnprintf(bf, sizeof(bf), "%-6.6s %s", dl->name, dl->ops.raw);
 		}
 
-		slsmg_write_nstring(bf, width - 12 - printed);
+		slsmg_write_nstring(bf, width - 11 - printed);
 	}
 
 	if (current_entry)
@@ -176,7 +175,7 @@ static void annotate_browser__draw_current_jump(struct ui_browser *browser)
 	if (!bcursor->jump_target)
 		start_width += ab->offset_width + 1;
 
-	__ui_browser__line_arrow(browser, 10, from, to, start_width);
+	__ui_browser__line_arrow(browser, 9, from, to, start_width);
 }
 
 static unsigned int annotate_browser__refresh(struct ui_browser *browser)
-- 
1.7.9.2.358.g22243

[PATCH 5/5] perf annotate browser: Don't display 0.00 percentages

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

Cleaning up more the output.

Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/n/tip-81pimnsnaa9y2j0a9plstu1c@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/annotate.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index a90680b..44fb6a4 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -64,12 +64,12 @@ static void annotate_browser__write(struct ui_browser *self, void *entry, int ro
 					         !self->navkeypressed)));
 	int width = self->width;
 
-	if (dl->offset != -1) {
+	if (dl->offset != -1 && bdl->percent != 0.0) {
 		ui_browser__set_percent_color(self, bdl->percent, current_entry);
-		slsmg_printf(" %7.2f ", bdl->percent);
+		slsmg_printf("%6.2f ", bdl->percent);
 	} else {
 		ui_browser__set_percent_color(self, 0, current_entry);
-		slsmg_write_nstring(" ", 9);
+		slsmg_write_nstring(" ", 7);
 	}
 
 	SLsmg_write_char(' ');
@@ -82,9 +82,9 @@ static void annotate_browser__write(struct ui_browser *self, void *entry, int ro
 		ui_browser__set_color(self, HE_COLORSET_CODE);
 
 	if (!*dl->line)
-		slsmg_write_nstring(" ", width - 9);
+		slsmg_write_nstring(" ", width - 7);
 	else if (dl->offset == -1)
-		slsmg_write_nstring(dl->line, width - 9);
+		slsmg_write_nstring(dl->line, width - 7);
 	else {
 		char bf[256];
 		u64 addr = dl->offset;
@@ -137,7 +137,7 @@ static void annotate_browser__write(struct ui_browser *self, void *entry, int ro
 			scnprintf(bf, sizeof(bf), "%-6.6s %s", dl->name, dl->ops.raw);
 		}
 
-		slsmg_write_nstring(bf, width - 11 - printed);
+		slsmg_write_nstring(bf, width - 9 - printed);
 	}
 
 	if (current_entry)
@@ -175,7 +175,7 @@ static void annotate_browser__draw_current_jump(struct ui_browser *browser)
 	if (!bcursor->jump_target)
 		start_width += ab->offset_width + 1;
 
-	__ui_browser__line_arrow(browser, 9, from, to, start_width);
+	__ui_browser__line_arrow(browser, 7, from, to, start_width);
 }
 
 static unsigned int annotate_browser__refresh(struct ui_browser *browser)
-- 
1.7.9.2.358.g22243

Re: [GIT PULL 0/5] perf/annotate fixes and improvements

[Peter Zijlstra]

On Wed, 2012-05-02 at 16:42 -0300, Arnaldo Carvalho de Melo wrote:
> 
> avtab_search_node
>                push   %rbp
>                mov    %rsp,%rbp
>              → callq  mcount
>                movzwl 0x6(%rsi),%edx
>                and    $0x7fff,%dx
>                test   %rdi,%rdi
>        ┌─────↓ jne    20
>        │  17:  xor    %eax,%eax
>        │  19:  leaveq
>        │     ← retq
>        │       nopl   0x0(%rax,%rax,1)
>        └─→20:  mov    (%rdi),%rax
>                test   %rax,%rax
>              ↑ je     17
>                movzwl (%rsi),%ecx
>                movzwl 0x2(%rsi),%r9d
>                movzwl 0x4(%rsi),%r8d
>                movzwl %cx,%esi
>                movzwl %r9w,%r10d
>                shl    $0x9,%esi
>                lea    (%rsi,%r10,4),%esi
>                lea    (%r8,%rsi,1),%esi
>                and    0x10(%rdi),%si
>                movzwl %si,%esi
>                mov    (%rax,%rsi,8),%rax
>   1.63         test   %rax,%rax
>              ↑ je     19
>                nopw   0x0(%rax,%rax,1)
>   4.88    60:  cmp    %cx,(%rax)
>              ↓ jne    7e
>                cmp    %r9w,0x2(%rax)
>              ↓ jne    7e
>                cmp    %r8w,0x4(%rax)
>              ↓ jne    79
>                test   %dx,0x6(%rax)
>              ↑ jne    19
>           79:  cmp    %r8w,0x4(%rax)
>  86.99    7e:↑ ja     17
>   3.25         mov    0x10(%rax),%rax
>   3.25         test   %rax,%rax
>              ↑ jne    60
>                leaveq
>              ← retq
> 
> . Changes:
> 
>         - Don't show the big vertical line. 

Not sure about that, loosing that separator makes it looks messy.

Re: [GIT PULL 0/5] perf/annotate fixes and improvements

[Arnaldo Carvalho de Melo]

Em Wed, May 02, 2012 at 09:46:43PM +0200, Peter Zijlstra escreveu:
> On Wed, 2012-05-02 at 16:42 -0300, Arnaldo Carvalho de Melo wrote:
> > 
> > avtab_search_node
> >                push   %rbp
> >                mov    %rsp,%rbp
> >              → callq  mcount
> >                movzwl 0x6(%rsi),%edx
> >                and    $0x7fff,%dx
> >                test   %rdi,%rdi
> >        ┌─────↓ jne    20
> >        │  17:  xor    %eax,%eax
> >        │  19:  leaveq
> >        │     ← retq
> >        │       nopl   0x0(%rax,%rax,1)
> >        └─→20:  mov    (%rdi),%rax
> >                test   %rax,%rax
> >              ↑ je     17
> >                movzwl (%rsi),%ecx
> >                movzwl 0x2(%rsi),%r9d
> >                movzwl 0x4(%rsi),%r8d
> >                movzwl %cx,%esi
> >                movzwl %r9w,%r10d
> >                shl    $0x9,%esi
> >                lea    (%rsi,%r10,4),%esi
> >                lea    (%r8,%rsi,1),%esi
> >                and    0x10(%rdi),%si
> >                movzwl %si,%esi
> >                mov    (%rax,%rsi,8),%rax
> >   1.63         test   %rax,%rax
> >              ↑ je     19
> >                nopw   0x0(%rax,%rax,1)
> >   4.88    60:  cmp    %cx,(%rax)
> >              ↓ jne    7e
> >                cmp    %r9w,0x2(%rax)
> >              ↓ jne    7e
> >                cmp    %r8w,0x4(%rax)
> >              ↓ jne    79
> >                test   %dx,0x6(%rax)
> >              ↑ jne    19
> >           79:  cmp    %r8w,0x4(%rax)
> >  86.99    7e:↑ ja     17
> >   3.25         mov    0x10(%rax),%rax
> >   3.25         test   %rax,%rax
> >              ↑ jne    60
> >                leaveq
> >              ← retq
> > 
> > . Changes:
> > 
> >         - Don't show the big vertical line. 
> 
> Not sure about that, loosing that separator makes it looks messy.

It was a request from Linus:

commit 3e8b5ddf17d4639d41bc57ecfb51633815b70e49
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Fri Apr 27 16:44:56 2012 -0300

    perf annotate browser: Remove the vertical line after the percentages
    
    It is confusing when used with jump -> target lines.
    
    Requested-by: Linus Torvalds <torvalds@linux-foundation.org>

Make it configurable? Press 'S' and you get a separator? Linus?

- Arnaldo

Re: [GIT PULL 0/5] perf/annotate fixes and improvements

[Arnaldo Carvalho de Melo]

[-- Attachment #1: Type: text/plain, Size: 2435 bytes --]

Em Wed, May 02, 2012 at 09:46:43PM +0200, Peter Zijlstra escreveu:
> On Wed, 2012-05-02 at 16:42 -0300, Arnaldo Carvalho de Melo wrote:
> > . Changes:
> > 
> >         - Don't show the big vertical line. 
> 
> Not sure about that, loosing that separator makes it looks messy.

How about what we discussed on IRC:

avtab_search_node
       │      push   %rbp
       │      mov    %rsp,%rbp
       │    → callq  mcount
       │      movzwl 0x6(%rsi),%edx
       │      and    $0x7fff,%dx
       │      test   %rdi,%rdi
       │    ↓ jne    20
  0.64 │17:┌─→xor    %eax,%eax
       │19:│  leaveq
  0.51 │   │← retq
       │   │  nopl   0x0(%rax,%rax,1)
       │20:│  mov    (%rdi),%rax
       │   │  test   %rax,%rax
       │   └──je     17
       │      movzwl (%rsi),%ecx
       │      movzwl 0x2(%rsi),%r9d
       │      movzwl 0x4(%rsi),%r8d
       │      movzwl %cx,%esi
       │      movzwl %r9w,%r10d
       │      shl    $0x9,%esi
       │      lea    (%rsi,%r10,4),%esi
       │      lea    (%r8,%rsi,1),%esi
       │      and    0x10(%rdi),%si
       │      movzwl %si,%esi
       │      mov    (%rax,%rsi,8),%rax
  0.89 │      test   %rax,%rax
       │    ↑ je     19
       │      nopw   0x0(%rax,%rax,1)
  3.44 │60:   cmp    %cx,(%rax)
       │    ↓ jne    7e
       │      cmp    %r9w,0x2(%rax)
       │    ↓ jne    7e
       │      cmp    %r8w,0x4(%rax)
       │    ↓ jne    79
       │      test   %dx,0x6(%rax)
       │    ↑ jne    19
       │79:   cmp    %r8w,0x4(%rax)
 83.46 │7e: ↑ ja     17
  3.82 │      mov    0x10(%rax),%rax
  7.25 │      test   %rax,%rax
       │    ↑ jne    60
       │      leaveq
       │    ← retq

I.e. the fixed vertical line now has a diff color and the jump arrows are
_after_ the jump labels that then stands out as a separated columns.

In addition, as you suggested, the extra arrows on the ends of a jump->label
arrow gets swallowed by the jump->label arrow.

Also the fixed vertical line is drawn after we refresh the lines, i.e.,
it has a solid color, not influenced by each line percentages, as Linus
noticed previously.

Now a question: when I add multiple event column overheads, do you think we
should have N fixed vertical lines separating them?

I probably need to add an space before the instructions and the arrow
start/end, or not?

- Arnaldo

[-- Attachment #2: annotate.patch --]
[-- Type: text/plain, Size: 6524 bytes --]

diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
index f4b2530..562499c 100644
--- a/tools/perf/ui/browser.c
+++ b/tools/perf/ui/browser.c
@@ -602,7 +602,7 @@ void ui_browser__write_graph(struct ui_browser *browser __used, int graph)
 
 static void __ui_browser__line_arrow_up(struct ui_browser *browser,
 					unsigned int column,
-					u64 start, u64 end, int start_width)
+					u64 start, u64 end)
 {
 	unsigned int row, end_row;
 
@@ -613,7 +613,7 @@ static void __ui_browser__line_arrow_up(struct ui_browser *browser,
 		ui_browser__gotorc(browser, row, column);
 		SLsmg_write_char(SLSMG_LLCORN_CHAR);
 		ui_browser__gotorc(browser, row, column + 1);
-		SLsmg_draw_hline(start_width);
+		SLsmg_draw_hline(2);
 
 		if (row-- == 0)
 			goto out;
@@ -642,7 +642,7 @@ out:
 
 static void __ui_browser__line_arrow_down(struct ui_browser *browser,
 					  unsigned int column,
-					  u64 start, u64 end, int start_width)
+					  u64 start, u64 end)
 {
 	unsigned int row, end_row;
 
@@ -653,7 +653,7 @@ static void __ui_browser__line_arrow_down(struct ui_browser *browser,
 		ui_browser__gotorc(browser, row, column);
 		SLsmg_write_char(SLSMG_ULCORN_CHAR);
 		ui_browser__gotorc(browser, row, column + 1);
-		SLsmg_draw_hline(start_width);
+		SLsmg_draw_hline(2);
 
 		if (row++ == 0)
 			goto out;
@@ -681,12 +681,21 @@ out:
 }
 
 void __ui_browser__line_arrow(struct ui_browser *browser, unsigned int column,
-			      u64 start, u64 end, int start_width)
+			      u64 start, u64 end)
 {
 	if (start > end)
-		__ui_browser__line_arrow_up(browser, column, start, end, start_width);
+		__ui_browser__line_arrow_up(browser, column, start, end);
 	else
-		__ui_browser__line_arrow_down(browser, column, start, end, start_width);
+		__ui_browser__line_arrow_down(browser, column, start, end);
+}
+
+void __ui_browser__vline(struct ui_browser *browser, unsigned int column,
+			 u16 start, u16 end)
+{
+	SLsmg_set_char_set(1);
+	ui_browser__gotorc(browser, start, column);
+	SLsmg_draw_vline(end - start + 1);
+	SLsmg_set_char_set(0);
 }
 
 void ui_browser__init(void)
diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
index 059764b..133432d 100644
--- a/tools/perf/ui/browser.h
+++ b/tools/perf/ui/browser.h
@@ -39,7 +39,9 @@ void ui_browser__reset_index(struct ui_browser *self);
 void ui_browser__gotorc(struct ui_browser *self, int y, int x);
 void ui_browser__write_graph(struct ui_browser *browser, int graph);
 void __ui_browser__line_arrow(struct ui_browser *browser, unsigned int column,
-			      u64 start, u64 end, int start_width);
+			      u64 start, u64 end);
+void __ui_browser__vline(struct ui_browser *browser, unsigned int column,
+			 u16 start, u16 end);
 void __ui_browser__show_title(struct ui_browser *browser, const char *title);
 void ui_browser__show_title(struct ui_browser *browser, const char *title);
 int ui_browser__show(struct ui_browser *self, const char *title,
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 44fb6a4..931db9f 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -62,7 +62,8 @@ static void annotate_browser__write(struct ui_browser *self, void *entry, int ro
 	bool change_color = (!ab->hide_src_code &&
 			     (!current_entry || (self->use_navkeypressed &&
 					         !self->navkeypressed)));
-	int width = self->width;
+	int width = self->width, printed;
+	char bf[256];
 
 	if (dl->offset != -1 && bdl->percent != 0.0) {
 		ui_browser__set_percent_color(self, bdl->percent, current_entry);
@@ -83,24 +84,26 @@ static void annotate_browser__write(struct ui_browser *self, void *entry, int ro
 
 	if (!*dl->line)
 		slsmg_write_nstring(" ", width - 7);
-	else if (dl->offset == -1)
-		slsmg_write_nstring(dl->line, width - 7);
-	else {
-		char bf[256];
+	else if (dl->offset == -1) {
+		printed = scnprintf(bf, sizeof(bf), "%*s ",
+				    ab->offset_width, " ");
+		slsmg_write_nstring(bf, printed);
+		slsmg_write_nstring(dl->line, width - printed - 5);
+	} else {
 		u64 addr = dl->offset;
-		int printed, color = -1;
+		int color = -1;
 
 		if (!ab->use_offset)
 			addr += ab->start;
 
 		if (!ab->use_offset) {
-			printed = scnprintf(bf, sizeof(bf), "  %" PRIx64 ":", addr);
+			printed = scnprintf(bf, sizeof(bf), "%" PRIx64 ": ", addr);
 		} else {
 			if (bdl->jump_target) {
-				printed = scnprintf(bf, sizeof(bf), "  %*" PRIx64 ":",
+				printed = scnprintf(bf, sizeof(bf), "%*" PRIx64 ": ",
 						    ab->offset_width, addr);
 			} else {
-				printed = scnprintf(bf, sizeof(bf), "  %*s ",
+				printed = scnprintf(bf, sizeof(bf), "%*s  ",
 						    ab->offset_width, " ");
 			}
 		}
@@ -137,7 +140,7 @@ static void annotate_browser__write(struct ui_browser *self, void *entry, int ro
 			scnprintf(bf, sizeof(bf), "%-6.6s %s", dl->name, dl->ops.raw);
 		}
 
-		slsmg_write_nstring(bf, width - 9 - printed);
+		slsmg_write_nstring(bf, width - 10 - printed);
 	}
 
 	if (current_entry)
@@ -149,7 +152,7 @@ static void annotate_browser__draw_current_jump(struct ui_browser *browser)
 	struct annotate_browser *ab = container_of(browser, struct annotate_browser, b);
 	struct disasm_line *cursor = ab->selection, *target;
 	struct browser_disasm_line *btarget, *bcursor;
-	unsigned int from, to, start_width = 2;
+	unsigned int from, to;
 
 	if (!cursor->ins || !ins__is_jump(cursor->ins) ||
 	    !disasm_line__has_offset(cursor))
@@ -171,11 +174,7 @@ static void annotate_browser__draw_current_jump(struct ui_browser *browser)
 	}
 
 	ui_browser__set_color(browser, HE_COLORSET_CODE);
-
-	if (!bcursor->jump_target)
-		start_width += ab->offset_width + 1;
-
-	__ui_browser__line_arrow(browser, 7, from, to, start_width);
+	__ui_browser__line_arrow(browser, 9 + ab->offset_width, from, to);
 }
 
 static unsigned int annotate_browser__refresh(struct ui_browser *browser)
@@ -186,6 +185,8 @@ static unsigned int annotate_browser__refresh(struct ui_browser *browser)
 	if (ab->jump_arrows)
 		annotate_browser__draw_current_jump(browser);
 
+	ui_browser__set_color(browser, HE_COLORSET_NORMAL);
+	__ui_browser__vline(browser, 7, 0, browser->height - 1);
 	return ret;
 }
 
@@ -618,6 +619,10 @@ static int annotate_browser__run(struct annotate_browser *self, int evidx,
 		case 'O':
 		case 'o':
 			self->use_offset = !self->use_offset;
+			if (self->use_offset)
+				self->offset_width = hex_width(symbol__size(sym));
+			else
+				self->offset_width = hex_width(sym->end);
 			continue;
 		case 'j':
 			self->jump_arrows = !self->jump_arrows;

Re: [GIT PULL 0/5] perf/annotate fixes and improvements

[Linus Torvalds]

On Wed, May 2, 2012 at 2:18 PM, Arnaldo Carvalho de Melo
<acme@infradead.org> wrote:
>
> Also the fixed vertical line is drawn after we refresh the lines, i.e.,
> it has a solid color, not influenced by each line percentages, as Linus
> noticed previously.

Looks good to me.  That said, I do wonder whether the vertical line
really is needed (or maybe it could be at the last decimal, so it only
shows for the instructions without percentages).

But with the color fixed, I doubt it annoys me nearly as much as it
used to. And the other cleanups have already meant that the horizontal
space is not as scarce a resource as it used to be.

                   Linus

Re: [GIT PULL 0/5] perf/annotate fixes and improvements

[Peter Zijlstra]

On Wed, 2012-05-02 at 18:18 -0300, Arnaldo Carvalho de Melo wrote:
> I.e. the fixed vertical line now has a diff color and the jump arrows are
> _after_ the jump labels that then stands out as a separated columns.

Note that if you have the source-code annotated thing enabled (which
seems to be the default) the arrows go straight through the source.

Also, in this case the asm is dark blue, which is nearly invisible on my
black background.

> In addition, as you suggested, the extra arrows on the ends of a jump->label
> arrow gets swallowed by the jump->label arrow.

Does look nice, thanks!

An alternative to all this arrow drawing could be to high-light the
jump-target, maybe not a full bar like the cursor line but something
like that.

Also, is there a key to jump to the jump target? I was instinctively
pressing '%' to do that, but that could be my vim brain-damage :-)

> Now a question: when I add multiple event column overheads, do you think we
> should have N fixed vertical lines separating them?

I'd start with that and go from there.

> I probably need to add an space before the instructions and the arrow
> start/end, or not? 

Since its a dynamic arrow (in a nearly invisible colour) it doesn't
really matter. But what happens when a function is large enough that
function offset doesn't fit in 2 hex digits anymore? Aah I see, that
width is dynamic, OK.

Re: [GIT PULL 0/5] perf/annotate fixes and improvements

[Arnaldo Carvalho de Melo]

Em Thu, May 03, 2012 at 10:01:40AM +0200, Peter Zijlstra escreveu:
> On Wed, 2012-05-02 at 18:18 -0300, Arnaldo Carvalho de Melo wrote:
> > I.e. the fixed vertical line now has a diff color and the jump arrows are
> > _after_ the jump labels that then stands out as a separated columns.
> 
> Note that if you have the source-code annotated thing enabled (which
> seems to be the default) the arrows go straight through the source.

I thought I had that fixed, damn, will fix.

> Also, in this case the asm is dark blue, which is nearly invisible on my
> black background.

What colour do you suggest? Also what kind of term color scheme do you
use? Finding the right default combo is kinda hard.
 
> > In addition, as you suggested, the extra arrows on the ends of a jump->label
> > arrow gets swallowed by the jump->label arrow.
> 
> Does look nice, thanks!
> 
> An alternative to all this arrow drawing could be to high-light the
> jump-target, maybe not a full bar like the cursor line but something
> like that.
> 
> Also, is there a key to jump to the jump target? I was instinctively
> pressing '%' to do that, but that could be my vim brain-damage :-)

Enter or ->, just like on calls and rets, what is missing is using <- to
go back the stack of navigated jumps, like with calls.
 
> > Now a question: when I add multiple event column overheads, do you think we
> > should have N fixed vertical lines separating them?
> 
> I'd start with that and go from there.

Ok
 
> > I probably need to add an space before the instructions and the arrow
> > start/end, or not? 
> 
> Since its a dynamic arrow (in a nearly invisible colour) it doesn't
> really matter. But what happens when a function is large enough that
> function offset doesn't fit in 2 hex digits anymore? Aah I see, that
> width is dynamic, OK.

Yes, it is, it is calculated at function start and when switching
to/from "O"ffset view.

I need to add a help window with the ever growing list of hot keys.

Also will try to asap make the last features toggled persistent.

- Arnaldo

Re: [GIT PULL 0/5] perf/annotate fixes and improvements

[Peter Zijlstra]

On Thu, 2012-05-03 at 10:05 -0300, Arnaldo Carvalho de Melo wrote:
> > Also, in this case the asm is dark blue, which is nearly invisible on my
> > black background.
> 
> What colour do you suggest? Also what kind of term color scheme do you
> use? Finding the right default combo is kinda hard. 

Yeah, I know.. I use white text on black background.

Typically for vim I use :se bg=dark

Anyway, I find it weird that the asm is coloured completely different
between the two modes. If anything I would keep the asm as in the
non-source variant and when you add the source, make that a different
colour.

Re: [GIT PULL 0/5] perf/annotate fixes and improvements

[Namhyung Kim]

Hi,

2012-05-03 (목), 15:12 +0200, Peter Zijlstra:
> On Thu, 2012-05-03 at 10:05 -0300, Arnaldo Carvalho de Melo wrote:
> > > Also, in this case the asm is dark blue, which is nearly invisible on my
> > > black background.
> > 
> > What colour do you suggest? Also what kind of term color scheme do you
> > use? Finding the right default combo is kinda hard. 
> 
> Yeah, I know.. I use white text on black background.
> 
> Typically for vim I use :se bg=dark
> 
> Anyway, I find it weird that the asm is coloured completely different
> between the two modes. If anything I would keep the asm as in the
> non-source variant and when you add the source, make that a different
> colour.
> 

It was changed on commit 58e817d997d1 ("perf annotate: Print asm code as
blue when source code is displayed") by me. I just tried to make the
output as same as the --stdio one, but I felt bad on printing the whole
asm lines as blue when no source is shown - I use similar color scheme
of yours - so I just ended up leaving it to the default white.

Thanks,
Namhyung

Re: [GIT PULL 0/5] perf/annotate fixes and improvements

[Arnaldo Carvalho de Melo]

Em Thu, May 03, 2012 at 03:12:14PM +0200, Peter Zijlstra escreveu:
> On Thu, 2012-05-03 at 10:05 -0300, Arnaldo Carvalho de Melo wrote:
> > > Also, in this case the asm is dark blue, which is nearly invisible on my
> > > black background.
> > 
> > What colour do you suggest? Also what kind of term color scheme do you
> > use? Finding the right default combo is kinda hard. 
> 
> Yeah, I know.. I use white text on black background.
> 
> Typically for vim I use :se bg=dark
> 
> Anyway, I find it weird that the asm is coloured completely different
> between the two modes. If anything I would keep the asm as in the
> non-source variant and when you add the source, make that a different
> colour.

Its a bug, it should be coloured the same in both modes. Will implement
as you suggest.

- Arnaldo

Re: [GIT PULL 0/5] perf/annotate fixes and improvements

[Arnaldo Carvalho de Melo]

Em Thu, May 03, 2012 at 11:11:07PM +0900, Namhyung Kim escreveu:
> Hi,
> 
> 2012-05-03 (목), 15:12 +0200, Peter Zijlstra:
> > On Thu, 2012-05-03 at 10:05 -0300, Arnaldo Carvalho de Melo wrote:
> > > > Also, in this case the asm is dark blue, which is nearly invisible on my
> > > > black background.
> > > 
> > > What colour do you suggest? Also what kind of term color scheme do you
> > > use? Finding the right default combo is kinda hard. 
> > 
> > Yeah, I know.. I use white text on black background.
> > 
> > Typically for vim I use :se bg=dark
> > 
> > Anyway, I find it weird that the asm is coloured completely different
> > between the two modes. If anything I would keep the asm as in the
> > non-source variant and when you add the source, make that a different
> > colour.
> > 
> 
> It was changed on commit 58e817d997d1 ("perf annotate: Print asm code as
> blue when source code is displayed") by me. I just tried to make the
> output as same as the --stdio one, but I felt bad on printing the whole
> asm lines as blue when no source is shown - I use similar color scheme
> of yours - so I just ended up leaving it to the default white.

I'll revert that one, I agree with Peter that we should have the same
color for asm with/without source code.

If you feel like source code should be in another color, then try to
find one that works well with some common term color schemes and propose
a patch.

- Arnaldo