Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1350271 > unrolled thread

[PATCH v2 3/8] perf tools: Use own hpp_list for hierarchy mode

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 3/8] perf tools: Use own hpp_list for hierarchy mode Namhyung Kim <namhyung@kernel.org> - 2016-03-04 16:10 +0100
    Re: [PATCH v2 3/8] perf tools: Use own hpp_list for hierarchy mode Jiri Olsa <jolsa@redhat.com> - 2016-03-05 19:20 +0100
      Re: [PATCH v2 3/8] perf tools: Use own hpp_list for hierarchy mode Namhyung Kim <namhyung@kernel.org> - 2016-03-07 13:50 +0100

#1350271 — [PATCH v2 3/8] perf tools: Use own hpp_list for hierarchy mode

Now each hists has its own hpp lists in hierarchy.  So instead of having
a pointer to a single perf_hpp_fmt in a hist entry, make it point the
hpp_list for its level.  This will be used to support multiple sort keys
in a single hierarchy level.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 45 +++++++++++++++------------
 tools/perf/ui/gtk/hists.c      | 20 ++++++++----
 tools/perf/ui/stdio/hist.c     | 44 +++++++++++++--------------
 tools/perf/util/hist.c         | 69 +++++++++++++++++++++++++++---------------
 tools/perf/util/sort.h         |  1 +
 5 files changed, 107 insertions(+), 72 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 5ffffcb1e3c5..928b4825b752 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1388,25 +1388,26 @@ static int hist_browser__show_hierarchy_entry(struct hist_browser *browser,
 					      HE_COLORSET_NORMAL);
 		}
 
-		ui_browser__write_nstring(&browser->b, "", 2);
-		width -= 2;
+		perf_hpp_list__for_each_format(entry->hpp_list, fmt) {
+			ui_browser__write_nstring(&browser->b, "", 2);
+			width -= 2;
 
-		/*
-		 * No need to call hist_entry__snprintf_alignment()
-		 * since this fmt is always the last column in the
-		 * hierarchy mode.
-		 */
-		fmt = entry->fmt;
-		if (fmt->color) {
-			width -= fmt->color(fmt, &hpp, entry);
-		} else {
-			int i = 0;
+			/*
+			 * No need to call hist_entry__snprintf_alignment()
+			 * since this fmt is always the last column in the
+			 * hierarchy mode.
+			 */
+			if (fmt->color) {
+				width -= fmt->color(fmt, &hpp, entry);
+			} else {
+				int i = 0;
 
-			width -= fmt->entry(fmt, &hpp, entry);
-			ui_browser__printf(&browser->b, "%s", ltrim(s));
+				width -= fmt->entry(fmt, &hpp, entry);
+				ui_browser__printf(&browser->b, "%s", ltrim(s));
 
-			while (isspace(s[i++]))
-				width++;
+				while (isspace(s[i++]))
+					width++;
+			}
 		}
 	}
 
@@ -1934,7 +1935,7 @@ static int hist_browser__fprintf_hierarchy_entry(struct hist_browser *browser,
 	struct perf_hpp_fmt *fmt;
 	bool first = true;
 	int ret;
-	int hierarchy_indent = (nr_sort_keys + 1) * HIERARCHY_INDENT;
+	int hierarchy_indent = nr_sort_keys * HIERARCHY_INDENT;
 
 	printed = fprintf(fp, "%*s", level * HIERARCHY_INDENT, "");
 
@@ -1962,9 +1963,13 @@ static int hist_browser__fprintf_hierarchy_entry(struct hist_browser *browser,
 	ret = scnprintf(hpp.buf, hpp.size, "%*s", hierarchy_indent, "");
 	advance_hpp(&hpp, ret);
 
-	fmt = he->fmt;
-	ret = fmt->entry(fmt, &hpp, he);
-	advance_hpp(&hpp, ret);
+	perf_hpp_list__for_each_format(he->hpp_list, fmt) {
+		ret = scnprintf(hpp.buf, hpp.size, "  ");
+		advance_hpp(&hpp, ret);
+
+		ret = fmt->entry(fmt, &hpp, he);
+		advance_hpp(&hpp, ret);
+	}
 
 	printed += fprintf(fp, "%s\n", rtrim(s));
 
diff --git a/tools/perf/ui/gtk/hists.c b/tools/perf/ui/gtk/hists.c
index a5758fdfbe1f..4534e2d7669c 100644
--- a/tools/perf/ui/gtk/hists.c
+++ b/tools/perf/ui/gtk/hists.c
@@ -412,6 +412,7 @@ static void perf_gtk__add_hierarchy_entries(struct hists *hists,
 	for (node = rb_first(root); node; node = rb_next(node)) {
 		GtkTreeIter iter;
 		float percent;
+		char *bf;
 
 		he = rb_entry(node, struct hist_entry, rb_node);
 		if (he->filtered)
@@ -437,13 +438,20 @@ static void perf_gtk__add_hierarchy_entries(struct hists *hists,
 			gtk_tree_store_set(store, &iter, col_idx++, hpp->buf, -1);
 		}
 
-		fmt = he->fmt;
-		if (fmt->color)
-			fmt->color(fmt, hpp, he);
-		else
-			fmt->entry(fmt, hpp, he);
+		bf = hpp->buf;
+		perf_hpp_list__for_each_format(he->hpp_list, fmt) {
+			int ret;
+
+			if (fmt->color)
+				ret = fmt->color(fmt, hpp, he);
+			else
+				ret = fmt->entry(fmt, hpp, he);
+
+			snprintf(hpp->buf + ret, hpp->size - ret, "  ");
+			advance_hpp(hpp, ret + 2);
+		}
 
-		gtk_tree_store_set(store, &iter, col_idx, rtrim(hpp->buf), -1);
+		gtk_tree_store_set(store, &iter, col_idx, rtrim(bf), -1);
 
 		if (!he->leaf) {
 			perf_gtk__add_hierarchy_entries(hists, &he->hroot_out,
diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index 6d06fbb365b6..073642a63cc9 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -451,33 +451,33 @@ static int hist_entry__hierarchy_fprintf(struct hist_entry *he,
 		advance_hpp(hpp, ret);
 	}
 
-	if (sep)
-		ret = scnprintf(hpp->buf, hpp->size, "%s", sep);
-	else
+	if (!sep)
 		ret = scnprintf(hpp->buf, hpp->size, "%*s",
-				(nr_sort_key - 1) * HIERARCHY_INDENT + 2, "");
+				(nr_sort_key - 1) * HIERARCHY_INDENT, "");
 	advance_hpp(hpp, ret);
 
 	printed += fprintf(fp, "%s", buf);
 
-	hpp->buf  = buf;
-	hpp->size = size;
-
-	/*
-	 * No need to call hist_entry__snprintf_alignment() since this
-	 * fmt is always the last column in the hierarchy mode.
-	 */
-	fmt = he->fmt;
-	if (perf_hpp__use_color() && fmt->color)
-		fmt->color(fmt, hpp, he);
-	else
-		fmt->entry(fmt, hpp, he);
-
-	/*
-	 * dynamic entries are right-aligned but we want left-aligned
-	 * in the hierarchy mode
-	 */
-	printed += fprintf(fp, "%s\n", ltrim(buf));
+	perf_hpp_list__for_each_format(he->hpp_list, fmt) {
+		hpp->buf  = buf;
+		hpp->size = size;
+
+		/*
+		 * No need to call hist_entry__snprintf_alignment() since this
+		 * fmt is always the last column in the hierarchy mode.
+		 */
+		if (perf_hpp__use_color() && fmt->color)
+			fmt->color(fmt, hpp, he);
+		else
+			fmt->entry(fmt, hpp, he);
+
+		/*
+		 * dynamic entries are right-aligned but we want left-aligned
+		 * in the hierarchy mode
+		 */
+		printed += fprintf(fp, "%s%s", sep ?: "  ", ltrim(buf));
+	}
+	printed += putc('\n', fp);
 
 	if (symbol_conf.use_callchain && he->leaf) {
 		u64 total = hists__total_period(hists);
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 83e98f420fe7..ca24e0cbdb4a 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1091,18 +1091,25 @@ static void hists__apply_filters(struct hists *hists, struct hist_entry *he);
 static struct hist_entry *hierarchy_insert_entry(struct hists *hists,
 						 struct rb_root *root,
 						 struct hist_entry *he,
-						 struct perf_hpp_fmt *fmt)
+						 struct perf_hpp_list *hpp_list)
 {
 	struct rb_node **p = &root->rb_node;
 	struct rb_node *parent = NULL;
 	struct hist_entry *iter, *new;
+	struct perf_hpp_fmt *fmt;
 	int64_t cmp;
 
 	while (*p != NULL) {
 		parent = *p;
 		iter = rb_entry(parent, struct hist_entry, rb_node_in);
 
-		cmp = fmt->collapse(fmt, iter, he);
+		cmp = 0;
+		perf_hpp_list__for_each_sort_list(hpp_list, fmt) {
+			cmp = fmt->collapse(fmt, iter, he);
+			if (cmp)
+				break;
+		}
+
 		if (!cmp) {
 			he_stat__add_stat(&iter->stat, &he->stat);
 			return iter;
@@ -1121,24 +1128,26 @@ static struct hist_entry *hierarchy_insert_entry(struct hists *hists,
 	hists__apply_filters(hists, new);
 	hists->nr_entries++;
 
-	/* save related format for output */
-	new->fmt = fmt;
+	/* save related format list for output */
+	new->hpp_list = hpp_list;
 
 	/* some fields are now passed to 'new' */
-	if (perf_hpp__is_trace_entry(fmt) || perf_hpp__is_dynamic_entry(fmt))
-		he->trace_output = NULL;
-	else
-		new->trace_output = NULL;
+	perf_hpp_list__for_each_sort_list(hpp_list, fmt) {
+		if (perf_hpp__is_trace_entry(fmt) || perf_hpp__is_dynamic_entry(fmt))
+			he->trace_output = NULL;
+		else
+			new->trace_output = NULL;
 
-	if (perf_hpp__is_srcline_entry(fmt))
-		he->srcline = NULL;
-	else
-		new->srcline = NULL;
+		if (perf_hpp__is_srcline_entry(fmt))
+			he->srcline = NULL;
+		else
+			new->srcline = NULL;
 
-	if (perf_hpp__is_srcfile_entry(fmt))
-		he->srcfile = NULL;
-	else
-		new->srcfile = NULL;
+		if (perf_hpp__is_srcfile_entry(fmt))
+			he->srcfile = NULL;
+		else
+			new->srcfile = NULL;
+	}
 
 	rb_link_node(&new->rb_node_in, parent, p);
 	rb_insert_color(&new->rb_node_in, root);
@@ -1150,20 +1159,29 @@ static int hists__hierarchy_insert_entry(struct hists *hists,
 					 struct hist_entry *he)
 {
 	struct perf_hpp_fmt *fmt;
+	struct perf_hpp_list_node *node;
 	struct hist_entry *new_he = NULL;
 	struct hist_entry *parent = NULL;
 	int depth = 0;
 	int ret = 0;
 
-	hists__for_each_sort_list(hists, fmt) {
-		if (!perf_hpp__is_sort_entry(fmt) &&
-		    !perf_hpp__is_dynamic_entry(fmt))
-			continue;
-		if (perf_hpp__should_skip(fmt, hists))
+	list_for_each_entry(node, &hists->hpp_formats, list) {
+		bool skip = false;
+
+		perf_hpp_list__for_each_sort_list(&node->hpp, fmt) {
+			if (!perf_hpp__is_sort_entry(fmt) &&
+			    !perf_hpp__is_dynamic_entry(fmt))
+				skip = true;
+			if (perf_hpp__should_skip(fmt, hists))
+				skip = true;
+			if (skip)
+				break;
+		}
+		if (skip)
 			continue;
 
 		/* insert copy of 'he' for each fmt into the hierarchy */
-		new_he = hierarchy_insert_entry(hists, root, he, fmt);
+		new_he = hierarchy_insert_entry(hists, root, he, &node->hpp);
 		if (new_he == NULL) {
 			ret = -1;
 			break;
@@ -1358,6 +1376,7 @@ static void hierarchy_insert_output_entry(struct rb_root *root,
 	struct rb_node **p = &root->rb_node;
 	struct rb_node *parent = NULL;
 	struct hist_entry *iter;
+	struct perf_hpp_fmt *fmt;
 
 	while (*p != NULL) {
 		parent = *p;
@@ -1373,8 +1392,10 @@ static void hierarchy_insert_output_entry(struct rb_root *root,
 	rb_insert_color(&he->rb_node, root);
 
 	/* update column width of dynamic entry */
-	if (perf_hpp__is_dynamic_entry(he->fmt))
-		he->fmt->sort(he->fmt, he, NULL);
+	perf_hpp_list__for_each_sort_list(he->hpp_list, fmt) {
+		if (perf_hpp__is_dynamic_entry(fmt))
+			fmt->sort(fmt, he, NULL);
+	}
 }
 
 static void hists__hierarchy_output_resort(struct hists *hists,
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 25a5529a94e4..ea1f722cffea 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -130,6 +130,7 @@ struct hist_entry {
 	u32			raw_size;
 	void			*trace_output;
 	struct perf_hpp_fmt	*fmt;
+	struct perf_hpp_list	*hpp_list;
 	struct hist_entry	*parent_he;
 	union {
 		/* this is for hierarchical entry structure */
-- 
2.7.2

[toc] | [next] | [standalone]

#1350957

On Fri, Mar 04, 2016 at 11:59:37PM +0900, Namhyung Kim wrote:

SNIP

> @@ -1150,20 +1159,29 @@ static int hists__hierarchy_insert_entry(struct hists *hists,
>  					 struct hist_entry *he)
>  {
>  	struct perf_hpp_fmt *fmt;
> +	struct perf_hpp_list_node *node;
>  	struct hist_entry *new_he = NULL;
>  	struct hist_entry *parent = NULL;
>  	int depth = 0;
>  	int ret = 0;
>  
> -	hists__for_each_sort_list(hists, fmt) {
> -		if (!perf_hpp__is_sort_entry(fmt) &&
> -		    !perf_hpp__is_dynamic_entry(fmt))
> -			continue;
> -		if (perf_hpp__should_skip(fmt, hists))
> +	list_for_each_entry(node, &hists->hpp_formats, list) {
> +		bool skip = false;
> +
> +		perf_hpp_list__for_each_sort_list(&node->hpp, fmt) {
> +			if (!perf_hpp__is_sort_entry(fmt) &&
> +			    !perf_hpp__is_dynamic_entry(fmt))
> +				skip = true;
> +			if (perf_hpp__should_skip(fmt, hists))
> +				skip = true;
> +			if (skip)
> +				break;

could we add skip bool into node and initialize it gradually in add_hierarchy_fmt?

jirka

[toc] | [prev] | [next] | [standalone]

#1351581

Hi Jiri,

On Sat, Mar 05, 2016 at 07:17:22PM +0100, Jiri Olsa wrote:
> On Fri, Mar 04, 2016 at 11:59:37PM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> > @@ -1150,20 +1159,29 @@ static int hists__hierarchy_insert_entry(struct hists *hists,
> >  					 struct hist_entry *he)
> >  {
> >  	struct perf_hpp_fmt *fmt;
> > +	struct perf_hpp_list_node *node;
> >  	struct hist_entry *new_he = NULL;
> >  	struct hist_entry *parent = NULL;
> >  	int depth = 0;
> >  	int ret = 0;
> >  
> > -	hists__for_each_sort_list(hists, fmt) {
> > -		if (!perf_hpp__is_sort_entry(fmt) &&
> > -		    !perf_hpp__is_dynamic_entry(fmt))
> > -			continue;
> > -		if (perf_hpp__should_skip(fmt, hists))
> > +	list_for_each_entry(node, &hists->hpp_formats, list) {
> > +		bool skip = false;
> > +
> > +		perf_hpp_list__for_each_sort_list(&node->hpp, fmt) {
> > +			if (!perf_hpp__is_sort_entry(fmt) &&
> > +			    !perf_hpp__is_dynamic_entry(fmt))
> > +				skip = true;
> > +			if (perf_hpp__should_skip(fmt, hists))
> > +				skip = true;
> > +			if (skip)
> > +				break;
> 
> could we add skip bool into node and initialize it gradually in add_hierarchy_fmt?

Right.

We can simply skip the node if all fmts it contains should be skipped.

Thanks,
Namhyung

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web