[PATCHSET 00/10] perf tools: Assorted cleanup for TUI (v3)

[PATCHSET 00/10] perf tools: Assorted cleanup for TUI (v3)

[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.

 * changed in v3)
   - save necessary info in hist_browser  (Arnaldo)
   - rename to struct popup_action  (Arnaldo)
   - split popup registration and callback  (Arnaldo)
   - move TUI-specific fields out of map_symbol

 * changes in v2)
   - add comment on the new union fields  (Jiri)
   - add missing sym->namelen check  (Jiri)
   - fix transient bug in zoom out on LEFT key press  (Jiri)

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

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

Thanks,
Namhyung

Namhyung Kim (10):
  perf tools: Move TUI-specific fields into unnamed union
  perf tools: Move init_have_children field to the unnamed union
  perf hists browser: Fix possible memory leak
  perf hists browser: Save hist_browser_timer pointer in hist_browser
  perf hists browser: Save pstack in the hist_browser
  perf hists browser: Save perf_session_env in the hist_browser
  perf hists browser: Split popup menu actions
  perf hists browser: Split popup menu actions - part 2
  perf hists browser: Simplify zooming code a bit
  perf tools: Move TUI-specific fields out of map_symbol

 tools/perf/ui/browsers/hists.c | 603 +++++++++++++++++++++++++----------------
 tools/perf/util/callchain.h    |   4 +
 tools/perf/util/hist.c         |   2 +-
 tools/perf/util/pstack.c       |   7 +
 tools/perf/util/pstack.h       |   1 +
 tools/perf/util/sort.h         |  22 +-
 tools/perf/util/symbol.h       |   2 -
 7 files changed, 399 insertions(+), 242 deletions(-)

-- 
2.3.5

[PATCH 01/10] perf tools: Move TUI-specific fields into unnamed union

[Namhyung Kim]

Since perf diff only supports stdio output, TUI fields are only accessed
from perf report (or perf top).  So add a new unnamed union and move
struct hist_entry_tui and those TUI-specific fields.

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

diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index de3303fe726d..7f0c0a8d615d 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -93,18 +93,24 @@ 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 {
+		/*
+		 * Since perf diff only supports the stdio output, TUI
+		 * fields are only accessed from perf report (or perf
+		 * top).  So make it an union to reduce memory usage.
+		 */
+		struct hist_entry_diff	diff;
+		struct /* for TUI */ {
+			u16	row_offset;
+			u16	nr_rows;
+		};
+	};
 	char			*srcline;
 	struct symbol		*parent;
 	struct rb_root		sorted_chain;
-- 
2.3.5

Re: [PATCH 01/10] perf tools: Move TUI-specific fields into unnamed union

[Arnaldo Carvalho de Melo]

Em Wed, Apr 22, 2015 at 04:18:12PM +0900, Namhyung Kim escreveu:
> +++ b/tools/perf/util/sort.h
> @@ -93,18 +93,24 @@ struct hist_entry {
>  	u8			cpumode;
> -	struct hist_entry_diff	diff;
> -
> -	/* XXX These two should move to some tree widget lib */
> -	u16			row_offset;
> -	u16			nr_rows;
> +	union {
> +		/*
> +		 * Since perf diff only supports the stdio output, TUI
> +		 * fields are only accessed from perf report (or perf
> +		 * top).  So make it an union to reduce memory usage.
> +		 */
> +		struct hist_entry_diff	diff;
> +		struct /* for TUI */ {
> +			u16	row_offset;
> +			u16	nr_rows;
> +		};
> +	};

Thanks, this way we don't have to touch the diff code, reducing this
patch size.

At some later date, we can shorten the accesses to diff.<fields>, if
struct hist_entry_diff isn't passed as a parameter, etc. But this
certainly is something for later, if at all.

Applying.

- Arnaldo

[tip:perf/core] perf tools: Move TUI-specific fields into unnamed union

[tip-bot for Namhyung Kim]

Commit-ID:  297508216556fbf4e3f70fb97d03280741b4a709
Gitweb:     http://git.kernel.org/tip/297508216556fbf4e3f70fb97d03280741b4a709
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 22 Apr 2015 16:18:12 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 5 May 2015 18:13:10 -0300

perf tools: Move TUI-specific fields into unnamed union

Since perf diff only supports stdio output, TUI fields are only accessed
from perf report (or perf top).  So add a new unnamed union and move
struct hist_entry_tui and those TUI-specific fields.

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

diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index de3303f..7f0c0a8 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -93,18 +93,24 @@ 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 {
+		/*
+		 * Since perf diff only supports the stdio output, TUI
+		 * fields are only accessed from perf report (or perf
+		 * top).  So make it an union to reduce memory usage.
+		 */
+		struct hist_entry_diff	diff;
+		struct /* for TUI */ {
+			u16	row_offset;
+			u16	nr_rows;
+		};
+	};
 	char			*srcline;
 	struct symbol		*parent;
 	struct rb_root		sorted_chain;

[PATCH 02/10] perf tools: Move init_have_children field to the unnamed union

[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 TUI-specific unnamed union member.

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

diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 7f0c0a8d615d..4d923e6e0069 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -96,7 +96,6 @@ struct hist_entry {
 	/* We are added by hists__add_dummy_entry. */
 	bool			dummy;
 
-	bool			init_have_children;
 	char			level;
 	u8			filtered;
 	union {
@@ -109,6 +108,7 @@ struct hist_entry {
 		struct /* for TUI */ {
 			u16	row_offset;
 			u16	nr_rows;
+			bool	init_have_children;
 		};
 	};
 	char			*srcline;
-- 
2.3.5

[tip:perf/core] perf tools: Move init_have_children field to the unnamed union

[tip-bot for Namhyung Kim]

Commit-ID:  d8a0f80042efc4ba08977f3d66fc4678037fe456
Gitweb:     http://git.kernel.org/tip/d8a0f80042efc4ba08977f3d66fc4678037fe456
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 22 Apr 2015 16:18:13 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 5 May 2015 18:13:11 -0300

perf tools: Move init_have_children field to the unnamed union

The init_have_children is used to init callchain info only for TUI.  So
it'd be better to move it to the TUI-specific unnamed union member.

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

diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 7f0c0a8..4d923e6 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -96,7 +96,6 @@ struct hist_entry {
 	/* We are added by hists__add_dummy_entry. */
 	bool			dummy;
 
-	bool			init_have_children;
 	char			level;
 	u8			filtered;
 	union {
@@ -109,6 +108,7 @@ struct hist_entry {
 		struct /* for TUI */ {
 			u16	row_offset;
 			u16	nr_rows;
+			bool	init_have_children;
 		};
 	};
 	char			*srcline;

[PATCH 03/10] 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 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 995b7a8596b1..0972d4722297 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1424,7 +1424,8 @@ 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];
+#define MAX_OPTIONS  16
+	char *options[MAX_OPTIONS];
 	int nr_options = 0;
 	int key = -1;
 	char buf[64];
@@ -1691,7 +1692,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 +1814,7 @@ out_free_stack:
 	pstack__delete(fstack);
 out:
 	hist_browser__delete(browser);
-	free_popup_options(options, nr_options - 1);
+	free_popup_options(options, MAX_OPTIONS);
 	return key;
 }
 
-- 
2.3.5

[tip:perf/core] perf hists browser: Fix possible memory leak

[tip-bot for Namhyung Kim]

Commit-ID:  f2b487db45f2aa203892384f6a08f0a761edad5d
Gitweb:     http://git.kernel.org/tip/f2b487db45f2aa203892384f6a08f0a761edad5d
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 22 Apr 2015 16:18:14 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 5 May 2015 18:13:13 -0300

perf hists browser: Fix possible memory leak

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>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1429687101-4360-4-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 995b7a8..0972d47 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1424,7 +1424,8 @@ 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];
+#define MAX_OPTIONS  16
+	char *options[MAX_OPTIONS];
 	int nr_options = 0;
 	int key = -1;
 	char buf[64];
@@ -1691,7 +1692,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 +1814,7 @@ out_free_stack:
 	pstack__delete(fstack);
 out:
 	hist_browser__delete(browser);
-	free_popup_options(options, nr_options - 1);
+	free_popup_options(options, MAX_OPTIONS);
 	return key;
 }
 

[PATCH 04/10] perf hists browser: Save hist_browser_timer pointer in hist_browser

[Namhyung Kim]

The struct hist_browser_timer is to carry perf-top related info
throughout the hist browser code.  So it'd be better to keep in the
struct hist_browser.  This is a preparation to later change.

Suggested-by: Arnaldo Carvalho de Melo <acme@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 0972d4722297..0847623f42e0 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -25,6 +25,7 @@ struct hist_browser {
 	struct hists	    *hists;
 	struct hist_entry   *he_selection;
 	struct map_symbol   *selection;
+	struct hist_browser_timer *hbt;
 	int		     print_seq;
 	bool		     show_dso;
 	bool		     show_headers;
@@ -406,11 +407,11 @@ static void ui_browser__warn_lost_events(struct ui_browser *browser)
 		"Or reduce the sampling frequency.");
 }
 
-static int hist_browser__run(struct hist_browser *browser,
-			     struct hist_browser_timer *hbt)
+static int hist_browser__run(struct hist_browser *browser)
 {
 	int key;
 	char title[160];
+	struct hist_browser_timer *hbt = browser->hbt;
 	int delay_secs = hbt ? hbt->refresh : 0;
 
 	browser->b.entries = &browser->hists->entries;
@@ -1195,7 +1196,8 @@ static int hist_browser__dump(struct hist_browser *browser)
 	return 0;
 }
 
-static struct hist_browser *hist_browser__new(struct hists *hists)
+static struct hist_browser *hist_browser__new(struct hists *hists,
+					      struct hist_browser_timer *hbt)
 {
 	struct hist_browser *browser = zalloc(sizeof(*browser));
 
@@ -1206,6 +1208,7 @@ static struct hist_browser *hist_browser__new(struct hists *hists)
 		browser->b.seek = ui_browser__hists_seek;
 		browser->b.use_navkeypressed = true;
 		browser->show_headers = symbol_conf.show_hist_headers;
+		browser->hbt = hbt;
 	}
 
 	return browser;
@@ -1421,7 +1424,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 				    struct perf_session_env *env)
 {
 	struct hists *hists = evsel__hists(evsel);
-	struct hist_browser *browser = hist_browser__new(hists);
+	struct hist_browser *browser = hist_browser__new(hists, hbt);
 	struct branch_info *bi;
 	struct pstack *fstack;
 #define MAX_OPTIONS  16
@@ -1499,7 +1502,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 
 		nr_options = 0;
 
-		key = hist_browser__run(browser, hbt);
+		key = hist_browser__run(browser);
 
 		if (browser->he_selection != NULL) {
 			thread = hist_browser__selected_thread(browser);
-- 
2.3.5

[tip:perf/core] perf hists browser: Save hist_browser_timer pointer in hist_browser

[tip-bot for Namhyung Kim]

Commit-ID:  c2a51ab802d17c572cd0a940fd97538b75aa7889
Gitweb:     http://git.kernel.org/tip/c2a51ab802d17c572cd0a940fd97538b75aa7889
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 22 Apr 2015 16:18:15 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 5 May 2015 18:13:14 -0300

perf hists browser: Save hist_browser_timer pointer in hist_browser

The struct hist_browser_timer is to carry perf-top related info
throughout the hist browser code.  So it'd be better to keep in the
struct hist_browser.  This is a preparation to later change.

Suggested-by: Arnaldo Carvalho de Melo <acme@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1429687101-4360-5-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 0972d47..0847623 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -25,6 +25,7 @@ struct hist_browser {
 	struct hists	    *hists;
 	struct hist_entry   *he_selection;
 	struct map_symbol   *selection;
+	struct hist_browser_timer *hbt;
 	int		     print_seq;
 	bool		     show_dso;
 	bool		     show_headers;
@@ -406,11 +407,11 @@ static void ui_browser__warn_lost_events(struct ui_browser *browser)
 		"Or reduce the sampling frequency.");
 }
 
-static int hist_browser__run(struct hist_browser *browser,
-			     struct hist_browser_timer *hbt)
+static int hist_browser__run(struct hist_browser *browser)
 {
 	int key;
 	char title[160];
+	struct hist_browser_timer *hbt = browser->hbt;
 	int delay_secs = hbt ? hbt->refresh : 0;
 
 	browser->b.entries = &browser->hists->entries;
@@ -1195,7 +1196,8 @@ static int hist_browser__dump(struct hist_browser *browser)
 	return 0;
 }
 
-static struct hist_browser *hist_browser__new(struct hists *hists)
+static struct hist_browser *hist_browser__new(struct hists *hists,
+					      struct hist_browser_timer *hbt)
 {
 	struct hist_browser *browser = zalloc(sizeof(*browser));
 
@@ -1206,6 +1208,7 @@ static struct hist_browser *hist_browser__new(struct hists *hists)
 		browser->b.seek = ui_browser__hists_seek;
 		browser->b.use_navkeypressed = true;
 		browser->show_headers = symbol_conf.show_hist_headers;
+		browser->hbt = hbt;
 	}
 
 	return browser;
@@ -1421,7 +1424,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 				    struct perf_session_env *env)
 {
 	struct hists *hists = evsel__hists(evsel);
-	struct hist_browser *browser = hist_browser__new(hists);
+	struct hist_browser *browser = hist_browser__new(hists, hbt);
 	struct branch_info *bi;
 	struct pstack *fstack;
 #define MAX_OPTIONS  16
@@ -1499,7 +1502,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 
 		nr_options = 0;
 
-		key = hist_browser__run(browser, hbt);
+		key = hist_browser__run(browser);
 
 		if (browser->he_selection != NULL) {
 			thread = hist_browser__selected_thread(browser);

[PATCH 05/10] perf hists browser: Save pstack in the hist_browser

[Namhyung Kim]

The struct pstack is to save currently applied thread and/or dso filters
in the browser.  So it'd be better to keep in the struct hist_browser.
This is a preparation to later change.

Suggested-by: Arnaldo Carvalho de Melo <acme@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 0847623f42e0..26d5548b796e 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -26,6 +26,7 @@ struct hist_browser {
 	struct hist_entry   *he_selection;
 	struct map_symbol   *selection;
 	struct hist_browser_timer *hbt;
+	struct pstack	    *pstack;
 	int		     print_seq;
 	bool		     show_dso;
 	bool		     show_headers;
@@ -1426,7 +1427,6 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	struct hists *hists = evsel__hists(evsel);
 	struct hist_browser *browser = hist_browser__new(hists, hbt);
 	struct branch_info *bi;
-	struct pstack *fstack;
 #define MAX_OPTIONS  16
 	char *options[MAX_OPTIONS];
 	int nr_options = 0;
@@ -1477,8 +1477,8 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 		hist_browser__update_nr_entries(browser);
 	}
 
-	fstack = pstack__new(2);
-	if (fstack == NULL)
+	browser->pstack = pstack__new(2);
+	if (browser->pstack == NULL)
 		goto out;
 
 	ui_helpline__push(helpline);
@@ -1587,7 +1587,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 		case K_LEFT: {
 			const void *top;
 
-			if (pstack__empty(fstack)) {
+			if (pstack__empty(browser->pstack)) {
 				/*
 				 * Go back to the perf_evsel_menu__run or other user
 				 */
@@ -1595,7 +1595,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 					goto out_free_stack;
 				continue;
 			}
-			top = pstack__pop(fstack);
+			top = pstack__pop(browser->pstack);
 			if (top == &browser->hists->dso_filter)
 				goto zoom_out_dso;
 			if (top == &browser->hists->thread_filter)
@@ -1753,7 +1753,7 @@ do_annotate:
 		else if (choice == zoom_dso) {
 zoom_dso:
 			if (browser->hists->dso_filter) {
-				pstack__remove(fstack, &browser->hists->dso_filter);
+				pstack__remove(browser->pstack, &browser->hists->dso_filter);
 zoom_out_dso:
 				ui_helpline__pop();
 				browser->hists->dso_filter = NULL;
@@ -1765,14 +1765,14 @@ zoom_out_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);
+				pstack__push(browser->pstack, &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);
+				pstack__remove(browser->pstack, &browser->hists->thread_filter);
 zoom_out_thread:
 				ui_helpline__pop();
 				thread__zput(browser->hists->thread_filter);
@@ -1783,7 +1783,7 @@ zoom_out_thread:
 						   thread->tid);
 				browser->hists->thread_filter = thread__get(thread);
 				perf_hpp__set_elide(HISTC_THREAD, false);
-				pstack__push(fstack, &browser->hists->thread_filter);
+				pstack__push(browser->pstack, &browser->hists->thread_filter);
 			}
 			hists__filter_by_thread(hists);
 			hist_browser__reset(browser);
@@ -1814,7 +1814,7 @@ do_data_switch:
 		}
 	}
 out_free_stack:
-	pstack__delete(fstack);
+	pstack__delete(browser->pstack);
 out:
 	hist_browser__delete(browser);
 	free_popup_options(options, MAX_OPTIONS);
-- 
2.3.5

[tip:perf/core] perf hists browser: Save pstack in the hist_browser

[tip-bot for Namhyung Kim]

Commit-ID:  01f00a1cd158ecd86d5f561ded271597d0550313
Gitweb:     http://git.kernel.org/tip/01f00a1cd158ecd86d5f561ded271597d0550313
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 22 Apr 2015 16:18:16 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 5 May 2015 18:13:16 -0300

perf hists browser: Save pstack in the hist_browser

The struct pstack is to save currently applied thread and/or dso filters
in the browser.  So it'd be better to keep in the struct hist_browser.
This is a preparation to later change.

Suggested-by: Arnaldo Carvalho de Melo <acme@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1429687101-4360-6-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 0847623..26d5548 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -26,6 +26,7 @@ struct hist_browser {
 	struct hist_entry   *he_selection;
 	struct map_symbol   *selection;
 	struct hist_browser_timer *hbt;
+	struct pstack	    *pstack;
 	int		     print_seq;
 	bool		     show_dso;
 	bool		     show_headers;
@@ -1426,7 +1427,6 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	struct hists *hists = evsel__hists(evsel);
 	struct hist_browser *browser = hist_browser__new(hists, hbt);
 	struct branch_info *bi;
-	struct pstack *fstack;
 #define MAX_OPTIONS  16
 	char *options[MAX_OPTIONS];
 	int nr_options = 0;
@@ -1477,8 +1477,8 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 		hist_browser__update_nr_entries(browser);
 	}
 
-	fstack = pstack__new(2);
-	if (fstack == NULL)
+	browser->pstack = pstack__new(2);
+	if (browser->pstack == NULL)
 		goto out;
 
 	ui_helpline__push(helpline);
@@ -1587,7 +1587,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 		case K_LEFT: {
 			const void *top;
 
-			if (pstack__empty(fstack)) {
+			if (pstack__empty(browser->pstack)) {
 				/*
 				 * Go back to the perf_evsel_menu__run or other user
 				 */
@@ -1595,7 +1595,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 					goto out_free_stack;
 				continue;
 			}
-			top = pstack__pop(fstack);
+			top = pstack__pop(browser->pstack);
 			if (top == &browser->hists->dso_filter)
 				goto zoom_out_dso;
 			if (top == &browser->hists->thread_filter)
@@ -1753,7 +1753,7 @@ do_annotate:
 		else if (choice == zoom_dso) {
 zoom_dso:
 			if (browser->hists->dso_filter) {
-				pstack__remove(fstack, &browser->hists->dso_filter);
+				pstack__remove(browser->pstack, &browser->hists->dso_filter);
 zoom_out_dso:
 				ui_helpline__pop();
 				browser->hists->dso_filter = NULL;
@@ -1765,14 +1765,14 @@ zoom_out_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);
+				pstack__push(browser->pstack, &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);
+				pstack__remove(browser->pstack, &browser->hists->thread_filter);
 zoom_out_thread:
 				ui_helpline__pop();
 				thread__zput(browser->hists->thread_filter);
@@ -1783,7 +1783,7 @@ zoom_out_thread:
 						   thread->tid);
 				browser->hists->thread_filter = thread__get(thread);
 				perf_hpp__set_elide(HISTC_THREAD, false);
-				pstack__push(fstack, &browser->hists->thread_filter);
+				pstack__push(browser->pstack, &browser->hists->thread_filter);
 			}
 			hists__filter_by_thread(hists);
 			hist_browser__reset(browser);
@@ -1814,7 +1814,7 @@ do_data_switch:
 		}
 	}
 out_free_stack:
-	pstack__delete(fstack);
+	pstack__delete(browser->pstack);
 out:
 	hist_browser__delete(browser);
 	free_popup_options(options, MAX_OPTIONS);

[PATCH 06/10] perf hists browser: Save perf_session_env in the hist_browser

[Namhyung Kim]

The perf_session_env is to save system informantion at the recording
time to be refered in the hist browser.  So it'd be better to keep in
the struct hist_browser.  This is a preparation to later change.

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

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 26d5548b796e..45704d6383e4 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -27,6 +27,7 @@ struct hist_browser {
 	struct map_symbol   *selection;
 	struct hist_browser_timer *hbt;
 	struct pstack	    *pstack;
+	struct perf_session_env *env;
 	int		     print_seq;
 	bool		     show_dso;
 	bool		     show_headers;
@@ -1198,7 +1199,8 @@ static int hist_browser__dump(struct hist_browser *browser)
 }
 
 static struct hist_browser *hist_browser__new(struct hists *hists,
-					      struct hist_browser_timer *hbt)
+					      struct hist_browser_timer *hbt,
+					      struct perf_session_env *env)
 {
 	struct hist_browser *browser = zalloc(sizeof(*browser));
 
@@ -1210,6 +1212,7 @@ static struct hist_browser *hist_browser__new(struct hists *hists,
 		browser->b.use_navkeypressed = true;
 		browser->show_headers = symbol_conf.show_hist_headers;
 		browser->hbt = hbt;
+		browser->env = env;
 	}
 
 	return browser;
@@ -1425,7 +1428,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 				    struct perf_session_env *env)
 {
 	struct hists *hists = evsel__hists(evsel);
-	struct hist_browser *browser = hist_browser__new(hists, hbt);
+	struct hist_browser *browser = hist_browser__new(hists, hbt, env);
 	struct branch_info *bi;
 #define MAX_OPTIONS  16
 	char *options[MAX_OPTIONS];
-- 
2.3.5

[tip:perf/core] perf hists browser: Save perf_session_env in the hist_browser

[tip-bot for Namhyung Kim]

Commit-ID:  b1a9ceef724341ce05b125d39abf9cfc7059b949
Gitweb:     http://git.kernel.org/tip/b1a9ceef724341ce05b125d39abf9cfc7059b949
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 22 Apr 2015 16:18:17 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 5 May 2015 18:13:17 -0300

perf hists browser: Save perf_session_env in the hist_browser

The perf_session_env is to save system informantion at the recording
time to be refered in the hist browser.  So it'd be better to keep in
the struct hist_browser.  This is a preparation to later change.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1429687101-4360-7-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 26d5548..45704d6 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -27,6 +27,7 @@ struct hist_browser {
 	struct map_symbol   *selection;
 	struct hist_browser_timer *hbt;
 	struct pstack	    *pstack;
+	struct perf_session_env *env;
 	int		     print_seq;
 	bool		     show_dso;
 	bool		     show_headers;
@@ -1198,7 +1199,8 @@ static int hist_browser__dump(struct hist_browser *browser)
 }
 
 static struct hist_browser *hist_browser__new(struct hists *hists,
-					      struct hist_browser_timer *hbt)
+					      struct hist_browser_timer *hbt,
+					      struct perf_session_env *env)
 {
 	struct hist_browser *browser = zalloc(sizeof(*browser));
 
@@ -1210,6 +1212,7 @@ static struct hist_browser *hist_browser__new(struct hists *hists,
 		browser->b.use_navkeypressed = true;
 		browser->show_headers = symbol_conf.show_hist_headers;
 		browser->hbt = hbt;
+		browser->env = env;
 	}
 
 	return browser;
@@ -1425,7 +1428,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 				    struct perf_session_env *env)
 {
 	struct hists *hists = evsel__hists(evsel);
-	struct hist_browser *browser = hist_browser__new(hists, hbt);
+	struct hist_browser *browser = hist_browser__new(hists, hbt, env);
 	struct branch_info *bi;
 #define MAX_OPTIONS  16
 	char *options[MAX_OPTIONS];

[PATCH 07/10] 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.

This patch introduces do_XXX() functions which corresponds to each goto
label.  This way we can call such functions both from key press actions
and popup menu actions.

No functional change intended.

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

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 45704d6383e4..7d88a1cdf04b 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1402,6 +1402,120 @@ close_file_and_continue:
 	return ret;
 }
 
+static int
+do_annotate(struct hist_browser *browser, struct map_symbol *ms)
+{
+	struct perf_evsel *evsel;
+	struct annotation *notes;
+	struct hist_entry *he;
+	int err;
+
+	if (!objdump_path && perf_session_env__lookup_objdump(browser->env))
+		return 0;
+
+	notes = symbol__annotation(ms->sym);
+	if (!notes->src)
+		return 0;
+
+	evsel = hists_to_evsel(browser->hists);
+	err = map_symbol__tui_annotate(ms, evsel, browser->hbt);
+	he = hist_browser__selected_entry(browser);
+	/*
+	 * offer option to annotate the other branch source or target
+	 * (if they exists) when returning from annotate
+	 */
+	if ((err == 'q' || err == CTRL('c')) && he->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
+do_zoom_thread(struct hist_browser *browser, struct thread *thread)
+{
+	if (browser->hists->thread_filter) {
+		pstack__remove(browser->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(browser->pstack, &browser->hists->thread_filter);
+	}
+
+	hists__filter_by_thread(browser->hists);
+	hist_browser__reset(browser);
+	return 0;
+}
+
+static int
+do_zoom_dso(struct hist_browser *browser, struct dso *dso)
+{
+	if (browser->hists->dso_filter) {
+		pstack__remove(browser->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(browser->pstack, &browser->hists->dso_filter);
+	}
+
+	hists__filter_by_dso(browser->hists);
+	hist_browser__reset(browser);
+	return 0;
+}
+
+static int
+do_browse_map(struct hist_browser *browser __maybe_unused, struct map *map)
+{
+	map__browse(map);
+	return 0;
+}
+
+static int
+do_run_script(struct hist_browser *browser __maybe_unused,
+	      struct thread *thread, struct symbol *sym)
+{
+	char script_opt[64];
+	memset(script_opt, 0, sizeof(script_opt));
+
+	if (thread) {
+		scnprintf(script_opt, sizeof(script_opt), " -c %s ",
+			  thread__comm_str(thread));
+	} else if (sym) {
+		scnprintf(script_opt, sizeof(script_opt), " -S %s ",
+			  sym->name);
+	}
+
+	script_browse(script_opt);
+	return 0;
+}
+
+static int
+do_switch_data(struct hist_browser *browser __maybe_unused, int key)
+{
+	if (switch_data_file()) {
+		ui__warning("Won't switch the data files due to\n"
+			    "no valid data file get selected!\n");
+		return key;
+	}
+
+	return K_SWITCH_INPUT_DATA;
+}
+
 static void hist_browser__update_nr_entries(struct hist_browser *hb)
 {
 	u64 nr_entries = 0;
@@ -1435,7 +1549,6 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	int nr_options = 0;
 	int key = -1;
 	char buf[64];
-	char script_opt[64];
 	int delay_secs = hbt ? hbt->refresh : 0;
 	struct perf_hpp_fmt *fmt;
 
@@ -1496,7 +1609,8 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 
 	while (1) {
 		struct thread *thread = NULL;
-		const struct dso *dso = NULL;
+		struct dso *dso = NULL;
+		struct map_symbol ms;
 		int choice = 0,
 		    annotate = -2, zoom_dso = -2, zoom_thread = -2,
 		    annotate_f = -2, annotate_t = -2, browse_map = -2;
@@ -1533,17 +1647,24 @@ 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;
+
+			ms.map = browser->selection->map;
+			ms.sym = browser->selection->sym;
+
+			do_annotate(browser, &ms);
+			continue;
 		case 'P':
 			hist_browser__dump(browser);
 			continue;
 		case 'd':
-			goto zoom_dso;
+			do_zoom_dso(browser, dso);
+			continue;
 		case 'V':
 			browser->show_dso = !browser->show_dso;
 			continue;
 		case 't':
-			goto zoom_thread;
+			do_zoom_thread(browser, thread);
+			continue;
 		case '/':
 			if (ui_browser__input_window("Symbol to show",
 					"Please enter the name of symbol you want to see",
@@ -1556,11 +1677,14 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			continue;
 		case 'r':
 			if (is_report_browser(hbt))
-				goto do_scripts;
+				do_run_script(browser, NULL, NULL);
 			continue;
 		case 's':
-			if (is_report_browser(hbt))
-				goto do_data_switch;
+			if (is_report_browser(hbt)) {
+				key = do_switch_data(browser, key);
+				if (key == K_SWITCH_INPUT_DATA)
+					goto out_free_stack;
+			}
 			continue;
 		case 'i':
 			/* env->arch is NULL for live-mode (i.e. perf top) */
@@ -1599,10 +1723,18 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 				continue;
 			}
 			top = pstack__pop(browser->pstack);
-			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;
+				hists__filter_by_dso(browser->hists);
+			}
+			if (top == &browser->hists->thread_filter) {
+				perf_hpp__set_elide(HISTC_THREAD, false);
+				thread__zput(browser->hists->thread_filter);
+				hists__filter_by_thread(browser->hists);
+			}
+			ui_helpline__pop();
+			hist_browser__reset(browser);
 			continue;
 		}
 		case K_ESC:
@@ -1713,12 +1845,6 @@ retry_popup_menu:
 
 		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;
 
 			he = hist_browser__selected_entry(browser);
 			if (he == NULL)
@@ -1734,86 +1860,30 @@ do_annotate:
 				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)
+			if (do_annotate(browser, &ms) == 1)
 				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(browser->pstack, &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(browser->pstack, &browser->hists->dso_filter);
-			}
-			hists__filter_by_dso(hists);
-			hist_browser__reset(browser);
+		} else if (choice == browse_map) {
+			do_browse_map(browser, browser->selection->map);
+		} else if (choice == zoom_dso) {
+			do_zoom_dso(browser, dso);
 		} else if (choice == zoom_thread) {
-zoom_thread:
-			if (browser->hists->thread_filter) {
-				pstack__remove(browser->pstack, &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(browser->pstack, &browser->hists->thread_filter);
-			}
-			hists__filter_by_thread(hists);
-			hist_browser__reset(browser);
+			do_zoom_thread(browser, thread);
 		}
 		/* perf scripts support */
 		else if (choice == scripts_all || choice == scripts_comm ||
 				choice == scripts_symbol) {
-do_scripts:
-			memset(script_opt, 0, 64);
-
 			if (choice == scripts_comm)
-				sprintf(script_opt, " -c %s ", thread__comm_str(browser->he_selection->thread));
-
+				do_run_script(browser, browser->he_selection->thread, NULL);
 			if (choice == scripts_symbol)
-				sprintf(script_opt, " -S %s ", browser->he_selection->ms.sym->name);
-
-			script_browse(script_opt);
+				do_run_script(browser, NULL, browser->he_selection->ms.sym);
+			if (choice == scripts_all)
+				do_run_script(browser, NULL, NULL);
 		}
 		/* Switch to another data file */
 		else if (choice == switch_data) {
-do_data_switch:
-			if (!switch_data_file()) {
-				key = K_SWITCH_INPUT_DATA;
+			key = do_switch_data(browser, key);
+			if (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");
 		}
 	}
 out_free_stack:
-- 
2.3.5

[tip:perf/core] perf hists browser: Split popup menu actions

[tip-bot for Namhyung Kim]

Commit-ID:  bc7cad429bcdda6f112525c17db9577a1be4c8aa
Gitweb:     http://git.kernel.org/tip/bc7cad429bcdda6f112525c17db9577a1be4c8aa
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 22 Apr 2015 16:18:18 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 5 May 2015 18:13:19 -0300

perf hists browser: Split popup menu actions

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.

This patch introduces do_XXX() functions which corresponds to each goto
label.  This way we can call such functions both from key press actions
and popup menu actions.

No functional change intended.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1429687101-4360-8-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 242 ++++++++++++++++++++++++++---------------
 1 file changed, 156 insertions(+), 86 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 45704d6..7d88a1c 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1402,6 +1402,120 @@ close_file_and_continue:
 	return ret;
 }
 
+static int
+do_annotate(struct hist_browser *browser, struct map_symbol *ms)
+{
+	struct perf_evsel *evsel;
+	struct annotation *notes;
+	struct hist_entry *he;
+	int err;
+
+	if (!objdump_path && perf_session_env__lookup_objdump(browser->env))
+		return 0;
+
+	notes = symbol__annotation(ms->sym);
+	if (!notes->src)
+		return 0;
+
+	evsel = hists_to_evsel(browser->hists);
+	err = map_symbol__tui_annotate(ms, evsel, browser->hbt);
+	he = hist_browser__selected_entry(browser);
+	/*
+	 * offer option to annotate the other branch source or target
+	 * (if they exists) when returning from annotate
+	 */
+	if ((err == 'q' || err == CTRL('c')) && he->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
+do_zoom_thread(struct hist_browser *browser, struct thread *thread)
+{
+	if (browser->hists->thread_filter) {
+		pstack__remove(browser->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(browser->pstack, &browser->hists->thread_filter);
+	}
+
+	hists__filter_by_thread(browser->hists);
+	hist_browser__reset(browser);
+	return 0;
+}
+
+static int
+do_zoom_dso(struct hist_browser *browser, struct dso *dso)
+{
+	if (browser->hists->dso_filter) {
+		pstack__remove(browser->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(browser->pstack, &browser->hists->dso_filter);
+	}
+
+	hists__filter_by_dso(browser->hists);
+	hist_browser__reset(browser);
+	return 0;
+}
+
+static int
+do_browse_map(struct hist_browser *browser __maybe_unused, struct map *map)
+{
+	map__browse(map);
+	return 0;
+}
+
+static int
+do_run_script(struct hist_browser *browser __maybe_unused,
+	      struct thread *thread, struct symbol *sym)
+{
+	char script_opt[64];
+	memset(script_opt, 0, sizeof(script_opt));
+
+	if (thread) {
+		scnprintf(script_opt, sizeof(script_opt), " -c %s ",
+			  thread__comm_str(thread));
+	} else if (sym) {
+		scnprintf(script_opt, sizeof(script_opt), " -S %s ",
+			  sym->name);
+	}
+
+	script_browse(script_opt);
+	return 0;
+}
+
+static int
+do_switch_data(struct hist_browser *browser __maybe_unused, int key)
+{
+	if (switch_data_file()) {
+		ui__warning("Won't switch the data files due to\n"
+			    "no valid data file get selected!\n");
+		return key;
+	}
+
+	return K_SWITCH_INPUT_DATA;
+}
+
 static void hist_browser__update_nr_entries(struct hist_browser *hb)
 {
 	u64 nr_entries = 0;
@@ -1435,7 +1549,6 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	int nr_options = 0;
 	int key = -1;
 	char buf[64];
-	char script_opt[64];
 	int delay_secs = hbt ? hbt->refresh : 0;
 	struct perf_hpp_fmt *fmt;
 
@@ -1496,7 +1609,8 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 
 	while (1) {
 		struct thread *thread = NULL;
-		const struct dso *dso = NULL;
+		struct dso *dso = NULL;
+		struct map_symbol ms;
 		int choice = 0,
 		    annotate = -2, zoom_dso = -2, zoom_thread = -2,
 		    annotate_f = -2, annotate_t = -2, browse_map = -2;
@@ -1533,17 +1647,24 @@ 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;
+
+			ms.map = browser->selection->map;
+			ms.sym = browser->selection->sym;
+
+			do_annotate(browser, &ms);
+			continue;
 		case 'P':
 			hist_browser__dump(browser);
 			continue;
 		case 'd':
-			goto zoom_dso;
+			do_zoom_dso(browser, dso);
+			continue;
 		case 'V':
 			browser->show_dso = !browser->show_dso;
 			continue;
 		case 't':
-			goto zoom_thread;
+			do_zoom_thread(browser, thread);
+			continue;
 		case '/':
 			if (ui_browser__input_window("Symbol to show",
 					"Please enter the name of symbol you want to see",
@@ -1556,11 +1677,14 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			continue;
 		case 'r':
 			if (is_report_browser(hbt))
-				goto do_scripts;
+				do_run_script(browser, NULL, NULL);
 			continue;
 		case 's':
-			if (is_report_browser(hbt))
-				goto do_data_switch;
+			if (is_report_browser(hbt)) {
+				key = do_switch_data(browser, key);
+				if (key == K_SWITCH_INPUT_DATA)
+					goto out_free_stack;
+			}
 			continue;
 		case 'i':
 			/* env->arch is NULL for live-mode (i.e. perf top) */
@@ -1599,10 +1723,18 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 				continue;
 			}
 			top = pstack__pop(browser->pstack);
-			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;
+				hists__filter_by_dso(browser->hists);
+			}
+			if (top == &browser->hists->thread_filter) {
+				perf_hpp__set_elide(HISTC_THREAD, false);
+				thread__zput(browser->hists->thread_filter);
+				hists__filter_by_thread(browser->hists);
+			}
+			ui_helpline__pop();
+			hist_browser__reset(browser);
 			continue;
 		}
 		case K_ESC:
@@ -1713,12 +1845,6 @@ retry_popup_menu:
 
 		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;
 
 			he = hist_browser__selected_entry(browser);
 			if (he == NULL)
@@ -1734,86 +1860,30 @@ do_annotate:
 				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)
+			if (do_annotate(browser, &ms) == 1)
 				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(browser->pstack, &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(browser->pstack, &browser->hists->dso_filter);
-			}
-			hists__filter_by_dso(hists);
-			hist_browser__reset(browser);
+		} else if (choice == browse_map) {
+			do_browse_map(browser, browser->selection->map);
+		} else if (choice == zoom_dso) {
+			do_zoom_dso(browser, dso);
 		} else if (choice == zoom_thread) {
-zoom_thread:
-			if (browser->hists->thread_filter) {
-				pstack__remove(browser->pstack, &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(browser->pstack, &browser->hists->thread_filter);
-			}
-			hists__filter_by_thread(hists);
-			hist_browser__reset(browser);
+			do_zoom_thread(browser, thread);
 		}
 		/* perf scripts support */
 		else if (choice == scripts_all || choice == scripts_comm ||
 				choice == scripts_symbol) {
-do_scripts:
-			memset(script_opt, 0, 64);
-
 			if (choice == scripts_comm)
-				sprintf(script_opt, " -c %s ", thread__comm_str(browser->he_selection->thread));
-
+				do_run_script(browser, browser->he_selection->thread, NULL);
 			if (choice == scripts_symbol)
-				sprintf(script_opt, " -S %s ", browser->he_selection->ms.sym->name);
-
-			script_browse(script_opt);
+				do_run_script(browser, NULL, browser->he_selection->ms.sym);
+			if (choice == scripts_all)
+				do_run_script(browser, NULL, NULL);
 		}
 		/* Switch to another data file */
 		else if (choice == switch_data) {
-do_data_switch:
-			if (!switch_data_file()) {
-				key = K_SWITCH_INPUT_DATA;
+			key = do_switch_data(browser, key);
+			if (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");
 		}
 	}
 out_free_stack:

[PATCH 08/10] perf hists browser: Split popup menu actions - part 2

[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_action and returns 1 so that it can increase the number of
items (nr_options).

With this change, we can simplify the code just to call selected
callback function without considering various conditions.  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 | 354 +++++++++++++++++++++++++----------------
 1 file changed, 214 insertions(+), 140 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 7d88a1cdf04b..9bd7b38de64c 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1402,8 +1402,16 @@ close_file_and_continue:
 	return ret;
 }
 
+struct popup_action {
+	struct thread 		*thread;
+	struct dso		*dso;
+	struct map_symbol 	ms;
+
+	int (*fn)(struct hist_browser *browser, struct popup_action *act);
+};
+
 static int
-do_annotate(struct hist_browser *browser, struct map_symbol *ms)
+do_annotate(struct hist_browser *browser, struct popup_action *act)
 {
 	struct perf_evsel *evsel;
 	struct annotation *notes;
@@ -1413,12 +1421,12 @@ do_annotate(struct hist_browser *browser, struct map_symbol *ms)
 	if (!objdump_path && perf_session_env__lookup_objdump(browser->env))
 		return 0;
 
-	notes = symbol__annotation(ms->sym);
+	notes = symbol__annotation(act->ms.sym);
 	if (!notes->src)
 		return 0;
 
 	evsel = hists_to_evsel(browser->hists);
-	err = map_symbol__tui_annotate(ms, evsel, browser->hbt);
+	err = map_symbol__tui_annotate(&act->ms, evsel, browser->hbt);
 	he = hist_browser__selected_entry(browser);
 	/*
 	 * offer option to annotate the other branch source or target
@@ -1434,8 +1442,27 @@ do_annotate(struct hist_browser *browser, struct map_symbol *ms)
 }
 
 static int
-do_zoom_thread(struct hist_browser *browser, struct thread *thread)
+add_annotate_opt(struct hist_browser *browser __maybe_unused,
+		 struct popup_action *act, char **optstr,
+		 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;
+
+	act->ms.map = map;
+	act->ms.sym = sym;
+	act->fn = do_annotate;
+	return 1;
+}
+
+static int
+do_zoom_thread(struct hist_browser *browser, struct popup_action *act)
+{
+	struct thread *thread = act->thread;
+
 	if (browser->hists->thread_filter) {
 		pstack__remove(browser->pstack, &browser->hists->thread_filter);
 		perf_hpp__set_elide(HISTC_THREAD, false);
@@ -1456,8 +1483,28 @@ do_zoom_thread(struct hist_browser *browser, struct thread *thread)
 }
 
 static int
-do_zoom_dso(struct hist_browser *browser, struct dso *dso)
+add_thread_opt(struct hist_browser *browser, struct popup_action *act,
+	       char **optstr, 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;
+
+	act->thread = thread;
+	act->fn = do_zoom_thread;
+	return 1;
+}
+
+static int
+do_zoom_dso(struct hist_browser *browser, struct popup_action *act)
 {
+	struct dso *dso = act->dso;
+
 	if (browser->hists->dso_filter) {
 		pstack__remove(browser->pstack, &browser->hists->dso_filter);
 		perf_hpp__set_elide(HISTC_DSO, false);
@@ -1479,25 +1526,58 @@ do_zoom_dso(struct hist_browser *browser, struct dso *dso)
 }
 
 static int
-do_browse_map(struct hist_browser *browser __maybe_unused, struct map *map)
+add_dso_opt(struct hist_browser *browser, struct popup_action *act,
+	    char **optstr, struct dso *dso)
 {
-	map__browse(map);
+	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;
+
+	act->dso = dso;
+	act->fn = do_zoom_dso;
+	return 1;
+}
+
+static int
+do_browse_map(struct hist_browser *browser __maybe_unused,
+	      struct popup_action *act)
+{
+	map__browse(act->ms.map);
 	return 0;
 }
 
 static int
+add_map_opt(struct hist_browser *browser __maybe_unused,
+	    struct popup_action *act, char **optstr, struct map *map)
+{
+	if (map == NULL)
+		return 0;
+
+	if (asprintf(optstr, "Browse map details") < 0)
+		return 0;
+
+	act->ms.map = map;
+	act->fn = do_browse_map;
+	return 1;
+}
+
+static int
 do_run_script(struct hist_browser *browser __maybe_unused,
-	      struct thread *thread, struct symbol *sym)
+	      struct popup_action *act)
 {
 	char script_opt[64];
 	memset(script_opt, 0, sizeof(script_opt));
 
-	if (thread) {
+	if (act->thread) {
 		scnprintf(script_opt, sizeof(script_opt), " -c %s ",
-			  thread__comm_str(thread));
-	} else if (sym) {
+			  thread__comm_str(act->thread));
+	} else if (act->ms.sym) {
 		scnprintf(script_opt, sizeof(script_opt), " -S %s ",
-			  sym->name);
+			  act->ms.sym->name);
 	}
 
 	script_browse(script_opt);
@@ -1505,17 +1585,74 @@ do_run_script(struct hist_browser *browser __maybe_unused,
 }
 
 static int
-do_switch_data(struct hist_browser *browser __maybe_unused, int key)
+add_script_opt(struct hist_browser *browser __maybe_unused,
+	       struct popup_action *act, char **optstr,
+	       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;
+	}
+
+	act->thread = thread;
+	act->ms.sym = sym;
+	act->fn = do_run_script;
+	return 1;
+}
+
+static int
+do_switch_data(struct hist_browser *browser __maybe_unused,
+	       struct popup_action *act __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 key;
+		return 0;
 	}
 
 	return K_SWITCH_INPUT_DATA;
 }
 
+static int
+add_switch_opt(struct hist_browser *browser,
+	       struct popup_action *act, char **optstr)
+{
+	if (!is_report_browser(browser->hbt))
+		return 0;
+
+	if (asprintf(optstr, "Switch to another data file in PWD") < 0)
+		return 0;
+
+	act->fn = do_switch_data;
+	return 1;
+}
+
+static int
+do_exit_browser(struct hist_browser *browser __maybe_unused,
+		struct popup_action *act __maybe_unused)
+{
+	return 0;
+}
+
+static int
+add_exit_opt(struct hist_browser *browser __maybe_unused,
+	     struct popup_action *act, char **optstr)
+{
+	if (asprintf(optstr, "Exit") < 0)
+		return 0;
+
+	act->fn = do_exit_browser;
+	return 1;
+}
+
 static void hist_browser__update_nr_entries(struct hist_browser *hb)
 {
 	u64 nr_entries = 0;
@@ -1546,6 +1683,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	struct branch_info *bi;
 #define MAX_OPTIONS  16
 	char *options[MAX_OPTIONS];
+	struct popup_action actions[MAX_OPTIONS];
 	int nr_options = 0;
 	int key = -1;
 	char buf[64];
@@ -1600,6 +1738,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	ui_helpline__push(helpline);
 
 	memset(options, 0, sizeof(options));
+	memset(actions, 0, sizeof(actions));
 
 	perf_hpp__for_each_format(fmt)
 		perf_hpp__reset_width(fmt, hists);
@@ -1610,12 +1749,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	while (1) {
 		struct thread *thread = NULL;
 		struct dso *dso = NULL;
-		struct map_symbol ms;
-		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;
+		int choice = 0;
 
 		nr_options = 0;
 
@@ -1648,22 +1782,23 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			    browser->selection->map->dso->annotate_warned)
 				continue;
 
-			ms.map = browser->selection->map;
-			ms.sym = browser->selection->sym;
-
-			do_annotate(browser, &ms);
+			actions->ms.map = browser->selection->map;
+			actions->ms.sym = browser->selection->sym;
+			do_annotate(browser, actions);
 			continue;
 		case 'P':
 			hist_browser__dump(browser);
 			continue;
 		case 'd':
-			do_zoom_dso(browser, dso);
+			actions->dso = dso;
+			do_zoom_dso(browser, actions);
 			continue;
 		case 'V':
 			browser->show_dso = !browser->show_dso;
 			continue;
 		case 't':
-			do_zoom_thread(browser, thread);
+			actions->thread = thread;
+			do_zoom_thread(browser, actions);
 			continue;
 		case '/':
 			if (ui_browser__input_window("Symbol to show",
@@ -1676,12 +1811,15 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			}
 			continue;
 		case 'r':
-			if (is_report_browser(hbt))
-				do_run_script(browser, NULL, NULL);
+			if (is_report_browser(hbt)) {
+				actions->thread = NULL;
+				actions->ms.sym = NULL;
+				do_run_script(browser, actions);
+			}
 			continue;
 		case 's':
 			if (is_report_browser(hbt)) {
-				key = do_switch_data(browser, key);
+				key = do_switch_data(browser, actions);
 				if (key == K_SWITCH_INPUT_DATA)
 					goto out_free_stack;
 			}
@@ -1762,129 +1900,65 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			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++;
-			}
+			nr_options += add_annotate_opt(browser,
+						       &actions[nr_options],
+						       &options[nr_options],
+						       bi->from.map,
+						       bi->from.sym);
+			if (bi->to.sym != bi->from.sym)
+				nr_options += add_annotate_opt(browser,
+							&actions[nr_options],
+							&options[nr_options],
+							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_options += add_annotate_opt(browser,
+						       &actions[nr_options],
+						       &options[nr_options],
+						       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_options += add_thread_opt(browser, &actions[nr_options],
+					     &options[nr_options], thread);
+		nr_options += add_dso_opt(browser, &actions[nr_options],
+					  &options[nr_options], dso);
+		nr_options += add_map_opt(browser, &actions[nr_options],
+					  &options[nr_options],
+					  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_options += add_script_opt(browser,
+						     &actions[nr_options],
+						     &options[nr_options],
+						     thread, NULL);
+			nr_options += add_script_opt(browser,
+						     &actions[nr_options],
+						     &options[nr_options],
+						     NULL, browser->selection->sym);
 		}
-
-		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_options += add_script_opt(browser, &actions[nr_options],
+					     &options[nr_options], NULL, NULL);
+		nr_options += add_switch_opt(browser, &actions[nr_options],
+					     &options[nr_options]);
 add_exit_option:
-		if (asprintf(&options[nr_options], "Exit") > 0)
-			nr_options++;
-retry_popup_menu:
-		choice = ui__popup_menu(nr_options, options);
+		nr_options += add_exit_opt(browser, &actions[nr_options],
+					   &options[nr_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;
+		do {
+			struct popup_action *act;
 
-			he = hist_browser__selected_entry(browser);
-			if (he == NULL)
-				continue;
+			choice = ui__popup_menu(nr_options, options);
+			if (choice == -1 || choice >= nr_options)
+				break;
 
-			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;
-			}
+			act = &actions[choice];
+			key = act->fn(browser, act);
+		} while (key == 1);
 
-			if (do_annotate(browser, &ms) == 1)
-				goto retry_popup_menu;
-		} else if (choice == browse_map) {
-			do_browse_map(browser, browser->selection->map);
-		} else if (choice == zoom_dso) {
-			do_zoom_dso(browser, dso);
-		} else if (choice == zoom_thread) {
-			do_zoom_thread(browser, thread);
-		}
-		/* perf scripts support */
-		else if (choice == scripts_all || choice == scripts_comm ||
-				choice == scripts_symbol) {
-			if (choice == scripts_comm)
-				do_run_script(browser, browser->he_selection->thread, NULL);
-			if (choice == scripts_symbol)
-				do_run_script(browser, NULL, browser->he_selection->ms.sym);
-			if (choice == scripts_all)
-				do_run_script(browser, NULL, NULL);
-		}
-		/* Switch to another data file */
-		else if (choice == switch_data) {
-			key = do_switch_data(browser, key);
-			if (key == K_SWITCH_INPUT_DATA)
-				break;
-		}
+		if (key == K_SWITCH_INPUT_DATA)
+			break;
 	}
 out_free_stack:
 	pstack__delete(browser->pstack);
-- 
2.3.5

Re: [PATCH 08/10] perf hists browser: Split popup menu actions - part 2

[Jiri Olsa]

On Wed, Apr 22, 2015 at 04:18:19PM +0900, Namhyung Kim wrote:

SNIP

>  	script_browse(script_opt);
> @@ -1505,17 +1585,74 @@ do_run_script(struct hist_browser *browser __maybe_unused,
>  }
>  
>  static int
> -do_switch_data(struct hist_browser *browser __maybe_unused, int key)
> +add_script_opt(struct hist_browser *browser __maybe_unused,
> +	       struct popup_action *act, char **optstr,
> +	       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) {

         ..............  && sym->namelen ;-)

but I think you're right, let's see if there's (sym && !sym->namelen) case,
and fix it at the proper place.. not here

jirka

Re: [PATCH 08/10] perf hists browser: Split popup menu actions - part 2

[Namhyung Kim]

Hi Jiri,

On Wed, Apr 22, 2015 at 12:54:09PM +0200, Jiri Olsa wrote:
> On Wed, Apr 22, 2015 at 04:18:19PM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> >  	script_browse(script_opt);
> > @@ -1505,17 +1585,74 @@ do_run_script(struct hist_browser *browser __maybe_unused,
> >  }
> >  
> >  static int
> > -do_switch_data(struct hist_browser *browser __maybe_unused, int key)
> > +add_script_opt(struct hist_browser *browser __maybe_unused,
> > +	       struct popup_action *act, char **optstr,
> > +	       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) {
> 
>          ..............  && sym->namelen ;-)

Ouch!  sorry for missing this again.

Arnaldo, any chance you can fix this if you're gonna merge it?


> 
> but I think you're right, let's see if there's (sym && !sym->namelen) case,
> and fix it at the proper place.. not here

Agreed.

Thanks,
Namhyung

Re: [PATCH 08/10] perf hists browser: Split popup menu actions - part 2

[Arnaldo Carvalho de Melo]

Em Wed, Apr 22, 2015 at 10:03:25PM +0900, Namhyung Kim escreveu:
> Hi Jiri,
> 
> On Wed, Apr 22, 2015 at 12:54:09PM +0200, Jiri Olsa wrote:
> > On Wed, Apr 22, 2015 at 04:18:19PM +0900, Namhyung Kim wrote:
> > > +		if (asprintf(optstr, "Run scripts for samples of thread [%s]",
> > > +			     thread__comm_str(thread)) < 0)
> > > +			return 0;
> > > +	} else if (sym) {

> >          ..............  && sym->namelen ;-)
 
> Ouch!  sorry for missing this again.
 
> Arnaldo, any chance you can fix this if you're gonna merge it?

I can do that, but that slows down my rate of merging, so, if you could
do like Adrian Hunter did, and fix this up, stick a V2 tag to it, then
send it as a reply to the patch being fixed, that would help, so that
when I get to it, and sort by thread, I will see the fix and will use
it instead.

It'll look like this:

    633 N T 04/09 Adrian Hunter   (4,4K) ├─>[PATCH 24/44] perf auxtrace: Add option to synthesize events for transactions
->  634   T 04/21 Adrian Hunter   (8,9K) │ ┌─>[PATCH V2 23/44] perf tools: Add build option NO_AUXTRACE to exclude AUX area tracing
    635 N T 04/09 Adrian Hunter   (8,8K) ├─>[PATCH 23/44] perf tools: Add build option NO_AUXTRACE to exclude AUX area tracing
    636 N T 04/09 Adrian Hunter   (2,9K) ├─>[PATCH 22/44] perf tools: Hit all build ids when AUX area tracing
    637 N T 04/09 Adrian Hunter   ( 13K) ├─>[PATCH 21/44] perf tools: Add AUX area tracing index
    638 N T 04/09 Adrian Hunter   (6,1K) ├─>[PATCH 20/44] perf inject: Add Instruction Tracing support
    639 N T 04/21 Adrian Hunter   (3,2K) │ ┌─>[PATCH V2 19/44] perf inject: Re-pipe AUX area tracing events
    640 N T 04/09 Adrian Hunter   (3,2K) ├─>[PATCH 19/44] perf inject: Re-pipe AUX area tracing events
    641 N T 04/09 Adrian Hunter   (3,2K) ├─>[PATCH 18/44] perf report: Add Instruction Tracing support

See the "V2" ones? That really helps, thanks Adrian!

Additionaly, Adrian did, in the body of that message:

---- acme ---- acme --- acme --- acme ---

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---


Changes in V2:

        Get rid of MIN()


 tools/perf/Makefile.perf    |   2 +
 tools/perf/builtin-inject.c |  53 +++++++++++-------

---- acme ---- acme --- acme --- acme ---

Which is perfect, I know have the new version and the summary of what
changed, in a way that git-am will trow away, etc. 

- Arnaldo

> > 
> > but I think you're right, let's see if there's (sym && !sym->namelen) case,
> > and fix it at the proper place.. not here
> 
> Agreed.
> 
> Thanks,
> Namhyung

Re: [PATCH 08/10] perf hists browser: Split popup menu actions - part 2

[Namhyung Kim]

On Wed, Apr 22, 2015 at 10:25 PM, Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
> Em Wed, Apr 22, 2015 at 10:03:25PM +0900, Namhyung Kim escreveu:
>> Hi Jiri,
>>
>> On Wed, Apr 22, 2015 at 12:54:09PM +0200, Jiri Olsa wrote:
>> > On Wed, Apr 22, 2015 at 04:18:19PM +0900, Namhyung Kim wrote:
>> > > +         if (asprintf(optstr, "Run scripts for samples of thread [%s]",
>> > > +                      thread__comm_str(thread)) < 0)
>> > > +                 return 0;
>> > > + } else if (sym) {
>
>> >          ..............  && sym->namelen ;-)
>
>> Ouch!  sorry for missing this again.
>
>> Arnaldo, any chance you can fix this if you're gonna merge it?
>
> I can do that, but that slows down my rate of merging, so, if you could
> do like Adrian Hunter did, and fix this up, stick a V2 tag to it, then
> send it as a reply to the patch being fixed, that would help, so that
> when I get to it, and sort by thread, I will see the fix and will use
> it instead.

OK.  Will send v2 soon!

Thanks,
Namhyung


>
> It'll look like this:
>
>     633 N T 04/09 Adrian Hunter   (4,4K) ├─>[PATCH 24/44] perf auxtrace: Add option to synthesize events for transactions
> ->  634   T 04/21 Adrian Hunter   (8,9K) │ ┌─>[PATCH V2 23/44] perf tools: Add build option NO_AUXTRACE to exclude AUX area tracing
>     635 N T 04/09 Adrian Hunter   (8,8K) ├─>[PATCH 23/44] perf tools: Add build option NO_AUXTRACE to exclude AUX area tracing
>     636 N T 04/09 Adrian Hunter   (2,9K) ├─>[PATCH 22/44] perf tools: Hit all build ids when AUX area tracing
>     637 N T 04/09 Adrian Hunter   ( 13K) ├─>[PATCH 21/44] perf tools: Add AUX area tracing index
>     638 N T 04/09 Adrian Hunter   (6,1K) ├─>[PATCH 20/44] perf inject: Add Instruction Tracing support
>     639 N T 04/21 Adrian Hunter   (3,2K) │ ┌─>[PATCH V2 19/44] perf inject: Re-pipe AUX area tracing events
>     640 N T 04/09 Adrian Hunter   (3,2K) ├─>[PATCH 19/44] perf inject: Re-pipe AUX area tracing events
>     641 N T 04/09 Adrian Hunter   (3,2K) ├─>[PATCH 18/44] perf report: Add Instruction Tracing support
>
> See the "V2" ones? That really helps, thanks Adrian!
>
> Additionaly, Adrian did, in the body of that message:
>
> ---- acme ---- acme --- acme --- acme ---
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>
>
> Changes in V2:
>
>         Get rid of MIN()
>
>
>  tools/perf/Makefile.perf    |   2 +
>  tools/perf/builtin-inject.c |  53 +++++++++++-------
>
> ---- acme ---- acme --- acme --- acme ---
>
> Which is perfect, I know have the new version and the summary of what
> changed, in a way that git-am will trow away, etc.
>
> - Arnaldo
>
>> >
>> > but I think you're right, let's see if there's (sym && !sym->namelen) case,
>> > and fix it at the proper place.. not here
>>
>> Agreed.
>>
>> Thanks,
>> Namhyung

Re: [PATCH 08/10] perf hists browser: Split popup menu actions - part 2

[Jiri Olsa]

On Wed, Apr 22, 2015 at 10:32:21PM +0900, Namhyung Kim wrote:
> On Wed, Apr 22, 2015 at 10:25 PM, Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> > Em Wed, Apr 22, 2015 at 10:03:25PM +0900, Namhyung Kim escreveu:
> >> Hi Jiri,
> >>
> >> On Wed, Apr 22, 2015 at 12:54:09PM +0200, Jiri Olsa wrote:
> >> > On Wed, Apr 22, 2015 at 04:18:19PM +0900, Namhyung Kim wrote:
> >> > > +         if (asprintf(optstr, "Run scripts for samples of thread [%s]",
> >> > > +                      thread__comm_str(thread)) < 0)
> >> > > +                 return 0;
> >> > > + } else if (sym) {
> >
> >> >          ..............  && sym->namelen ;-)
> >
> >> Ouch!  sorry for missing this again.
> >
> >> Arnaldo, any chance you can fix this if you're gonna merge it?
> >
> > I can do that, but that slows down my rate of merging, so, if you could
> > do like Adrian Hunter did, and fix this up, stick a V2 tag to it, then
> > send it as a reply to the patch being fixed, that would help, so that
> > when I get to it, and sort by thread, I will see the fix and will use
> > it instead.
> 
> OK.  Will send v2 soon!

hum, I just told Arnaldo that I'm ok with not having the sym->namelen check..

let's leave it as it is now and if we see blank line
then we go and fix it on the proper place

thanks,
jirka

Re: [PATCH 08/10] perf hists browser: Split popup menu actions - part 2

[Namhyung Kim]

On Wed, Apr 22, 2015 at 10:41 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Wed, Apr 22, 2015 at 10:32:21PM +0900, Namhyung Kim wrote:
>> On Wed, Apr 22, 2015 at 10:25 PM, Arnaldo Carvalho de Melo
>> <acme@kernel.org> wrote:
>> > Em Wed, Apr 22, 2015 at 10:03:25PM +0900, Namhyung Kim escreveu:
>> >> Hi Jiri,
>> >>
>> >> On Wed, Apr 22, 2015 at 12:54:09PM +0200, Jiri Olsa wrote:
>> >> > On Wed, Apr 22, 2015 at 04:18:19PM +0900, Namhyung Kim wrote:
>> >> > > +         if (asprintf(optstr, "Run scripts for samples of thread [%s]",
>> >> > > +                      thread__comm_str(thread)) < 0)
>> >> > > +                 return 0;
>> >> > > + } else if (sym) {
>> >
>> >> >          ..............  && sym->namelen ;-)
>> >
>> >> Ouch!  sorry for missing this again.
>> >
>> >> Arnaldo, any chance you can fix this if you're gonna merge it?
>> >
>> > I can do that, but that slows down my rate of merging, so, if you could
>> > do like Adrian Hunter did, and fix this up, stick a V2 tag to it, then
>> > send it as a reply to the patch being fixed, that would help, so that
>> > when I get to it, and sort by thread, I will see the fix and will use
>> > it instead.
>>
>> OK.  Will send v2 soon!
>
> hum, I just told Arnaldo that I'm ok with not having the sym->namelen check..
>
> let's leave it as it is now and if we see blank line
> then we go and fix it on the proper place

Argh, I misunderstood your point,  I thought you wanted to leave the
sym->namelen check here and removing it in another patch with some
more investigation.  Yes, English is hard for me - I feel it more and
more these days ;-)

So it's all OK in the current form, right?

Thanks,
Namhyung

Re: [PATCH 08/10] perf hists browser: Split popup menu actions - part 2

[Jiri Olsa]

On Wed, Apr 22, 2015 at 10:53:45PM +0900, Namhyung Kim wrote:
> On Wed, Apr 22, 2015 at 10:41 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> > On Wed, Apr 22, 2015 at 10:32:21PM +0900, Namhyung Kim wrote:
> >> On Wed, Apr 22, 2015 at 10:25 PM, Arnaldo Carvalho de Melo
> >> <acme@kernel.org> wrote:
> >> > Em Wed, Apr 22, 2015 at 10:03:25PM +0900, Namhyung Kim escreveu:
> >> >> Hi Jiri,
> >> >>
> >> >> On Wed, Apr 22, 2015 at 12:54:09PM +0200, Jiri Olsa wrote:
> >> >> > On Wed, Apr 22, 2015 at 04:18:19PM +0900, Namhyung Kim wrote:
> >> >> > > +         if (asprintf(optstr, "Run scripts for samples of thread [%s]",
> >> >> > > +                      thread__comm_str(thread)) < 0)
> >> >> > > +                 return 0;
> >> >> > > + } else if (sym) {
> >> >
> >> >> >          ..............  && sym->namelen ;-)
> >> >
> >> >> Ouch!  sorry for missing this again.
> >> >
> >> >> Arnaldo, any chance you can fix this if you're gonna merge it?
> >> >
> >> > I can do that, but that slows down my rate of merging, so, if you could
> >> > do like Adrian Hunter did, and fix this up, stick a V2 tag to it, then
> >> > send it as a reply to the patch being fixed, that would help, so that
> >> > when I get to it, and sort by thread, I will see the fix and will use
> >> > it instead.
> >>
> >> OK.  Will send v2 soon!
> >
> > hum, I just told Arnaldo that I'm ok with not having the sym->namelen check..
> >
> > let's leave it as it is now and if we see blank line
> > then we go and fix it on the proper place
> 
> Argh, I misunderstood your point,  I thought you wanted to leave the
> sym->namelen check here and removing it in another patch with some
> more investigation.  Yes, English is hard for me - I feel it more and
> more these days ;-)

no worries, I believe mine is worse ;-)

> 
> So it's all OK in the current form, right?

yep

jirka

[tip:perf/core] perf hists browser: Split popup menu actions - part 2

[tip-bot for Namhyung Kim]

Commit-ID:  ea7cd59233097984850adc0e4119644f089be734
Gitweb:     http://git.kernel.org/tip/ea7cd59233097984850adc0e4119644f089be734
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 22 Apr 2015 16:18:19 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 5 May 2015 18:13:20 -0300

perf hists browser: Split popup menu actions - part 2

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_action and returns 1 so that it can increase the number of
items (nr_options).

With this change, we can simplify the code just to call selected
callback function without considering various conditions.  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>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1429687101-4360-9-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 354 +++++++++++++++++++++++++----------------
 1 file changed, 214 insertions(+), 140 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 7d88a1c..9bd7b38 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1402,8 +1402,16 @@ close_file_and_continue:
 	return ret;
 }
 
+struct popup_action {
+	struct thread 		*thread;
+	struct dso		*dso;
+	struct map_symbol 	ms;
+
+	int (*fn)(struct hist_browser *browser, struct popup_action *act);
+};
+
 static int
-do_annotate(struct hist_browser *browser, struct map_symbol *ms)
+do_annotate(struct hist_browser *browser, struct popup_action *act)
 {
 	struct perf_evsel *evsel;
 	struct annotation *notes;
@@ -1413,12 +1421,12 @@ do_annotate(struct hist_browser *browser, struct map_symbol *ms)
 	if (!objdump_path && perf_session_env__lookup_objdump(browser->env))
 		return 0;
 
-	notes = symbol__annotation(ms->sym);
+	notes = symbol__annotation(act->ms.sym);
 	if (!notes->src)
 		return 0;
 
 	evsel = hists_to_evsel(browser->hists);
-	err = map_symbol__tui_annotate(ms, evsel, browser->hbt);
+	err = map_symbol__tui_annotate(&act->ms, evsel, browser->hbt);
 	he = hist_browser__selected_entry(browser);
 	/*
 	 * offer option to annotate the other branch source or target
@@ -1434,8 +1442,27 @@ do_annotate(struct hist_browser *browser, struct map_symbol *ms)
 }
 
 static int
-do_zoom_thread(struct hist_browser *browser, struct thread *thread)
+add_annotate_opt(struct hist_browser *browser __maybe_unused,
+		 struct popup_action *act, char **optstr,
+		 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;
+
+	act->ms.map = map;
+	act->ms.sym = sym;
+	act->fn = do_annotate;
+	return 1;
+}
+
+static int
+do_zoom_thread(struct hist_browser *browser, struct popup_action *act)
+{
+	struct thread *thread = act->thread;
+
 	if (browser->hists->thread_filter) {
 		pstack__remove(browser->pstack, &browser->hists->thread_filter);
 		perf_hpp__set_elide(HISTC_THREAD, false);
@@ -1456,8 +1483,28 @@ do_zoom_thread(struct hist_browser *browser, struct thread *thread)
 }
 
 static int
-do_zoom_dso(struct hist_browser *browser, struct dso *dso)
+add_thread_opt(struct hist_browser *browser, struct popup_action *act,
+	       char **optstr, 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;
+
+	act->thread = thread;
+	act->fn = do_zoom_thread;
+	return 1;
+}
+
+static int
+do_zoom_dso(struct hist_browser *browser, struct popup_action *act)
 {
+	struct dso *dso = act->dso;
+
 	if (browser->hists->dso_filter) {
 		pstack__remove(browser->pstack, &browser->hists->dso_filter);
 		perf_hpp__set_elide(HISTC_DSO, false);
@@ -1479,25 +1526,58 @@ do_zoom_dso(struct hist_browser *browser, struct dso *dso)
 }
 
 static int
-do_browse_map(struct hist_browser *browser __maybe_unused, struct map *map)
+add_dso_opt(struct hist_browser *browser, struct popup_action *act,
+	    char **optstr, struct dso *dso)
 {
-	map__browse(map);
+	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;
+
+	act->dso = dso;
+	act->fn = do_zoom_dso;
+	return 1;
+}
+
+static int
+do_browse_map(struct hist_browser *browser __maybe_unused,
+	      struct popup_action *act)
+{
+	map__browse(act->ms.map);
 	return 0;
 }
 
 static int
+add_map_opt(struct hist_browser *browser __maybe_unused,
+	    struct popup_action *act, char **optstr, struct map *map)
+{
+	if (map == NULL)
+		return 0;
+
+	if (asprintf(optstr, "Browse map details") < 0)
+		return 0;
+
+	act->ms.map = map;
+	act->fn = do_browse_map;
+	return 1;
+}
+
+static int
 do_run_script(struct hist_browser *browser __maybe_unused,
-	      struct thread *thread, struct symbol *sym)
+	      struct popup_action *act)
 {
 	char script_opt[64];
 	memset(script_opt, 0, sizeof(script_opt));
 
-	if (thread) {
+	if (act->thread) {
 		scnprintf(script_opt, sizeof(script_opt), " -c %s ",
-			  thread__comm_str(thread));
-	} else if (sym) {
+			  thread__comm_str(act->thread));
+	} else if (act->ms.sym) {
 		scnprintf(script_opt, sizeof(script_opt), " -S %s ",
-			  sym->name);
+			  act->ms.sym->name);
 	}
 
 	script_browse(script_opt);
@@ -1505,17 +1585,74 @@ do_run_script(struct hist_browser *browser __maybe_unused,
 }
 
 static int
-do_switch_data(struct hist_browser *browser __maybe_unused, int key)
+add_script_opt(struct hist_browser *browser __maybe_unused,
+	       struct popup_action *act, char **optstr,
+	       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;
+	}
+
+	act->thread = thread;
+	act->ms.sym = sym;
+	act->fn = do_run_script;
+	return 1;
+}
+
+static int
+do_switch_data(struct hist_browser *browser __maybe_unused,
+	       struct popup_action *act __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 key;
+		return 0;
 	}
 
 	return K_SWITCH_INPUT_DATA;
 }
 
+static int
+add_switch_opt(struct hist_browser *browser,
+	       struct popup_action *act, char **optstr)
+{
+	if (!is_report_browser(browser->hbt))
+		return 0;
+
+	if (asprintf(optstr, "Switch to another data file in PWD") < 0)
+		return 0;
+
+	act->fn = do_switch_data;
+	return 1;
+}
+
+static int
+do_exit_browser(struct hist_browser *browser __maybe_unused,
+		struct popup_action *act __maybe_unused)
+{
+	return 0;
+}
+
+static int
+add_exit_opt(struct hist_browser *browser __maybe_unused,
+	     struct popup_action *act, char **optstr)
+{
+	if (asprintf(optstr, "Exit") < 0)
+		return 0;
+
+	act->fn = do_exit_browser;
+	return 1;
+}
+
 static void hist_browser__update_nr_entries(struct hist_browser *hb)
 {
 	u64 nr_entries = 0;
@@ -1546,6 +1683,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	struct branch_info *bi;
 #define MAX_OPTIONS  16
 	char *options[MAX_OPTIONS];
+	struct popup_action actions[MAX_OPTIONS];
 	int nr_options = 0;
 	int key = -1;
 	char buf[64];
@@ -1600,6 +1738,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	ui_helpline__push(helpline);
 
 	memset(options, 0, sizeof(options));
+	memset(actions, 0, sizeof(actions));
 
 	perf_hpp__for_each_format(fmt)
 		perf_hpp__reset_width(fmt, hists);
@@ -1610,12 +1749,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	while (1) {
 		struct thread *thread = NULL;
 		struct dso *dso = NULL;
-		struct map_symbol ms;
-		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;
+		int choice = 0;
 
 		nr_options = 0;
 
@@ -1648,22 +1782,23 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			    browser->selection->map->dso->annotate_warned)
 				continue;
 
-			ms.map = browser->selection->map;
-			ms.sym = browser->selection->sym;
-
-			do_annotate(browser, &ms);
+			actions->ms.map = browser->selection->map;
+			actions->ms.sym = browser->selection->sym;
+			do_annotate(browser, actions);
 			continue;
 		case 'P':
 			hist_browser__dump(browser);
 			continue;
 		case 'd':
-			do_zoom_dso(browser, dso);
+			actions->dso = dso;
+			do_zoom_dso(browser, actions);
 			continue;
 		case 'V':
 			browser->show_dso = !browser->show_dso;
 			continue;
 		case 't':
-			do_zoom_thread(browser, thread);
+			actions->thread = thread;
+			do_zoom_thread(browser, actions);
 			continue;
 		case '/':
 			if (ui_browser__input_window("Symbol to show",
@@ -1676,12 +1811,15 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			}
 			continue;
 		case 'r':
-			if (is_report_browser(hbt))
-				do_run_script(browser, NULL, NULL);
+			if (is_report_browser(hbt)) {
+				actions->thread = NULL;
+				actions->ms.sym = NULL;
+				do_run_script(browser, actions);
+			}
 			continue;
 		case 's':
 			if (is_report_browser(hbt)) {
-				key = do_switch_data(browser, key);
+				key = do_switch_data(browser, actions);
 				if (key == K_SWITCH_INPUT_DATA)
 					goto out_free_stack;
 			}
@@ -1762,129 +1900,65 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			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++;
-			}
+			nr_options += add_annotate_opt(browser,
+						       &actions[nr_options],
+						       &options[nr_options],
+						       bi->from.map,
+						       bi->from.sym);
+			if (bi->to.sym != bi->from.sym)
+				nr_options += add_annotate_opt(browser,
+							&actions[nr_options],
+							&options[nr_options],
+							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_options += add_annotate_opt(browser,
+						       &actions[nr_options],
+						       &options[nr_options],
+						       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_options += add_thread_opt(browser, &actions[nr_options],
+					     &options[nr_options], thread);
+		nr_options += add_dso_opt(browser, &actions[nr_options],
+					  &options[nr_options], dso);
+		nr_options += add_map_opt(browser, &actions[nr_options],
+					  &options[nr_options],
+					  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_options += add_script_opt(browser,
+						     &actions[nr_options],
+						     &options[nr_options],
+						     thread, NULL);
+			nr_options += add_script_opt(browser,
+						     &actions[nr_options],
+						     &options[nr_options],
+						     NULL, browser->selection->sym);
 		}
-
-		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_options += add_script_opt(browser, &actions[nr_options],
+					     &options[nr_options], NULL, NULL);
+		nr_options += add_switch_opt(browser, &actions[nr_options],
+					     &options[nr_options]);
 add_exit_option:
-		if (asprintf(&options[nr_options], "Exit") > 0)
-			nr_options++;
-retry_popup_menu:
-		choice = ui__popup_menu(nr_options, options);
+		nr_options += add_exit_opt(browser, &actions[nr_options],
+					   &options[nr_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;
+		do {
+			struct popup_action *act;
 
-			he = hist_browser__selected_entry(browser);
-			if (he == NULL)
-				continue;
+			choice = ui__popup_menu(nr_options, options);
+			if (choice == -1 || choice >= nr_options)
+				break;
 
-			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;
-			}
+			act = &actions[choice];
+			key = act->fn(browser, act);
+		} while (key == 1);
 
-			if (do_annotate(browser, &ms) == 1)
-				goto retry_popup_menu;
-		} else if (choice == browse_map) {
-			do_browse_map(browser, browser->selection->map);
-		} else if (choice == zoom_dso) {
-			do_zoom_dso(browser, dso);
-		} else if (choice == zoom_thread) {
-			do_zoom_thread(browser, thread);
-		}
-		/* perf scripts support */
-		else if (choice == scripts_all || choice == scripts_comm ||
-				choice == scripts_symbol) {
-			if (choice == scripts_comm)
-				do_run_script(browser, browser->he_selection->thread, NULL);
-			if (choice == scripts_symbol)
-				do_run_script(browser, NULL, browser->he_selection->ms.sym);
-			if (choice == scripts_all)
-				do_run_script(browser, NULL, NULL);
-		}
-		/* Switch to another data file */
-		else if (choice == switch_data) {
-			key = do_switch_data(browser, key);
-			if (key == K_SWITCH_INPUT_DATA)
-				break;
-		}
+		if (key == K_SWITCH_INPUT_DATA)
+			break;
 	}
 out_free_stack:
 	pstack__delete(browser->pstack);

[PATCH 09/10] 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 | 18 +++++-------------
 tools/perf/util/pstack.c       |  7 +++++++
 tools/perf/util/pstack.h       |  1 +
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 9bd7b38de64c..a0da3f9f0513 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1860,19 +1860,11 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 					goto out_free_stack;
 				continue;
 			}
-			top = pstack__pop(browser->pstack);
-			if (top == &browser->hists->dso_filter) {
-				perf_hpp__set_elide(HISTC_DSO, false);
-				browser->hists->dso_filter = NULL;
-				hists__filter_by_dso(browser->hists);
-			}
-			if (top == &browser->hists->thread_filter) {
-				perf_hpp__set_elide(HISTC_THREAD, false);
-				thread__zput(browser->hists->thread_filter);
-				hists__filter_by_thread(browser->hists);
-			}
-			ui_helpline__pop();
-			hist_browser__reset(browser);
+			top = pstack__peek(browser->pstack);
+			if (top == &browser->hists->dso_filter)
+				do_zoom_dso(browser, actions);
+			if (top == &browser->hists->thread_filter)
+				do_zoom_thread(browser, actions);
 			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: [PATCH 09/10] perf hists browser: Simplify zooming code a bit

[Arnaldo Carvalho de Melo]

Em Wed, Apr 22, 2015 at 04:18:20PM +0900, Namhyung Kim escreveu:
> Introduce pstack_peek() and reuse do_zoom_dso/thread() function.

Please break this patch into two, one introducing pstack__peek, then
making use of it, I thought I said this about another patch like this
today :-)

Yeah, to Masami, this one:

[PATCH perf/core v2 3/8] perf probe: Accept multiple filter options

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/browsers/hists.c | 18 +++++-------------
>  tools/perf/util/pstack.c       |  7 +++++++
>  tools/perf/util/pstack.h       |  1 +
>  3 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 9bd7b38de64c..a0da3f9f0513 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -1860,19 +1860,11 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  					goto out_free_stack;
>  				continue;
>  			}
> -			top = pstack__pop(browser->pstack);
> -			if (top == &browser->hists->dso_filter) {
> -				perf_hpp__set_elide(HISTC_DSO, false);
> -				browser->hists->dso_filter = NULL;
> -				hists__filter_by_dso(browser->hists);
> -			}
> -			if (top == &browser->hists->thread_filter) {
> -				perf_hpp__set_elide(HISTC_THREAD, false);
> -				thread__zput(browser->hists->thread_filter);
> -				hists__filter_by_thread(browser->hists);
> -			}
> -			ui_helpline__pop();
> -			hist_browser__reset(browser);
> +			top = pstack__peek(browser->pstack);
> +			if (top == &browser->hists->dso_filter)
> +				do_zoom_dso(browser, actions);
> +			if (top == &browser->hists->thread_filter)
> +				do_zoom_thread(browser, actions);
>  			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

[PATCH v2 9/10] perf tools: Introduce pstack_peek()

[Namhyung Kim]

The pstack_peek() is to get the topmost entry without removing it.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
I also pushed to perf/tui-cleanup-v4.

 tools/perf/util/pstack.c | 7 +++++++
 tools/perf/util/pstack.h | 1 +
 2 files changed, 8 insertions(+)

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.4

[PATCH v2 9.5/10] perf hists browser: Simplify zooming code using pstack_peek()

[Namhyung Kim]

Now LEFT key press action can just use do_zoom_dso/thread() code to
get out of the current filter.

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

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 9bd7b38de64c..8733d577db78 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1860,19 +1860,17 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 					goto out_free_stack;
 				continue;
 			}
-			top = pstack__pop(browser->pstack);
+			top = pstack__peek(browser->pstack);
 			if (top == &browser->hists->dso_filter) {
-				perf_hpp__set_elide(HISTC_DSO, false);
-				browser->hists->dso_filter = NULL;
-				hists__filter_by_dso(browser->hists);
-			}
-			if (top == &browser->hists->thread_filter) {
-				perf_hpp__set_elide(HISTC_THREAD, false);
-				thread__zput(browser->hists->thread_filter);
-				hists__filter_by_thread(browser->hists);
+				/*
+				 * No need to set actions->dso here since
+				 * it's just to remove the current filter.
+				 * Ditto for thread below.
+				 */
+				do_zoom_dso(browser, actions);
 			}
-			ui_helpline__pop();
-			hist_browser__reset(browser);
+			if (top == &browser->hists->thread_filter)
+				do_zoom_thread(browser, actions);
 			continue;
 		}
 		case K_ESC:
-- 
2.3.4

[tip:perf/core] perf hists browser: Simplify zooming code using pstack_peek()

[tip-bot for Namhyung Kim]

Commit-ID:  6422184b087ff4355951d72e0bb320f52e107185
Gitweb:     http://git.kernel.org/tip/6422184b087ff4355951d72e0bb320f52e107185
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Fri, 24 Apr 2015 10:15:33 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 5 May 2015 18:13:23 -0300

perf hists browser: Simplify zooming code using pstack_peek()

Now LEFT key press action can just use do_zoom_dso/thread() code to get
out of the current filter.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1429838133-14001-2-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 9bd7b38..8733d57 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1860,19 +1860,17 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 					goto out_free_stack;
 				continue;
 			}
-			top = pstack__pop(browser->pstack);
+			top = pstack__peek(browser->pstack);
 			if (top == &browser->hists->dso_filter) {
-				perf_hpp__set_elide(HISTC_DSO, false);
-				browser->hists->dso_filter = NULL;
-				hists__filter_by_dso(browser->hists);
-			}
-			if (top == &browser->hists->thread_filter) {
-				perf_hpp__set_elide(HISTC_THREAD, false);
-				thread__zput(browser->hists->thread_filter);
-				hists__filter_by_thread(browser->hists);
+				/*
+				 * No need to set actions->dso here since
+				 * it's just to remove the current filter.
+				 * Ditto for thread below.
+				 */
+				do_zoom_dso(browser, actions);
 			}
-			ui_helpline__pop();
-			hist_browser__reset(browser);
+			if (top == &browser->hists->thread_filter)
+				do_zoom_thread(browser, actions);
 			continue;
 		}
 		case K_ESC:

[tip:perf/core] perf tools: Introduce pstack_peek()

[tip-bot for Namhyung Kim]

Commit-ID:  c8539e3fc630067020814657636b45095edfb5bb
Gitweb:     http://git.kernel.org/tip/c8539e3fc630067020814657636b45095edfb5bb
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Fri, 24 Apr 2015 10:15:32 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 5 May 2015 18:13:22 -0300

perf tools: Introduce pstack_peek()

The pstack_peek() is to get the topmost entry without removing it.

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

diff --git a/tools/perf/util/pstack.c b/tools/perf/util/pstack.c
index a126e6c..b234a6e 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 c3cb658..ded7f2e 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_ */

[PATCH 10/10] perf tools: Move TUI-specific fields out of map_symbol

[Namhyung Kim]

The has_children and unfolded fields don't belong to struct map_symbol
since they're used by TUI only.  Move those fields out of map_symbol
since the struct is also used by other places.

This will also help to compact the sizeof struct hist_entry.

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

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index a0da3f9f0513..3b82e3b8edb0 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -63,7 +63,7 @@ static int hist_browser__get_folding(struct hist_browser *browser)
 		struct hist_entry *he =
 			rb_entry(nd, struct hist_entry, rb_node);
 
-		if (he->ms.unfolded)
+		if (he->unfolded)
 			unfolded_rows += he->nr_rows;
 	}
 	return unfolded_rows;
@@ -139,24 +139,19 @@ static char tree__folded_sign(bool unfolded)
 	return unfolded ? '-' : '+';
 }
 
-static char map_symbol__folded(const struct map_symbol *ms)
-{
-	return ms->has_children ? tree__folded_sign(ms->unfolded) : ' ';
-}
-
 static char hist_entry__folded(const struct hist_entry *he)
 {
-	return map_symbol__folded(&he->ms);
+	return he->has_children ? tree__folded_sign(he->unfolded) : ' ';
 }
 
 static char callchain_list__folded(const struct callchain_list *cl)
 {
-	return map_symbol__folded(&cl->ms);
+	return cl->has_children ? tree__folded_sign(cl->unfolded) : ' ';
 }
 
-static void map_symbol__set_folding(struct map_symbol *ms, bool unfold)
+static void callchain_list__set_folding(struct callchain_list *cl, bool unfold)
 {
-	ms->unfolded = unfold ? ms->has_children : false;
+	cl->unfolded = unfold ? cl->has_children : false;
 }
 
 static int callchain_node__count_rows_rb_tree(struct callchain_node *node)
@@ -192,7 +187,7 @@ static int callchain_node__count_rows(struct callchain_node *node)
 
 	list_for_each_entry(chain, &node->val, list) {
 		++n;
-		unfolded = chain->ms.unfolded;
+		unfolded = chain->unfolded;
 	}
 
 	if (unfolded)
@@ -214,15 +209,15 @@ static int callchain__count_rows(struct rb_root *chain)
 	return n;
 }
 
-static bool map_symbol__toggle_fold(struct map_symbol *ms)
+static bool hist_entry__toggle_fold(struct hist_entry *he)
 {
-	if (!ms)
+	if (!he)
 		return false;
 
-	if (!ms->has_children)
+	if (!he->has_children)
 		return false;
 
-	ms->unfolded = !ms->unfolded;
+	he->unfolded = !he->unfolded;
 	return true;
 }
 
@@ -238,10 +233,10 @@ static void callchain_node__init_have_children_rb_tree(struct callchain_node *no
 		list_for_each_entry(chain, &child->val, list) {
 			if (first) {
 				first = false;
-				chain->ms.has_children = chain->list.next != &child->val ||
+				chain->has_children = chain->list.next != &child->val ||
 							 !RB_EMPTY_ROOT(&child->rb_root);
 			} else
-				chain->ms.has_children = chain->list.next == &child->val &&
+				chain->has_children = chain->list.next == &child->val &&
 							 !RB_EMPTY_ROOT(&child->rb_root);
 		}
 
@@ -255,11 +250,11 @@ static void callchain_node__init_have_children(struct callchain_node *node,
 	struct callchain_list *chain;
 
 	chain = list_entry(node->val.next, struct callchain_list, list);
-	chain->ms.has_children = has_sibling;
+	chain->has_children = has_sibling;
 
 	if (!list_empty(&node->val)) {
 		chain = list_entry(node->val.prev, struct callchain_list, list);
-		chain->ms.has_children = !RB_EMPTY_ROOT(&node->rb_root);
+		chain->has_children = !RB_EMPTY_ROOT(&node->rb_root);
 	}
 
 	callchain_node__init_have_children_rb_tree(node);
@@ -279,7 +274,7 @@ 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) {
-		he->ms.has_children = !RB_EMPTY_ROOT(&he->sorted_chain);
+		he->has_children = !RB_EMPTY_ROOT(&he->sorted_chain);
 		callchain__init_have_children(&he->sorted_chain);
 		he->init_have_children = true;
 	}
@@ -287,14 +282,14 @@ static void hist_entry__init_have_children(struct hist_entry *he)
 
 static bool hist_browser__toggle_fold(struct hist_browser *browser)
 {
-	if (map_symbol__toggle_fold(browser->selection)) {
+	if (hist_entry__toggle_fold(browser->he_selection)) {
 		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;
 
-		if (he->ms.unfolded)
+		if (he->unfolded)
 			he->nr_rows = callchain__count_rows(&he->sorted_chain);
 		else
 			he->nr_rows = 0;
@@ -321,8 +316,8 @@ static int callchain_node__set_folding_rb_tree(struct callchain_node *node, bool
 
 		list_for_each_entry(chain, &child->val, list) {
 			++n;
-			map_symbol__set_folding(&chain->ms, unfold);
-			has_children = chain->ms.has_children;
+			callchain_list__set_folding(chain, unfold);
+			has_children = chain->has_children;
 		}
 
 		if (has_children)
@@ -340,8 +335,8 @@ static int callchain_node__set_folding(struct callchain_node *node, bool unfold)
 
 	list_for_each_entry(chain, &node->val, list) {
 		++n;
-		map_symbol__set_folding(&chain->ms, unfold);
-		has_children = chain->ms.has_children;
+		callchain_list__set_folding(chain, unfold);
+		has_children = chain->has_children;
 	}
 
 	if (has_children)
@@ -366,9 +361,9 @@ static int callchain__set_folding(struct rb_root *chain, bool unfold)
 static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
 {
 	hist_entry__init_have_children(he);
-	map_symbol__set_folding(&he->ms, unfold);
+	hist_entry__set_folding(he, unfold);
 
-	if (he->ms.has_children) {
+	if (he->has_children) {
 		int n = callchain__set_folding(&he->sorted_chain, unfold);
 		he->nr_rows = unfold ? n : 0;
 	} else
@@ -1019,7 +1014,7 @@ do_offset:
 	if (offset > 0) {
 		do {
 			h = rb_entry(nd, struct hist_entry, rb_node);
-			if (h->ms.unfolded) {
+			if (h->unfolded) {
 				u16 remaining = h->nr_rows - h->row_offset;
 				if (offset > remaining) {
 					offset -= remaining;
@@ -1040,7 +1035,7 @@ do_offset:
 	} else if (offset < 0) {
 		while (1) {
 			h = rb_entry(nd, struct hist_entry, rb_node);
-			if (h->ms.unfolded) {
+			if (h->unfolded) {
 				if (first) {
 					if (-offset > h->row_offset) {
 						offset += h->row_offset;
@@ -1077,7 +1072,7 @@ do_offset:
 				 * row_offset at its last entry.
 				 */
 				h = rb_entry(nd, struct hist_entry, rb_node);
-				if (h->ms.unfolded)
+				if (h->unfolded)
 					h->row_offset = h->nr_rows;
 				break;
 			}
diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
index 6033a0a212ca..679c2c6d8ade 100644
--- a/tools/perf/util/callchain.h
+++ b/tools/perf/util/callchain.h
@@ -72,6 +72,10 @@ extern struct callchain_param callchain_param;
 struct callchain_list {
 	u64			ip;
 	struct map_symbol	ms;
+	struct /* for TUI */ {
+		bool		unfolded;
+		bool		has_children;
+	};
 	char		       *srcline;
 	struct list_head	list;
 };
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index cc22b9158b93..338770679863 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1163,7 +1163,7 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
 		return;
 
 	/* force fold unfiltered entry for simplicity */
-	h->ms.unfolded = false;
+	h->unfolded = false;
 	h->row_offset = 0;
 	h->nr_rows = 0;
 
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 4d923e6e0069..e97cd476d336 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -109,6 +109,8 @@ struct hist_entry {
 			u16	row_offset;
 			u16	nr_rows;
 			bool	init_have_children;
+			bool	unfolded;
+			bool	has_children;
 		};
 	};
 	char			*srcline;
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 09561500164a..8483a864a915 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -158,8 +158,6 @@ struct ref_reloc_sym {
 struct map_symbol {
 	struct map    *map;
 	struct symbol *sym;
-	bool	      unfolded;
-	bool	      has_children;
 };
 
 struct addr_map_symbol {
-- 
2.3.5

Re: [PATCH 10/10] perf tools: Move TUI-specific fields out of map_symbol

[Arnaldo Carvalho de Melo]

Em Wed, Apr 22, 2015 at 04:18:21PM +0900, Namhyung Kim escreveu:

> @@ -139,24 +139,19 @@ static char tree__folded_sign(bool unfolded)
>  	return unfolded ? '-' : '+';
>  }
>  
> -static char map_symbol__folded(const struct map_symbol *ms)
> -{
> -	return ms->has_children ? tree__folded_sign(ms->unfolded) : ' ';
> -}
> -

So what is wrong with renaming the above function to
hist_entry__folded() instead of open coding it multiple times? Applied
all the other patches, thanks,

- Arnaldo

>  static char hist_entry__folded(const struct hist_entry *he)
>  {
> -	return map_symbol__folded(&he->ms);
> +	return he->has_children ? tree__folded_sign(he->unfolded) : ' ';
>  }
>  
>  static char callchain_list__folded(const struct callchain_list *cl)
>  {
> -	return map_symbol__folded(&cl->ms);
> +	return cl->has_children ? tree__folded_sign(cl->unfolded) : ' ';
>  }
>  
> -static void map_symbol__set_folding(struct map_symbol *ms, bool unfold)
> +static void callchain_list__set_folding(struct callchain_list *cl, bool unfold)
>  {
> -	ms->unfolded = unfold ? ms->has_children : false;
> +	cl->unfolded = unfold ? cl->has_children : false;
>  }
>  
>  static int callchain_node__count_rows_rb_tree(struct callchain_node *node)
> @@ -192,7 +187,7 @@ static int callchain_node__count_rows(struct callchain_node *node)
>  
>  	list_for_each_entry(chain, &node->val, list) {
>  		++n;
> -		unfolded = chain->ms.unfolded;
> +		unfolded = chain->unfolded;
>  	}
>  
>  	if (unfolded)
> @@ -214,15 +209,15 @@ static int callchain__count_rows(struct rb_root *chain)
>  	return n;
>  }
>  
> -static bool map_symbol__toggle_fold(struct map_symbol *ms)
> +static bool hist_entry__toggle_fold(struct hist_entry *he)
>  {
> -	if (!ms)
> +	if (!he)
>  		return false;
>  
> -	if (!ms->has_children)
> +	if (!he->has_children)
>  		return false;
>  
> -	ms->unfolded = !ms->unfolded;
> +	he->unfolded = !he->unfolded;
>  	return true;
>  }
>  
> @@ -238,10 +233,10 @@ static void callchain_node__init_have_children_rb_tree(struct callchain_node *no
>  		list_for_each_entry(chain, &child->val, list) {
>  			if (first) {
>  				first = false;
> -				chain->ms.has_children = chain->list.next != &child->val ||
> +				chain->has_children = chain->list.next != &child->val ||
>  							 !RB_EMPTY_ROOT(&child->rb_root);
>  			} else
> -				chain->ms.has_children = chain->list.next == &child->val &&
> +				chain->has_children = chain->list.next == &child->val &&
>  							 !RB_EMPTY_ROOT(&child->rb_root);
>  		}
>  
> @@ -255,11 +250,11 @@ static void callchain_node__init_have_children(struct callchain_node *node,
>  	struct callchain_list *chain;
>  
>  	chain = list_entry(node->val.next, struct callchain_list, list);
> -	chain->ms.has_children = has_sibling;
> +	chain->has_children = has_sibling;
>  
>  	if (!list_empty(&node->val)) {
>  		chain = list_entry(node->val.prev, struct callchain_list, list);
> -		chain->ms.has_children = !RB_EMPTY_ROOT(&node->rb_root);
> +		chain->has_children = !RB_EMPTY_ROOT(&node->rb_root);
>  	}
>  
>  	callchain_node__init_have_children_rb_tree(node);
> @@ -279,7 +274,7 @@ 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) {
> -		he->ms.has_children = !RB_EMPTY_ROOT(&he->sorted_chain);
> +		he->has_children = !RB_EMPTY_ROOT(&he->sorted_chain);
>  		callchain__init_have_children(&he->sorted_chain);
>  		he->init_have_children = true;
>  	}
> @@ -287,14 +282,14 @@ static void hist_entry__init_have_children(struct hist_entry *he)
>  
>  static bool hist_browser__toggle_fold(struct hist_browser *browser)
>  {
> -	if (map_symbol__toggle_fold(browser->selection)) {
> +	if (hist_entry__toggle_fold(browser->he_selection)) {
>  		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;
>  
> -		if (he->ms.unfolded)
> +		if (he->unfolded)
>  			he->nr_rows = callchain__count_rows(&he->sorted_chain);
>  		else
>  			he->nr_rows = 0;
> @@ -321,8 +316,8 @@ static int callchain_node__set_folding_rb_tree(struct callchain_node *node, bool
>  
>  		list_for_each_entry(chain, &child->val, list) {
>  			++n;
> -			map_symbol__set_folding(&chain->ms, unfold);
> -			has_children = chain->ms.has_children;
> +			callchain_list__set_folding(chain, unfold);
> +			has_children = chain->has_children;
>  		}
>  
>  		if (has_children)
> @@ -340,8 +335,8 @@ static int callchain_node__set_folding(struct callchain_node *node, bool unfold)
>  
>  	list_for_each_entry(chain, &node->val, list) {
>  		++n;
> -		map_symbol__set_folding(&chain->ms, unfold);
> -		has_children = chain->ms.has_children;
> +		callchain_list__set_folding(chain, unfold);
> +		has_children = chain->has_children;
>  	}
>  
>  	if (has_children)
> @@ -366,9 +361,9 @@ static int callchain__set_folding(struct rb_root *chain, bool unfold)
>  static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
>  {
>  	hist_entry__init_have_children(he);
> -	map_symbol__set_folding(&he->ms, unfold);
> +	hist_entry__set_folding(he, unfold);
>  
> -	if (he->ms.has_children) {
> +	if (he->has_children) {
>  		int n = callchain__set_folding(&he->sorted_chain, unfold);
>  		he->nr_rows = unfold ? n : 0;
>  	} else
> @@ -1019,7 +1014,7 @@ do_offset:
>  	if (offset > 0) {
>  		do {
>  			h = rb_entry(nd, struct hist_entry, rb_node);
> -			if (h->ms.unfolded) {
> +			if (h->unfolded) {
>  				u16 remaining = h->nr_rows - h->row_offset;
>  				if (offset > remaining) {
>  					offset -= remaining;
> @@ -1040,7 +1035,7 @@ do_offset:
>  	} else if (offset < 0) {
>  		while (1) {
>  			h = rb_entry(nd, struct hist_entry, rb_node);
> -			if (h->ms.unfolded) {
> +			if (h->unfolded) {
>  				if (first) {
>  					if (-offset > h->row_offset) {
>  						offset += h->row_offset;
> @@ -1077,7 +1072,7 @@ do_offset:
>  				 * row_offset at its last entry.
>  				 */
>  				h = rb_entry(nd, struct hist_entry, rb_node);
> -				if (h->ms.unfolded)
> +				if (h->unfolded)
>  					h->row_offset = h->nr_rows;
>  				break;
>  			}
> diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
> index 6033a0a212ca..679c2c6d8ade 100644
> --- a/tools/perf/util/callchain.h
> +++ b/tools/perf/util/callchain.h
> @@ -72,6 +72,10 @@ extern struct callchain_param callchain_param;
>  struct callchain_list {
>  	u64			ip;
>  	struct map_symbol	ms;
> +	struct /* for TUI */ {
> +		bool		unfolded;
> +		bool		has_children;
> +	};
>  	char		       *srcline;
>  	struct list_head	list;
>  };
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index cc22b9158b93..338770679863 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -1163,7 +1163,7 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
>  		return;
>  
>  	/* force fold unfiltered entry for simplicity */
> -	h->ms.unfolded = false;
> +	h->unfolded = false;
>  	h->row_offset = 0;
>  	h->nr_rows = 0;
>  
> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index 4d923e6e0069..e97cd476d336 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -109,6 +109,8 @@ struct hist_entry {
>  			u16	row_offset;
>  			u16	nr_rows;
>  			bool	init_have_children;
> +			bool	unfolded;
> +			bool	has_children;
>  		};
>  	};
>  	char			*srcline;
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 09561500164a..8483a864a915 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -158,8 +158,6 @@ struct ref_reloc_sym {
>  struct map_symbol {
>  	struct map    *map;
>  	struct symbol *sym;
> -	bool	      unfolded;
> -	bool	      has_children;
>  };
>  
>  struct addr_map_symbol {
> -- 
> 2.3.5

Re: [PATCH 10/10] perf tools: Move TUI-specific fields out of map_symbol

[Namhyung Kim]

Hi Arnaldo,

On Mon, Apr 27, 2015 at 02:20:36PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 22, 2015 at 04:18:21PM +0900, Namhyung Kim escreveu:
> 
> > @@ -139,24 +139,19 @@ static char tree__folded_sign(bool unfolded)
> >  	return unfolded ? '-' : '+';
> >  }
> >  
> > -static char map_symbol__folded(const struct map_symbol *ms)
> > -{
> > -	return ms->has_children ? tree__folded_sign(ms->unfolded) : ' ';
> > -}
> > -
> 
> So what is wrong with renaming the above function to
> hist_entry__folded() instead of open coding it multiple times? Applied
> all the other patches, thanks,

Please see below..

The struct map_symbol__folded() was called from hist_entry__folded()
and callchain_list__folded().  I just removed map_symbol__folded()
since it does not contain the info anymore and open coded it just for
the two callers.  The real callers of these functions are not touched.

> 
> >  static char hist_entry__folded(const struct hist_entry *he)
> >  {
> > -	return map_symbol__folded(&he->ms);
> > +	return he->has_children ? tree__folded_sign(he->unfolded) : ' ';
> >  }
> >  
> >  static char callchain_list__folded(const struct callchain_list *cl)
> >  {
> > -	return map_symbol__folded(&cl->ms);
> > +	return cl->has_children ? tree__folded_sign(cl->unfolded) : ' ';
> >  }

Here.

Thanks,
Namhyung


> >  
> > -static void map_symbol__set_folding(struct map_symbol *ms, bool unfold)
> > +static void callchain_list__set_folding(struct callchain_list *cl, bool unfold)
> >  {
> > -	ms->unfolded = unfold ? ms->has_children : false;
> > +	cl->unfolded = unfold ? cl->has_children : false;
> >  }
> >  
> >  static int callchain_node__count_rows_rb_tree(struct callchain_node *node)
> > @@ -192,7 +187,7 @@ static int callchain_node__count_rows(struct callchain_node *node)
> >  
> >  	list_for_each_entry(chain, &node->val, list) {
> >  		++n;
> > -		unfolded = chain->ms.unfolded;
> > +		unfolded = chain->unfolded;
> >  	}
> >  
> >  	if (unfolded)
> > @@ -214,15 +209,15 @@ static int callchain__count_rows(struct rb_root *chain)
> >  	return n;
> >  }
> >  
> > -static bool map_symbol__toggle_fold(struct map_symbol *ms)
> > +static bool hist_entry__toggle_fold(struct hist_entry *he)
> >  {
> > -	if (!ms)
> > +	if (!he)
> >  		return false;
> >  
> > -	if (!ms->has_children)
> > +	if (!he->has_children)
> >  		return false;
> >  
> > -	ms->unfolded = !ms->unfolded;
> > +	he->unfolded = !he->unfolded;
> >  	return true;
> >  }
> >  
> > @@ -238,10 +233,10 @@ static void callchain_node__init_have_children_rb_tree(struct callchain_node *no
> >  		list_for_each_entry(chain, &child->val, list) {
> >  			if (first) {
> >  				first = false;
> > -				chain->ms.has_children = chain->list.next != &child->val ||
> > +				chain->has_children = chain->list.next != &child->val ||
> >  							 !RB_EMPTY_ROOT(&child->rb_root);
> >  			} else
> > -				chain->ms.has_children = chain->list.next == &child->val &&
> > +				chain->has_children = chain->list.next == &child->val &&
> >  							 !RB_EMPTY_ROOT(&child->rb_root);
> >  		}
> >  
> > @@ -255,11 +250,11 @@ static void callchain_node__init_have_children(struct callchain_node *node,
> >  	struct callchain_list *chain;
> >  
> >  	chain = list_entry(node->val.next, struct callchain_list, list);
> > -	chain->ms.has_children = has_sibling;
> > +	chain->has_children = has_sibling;
> >  
> >  	if (!list_empty(&node->val)) {
> >  		chain = list_entry(node->val.prev, struct callchain_list, list);
> > -		chain->ms.has_children = !RB_EMPTY_ROOT(&node->rb_root);
> > +		chain->has_children = !RB_EMPTY_ROOT(&node->rb_root);
> >  	}
> >  
> >  	callchain_node__init_have_children_rb_tree(node);
> > @@ -279,7 +274,7 @@ 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) {
> > -		he->ms.has_children = !RB_EMPTY_ROOT(&he->sorted_chain);
> > +		he->has_children = !RB_EMPTY_ROOT(&he->sorted_chain);
> >  		callchain__init_have_children(&he->sorted_chain);
> >  		he->init_have_children = true;
> >  	}
> > @@ -287,14 +282,14 @@ static void hist_entry__init_have_children(struct hist_entry *he)
> >  
> >  static bool hist_browser__toggle_fold(struct hist_browser *browser)
> >  {
> > -	if (map_symbol__toggle_fold(browser->selection)) {
> > +	if (hist_entry__toggle_fold(browser->he_selection)) {
> >  		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;
> >  
> > -		if (he->ms.unfolded)
> > +		if (he->unfolded)
> >  			he->nr_rows = callchain__count_rows(&he->sorted_chain);
> >  		else
> >  			he->nr_rows = 0;
> > @@ -321,8 +316,8 @@ static int callchain_node__set_folding_rb_tree(struct callchain_node *node, bool
> >  
> >  		list_for_each_entry(chain, &child->val, list) {
> >  			++n;
> > -			map_symbol__set_folding(&chain->ms, unfold);
> > -			has_children = chain->ms.has_children;
> > +			callchain_list__set_folding(chain, unfold);
> > +			has_children = chain->has_children;
> >  		}
> >  
> >  		if (has_children)
> > @@ -340,8 +335,8 @@ static int callchain_node__set_folding(struct callchain_node *node, bool unfold)
> >  
> >  	list_for_each_entry(chain, &node->val, list) {
> >  		++n;
> > -		map_symbol__set_folding(&chain->ms, unfold);
> > -		has_children = chain->ms.has_children;
> > +		callchain_list__set_folding(chain, unfold);
> > +		has_children = chain->has_children;
> >  	}
> >  
> >  	if (has_children)
> > @@ -366,9 +361,9 @@ static int callchain__set_folding(struct rb_root *chain, bool unfold)
> >  static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
> >  {
> >  	hist_entry__init_have_children(he);
> > -	map_symbol__set_folding(&he->ms, unfold);
> > +	hist_entry__set_folding(he, unfold);
> >  
> > -	if (he->ms.has_children) {
> > +	if (he->has_children) {
> >  		int n = callchain__set_folding(&he->sorted_chain, unfold);
> >  		he->nr_rows = unfold ? n : 0;
> >  	} else
> > @@ -1019,7 +1014,7 @@ do_offset:
> >  	if (offset > 0) {
> >  		do {
> >  			h = rb_entry(nd, struct hist_entry, rb_node);
> > -			if (h->ms.unfolded) {
> > +			if (h->unfolded) {
> >  				u16 remaining = h->nr_rows - h->row_offset;
> >  				if (offset > remaining) {
> >  					offset -= remaining;
> > @@ -1040,7 +1035,7 @@ do_offset:
> >  	} else if (offset < 0) {
> >  		while (1) {
> >  			h = rb_entry(nd, struct hist_entry, rb_node);
> > -			if (h->ms.unfolded) {
> > +			if (h->unfolded) {
> >  				if (first) {
> >  					if (-offset > h->row_offset) {
> >  						offset += h->row_offset;
> > @@ -1077,7 +1072,7 @@ do_offset:
> >  				 * row_offset at its last entry.
> >  				 */
> >  				h = rb_entry(nd, struct hist_entry, rb_node);
> > -				if (h->ms.unfolded)
> > +				if (h->unfolded)
> >  					h->row_offset = h->nr_rows;
> >  				break;
> >  			}
> > diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
> > index 6033a0a212ca..679c2c6d8ade 100644
> > --- a/tools/perf/util/callchain.h
> > +++ b/tools/perf/util/callchain.h
> > @@ -72,6 +72,10 @@ extern struct callchain_param callchain_param;
> >  struct callchain_list {
> >  	u64			ip;
> >  	struct map_symbol	ms;
> > +	struct /* for TUI */ {
> > +		bool		unfolded;
> > +		bool		has_children;
> > +	};
> >  	char		       *srcline;
> >  	struct list_head	list;
> >  };
> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > index cc22b9158b93..338770679863 100644
> > --- a/tools/perf/util/hist.c
> > +++ b/tools/perf/util/hist.c
> > @@ -1163,7 +1163,7 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
> >  		return;
> >  
> >  	/* force fold unfiltered entry for simplicity */
> > -	h->ms.unfolded = false;
> > +	h->unfolded = false;
> >  	h->row_offset = 0;
> >  	h->nr_rows = 0;
> >  
> > diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> > index 4d923e6e0069..e97cd476d336 100644
> > --- a/tools/perf/util/sort.h
> > +++ b/tools/perf/util/sort.h
> > @@ -109,6 +109,8 @@ struct hist_entry {
> >  			u16	row_offset;
> >  			u16	nr_rows;
> >  			bool	init_have_children;
> > +			bool	unfolded;
> > +			bool	has_children;
> >  		};
> >  	};
> >  	char			*srcline;
> > diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> > index 09561500164a..8483a864a915 100644
> > --- a/tools/perf/util/symbol.h
> > +++ b/tools/perf/util/symbol.h
> > @@ -158,8 +158,6 @@ struct ref_reloc_sym {
> >  struct map_symbol {
> >  	struct map    *map;
> >  	struct symbol *sym;
> > -	bool	      unfolded;
> > -	bool	      has_children;
> >  };
> >  
> >  struct addr_map_symbol {
> > -- 
> > 2.3.5
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 10/10] perf tools: Move TUI-specific fields out of map_symbol

[Arnaldo Carvalho de Melo]

Em Tue, Apr 28, 2015 at 04:30:29PM +0900, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> On Mon, Apr 27, 2015 at 02:20:36PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Apr 22, 2015 at 04:18:21PM +0900, Namhyung Kim escreveu:
> > 
> > > @@ -139,24 +139,19 @@ static char tree__folded_sign(bool unfolded)
> > >  	return unfolded ? '-' : '+';
> > >  }
> > >  
> > > -static char map_symbol__folded(const struct map_symbol *ms)
> > > -{
> > > -	return ms->has_children ? tree__folded_sign(ms->unfolded) : ' ';
> > > -}
> > > -
> > 
> > So what is wrong with renaming the above function to
> > hist_entry__folded() instead of open coding it multiple times? Applied
> > all the other patches, thanks,
> 
> Please see below..

> The struct map_symbol__folded() was called from hist_entry__folded()
> and callchain_list__folded().  I just removed map_symbol__folded()
> since it does not contain the info anymore and open coded it just for
> the two callers.  The real callers of these functions are not touched.

You are right, nevermind, ENOCOFFEE or something, applying!

- Arnaldo
 
> > >  static char hist_entry__folded(const struct hist_entry *he)
> > >  {
> > > -	return map_symbol__folded(&he->ms);
> > > +	return he->has_children ? tree__folded_sign(he->unfolded) : ' ';
> > >  }
> > >  
> > >  static char callchain_list__folded(const struct callchain_list *cl)
> > >  {
> > > -	return map_symbol__folded(&cl->ms);
> > > +	return cl->has_children ? tree__folded_sign(cl->unfolded) : ' ';
> > >  }
> 
> Here.
> 
> Thanks,
> Namhyung
> 
> 
> > >  
> > > -static void map_symbol__set_folding(struct map_symbol *ms, bool unfold)
> > > +static void callchain_list__set_folding(struct callchain_list *cl, bool unfold)
> > >  {
> > > -	ms->unfolded = unfold ? ms->has_children : false;
> > > +	cl->unfolded = unfold ? cl->has_children : false;
> > >  }
> > >  
> > >  static int callchain_node__count_rows_rb_tree(struct callchain_node *node)
> > > @@ -192,7 +187,7 @@ static int callchain_node__count_rows(struct callchain_node *node)
> > >  
> > >  	list_for_each_entry(chain, &node->val, list) {
> > >  		++n;
> > > -		unfolded = chain->ms.unfolded;
> > > +		unfolded = chain->unfolded;
> > >  	}
> > >  
> > >  	if (unfolded)
> > > @@ -214,15 +209,15 @@ static int callchain__count_rows(struct rb_root *chain)
> > >  	return n;
> > >  }
> > >  
> > > -static bool map_symbol__toggle_fold(struct map_symbol *ms)
> > > +static bool hist_entry__toggle_fold(struct hist_entry *he)
> > >  {
> > > -	if (!ms)
> > > +	if (!he)
> > >  		return false;
> > >  
> > > -	if (!ms->has_children)
> > > +	if (!he->has_children)
> > >  		return false;
> > >  
> > > -	ms->unfolded = !ms->unfolded;
> > > +	he->unfolded = !he->unfolded;
> > >  	return true;
> > >  }
> > >  
> > > @@ -238,10 +233,10 @@ static void callchain_node__init_have_children_rb_tree(struct callchain_node *no
> > >  		list_for_each_entry(chain, &child->val, list) {
> > >  			if (first) {
> > >  				first = false;
> > > -				chain->ms.has_children = chain->list.next != &child->val ||
> > > +				chain->has_children = chain->list.next != &child->val ||
> > >  							 !RB_EMPTY_ROOT(&child->rb_root);
> > >  			} else
> > > -				chain->ms.has_children = chain->list.next == &child->val &&
> > > +				chain->has_children = chain->list.next == &child->val &&
> > >  							 !RB_EMPTY_ROOT(&child->rb_root);
> > >  		}
> > >  
> > > @@ -255,11 +250,11 @@ static void callchain_node__init_have_children(struct callchain_node *node,
> > >  	struct callchain_list *chain;
> > >  
> > >  	chain = list_entry(node->val.next, struct callchain_list, list);
> > > -	chain->ms.has_children = has_sibling;
> > > +	chain->has_children = has_sibling;
> > >  
> > >  	if (!list_empty(&node->val)) {
> > >  		chain = list_entry(node->val.prev, struct callchain_list, list);
> > > -		chain->ms.has_children = !RB_EMPTY_ROOT(&node->rb_root);
> > > +		chain->has_children = !RB_EMPTY_ROOT(&node->rb_root);
> > >  	}
> > >  
> > >  	callchain_node__init_have_children_rb_tree(node);
> > > @@ -279,7 +274,7 @@ 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) {
> > > -		he->ms.has_children = !RB_EMPTY_ROOT(&he->sorted_chain);
> > > +		he->has_children = !RB_EMPTY_ROOT(&he->sorted_chain);
> > >  		callchain__init_have_children(&he->sorted_chain);
> > >  		he->init_have_children = true;
> > >  	}
> > > @@ -287,14 +282,14 @@ static void hist_entry__init_have_children(struct hist_entry *he)
> > >  
> > >  static bool hist_browser__toggle_fold(struct hist_browser *browser)
> > >  {
> > > -	if (map_symbol__toggle_fold(browser->selection)) {
> > > +	if (hist_entry__toggle_fold(browser->he_selection)) {
> > >  		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;
> > >  
> > > -		if (he->ms.unfolded)
> > > +		if (he->unfolded)
> > >  			he->nr_rows = callchain__count_rows(&he->sorted_chain);
> > >  		else
> > >  			he->nr_rows = 0;
> > > @@ -321,8 +316,8 @@ static int callchain_node__set_folding_rb_tree(struct callchain_node *node, bool
> > >  
> > >  		list_for_each_entry(chain, &child->val, list) {
> > >  			++n;
> > > -			map_symbol__set_folding(&chain->ms, unfold);
> > > -			has_children = chain->ms.has_children;
> > > +			callchain_list__set_folding(chain, unfold);
> > > +			has_children = chain->has_children;
> > >  		}
> > >  
> > >  		if (has_children)
> > > @@ -340,8 +335,8 @@ static int callchain_node__set_folding(struct callchain_node *node, bool unfold)
> > >  
> > >  	list_for_each_entry(chain, &node->val, list) {
> > >  		++n;
> > > -		map_symbol__set_folding(&chain->ms, unfold);
> > > -		has_children = chain->ms.has_children;
> > > +		callchain_list__set_folding(chain, unfold);
> > > +		has_children = chain->has_children;
> > >  	}
> > >  
> > >  	if (has_children)
> > > @@ -366,9 +361,9 @@ static int callchain__set_folding(struct rb_root *chain, bool unfold)
> > >  static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
> > >  {
> > >  	hist_entry__init_have_children(he);
> > > -	map_symbol__set_folding(&he->ms, unfold);
> > > +	hist_entry__set_folding(he, unfold);
> > >  
> > > -	if (he->ms.has_children) {
> > > +	if (he->has_children) {
> > >  		int n = callchain__set_folding(&he->sorted_chain, unfold);
> > >  		he->nr_rows = unfold ? n : 0;
> > >  	} else
> > > @@ -1019,7 +1014,7 @@ do_offset:
> > >  	if (offset > 0) {
> > >  		do {
> > >  			h = rb_entry(nd, struct hist_entry, rb_node);
> > > -			if (h->ms.unfolded) {
> > > +			if (h->unfolded) {
> > >  				u16 remaining = h->nr_rows - h->row_offset;
> > >  				if (offset > remaining) {
> > >  					offset -= remaining;
> > > @@ -1040,7 +1035,7 @@ do_offset:
> > >  	} else if (offset < 0) {
> > >  		while (1) {
> > >  			h = rb_entry(nd, struct hist_entry, rb_node);
> > > -			if (h->ms.unfolded) {
> > > +			if (h->unfolded) {
> > >  				if (first) {
> > >  					if (-offset > h->row_offset) {
> > >  						offset += h->row_offset;
> > > @@ -1077,7 +1072,7 @@ do_offset:
> > >  				 * row_offset at its last entry.
> > >  				 */
> > >  				h = rb_entry(nd, struct hist_entry, rb_node);
> > > -				if (h->ms.unfolded)
> > > +				if (h->unfolded)
> > >  					h->row_offset = h->nr_rows;
> > >  				break;
> > >  			}
> > > diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
> > > index 6033a0a212ca..679c2c6d8ade 100644
> > > --- a/tools/perf/util/callchain.h
> > > +++ b/tools/perf/util/callchain.h
> > > @@ -72,6 +72,10 @@ extern struct callchain_param callchain_param;
> > >  struct callchain_list {
> > >  	u64			ip;
> > >  	struct map_symbol	ms;
> > > +	struct /* for TUI */ {
> > > +		bool		unfolded;
> > > +		bool		has_children;
> > > +	};
> > >  	char		       *srcline;
> > >  	struct list_head	list;
> > >  };
> > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > > index cc22b9158b93..338770679863 100644
> > > --- a/tools/perf/util/hist.c
> > > +++ b/tools/perf/util/hist.c
> > > @@ -1163,7 +1163,7 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
> > >  		return;
> > >  
> > >  	/* force fold unfiltered entry for simplicity */
> > > -	h->ms.unfolded = false;
> > > +	h->unfolded = false;
> > >  	h->row_offset = 0;
> > >  	h->nr_rows = 0;
> > >  
> > > diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> > > index 4d923e6e0069..e97cd476d336 100644
> > > --- a/tools/perf/util/sort.h
> > > +++ b/tools/perf/util/sort.h
> > > @@ -109,6 +109,8 @@ struct hist_entry {
> > >  			u16	row_offset;
> > >  			u16	nr_rows;
> > >  			bool	init_have_children;
> > > +			bool	unfolded;
> > > +			bool	has_children;
> > >  		};
> > >  	};
> > >  	char			*srcline;
> > > diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> > > index 09561500164a..8483a864a915 100644
> > > --- a/tools/perf/util/symbol.h
> > > +++ b/tools/perf/util/symbol.h
> > > @@ -158,8 +158,6 @@ struct ref_reloc_sym {
> > >  struct map_symbol {
> > >  	struct map    *map;
> > >  	struct symbol *sym;
> > > -	bool	      unfolded;
> > > -	bool	      has_children;
> > >  };
> > >  
> > >  struct addr_map_symbol {
> > > -- 
> > > 2.3.5
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 10/10] perf tools: Move TUI-specific fields out of map_symbol

[Arnaldo Carvalho de Melo]

Em Wed, Apr 22, 2015 at 04:18:21PM +0900, Namhyung Kim escreveu:
> The has_children and unfolded fields don't belong to struct map_symbol
> since they're used by TUI only.  Move those fields out of map_symbol
> since the struct is also used by other places.
> 
> This will also help to compact the sizeof struct hist_entry.

So, while testing your 'perf kmem' patchkit, I noticed that I couldn't,
on a 'perf report --no-ch' (probably affects the children case as well,
have not tested that), expand the first callchain entry, I did a bisect
on my perf/core branch and it ended in this patch:


[acme@ssdandy linux]$ git bisect good 
7ee14d744acb33e5b3114484d7d850caecb339a2 is the first bad commit
commit 7ee14d744acb33e5b3114484d7d850caecb339a2
Author: Namhyung Kim <namhyung@kernel.org>
Date:   Wed Apr 22 16:18:21 2015 +0900

    perf tools: Move TUI-specific fields out of map_symbol
    
    The has_children and unfolded fields don't belong to struct map_symbol
    since they're used by TUI only.  Move those fields out of map_symbol
    since the struct is also used by other places.
    
    This will also help to compact the sizeof struct hist_entry.
    
    Signed-off-by: Namhyung Kim <namhyung@kernel.org>
    Acked-by: Jiri Olsa <jolsa@redhat.com>
    Cc: David Ahern <dsahern@gmail.com>
    Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
    Link: http://lkml.kernel.org/r/1429687101-4360-11-git-send-email-namhyung@kernel.org
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

:040000 040000 a72f0be94a86bbdad1cc38d0ce1e8578337612f4 4f84cc2bac01666a97b918aef0151b07e9a4015c M	tools
[acme@ssdandy linux]$ 

So I am moving this series to the top of perf/core and then moving it to a
separate branch, so that you can try to reproduce this and I can push the rest
to Ingo, ok?

Ah, while trying to find new_slab() in callchains one other thing I tried was
to press 'E', to expand all the callchains, and it segfaulted, unsure if this
is related.

Will send another message with the branch details when done.

- Arnaldo

Re: [PATCH 10/10] perf tools: Move TUI-specific fields out of map_symbol

[Arnaldo Carvalho de Melo]

Em Mon, May 04, 2015 at 12:42:27PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Apr 22, 2015 at 04:18:21PM +0900, Namhyung Kim escreveu:
> > The has_children and unfolded fields don't belong to struct map_symbol
> > since they're used by TUI only.  Move those fields out of map_symbol
> > since the struct is also used by other places.

> > This will also help to compact the sizeof struct hist_entry.

> 7ee14d744acb33e5b3114484d7d850caecb339a2 is the first bad commit
> Author: Namhyung Kim <namhyung@kernel.org>
> Date:   Wed Apr 22 16:18:21 2015 +0900
 
>     perf tools: Move TUI-specific fields out of map_symbol
 
> So I am moving this series to the top of perf/core and then moving it to a
> separate branch, so that you can try to reproduce this and I can push the rest
> to Ingo, ok?

Moved to perf/core-hists_browser
 
> Ah, while trying to find new_slab() in callchains one other thing I tried was
> to press 'E', to expand all the callchains, and it segfaulted, unsure if this
> is related.

It is related to this patchset, without it, I can press 'E' to expand
all callchains and then 'C' to collapse them all as usual.

- Arnaldo

Re: [PATCH 10/10] perf tools: Move TUI-specific fields out of map_symbol

[Namhyung Kim]

Hi Arnaldo,

On Mon, May 04, 2015 at 12:51:16PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, May 04, 2015 at 12:42:27PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Wed, Apr 22, 2015 at 04:18:21PM +0900, Namhyung Kim escreveu:
> > > The has_children and unfolded fields don't belong to struct map_symbol
> > > since they're used by TUI only.  Move those fields out of map_symbol
> > > since the struct is also used by other places.
> 
> > > This will also help to compact the sizeof struct hist_entry.
> 
> > 7ee14d744acb33e5b3114484d7d850caecb339a2 is the first bad commit
> > Author: Namhyung Kim <namhyung@kernel.org>
> > Date:   Wed Apr 22 16:18:21 2015 +0900
>  
> >     perf tools: Move TUI-specific fields out of map_symbol
>  
> > So I am moving this series to the top of perf/core and then moving it to a
> > separate branch, so that you can try to reproduce this and I can push the rest
> > to Ingo, ok?
> 
> Moved to perf/core-hists_browser
>  
> > Ah, while trying to find new_slab() in callchains one other thing I tried was
> > to press 'E', to expand all the callchains, and it segfaulted, unsure if this
> > is related.
> 
> It is related to this patchset, without it, I can press 'E' to expand
> all callchains and then 'C' to collapse them all as usual.

I tested and reproduced it.  There's a recursion by mistake during
refactoring.  The fix is below, I'll send v2.

Thanks,
Namhyung


diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index f422e5e82a6a..7a70493e938c 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -361,7 +361,7 @@ static int callchain__set_folding(struct rb_root *chain, bool unfold)
 static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
 {
 	hist_entry__init_have_children(he);
-	hist_entry__set_folding(he, unfold);
+	he->unfolded = unfold ? he->has_children : false;
 
 	if (he->has_children) {
 		int n = callchain__set_folding(&he->sorted_chain, unfold);

Re: [PATCH 10/10] perf tools: Move TUI-specific fields out of map_symbol

[Arnaldo Carvalho de Melo]

Em Tue, May 05, 2015 at 10:12:45AM +0900, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> On Mon, May 04, 2015 at 12:51:16PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, May 04, 2015 at 12:42:27PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Wed, Apr 22, 2015 at 04:18:21PM +0900, Namhyung Kim escreveu:
> > > > The has_children and unfolded fields don't belong to struct map_symbol
> > > > since they're used by TUI only.  Move those fields out of map_symbol
> > > > since the struct is also used by other places.
> > 
> > > > This will also help to compact the sizeof struct hist_entry.
> > 
> > > 7ee14d744acb33e5b3114484d7d850caecb339a2 is the first bad commit
> > > Author: Namhyung Kim <namhyung@kernel.org>
> > > Date:   Wed Apr 22 16:18:21 2015 +0900
> >  
> > >     perf tools: Move TUI-specific fields out of map_symbol
> >  
> > > So I am moving this series to the top of perf/core and then moving it to a
> > > separate branch, so that you can try to reproduce this and I can push the rest
> > > to Ingo, ok?
> > 
> > Moved to perf/core-hists_browser
> >  
> > > Ah, while trying to find new_slab() in callchains one other thing I tried was
> > > to press 'E', to expand all the callchains, and it segfaulted, unsure if this
> > > is related.
> > 
> > It is related to this patchset, without it, I can press 'E' to expand
> > all callchains and then 'C' to collapse them all as usual.
> 
> I tested and reproduced it.  There's a recursion by mistake during
> refactoring.  The fix is below, I'll send v2.

Does it fix those two reported problems? I'll check.

- Arnaldo
 
> Thanks,
> Namhyung
> 
> 
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index f422e5e82a6a..7a70493e938c 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -361,7 +361,7 @@ static int callchain__set_folding(struct rb_root *chain, bool unfold)
>  static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
>  {
>  	hist_entry__init_have_children(he);
> -	hist_entry__set_folding(he, unfold);
> +	he->unfolded = unfold ? he->has_children : false;
>  
>  	if (he->has_children) {
>  		int n = callchain__set_folding(&he->sorted_chain, unfold);

Re: [PATCH 10/10] perf tools: Move TUI-specific fields out of map_symbol

[Namhyung Kim]

Hi Arnaldo,

On Tue, May 5, 2015 at 11:07 PM, Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
> Em Tue, May 05, 2015 at 10:12:45AM +0900, Namhyung Kim escreveu:
>> Hi Arnaldo,
>>
>> On Mon, May 04, 2015 at 12:51:16PM -0300, Arnaldo Carvalho de Melo wrote:
>> > Em Mon, May 04, 2015 at 12:42:27PM -0300, Arnaldo Carvalho de Melo escreveu:
>> > > Em Wed, Apr 22, 2015 at 04:18:21PM +0900, Namhyung Kim escreveu:
>> > > > The has_children and unfolded fields don't belong to struct map_symbol
>> > > > since they're used by TUI only.  Move those fields out of map_symbol
>> > > > since the struct is also used by other places.
>> >
>> > > > This will also help to compact the sizeof struct hist_entry.
>> >
>> > > 7ee14d744acb33e5b3114484d7d850caecb339a2 is the first bad commit
>> > > Author: Namhyung Kim <namhyung@kernel.org>
>> > > Date:   Wed Apr 22 16:18:21 2015 +0900
>> >
>> > >     perf tools: Move TUI-specific fields out of map_symbol
>> >
>> > > So I am moving this series to the top of perf/core and then moving it to a
>> > > separate branch, so that you can try to reproduce this and I can push the rest
>> > > to Ingo, ok?
>> >
>> > Moved to perf/core-hists_browser
>> >
>> > > Ah, while trying to find new_slab() in callchains one other thing I tried was
>> > > to press 'E', to expand all the callchains, and it segfaulted, unsure if this
>> > > is related.
>> >
>> > It is related to this patchset, without it, I can press 'E' to expand
>> > all callchains and then 'C' to collapse them all as usual.
>>
>> I tested and reproduced it.  There's a recursion by mistake during
>> refactoring.  The fix is below, I'll send v2.
>
> Does it fix those two reported problems? I'll check.

Oh, I missed the first bug.. this patch just fixes the segfault
problem.  I'll look into the first one too.

Thanks,
Namhyung

[PATCH v2 10/10] perf tools: Move TUI-specific fields out of map_symbol

[Namhyung Kim]

The has_children and unfolded fields don't belong to struct map_symbol
since they're used by TUI only.  Move those fields out of map_symbol
since the struct is also used by other places.

This will also help to compact the sizeof struct hist_entry.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1429687101-4360-11-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
fix segfault due to recursion in hist_entry__set_folding().

 tools/perf/ui/browsers/hists.c | 57 +++++++++++++++++++-----------------------
 tools/perf/util/callchain.h    |  4 +++
 tools/perf/util/hist.c         |  2 +-
 tools/perf/util/sort.h         |  2 ++
 tools/perf/util/symbol.h       |  2 --
 5 files changed, 33 insertions(+), 34 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 8733d577db78..7a70493e938c 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -63,7 +63,7 @@ static int hist_browser__get_folding(struct hist_browser *browser)
 		struct hist_entry *he =
 			rb_entry(nd, struct hist_entry, rb_node);
 
-		if (he->ms.unfolded)
+		if (he->unfolded)
 			unfolded_rows += he->nr_rows;
 	}
 	return unfolded_rows;
@@ -139,24 +139,19 @@ static char tree__folded_sign(bool unfolded)
 	return unfolded ? '-' : '+';
 }
 
-static char map_symbol__folded(const struct map_symbol *ms)
-{
-	return ms->has_children ? tree__folded_sign(ms->unfolded) : ' ';
-}
-
 static char hist_entry__folded(const struct hist_entry *he)
 {
-	return map_symbol__folded(&he->ms);
+	return he->has_children ? tree__folded_sign(he->unfolded) : ' ';
 }
 
 static char callchain_list__folded(const struct callchain_list *cl)
 {
-	return map_symbol__folded(&cl->ms);
+	return cl->has_children ? tree__folded_sign(cl->unfolded) : ' ';
 }
 
-static void map_symbol__set_folding(struct map_symbol *ms, bool unfold)
+static void callchain_list__set_folding(struct callchain_list *cl, bool unfold)
 {
-	ms->unfolded = unfold ? ms->has_children : false;
+	cl->unfolded = unfold ? cl->has_children : false;
 }
 
 static int callchain_node__count_rows_rb_tree(struct callchain_node *node)
@@ -192,7 +187,7 @@ static int callchain_node__count_rows(struct callchain_node *node)
 
 	list_for_each_entry(chain, &node->val, list) {
 		++n;
-		unfolded = chain->ms.unfolded;
+		unfolded = chain->unfolded;
 	}
 
 	if (unfolded)
@@ -214,15 +209,15 @@ static int callchain__count_rows(struct rb_root *chain)
 	return n;
 }
 
-static bool map_symbol__toggle_fold(struct map_symbol *ms)
+static bool hist_entry__toggle_fold(struct hist_entry *he)
 {
-	if (!ms)
+	if (!he)
 		return false;
 
-	if (!ms->has_children)
+	if (!he->has_children)
 		return false;
 
-	ms->unfolded = !ms->unfolded;
+	he->unfolded = !he->unfolded;
 	return true;
 }
 
@@ -238,10 +233,10 @@ static void callchain_node__init_have_children_rb_tree(struct callchain_node *no
 		list_for_each_entry(chain, &child->val, list) {
 			if (first) {
 				first = false;
-				chain->ms.has_children = chain->list.next != &child->val ||
+				chain->has_children = chain->list.next != &child->val ||
 							 !RB_EMPTY_ROOT(&child->rb_root);
 			} else
-				chain->ms.has_children = chain->list.next == &child->val &&
+				chain->has_children = chain->list.next == &child->val &&
 							 !RB_EMPTY_ROOT(&child->rb_root);
 		}
 
@@ -255,11 +250,11 @@ static void callchain_node__init_have_children(struct callchain_node *node,
 	struct callchain_list *chain;
 
 	chain = list_entry(node->val.next, struct callchain_list, list);
-	chain->ms.has_children = has_sibling;
+	chain->has_children = has_sibling;
 
 	if (!list_empty(&node->val)) {
 		chain = list_entry(node->val.prev, struct callchain_list, list);
-		chain->ms.has_children = !RB_EMPTY_ROOT(&node->rb_root);
+		chain->has_children = !RB_EMPTY_ROOT(&node->rb_root);
 	}
 
 	callchain_node__init_have_children_rb_tree(node);
@@ -279,7 +274,7 @@ 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) {
-		he->ms.has_children = !RB_EMPTY_ROOT(&he->sorted_chain);
+		he->has_children = !RB_EMPTY_ROOT(&he->sorted_chain);
 		callchain__init_have_children(&he->sorted_chain);
 		he->init_have_children = true;
 	}
@@ -287,14 +282,14 @@ static void hist_entry__init_have_children(struct hist_entry *he)
 
 static bool hist_browser__toggle_fold(struct hist_browser *browser)
 {
-	if (map_symbol__toggle_fold(browser->selection)) {
+	if (hist_entry__toggle_fold(browser->he_selection)) {
 		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;
 
-		if (he->ms.unfolded)
+		if (he->unfolded)
 			he->nr_rows = callchain__count_rows(&he->sorted_chain);
 		else
 			he->nr_rows = 0;
@@ -321,8 +316,8 @@ static int callchain_node__set_folding_rb_tree(struct callchain_node *node, bool
 
 		list_for_each_entry(chain, &child->val, list) {
 			++n;
-			map_symbol__set_folding(&chain->ms, unfold);
-			has_children = chain->ms.has_children;
+			callchain_list__set_folding(chain, unfold);
+			has_children = chain->has_children;
 		}
 
 		if (has_children)
@@ -340,8 +335,8 @@ static int callchain_node__set_folding(struct callchain_node *node, bool unfold)
 
 	list_for_each_entry(chain, &node->val, list) {
 		++n;
-		map_symbol__set_folding(&chain->ms, unfold);
-		has_children = chain->ms.has_children;
+		callchain_list__set_folding(chain, unfold);
+		has_children = chain->has_children;
 	}
 
 	if (has_children)
@@ -366,9 +361,9 @@ static int callchain__set_folding(struct rb_root *chain, bool unfold)
 static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
 {
 	hist_entry__init_have_children(he);
-	map_symbol__set_folding(&he->ms, unfold);
+	he->unfolded = unfold ? he->has_children : false;
 
-	if (he->ms.has_children) {
+	if (he->has_children) {
 		int n = callchain__set_folding(&he->sorted_chain, unfold);
 		he->nr_rows = unfold ? n : 0;
 	} else
@@ -1019,7 +1014,7 @@ do_offset:
 	if (offset > 0) {
 		do {
 			h = rb_entry(nd, struct hist_entry, rb_node);
-			if (h->ms.unfolded) {
+			if (h->unfolded) {
 				u16 remaining = h->nr_rows - h->row_offset;
 				if (offset > remaining) {
 					offset -= remaining;
@@ -1040,7 +1035,7 @@ do_offset:
 	} else if (offset < 0) {
 		while (1) {
 			h = rb_entry(nd, struct hist_entry, rb_node);
-			if (h->ms.unfolded) {
+			if (h->unfolded) {
 				if (first) {
 					if (-offset > h->row_offset) {
 						offset += h->row_offset;
@@ -1077,7 +1072,7 @@ do_offset:
 				 * row_offset at its last entry.
 				 */
 				h = rb_entry(nd, struct hist_entry, rb_node);
-				if (h->ms.unfolded)
+				if (h->unfolded)
 					h->row_offset = h->nr_rows;
 				break;
 			}
diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
index 6033a0a212ca..679c2c6d8ade 100644
--- a/tools/perf/util/callchain.h
+++ b/tools/perf/util/callchain.h
@@ -72,6 +72,10 @@ extern struct callchain_param callchain_param;
 struct callchain_list {
 	u64			ip;
 	struct map_symbol	ms;
+	struct /* for TUI */ {
+		bool		unfolded;
+		bool		has_children;
+	};
 	char		       *srcline;
 	struct list_head	list;
 };
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index cc22b9158b93..338770679863 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1163,7 +1163,7 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
 		return;
 
 	/* force fold unfiltered entry for simplicity */
-	h->ms.unfolded = false;
+	h->unfolded = false;
 	h->row_offset = 0;
 	h->nr_rows = 0;
 
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 4d923e6e0069..e97cd476d336 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -109,6 +109,8 @@ struct hist_entry {
 			u16	row_offset;
 			u16	nr_rows;
 			bool	init_have_children;
+			bool	unfolded;
+			bool	has_children;
 		};
 	};
 	char			*srcline;
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 90965296c129..bef47ead1d9b 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -158,8 +158,6 @@ struct ref_reloc_sym {
 struct map_symbol {
 	struct map    *map;
 	struct symbol *sym;
-	bool	      unfolded;
-	bool	      has_children;
 };
 
 struct addr_map_symbol {
-- 
2.3.7

Re: [PATCH v2 10/10] perf tools: Move TUI-specific fields out of map_symbol

[Arnaldo Carvalho de Melo]

Em Tue, May 05, 2015 at 10:18:10AM +0900, Namhyung Kim escreveu:
> The has_children and unfolded fields don't belong to struct map_symbol
> since they're used by TUI only.  Move those fields out of map_symbol
> since the struct is also used by other places.
 
> This will also help to compact the sizeof struct hist_entry.
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> Acked-by: Jiri Olsa <jolsa@redhat.com>

In a case like this, with changes, should we keep the Ack? I guess not, the fix
may introduce another problem, etc.

> fix segfault due to recursion in hist_entry__set_folding().

We will need a v3, this fixes the 'E'xpand segfault, but not the first
problem reported, again, this time step by step:

Samples: 1K of event 'cycles', Event count (approx.): 1597853394
  Children      Self  Command  Shared Object               Symbol
-   99.86%    99.86%  swapper  [kernel.vmlinux]            [k] cpu_startup_entry
   + cpu_startup_entry
+   90.17%     0.00%  swapper  [kernel.vmlinux]            [k] start_secondary
+    9.79%     0.00%  swapper  [kernel.vmlinux].init.text  [k] x86_64_start_kernel
+    9.79%     0.00%  swapper  [kernel.vmlinux].init.text  [k] x86_64_start_reservations
+    9.79%     0.00%  swapper  [kernel.vmlinux].init.text  [k] start_kernel
+    9.79%     0.00%  swapper  [kernel.vmlinux]            [k] rest_init
+    0.08%     0.00%  swapper  [kernel.vmlinux]            [k] thermal_interrupt

---------------------------------------

See the "+ cpu_startup_entry"? If I go there and press enter, I would expect to
see its callers, but what happens is:


Samples: 1K of event 'cycles', Event count (approx.): 1597853394
  Children      Self  Command  Shared Object               Symbol
+   99.86%    99.86%  swapper  [kernel.vmlinux]            [k] cpu_startup_entry
+   90.17%     0.00%  swapper  [kernel.vmlinux]            [k] start_secondary
+    9.79%     0.00%  swapper  [kernel.vmlinux].init.text  [k] x86_64_start_kernel
+    9.79%     0.00%  swapper  [kernel.vmlinux].init.text  [k] x86_64_start_reservations
+    9.79%     0.00%  swapper  [kernel.vmlinux].init.text  [k] start_kernel
+    9.79%     0.00%  swapper  [kernel.vmlinux]            [k] rest_init
+    0.08%     0.00%  swapper  [kernel.vmlinux]            [k] thermal_interrupt

---------------------------------------

It collapses the hist_entry instead of expanding it further.

I'm updating that branch, that now sits on top of what is in acme/perf/core.

- Arnaldo

Re: [PATCH v2 10/10] perf tools: Move TUI-specific fields out of map_symbol

[Arnaldo Carvalho de Melo]

Em Tue, May 05, 2015 at 11:22:31AM -0300, Arnaldo Carvalho de Melo escreveu:
> We will need a v3, this fixes the 'E'xpand segfault, but not the first
> problem reported, again, this time step by step:
 
> Samples: 1K of event 'cycles', Event count (approx.): 1597853394
>   Children      Self  Command  Shared Object               Symbol
> -   99.86%    99.86%  swapper  [kernel.vmlinux]            [k] cpu_startup_entry
>    + cpu_startup_entry
> +   90.17%     0.00%  swapper  [kernel.vmlinux]            [k] start_secondary
> ---------------------------------------
> See the "+ cpu_startup_entry"? If I go there and press enter, I would expect to
> see its callers, but what happens is:
 
> Samples: 1K of event 'cycles', Event count (approx.): 1597853394
>   Children      Self  Command  Shared Object               Symbol
> +   99.86%    99.86%  swapper  [kernel.vmlinux]            [k] cpu_startup_entry
> +   90.17%     0.00%  swapper  [kernel.vmlinux]            [k] start_secondary
> ---------------------------------------
 
> It collapses the hist_entry instead of expanding it further.

I just repeated the bisect, because I was unsure if last time I did it
using the 'E' as the good/bad determiner, the
collapse-when-it-should-expand bug described above is indeed also
introduced by this changeset.

- Arnaldo

Re: [PATCH v2 10/10] perf tools: Move TUI-specific fields out of map_symbol

[Namhyung Kim]

On Tue, May 05, 2015 at 11:26:26AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, May 05, 2015 at 11:22:31AM -0300, Arnaldo Carvalho de Melo escreveu:
> > We will need a v3, this fixes the 'E'xpand segfault, but not the first
> > problem reported, again, this time step by step:
>  
> > Samples: 1K of event 'cycles', Event count (approx.): 1597853394
> >   Children      Self  Command  Shared Object               Symbol
> > -   99.86%    99.86%  swapper  [kernel.vmlinux]            [k] cpu_startup_entry
> >    + cpu_startup_entry
> > +   90.17%     0.00%  swapper  [kernel.vmlinux]            [k] start_secondary
> > ---------------------------------------
> > See the "+ cpu_startup_entry"? If I go there and press enter, I would expect to
> > see its callers, but what happens is:
>  
> > Samples: 1K of event 'cycles', Event count (approx.): 1597853394
> >   Children      Self  Command  Shared Object               Symbol
> > +   99.86%    99.86%  swapper  [kernel.vmlinux]            [k] cpu_startup_entry
> > +   90.17%     0.00%  swapper  [kernel.vmlinux]            [k] start_secondary
> > ---------------------------------------
>  
> > It collapses the hist_entry instead of expanding it further.
> 
> I just repeated the bisect, because I was unsure if last time I did it
> using the 'E' as the good/bad determiner, the
> collapse-when-it-should-expand bug described above is indeed also
> introduced by this changeset.

Okay, this is the fix.

I need to check whether the browser->selection is came from hist entry
or callchain list.  Will send v3 soon.

Thanks,
Namhyung


diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 7a70493e938c..f981cb8f0158 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -221,6 +221,18 @@ static bool hist_entry__toggle_fold(struct hist_entry *he)
 	return true;
 }
 
+static bool callchain_list__toggle_fold(struct callchain_list *cl)
+{
+	if (!cl)
+		return false;
+
+	if (!cl->has_children)
+		return false;
+
+	cl->unfolded = !cl->unfolded;
+	return true;
+}
+
 static void callchain_node__init_have_children_rb_tree(struct callchain_node *node)
 {
 	struct rb_node *nd = rb_first(&node->rb_root);
@@ -282,9 +294,17 @@ static void hist_entry__init_have_children(struct hist_entry *he)
 
 static bool hist_browser__toggle_fold(struct hist_browser *browser)
 {
-	if (hist_entry__toggle_fold(browser->he_selection)) {
-		struct hist_entry *he = browser->he_selection;
+	struct hist_entry *he = browser->he_selection;
+	struct map_symbol *ms = browser->selection;
+	struct callchain_list *cl = container_of(ms, struct callchain_list, ms);
+	bool has_children;
+
+	if (ms == &he->ms)
+		has_children = hist_entry__toggle_fold(he);
+	else
+		has_children = callchain_list__toggle_fold(cl);
 
+	if (has_children) {
 		hist_entry__init_have_children(he);
 		browser->b.nr_entries -= he->nr_rows;
 		browser->nr_callchain_rows -= he->nr_rows;

[PATCH v3 10/10] perf tools: Move TUI-specific fields out of map_symbol

[Namhyung Kim]

The has_children and unfolded fields don't belong to struct map_symbol
since they're used by TUI only.  Move those fields out of map_symbol
since the struct is also used by other places.

This will also help to compact the sizeof struct hist_entry.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1429687101-4360-11-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
fix expanding callchain list entries

 tools/perf/ui/browsers/hists.c | 79 +++++++++++++++++++++++++-----------------
 tools/perf/util/callchain.h    |  4 +++
 tools/perf/util/hist.c         |  2 +-
 tools/perf/util/sort.h         |  2 ++
 tools/perf/util/symbol.h       |  2 --
 5 files changed, 54 insertions(+), 35 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 8733d577db78..f981cb8f0158 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -63,7 +63,7 @@ static int hist_browser__get_folding(struct hist_browser *browser)
 		struct hist_entry *he =
 			rb_entry(nd, struct hist_entry, rb_node);
 
-		if (he->ms.unfolded)
+		if (he->unfolded)
 			unfolded_rows += he->nr_rows;
 	}
 	return unfolded_rows;
@@ -139,24 +139,19 @@ static char tree__folded_sign(bool unfolded)
 	return unfolded ? '-' : '+';
 }
 
-static char map_symbol__folded(const struct map_symbol *ms)
-{
-	return ms->has_children ? tree__folded_sign(ms->unfolded) : ' ';
-}
-
 static char hist_entry__folded(const struct hist_entry *he)
 {
-	return map_symbol__folded(&he->ms);
+	return he->has_children ? tree__folded_sign(he->unfolded) : ' ';
 }
 
 static char callchain_list__folded(const struct callchain_list *cl)
 {
-	return map_symbol__folded(&cl->ms);
+	return cl->has_children ? tree__folded_sign(cl->unfolded) : ' ';
 }
 
-static void map_symbol__set_folding(struct map_symbol *ms, bool unfold)
+static void callchain_list__set_folding(struct callchain_list *cl, bool unfold)
 {
-	ms->unfolded = unfold ? ms->has_children : false;
+	cl->unfolded = unfold ? cl->has_children : false;
 }
 
 static int callchain_node__count_rows_rb_tree(struct callchain_node *node)
@@ -192,7 +187,7 @@ static int callchain_node__count_rows(struct callchain_node *node)
 
 	list_for_each_entry(chain, &node->val, list) {
 		++n;
-		unfolded = chain->ms.unfolded;
+		unfolded = chain->unfolded;
 	}
 
 	if (unfolded)
@@ -214,15 +209,27 @@ static int callchain__count_rows(struct rb_root *chain)
 	return n;
 }
 
-static bool map_symbol__toggle_fold(struct map_symbol *ms)
+static bool hist_entry__toggle_fold(struct hist_entry *he)
 {
-	if (!ms)
+	if (!he)
 		return false;
 
-	if (!ms->has_children)
+	if (!he->has_children)
 		return false;
 
-	ms->unfolded = !ms->unfolded;
+	he->unfolded = !he->unfolded;
+	return true;
+}
+
+static bool callchain_list__toggle_fold(struct callchain_list *cl)
+{
+	if (!cl)
+		return false;
+
+	if (!cl->has_children)
+		return false;
+
+	cl->unfolded = !cl->unfolded;
 	return true;
 }
 
@@ -238,10 +245,10 @@ static void callchain_node__init_have_children_rb_tree(struct callchain_node *no
 		list_for_each_entry(chain, &child->val, list) {
 			if (first) {
 				first = false;
-				chain->ms.has_children = chain->list.next != &child->val ||
+				chain->has_children = chain->list.next != &child->val ||
 							 !RB_EMPTY_ROOT(&child->rb_root);
 			} else
-				chain->ms.has_children = chain->list.next == &child->val &&
+				chain->has_children = chain->list.next == &child->val &&
 							 !RB_EMPTY_ROOT(&child->rb_root);
 		}
 
@@ -255,11 +262,11 @@ static void callchain_node__init_have_children(struct callchain_node *node,
 	struct callchain_list *chain;
 
 	chain = list_entry(node->val.next, struct callchain_list, list);
-	chain->ms.has_children = has_sibling;
+	chain->has_children = has_sibling;
 
 	if (!list_empty(&node->val)) {
 		chain = list_entry(node->val.prev, struct callchain_list, list);
-		chain->ms.has_children = !RB_EMPTY_ROOT(&node->rb_root);
+		chain->has_children = !RB_EMPTY_ROOT(&node->rb_root);
 	}
 
 	callchain_node__init_have_children_rb_tree(node);
@@ -279,7 +286,7 @@ 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) {
-		he->ms.has_children = !RB_EMPTY_ROOT(&he->sorted_chain);
+		he->has_children = !RB_EMPTY_ROOT(&he->sorted_chain);
 		callchain__init_have_children(&he->sorted_chain);
 		he->init_have_children = true;
 	}
@@ -287,14 +294,22 @@ static void hist_entry__init_have_children(struct hist_entry *he)
 
 static bool hist_browser__toggle_fold(struct hist_browser *browser)
 {
-	if (map_symbol__toggle_fold(browser->selection)) {
-		struct hist_entry *he = browser->he_selection;
+	struct hist_entry *he = browser->he_selection;
+	struct map_symbol *ms = browser->selection;
+	struct callchain_list *cl = container_of(ms, struct callchain_list, ms);
+	bool has_children;
+
+	if (ms == &he->ms)
+		has_children = hist_entry__toggle_fold(he);
+	else
+		has_children = callchain_list__toggle_fold(cl);
 
+	if (has_children) {
 		hist_entry__init_have_children(he);
 		browser->b.nr_entries -= he->nr_rows;
 		browser->nr_callchain_rows -= he->nr_rows;
 
-		if (he->ms.unfolded)
+		if (he->unfolded)
 			he->nr_rows = callchain__count_rows(&he->sorted_chain);
 		else
 			he->nr_rows = 0;
@@ -321,8 +336,8 @@ static int callchain_node__set_folding_rb_tree(struct callchain_node *node, bool
 
 		list_for_each_entry(chain, &child->val, list) {
 			++n;
-			map_symbol__set_folding(&chain->ms, unfold);
-			has_children = chain->ms.has_children;
+			callchain_list__set_folding(chain, unfold);
+			has_children = chain->has_children;
 		}
 
 		if (has_children)
@@ -340,8 +355,8 @@ static int callchain_node__set_folding(struct callchain_node *node, bool unfold)
 
 	list_for_each_entry(chain, &node->val, list) {
 		++n;
-		map_symbol__set_folding(&chain->ms, unfold);
-		has_children = chain->ms.has_children;
+		callchain_list__set_folding(chain, unfold);
+		has_children = chain->has_children;
 	}
 
 	if (has_children)
@@ -366,9 +381,9 @@ static int callchain__set_folding(struct rb_root *chain, bool unfold)
 static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
 {
 	hist_entry__init_have_children(he);
-	map_symbol__set_folding(&he->ms, unfold);
+	he->unfolded = unfold ? he->has_children : false;
 
-	if (he->ms.has_children) {
+	if (he->has_children) {
 		int n = callchain__set_folding(&he->sorted_chain, unfold);
 		he->nr_rows = unfold ? n : 0;
 	} else
@@ -1019,7 +1034,7 @@ do_offset:
 	if (offset > 0) {
 		do {
 			h = rb_entry(nd, struct hist_entry, rb_node);
-			if (h->ms.unfolded) {
+			if (h->unfolded) {
 				u16 remaining = h->nr_rows - h->row_offset;
 				if (offset > remaining) {
 					offset -= remaining;
@@ -1040,7 +1055,7 @@ do_offset:
 	} else if (offset < 0) {
 		while (1) {
 			h = rb_entry(nd, struct hist_entry, rb_node);
-			if (h->ms.unfolded) {
+			if (h->unfolded) {
 				if (first) {
 					if (-offset > h->row_offset) {
 						offset += h->row_offset;
@@ -1077,7 +1092,7 @@ do_offset:
 				 * row_offset at its last entry.
 				 */
 				h = rb_entry(nd, struct hist_entry, rb_node);
-				if (h->ms.unfolded)
+				if (h->unfolded)
 					h->row_offset = h->nr_rows;
 				break;
 			}
diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
index 6033a0a212ca..679c2c6d8ade 100644
--- a/tools/perf/util/callchain.h
+++ b/tools/perf/util/callchain.h
@@ -72,6 +72,10 @@ extern struct callchain_param callchain_param;
 struct callchain_list {
 	u64			ip;
 	struct map_symbol	ms;
+	struct /* for TUI */ {
+		bool		unfolded;
+		bool		has_children;
+	};
 	char		       *srcline;
 	struct list_head	list;
 };
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index cc22b9158b93..338770679863 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1163,7 +1163,7 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
 		return;
 
 	/* force fold unfiltered entry for simplicity */
-	h->ms.unfolded = false;
+	h->unfolded = false;
 	h->row_offset = 0;
 	h->nr_rows = 0;
 
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 4d923e6e0069..e97cd476d336 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -109,6 +109,8 @@ struct hist_entry {
 			u16	row_offset;
 			u16	nr_rows;
 			bool	init_have_children;
+			bool	unfolded;
+			bool	has_children;
 		};
 	};
 	char			*srcline;
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 90965296c129..bef47ead1d9b 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -158,8 +158,6 @@ struct ref_reloc_sym {
 struct map_symbol {
 	struct map    *map;
 	struct symbol *sym;
-	bool	      unfolded;
-	bool	      has_children;
 };
 
 struct addr_map_symbol {
-- 
2.3.7

Re: [PATCH v3 10/10] perf tools: Move TUI-specific fields out of map_symbol

[Arnaldo Carvalho de Melo]

Em Tue, May 05, 2015 at 11:55:46PM +0900, Namhyung Kim escreveu:
> The has_children and unfolded fields don't belong to struct map_symbol
> since they're used by TUI only.  Move those fields out of map_symbol
> since the struct is also used by other places.
> 
> This will also help to compact the sizeof struct hist_entry.

So this fixes that problem, but to test this patchkit a bit more before
commiting it, I tried the following:

1) perf record -a -g sleep 2

2) perf report

3) press E to expand all callchains

4) press P to save a perf.hist.0 "screenshot" of all the hist entries
   and its callchains, in their current state, i.e. expanded


Ok I did this with and without your patchset, and there are differences
in the result, that I don't know if are unintended fixes or how to
account them for, could you try reproducing these steps to see if you
notice this difference too?

Ok, now I understand, its a bug, when collecting callchains we need to
process them all, marking the DSOs that had hits _in the callchains_ and
save the build-id for those in the perf.data header, in addition to
saving the DSO in the buildid cache.

What happened was that the different entries I noticed where all in the
callchains, and the perf tool got rebuilt, and all the samples, in my
case, were in kernel functions, that were called by the perf binary....

So for something like:

--- perf.hist.0.WITHOUT 2015-05-05 12:49:06.557802910 -0300
+++ perf.hist.0.WITH_NAMHYUNG_PATCHSET  2015-05-05 12:48:13.871216253 -0300
@@ -167,26 +167,26 @@
 -    0.07%     0.00%  perf            perf                        [.] main
      main
      __libc_start_main
--    0.07%     0.00%  perf            perf                        [.] browser_command_config
-     browser_command_config
+-    0.07%     0.00%  perf            perf                        [.] run_builtin

And I collected the perf.data with your patchset applied, the correct is to
appear "run_builtin", not like here:

-    0.07%     0.00%  perf            perf                        [.] browser_command_config
     browser_command_config
     main

This is all because the perf binary used got lost, replaced by the new one, as it
wasn't saved in the perf.data file header/~/.debug/ cache:

[root@ssdandy linux]# perf buildid-list
cd1d2cf9f473d0cac668e3afee32866da4540bd4 /lib/modules/4.0.0-rc6+/build/vmlinux
2c07e0dc941be12d3db663ea617c08cf10dbbfa1 /lib/modules/4.0.0-rc6+/kernel/drivers/net/wireless/iwlwifi/iwlwifi.ko
[root@ssdandy linux]#

The fix probably will make the buildid collection at the end of a 'perf record'
session more expensive, will try to fix this...

But your patch is not responsible for this, so thanks for the fixes and
I'm applying it.

- Arnaldo

[tip:perf/core] perf tools: Move TUI-specific fields out of map_symbol

[tip-bot for Namhyung Kim]

Commit-ID:  3698dab1c849c7e1cd440df4fca24baa1973d53b
Gitweb:     http://git.kernel.org/tip/3698dab1c849c7e1cd440df4fca24baa1973d53b
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Tue, 5 May 2015 23:55:46 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 5 May 2015 18:13:24 -0300

perf tools: Move TUI-specific fields out of map_symbol

The has_children and unfolded fields don't belong to the struct
map_symbol since they're used by the TUI only.  Move those fields out of
map_symbol since the struct is also used by other places.

This will also help to compact the sizeof struct hist_entry.

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/1429687101-4360-11-git-send-email-namhyung@kernel.org
Link: http://lkml.kernel.org/r/1430837746-5439-1-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 79 +++++++++++++++++++++++++-----------------
 tools/perf/util/callchain.h    |  4 +++
 tools/perf/util/hist.c         |  2 +-
 tools/perf/util/sort.h         |  2 ++
 tools/perf/util/symbol.h       |  2 --
 5 files changed, 54 insertions(+), 35 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 8733d57..f981cb8 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -63,7 +63,7 @@ static int hist_browser__get_folding(struct hist_browser *browser)
 		struct hist_entry *he =
 			rb_entry(nd, struct hist_entry, rb_node);
 
-		if (he->ms.unfolded)
+		if (he->unfolded)
 			unfolded_rows += he->nr_rows;
 	}
 	return unfolded_rows;
@@ -139,24 +139,19 @@ static char tree__folded_sign(bool unfolded)
 	return unfolded ? '-' : '+';
 }
 
-static char map_symbol__folded(const struct map_symbol *ms)
-{
-	return ms->has_children ? tree__folded_sign(ms->unfolded) : ' ';
-}
-
 static char hist_entry__folded(const struct hist_entry *he)
 {
-	return map_symbol__folded(&he->ms);
+	return he->has_children ? tree__folded_sign(he->unfolded) : ' ';
 }
 
 static char callchain_list__folded(const struct callchain_list *cl)
 {
-	return map_symbol__folded(&cl->ms);
+	return cl->has_children ? tree__folded_sign(cl->unfolded) : ' ';
 }
 
-static void map_symbol__set_folding(struct map_symbol *ms, bool unfold)
+static void callchain_list__set_folding(struct callchain_list *cl, bool unfold)
 {
-	ms->unfolded = unfold ? ms->has_children : false;
+	cl->unfolded = unfold ? cl->has_children : false;
 }
 
 static int callchain_node__count_rows_rb_tree(struct callchain_node *node)
@@ -192,7 +187,7 @@ static int callchain_node__count_rows(struct callchain_node *node)
 
 	list_for_each_entry(chain, &node->val, list) {
 		++n;
-		unfolded = chain->ms.unfolded;
+		unfolded = chain->unfolded;
 	}
 
 	if (unfolded)
@@ -214,15 +209,27 @@ static int callchain__count_rows(struct rb_root *chain)
 	return n;
 }
 
-static bool map_symbol__toggle_fold(struct map_symbol *ms)
+static bool hist_entry__toggle_fold(struct hist_entry *he)
 {
-	if (!ms)
+	if (!he)
 		return false;
 
-	if (!ms->has_children)
+	if (!he->has_children)
 		return false;
 
-	ms->unfolded = !ms->unfolded;
+	he->unfolded = !he->unfolded;
+	return true;
+}
+
+static bool callchain_list__toggle_fold(struct callchain_list *cl)
+{
+	if (!cl)
+		return false;
+
+	if (!cl->has_children)
+		return false;
+
+	cl->unfolded = !cl->unfolded;
 	return true;
 }
 
@@ -238,10 +245,10 @@ static void callchain_node__init_have_children_rb_tree(struct callchain_node *no
 		list_for_each_entry(chain, &child->val, list) {
 			if (first) {
 				first = false;
-				chain->ms.has_children = chain->list.next != &child->val ||
+				chain->has_children = chain->list.next != &child->val ||
 							 !RB_EMPTY_ROOT(&child->rb_root);
 			} else
-				chain->ms.has_children = chain->list.next == &child->val &&
+				chain->has_children = chain->list.next == &child->val &&
 							 !RB_EMPTY_ROOT(&child->rb_root);
 		}
 
@@ -255,11 +262,11 @@ static void callchain_node__init_have_children(struct callchain_node *node,
 	struct callchain_list *chain;
 
 	chain = list_entry(node->val.next, struct callchain_list, list);
-	chain->ms.has_children = has_sibling;
+	chain->has_children = has_sibling;
 
 	if (!list_empty(&node->val)) {
 		chain = list_entry(node->val.prev, struct callchain_list, list);
-		chain->ms.has_children = !RB_EMPTY_ROOT(&node->rb_root);
+		chain->has_children = !RB_EMPTY_ROOT(&node->rb_root);
 	}
 
 	callchain_node__init_have_children_rb_tree(node);
@@ -279,7 +286,7 @@ 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) {
-		he->ms.has_children = !RB_EMPTY_ROOT(&he->sorted_chain);
+		he->has_children = !RB_EMPTY_ROOT(&he->sorted_chain);
 		callchain__init_have_children(&he->sorted_chain);
 		he->init_have_children = true;
 	}
@@ -287,14 +294,22 @@ static void hist_entry__init_have_children(struct hist_entry *he)
 
 static bool hist_browser__toggle_fold(struct hist_browser *browser)
 {
-	if (map_symbol__toggle_fold(browser->selection)) {
-		struct hist_entry *he = browser->he_selection;
+	struct hist_entry *he = browser->he_selection;
+	struct map_symbol *ms = browser->selection;
+	struct callchain_list *cl = container_of(ms, struct callchain_list, ms);
+	bool has_children;
+
+	if (ms == &he->ms)
+		has_children = hist_entry__toggle_fold(he);
+	else
+		has_children = callchain_list__toggle_fold(cl);
 
+	if (has_children) {
 		hist_entry__init_have_children(he);
 		browser->b.nr_entries -= he->nr_rows;
 		browser->nr_callchain_rows -= he->nr_rows;
 
-		if (he->ms.unfolded)
+		if (he->unfolded)
 			he->nr_rows = callchain__count_rows(&he->sorted_chain);
 		else
 			he->nr_rows = 0;
@@ -321,8 +336,8 @@ static int callchain_node__set_folding_rb_tree(struct callchain_node *node, bool
 
 		list_for_each_entry(chain, &child->val, list) {
 			++n;
-			map_symbol__set_folding(&chain->ms, unfold);
-			has_children = chain->ms.has_children;
+			callchain_list__set_folding(chain, unfold);
+			has_children = chain->has_children;
 		}
 
 		if (has_children)
@@ -340,8 +355,8 @@ static int callchain_node__set_folding(struct callchain_node *node, bool unfold)
 
 	list_for_each_entry(chain, &node->val, list) {
 		++n;
-		map_symbol__set_folding(&chain->ms, unfold);
-		has_children = chain->ms.has_children;
+		callchain_list__set_folding(chain, unfold);
+		has_children = chain->has_children;
 	}
 
 	if (has_children)
@@ -366,9 +381,9 @@ static int callchain__set_folding(struct rb_root *chain, bool unfold)
 static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
 {
 	hist_entry__init_have_children(he);
-	map_symbol__set_folding(&he->ms, unfold);
+	he->unfolded = unfold ? he->has_children : false;
 
-	if (he->ms.has_children) {
+	if (he->has_children) {
 		int n = callchain__set_folding(&he->sorted_chain, unfold);
 		he->nr_rows = unfold ? n : 0;
 	} else
@@ -1019,7 +1034,7 @@ do_offset:
 	if (offset > 0) {
 		do {
 			h = rb_entry(nd, struct hist_entry, rb_node);
-			if (h->ms.unfolded) {
+			if (h->unfolded) {
 				u16 remaining = h->nr_rows - h->row_offset;
 				if (offset > remaining) {
 					offset -= remaining;
@@ -1040,7 +1055,7 @@ do_offset:
 	} else if (offset < 0) {
 		while (1) {
 			h = rb_entry(nd, struct hist_entry, rb_node);
-			if (h->ms.unfolded) {
+			if (h->unfolded) {
 				if (first) {
 					if (-offset > h->row_offset) {
 						offset += h->row_offset;
@@ -1077,7 +1092,7 @@ do_offset:
 				 * row_offset at its last entry.
 				 */
 				h = rb_entry(nd, struct hist_entry, rb_node);
-				if (h->ms.unfolded)
+				if (h->unfolded)
 					h->row_offset = h->nr_rows;
 				break;
 			}
diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
index 6033a0a..679c2c6 100644
--- a/tools/perf/util/callchain.h
+++ b/tools/perf/util/callchain.h
@@ -72,6 +72,10 @@ extern struct callchain_param callchain_param;
 struct callchain_list {
 	u64			ip;
 	struct map_symbol	ms;
+	struct /* for TUI */ {
+		bool		unfolded;
+		bool		has_children;
+	};
 	char		       *srcline;
 	struct list_head	list;
 };
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index cc22b91..3387706 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1163,7 +1163,7 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
 		return;
 
 	/* force fold unfiltered entry for simplicity */
-	h->ms.unfolded = false;
+	h->unfolded = false;
 	h->row_offset = 0;
 	h->nr_rows = 0;
 
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 4d923e6..e97cd47 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -109,6 +109,8 @@ struct hist_entry {
 			u16	row_offset;
 			u16	nr_rows;
 			bool	init_have_children;
+			bool	unfolded;
+			bool	has_children;
 		};
 	};
 	char			*srcline;
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 9096529..bef47ead 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -158,8 +158,6 @@ struct ref_reloc_sym {
 struct map_symbol {
 	struct map    *map;
 	struct symbol *sym;
-	bool	      unfolded;
-	bool	      has_children;
 };
 
 struct addr_map_symbol {

Re: [PATCHSET 00/10] perf tools: Assorted cleanup for TUI (v3)

[Jiri Olsa]

On Wed, Apr 22, 2015 at 04:18:11PM +0900, Namhyung Kim wrote:
> 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.
> 
>  * changed in v3)
>    - save necessary info in hist_browser  (Arnaldo)
>    - rename to struct popup_action  (Arnaldo)
>    - split popup registration and callback  (Arnaldo)
>    - move TUI-specific fields out of map_symbol

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

Re: [PATCHSET 00/10] perf tools: Assorted cleanup for TUI (v3)

[Namhyung Kim]

On Wed, Apr 22, 2015 at 01:12:14PM +0200, Jiri Olsa wrote:
> On Wed, Apr 22, 2015 at 04:18:11PM +0900, Namhyung Kim wrote:
> > 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.
> > 
> >  * changed in v3)
> >    - save necessary info in hist_browser  (Arnaldo)
> >    - rename to struct popup_action  (Arnaldo)
> >    - split popup registration and callback  (Arnaldo)
> >    - move TUI-specific fields out of map_symbol
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>

Thank you very much!
Namhyung