[PATCHSET 0/2] perf hists browser: Cleanup callchain routines (v2)

[PATCHSET 0/2] perf hists browser: Cleanup callchain routines (v2)

[Namhyung Kim]

Hello,

This patch fixes and cleans up TUI callchain routines.  I tried to
consolidate similar functions but not to break the current output with
this change.  Hope that it makes code more readable and maintainable.

It should not change any behavior and outputs.  I verified it with
expanding by 'E' key + dumping by 'P' key on TUI and then running
"diff -u" results before and after the patchset.

Actually I have more changes that would change (or improve) some
behavior and output.  I'll post them after this patchset is merged.


 * changes in v2)
  - drop patch 1-3 in v1 since it's already merged
  - update description  (Arnaldo)
  - remove stylish changes  (Arnaldo)
  - remove unnecessary 'has_single_node' check


You can get this from 'perf/callchain-fix-v2' branch on my tree:

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

Any comments are welcome, thanks
Namhyung


Namhyung Kim (2):
  perf hists browser: Cleanup callchain print functions
  perf hists browser: Consolidate callchain print functions in TUI

 tools/perf/ui/browsers/hists.c | 302 +++++++++++++----------------------------
 1 file changed, 95 insertions(+), 207 deletions(-)

-- 
2.0.0

[PATCH v2 1/2] perf hists browser: Cleanup callchain print functions

[Namhyung Kim]

The hist_browser__show_callchain() and friends don't need to be that
complex.  They're splitted in 3 pieces - one for traversing top-level
tree, other one for special casing first chains in the top-level
entries, and last one for recursive traversing inner trees.  It led to
code duplication and unnecessary complexity IMHO.

Simplify the function and consolidate the logic into a single function
- it can recursively call itself.  A little difference in printing
callchains in top-level tree can be handled with a small change.

It should have no functional change.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 112 +++++++++++------------------------------
 1 file changed, 29 insertions(+), 83 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index d42d8a8f3810..519353d9f5fb 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -502,23 +502,16 @@ static void hist_browser__show_callchain_entry(struct hist_browser *browser,
 
 #define LEVEL_OFFSET_STEP 3
 
-static int hist_browser__show_callchain_node_rb_tree(struct hist_browser *browser,
-						     struct callchain_node *chain_node,
-						     u64 total, int level,
-						     unsigned short row,
-						     off_t *row_offset,
-						     bool *is_current_entry)
+static int hist_browser__show_callchain(struct hist_browser *browser,
+					struct rb_root *root, int level,
+					unsigned short row, off_t *row_offset,
+					u64 total, bool *is_current_entry)
 {
 	struct rb_node *node;
 	int first_row = row, offset = level * LEVEL_OFFSET_STEP;
 	u64 new_total;
 
-	if (callchain_param.mode == CHAIN_GRAPH_REL)
-		new_total = chain_node->children_hit;
-	else
-		new_total = total;
-
-	node = rb_first(&chain_node->rb_root);
+	node = rb_first(root);
 	while (node) {
 		struct callchain_node *child = rb_entry(node, struct callchain_node, rb_node);
 		struct rb_node *next = rb_next(node);
@@ -535,7 +528,7 @@ static int hist_browser__show_callchain_node_rb_tree(struct hist_browser *browse
 
 			if (first)
 				first = false;
-			else
+			else if (level > 1)
 				extra_offset = LEVEL_OFFSET_STEP;
 
 			folded_sign = callchain_list__folded(chain);
@@ -547,8 +540,9 @@ static int hist_browser__show_callchain_node_rb_tree(struct hist_browser *browse
 			alloc_str = NULL;
 			str = callchain_list__sym_name(chain, bf, sizeof(bf),
 						       browser->show_dso);
-			if (was_first) {
-				double percent = cumul * 100.0 / new_total;
+
+			if (was_first && level > 1) {
+				double percent = cumul * 100.0 / total;
 
 				if (asprintf(&alloc_str, "%2.2f%% %s", percent, str) < 0)
 					str = "Not enough memory!";
@@ -571,78 +565,23 @@ do_next:
 
 		if (folded_sign == '-') {
 			const int new_level = level + (extra_offset ? 2 : 1);
-			row += hist_browser__show_callchain_node_rb_tree(browser, child, new_total,
-									 new_level, row, row_offset,
-									 is_current_entry);
-		}
-		if (row == browser->b.rows)
-			goto out;
-		node = next;
-	}
-out:
-	return row - first_row;
-}
-
-static int hist_browser__show_callchain_node(struct hist_browser *browser,
-					     struct callchain_node *node,
-					     int level, unsigned short row,
-					     off_t *row_offset,
-					     bool *is_current_entry)
-{
-	struct callchain_list *chain;
-	int first_row = row;
-	int offset = level * LEVEL_OFFSET_STEP;
-	char folded_sign = ' ';
-
-	list_for_each_entry(chain, &node->val, list) {
-		char bf[1024], *s;
 
-		folded_sign = callchain_list__folded(chain);
+			if (callchain_param.mode == CHAIN_GRAPH_REL)
+				new_total = child->children_hit;
+			else
+				new_total = total;
 
-		if (*row_offset != 0) {
-			--*row_offset;
-			continue;
+			row += hist_browser__show_callchain(browser, &child->rb_root,
+							    new_level,
+							    row, row_offset,
+							    new_total,
+							    is_current_entry);
 		}
-
-		s = callchain_list__sym_name(chain, bf, sizeof(bf),
-					     browser->show_dso);
-		hist_browser__show_callchain_entry(browser, chain, row,
-						   offset, folded_sign, s,
-						   is_current_entry);
-
-		if (++row == browser->b.rows)
-			goto out;
-	}
-
-	if (folded_sign == '-')
-		row += hist_browser__show_callchain_node_rb_tree(browser, node,
-								 browser->hists->stats.total_period,
-								 level + 1, row,
-								 row_offset,
-								 is_current_entry);
-out:
-	return row - first_row;
-}
-
-static int hist_browser__show_callchain(struct hist_browser *browser,
-					struct rb_root *chain,
-					int level, unsigned short row,
-					off_t *row_offset,
-					bool *is_current_entry)
-{
-	struct rb_node *nd;
-	int first_row = row;
-
-	for (nd = rb_first(chain); nd; nd = rb_next(nd)) {
-		struct callchain_node *node = rb_entry(nd, struct callchain_node, rb_node);
-
-		row += hist_browser__show_callchain_node(browser, node, level,
-							 row, row_offset,
-							 is_current_entry);
 		if (row == browser->b.rows)
 			break;
+		node = next;
 	}
-
+out:
 	return row - first_row;
 }
 
@@ -817,9 +756,16 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 		--row_offset;
 
 	if (folded_sign == '-' && row != browser->b.rows) {
-		printed += hist_browser__show_callchain(browser, &entry->sorted_chain,
+		u64 total = hists__total_period(entry->hists);
+
+		if (symbol_conf.cumulate_callchain)
+			total = entry->stat_acc->period;
+
+		printed += hist_browser__show_callchain(browser,
+							&entry->sorted_chain,
 							1, row, &row_offset,
-							&current_entry);
+							total, &current_entry);
+
 		if (current_entry)
 			browser->he_selection = entry;
 	}
-- 
2.0.0

[PATCH v2 2/2] perf hists browser: Consolidate callchain print functions in TUI

[Namhyung Kim]

Currently there're two callchain print functions in TUI - one for the
hists browser and another for file dump.  They do almost same job so
it'd be better consolidate the codes.

To do that, move row calculation code into a print callback so that
the dump code cannot be limited by the current screen size.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 210 +++++++++++++++--------------------------
 1 file changed, 76 insertions(+), 134 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 519353d9f5fb..48d8c8eee6c2 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -477,20 +477,32 @@ static char *callchain_list__sym_name(struct callchain_list *cl,
 	return bf;
 }
 
+struct callchain_print_arg {
+	/* for hists browser */
+	unsigned short row;
+	off_t row_offset;
+	bool is_current_entry;
+
+	/* for file dump */
+	FILE *fp;
+	int printed;
+};
+
 static void hist_browser__show_callchain_entry(struct hist_browser *browser,
 					       struct callchain_list *chain,
-					       unsigned short row, int offset,
-					       char folded_sign, const char *str,
-					       bool *is_current_entry)
+					       const char *str, int offset,
+					       struct callchain_print_arg *arg)
 {
 	int color, width;
+	unsigned short row = arg->row;
+	char folded_sign = callchain_list__folded(chain);
 
 	color = HE_COLORSET_NORMAL;
 	width = browser->b.width - (offset + 2);
 	if (ui_browser__is_current_entry(&browser->b, row)) {
 		browser->selection = &chain->ms;
 		color = HE_COLORSET_SELECTED;
-		*is_current_entry = true;
+		arg->is_current_entry = true;
 	}
 
 	ui_browser__set_color(&browser->b, color);
@@ -498,17 +510,40 @@ static void hist_browser__show_callchain_entry(struct hist_browser *browser,
 	slsmg_write_nstring(" ", offset);
 	slsmg_printf("%c ", folded_sign);
 	slsmg_write_nstring(str, width);
+
+	/*
+	 * increase row here so that we can reuse the
+	 * hist_browser__show_callchain() for dumping the whole
+	 * callchain to a file.
+	 */
+	arg->row++;
+}
+
+static void hist_browser__fprintf_callchain_entry(struct hist_browser *b __maybe_unused,
+						  struct callchain_list *chain,
+						  const char *str, int offset,
+						  struct callchain_print_arg *arg)
+{
+	char folded_sign = callchain_list__folded(chain);
+
+	arg->printed += fprintf(arg->fp, "%*s%c %s\n", offset, " ",
+				folded_sign, str);
 }
 
+typedef void (*print_callchain_entry_fn)(struct hist_browser *browser,
+					 struct callchain_list *chain,
+					 const char *str, int offset,
+					 struct callchain_print_arg *arg);
+
 #define LEVEL_OFFSET_STEP 3
 
 static int hist_browser__show_callchain(struct hist_browser *browser,
-					struct rb_root *root, int level,
-					unsigned short row, off_t *row_offset,
-					u64 total, bool *is_current_entry)
+					struct rb_root *root, int level, u64 total,
+					print_callchain_entry_fn print,
+					struct callchain_print_arg *arg)
 {
 	struct rb_node *node;
-	int first_row = row, offset = level * LEVEL_OFFSET_STEP;
+	int first_row = arg->row, offset = level * LEVEL_OFFSET_STEP;
 	u64 new_total;
 
 	node = rb_first(root);
@@ -532,8 +567,8 @@ static int hist_browser__show_callchain(struct hist_browser *browser,
 				extra_offset = LEVEL_OFFSET_STEP;
 
 			folded_sign = callchain_list__folded(chain);
-			if (*row_offset != 0) {
-				--*row_offset;
+			if (arg->row_offset != 0) {
+				arg->row_offset--;
 				goto do_next;
 			}
 
@@ -550,13 +585,11 @@ static int hist_browser__show_callchain(struct hist_browser *browser,
 					str = alloc_str;
 			}
 
-			hist_browser__show_callchain_entry(browser, chain, row,
-							   offset + extra_offset,
-							   folded_sign, str,
-							   is_current_entry);
+			print(browser, chain, str, offset + extra_offset, arg);
+
 			free(alloc_str);
 
-			if (++row == browser->b.rows)
+			if (arg->row == browser->b.rows)
 				goto out;
 do_next:
 			if (folded_sign == '+')
@@ -571,18 +604,16 @@ do_next:
 			else
 				new_total = total;
 
-			row += hist_browser__show_callchain(browser, &child->rb_root,
-							    new_level,
-							    row, row_offset,
-							    new_total,
-							    is_current_entry);
+			hist_browser__show_callchain(browser, &child->rb_root,
+						     new_level, new_total,
+						     print, arg);
 		}
-		if (row == browser->b.rows)
+		if (arg->row == browser->b.rows)
 			break;
 		node = next;
 	}
 out:
-	return row - first_row;
+	return arg->row - first_row;
 }
 
 struct hpp_arg {
@@ -757,16 +788,21 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 
 	if (folded_sign == '-' && row != browser->b.rows) {
 		u64 total = hists__total_period(entry->hists);
+		struct callchain_print_arg arg = {
+			.row = row,
+			.row_offset = row_offset,
+			.is_current_entry = current_entry,
+		};
 
 		if (symbol_conf.cumulate_callchain)
 			total = entry->stat_acc->period;
 
 		printed += hist_browser__show_callchain(browser,
-							&entry->sorted_chain,
-							1, row, &row_offset,
-							total, &current_entry);
+					&entry->sorted_chain, 1, total,
+					hist_browser__show_callchain_entry,
+					&arg);
 
-		if (current_entry)
+		if (arg.is_current_entry)
 			browser->he_selection = entry;
 	}
 
@@ -1022,112 +1058,6 @@ do_offset:
 	}
 }
 
-static int hist_browser__fprintf_callchain_node_rb_tree(struct hist_browser *browser,
-							struct callchain_node *chain_node,
-							u64 total, int level,
-							FILE *fp)
-{
-	struct rb_node *node;
-	int offset = level * LEVEL_OFFSET_STEP;
-	u64 new_total;
-	int printed = 0;
-
-	if (callchain_param.mode == CHAIN_GRAPH_REL)
-		new_total = chain_node->children_hit;
-	else
-		new_total = total;
-
-	node = rb_first(&chain_node->rb_root);
-	while (node) {
-		struct callchain_node *child = rb_entry(node, struct callchain_node, rb_node);
-		struct rb_node *next = rb_next(node);
-		u64 cumul = callchain_cumul_hits(child);
-		struct callchain_list *chain;
-		char folded_sign = ' ';
-		int first = true;
-		int extra_offset = 0;
-
-		list_for_each_entry(chain, &child->val, list) {
-			char bf[1024], *alloc_str;
-			const char *str;
-			bool was_first = first;
-
-			if (first)
-				first = false;
-			else
-				extra_offset = LEVEL_OFFSET_STEP;
-
-			folded_sign = callchain_list__folded(chain);
-
-			alloc_str = NULL;
-			str = callchain_list__sym_name(chain, bf, sizeof(bf),
-						       browser->show_dso);
-			if (was_first) {
-				double percent = cumul * 100.0 / new_total;
-
-				if (asprintf(&alloc_str, "%2.2f%% %s", percent, str) < 0)
-					str = "Not enough memory!";
-				else
-					str = alloc_str;
-			}
-
-			printed += fprintf(fp, "%*s%c %s\n", offset + extra_offset, " ", folded_sign, str);
-			free(alloc_str);
-			if (folded_sign == '+')
-				break;
-		}
-
-		if (folded_sign == '-') {
-			const int new_level = level + (extra_offset ? 2 : 1);
-			printed += hist_browser__fprintf_callchain_node_rb_tree(browser, child, new_total,
-										new_level, fp);
-		}
-
-		node = next;
-	}
-
-	return printed;
-}
-
-static int hist_browser__fprintf_callchain_node(struct hist_browser *browser,
-						struct callchain_node *node,
-						int level, FILE *fp)
-{
-	struct callchain_list *chain;
-	int offset = level * LEVEL_OFFSET_STEP;
-	char folded_sign = ' ';
-	int printed = 0;
-
-	list_for_each_entry(chain, &node->val, list) {
-		char bf[1024], *s;
-
-		folded_sign = callchain_list__folded(chain);
-		s = callchain_list__sym_name(chain, bf, sizeof(bf), browser->show_dso);
-		printed += fprintf(fp, "%*s%c %s\n", offset, " ", folded_sign, s);
-	}
-
-	if (folded_sign == '-')
-		printed += hist_browser__fprintf_callchain_node_rb_tree(browser, node,
-									browser->hists->stats.total_period,
-									level + 1,  fp);
-	return printed;
-}
-
-static int hist_browser__fprintf_callchain(struct hist_browser *browser,
-					   struct rb_root *chain, int level, FILE *fp)
-{
-	struct rb_node *nd;
-	int printed = 0;
-
-	for (nd = rb_first(chain); nd; nd = rb_next(nd)) {
-		struct callchain_node *node = rb_entry(nd, struct callchain_node, rb_node);
-
-		printed += hist_browser__fprintf_callchain_node(browser, node, level, fp);
-	}
-
-	return printed;
-}
-
 static int hist_browser__fprintf_entry(struct hist_browser *browser,
 				       struct hist_entry *he, FILE *fp)
 {
@@ -1163,8 +1093,20 @@ static int hist_browser__fprintf_entry(struct hist_browser *browser,
 	}
 	printed += fprintf(fp, "%s\n", rtrim(s));
 
-	if (folded_sign == '-')
-		printed += hist_browser__fprintf_callchain(browser, &he->sorted_chain, 1, fp);
+	if (folded_sign == '-') {
+		u64 total = hists__total_period(he->hists);
+		struct callchain_print_arg arg  = {
+			.fp = fp,
+		};
+
+		if (symbol_conf.cumulate_callchain)
+			total = he->stat_acc->period;
+
+		hist_browser__show_callchain(browser, &he->sorted_chain, 1, total,
+					     hist_browser__fprintf_callchain_entry,
+					     &arg);
+		printed += arg.printed;
+	}
 
 	return printed;
 }
-- 
2.0.0

Re: [PATCHSET 0/2] perf hists browser: Cleanup callchain routines (v2)

[Jiri Olsa]

On Thu, Aug 21, 2014 at 10:15:44AM +0900, Namhyung Kim wrote:
> Hello,
> 
> This patch fixes and cleans up TUI callchain routines.  I tried to
> consolidate similar functions but not to break the current output with
> this change.  Hope that it makes code more readable and maintainable.

76 insertions(+), 134 deletions(-) ... nice! ;-)

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

jirka

Re: [PATCHSET 0/2] perf hists browser: Cleanup callchain routines (v2)

[Arnaldo Carvalho de Melo]

Em Thu, Aug 21, 2014 at 10:11:17AM +0200, Jiri Olsa escreveu:
> On Thu, Aug 21, 2014 at 10:15:44AM +0900, Namhyung Kim wrote:
> > Hello,
> > 
> > This patch fixes and cleans up TUI callchain routines.  I tried to
> > consolidate similar functions but not to break the current output with
> > this change.  Hope that it makes code more readable and maintainable.
> 
> 76 insertions(+), 134 deletions(-) ... nice! ;-)
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>

indeed :-)

- Arnaldo

Re: [PATCH v2 2/2] perf hists browser: Consolidate callchain print functions in TUI

[Arnaldo Carvalho de Melo]

Em Thu, Aug 21, 2014 at 10:15:46AM +0900, Namhyung Kim escreveu:
> Currently there're two callchain print functions in TUI - one for the
> hists browser and another for file dump.  They do almost same job so
> it'd be better consolidate the codes.

Some comments below, thanks for doing this work!
 
> To do that, move row calculation code into a print callback so that
> the dump code cannot be limited by the current screen size.
 
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/browsers/hists.c | 210 +++++++++++++++--------------------------
>  1 file changed, 76 insertions(+), 134 deletions(-)
> 
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 519353d9f5fb..48d8c8eee6c2 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -477,20 +477,32 @@ static char *callchain_list__sym_name(struct callchain_list *cl,
>  	return bf;
>  }
>  
> +struct callchain_print_arg {
> +	/* for hists browser */
> +	unsigned short row;
> +	off_t row_offset;
> +	bool is_current_entry;
> +
> +	/* for file dump */
> +	FILE *fp;
> +	int printed;
> +};
> +
>  static void hist_browser__show_callchain_entry(struct hist_browser *browser,
>  					       struct callchain_list *chain,
> -					       unsigned short row, int offset,
> -					       char folded_sign, const char *str,
> -					       bool *is_current_entry)
> +					       const char *str, int offset,
> +					       struct callchain_print_arg *arg)
>  {
>  	int color, width;
> +	unsigned short row = arg->row;
> +	char folded_sign = callchain_list__folded(chain);
>  
>  	color = HE_COLORSET_NORMAL;
>  	width = browser->b.width - (offset + 2);
>  	if (ui_browser__is_current_entry(&browser->b, row)) {
>  		browser->selection = &chain->ms;
>  		color = HE_COLORSET_SELECTED;
> -		*is_current_entry = true;
> +		arg->is_current_entry = true;
>  	}
>  
>  	ui_browser__set_color(&browser->b, color);
> @@ -498,17 +510,40 @@ static void hist_browser__show_callchain_entry(struct hist_browser *browser,
>  	slsmg_write_nstring(" ", offset);
>  	slsmg_printf("%c ", folded_sign);
>  	slsmg_write_nstring(str, width);
> +
> +	/*
> +	 * increase row here so that we can reuse the
> +	 * hist_browser__show_callchain() for dumping the whole
> +	 * callchain to a file.
> +	 */
> +	arg->row++;
> +}
> +
> +static void hist_browser__fprintf_callchain_entry(struct hist_browser *b __maybe_unused,
> +						  struct callchain_list *chain,
> +						  const char *str, int offset,
> +						  struct callchain_print_arg *arg)
> +{
> +	char folded_sign = callchain_list__folded(chain);
> +
> +	arg->printed += fprintf(arg->fp, "%*s%c %s\n", offset, " ",
> +				folded_sign, str);
>  }
>  
> +typedef void (*print_callchain_entry_fn)(struct hist_browser *browser,
> +					 struct callchain_list *chain,
> +					 const char *str, int offset,
> +					 struct callchain_print_arg *arg);
> +
>  #define LEVEL_OFFSET_STEP 3
>  
>  static int hist_browser__show_callchain(struct hist_browser *browser,
> -					struct rb_root *root, int level,
> -					unsigned short row, off_t *row_offset,
> -					u64 total, bool *is_current_entry)
> +					struct rb_root *root, int level, u64 total,
> +					print_callchain_entry_fn print,
> +					struct callchain_print_arg *arg)
>  {
>  	struct rb_node *node;
> -	int first_row = row, offset = level * LEVEL_OFFSET_STEP;
> +	int first_row = arg->row, offset = level * LEVEL_OFFSET_STEP;
>  	u64 new_total;
>  
>  	node = rb_first(root);
> @@ -532,8 +567,8 @@ static int hist_browser__show_callchain(struct hist_browser *browser,
>  				extra_offset = LEVEL_OFFSET_STEP;
>  
>  			folded_sign = callchain_list__folded(chain);
> -			if (*row_offset != 0) {
> -				--*row_offset;
> +			if (arg->row_offset != 0) {
> +				arg->row_offset--;
>  				goto do_next;
>  			}
>  
> @@ -550,13 +585,11 @@ static int hist_browser__show_callchain(struct hist_browser *browser,
>  					str = alloc_str;
>  			}
>  
> -			hist_browser__show_callchain_entry(browser, chain, row,
> -							   offset + extra_offset,
> -							   folded_sign, str,
> -							   is_current_entry);
> +			print(browser, chain, str, offset + extra_offset, arg);
> +
>  			free(alloc_str);
>  
> -			if (++row == browser->b.rows)
> +			if (arg->row == browser->b.rows)

Humm, why aren't we incrementing arg->row before comparing it? I.e. this
looks like a change in behaviour.

I see, that callback will do it just before this if, ok. I guess this is
because you don't need to account the number of rows printed when
dumping it to a file, but then I think you can keep doing it even if is
not being used in the dumping to the file, this way the above hunk
becomes:

> -			if (++row == browser->b.rows)
> +			if (++arg->row == browser->b.rows)

And a reviewer wouldn't think about this as a change in behaviour. :-)

>  				goto out;
>  do_next:
>  			if (folded_sign == '+')
> @@ -571,18 +604,16 @@ do_next:
>  			else
>  				new_total = total;
>  
> -			row += hist_browser__show_callchain(browser, &child->rb_root,
> -							    new_level,
> -							    row, row_offset,
> -							    new_total,
> -							    is_current_entry);
> +			hist_browser__show_callchain(browser, &child->rb_root,
> +						     new_level, new_total,
> +						     print, arg);

Same thing here, humm, understood, you don't want to stop printing...
but this looks kludgy, perhaps you should have another callback in
addition to print_callchain_entry_fn() that would, with those
parameters, state if it should continue or not, making this all clearer.

>  		}
> -		if (row == browser->b.rows)
> +		if (arg->row == browser->b.rows)
>  			break;

I.e. something like:

		if (output_full(row, browser->b.rows))
			break;

Then the screen dumper would do the existing test:

		return (row == browser->b.rows);

And the file dumper would just:

		return false;

Because it wants all rows.

I.e. instead of just have special cases for the making the test about
reaching the max number of rows not ever be true in the file dumping
case by plain not incrementing the number of rows, do as before, i.e.
increase the number of printed rows and just provide different
output_full() callbacks to decide when to stop.

>  		node = next;
>  	}
>  out:
> -	return row - first_row;
> +	return arg->row - first_row;
>  }
>  
>  struct hpp_arg {
> @@ -757,16 +788,21 @@ static int hist_browser__show_entry(struct hist_browser *browser,
>  
>  	if (folded_sign == '-' && row != browser->b.rows) {
>  		u64 total = hists__total_period(entry->hists);
> +		struct callchain_print_arg arg = {
> +			.row = row,
> +			.row_offset = row_offset,
> +			.is_current_entry = current_entry,
> +		};
>  
>  		if (symbol_conf.cumulate_callchain)
>  			total = entry->stat_acc->period;
>  
>  		printed += hist_browser__show_callchain(browser,
> -							&entry->sorted_chain,
> -							1, row, &row_offset,
> -							total, &current_entry);
> +					&entry->sorted_chain, 1, total,
> +					hist_browser__show_callchain_entry,
> +					&arg);
>  
> -		if (current_entry)
> +		if (arg.is_current_entry)
>  			browser->he_selection = entry;
>  	}
>  
> @@ -1022,112 +1058,6 @@ do_offset:
>  	}
>  }
>  
> -static int hist_browser__fprintf_callchain_node_rb_tree(struct hist_browser *browser,
> -							struct callchain_node *chain_node,
> -							u64 total, int level,
> -							FILE *fp)
> -{
> -	struct rb_node *node;
> -	int offset = level * LEVEL_OFFSET_STEP;
> -	u64 new_total;
> -	int printed = 0;
> -
> -	if (callchain_param.mode == CHAIN_GRAPH_REL)
> -		new_total = chain_node->children_hit;
> -	else
> -		new_total = total;
> -
> -	node = rb_first(&chain_node->rb_root);
> -	while (node) {
> -		struct callchain_node *child = rb_entry(node, struct callchain_node, rb_node);
> -		struct rb_node *next = rb_next(node);
> -		u64 cumul = callchain_cumul_hits(child);
> -		struct callchain_list *chain;
> -		char folded_sign = ' ';
> -		int first = true;
> -		int extra_offset = 0;
> -
> -		list_for_each_entry(chain, &child->val, list) {
> -			char bf[1024], *alloc_str;
> -			const char *str;
> -			bool was_first = first;
> -
> -			if (first)
> -				first = false;
> -			else
> -				extra_offset = LEVEL_OFFSET_STEP;
> -
> -			folded_sign = callchain_list__folded(chain);
> -
> -			alloc_str = NULL;
> -			str = callchain_list__sym_name(chain, bf, sizeof(bf),
> -						       browser->show_dso);
> -			if (was_first) {
> -				double percent = cumul * 100.0 / new_total;
> -
> -				if (asprintf(&alloc_str, "%2.2f%% %s", percent, str) < 0)
> -					str = "Not enough memory!";
> -				else
> -					str = alloc_str;
> -			}
> -
> -			printed += fprintf(fp, "%*s%c %s\n", offset + extra_offset, " ", folded_sign, str);
> -			free(alloc_str);
> -			if (folded_sign == '+')
> -				break;
> -		}
> -
> -		if (folded_sign == '-') {
> -			const int new_level = level + (extra_offset ? 2 : 1);
> -			printed += hist_browser__fprintf_callchain_node_rb_tree(browser, child, new_total,
> -										new_level, fp);
> -		}
> -
> -		node = next;
> -	}
> -
> -	return printed;
> -}
> -
> -static int hist_browser__fprintf_callchain_node(struct hist_browser *browser,
> -						struct callchain_node *node,
> -						int level, FILE *fp)
> -{
> -	struct callchain_list *chain;
> -	int offset = level * LEVEL_OFFSET_STEP;
> -	char folded_sign = ' ';
> -	int printed = 0;
> -
> -	list_for_each_entry(chain, &node->val, list) {
> -		char bf[1024], *s;
> -
> -		folded_sign = callchain_list__folded(chain);
> -		s = callchain_list__sym_name(chain, bf, sizeof(bf), browser->show_dso);
> -		printed += fprintf(fp, "%*s%c %s\n", offset, " ", folded_sign, s);
> -	}
> -
> -	if (folded_sign == '-')
> -		printed += hist_browser__fprintf_callchain_node_rb_tree(browser, node,
> -									browser->hists->stats.total_period,
> -									level + 1,  fp);
> -	return printed;
> -}
> -
> -static int hist_browser__fprintf_callchain(struct hist_browser *browser,
> -					   struct rb_root *chain, int level, FILE *fp)
> -{
> -	struct rb_node *nd;
> -	int printed = 0;
> -
> -	for (nd = rb_first(chain); nd; nd = rb_next(nd)) {
> -		struct callchain_node *node = rb_entry(nd, struct callchain_node, rb_node);
> -
> -		printed += hist_browser__fprintf_callchain_node(browser, node, level, fp);
> -	}
> -
> -	return printed;
> -}
> -
>  static int hist_browser__fprintf_entry(struct hist_browser *browser,
>  				       struct hist_entry *he, FILE *fp)
>  {
> @@ -1163,8 +1093,20 @@ static int hist_browser__fprintf_entry(struct hist_browser *browser,
>  	}
>  	printed += fprintf(fp, "%s\n", rtrim(s));
>  
> -	if (folded_sign == '-')
> -		printed += hist_browser__fprintf_callchain(browser, &he->sorted_chain, 1, fp);

So here please keep the call to hist_browser__fprintf_callchain() and
make it have the following lines, so that we don't even get to this
point and by reading the patch we more clearly understand what is
replacing hist_browser__fprintf_callchain().

> +	if (folded_sign == '-') {
> +		u64 total = hists__total_period(he->hists);
> +		struct callchain_print_arg arg  = {
> +			.fp = fp,
> +		};
> +
> +		if (symbol_conf.cumulate_callchain)
> +			total = he->stat_acc->period;
> +
> +		hist_browser__show_callchain(browser, &he->sorted_chain, 1, total,
> +					     hist_browser__fprintf_callchain_entry,
> +					     &arg);
> +		printed += arg.printed;
> +	}
>  
>  	return printed;
>  }
> -- 
> 2.0.0

Re: [PATCH v2 2/2] perf hists browser: Consolidate callchain print functions in TUI

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Thu, Aug 21, 2014 at 10:15:46AM +0900, Namhyung Kim escreveu:
> > Currently there're two callchain print functions in TUI - one for the
> > hists browser and another for file dump.  They do almost same job so
> > it'd be better consolidate the codes.
> 
> Some comments below, thanks for doing this work!
>  
> > To do that, move row calculation code into a print callback so that
> > the dump code cannot be limited by the current screen size.
>  
> > Cc: Frederic Weisbecker <fweisbec@gmail.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/ui/browsers/hists.c | 210 +++++++++++++++--------------------------
> >  1 file changed, 76 insertions(+), 134 deletions(-)
> > 
> > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> > index 519353d9f5fb..48d8c8eee6c2 100644
> > --- a/tools/perf/ui/browsers/hists.c
> > +++ b/tools/perf/ui/browsers/hists.c
> > @@ -477,20 +477,32 @@ static char *callchain_list__sym_name(struct callchain_list *cl,
> >  	return bf;
> >  }
> >  
> > +struct callchain_print_arg {
> > +	/* for hists browser */
> > +	unsigned short row;
> > +	off_t row_offset;
> > +	bool is_current_entry;
> > +
> > +	/* for file dump */
> > +	FILE *fp;
> > +	int printed;
> > +};

Just a data type definition nitpicking pet peeve of mine, don't 
you guys too find this vertically aligned form infinitely more 
readable:

struct callchain_print_arg {
	/* for hists browser */
	unsigned short	row;
	off_t		row_offset;
	bool		is_current_entry;

	/* for file dump */
	FILE		*fp;
	int		printed;
};

especially when looking at it via email, without syntax 
highlighting?

Thanks,

	Ingo

Re: [PATCHSET 0/2] perf hists browser: Cleanup callchain routines (v2)

[Ingo Molnar]

* Jiri Olsa <jolsa@redhat.com> wrote:

> On Thu, Aug 21, 2014 at 10:15:44AM +0900, Namhyung Kim wrote:
> > Hello,
> > 
> > This patch fixes and cleans up TUI callchain routines.  I tried to
> > consolidate similar functions but not to break the current output with
> > this change.  Hope that it makes code more readable and maintainable.
> 
> 76 insertions(+), 134 deletions(-) ... nice! ;-)
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>

Indeed, very nice!

	Ingo

Re: [PATCH v2 2/2] perf hists browser: Consolidate callchain print functions in TUI

[Arnaldo Carvalho de Melo]

Em Fri, Aug 22, 2014 at 10:02:36AM +0200, Ingo Molnar escreveu:
> > > +struct callchain_print_arg {
> > > +	/* for hists browser */
> > > +	unsigned short row;
> > > +	off_t row_offset;
> > > +	bool is_current_entry;
> > > +
> > > +	/* for file dump */
> > > +	FILE *fp;
> > > +	int printed;
> > > +};
> 
> Just a data type definition nitpicking pet peeve of mine, don't 
> you guys too find this vertically aligned form infinitely more 
> readable:
> 
> struct callchain_print_arg {
> 	/* for hists browser */
> 	unsigned short	row;
> 	off_t		row_offset;
> 	bool		is_current_entry;
> 
> 	/* for file dump */
> 	FILE		*fp;
> 	int		printed;
> };
> 
> especially when looking at it via email, without syntax 
> highlighting?

Agreed, will make it so when applying it.

- Arnaldo

[tip:perf/core] perf hists browser: Cleanup callchain print functions

[tip-bot for Namhyung Kim]

Commit-ID:  c09a7e755c025558cb882fa20a5f30da738536fa
Gitweb:     http://git.kernel.org/tip/c09a7e755c025558cb882fa20a5f30da738536fa
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Thu, 21 Aug 2014 10:15:45 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 22 Aug 2014 13:12:12 -0300

perf hists browser: Cleanup callchain print functions

The hist_browser__show_callchain() and friends don't need to be that
complex.  They're splitted in 3 pieces - one for traversing top-level
tree, other one for special casing first chains in the top-level
entries, and last one for recursive traversing inner trees.  It led to
code duplication and unnecessary complexity IMHO.

Simplify the function and consolidate the logic into a single function
- it can recursively call itself.  A little difference in printing
  callchains in top-level tree can be handled with a small change.

It should have no functional change.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1408583746-5540-2-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 112 +++++++++++------------------------------
 1 file changed, 29 insertions(+), 83 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index d42d8a8..519353d 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -502,23 +502,16 @@ static void hist_browser__show_callchain_entry(struct hist_browser *browser,
 
 #define LEVEL_OFFSET_STEP 3
 
-static int hist_browser__show_callchain_node_rb_tree(struct hist_browser *browser,
-						     struct callchain_node *chain_node,
-						     u64 total, int level,
-						     unsigned short row,
-						     off_t *row_offset,
-						     bool *is_current_entry)
+static int hist_browser__show_callchain(struct hist_browser *browser,
+					struct rb_root *root, int level,
+					unsigned short row, off_t *row_offset,
+					u64 total, bool *is_current_entry)
 {
 	struct rb_node *node;
 	int first_row = row, offset = level * LEVEL_OFFSET_STEP;
 	u64 new_total;
 
-	if (callchain_param.mode == CHAIN_GRAPH_REL)
-		new_total = chain_node->children_hit;
-	else
-		new_total = total;
-
-	node = rb_first(&chain_node->rb_root);
+	node = rb_first(root);
 	while (node) {
 		struct callchain_node *child = rb_entry(node, struct callchain_node, rb_node);
 		struct rb_node *next = rb_next(node);
@@ -535,7 +528,7 @@ static int hist_browser__show_callchain_node_rb_tree(struct hist_browser *browse
 
 			if (first)
 				first = false;
-			else
+			else if (level > 1)
 				extra_offset = LEVEL_OFFSET_STEP;
 
 			folded_sign = callchain_list__folded(chain);
@@ -547,8 +540,9 @@ static int hist_browser__show_callchain_node_rb_tree(struct hist_browser *browse
 			alloc_str = NULL;
 			str = callchain_list__sym_name(chain, bf, sizeof(bf),
 						       browser->show_dso);
-			if (was_first) {
-				double percent = cumul * 100.0 / new_total;
+
+			if (was_first && level > 1) {
+				double percent = cumul * 100.0 / total;
 
 				if (asprintf(&alloc_str, "%2.2f%% %s", percent, str) < 0)
 					str = "Not enough memory!";
@@ -571,78 +565,23 @@ do_next:
 
 		if (folded_sign == '-') {
 			const int new_level = level + (extra_offset ? 2 : 1);
-			row += hist_browser__show_callchain_node_rb_tree(browser, child, new_total,
-									 new_level, row, row_offset,
-									 is_current_entry);
-		}
-		if (row == browser->b.rows)
-			goto out;
-		node = next;
-	}
-out:
-	return row - first_row;
-}
-
-static int hist_browser__show_callchain_node(struct hist_browser *browser,
-					     struct callchain_node *node,
-					     int level, unsigned short row,
-					     off_t *row_offset,
-					     bool *is_current_entry)
-{
-	struct callchain_list *chain;
-	int first_row = row;
-	int offset = level * LEVEL_OFFSET_STEP;
-	char folded_sign = ' ';
-
-	list_for_each_entry(chain, &node->val, list) {
-		char bf[1024], *s;
 
-		folded_sign = callchain_list__folded(chain);
+			if (callchain_param.mode == CHAIN_GRAPH_REL)
+				new_total = child->children_hit;
+			else
+				new_total = total;
 
-		if (*row_offset != 0) {
-			--*row_offset;
-			continue;
+			row += hist_browser__show_callchain(browser, &child->rb_root,
+							    new_level,
+							    row, row_offset,
+							    new_total,
+							    is_current_entry);
 		}
-
-		s = callchain_list__sym_name(chain, bf, sizeof(bf),
-					     browser->show_dso);
-		hist_browser__show_callchain_entry(browser, chain, row,
-						   offset, folded_sign, s,
-						   is_current_entry);
-
-		if (++row == browser->b.rows)
-			goto out;
-	}
-
-	if (folded_sign == '-')
-		row += hist_browser__show_callchain_node_rb_tree(browser, node,
-								 browser->hists->stats.total_period,
-								 level + 1, row,
-								 row_offset,
-								 is_current_entry);
-out:
-	return row - first_row;
-}
-
-static int hist_browser__show_callchain(struct hist_browser *browser,
-					struct rb_root *chain,
-					int level, unsigned short row,
-					off_t *row_offset,
-					bool *is_current_entry)
-{
-	struct rb_node *nd;
-	int first_row = row;
-
-	for (nd = rb_first(chain); nd; nd = rb_next(nd)) {
-		struct callchain_node *node = rb_entry(nd, struct callchain_node, rb_node);
-
-		row += hist_browser__show_callchain_node(browser, node, level,
-							 row, row_offset,
-							 is_current_entry);
 		if (row == browser->b.rows)
 			break;
+		node = next;
 	}
-
+out:
 	return row - first_row;
 }
 
@@ -817,9 +756,16 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 		--row_offset;
 
 	if (folded_sign == '-' && row != browser->b.rows) {
-		printed += hist_browser__show_callchain(browser, &entry->sorted_chain,
+		u64 total = hists__total_period(entry->hists);
+
+		if (symbol_conf.cumulate_callchain)
+			total = entry->stat_acc->period;
+
+		printed += hist_browser__show_callchain(browser,
+							&entry->sorted_chain,
 							1, row, &row_offset,
-							&current_entry);
+							total, &current_entry);
+
 		if (current_entry)
 			browser->he_selection = entry;
 	}