perf tools: Assorted cleanup for TUI (v3)

PATCH In review Namhyung Kim 4 participants 49 messages Apr 22, 2015 – May 6, 2015 perf events

lore

Share & tools

lore

Namhyung Kim

author maintainer

2015-04-22 07:19 UTC #0 lore raw

cover perf tools: Assorted cleanup for TUI (v3)

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(-)

Signature · 1 line

2.3.5

Namhyung Kim

author maintainer

2015-04-22 07:19 UTC #1 lore raw

PATCH 1/10 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 <[email protected]>

---
 tools/perf/util/sort.h | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

tools/perf/util/sort.h +12 -6

index de3303fe726d..7f0c0a8d615d 100644
@@ -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;

Signature · 1 line

2.3.5

Arnaldo Carvalho de Melo

maintainer

2015-04-22 11:57 UTC #2 lore raw

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

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

Quoted 10 lines

+++ b/tools/perf/util/sort.h

tools/perf/util/sort.h +2 -5

@@ -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 {
+		/*

tools/perf/util/sort.h struct hist_entry { · line 105

Quoted 10 lines

tools/perf/util/sort.h +10 -0

+		 * 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-bot for Namhyung Kim

merge bot

2015-05-06 03:20 UTC #3 lore raw

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

In reply to Namhyung Kim

perf tools: Move TUI-specific fields into unnamed union

Commit-ID:  297508216556fbf4e3f70fb97d03280741b4a709
Gitweb:     http://git.kernel.org/tip/297508216556fbf4e3f70fb97d03280741b4a709
Author:     Namhyung Kim <[email protected]>
AuthorDate: Wed, 22 Apr 2015 16:18:12 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>

Acked-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/util/sort.h | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

tools/perf/util/sort.h +12 -6

index de3303f..7f0c0a8 100644
@@ -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;

Namhyung Kim

author maintainer

2015-04-22 07:19 UTC #4 lore raw

PATCH 2/10 perf tools: Move init_have_children field to the unnamed union

In reply to 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 <[email protected]>

---
 tools/perf/util/sort.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/perf/util/sort.h +1 -1

index 7f0c0a8d615d..4d923e6e0069 100644
@@ -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;

Signature · 1 line

2.3.5

tip-bot for Namhyung Kim

merge bot

2015-05-06 03:20 UTC #5 lore raw

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

perf tools: Move init_have_children field to the unnamed union

Commit-ID:  d8a0f80042efc4ba08977f3d66fc4678037fe456
Gitweb:     http://git.kernel.org/tip/d8a0f80042efc4ba08977f3d66fc4678037fe456
Author:     Namhyung Kim <[email protected]>
AuthorDate: Wed, 22 Apr 2015 16:18:13 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>

Acked-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/util/sort.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/perf/util/sort.h +1 -1

index 7f0c0a8..4d923e6 100644
@@ -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;

Namhyung Kim

author maintainer

2015-04-22 07:19 UTC #6 lore raw

PATCH 3/10 perf hists browser: Fix possible memory leak

In reply to 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 <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

tools/perf/ui/browsers/hists.c +5 -3

index 995b7a8596b1..0972d4722297 100644
@@ -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;
 }

Signature · 1 line

2.3.5

tip-bot for Namhyung Kim

merge bot

2015-05-06 03:20 UTC #7 lore raw

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

perf hists browser: Fix possible memory leak

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

perf hists browser: Fix possible memory leak

Signed-off-by Namhyung Kim <[email protected]>

Acked-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

tools/perf/ui/browsers/hists.c +5 -3

index 995b7a8..0972d47 100644
@@ -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;
 }

Namhyung Kim

author maintainer

2015-04-22 07:21 UTC #8 lore raw

PATCH 4/10 perf hists browser: Save hist_browser_timer pointer in hist_browser

In reply to 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.

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

tools/perf/ui/browsers/hists.c +8 -5

index 0972d4722297..0847623f42e0 100644
@@ -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);

Signature · 1 line

2.3.5

tip-bot for Namhyung Kim

merge bot

2015-05-06 03:21 UTC #9 lore raw

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

perf hists browser: Save hist_browser_timer pointer in hist_browser

Commit-ID:  c2a51ab802d17c572cd0a940fd97538b75aa7889
Gitweb:     http://git.kernel.org/tip/c2a51ab802d17c572cd0a940fd97538b75aa7889
Author:     Namhyung Kim <[email protected]>
AuthorDate: Wed, 22 Apr 2015 16:18:15 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
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.

Signed-off-by Namhyung Kim <[email protected]>

Acked-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

tools/perf/ui/browsers/hists.c +8 -5

index 0972d47..0847623 100644
@@ -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);

Namhyung Kim

author maintainer

2015-04-22 07:21 UTC #10 lore raw

PATCH 5/10 perf hists browser: Save pstack in the hist_browser

In reply to 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.

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

tools/perf/ui/browsers/hists.c +10 ~~-10~~

index 0847623f42e0..26d5548b796e 100644
@@ -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);

Signature · 1 line

2.3.5

tip-bot for Namhyung Kim

merge bot

2015-05-06 03:21 UTC #11 lore raw

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

perf hists browser: Save pstack in the hist_browser

Commit-ID:  01f00a1cd158ecd86d5f561ded271597d0550313
Gitweb:     http://git.kernel.org/tip/01f00a1cd158ecd86d5f561ded271597d0550313
Author:     Namhyung Kim <[email protected]>
AuthorDate: Wed, 22 Apr 2015 16:18:16 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
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.

Signed-off-by Namhyung Kim <[email protected]>

Acked-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

tools/perf/ui/browsers/hists.c +10 ~~-10~~

index 0847623..26d5548 100644
@@ -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);

Namhyung Kim

author maintainer

2015-04-22 07:19 UTC #12 lore raw

PATCH 6/10 perf hists browser: Save perf_session_env in the hist_browser

In reply to 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 <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

tools/perf/ui/browsers/hists.c +5 -2

index 26d5548b796e..45704d6383e4 100644
@@ -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];

Signature · 1 line

2.3.5

tip-bot for Namhyung Kim

merge bot

2015-05-06 03:21 UTC #13 lore raw

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

perf hists browser: Save perf_session_env in the hist_browser

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

perf hists browser: Save perf_session_env in the hist_browser

Signed-off-by Namhyung Kim <[email protected]>

Acked-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

tools/perf/ui/browsers/hists.c +5 -2

index 26d5548..45704d6 100644
@@ -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];

Namhyung Kim

author maintainer

2015-04-22 07:20 UTC #14 lore raw

PATCH 7/10 perf hists browser: Split popup menu actions

In reply to 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 <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 242 ++++++++++++++++++++++++++---------------
 1 file changed, 156 insertions(+), 86 deletions(-)

tools/perf/ui/browsers/hists.c +156 ~~-86~~

index 45704d6383e4..7d88a1cdf04b 100644
@@ -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:

Signature · 1 line

2.3.5

tip-bot for Namhyung Kim

merge bot

2015-05-06 03:22 UTC #15 lore raw

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

perf hists browser: Split popup menu actions

Commit-ID:  bc7cad429bcdda6f112525c17db9577a1be4c8aa
Gitweb:     http://git.kernel.org/tip/bc7cad429bcdda6f112525c17db9577a1be4c8aa
Author:     Namhyung Kim <[email protected]>
AuthorDate: Wed, 22 Apr 2015 16:18:18 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>

Acked-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 242 ++++++++++++++++++++++++++---------------
 1 file changed, 156 insertions(+), 86 deletions(-)

tools/perf/ui/browsers/hists.c +156 ~~-86~~

index 45704d6..7d88a1c 100644
@@ -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:

Namhyung Kim

author maintainer

2015-04-22 07:19 UTC #16 lore raw

PATCH 8/10 perf hists browser: Split popup menu actions - part 2

In reply to 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 <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 354 +++++++++++++++++++++++++----------------
 1 file changed, 214 insertions(+), 140 deletions(-)

tools/perf/ui/browsers/hists.c +214 ~~-140~~

index 7d88a1cdf04b..9bd7b38de64c 100644
@@ -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);

Signature · 1 line

2.3.5

Jiri Olsa

2015-04-22 10:54 UTC #17 lore raw

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

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

SNIP

Quoted 5 lines

tools/perf/ui/browsers/hists.c +0 -0

 	script_browse(script_opt);

@@ -1505,17 +1585,74 @@ do_run_script(struct hist_browser *browser __maybe_unused,
 }
 
 static int

tools/perf/ui/browsers/hists.c do_run_script(struct hist_browser *browser __maybe_unused, · line 1596

Quoted 10 lines

tools/perf/ui/browsers/hists.c +9 -1

-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

Namhyung Kim

author maintainer

2015-04-22 13:04 UTC #18 lore raw

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

Hi Jiri,

Quoted 8 lines · Jiri Olsa

On Wed, Apr 22, 2015 at 04:18:19PM +0900, Namhyung Kim wrote:
SNIP
Quoted 4 lines
tools/perf/ui/browsers/hists.c +0 -0
 	script_browse(script_opt);
@@ -1505,17 +1585,74 @@ do_run_script(struct hist_browser *browser __maybe_unused,
 }
 

Quoted 13 lines

tools/perf/ui/browsers/hists.c do_run_script(struct hist_browser *browser __maybe_unused, · line 1596

Quoted 11 lines

tools/perf/ui/browsers/hists.c +9 -1

 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?

Quoted 3 lines

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

Arnaldo Carvalho de Melo

maintainer

2015-04-22 13:25 UTC #19 lore raw

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

Em Wed, Apr 22, 2015 at 10:03:25PM +0900, Namhyung Kim escreveu:

Quoted 11 lines

Hi Jiri,
Quoted 7 lines · Jiri Olsa
tools/perf/ui/browsers/hists.c do_run_script(struct hist_browser *browser __maybe_unused, · line 1596
Quoted 4 lines · Namhyung Kim
tools/perf/ui/browsers/hists.c +4 -0
+		if (asprintf(optstr, "Run scripts for samples of thread [%s]",
+			     thread__comm_str(thread)) < 0)
+			return 0;
+	} else if (sym) {
         ..............  && sym->namelen ;-)

Quoted 3 lines

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 <[email protected]>

---

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

Quoted 8 lines

Quoted 3 lines
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

Namhyung Kim

author maintainer

2015-04-22 13:32 UTC #20 lore raw

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

Quoted 14 lines · Arnaldo Carvalho de Melo

Em Wed, Apr 22, 2015 at 10:03:25PM +0900, Namhyung Kim escreveu:
Quoted 12 lines
Hi Jiri,
Quoted 7 lines · Jiri Olsa
tools/perf/ui/browsers/hists.c do_run_script(struct hist_browser *browser __maybe_unused, · line 1596
Quoted 4 lines · Namhyung Kim
tools/perf/ui/browsers/hists.c +4 -0
+         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.

Quoted 7 lines

Quoted 1 line
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

Quoted 46 lines

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 <[email protected]>
---
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
Quoted 8 lines
Quoted 3 lines
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

Jiri Olsa

2015-04-22 13:41 UTC #21 lore raw

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

Quoted 18 lines · Namhyung Kim

Quoted 16 lines · Arnaldo Carvalho de Melo
Em Wed, Apr 22, 2015 at 10:03:25PM +0900, Namhyung Kim escreveu:
Quoted 14 lines
Hi Jiri,
Quoted 7 lines · Jiri Olsa
tools/perf/ui/browsers/hists.c do_run_script(struct hist_browser *browser __maybe_unused, · line 1596
Quoted 4 lines · Namhyung Kim
tools/perf/ui/browsers/hists.c +4 -0
+         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?

Quoted 7 lines

Quoted 5 lines
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

Namhyung Kim

author maintainer

2015-04-22 13:54 UTC #22 lore raw

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

Quoted 25 lines · Jiri Olsa

Quoted 24 lines · Namhyung Kim
Quoted 21 lines · Arnaldo Carvalho de Melo
Em Wed, Apr 22, 2015 at 10:03:25PM +0900, Namhyung Kim escreveu:
Quoted 9 lines
Hi Jiri,
Quoted 5 lines · Jiri Olsa
Quoted 4 lines · Namhyung Kim
tools/perf/ui/browsers/hists.c +4 -0
+         if (asprintf(optstr, "Run scripts for samples of thread [%s]",
+                      thread__comm_str(thread)) < 0)
+                 return 0;
+ } else if (sym) {
Quoted 5 lines
Quoted 1 line
         ..............  && 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.

Quoted 6 lines

Quoted 1 line
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

Jiri Olsa

2015-04-22 14:03 UTC #23 lore raw

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

Quoted 30 lines · Namhyung Kim

Quoted 29 lines · Jiri Olsa
Quoted 18 lines · Namhyung Kim
Quoted 16 lines · Arnaldo Carvalho de Melo
Em Wed, Apr 22, 2015 at 10:03:25PM +0900, Namhyung Kim escreveu:
Quoted 14 lines
Hi Jiri,
Quoted 7 lines · Jiri Olsa
Review
Quoted 4 lines · Namhyung Kim
(quoted diff) +4 -0
+         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?
Quoted 7 lines
Quoted 5 lines
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..

Quoted 7 lines

Quoted 2 lines
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 ;-)

Quoted 2 lines

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

yep

jirka

tip-bot for Namhyung Kim

merge bot

2015-05-06 03:22 UTC #24 lore raw

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

In reply to Namhyung Kim

perf hists browser: Split popup menu actions - part 2

Commit-ID:  ea7cd59233097984850adc0e4119644f089be734
Gitweb:     http://git.kernel.org/tip/ea7cd59233097984850adc0e4119644f089be734
Author:     Namhyung Kim <[email protected]>
AuthorDate: Wed, 22 Apr 2015 16:18:19 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
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.

No functional change intended.

Signed-off-by Namhyung Kim <[email protected]>

Acked-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 354 +++++++++++++++++++++++++----------------
 1 file changed, 214 insertions(+), 140 deletions(-)

tools/perf/ui/browsers/hists.c +214 ~~-140~~

index 7d88a1c..9bd7b38 100644
@@ -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);

Namhyung Kim

author maintainer

2015-04-22 07:19 UTC #25 lore raw

PATCH 9/10 perf hists browser: Simplify zooming code a bit

In reply to Namhyung Kim

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

Signed-off-by Namhyung Kim <[email protected]>

---
 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(-)

tools/perf/ui/browsers/hists.c +5 ~~-13~~

index 9bd7b38de64c..a0da3f9f0513 100644
@@ -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:

tools/perf/util/pstack.c +7 -0

index a126e6cc6e73..b234a6e3d0d4 100644
@@ -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];
+}

tools/perf/util/pstack.h +1 -0

index c3cb6584d527..ded7f2e36624 100644
@@ -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_ */

Signature · 1 line

2.3.5

Arnaldo Carvalho de Melo

maintainer

2015-04-23 22:31 UTC #26 lore raw

Re: [PATCH 09/10] perf hists browser: Simplify zooming code a bit

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

Quoted 1 line

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

Quoted 64 lines

Signed-off-by Namhyung Kim <[email protected]>

---
 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(-)

tools/perf/ui/browsers/hists.c +5 ~~-13~~

index 9bd7b38de64c..a0da3f9f0513 100644
@@ -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:

tools/perf/util/pstack.c +7 -0

index a126e6cc6e73..b234a6e3d0d4 100644
@@ -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];
+}

tools/perf/util/pstack.h +1 -0

index c3cb6584d527..ded7f2e36624 100644
@@ -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_ */

Signature · 1 line

2.3.5

Namhyung Kim

author maintainer

2015-04-24 01:20 UTC #27 lore raw

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

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

Signed-off-by Namhyung Kim <[email protected]>

---
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(+)

tools/perf/util/pstack.c +7 -0

index a126e6cc6e73..b234a6e3d0d4 100644
@@ -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];
+}

tools/perf/util/pstack.h +1 -0

index c3cb6584d527..ded7f2e36624 100644
@@ -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_ */

Signature · 1 line

2.3.4

Namhyung Kim

author maintainer

2015-04-24 01:20 UTC #28 lore raw

PATCH v2 1/1 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 <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

tools/perf/ui/browsers/hists.c +9 ~~-11~~

index 9bd7b38de64c..8733d577db78 100644
@@ -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:

Signature · 1 line

2.3.4

tip-bot for Namhyung Kim

merge bot

2015-05-06 03:22 UTC #29 lore raw

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

perf hists browser: Simplify zooming code using pstack_peek()

Commit-ID:  6422184b087ff4355951d72e0bb320f52e107185
Gitweb:     http://git.kernel.org/tip/6422184b087ff4355951d72e0bb320f52e107185
Author:     Namhyung Kim <[email protected]>
AuthorDate: Fri, 24 Apr 2015 10:15:33 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>

Acked-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

tools/perf/ui/browsers/hists.c +9 ~~-11~~

index 9bd7b38..8733d57 100644
@@ -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-bot for Namhyung Kim

merge bot

2015-05-06 03:22 UTC #30 lore raw

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

In reply to Namhyung Kim

perf tools: Introduce pstack_peek()

Commit-ID:  c8539e3fc630067020814657636b45095edfb5bb
Gitweb:     http://git.kernel.org/tip/c8539e3fc630067020814657636b45095edfb5bb
Author:     Namhyung Kim <[email protected]>
AuthorDate: Fri, 24 Apr 2015 10:15:32 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>

Acked-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

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

tools/perf/util/pstack.c +7 -0

index a126e6c..b234a6e 100644
@@ -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];
+}

tools/perf/util/pstack.h +1 -0

index c3cb658..ded7f2e 100644
@@ -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_ */

Namhyung Kim

author maintainer

2015-04-22 07:20 UTC #31 lore raw

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

In reply to 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 <[email protected]>

---
 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(-)

tools/perf/ui/browsers/hists.c +26 ~~-31~~

index a0da3f9f0513..3b82e3b8edb0 100644
@@ -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;
 			}

tools/perf/util/callchain.h +4 -0

index 6033a0a212ca..679c2c6d8ade 100644
@@ -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;
 };

tools/perf/util/hist.c +1 -1

index cc22b9158b93..338770679863 100644
@@ -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;

tools/perf/util/sort.h +2 -0

index 4d923e6e0069..e97cd476d336 100644
@@ -109,6 +109,8 @@ struct hist_entry {
 			u16	row_offset;
 			u16	nr_rows;
 			bool	init_have_children;
+			bool	unfolded;
+			bool	has_children;
 		};
 	};
 	char			*srcline;

tools/perf/util/symbol.h +0 -2

index 09561500164a..8483a864a915 100644
@@ -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 {

Signature · 1 line

2.3.5

Arnaldo Carvalho de Melo

maintainer

2015-04-27 17:20 UTC #32 lore raw

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

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

tools/perf/ui/browsers/hists.c static char tree__folded_sign(bool unfolded) · line 146

Quoted 9 lines

tools/perf/ui/browsers/hists.c +0 -5

@@ -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

Quoted 219 lines

tools/perf/ui/browsers/hists.c +25 ~~-25~~

 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;
 			}

tools/perf/util/callchain.h +4 -0

index 6033a0a212ca..679c2c6d8ade 100644
@@ -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;
 };

tools/perf/util/hist.c +1 -1

index cc22b9158b93..338770679863 100644
@@ -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;

tools/perf/util/sort.h +2 -0

index 4d923e6e0069..e97cd476d336 100644
@@ -109,6 +109,8 @@ struct hist_entry {
 			u16	row_offset;
 			u16	nr_rows;
 			bool	init_have_children;
+			bool	unfolded;
+			bool	has_children;
 		};
 	};
 	char			*srcline;

tools/perf/util/symbol.h +0 -2

index 09561500164a..8483a864a915 100644
@@ -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 {

Signature · 1 line

2.3.5

Namhyung Kim

author maintainer

2015-04-28 07:39 UTC #33 lore raw

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

Hi Arnaldo,

Quoted 6 lines · Arnaldo Carvalho de Melo

Em Wed, Apr 22, 2015 at 04:18:21PM +0900, Namhyung Kim escreveu:
Quoted 4 lines
tools/perf/ui/browsers/hists.c +0 -0
@@ -139,24 +139,19 @@ static char tree__folded_sign(bool unfolded)
 	return unfolded ? '-' : '+';
 }
 

Quoted 9 lines

tools/perf/ui/browsers/hists.c static char tree__folded_sign(bool unfolded) · line 146
Quoted 5 lines
tools/perf/ui/browsers/hists.c +0 -5
-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.

Quoted 7 lines

tools/perf/ui/browsers/hists.c +1 -1

 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) : ' ';
 }

Quoted 5 lines

tools/perf/ui/browsers/hists.c static char tree__folded_sign(bool unfolded) · line 150
Quoted 5 lines
tools/perf/ui/browsers/hists.c +1 -1
 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

Quoted 213 lines

Quoted 208 lines

tools/perf/ui/browsers/hists.c +23 ~~-23~~

-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;
 			}

tools/perf/util/callchain.h +4 -0

index 6033a0a212ca..679c2c6d8ade 100644
@@ -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;
 };

tools/perf/util/hist.c +1 -1

index cc22b9158b93..338770679863 100644
@@ -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;

tools/perf/util/sort.h +2 -0

index 4d923e6e0069..e97cd476d336 100644
@@ -109,6 +109,8 @@ struct hist_entry {
 			u16	row_offset;
 			u16	nr_rows;
 			bool	init_have_children;
+			bool	unfolded;
+			bool	has_children;
 		};
 	};
 	char			*srcline;

tools/perf/util/symbol.h +0 -2

index 09561500164a..8483a864a915 100644
@@ -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 {

Signature · 1 line

2.3.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Arnaldo Carvalho de Melo

maintainer

2015-04-28 12:29 UTC #34 lore raw

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

Em Tue, Apr 28, 2015 at 04:30:29PM +0900, Namhyung Kim escreveu:

Quoted 19 lines

Hi Arnaldo,
Quoted 6 lines · Arnaldo Carvalho de Melo
Em Wed, Apr 22, 2015 at 04:18:21PM +0900, Namhyung Kim escreveu:
Quoted 4 lines
tools/perf/ui/browsers/hists.c +0 -0
@@ -139,24 +139,19 @@ static char tree__folded_sign(bool unfolded)
 	return unfolded ? '-' : '+';
 }
 
Quoted 9 lines
tools/perf/ui/browsers/hists.c static char tree__folded_sign(bool unfolded) · line 146
Quoted 5 lines
tools/perf/ui/browsers/hists.c +0 -5
-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,

Quoted 6 lines

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

Quoted 231 lines

Quoted 11 lines

tools/perf/ui/browsers/hists.c static char tree__folded_sign(bool unfolded) · line 150

Quoted 11 lines

tools/perf/ui/browsers/hists.c +2 -2

 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

Quoted 213 lines

Quoted 208 lines

tools/perf/ui/browsers/hists.c +23 ~~-23~~

-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;
 			}

tools/perf/util/callchain.h +4 -0

index 6033a0a212ca..679c2c6d8ade 100644
@@ -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;
 };

tools/perf/util/hist.c +1 -1

index cc22b9158b93..338770679863 100644
@@ -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;

tools/perf/util/sort.h +2 -0

index 4d923e6e0069..e97cd476d336 100644
@@ -109,6 +109,8 @@ struct hist_entry {
 			u16	row_offset;
 			u16	nr_rows;
 			bool	init_have_children;
+			bool	unfolded;
+			bool	has_children;
 		};
 	};
 	char			*srcline;

tools/perf/util/symbol.h +0 -2

index 09561500164a..8483a864a915 100644
@@ -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 {

Signature · 1 line

2.3.5

Arnaldo Carvalho de Melo

maintainer Acked-by Jiri Olsa on v1 10/10

2015-05-04 20:55 UTC #35 lore raw

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

In reply to Namhyung Kim

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

Quoted 5 lines

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 <[email protected]>
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 <[email protected]>

Acked-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

: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

Arnaldo Carvalho de Melo

maintainer

2015-05-04 20:56 UTC #36 lore raw

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

Em Mon, May 04, 2015 at 12:42:27PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 8 lines

Em Wed, Apr 22, 2015 at 04:18:21PM +0900, Namhyung Kim escreveu:
Quoted 5 lines
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

Quoted 8 lines

Author: Namhyung Kim <[email protected]>
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

Quoted 3 lines

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

Namhyung Kim

author maintainer

2015-05-05 01:14 UTC #37 lore raw

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

Hi Arnaldo,

Quoted 18 lines · Arnaldo Carvalho de Melo

Em Mon, May 04, 2015 at 12:42:27PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 8 lines
Em Wed, Apr 22, 2015 at 04:18:21PM +0900, Namhyung Kim escreveu:
Quoted 5 lines
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
Quoted 8 lines
Author: Namhyung Kim <[email protected]>
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?

Quoted 8 lines

Moved to perf/core-hists_browser
Quoted 3 lines
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

tools/perf/ui/browsers/hists.c +1 -1

index f422e5e82a6a..7a70493e938c 100644
@@ -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);

Arnaldo Carvalho de Melo

maintainer

2015-05-05 14:07 UTC #38 lore raw

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

Em Tue, May 05, 2015 at 10:12:45AM +0900, Namhyung Kim escreveu:

Quoted 27 lines

Hi Arnaldo,
Quoted 24 lines · Arnaldo Carvalho de Melo
Em Mon, May 04, 2015 at 12:42:27PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 8 lines
Em Wed, Apr 22, 2015 at 04:18:21PM +0900, Namhyung Kim escreveu:
Quoted 5 lines
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
Quoted 8 lines
Author: Namhyung Kim <[email protected]>
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
Quoted 3 lines
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.

Quoted 5 lines

Quoted 2 lines
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

Quoted 17 lines

Thanks,
Namhyung

tools/perf/ui/browsers/hists.c +1 -1

index f422e5e82a6a..7a70493e938c 100644
@@ -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);

Namhyung Kim

author maintainer

2015-05-05 16:06 UTC #39 lore raw

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

Hi Arnaldo,

Quoted 28 lines · Arnaldo Carvalho de Melo

Em Tue, May 05, 2015 at 10:12:45AM +0900, Namhyung Kim escreveu:
Quoted 27 lines
Hi Arnaldo,
Quoted 24 lines · Arnaldo Carvalho de Melo
Em Mon, May 04, 2015 at 12:42:27PM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 8 lines
Em Wed, Apr 22, 2015 at 04:18:21PM +0900, Namhyung Kim escreveu:
Quoted 5 lines
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
Quoted 8 lines
Author: Namhyung Kim <[email protected]>
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
Quoted 3 lines
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.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
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

Namhyung Kim

author maintainer

2015-05-05 01:19 UTC #40 lore raw

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

In reply to Arnaldo Carvalho de Melo

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 <[email protected]>

Acked-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
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(-)

tools/perf/ui/browsers/hists.c +26 ~~-31~~

index 8733d577db78..7a70493e938c 100644
@@ -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;
 			}

tools/perf/util/callchain.h +4 -0

index 6033a0a212ca..679c2c6d8ade 100644
@@ -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;
 };

tools/perf/util/hist.c +1 -1

index cc22b9158b93..338770679863 100644
@@ -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;

tools/perf/util/sort.h +2 -0

index 4d923e6e0069..e97cd476d336 100644
@@ -109,6 +109,8 @@ struct hist_entry {
 			u16	row_offset;
 			u16	nr_rows;
 			bool	init_have_children;
+			bool	unfolded;
+			bool	has_children;
 		};
 	};
 	char			*srcline;

tools/perf/util/symbol.h +0 -2

index 90965296c129..bef47ead1d9b 100644
@@ -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 {

Signature · 1 line

2.3.7

Arnaldo Carvalho de Melo

maintainer

2015-05-05 16:06 UTC #41 lore raw

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

Em Tue, May 05, 2015 at 10:18:10AM +0900, Namhyung Kim escreveu:

Quoted 8 lines

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 <[email protected]>
Acked-by Jiri Olsa <[email protected]>

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

Quoted 1 line

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

Arnaldo Carvalho de Melo

maintainer

2015-05-05 16:06 UTC #42 lore raw

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

Em Tue, May 05, 2015 at 11:22:31AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 18 lines

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
(quoted diff) +1 -2
-   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
(quoted diff) +2 -1
+   99.86%    99.86%  swapper  [kernel.vmlinux]            [k] cpu_startup_entry
+   90.17%     0.00%  swapper  [kernel.vmlinux]            [k] start_secondary
---------------------------------------

Quoted 1 line

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

Namhyung Kim

author maintainer

2015-05-05 16:07 UTC #43 lore raw

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

Quoted 19 lines · Arnaldo Carvalho de Melo

Em Tue, May 05, 2015 at 11:22:31AM -0300, Arnaldo Carvalho de Melo escreveu:
Quoted 17 lines
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
(quoted diff) +1 -2
-   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
(quoted diff) +2 -1
+   99.86%    99.86%  swapper  [kernel.vmlinux]            [k] cpu_startup_entry
+   90.17%     0.00%  swapper  [kernel.vmlinux]            [k] start_secondary
---------------------------------------

Quoted 6 lines

Quoted 1 line
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

tools/perf/ui/browsers/hists.c +22 -2

index 7a70493e938c..f981cb8f0158 100644
@@ -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;

Namhyung Kim

author maintainer

2015-05-05 16:08 UTC #44 lore raw

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

In reply to Arnaldo Carvalho de Melo

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 <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
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(-)

tools/perf/ui/browsers/hists.c +47 ~~-32~~

index 8733d577db78..f981cb8f0158 100644
@@ -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;
 			}

tools/perf/util/callchain.h +4 -0

index 6033a0a212ca..679c2c6d8ade 100644
@@ -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;
 };

tools/perf/util/hist.c +1 -1

index cc22b9158b93..338770679863 100644
@@ -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;

tools/perf/util/sort.h +2 -0

index 4d923e6e0069..e97cd476d336 100644
@@ -109,6 +109,8 @@ struct hist_entry {
 			u16	row_offset;
 			u16	nr_rows;
 			bool	init_have_children;
+			bool	unfolded;
+			bool	has_children;
 		};
 	};
 	char			*srcline;

tools/perf/util/symbol.h +0 -2

index 90965296c129..bef47ead1d9b 100644
@@ -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 {

Signature · 1 line

2.3.7

Arnaldo Carvalho de Melo

maintainer

2015-05-05 16:11 UTC #45 lore raw

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

Em Tue, May 05, 2015 at 11:55:46PM +0900, Namhyung Kim escreveu:

Quoted 5 lines

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.WITH_NAMHYUNG_PATCHSET 2015-05-05 12:48:13.871216253 -0300 renamed from perf.hist.0.WITHOUT 2015-05-05 12:49:06.557802910 -0300 +1 -2

@@ -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-bot for Namhyung Kim

merge bot

2015-05-06 03:23 UTC #46 lore raw

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

In reply to Namhyung Kim

perf tools: Move TUI-specific fields out of map_symbol

Commit-ID:  3698dab1c849c7e1cd440df4fca24baa1973d53b
Gitweb:     http://git.kernel.org/tip/3698dab1c849c7e1cd440df4fca24baa1973d53b
Author:     Namhyung Kim <[email protected]>
AuthorDate: Tue, 5 May 2015 23:55:46 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>

Cc David Ahern <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 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(-)

tools/perf/ui/browsers/hists.c +47 ~~-32~~

index 8733d57..f981cb8 100644
@@ -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;
 			}

tools/perf/util/callchain.h +4 -0

index 6033a0a..679c2c6 100644
@@ -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;
 };

tools/perf/util/hist.c +1 -1

index cc22b91..3387706 100644
@@ -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;

tools/perf/util/sort.h +2 -0

index 4d923e6..e97cd47 100644
@@ -109,6 +109,8 @@ struct hist_entry {
 			u16	row_offset;
 			u16	nr_rows;
 			bool	init_have_children;
+			bool	unfolded;
+			bool	has_children;
 		};
 	};
 	char			*srcline;

tools/perf/util/symbol.h +0 -2

index 9096529..bef47ead 100644
@@ -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 {

Jiri Olsa

Acked-by on the v1 series

2015-04-22 11:17 UTC #47 lore raw

In reply to Namhyung Kim

Quoted 7 lines · 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.

Quoted 5 lines

* 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 <[email protected]>

thanks,
jirka

Namhyung Kim

author maintainer

2015-04-22 13:06 UTC #48 lore raw

Quoted 8 lines · Jiri Olsa

Quoted 7 lines · 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.

Quoted 7 lines

Quoted 5 lines
* 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 <[email protected]>

Thank you very much!
Namhyung