[PATCHSET 0/7] perf tools: Assorted cleanup for TUI (v1)

[PATCHSET 0/7] perf tools: Assorted cleanup for TUI (v1)

[Namhyung Kim]

Hello,

This patches are to cleanup TUI hists browser code for later work.  I
moved hist_entry_diff and hist_entry_tui under an union in order to
reduce memory footprint of hist entry.  Also split out hist browser
functions to make it easier to read.

It's available on 'perf/tui-cleanup-v1' branch in my tree:

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung


Namhyung Kim (7):
  perf tools: Get rid of position field from struct hist_entry
  perf diff: Make hist_entry_diff fields union
  perf tools: Move TUI-specific fields to struct hist_entry_tui
  perf tools: Move init_have_children field to struct hist_entry_tui
  perf hists browser: Fix possible memory leak
  perf hists browser: Split popup menu actions
  perf hists browser: Simplify zooming code a bit

 tools/perf/ui/browsers/hists.c | 612 ++++++++++++++++++++++++++---------------
 tools/perf/util/hist.c         |   4 +-
 tools/perf/util/pstack.c       |   7 +
 tools/perf/util/pstack.h       |   1 +
 tools/perf/util/sort.h         |  31 ++-
 5 files changed, 411 insertions(+), 244 deletions(-)

-- 
2.3.5

[PATCH 1/7] perf tools: Get rid of position field from struct hist_entry

[Namhyung Kim]

It's not used anywhere, let's get rid of it.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/sort.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 846036a921dc..af192f172fa2 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -106,7 +106,6 @@ struct hist_entry {
 	u8			filtered;
 	char			*srcline;
 	struct symbol		*parent;
-	unsigned long		position;
 	struct rb_root		sorted_chain;
 	struct branch_info	*branch_info;
 	struct hists		*hists;
-- 
2.3.5

[tip:perf/core] perf hists: Get rid of position field from struct hist_entry

[tip-bot for Namhyung Kim]

Commit-ID:  cc5e461ae8e071a95bf6ee917b50998dc31cae17
Gitweb:     http://git.kernel.org/tip/cc5e461ae8e071a95bf6ee917b50998dc31cae17
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Sun, 19 Apr 2015 13:04:09 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 29 Apr 2015 10:37:44 -0300

perf hists: Get rid of position field from struct hist_entry

It's not used anywhere, let's get rid of it.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1429416255-12070-2-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/sort.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 846036a..af192f1 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -106,7 +106,6 @@ struct hist_entry {
 	u8			filtered;
 	char			*srcline;
 	struct symbol		*parent;
-	unsigned long		position;
 	struct rb_root		sorted_chain;
 	struct branch_info	*branch_info;
 	struct hists		*hists;

[PATCH 2/7] perf diff: Make hist_entry_diff fields union

[Namhyung Kim]

The period_ratio_delta, period_ratio and wdiff are never by used at the
same time.  Instead, Just one of them is accessed according to a
comparison method.  So make it union to reduce memory footprint.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/sort.h | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index af192f172fa2..de3303fe726d 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -58,15 +58,16 @@ struct he_stat {
 
 struct hist_entry_diff {
 	bool	computed;
+	union {
+		/* PERF_HPP__DELTA */
+		double	period_ratio_delta;
 
-	/* PERF_HPP__DELTA */
-	double	period_ratio_delta;
-
-	/* PERF_HPP__RATIO */
-	double	period_ratio;
+		/* PERF_HPP__RATIO */
+		double	period_ratio;
 
-	/* HISTC_WEIGHTED_DIFF */
-	s64	wdiff;
+		/* HISTC_WEIGHTED_DIFF */
+		s64	wdiff;
+	};
 };
 
 /**
-- 
2.3.5

[tip:perf/core] perf diff: Make hist_entry_diff fields union

[tip-bot for Namhyung Kim]

Commit-ID:  a0b404f4c0820a934ae1b6ce39d8a4a0f01a7a20
Gitweb:     http://git.kernel.org/tip/a0b404f4c0820a934ae1b6ce39d8a4a0f01a7a20
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Sun, 19 Apr 2015 13:04:10 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 29 Apr 2015 10:37:44 -0300

perf diff: Make hist_entry_diff fields union

The period_ratio_delta, period_ratio and wdiff are never by used at the
same time.  Instead, Just one of them is accessed according to a
comparison method.  So make it union to reduce memory footprint.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1429416255-12070-3-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/sort.h | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index af192f1..de3303f 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -58,15 +58,16 @@ struct he_stat {
 
 struct hist_entry_diff {
 	bool	computed;
+	union {
+		/* PERF_HPP__DELTA */
+		double	period_ratio_delta;
 
-	/* PERF_HPP__DELTA */
-	double	period_ratio_delta;
-
-	/* PERF_HPP__RATIO */
-	double	period_ratio;
+		/* PERF_HPP__RATIO */
+		double	period_ratio;
 
-	/* HISTC_WEIGHTED_DIFF */
-	s64	wdiff;
+		/* HISTC_WEIGHTED_DIFF */
+		s64	wdiff;
+	};
 };
 
 /**

[PATCH 3/7] perf tools: Move TUI-specific fields to struct hist_entry_tui

[Namhyung Kim]

Since perf diff only support stdio output, TUI fields are only accessed
from perf report (or perf top).  So add new struct hist_entry_tui and
move those fields into them.  And include it as an union member.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 52 +++++++++++++++++++++---------------------
 tools/perf/util/hist.c         |  4 ++--
 tools/perf/util/sort.h         | 15 +++++++-----
 3 files changed, 37 insertions(+), 34 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 995b7a8596b1..8b8a647be999 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -61,7 +61,7 @@ static int hist_browser__get_folding(struct hist_browser *browser)
 			rb_entry(nd, struct hist_entry, rb_node);
 
 		if (he->ms.unfolded)
-			unfolded_rows += he->nr_rows;
+			unfolded_rows += he->tui.nr_rows;
 	}
 	return unfolded_rows;
 }
@@ -288,16 +288,16 @@ static bool hist_browser__toggle_fold(struct hist_browser *browser)
 		struct hist_entry *he = browser->he_selection;
 
 		hist_entry__init_have_children(he);
-		browser->b.nr_entries -= he->nr_rows;
-		browser->nr_callchain_rows -= he->nr_rows;
+		browser->b.nr_entries -= he->tui.nr_rows;
+		browser->nr_callchain_rows -= he->tui.nr_rows;
 
 		if (he->ms.unfolded)
-			he->nr_rows = callchain__count_rows(&he->sorted_chain);
+			he->tui.nr_rows = callchain__count_rows(&he->sorted_chain);
 		else
-			he->nr_rows = 0;
+			he->tui.nr_rows = 0;
 
-		browser->b.nr_entries += he->nr_rows;
-		browser->nr_callchain_rows += he->nr_rows;
+		browser->b.nr_entries += he->tui.nr_rows;
+		browser->nr_callchain_rows += he->tui.nr_rows;
 
 		return true;
 	}
@@ -367,9 +367,9 @@ static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
 
 	if (he->ms.has_children) {
 		int n = callchain__set_folding(&he->sorted_chain, unfold);
-		he->nr_rows = unfold ? n : 0;
+		he->tui.nr_rows = unfold ? n : 0;
 	} else
-		he->nr_rows = 0;
+		he->tui.nr_rows = 0;
 }
 
 static void
@@ -383,7 +383,7 @@ __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 	     nd = rb_next(nd)) {
 		struct hist_entry *he = rb_entry(nd, struct hist_entry, rb_node);
 		hist_entry__set_folding(he, unfold);
-		browser->nr_callchain_rows += he->nr_rows;
+		browser->nr_callchain_rows += he->tui.nr_rows;
 	}
 }
 
@@ -459,7 +459,7 @@ static int hist_browser__run(struct hist_browser *browser,
 					   browser->b.rows,
 					   browser->b.index,
 					   browser->b.top_idx,
-					   h->row_offset, h->nr_rows);
+					   h->tui.row_offset, h->tui.nr_rows);
 		}
 			break;
 		case 'C':
@@ -742,7 +742,7 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 	int width = browser->b.width;
 	char folded_sign = ' ';
 	bool current_entry = ui_browser__is_current_entry(&browser->b, row);
-	off_t row_offset = entry->row_offset;
+	off_t row_offset = entry->tui.row_offset;
 	bool first = true;
 	struct perf_hpp_fmt *fmt;
 
@@ -997,7 +997,7 @@ static void ui_browser__hists_seek(struct ui_browser *browser,
 	 * row_offset:
 	 */
 	h = rb_entry(browser->top, struct hist_entry, rb_node);
-	h->row_offset = 0;
+	h->tui.row_offset = 0;
 
 	/*
 	 * Here we have to check if nd is expanded (+), if it is we can't go
@@ -1017,12 +1017,12 @@ do_offset:
 		do {
 			h = rb_entry(nd, struct hist_entry, rb_node);
 			if (h->ms.unfolded) {
-				u16 remaining = h->nr_rows - h->row_offset;
+				u16 remaining = h->tui.nr_rows - h->tui.row_offset;
 				if (offset > remaining) {
 					offset -= remaining;
-					h->row_offset = 0;
+					h->tui.row_offset = 0;
 				} else {
-					h->row_offset += offset;
+					h->tui.row_offset += offset;
 					offset = 0;
 					browser->top = nd;
 					break;
@@ -1039,21 +1039,21 @@ do_offset:
 			h = rb_entry(nd, struct hist_entry, rb_node);
 			if (h->ms.unfolded) {
 				if (first) {
-					if (-offset > h->row_offset) {
-						offset += h->row_offset;
-						h->row_offset = 0;
+					if (-offset > h->tui.row_offset) {
+						offset += h->tui.row_offset;
+						h->tui.row_offset = 0;
 					} else {
-						h->row_offset += offset;
+						h->tui.row_offset += offset;
 						offset = 0;
 						browser->top = nd;
 						break;
 					}
 				} else {
-					if (-offset > h->nr_rows) {
-						offset += h->nr_rows;
-						h->row_offset = 0;
+					if (-offset > h->tui.nr_rows) {
+						offset += h->tui.nr_rows;
+						h->tui.row_offset = 0;
 					} else {
-						h->row_offset = h->nr_rows + offset;
+						h->tui.row_offset = h->tui.nr_rows + offset;
 						offset = 0;
 						browser->top = nd;
 						break;
@@ -1075,7 +1075,7 @@ do_offset:
 				 */
 				h = rb_entry(nd, struct hist_entry, rb_node);
 				if (h->ms.unfolded)
-					h->row_offset = h->nr_rows;
+					h->tui.row_offset = h->tui.nr_rows;
 				break;
 			}
 			first = false;
@@ -1083,7 +1083,7 @@ do_offset:
 	} else {
 		browser->top = nd;
 		h = rb_entry(nd, struct hist_entry, rb_node);
-		h->row_offset = 0;
+		h->tui.row_offset = 0;
 	}
 }
 
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index cc22b9158b93..d2fc802db26a 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1164,8 +1164,8 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
 
 	/* force fold unfiltered entry for simplicity */
 	h->ms.unfolded = false;
-	h->row_offset = 0;
-	h->nr_rows = 0;
+	h->tui.row_offset = 0;
+	h->tui.nr_rows = 0;
 
 	hists->stats.nr_non_filtered_samples += h->stat.nr_events;
 
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index de3303fe726d..fae3bc5c1ea6 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -70,6 +70,11 @@ struct hist_entry_diff {
 	};
 };
 
+struct hist_entry_tui {
+	u16			row_offset;
+	u16			nr_rows;
+};
+
 /**
  * struct hist_entry - histogram entry
  *
@@ -93,18 +98,16 @@ struct hist_entry {
 	s32			cpu;
 	u8			cpumode;
 
-	struct hist_entry_diff	diff;
-
 	/* We are added by hists__add_dummy_entry. */
 	bool			dummy;
 
-	/* XXX These two should move to some tree widget lib */
-	u16			row_offset;
-	u16			nr_rows;
-
 	bool			init_have_children;
 	char			level;
 	u8			filtered;
+	union {
+		struct hist_entry_diff	diff;
+		struct hist_entry_tui	tui;
+	};
 	char			*srcline;
 	struct symbol		*parent;
 	struct rb_root		sorted_chain;
-- 
2.3.5

Re: [PATCH 3/7] perf tools: Move TUI-specific fields to struct hist_entry_tui

[Jiri Olsa]

On Sun, Apr 19, 2015 at 01:04:11PM +0900, Namhyung Kim wrote:
> Since perf diff only support stdio output, TUI fields are only accessed
> from perf report (or perf top).  So add new struct hist_entry_tui and
> move those fields into them.  And include it as an union member.
> 

SNIP

>  
> -	/* XXX These two should move to some tree widget lib */
> -	u16			row_offset;
> -	u16			nr_rows;
> -
>  	bool			init_have_children;
>  	char			level;
>  	u8			filtered;
> +	union {
> +		struct hist_entry_diff	diff;
> +		struct hist_entry_tui	tui;
> +	};

could you please add the comment from changelog in here,
so it's easy to figure this out in the future 

thanks,
jirka

Re: [PATCH 3/7] perf tools: Move TUI-specific fields to struct hist_entry_tui

[Namhyung Kim]

Hi Jiri,

On Mon, Apr 20, 2015 at 10:20:02AM +0200, Jiri Olsa wrote:
> On Sun, Apr 19, 2015 at 01:04:11PM +0900, Namhyung Kim wrote:
> > Since perf diff only support stdio output, TUI fields are only accessed
> > from perf report (or perf top).  So add new struct hist_entry_tui and
> > move those fields into them.  And include it as an union member.
> > 
> 
> SNIP
> 
> >  
> > -	/* XXX These two should move to some tree widget lib */
> > -	u16			row_offset;
> > -	u16			nr_rows;
> > -
> >  	bool			init_have_children;
> >  	char			level;
> >  	u8			filtered;
> > +	union {
> > +		struct hist_entry_diff	diff;
> > +		struct hist_entry_tui	tui;
> > +	};
> 
> could you please add the comment from changelog in here,
> so it's easy to figure this out in the future 

OK, will do.

Thanks,
Namhyung

[PATCH 4/7] perf tools: Move init_have_children field to struct hist_entry_tui

[Namhyung Kim]

The init_have_children is used to init callchain info only for TUI.  So
it'd be better to move it to the hist_entry_tui struct.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 4 ++--
 tools/perf/util/sort.h         | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 8b8a647be999..8f303eb5044d 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -275,10 +275,10 @@ static void callchain__init_have_children(struct rb_root *root)
 
 static void hist_entry__init_have_children(struct hist_entry *he)
 {
-	if (!he->init_have_children) {
+	if (!he->tui.init_have_children) {
 		he->ms.has_children = !RB_EMPTY_ROOT(&he->sorted_chain);
 		callchain__init_have_children(&he->sorted_chain);
-		he->init_have_children = true;
+		he->tui.init_have_children = true;
 	}
 }
 
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index fae3bc5c1ea6..afdfaeb54b73 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -73,6 +73,7 @@ struct hist_entry_diff {
 struct hist_entry_tui {
 	u16			row_offset;
 	u16			nr_rows;
+	bool			init_have_children;
 };
 
 /**
@@ -101,7 +102,6 @@ struct hist_entry {
 	/* We are added by hists__add_dummy_entry. */
 	bool			dummy;
 
-	bool			init_have_children;
 	char			level;
 	u8			filtered;
 	union {
-- 
2.3.5

[PATCH 5/7] perf hists browser: Fix possible memory leak

[Namhyung Kim]

The options array saves strings for each popup menu item.  The number of
items can be vary according to the currently selected item.  So it can
leak some memory if it's exited from a small item.  Fix it by freeing
all items when loop terminates.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 8f303eb5044d..cace2df7e561 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1691,7 +1691,8 @@ skip_annotation:
 				"Switch to another data file in PWD") > 0)
 			switch_data = nr_options++;
 add_exit_option:
-		options[nr_options++] = (char *)"Exit";
+		if (asprintf(&options[nr_options], "Exit") > 0)
+			nr_options++;
 retry_popup_menu:
 		choice = ui__popup_menu(nr_options, options);
 
@@ -1812,7 +1813,7 @@ out_free_stack:
 	pstack__delete(fstack);
 out:
 	hist_browser__delete(browser);
-	free_popup_options(options, nr_options - 1);
+	free_popup_options(options, ARRAY_SIZE(options));
 	return key;
 }
 
-- 
2.3.5

Re: [PATCH 5/7] perf hists browser: Fix possible memory leak

[Jiri Olsa]

On Sun, Apr 19, 2015 at 01:04:13PM +0900, Namhyung Kim wrote:
> The options array saves strings for each popup menu item.  The number of
> items can be vary according to the currently selected item.  So it can
> leak some memory if it's exited from a small item.  Fix it by freeing
> all items when loop terminates.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/browsers/hists.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 8f303eb5044d..cace2df7e561 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -1691,7 +1691,8 @@ skip_annotation:
>  				"Switch to another data file in PWD") > 0)
>  			switch_data = nr_options++;
>  add_exit_option:
> -		options[nr_options++] = (char *)"Exit";
> +		if (asprintf(&options[nr_options], "Exit") > 0)
> +			nr_options++;

this one could cause segfault right? nice catch

>  retry_popup_menu:
>  		choice = ui__popup_menu(nr_options, options);
>  
> @@ -1812,7 +1813,7 @@ out_free_stack:
>  	pstack__delete(fstack);
>  out:
>  	hist_browser__delete(browser);
> -	free_popup_options(options, nr_options - 1);
> +	free_popup_options(options, ARRAY_SIZE(options));
>  	return key;
>  }
>  
> -- 
> 2.3.5
> 

Re: [PATCH 5/7] perf hists browser: Fix possible memory leak

[Namhyung Kim]

On Mon, Apr 20, 2015 at 10:24:15AM +0200, Jiri Olsa wrote:
> On Sun, Apr 19, 2015 at 01:04:13PM +0900, Namhyung Kim wrote:
> > The options array saves strings for each popup menu item.  The number of
> > items can be vary according to the currently selected item.  So it can
> > leak some memory if it's exited from a small item.  Fix it by freeing
> > all items when loop terminates.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/ui/browsers/hists.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> > index 8f303eb5044d..cace2df7e561 100644
> > --- a/tools/perf/ui/browsers/hists.c
> > +++ b/tools/perf/ui/browsers/hists.c
> > @@ -1691,7 +1691,8 @@ skip_annotation:
> >  				"Switch to another data file in PWD") > 0)
> >  			switch_data = nr_options++;
> >  add_exit_option:
> > -		options[nr_options++] = (char *)"Exit";
> > +		if (asprintf(&options[nr_options], "Exit") > 0)
> > +			nr_options++;
> 
> this one could cause segfault right? nice catch

Yes.  But the original code don't cause segfault since it just stops
at 'nr_options - 1'.  When I changed it to go through the whole array,
I found it caused a segfault, so changed it.

Thanks,
Namhyung


> 
> >  retry_popup_menu:
> >  		choice = ui__popup_menu(nr_options, options);
> >  
> > @@ -1812,7 +1813,7 @@ out_free_stack:
> >  	pstack__delete(fstack);
> >  out:
> >  	hist_browser__delete(browser);
> > -	free_popup_options(options, nr_options - 1);
> > +	free_popup_options(options, ARRAY_SIZE(options));
> >  	return key;
> >  }
> >  
> > -- 
> > 2.3.5
> > 

[PATCH 6/7] perf hists browser: Split popup menu actions

[Namhyung Kim]

Currently perf_evsel__hists_browse() function spins on a huge loop and
handles many key actions.  Since it's hard to read and modify, let's
split it out into small helper functions.

The add_XXX_opt() functions are to register popup menu item on the
selected entry.  When it adds an item, it also saves related data into
struct popup_option and returns 1 so that it can increase the number of
items (nr_opts).  A callback function named do_XXX is called with saved
data when the item is selected by user.

No functional change intended.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 565 ++++++++++++++++++++++++++---------------
 1 file changed, 363 insertions(+), 202 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index cace2df7e561..315ebc493508 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1216,11 +1216,6 @@ static void hist_browser__delete(struct hist_browser *browser)
 	free(browser);
 }
 
-static struct hist_entry *hist_browser__selected_entry(struct hist_browser *browser)
-{
-	return browser->he_selection;
-}
-
 static struct thread *hist_browser__selected_thread(struct hist_browser *browser)
 {
 	return browser->he_selection->thread;
@@ -1395,6 +1390,281 @@ close_file_and_continue:
 	return ret;
 }
 
+struct popup_option {
+	struct thread 	*thread;
+	struct map 	*map;
+	struct dso	*dso;
+	struct symbol 	*sym;
+
+	int (*fn)(struct popup_option *opt, struct hist_browser *browser,
+		  struct hist_browser_timer *hbt, struct pstack *pstack,
+		  struct perf_session_env *env);
+};
+
+static int
+do_annotate(struct popup_option *opt, struct hist_browser *browser,
+	    struct hist_browser_timer *hbt, struct pstack *pstack __maybe_unused,
+	    struct perf_session_env *env)
+{
+	struct perf_evsel *evsel;
+	struct annotation *notes;
+	struct map_symbol ms;
+	int err;
+
+	if (!objdump_path && perf_session_env__lookup_objdump(env))
+		return 0;
+
+	ms.map = opt->map;
+	ms.sym = opt->sym;
+
+	notes = symbol__annotation(ms.sym);
+	if (!notes->src)
+		return 0;
+
+	evsel = hists_to_evsel(browser->hists);
+	err = map_symbol__tui_annotate(&ms, evsel, hbt);
+	/*
+	 * offer option to annotate the other branch source or target
+	 * (if they exists) when returning from annotate
+	 */
+	if ((err == 'q' || err == CTRL('c'))
+	    && browser->he_selection->branch_info)
+		return 1;
+
+	ui_browser__update_nr_entries(&browser->b, browser->hists->nr_entries);
+	if (err)
+		ui_browser__handle_resize(&browser->b);
+	return 0;
+}
+
+static int
+add_annotate_opt(struct popup_option *opt, char **optstr,
+		 struct hist_browser *browser __maybe_unused,
+		 struct map *map, struct symbol *sym)
+{
+	if (sym == NULL || map->dso->annotate_warned)
+		return 0;
+
+	if (asprintf(optstr, "Annotate %s", sym->name) < 0)
+		return 0;
+
+	opt->map = map;
+	opt->sym = sym;
+	opt->fn = do_annotate;
+	return 1;
+}
+
+static int do_zoom_thread(struct popup_option *opt,
+			  struct hist_browser *browser,
+			  struct hist_browser_timer *hbt __maybe_unused,
+			  struct pstack *pstack,
+			  struct perf_session_env *env __maybe_unused)
+{
+	struct thread *thread = opt->thread;
+
+	if (browser->hists->thread_filter) {
+		pstack__remove(pstack, &browser->hists->thread_filter);
+		perf_hpp__set_elide(HISTC_THREAD, false);
+		thread__zput(browser->hists->thread_filter);
+		ui_helpline__pop();
+	} else {
+		ui_helpline__fpush("To zoom out press <- or -> + \"Zoom out of %s(%d) thread\"",
+				   thread->comm_set ? thread__comm_str(thread) : "",
+				   thread->tid);
+		browser->hists->thread_filter = thread__get(thread);
+		perf_hpp__set_elide(HISTC_THREAD, false);
+		pstack__push(pstack, &browser->hists->thread_filter);
+	}
+
+	hists__filter_by_thread(browser->hists);
+	hist_browser__reset(browser);
+	return 0;
+}
+
+static int
+add_thread_opt(struct popup_option *opt, char **optstr,
+	       struct hist_browser *browser, struct thread *thread)
+{
+	if (thread == NULL)
+		return 0;
+
+	if (asprintf(optstr, "Zoom %s %s(%d) thread",
+		     browser->hists->thread_filter ? "out of" : "into",
+		     thread->comm_set ? thread__comm_str(thread) : "",
+		     thread->tid) < 0)
+		return 0;
+
+	opt->thread = thread;
+	opt->fn = do_zoom_thread;
+	return 1;
+}
+
+static int do_zoom_dso(struct popup_option *opt,
+		       struct hist_browser *browser,
+		       struct hist_browser_timer *hbt __maybe_unused,
+		       struct pstack *pstack,
+		       struct perf_session_env *env __maybe_unused)
+{
+	struct dso *dso = opt->dso;
+
+	if (browser->hists->dso_filter) {
+		pstack__remove(pstack, &browser->hists->dso_filter);
+		perf_hpp__set_elide(HISTC_DSO, false);
+		browser->hists->dso_filter = NULL;
+		ui_helpline__pop();
+	} else {
+		if (dso == NULL)
+			return 0;
+		ui_helpline__fpush("To zoom out press <- or -> + \"Zoom out of %s DSO\"",
+				   dso->kernel ? "the Kernel" : dso->short_name);
+		browser->hists->dso_filter = dso;
+		perf_hpp__set_elide(HISTC_DSO, true);
+		pstack__push(pstack, &browser->hists->dso_filter);
+	}
+
+	hists__filter_by_dso(browser->hists);
+	hist_browser__reset(browser);
+	return 0;
+}
+
+static int
+add_dso_opt(struct popup_option *opt, char **optstr,
+	    struct hist_browser *browser, struct dso *dso)
+{
+	if (dso == NULL)
+		return 0;
+
+	if (asprintf(optstr, "Zoom %s %s DSO",
+		     browser->hists->dso_filter ? "out of" : "into",
+		     dso->kernel ? "the Kernel" : dso->short_name) < 0)
+		return 0;
+
+	opt->dso = dso;
+	opt->fn = do_zoom_dso;
+	return 1;
+}
+
+static int do_browse_map(struct popup_option *opt,
+			 struct hist_browser *browser __maybe_unused,
+			 struct hist_browser_timer *hbt __maybe_unused,
+			 struct pstack *pstack __maybe_unused,
+			 struct perf_session_env *env __maybe_unused)
+{
+	map__browse(opt->map);
+	return 0;
+}
+
+static int
+add_map_opt(struct popup_option *opt, char **optstr,
+	    struct hist_browser *browser __maybe_unused, struct map *map)
+{
+	if (map == NULL)
+		return 0;
+
+	if (asprintf(optstr, "Browse map details") < 0)
+		return 0;
+
+	opt->map = map;
+	opt->fn = do_browse_map;
+	return 1;
+}
+
+static int do_run_script(struct popup_option *opt,
+			 struct hist_browser *browser __maybe_unused,
+			 struct hist_browser_timer *hbt __maybe_unused,
+			 struct pstack *pstack __maybe_unused,
+			 struct perf_session_env *env __maybe_unused)
+{
+	char script_opt[64];
+	memset(script_opt, 0, sizeof(script_opt));
+
+	if (opt->thread) {
+		scnprintf(script_opt, sizeof(script_opt), " -c %s ",
+			  thread__comm_str(opt->thread));
+	} else if (opt->sym) {
+		scnprintf(script_opt, sizeof(script_opt), " -S %s ",
+			  opt->sym->name);
+	}
+
+	script_browse(script_opt);
+	return 0;
+}
+
+static int
+add_script_opt(struct popup_option *opt, char **optstr,
+	       struct hist_browser *browser __maybe_unused,
+	       struct thread *thread, struct symbol *sym)
+{
+	if (thread) {
+		if (asprintf(optstr, "Run scripts for samples of thread [%s]",
+			     thread__comm_str(thread)) < 0)
+			return 0;
+	} else if (sym) {
+		if (asprintf(optstr, "Run scripts for samples of symbol [%s]",
+			     sym->name) < 0)
+			return 0;
+	} else {
+		if (asprintf(optstr, "Run scripts for all samples") < 0)
+			return 0;
+	}
+
+	opt->thread = thread;
+	opt->sym = sym;
+	opt->fn = do_run_script;
+
+	return 1;
+}
+
+static int do_switch_data(struct popup_option *opt __maybe_unused,
+			  struct hist_browser *browser __maybe_unused,
+			  struct hist_browser_timer *hbt __maybe_unused,
+			  struct pstack *pstack __maybe_unused,
+			  struct perf_session_env *env __maybe_unused)
+{
+	if (switch_data_file()) {
+		ui__warning("Won't switch the data files due to\n"
+			    "no valid data file get selected!\n");
+		return 0;
+	}
+
+	return K_SWITCH_INPUT_DATA;
+}
+
+static int
+add_switch_opt(struct popup_option *opt, char **optstr,
+	       struct hist_browser *browser __maybe_unused,
+	       struct hist_browser_timer *hbt)
+{
+	if (!is_report_browser(hbt))
+		return 0;
+
+	if (asprintf(optstr, "Switch to another data file in PWD") < 0)
+		return 0;
+
+	opt->fn = do_switch_data;
+	return 1;
+}
+
+static int do_exit_browser(struct popup_option *opt __maybe_unused,
+			   struct hist_browser *browser __maybe_unused,
+			   struct hist_browser_timer *hbt __maybe_unused,
+			   struct pstack *pstack __maybe_unused,
+			   struct perf_session_env *env __maybe_unused)
+{
+	return 0;
+}
+
+static int
+add_exit_opt(struct popup_option *opt, char **optstr,
+	     struct hist_browser *browser __maybe_unused)
+{
+	if (asprintf(optstr, "Exit") < 0)
+		return 0;
+
+	opt->fn = do_exit_browser;
+	return 1;
+}
+
 static void hist_browser__update_nr_entries(struct hist_browser *hb)
 {
 	u64 nr_entries = 0;
@@ -1424,11 +1694,11 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	struct hist_browser *browser = hist_browser__new(hists);
 	struct branch_info *bi;
 	struct pstack *fstack;
-	char *options[16];
-	int nr_options = 0;
+	char *optstr[16];
+	struct popup_option opts[16];
+	int nr_opts = 0;
 	int key = -1;
 	char buf[64];
-	char script_opt[64];
 	int delay_secs = hbt ? hbt->refresh : 0;
 	struct perf_hpp_fmt *fmt;
 
@@ -1479,7 +1749,8 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 
 	ui_helpline__push(helpline);
 
-	memset(options, 0, sizeof(options));
+	memset(optstr, 0, sizeof(optstr));
+	memset(opts, 0, sizeof(opts));
 
 	perf_hpp__for_each_format(fmt)
 		perf_hpp__reset_width(fmt, hists);
@@ -1489,14 +1760,10 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 
 	while (1) {
 		struct thread *thread = NULL;
-		const struct dso *dso = NULL;
-		int choice = 0,
-		    annotate = -2, zoom_dso = -2, zoom_thread = -2,
-		    annotate_f = -2, annotate_t = -2, browse_map = -2;
-		int scripts_comm = -2, scripts_symbol = -2,
-		    scripts_all = -2, switch_data = -2;
+		struct dso *dso = NULL;
+		int choice = 0;
 
-		nr_options = 0;
+		nr_opts = 0;
 
 		key = hist_browser__run(browser, hbt);
 
@@ -1526,17 +1793,28 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			    browser->selection->sym == NULL ||
 			    browser->selection->map->dso->annotate_warned)
 				continue;
-			goto do_annotate;
+
+			opts->map = browser->selection->map;
+			opts->sym = browser->selection->sym;
+
+			do_annotate(opts, browser, hbt, fstack, env);
+			continue;
 		case 'P':
 			hist_browser__dump(browser);
 			continue;
 		case 'd':
-			goto zoom_dso;
+			opts->dso = dso;
+
+			do_zoom_dso(opts, browser, hbt, fstack, env);
+			continue;
 		case 'V':
 			browser->show_dso = !browser->show_dso;
 			continue;
 		case 't':
-			goto zoom_thread;
+			opts->thread = thread;
+
+			do_zoom_thread(opts, browser, hbt, fstack, env);
+			continue;
 		case '/':
 			if (ui_browser__input_window("Symbol to show",
 					"Please enter the name of symbol you want to see",
@@ -1548,12 +1826,20 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			}
 			continue;
 		case 'r':
-			if (is_report_browser(hbt))
-				goto do_scripts;
+			if (is_report_browser(hbt)) {
+				opts->thread = NULL;
+				opts->sym = NULL;
+
+				do_run_script(opts, browser, hbt, fstack, env);
+			}
 			continue;
 		case 's':
-			if (is_report_browser(hbt))
-				goto do_data_switch;
+			if (is_report_browser(hbt)) {
+				key = do_switch_data(opts, browser, hbt,
+						     fstack, env);
+				if (key == K_SWITCH_INPUT_DATA)
+					goto out_free_stack;
+			}
 			continue;
 		case 'i':
 			/* env->arch is NULL for live-mode (i.e. perf top) */
@@ -1592,10 +1878,16 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 				continue;
 			}
 			top = pstack__pop(fstack);
-			if (top == &browser->hists->dso_filter)
-				goto zoom_out_dso;
-			if (top == &browser->hists->thread_filter)
-				goto zoom_out_thread;
+			if (top == &browser->hists->dso_filter) {
+				perf_hpp__set_elide(HISTC_DSO, false);
+				browser->hists->dso_filter = NULL;
+				ui_helpline__pop();
+			}
+			if (top == &browser->hists->thread_filter) {
+				perf_hpp__set_elide(HISTC_THREAD, false);
+				thread__zput(browser->hists->thread_filter);
+				ui_helpline__pop();
+			}
 			continue;
 		}
 		case K_ESC:
@@ -1620,200 +1912,69 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 		if (sort__mode == SORT_MODE__BRANCH) {
 			bi = browser->he_selection->branch_info;
 
-			if (bi == NULL)
-				goto skip_annotation;
-
-			if (bi->from.sym != NULL &&
-			    !bi->from.map->dso->annotate_warned &&
-			    asprintf(&options[nr_options], "Annotate %s", bi->from.sym->name) > 0) {
-				annotate_f = nr_options++;
-			}
-
-			if (bi->to.sym != NULL &&
-			    !bi->to.map->dso->annotate_warned &&
-			    (bi->to.sym != bi->from.sym ||
-			     bi->to.map->dso != bi->from.map->dso) &&
-			    asprintf(&options[nr_options], "Annotate %s", bi->to.sym->name) > 0) {
-				annotate_t = nr_options++;
+			if (bi != NULL) {
+				nr_opts += add_annotate_opt(&opts[nr_opts],
+							    &optstr[nr_opts],
+							    browser,
+							    bi->from.map,
+							    bi->from.sym);
+				if (bi->to.sym != bi->from.sym)
+					nr_opts += add_annotate_opt(&opts[nr_opts],
+								    &optstr[nr_opts],
+								    browser,
+								    bi->to.map,
+								    bi->to.sym);
 			}
 		} else {
-			if (browser->selection->sym != NULL &&
-			    !browser->selection->map->dso->annotate_warned) {
-				struct annotation *notes;
-
-				notes = symbol__annotation(browser->selection->sym);
-
-				if (notes->src &&
-				    asprintf(&options[nr_options], "Annotate %s",
-						 browser->selection->sym->name) > 0) {
-					annotate = nr_options++;
-				}
-			}
+			nr_opts += add_annotate_opt(&opts[nr_opts],
+						    &optstr[nr_opts], browser,
+						    browser->selection->map,
+						    browser->selection->sym);
 		}
 skip_annotation:
-		if (thread != NULL &&
-		    asprintf(&options[nr_options], "Zoom %s %s(%d) thread",
-			     (browser->hists->thread_filter ? "out of" : "into"),
-			     (thread->comm_set ? thread__comm_str(thread) : ""),
-			     thread->tid) > 0)
-			zoom_thread = nr_options++;
-
-		if (dso != NULL &&
-		    asprintf(&options[nr_options], "Zoom %s %s DSO",
-			     (browser->hists->dso_filter ? "out of" : "into"),
-			     (dso->kernel ? "the Kernel" : dso->short_name)) > 0)
-			zoom_dso = nr_options++;
-
-		if (browser->selection != NULL &&
-		    browser->selection->map != NULL &&
-		    asprintf(&options[nr_options], "Browse map details") > 0)
-			browse_map = nr_options++;
+		nr_opts += add_thread_opt(&opts[nr_opts], &optstr[nr_opts],
+					  browser, thread);
+		nr_opts += add_dso_opt(&opts[nr_opts], &optstr[nr_opts],
+				       browser, dso);
+		nr_opts += add_map_opt(&opts[nr_opts], &optstr[nr_opts],
+				       browser, browser->selection->map);
 
 		/* perf script support */
 		if (browser->he_selection) {
-			struct symbol *sym;
-
-			if (asprintf(&options[nr_options], "Run scripts for samples of thread [%s]",
-				     thread__comm_str(browser->he_selection->thread)) > 0)
-				scripts_comm = nr_options++;
-
-			sym = browser->he_selection->ms.sym;
-			if (sym && sym->namelen &&
-				asprintf(&options[nr_options], "Run scripts for samples of symbol [%s]",
-						sym->name) > 0)
-				scripts_symbol = nr_options++;
+			nr_opts += add_script_opt(&opts[nr_opts], &optstr[nr_opts],
+						  browser, thread, NULL);
+			nr_opts += add_script_opt(&opts[nr_opts], &optstr[nr_opts],
+						  browser, NULL,
+						  browser->selection->sym);
 		}
+		nr_opts += add_script_opt(&opts[nr_opts], &optstr[nr_opts],
+					  browser, NULL, NULL);
 
-		if (asprintf(&options[nr_options], "Run scripts for all samples") > 0)
-			scripts_all = nr_options++;
-
-		if (is_report_browser(hbt) && asprintf(&options[nr_options],
-				"Switch to another data file in PWD") > 0)
-			switch_data = nr_options++;
+		nr_opts += add_switch_opt(&opts[nr_opts], &optstr[nr_opts],
+					  browser, hbt);
 add_exit_option:
-		if (asprintf(&options[nr_options], "Exit") > 0)
-			nr_options++;
-retry_popup_menu:
-		choice = ui__popup_menu(nr_options, options);
-
-		if (choice == nr_options - 1)
-			break;
-
-		if (choice == -1) {
-			free_popup_options(options, nr_options - 1);
-			continue;
-		}
-
-		if (choice == annotate || choice == annotate_t || choice == annotate_f) {
-			struct hist_entry *he;
-			struct annotation *notes;
-			struct map_symbol ms;
-			int err;
-do_annotate:
-			if (!objdump_path && perf_session_env__lookup_objdump(env))
-				continue;
+		nr_opts += add_exit_opt(&opts[nr_opts], &optstr[nr_opts],
+					browser);
 
-			he = hist_browser__selected_entry(browser);
-			if (he == NULL)
-				continue;
-
-			if (choice == annotate_f) {
-				ms.map = he->branch_info->from.map;
-				ms.sym = he->branch_info->from.sym;
-			} else if (choice == annotate_t) {
-				ms.map = he->branch_info->to.map;
-				ms.sym = he->branch_info->to.sym;
-			} else {
-				ms = *browser->selection;
-			}
-
-			notes = symbol__annotation(ms.sym);
-			if (!notes->src)
-				continue;
-
-			err = map_symbol__tui_annotate(&ms, evsel, hbt);
-			/*
-			 * offer option to annotate the other branch source or target
-			 * (if they exists) when returning from annotate
-			 */
-			if ((err == 'q' || err == CTRL('c'))
-			    && annotate_t != -2 && annotate_f != -2)
-				goto retry_popup_menu;
-
-			ui_browser__update_nr_entries(&browser->b, browser->hists->nr_entries);
-			if (err)
-				ui_browser__handle_resize(&browser->b);
-
-		} else if (choice == browse_map)
-			map__browse(browser->selection->map);
-		else if (choice == zoom_dso) {
-zoom_dso:
-			if (browser->hists->dso_filter) {
-				pstack__remove(fstack, &browser->hists->dso_filter);
-zoom_out_dso:
-				ui_helpline__pop();
-				browser->hists->dso_filter = NULL;
-				perf_hpp__set_elide(HISTC_DSO, false);
-			} else {
-				if (dso == NULL)
-					continue;
-				ui_helpline__fpush("To zoom out press <- or -> + \"Zoom out of %s DSO\"",
-						   dso->kernel ? "the Kernel" : dso->short_name);
-				browser->hists->dso_filter = dso;
-				perf_hpp__set_elide(HISTC_DSO, true);
-				pstack__push(fstack, &browser->hists->dso_filter);
-			}
-			hists__filter_by_dso(hists);
-			hist_browser__reset(browser);
-		} else if (choice == zoom_thread) {
-zoom_thread:
-			if (browser->hists->thread_filter) {
-				pstack__remove(fstack, &browser->hists->thread_filter);
-zoom_out_thread:
-				ui_helpline__pop();
-				thread__zput(browser->hists->thread_filter);
-				perf_hpp__set_elide(HISTC_THREAD, false);
-			} else {
-				ui_helpline__fpush("To zoom out press <- or -> + \"Zoom out of %s(%d) thread\"",
-						   thread->comm_set ? thread__comm_str(thread) : "",
-						   thread->tid);
-				browser->hists->thread_filter = thread__get(thread);
-				perf_hpp__set_elide(HISTC_THREAD, false);
-				pstack__push(fstack, &browser->hists->thread_filter);
-			}
-			hists__filter_by_thread(hists);
-			hist_browser__reset(browser);
-		}
-		/* perf scripts support */
-		else if (choice == scripts_all || choice == scripts_comm ||
-				choice == scripts_symbol) {
-do_scripts:
-			memset(script_opt, 0, 64);
+		do {
+			struct popup_option *opt;
 
-			if (choice == scripts_comm)
-				sprintf(script_opt, " -c %s ", thread__comm_str(browser->he_selection->thread));
+			choice = ui__popup_menu(nr_opts, optstr);
+			if (choice == -1 || choice >= nr_opts)
+				break;
 
-			if (choice == scripts_symbol)
-				sprintf(script_opt, " -S %s ", browser->he_selection->ms.sym->name);
+			opt = &opts[choice];
+			key = opt->fn(opt, browser, hbt, fstack, env);
+		} while (key == 1);
 
-			script_browse(script_opt);
-		}
-		/* Switch to another data file */
-		else if (choice == switch_data) {
-do_data_switch:
-			if (!switch_data_file()) {
-				key = K_SWITCH_INPUT_DATA;
-				break;
-			} else
-				ui__warning("Won't switch the data files due to\n"
-					"no valid data file get selected!\n");
-		}
+		if (key == K_SWITCH_INPUT_DATA)
+			break;
 	}
 out_free_stack:
 	pstack__delete(fstack);
 out:
 	hist_browser__delete(browser);
-	free_popup_options(options, ARRAY_SIZE(options));
+	free_popup_options(optstr, ARRAY_SIZE(optstr));
 	return key;
 }
 
-- 
2.3.5

Re: [PATCH 6/7] perf hists browser: Split popup menu actions

[Jiri Olsa]

On Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim wrote:

SNIP

> +
> +static int
> +add_script_opt(struct popup_option *opt, char **optstr,
> +	       struct hist_browser *browser __maybe_unused,
> +	       struct thread *thread, struct symbol *sym)
> +{
> +	if (thread) {
> +		if (asprintf(optstr, "Run scripts for samples of thread [%s]",
> +			     thread__comm_str(thread)) < 0)
> +			return 0;
> +	} else if (sym) {

there's also check for sym->namelen in the original code

> +		if (asprintf(optstr, "Run scripts for samples of symbol [%s]",
> +			     sym->name) < 0)
> +			return 0;
> +	} else {
> +		if (asprintf(optstr, "Run scripts for all samples") < 0)
> +			return 0;
> +	}
> +
> +	opt->thread = thread;
> +	opt->sym = sym;
> +	opt->fn = do_run_script;
> +
> +	return 1;
> +}

SNIP

Re: [PATCH 6/7] perf hists browser: Split popup menu actions

[Namhyung Kim]

On Mon, Apr 20, 2015 at 11:21:59AM +0200, Jiri Olsa wrote:
> On Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> > +
> > +static int
> > +add_script_opt(struct popup_option *opt, char **optstr,
> > +	       struct hist_browser *browser __maybe_unused,
> > +	       struct thread *thread, struct symbol *sym)
> > +{
> > +	if (thread) {
> > +		if (asprintf(optstr, "Run scripts for samples of thread [%s]",
> > +			     thread__comm_str(thread)) < 0)
> > +			return 0;
> > +	} else if (sym) {
> 
> there's also check for sym->namelen in the original code

Ah, right.  I'll change it.

But I wonder there's a case 'sym && !sym->namelen' though..

Thanks,
Namhyung


> 
> > +		if (asprintf(optstr, "Run scripts for samples of symbol [%s]",
> > +			     sym->name) < 0)
> > +			return 0;
> > +	} else {
> > +		if (asprintf(optstr, "Run scripts for all samples") < 0)
> > +			return 0;
> > +	}
> > +
> > +	opt->thread = thread;
> > +	opt->sym = sym;
> > +	opt->fn = do_run_script;
> > +
> > +	return 1;
> > +}
> 
> SNIP

Re: [PATCH 6/7] perf hists browser: Split popup menu actions

[Jiri Olsa]

On Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim wrote:

SNIP

>  			continue;
>  		case 's':
> -			if (is_report_browser(hbt))
> -				goto do_data_switch;
> +			if (is_report_browser(hbt)) {
> +				key = do_switch_data(opts, browser, hbt,
> +						     fstack, env);
> +				if (key == K_SWITCH_INPUT_DATA)
> +					goto out_free_stack;
> +			}
>  			continue;
>  		case 'i':
>  			/* env->arch is NULL for live-mode (i.e. perf top) */
> @@ -1592,10 +1878,16 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  				continue;
>  			}
>  			top = pstack__pop(fstack);
> -			if (top == &browser->hists->dso_filter)
> -				goto zoom_out_dso;
> -			if (top == &browser->hists->thread_filter)
> -				goto zoom_out_thread;
> +			if (top == &browser->hists->dso_filter) {
> +				perf_hpp__set_elide(HISTC_DSO, false);
> +				browser->hists->dso_filter = NULL;
> +				ui_helpline__pop();
> +			}
> +			if (top == &browser->hists->thread_filter) {
> +				perf_hpp__set_elide(HISTC_THREAD, false);
> +				thread__zput(browser->hists->thread_filter);
> +				ui_helpline__pop();
> +			}

ui_helpline__pop could be called in heare and removed in above legs:

			ui_helpline__pop()

also the original code calls following for zoom out:

-                       hists__filter_by_dso(hists);
-                       hist_browser__reset(browser);


jirka
>  			continue;
>  		}
>  		case K_ESC:
> @@ -1620,200 +1912,69 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  		if (sort__mode == SORT_MODE__BRANCH) {
>  			bi = browser->he_selection->branch_info;
>  

SNIP

Re: [PATCH 6/7] perf hists browser: Split popup menu actions

[Namhyung Kim]

On Mon, Apr 20, 2015 at 11:46:07AM +0200, Jiri Olsa wrote:
> On Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> >  			continue;
> >  		case 's':
> > -			if (is_report_browser(hbt))
> > -				goto do_data_switch;
> > +			if (is_report_browser(hbt)) {
> > +				key = do_switch_data(opts, browser, hbt,
> > +						     fstack, env);
> > +				if (key == K_SWITCH_INPUT_DATA)
> > +					goto out_free_stack;
> > +			}
> >  			continue;
> >  		case 'i':
> >  			/* env->arch is NULL for live-mode (i.e. perf top) */
> > @@ -1592,10 +1878,16 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
> >  				continue;
> >  			}
> >  			top = pstack__pop(fstack);
> > -			if (top == &browser->hists->dso_filter)
> > -				goto zoom_out_dso;
> > -			if (top == &browser->hists->thread_filter)
> > -				goto zoom_out_thread;
> > +			if (top == &browser->hists->dso_filter) {
> > +				perf_hpp__set_elide(HISTC_DSO, false);
> > +				browser->hists->dso_filter = NULL;
> > +				ui_helpline__pop();
> > +			}
> > +			if (top == &browser->hists->thread_filter) {
> > +				perf_hpp__set_elide(HISTC_THREAD, false);
> > +				thread__zput(browser->hists->thread_filter);
> > +				ui_helpline__pop();
> > +			}
> 
> ui_helpline__pop could be called in heare and removed in above legs:
> 
> 			ui_helpline__pop()
> 
> also the original code calls following for zoom out:
> 
> -                       hists__filter_by_dso(hists);
> -                       hist_browser__reset(browser);
> 

Right.  The intention was to call do_zoom_thread() or do_zoom_dso()
like in the next patch.  Will fix this anyway.

Thanks for your careful review!
Namhyung

Re: [PATCH 6/7] perf hists browser: Split popup menu actions

[Arnaldo Carvalho de Melo]

Em Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim escreveu:
> Currently perf_evsel__hists_browse() function spins on a huge loop and
> handles many key actions.  Since it's hard to read and modify, let's
> split it out into small helper functions.
> 
> The add_XXX_opt() functions are to register popup menu item on the
> selected entry.  When it adds an item, it also saves related data into
> struct popup_option and returns 1 so that it can increase the number of
> items (nr_opts).  A callback function named do_XXX is called with saved
> data when the item is selected by user.
> 
> No functional change intended.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/browsers/hists.c | 565 ++++++++++++++++++++++++++---------------
>  1 file changed, 363 insertions(+), 202 deletions(-)
> 
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index cace2df7e561..315ebc493508 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -1216,11 +1216,6 @@ static void hist_browser__delete(struct hist_browser *browser)
>  	free(browser);
>  }
>  
> -static struct hist_entry *hist_browser__selected_entry(struct hist_browser *browser)
> -{
> -	return browser->he_selection;
> -}
> -

Why remove the above function? To reduce the patch size you could have
left it and if the reason for removing it is that compelling, remove it
in a later patch.

>  static struct thread *hist_browser__selected_thread(struct hist_browser *browser)
>  {
>  	return browser->he_selection->thread;
> @@ -1395,6 +1390,281 @@ close_file_and_continue:
>  	return ret;
>  }
>  
> +struct popup_option {
> +	struct thread 	*thread;
> +	struct map 	*map;
> +	struct dso	*dso;
> +	struct symbol 	*sym;

You could use struct map_symbol, that has the three above, right? In
some cases you would have less lines by doing:

	
	ms = po->ms;

> +	int (*fn)(struct popup_option *opt, struct hist_browser *browser,
> +		  struct hist_browser_timer *hbt, struct pstack *pstack,

I wonder if, as a prep patch, you couldn't have browser->hbt, so that we
would reduce the function signature above. Ditto for pstack.


Also, you're adding the mechanism (popup_option) at the same time that
you make those new functions use it.

I think that it would be better if you first created the functions, call
them directly, then introduce popup_option (probably better named as
"popup_action").

Each patch would be smaller and more easily reviewable, as well bisect
would work better when locating bugs.

> +		  struct perf_session_env *env);
> +};
> +
> +static int
> +do_annotate(struct popup_option *opt, struct hist_browser *browser,
> +	    struct hist_browser_timer *hbt, struct pstack *pstack __maybe_unused,
> +	    struct perf_session_env *env)
> +{
> +	struct perf_evsel *evsel;
> +	struct annotation *notes;
> +	struct map_symbol ms;
> +	int err;
> +
> +	if (!objdump_path && perf_session_env__lookup_objdump(env))
> +		return 0;
> +
> +	ms.map = opt->map;
> +	ms.sym = opt->sym;
> +
> +	notes = symbol__annotation(ms.sym);
> +	if (!notes->src)
> +		return 0;
> +
> +	evsel = hists_to_evsel(browser->hists);
> +	err = map_symbol__tui_annotate(&ms, evsel, hbt);
> +	/*
> +	 * offer option to annotate the other branch source or target
> +	 * (if they exists) when returning from annotate
> +	 */
> +	if ((err == 'q' || err == CTRL('c'))
> +	    && browser->he_selection->branch_info)

Please put the && operator at the end of the first line, even if
originally it was like that (was it?).

> +		return 1;
> +
> +	ui_browser__update_nr_entries(&browser->b, browser->hists->nr_entries);
> +	if (err)
> +		ui_browser__handle_resize(&browser->b);
> +	return 0;
> +}
> +
> +static int
> +add_annotate_opt(struct popup_option *opt, char **optstr,
> +		 struct hist_browser *browser __maybe_unused,
> +		 struct map *map, struct symbol *sym)
> +{
> +	if (sym == NULL || map->dso->annotate_warned)
> +		return 0;
> +
> +	if (asprintf(optstr, "Annotate %s", sym->name) < 0)
> +		return 0;
> +
> +	opt->map = map;
> +	opt->sym = sym;
> +	opt->fn = do_annotate;
> +	return 1;
> +}
> +
> +static int do_zoom_thread(struct popup_option *opt,
> +			  struct hist_browser *browser,
> +			  struct hist_browser_timer *hbt __maybe_unused,
> +			  struct pstack *pstack,
> +			  struct perf_session_env *env __maybe_unused)
> +{
> +	struct thread *thread = opt->thread;
> +
> +	if (browser->hists->thread_filter) {
> +		pstack__remove(pstack, &browser->hists->thread_filter);
> +		perf_hpp__set_elide(HISTC_THREAD, false);
> +		thread__zput(browser->hists->thread_filter);
> +		ui_helpline__pop();
> +	} else {
> +		ui_helpline__fpush("To zoom out press <- or -> + \"Zoom out of %s(%d) thread\"",
> +				   thread->comm_set ? thread__comm_str(thread) : "",
> +				   thread->tid);
> +		browser->hists->thread_filter = thread__get(thread);
> +		perf_hpp__set_elide(HISTC_THREAD, false);
> +		pstack__push(pstack, &browser->hists->thread_filter);
> +	}
> +
> +	hists__filter_by_thread(browser->hists);
> +	hist_browser__reset(browser);
> +	return 0;
> +}
> +
> +static int
> +add_thread_opt(struct popup_option *opt, char **optstr,
> +	       struct hist_browser *browser, struct thread *thread)
> +{
> +	if (thread == NULL)
> +		return 0;
> +
> +	if (asprintf(optstr, "Zoom %s %s(%d) thread",
> +		     browser->hists->thread_filter ? "out of" : "into",
> +		     thread->comm_set ? thread__comm_str(thread) : "",
> +		     thread->tid) < 0)
> +		return 0;
> +
> +	opt->thread = thread;
> +	opt->fn = do_zoom_thread;
> +	return 1;
> +}
> +
> +static int do_zoom_dso(struct popup_option *opt,
> +		       struct hist_browser *browser,
> +		       struct hist_browser_timer *hbt __maybe_unused,
> +		       struct pstack *pstack,
> +		       struct perf_session_env *env __maybe_unused)
> +{
> +	struct dso *dso = opt->dso;
> +
> +	if (browser->hists->dso_filter) {
> +		pstack__remove(pstack, &browser->hists->dso_filter);
> +		perf_hpp__set_elide(HISTC_DSO, false);
> +		browser->hists->dso_filter = NULL;
> +		ui_helpline__pop();
> +	} else {
> +		if (dso == NULL)
> +			return 0;
> +		ui_helpline__fpush("To zoom out press <- or -> + \"Zoom out of %s DSO\"",
> +				   dso->kernel ? "the Kernel" : dso->short_name);
> +		browser->hists->dso_filter = dso;
> +		perf_hpp__set_elide(HISTC_DSO, true);
> +		pstack__push(pstack, &browser->hists->dso_filter);
> +	}
> +
> +	hists__filter_by_dso(browser->hists);
> +	hist_browser__reset(browser);
> +	return 0;
> +}
> +
> +static int
> +add_dso_opt(struct popup_option *opt, char **optstr,
> +	    struct hist_browser *browser, struct dso *dso)
> +{
> +	if (dso == NULL)
> +		return 0;
> +
> +	if (asprintf(optstr, "Zoom %s %s DSO",
> +		     browser->hists->dso_filter ? "out of" : "into",
> +		     dso->kernel ? "the Kernel" : dso->short_name) < 0)
> +		return 0;
> +
> +	opt->dso = dso;
> +	opt->fn = do_zoom_dso;
> +	return 1;
> +}
> +
> +static int do_browse_map(struct popup_option *opt,
> +			 struct hist_browser *browser __maybe_unused,
> +			 struct hist_browser_timer *hbt __maybe_unused,
> +			 struct pstack *pstack __maybe_unused,
> +			 struct perf_session_env *env __maybe_unused)
> +{
> +	map__browse(opt->map);
> +	return 0;
> +}
> +
> +static int
> +add_map_opt(struct popup_option *opt, char **optstr,
> +	    struct hist_browser *browser __maybe_unused, struct map *map)
> +{
> +	if (map == NULL)
> +		return 0;
> +
> +	if (asprintf(optstr, "Browse map details") < 0)
> +		return 0;
> +
> +	opt->map = map;
> +	opt->fn = do_browse_map;
> +	return 1;
> +}
> +
> +static int do_run_script(struct popup_option *opt,
> +			 struct hist_browser *browser __maybe_unused,
> +			 struct hist_browser_timer *hbt __maybe_unused,
> +			 struct pstack *pstack __maybe_unused,
> +			 struct perf_session_env *env __maybe_unused)
> +{
> +	char script_opt[64];
> +	memset(script_opt, 0, sizeof(script_opt));
> +
> +	if (opt->thread) {
> +		scnprintf(script_opt, sizeof(script_opt), " -c %s ",
> +			  thread__comm_str(opt->thread));
> +	} else if (opt->sym) {
> +		scnprintf(script_opt, sizeof(script_opt), " -S %s ",
> +			  opt->sym->name);
> +	}
> +
> +	script_browse(script_opt);
> +	return 0;
> +}
> +
> +static int
> +add_script_opt(struct popup_option *opt, char **optstr,
> +	       struct hist_browser *browser __maybe_unused,
> +	       struct thread *thread, struct symbol *sym)
> +{
> +	if (thread) {
> +		if (asprintf(optstr, "Run scripts for samples of thread [%s]",
> +			     thread__comm_str(thread)) < 0)
> +			return 0;
> +	} else if (sym) {
> +		if (asprintf(optstr, "Run scripts for samples of symbol [%s]",
> +			     sym->name) < 0)
> +			return 0;
> +	} else {
> +		if (asprintf(optstr, "Run scripts for all samples") < 0)
> +			return 0;
> +	}
> +
> +	opt->thread = thread;
> +	opt->sym = sym;
> +	opt->fn = do_run_script;
> +
> +	return 1;
> +}
> +
> +static int do_switch_data(struct popup_option *opt __maybe_unused,
> +			  struct hist_browser *browser __maybe_unused,
> +			  struct hist_browser_timer *hbt __maybe_unused,
> +			  struct pstack *pstack __maybe_unused,
> +			  struct perf_session_env *env __maybe_unused)
> +{
> +	if (switch_data_file()) {
> +		ui__warning("Won't switch the data files due to\n"
> +			    "no valid data file get selected!\n");
> +		return 0;
> +	}
> +
> +	return K_SWITCH_INPUT_DATA;
> +}
> +
> +static int
> +add_switch_opt(struct popup_option *opt, char **optstr,
> +	       struct hist_browser *browser __maybe_unused,
> +	       struct hist_browser_timer *hbt)
> +{
> +	if (!is_report_browser(hbt))
> +		return 0;
> +
> +	if (asprintf(optstr, "Switch to another data file in PWD") < 0)
> +		return 0;
> +
> +	opt->fn = do_switch_data;
> +	return 1;
> +}
> +
> +static int do_exit_browser(struct popup_option *opt __maybe_unused,
> +			   struct hist_browser *browser __maybe_unused,
> +			   struct hist_browser_timer *hbt __maybe_unused,
> +			   struct pstack *pstack __maybe_unused,
> +			   struct perf_session_env *env __maybe_unused)
> +{
> +	return 0;
> +}
> +
> +static int
> +add_exit_opt(struct popup_option *opt, char **optstr,
> +	     struct hist_browser *browser __maybe_unused)
> +{
> +	if (asprintf(optstr, "Exit") < 0)
> +		return 0;
> +
> +	opt->fn = do_exit_browser;
> +	return 1;
> +}
> +
>  static void hist_browser__update_nr_entries(struct hist_browser *hb)
>  {
>  	u64 nr_entries = 0;
> @@ -1424,11 +1694,11 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  	struct hist_browser *browser = hist_browser__new(hists);
>  	struct branch_info *bi;
>  	struct pstack *fstack;
> -	char *options[16];
> -	int nr_options = 0;
> +	char *optstr[16];
> +	struct popup_option opts[16];
> +	int nr_opts = 0;
>  	int key = -1;
>  	char buf[64];
> -	char script_opt[64];
>  	int delay_secs = hbt ? hbt->refresh : 0;
>  	struct perf_hpp_fmt *fmt;
>  
> @@ -1479,7 +1749,8 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  
>  	ui_helpline__push(helpline);
>  
> -	memset(options, 0, sizeof(options));
> +	memset(optstr, 0, sizeof(optstr));
> +	memset(opts, 0, sizeof(opts));
>  
>  	perf_hpp__for_each_format(fmt)
>  		perf_hpp__reset_width(fmt, hists);
> @@ -1489,14 +1760,10 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  
>  	while (1) {
>  		struct thread *thread = NULL;
> -		const struct dso *dso = NULL;
> -		int choice = 0,
> -		    annotate = -2, zoom_dso = -2, zoom_thread = -2,
> -		    annotate_f = -2, annotate_t = -2, browse_map = -2;
> -		int scripts_comm = -2, scripts_symbol = -2,
> -		    scripts_all = -2, switch_data = -2;
> +		struct dso *dso = NULL;
> +		int choice = 0;
>  
> -		nr_options = 0;
> +		nr_opts = 0;
>  
>  		key = hist_browser__run(browser, hbt);
>  
> @@ -1526,17 +1793,28 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  			    browser->selection->sym == NULL ||
>  			    browser->selection->map->dso->annotate_warned)
>  				continue;
> -			goto do_annotate;
> +
> +			opts->map = browser->selection->map;
> +			opts->sym = browser->selection->sym;
> +
> +			do_annotate(opts, browser, hbt, fstack, env);
> +			continue;
>  		case 'P':
>  			hist_browser__dump(browser);
>  			continue;
>  		case 'd':
> -			goto zoom_dso;
> +			opts->dso = dso;
> +
> +			do_zoom_dso(opts, browser, hbt, fstack, env);
> +			continue;
>  		case 'V':
>  			browser->show_dso = !browser->show_dso;
>  			continue;
>  		case 't':
> -			goto zoom_thread;
> +			opts->thread = thread;
> +
> +			do_zoom_thread(opts, browser, hbt, fstack, env);
> +			continue;
>  		case '/':
>  			if (ui_browser__input_window("Symbol to show",
>  					"Please enter the name of symbol you want to see",
> @@ -1548,12 +1826,20 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  			}
>  			continue;
>  		case 'r':
> -			if (is_report_browser(hbt))
> -				goto do_scripts;
> +			if (is_report_browser(hbt)) {
> +				opts->thread = NULL;
> +				opts->sym = NULL;
> +
> +				do_run_script(opts, browser, hbt, fstack, env);
> +			}
>  			continue;
>  		case 's':
> -			if (is_report_browser(hbt))
> -				goto do_data_switch;
> +			if (is_report_browser(hbt)) {
> +				key = do_switch_data(opts, browser, hbt,
> +						     fstack, env);
> +				if (key == K_SWITCH_INPUT_DATA)
> +					goto out_free_stack;
> +			}
>  			continue;
>  		case 'i':
>  			/* env->arch is NULL for live-mode (i.e. perf top) */
> @@ -1592,10 +1878,16 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  				continue;
>  			}
>  			top = pstack__pop(fstack);
> -			if (top == &browser->hists->dso_filter)
> -				goto zoom_out_dso;
> -			if (top == &browser->hists->thread_filter)
> -				goto zoom_out_thread;
> +			if (top == &browser->hists->dso_filter) {
> +				perf_hpp__set_elide(HISTC_DSO, false);
> +				browser->hists->dso_filter = NULL;
> +				ui_helpline__pop();
> +			}
> +			if (top == &browser->hists->thread_filter) {
> +				perf_hpp__set_elide(HISTC_THREAD, false);
> +				thread__zput(browser->hists->thread_filter);
> +				ui_helpline__pop();
> +			}
>  			continue;
>  		}
>  		case K_ESC:
> @@ -1620,200 +1912,69 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  		if (sort__mode == SORT_MODE__BRANCH) {
>  			bi = browser->he_selection->branch_info;
>  
> -			if (bi == NULL)
> -				goto skip_annotation;
> -
> -			if (bi->from.sym != NULL &&
> -			    !bi->from.map->dso->annotate_warned &&
> -			    asprintf(&options[nr_options], "Annotate %s", bi->from.sym->name) > 0) {
> -				annotate_f = nr_options++;
> -			}
> -
> -			if (bi->to.sym != NULL &&
> -			    !bi->to.map->dso->annotate_warned &&
> -			    (bi->to.sym != bi->from.sym ||
> -			     bi->to.map->dso != bi->from.map->dso) &&
> -			    asprintf(&options[nr_options], "Annotate %s", bi->to.sym->name) > 0) {
> -				annotate_t = nr_options++;
> +			if (bi != NULL) {
> +				nr_opts += add_annotate_opt(&opts[nr_opts],
> +							    &optstr[nr_opts],
> +							    browser,
> +							    bi->from.map,
> +							    bi->from.sym);
> +				if (bi->to.sym != bi->from.sym)
> +					nr_opts += add_annotate_opt(&opts[nr_opts],
> +								    &optstr[nr_opts],
> +								    browser,
> +								    bi->to.map,
> +								    bi->to.sym);
>  			}
>  		} else {
> -			if (browser->selection->sym != NULL &&
> -			    !browser->selection->map->dso->annotate_warned) {
> -				struct annotation *notes;
> -
> -				notes = symbol__annotation(browser->selection->sym);
> -
> -				if (notes->src &&
> -				    asprintf(&options[nr_options], "Annotate %s",
> -						 browser->selection->sym->name) > 0) {
> -					annotate = nr_options++;
> -				}
> -			}
> +			nr_opts += add_annotate_opt(&opts[nr_opts],
> +						    &optstr[nr_opts], browser,
> +						    browser->selection->map,
> +						    browser->selection->sym);
>  		}
>  skip_annotation:
> -		if (thread != NULL &&
> -		    asprintf(&options[nr_options], "Zoom %s %s(%d) thread",
> -			     (browser->hists->thread_filter ? "out of" : "into"),
> -			     (thread->comm_set ? thread__comm_str(thread) : ""),
> -			     thread->tid) > 0)
> -			zoom_thread = nr_options++;
> -
> -		if (dso != NULL &&
> -		    asprintf(&options[nr_options], "Zoom %s %s DSO",
> -			     (browser->hists->dso_filter ? "out of" : "into"),
> -			     (dso->kernel ? "the Kernel" : dso->short_name)) > 0)
> -			zoom_dso = nr_options++;
> -
> -		if (browser->selection != NULL &&
> -		    browser->selection->map != NULL &&
> -		    asprintf(&options[nr_options], "Browse map details") > 0)
> -			browse_map = nr_options++;
> +		nr_opts += add_thread_opt(&opts[nr_opts], &optstr[nr_opts],
> +					  browser, thread);
> +		nr_opts += add_dso_opt(&opts[nr_opts], &optstr[nr_opts],
> +				       browser, dso);
> +		nr_opts += add_map_opt(&opts[nr_opts], &optstr[nr_opts],
> +				       browser, browser->selection->map);
>  
>  		/* perf script support */
>  		if (browser->he_selection) {
> -			struct symbol *sym;
> -
> -			if (asprintf(&options[nr_options], "Run scripts for samples of thread [%s]",
> -				     thread__comm_str(browser->he_selection->thread)) > 0)
> -				scripts_comm = nr_options++;
> -
> -			sym = browser->he_selection->ms.sym;
> -			if (sym && sym->namelen &&
> -				asprintf(&options[nr_options], "Run scripts for samples of symbol [%s]",
> -						sym->name) > 0)
> -				scripts_symbol = nr_options++;
> +			nr_opts += add_script_opt(&opts[nr_opts], &optstr[nr_opts],
> +						  browser, thread, NULL);
> +			nr_opts += add_script_opt(&opts[nr_opts], &optstr[nr_opts],
> +						  browser, NULL,
> +						  browser->selection->sym);
>  		}
> +		nr_opts += add_script_opt(&opts[nr_opts], &optstr[nr_opts],
> +					  browser, NULL, NULL);
>  
> -		if (asprintf(&options[nr_options], "Run scripts for all samples") > 0)
> -			scripts_all = nr_options++;
> -
> -		if (is_report_browser(hbt) && asprintf(&options[nr_options],
> -				"Switch to another data file in PWD") > 0)
> -			switch_data = nr_options++;
> +		nr_opts += add_switch_opt(&opts[nr_opts], &optstr[nr_opts],
> +					  browser, hbt);
>  add_exit_option:
> -		if (asprintf(&options[nr_options], "Exit") > 0)
> -			nr_options++;
> -retry_popup_menu:
> -		choice = ui__popup_menu(nr_options, options);
> -
> -		if (choice == nr_options - 1)
> -			break;
> -
> -		if (choice == -1) {
> -			free_popup_options(options, nr_options - 1);
> -			continue;
> -		}
> -
> -		if (choice == annotate || choice == annotate_t || choice == annotate_f) {
> -			struct hist_entry *he;
> -			struct annotation *notes;
> -			struct map_symbol ms;
> -			int err;
> -do_annotate:
> -			if (!objdump_path && perf_session_env__lookup_objdump(env))
> -				continue;
> +		nr_opts += add_exit_opt(&opts[nr_opts], &optstr[nr_opts],
> +					browser);
>  
> -			he = hist_browser__selected_entry(browser);
> -			if (he == NULL)
> -				continue;
> -
> -			if (choice == annotate_f) {
> -				ms.map = he->branch_info->from.map;
> -				ms.sym = he->branch_info->from.sym;
> -			} else if (choice == annotate_t) {
> -				ms.map = he->branch_info->to.map;
> -				ms.sym = he->branch_info->to.sym;
> -			} else {
> -				ms = *browser->selection;
> -			}
> -
> -			notes = symbol__annotation(ms.sym);
> -			if (!notes->src)
> -				continue;
> -
> -			err = map_symbol__tui_annotate(&ms, evsel, hbt);
> -			/*
> -			 * offer option to annotate the other branch source or target
> -			 * (if they exists) when returning from annotate
> -			 */
> -			if ((err == 'q' || err == CTRL('c'))
> -			    && annotate_t != -2 && annotate_f != -2)
> -				goto retry_popup_menu;
> -
> -			ui_browser__update_nr_entries(&browser->b, browser->hists->nr_entries);
> -			if (err)
> -				ui_browser__handle_resize(&browser->b);
> -
> -		} else if (choice == browse_map)
> -			map__browse(browser->selection->map);
> -		else if (choice == zoom_dso) {
> -zoom_dso:
> -			if (browser->hists->dso_filter) {
> -				pstack__remove(fstack, &browser->hists->dso_filter);
> -zoom_out_dso:
> -				ui_helpline__pop();
> -				browser->hists->dso_filter = NULL;
> -				perf_hpp__set_elide(HISTC_DSO, false);
> -			} else {
> -				if (dso == NULL)
> -					continue;
> -				ui_helpline__fpush("To zoom out press <- or -> + \"Zoom out of %s DSO\"",
> -						   dso->kernel ? "the Kernel" : dso->short_name);
> -				browser->hists->dso_filter = dso;
> -				perf_hpp__set_elide(HISTC_DSO, true);
> -				pstack__push(fstack, &browser->hists->dso_filter);
> -			}
> -			hists__filter_by_dso(hists);
> -			hist_browser__reset(browser);
> -		} else if (choice == zoom_thread) {
> -zoom_thread:
> -			if (browser->hists->thread_filter) {
> -				pstack__remove(fstack, &browser->hists->thread_filter);
> -zoom_out_thread:
> -				ui_helpline__pop();
> -				thread__zput(browser->hists->thread_filter);
> -				perf_hpp__set_elide(HISTC_THREAD, false);
> -			} else {
> -				ui_helpline__fpush("To zoom out press <- or -> + \"Zoom out of %s(%d) thread\"",
> -						   thread->comm_set ? thread__comm_str(thread) : "",
> -						   thread->tid);
> -				browser->hists->thread_filter = thread__get(thread);
> -				perf_hpp__set_elide(HISTC_THREAD, false);
> -				pstack__push(fstack, &browser->hists->thread_filter);
> -			}
> -			hists__filter_by_thread(hists);
> -			hist_browser__reset(browser);
> -		}
> -		/* perf scripts support */
> -		else if (choice == scripts_all || choice == scripts_comm ||
> -				choice == scripts_symbol) {
> -do_scripts:
> -			memset(script_opt, 0, 64);
> +		do {
> +			struct popup_option *opt;
>  
> -			if (choice == scripts_comm)
> -				sprintf(script_opt, " -c %s ", thread__comm_str(browser->he_selection->thread));
> +			choice = ui__popup_menu(nr_opts, optstr);
> +			if (choice == -1 || choice >= nr_opts)
> +				break;
>  
> -			if (choice == scripts_symbol)
> -				sprintf(script_opt, " -S %s ", browser->he_selection->ms.sym->name);
> +			opt = &opts[choice];
> +			key = opt->fn(opt, browser, hbt, fstack, env);
> +		} while (key == 1);
>  
> -			script_browse(script_opt);
> -		}
> -		/* Switch to another data file */
> -		else if (choice == switch_data) {
> -do_data_switch:
> -			if (!switch_data_file()) {
> -				key = K_SWITCH_INPUT_DATA;
> -				break;
> -			} else
> -				ui__warning("Won't switch the data files due to\n"
> -					"no valid data file get selected!\n");
> -		}
> +		if (key == K_SWITCH_INPUT_DATA)
> +			break;
>  	}
>  out_free_stack:
>  	pstack__delete(fstack);
>  out:
>  	hist_browser__delete(browser);
> -	free_popup_options(options, ARRAY_SIZE(options));
> +	free_popup_options(optstr, ARRAY_SIZE(optstr));
>  	return key;
>  }
>  
> -- 
> 2.3.5

Re: [PATCH 6/7] perf hists browser: Split popup menu actions

[Namhyung Kim]

Hi Arnaldo,

On Mon, Apr 20, 2015 at 11:00:20AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim escreveu:
> > Currently perf_evsel__hists_browse() function spins on a huge loop and
> > handles many key actions.  Since it's hard to read and modify, let's
> > split it out into small helper functions.
> > 
> > The add_XXX_opt() functions are to register popup menu item on the
> > selected entry.  When it adds an item, it also saves related data into
> > struct popup_option and returns 1 so that it can increase the number of
> > items (nr_opts).  A callback function named do_XXX is called with saved
> > data when the item is selected by user.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/ui/browsers/hists.c | 565 ++++++++++++++++++++++++++---------------
> >  1 file changed, 363 insertions(+), 202 deletions(-)
> > 
> > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> > index cace2df7e561..315ebc493508 100644
> > --- a/tools/perf/ui/browsers/hists.c
> > +++ b/tools/perf/ui/browsers/hists.c
> > @@ -1216,11 +1216,6 @@ static void hist_browser__delete(struct hist_browser *browser)
> >  	free(browser);
> >  }
> >  
> > -static struct hist_entry *hist_browser__selected_entry(struct hist_browser *browser)
> > -{
> > -	return browser->he_selection;
> > -}
> > -
> 
> Why remove the above function? To reduce the patch size you could have
> left it and if the reason for removing it is that compelling, remove it
> in a later patch.

OK, will do.

> 
> >  static struct thread *hist_browser__selected_thread(struct hist_browser *browser)
> >  {
> >  	return browser->he_selection->thread;
> > @@ -1395,6 +1390,281 @@ close_file_and_continue:
> >  	return ret;
> >  }
> >  
> > +struct popup_option {
> > +	struct thread 	*thread;
> > +	struct map 	*map;
> > +	struct dso	*dso;
> > +	struct symbol 	*sym;
> 
> You could use struct map_symbol, that has the three above, right? In
> some cases you would have less lines by doing:
> 
> 	
> 	ms = po->ms;

OK.

> 
> > +	int (*fn)(struct popup_option *opt, struct hist_browser *browser,
> > +		  struct hist_browser_timer *hbt, struct pstack *pstack,
> 
> I wonder if, as a prep patch, you couldn't have browser->hbt, so that we
> would reduce the function signature above. Ditto for pstack.

I don't get it.  The hbt and pstack is needed to annotate and zoom.
Are you saying about step-by-step conversion for each action like
first patch for annotate, seconf for zoom, and so on..?

> 
> 
> Also, you're adding the mechanism (popup_option) at the same time that
> you make those new functions use it.
> 
> I think that it would be better if you first created the functions, call
> them directly, then introduce popup_option (probably better named as
> "popup_action").
> 
> Each patch would be smaller and more easily reviewable, as well bisect
> would work better when locating bugs.

OK, I'll split the patch into steps..

> 
> > +		  struct perf_session_env *env);
> > +};
> > +
> > +static int
> > +do_annotate(struct popup_option *opt, struct hist_browser *browser,
> > +	    struct hist_browser_timer *hbt, struct pstack *pstack __maybe_unused,
> > +	    struct perf_session_env *env)
> > +{
> > +	struct perf_evsel *evsel;
> > +	struct annotation *notes;
> > +	struct map_symbol ms;
> > +	int err;
> > +
> > +	if (!objdump_path && perf_session_env__lookup_objdump(env))
> > +		return 0;
> > +
> > +	ms.map = opt->map;
> > +	ms.sym = opt->sym;
> > +
> > +	notes = symbol__annotation(ms.sym);
> > +	if (!notes->src)
> > +		return 0;
> > +
> > +	evsel = hists_to_evsel(browser->hists);
> > +	err = map_symbol__tui_annotate(&ms, evsel, hbt);
> > +	/*
> > +	 * offer option to annotate the other branch source or target
> > +	 * (if they exists) when returning from annotate
> > +	 */
> > +	if ((err == 'q' || err == CTRL('c'))
> > +	    && browser->he_selection->branch_info)
> 
> Please put the && operator at the end of the first line, even if
> originally it was like that (was it?).

Yes, it was.  I'll change it anyway..

Thanks,
Namhyung

Re: [PATCH 6/7] perf hists browser: Split popup menu actions

[Arnaldo Carvalho de Melo]

Em Tue, Apr 21, 2015 at 12:22:06AM +0900, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> On Mon, Apr 20, 2015 at 11:00:20AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim escreveu:
> > > Currently perf_evsel__hists_browse() function spins on a huge loop and
> > > handles many key actions.  Since it's hard to read and modify, let's
> > > split it out into small helper functions.
> > > 
> > > The add_XXX_opt() functions are to register popup menu item on the
> > > selected entry.  When it adds an item, it also saves related data into
> > > struct popup_option and returns 1 so that it can increase the number of
> > > items (nr_opts).  A callback function named do_XXX is called with saved
> > > data when the item is selected by user.
> > > 
> > > No functional change intended.
> > > 
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > ---
> > >  tools/perf/ui/browsers/hists.c | 565 ++++++++++++++++++++++++++---------------
> > >  1 file changed, 363 insertions(+), 202 deletions(-)
> > > 
> > > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> > > index cace2df7e561..315ebc493508 100644
> > > --- a/tools/perf/ui/browsers/hists.c
> > > +++ b/tools/perf/ui/browsers/hists.c
> > > @@ -1216,11 +1216,6 @@ static void hist_browser__delete(struct hist_browser *browser)
> > >  	free(browser);
> > >  }
> > >  
> > > -static struct hist_entry *hist_browser__selected_entry(struct hist_browser *browser)
> > > -{
> > > -	return browser->he_selection;
> > > -}
> > > -
> > 
> > Why remove the above function? To reduce the patch size you could have
> > left it and if the reason for removing it is that compelling, remove it
> > in a later patch.
> 
> OK, will do.
> 
> > 
> > >  static struct thread *hist_browser__selected_thread(struct hist_browser *browser)
> > >  {
> > >  	return browser->he_selection->thread;
> > > @@ -1395,6 +1390,281 @@ close_file_and_continue:
> > >  	return ret;
> > >  }
> > >  
> > > +struct popup_option {
> > > +	struct thread 	*thread;
> > > +	struct map 	*map;
> > > +	struct dso	*dso;
> > > +	struct symbol 	*sym;
> > 
> > You could use struct map_symbol, that has the three above, right? In
> > some cases you would have less lines by doing:
> > 
> > 	
> > 	ms = po->ms;
> 
> OK.
> 
> > 
> > > +	int (*fn)(struct popup_option *opt, struct hist_browser *browser,
> > > +		  struct hist_browser_timer *hbt, struct pstack *pstack,
> > 
> > I wonder if, as a prep patch, you couldn't have browser->hbt, so that we
> > would reduce the function signature above. Ditto for pstack.
> 
> I don't get it.  The hbt and pstack is needed to annotate and zoom.
> Are you saying about step-by-step conversion for each action like
> first patch for annotate, seconf for zoom, and so on..?

I am saying that pstack was a local variable in that big function,
instead, perhaps we can have it as a member of struct hists_browser, so
that we don't have to pass (struct hist_browser *, struct pstack *),
just (struct hist_browser *).

- Arnaldo

Re: [PATCH 6/7] perf hists browser: Split popup menu actions

[Namhyung Kim]

Hi Arnaldo,

On Mon, Apr 20, 2015 at 06:28:45PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Apr 21, 2015 at 12:22:06AM +0900, Namhyung Kim escreveu:
> > Hi Arnaldo,
> > 
> > On Mon, Apr 20, 2015 at 11:00:20AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim escreveu:
> > > > Currently perf_evsel__hists_browse() function spins on a huge loop and
> > > > handles many key actions.  Since it's hard to read and modify, let's
> > > > split it out into small helper functions.
> > > > 
> > > > The add_XXX_opt() functions are to register popup menu item on the
> > > > selected entry.  When it adds an item, it also saves related data into
> > > > struct popup_option and returns 1 so that it can increase the number of
> > > > items (nr_opts).  A callback function named do_XXX is called with saved
> > > > data when the item is selected by user.
> > > > 
> > > > No functional change intended.
> > > > 
> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > ---
> > > >  tools/perf/ui/browsers/hists.c | 565 ++++++++++++++++++++++++++---------------
> > > >  1 file changed, 363 insertions(+), 202 deletions(-)
> > > > 
> > > > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> > > > index cace2df7e561..315ebc493508 100644
> > > > --- a/tools/perf/ui/browsers/hists.c
> > > > +++ b/tools/perf/ui/browsers/hists.c
> > > > @@ -1216,11 +1216,6 @@ static void hist_browser__delete(struct hist_browser *browser)
> > > >  	free(browser);
> > > >  }
> > > >  
> > > > -static struct hist_entry *hist_browser__selected_entry(struct hist_browser *browser)
> > > > -{
> > > > -	return browser->he_selection;
> > > > -}
> > > > -
> > > 
> > > Why remove the above function? To reduce the patch size you could have
> > > left it and if the reason for removing it is that compelling, remove it
> > > in a later patch.
> > 
> > OK, will do.
> > 
> > > 
> > > >  static struct thread *hist_browser__selected_thread(struct hist_browser *browser)
> > > >  {
> > > >  	return browser->he_selection->thread;
> > > > @@ -1395,6 +1390,281 @@ close_file_and_continue:
> > > >  	return ret;
> > > >  }
> > > >  
> > > > +struct popup_option {
> > > > +	struct thread 	*thread;
> > > > +	struct map 	*map;
> > > > +	struct dso	*dso;
> > > > +	struct symbol 	*sym;
> > > 
> > > You could use struct map_symbol, that has the three above, right? In
> > > some cases you would have less lines by doing:
> > > 
> > > 	
> > > 	ms = po->ms;
> > 
> > OK.
> > 
> > > 
> > > > +	int (*fn)(struct popup_option *opt, struct hist_browser *browser,
> > > > +		  struct hist_browser_timer *hbt, struct pstack *pstack,
> > > 
> > > I wonder if, as a prep patch, you couldn't have browser->hbt, so that we
> > > would reduce the function signature above. Ditto for pstack.
> > 
> > I don't get it.  The hbt and pstack is needed to annotate and zoom.
> > Are you saying about step-by-step conversion for each action like
> > first patch for annotate, seconf for zoom, and so on..?
> 
> I am saying that pstack was a local variable in that big function,
> instead, perhaps we can have it as a member of struct hists_browser, so
> that we don't have to pass (struct hist_browser *, struct pstack *),
> just (struct hist_browser *).

Ah, I got it.  Will change that way.

Thanks,
Namhyung

[PATCH 7/7] perf hists browser: Simplify zooming code a bit

[Namhyung Kim]

Introduce pstack_peek() and reuse do_zoom_dso/thread() function.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 16 +++++-----------
 tools/perf/util/pstack.c       |  7 +++++++
 tools/perf/util/pstack.h       |  1 +
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 315ebc493508..b776c2389137 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1877,17 +1877,11 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 					goto out_free_stack;
 				continue;
 			}
-			top = pstack__pop(fstack);
-			if (top == &browser->hists->dso_filter) {
-				perf_hpp__set_elide(HISTC_DSO, false);
-				browser->hists->dso_filter = NULL;
-				ui_helpline__pop();
-			}
-			if (top == &browser->hists->thread_filter) {
-				perf_hpp__set_elide(HISTC_THREAD, false);
-				thread__zput(browser->hists->thread_filter);
-				ui_helpline__pop();
-			}
+			top = pstack__peek(fstack);
+			if (top == &browser->hists->dso_filter)
+				do_zoom_dso(opts, browser, hbt, fstack, env);
+			if (top == &browser->hists->thread_filter)
+				do_zoom_thread(opts, browser, hbt, fstack, env);
 			continue;
 		}
 		case K_ESC:
diff --git a/tools/perf/util/pstack.c b/tools/perf/util/pstack.c
index a126e6cc6e73..b234a6e3d0d4 100644
--- a/tools/perf/util/pstack.c
+++ b/tools/perf/util/pstack.c
@@ -74,3 +74,10 @@ void *pstack__pop(struct pstack *pstack)
 	pstack->entries[pstack->top] = NULL;
 	return ret;
 }
+
+void *pstack__peek(struct pstack *pstack)
+{
+	if (pstack->top == 0)
+		return NULL;
+	return pstack->entries[pstack->top - 1];
+}
diff --git a/tools/perf/util/pstack.h b/tools/perf/util/pstack.h
index c3cb6584d527..ded7f2e36624 100644
--- a/tools/perf/util/pstack.h
+++ b/tools/perf/util/pstack.h
@@ -10,5 +10,6 @@ bool pstack__empty(const struct pstack *pstack);
 void pstack__remove(struct pstack *pstack, void *key);
 void pstack__push(struct pstack *pstack, void *key);
 void *pstack__pop(struct pstack *pstack);
+void *pstack__peek(struct pstack *pstack);
 
 #endif /* _PERF_PSTACK_ */
-- 
2.3.5

Re: [PATCHSET 0/7] perf tools: Assorted cleanup for TUI (v1)

[Arnaldo Carvalho de Melo]

Em Sun, Apr 19, 2015 at 01:04:08PM +0900, Namhyung Kim escreveu:
> Hello,
> 
> This patches are to cleanup TUI hists browser code for later work.  I
> moved hist_entry_diff and hist_entry_tui under an union in order to
> reduce memory footprint of hist entry.  Also split out hist browser
> functions to make it easier to read.
 
> Namhyung Kim (7):
>   perf tools: Get rid of position field from struct hist_entry
>   perf diff: Make hist_entry_diff fields union

Applied the first two.

- Arnaldo