Skip to contentSkip to conversation

perf tools: Few fixes

PATCH In review Jiri Olsa 4 participants 12 messages Jan 20, 2017 – Jan 26, 2017 perf events

lore
Share & tools
lore

Jiri Olsa

author reviewer
#0 lore raw

cover perf tools: Few fixes

hi,
sending out few assorted fixes before I loose them ;-)

Available also here:
git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
perf/fixes

thanks,
jirka

---
Jiri Olsa (4):
perf hists browser: Put hist_entry folding login into single function perf hists browser: Add e/c key handlers to expand callchain for current entry perf c2c report: Display Total records column in offset view perf c2c report: Coalesce by default only by pid,iaddr

 tools/perf/Documentation/perf-c2c.txt |  2 +-
 tools/perf/builtin-c2c.c              |  3 ++-
 tools/perf/ui/browsers/hists.c        | 60 ++++++++++++++++++++++++++++++++++++++++++------------------
 3 files changed, 45 insertions(+), 20 deletions(-)
author reviewer
#1 lore raw

PATCH 1/4 perf hists browser: Put hist_entry folding login into single function

It will be used in following patch to expand or collapse only the current entry of the browser.

---
 tools/perf/ui/browsers/hists.c | 43 ++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)
tools/perf/ui/browsers/hists.c +25 -18
index 641b40234a9d..8bf18afe2a1f 100644
@@ -501,8 +501,8 @@ static int hierarchy_set_folding(struct hist_browser *hb, struct hist_entry *he,
 	return n;
 }
 
-static void hist_entry__set_folding(struct hist_entry *he,
-				    struct hist_browser *hb, bool unfold)
+static void __hist_entry__set_folding(struct hist_entry *he,
+				      struct hist_browser *hb, bool unfold)
 {
 	hist_entry__init_have_children(he);
 	he->unfolded = unfold ? he->has_children : false;
@@ -520,12 +520,34 @@ static void hist_entry__set_folding(struct hist_entry *he,
 		he->nr_rows = 0;
 }
 
+static void hist_entry__set_folding(struct hist_entry *he,
+				    struct hist_browser *browser, bool unfold)
+{
+	double percent;
+
+	percent = hist_entry__get_percent_limit(he);
+	if (he->filtered || percent < browser->min_pcnt)
+		return;
+
+	__hist_entry__set_folding(he, browser, unfold);
+
+	if (!he->depth || unfold)
+		browser->nr_hierarchy_entries++;
+	if (he->leaf)
+		browser->nr_callchain_rows += he->nr_rows;
+	else if (unfold && !hist_entry__has_hierarchy_children(he, browser->min_pcnt)) {
+		browser->nr_hierarchy_entries++;
+		he->has_no_entry = true;
+		he->nr_rows = 1;
+	} else
+		he->has_no_entry = false;
+}
+
 static void
 __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 {
 	struct rb_node *nd;
 	struct hist_entry *he;
-	double percent;
 
 	nd = rb_first(&browser->hists->entries);
 	while (nd) {
@@ -535,21 +557,6 @@ __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 		nd = __rb_hierarchy_next(nd, HMD_FORCE_CHILD);
 
 		hist_entry__set_folding(he, browser, unfold);
-
-		percent = hist_entry__get_percent_limit(he);
-		if (he->filtered || percent < browser->min_pcnt)
-			continue;
-
-		if (!he->depth || unfold)
-			browser->nr_hierarchy_entries++;
-		if (he->leaf)
-			browser->nr_callchain_rows += he->nr_rows;
-		else if (unfold && !hist_entry__has_hierarchy_children(he, browser->min_pcnt)) {
-			browser->nr_hierarchy_entries++;
-			he->has_no_entry = true;
-			he->nr_rows = 1;
-		} else
-			he->has_no_entry = false;
 	}
 }
 
Signature · 1 line
2.7.4
#2 lore raw

[tip:perf/core] perf hists browser: Put hist_entry folding logic into single function

perf hists browser: Put hist_entry folding logic into single function
Commit-ID:  b33f922651011effafec4508474e8591569a3e98
Gitweb:     http://git.kernel.org/tip/b33f922651011effafec4508474e8591569a3e98
Author:     Jiri Olsa <[email protected]>
AuthorDate: Fri, 20 Jan 2017 10:20:29 +0100
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 20 Jan 2017 10:04:45 -0300

perf hists browser: Put hist_entry folding logic into single function

It will be used in following patch to expand or collapse only the current browser entry.

---
 tools/perf/ui/browsers/hists.c | 43 ++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)
tools/perf/ui/browsers/hists.c +25 -18
index 641b402..8bf18af 100644
@@ -501,8 +501,8 @@ static int hierarchy_set_folding(struct hist_browser *hb, struct hist_entry *he,
 	return n;
 }
 
-static void hist_entry__set_folding(struct hist_entry *he,
-				    struct hist_browser *hb, bool unfold)
+static void __hist_entry__set_folding(struct hist_entry *he,
+				      struct hist_browser *hb, bool unfold)
 {
 	hist_entry__init_have_children(he);
 	he->unfolded = unfold ? he->has_children : false;
@@ -520,12 +520,34 @@ static void hist_entry__set_folding(struct hist_entry *he,
 		he->nr_rows = 0;
 }
 
+static void hist_entry__set_folding(struct hist_entry *he,
+				    struct hist_browser *browser, bool unfold)
+{
+	double percent;
+
+	percent = hist_entry__get_percent_limit(he);
+	if (he->filtered || percent < browser->min_pcnt)
+		return;
+
+	__hist_entry__set_folding(he, browser, unfold);
+
+	if (!he->depth || unfold)
+		browser->nr_hierarchy_entries++;
+	if (he->leaf)
+		browser->nr_callchain_rows += he->nr_rows;
+	else if (unfold && !hist_entry__has_hierarchy_children(he, browser->min_pcnt)) {
+		browser->nr_hierarchy_entries++;
+		he->has_no_entry = true;
+		he->nr_rows = 1;
+	} else
+		he->has_no_entry = false;
+}
+
 static void
 __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 {
 	struct rb_node *nd;
 	struct hist_entry *he;
-	double percent;
 
 	nd = rb_first(&browser->hists->entries);
 	while (nd) {
@@ -535,21 +557,6 @@ __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 		nd = __rb_hierarchy_next(nd, HMD_FORCE_CHILD);
 
 		hist_entry__set_folding(he, browser, unfold);
-
-		percent = hist_entry__get_percent_limit(he);
-		if (he->filtered || percent < browser->min_pcnt)
-			continue;
-
-		if (!he->depth || unfold)
-			browser->nr_hierarchy_entries++;
-		if (he->leaf)
-			browser->nr_callchain_rows += he->nr_rows;
-		else if (unfold && !hist_entry__has_hierarchy_children(he, browser->min_pcnt)) {
-			browser->nr_hierarchy_entries++;
-			he->has_no_entry = true;
-			he->nr_rows = 1;
-		} else
-			he->has_no_entry = false;
 	}
 }
 
author reviewer
#3 lore raw

PATCH 2/4 perf hists browser: Add e/c key handlers to expand callchain for current entry

In reply to Jiri Olsa

Currently we allow only to expand or collapse all entries in the browser with E or C keys. Allow user to expand or collapse only current entry in the browser with e or c key.

---
 tools/perf/ui/browsers/hists.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
tools/perf/ui/browsers/hists.c +17 -0
index 8bf18afe2a1f..fc4fb669ceee 100644
@@ -571,6 +571,15 @@ static void hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 	ui_browser__reset_index(&browser->b);
 }
 
+static void hist_browser__set_folding_selected(struct hist_browser *browser, bool unfold)
+{
+	if (!browser->he_selection)
+		return;
+
+	hist_entry__set_folding(browser->he_selection, browser, unfold);
+	browser->b.nr_entries = hist_browser__nr_entries(browser);
+}
+
 static void ui_browser__warn_lost_events(struct ui_browser *browser)
 {
 	ui_browser__warning(browser, 4,
@@ -644,10 +653,18 @@ int hist_browser__run(struct hist_browser *browser, const char *help)
 			/* Collapse the whole world. */
 			hist_browser__set_folding(browser, false);
 			break;
+		case 'c':
+			/* Collapse the selected entry. */
+			hist_browser__set_folding_selected(browser, false);
+			break;
 		case 'E':
 			/* Expand the whole world. */
 			hist_browser__set_folding(browser, true);
 			break;
+		case 'e':
+			/* Expand the selected entry. */
+			hist_browser__set_folding_selected(browser, true);
+			break;
 		case 'H':
 			browser->show_headers = !browser->show_headers;
 			hist_browser__update_rows(browser);
Signature · 1 line
2.7.4
#4 lore raw

Re: [PATCH 2/4] perf hists browser: Add e/c key handlers to expand callchain for current entry

Em Fri, Jan 20, 2017 at 10:20:30AM +0100, Jiri Olsa escreveu:

Quoted 3 lines

Currently we allow only to expand or collapse all entries in the browser with E or C keys. Allow user to expand or collapse only current entry in the browser with e or c key.

I guess we can start with this but then fix it to collapse just the subtree, not the whole tree for the current hist_entry, for big callchains we can have multiple sub(sub)trees, being able to expand/collapse just from where we are in a callchain seems even better.

Applying anyway, better than what we have now,

Thanks!

- Arnaldo

Quoted 50 lines
---
 tools/perf/ui/browsers/hists.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
tools/perf/ui/browsers/hists.c +17 -0
index 8bf18afe2a1f..fc4fb669ceee 100644
@@ -571,6 +571,15 @@ static void hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 	ui_browser__reset_index(&browser->b);
 }
 
+static void hist_browser__set_folding_selected(struct hist_browser *browser, bool unfold)
+{
+	if (!browser->he_selection)
+		return;
+
+	hist_entry__set_folding(browser->he_selection, browser, unfold);
+	browser->b.nr_entries = hist_browser__nr_entries(browser);
+}
+
 static void ui_browser__warn_lost_events(struct ui_browser *browser)
 {
 	ui_browser__warning(browser, 4,
@@ -644,10 +653,18 @@ int hist_browser__run(struct hist_browser *browser, const char *help)
 			/* Collapse the whole world. */
 			hist_browser__set_folding(browser, false);
 			break;
+		case 'c':
+			/* Collapse the selected entry. */
+			hist_browser__set_folding_selected(browser, false);
+			break;
 		case 'E':
 			/* Expand the whole world. */
 			hist_browser__set_folding(browser, true);
 			break;
+		case 'e':
+			/* Expand the selected entry. */
+			hist_browser__set_folding_selected(browser, true);
+			break;
 		case 'H':
 			browser->show_headers = !browser->show_headers;
 			hist_browser__update_rows(browser);
Signature · 1 line
2.7.4
#5 lore raw

Re: [PATCH 2/4] perf hists browser: Add e/c key handlers to expand callchain for current entry

Em Fri, Jan 20, 2017 at 01:41:33PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 11 lines

Em Fri, Jan 20, 2017 at 10:20:30AM +0100, Jiri Olsa escreveu:

Quoted 3 lines

Currently we allow only to expand or collapse all entries in the browser with E or C keys. Allow user to expand or collapse only current entry in the browser with e or c key.

I guess we can start with this but then fix it to collapse just the subtree, not the whole tree for the current hist_entry, for big callchains we can have multiple sub(sub)trees, being able to expand/collapse just from where we are in a callchain seems even better.

Applying anyway, better than what we have now,

Also I think that pressing 'e' on an already expanded tree should mean 'c', i.e. make it a toggle, so that we can go back and forth faster.

- Arnaldo

Quoted 50 lines
Quoted 50 lines
---
 tools/perf/ui/browsers/hists.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
tools/perf/ui/browsers/hists.c +17 -0
index 8bf18afe2a1f..fc4fb669ceee 100644
@@ -571,6 +571,15 @@ static void hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 	ui_browser__reset_index(&browser->b);
 }
 
+static void hist_browser__set_folding_selected(struct hist_browser *browser, bool unfold)
+{
+	if (!browser->he_selection)
+		return;
+
+	hist_entry__set_folding(browser->he_selection, browser, unfold);
+	browser->b.nr_entries = hist_browser__nr_entries(browser);
+}
+
 static void ui_browser__warn_lost_events(struct ui_browser *browser)
 {
 	ui_browser__warning(browser, 4,
@@ -644,10 +653,18 @@ int hist_browser__run(struct hist_browser *browser, const char *help)
 			/* Collapse the whole world. */
 			hist_browser__set_folding(browser, false);
 			break;
+		case 'c':
+			/* Collapse the selected entry. */
+			hist_browser__set_folding_selected(browser, false);
+			break;
 		case 'E':
 			/* Expand the whole world. */
 			hist_browser__set_folding(browser, true);
 			break;
+		case 'e':
+			/* Expand the selected entry. */
+			hist_browser__set_folding_selected(browser, true);
+			break;
 		case 'H':
 			browser->show_headers = !browser->show_headers;
 			hist_browser__update_rows(browser);
Signature · 1 line
2.7.4
#6 lore raw

Re: [PATCH 2/4] perf hists browser: Add e/c key handlers to expand callchain for current entry

Quoted 11 lines · Arnaldo Carvalho de Melo

Em Fri, Jan 20, 2017 at 01:41:33PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 10 lines

Em Fri, Jan 20, 2017 at 10:20:30AM +0100, Jiri Olsa escreveu:

Quoted 3 lines

Currently we allow only to expand or collapse all entries in the browser with E or C keys. Allow user to expand or collapse only current entry in the browser with e or c key.

I guess we can start with this but then fix it to collapse just the subtree, not the whole tree for the current hist_entry, for big callchains we can have multiple sub(sub)trees, being able to expand/collapse just from where we are in a callchain seems even better.

Quoted 4 lines
Quoted 1 line

Applying anyway, better than what we have now,

Also I think that pressing 'e' on an already expanded tree should mean 'c', i.e. make it a toggle, so that we can go back and forth faster.

yep, that was my impression also, but wanted to keep the current way

I think we could use only C/c keys to toggle 'collapse all'/'collapse current'

thanks,
jirka

#7 lore raw

[tip:perf/core] perf hists browser: Add e/c hotkeys to expand/collapse callchain for current entry

In reply to Jiri Olsa
perf hists browser: Add e/c hotkeys to expand/collapse callchain for current entry
Commit-ID:  0e3fa7a7acdd5f6ec89b3692276e35006c06fb92
Gitweb:     http://git.kernel.org/tip/0e3fa7a7acdd5f6ec89b3692276e35006c06fb92
Author:     Jiri Olsa <[email protected]>
AuthorDate: Fri, 20 Jan 2017 10:20:30 +0100
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 20 Jan 2017 13:37:26 -0300

perf hists browser: Add e/c hotkeys to expand/collapse callchain for current entry

Currently we allow only to expand or collapse all entries in the browser with 'E' or 'C' keys. Allow user to expand or collapse only current entry in the browser with e or c key.

---
 tools/perf/ui/browsers/hists.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
tools/perf/ui/browsers/hists.c +17 -0
index 8bf18af..fc4fb66 100644
@@ -571,6 +571,15 @@ static void hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 	ui_browser__reset_index(&browser->b);
 }
 
+static void hist_browser__set_folding_selected(struct hist_browser *browser, bool unfold)
+{
+	if (!browser->he_selection)
+		return;
+
+	hist_entry__set_folding(browser->he_selection, browser, unfold);
+	browser->b.nr_entries = hist_browser__nr_entries(browser);
+}
+
 static void ui_browser__warn_lost_events(struct ui_browser *browser)
 {
 	ui_browser__warning(browser, 4,
@@ -644,10 +653,18 @@ int hist_browser__run(struct hist_browser *browser, const char *help)
 			/* Collapse the whole world. */
 			hist_browser__set_folding(browser, false);
 			break;
+		case 'c':
+			/* Collapse the selected entry. */
+			hist_browser__set_folding_selected(browser, false);
+			break;
 		case 'E':
 			/* Expand the whole world. */
 			hist_browser__set_folding(browser, true);
 			break;
+		case 'e':
+			/* Expand the selected entry. */
+			hist_browser__set_folding_selected(browser, true);
+			break;
 		case 'H':
 			browser->show_headers = !browser->show_headers;
 			hist_browser__update_rows(browser);
author reviewer
#8 lore raw

PATCH 3/4 perf c2c report: Display Total records column in offset view

In reply to Jiri Olsa

Adding "Total records" column into cacheline pareto table, between cycles and cpu info.

  $ perf c2c report
  ...

--- ---------- cycles ---------- Total cpu

         rmt hitm  lcl hitm      load  records       cnt
  ...    ........  ........  ........  .......  ........
                0       112        71       34         4
                0         0         0       18         1
                0         0         0        2         1
                0       132         0        3         3

...

It's useful to see how many recorded samples
represent each offset.

---
 tools/perf/builtin-c2c.c | 1 +
 1 file changed, 1 insertion(+)
tools/perf/builtin-c2c.c +1 -0
index f8ca7a4ebabc..616cb1418c3f 100644
@@ -2476,6 +2476,7 @@ static int build_cl_output(char *cl_sort, bool no_source)
 		"mean_rmt,"
 		"mean_lcl,"
 		"mean_load,"
+		"tot_recs,"
 		"cpucnt,",
 		add_sym ? "symbol," : "",
 		add_dso ? "dso," : "",
Signature · 1 line
2.7.4
#9 lore raw

[tip:perf/core] perf c2c report: Display Total records column in offset view

perf c2c report: Display Total records column in offset view
Commit-ID:  8763e6ac2d368ac9d650df35b00c3af2a7c1878b
Gitweb:     http://git.kernel.org/tip/8763e6ac2d368ac9d650df35b00c3af2a7c1878b
Author:     Jiri Olsa <[email protected]>
AuthorDate: Fri, 20 Jan 2017 10:20:31 +0100
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 20 Jan 2017 16:52:43 -0300

perf c2c report: Display Total records column in offset view

Adding "Total records" column into cacheline pareto table, between cycles and cpu info.

  $ perf c2c report
  ...

--- ---------- cycles ---------- Total cpu

         rmt hitm  lcl hitm      load  records       cnt
  ...    ........  ........  ........  .......  ........
                0       112        71       34         4
                0         0         0       18         1
                0         0         0        2         1
                0       132         0        3         3

...

It's useful to see how many recorded samples represent each offset.

---
 tools/perf/builtin-c2c.c | 1 +
 1 file changed, 1 insertion(+)
tools/perf/builtin-c2c.c +1 -0
index f8ca7a4..616cb14 100644
@@ -2476,6 +2476,7 @@ static int build_cl_output(char *cl_sort, bool no_source)
 		"mean_rmt,"
 		"mean_lcl,"
 		"mean_load,"
+		"tot_recs,"
 		"cpucnt,",
 		add_sym ? "symbol," : "",
 		add_dso ? "dso," : "",
author reviewer
#10 lore raw

PATCH 4/4 perf c2c report: Coalesce by default only by pid,iaddr

In reply to Jiri Olsa

It seems to be the most used argument for -c option so far. In the beginning when you want to have the overall process report, so it makes sense to make it the default one.

---
 tools/perf/Documentation/perf-c2c.txt | 2 +-
 tools/perf/builtin-c2c.c              | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
tools/perf/Documentation/perf-c2c.txt +1 -1
index 3f06730c7f47..2da07e51e119 100644
@@ -248,7 +248,7 @@ output fields set for caheline offsets output:
              Code address, Code symbol, Shared Object, Source line
   dso   - coalesced by shared object
 
-By default the coalescing is setup with 'pid,tid,iaddr'.
+By default the coalescing is setup with 'pid,iaddr'.
 
 STDIO OUTPUT
 ------------
tools/perf/builtin-c2c.c +1 -1
index 616cb1418c3f..e2b21723bbf8 100644
@@ -58,7 +58,7 @@ struct c2c_hist_entry {
 	struct hist_entry	he;
 };
 
-static char const *coalesce_default = "pid,tid,iaddr";
+static char const *coalesce_default = "pid,iaddr";
 
 struct perf_c2c {
 	struct perf_tool	tool;
Signature · 1 line
2.7.4
#11 lore raw

[tip:perf/core] perf c2c report: Coalesce by default only by pid,iaddr

perf c2c report: Coalesce by default only by pid,iaddr
Commit-ID:  190bacca16d0627dce1d4ceb873f041ebbaef69a
Gitweb:     http://git.kernel.org/tip/190bacca16d0627dce1d4ceb873f041ebbaef69a
Author:     Jiri Olsa <[email protected]>
AuthorDate: Fri, 20 Jan 2017 10:20:32 +0100
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 20 Jan 2017 16:52:56 -0300

perf c2c report: Coalesce by default only by pid,iaddr

It seems to be the most used argument for -c option so far. In the beginning when you want to have the overall process report, so it makes sense to make it the default one.

---
 tools/perf/Documentation/perf-c2c.txt | 2 +-
 tools/perf/builtin-c2c.c              | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
tools/perf/Documentation/perf-c2c.txt +1 -1
index 3f06730..2da07e5 100644
@@ -248,7 +248,7 @@ output fields set for caheline offsets output:
              Code address, Code symbol, Shared Object, Source line
   dso   - coalesced by shared object
 
-By default the coalescing is setup with 'pid,tid,iaddr'.
+By default the coalescing is setup with 'pid,iaddr'.
 
 STDIO OUTPUT
 ------------
tools/perf/builtin-c2c.c +1 -1
index 616cb14..e2b2172 100644
@@ -58,7 +58,7 @@ struct c2c_hist_entry {
 	struct hist_entry	he;
 };
 
-static char const *coalesce_default = "pid,tid,iaddr";
+static char const *coalesce_default = "pid,iaddr";
 
 struct perf_c2c {
 	struct perf_tool	tool;