Skip to contentSkip to conversation

perf hists: Changes on hists__{match,link}

PATCH In review Namhyung Kim 3 participants 12 messages Dec 5, 2012 – Jan 24, 2013 perf events

lore
Share & tools
lore

Namhyung Kim

author maintainer
#0 lore raw

v2 cover perf hists: Changes on hists__{match,link}

Hi Arnaldo,

I exchanged arguments of hist_entry__cmp() and friends as you requested. You can get this on my perf/link-v2 branch at

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

Thanks,
Namhyung

Namhyung Kim (3):
perf hists: Exchange order of comparing items when collapsing hists perf hists: Link hist entries before inserting to an output tree perf diff: Use internal rb tree for compute resort

 tools/perf/builtin-diff.c | 95 +++++++++++++++++++++--------------------------
 tools/perf/util/hist.c    | 51 ++++++++++++++++++-------
 2 files changed, 80 insertions(+), 66 deletions(-)
Signature · 1 line
1.7.11.7
author maintainer
#1 lore raw

PATCH v2 1/3 perf hists: Exchange order of comparing items when collapsing hists

From: Namhyung Kim <[email protected]>

When comparing entries for collapsing put the given entry first, and then the iterated entry. This is not the case of hist_entry__cmp() when called if given sort keys don't require collapsing. So change the order for the sake of consistency. It will be required for matching and/or linking multiple hist entries.

Cc Jiri Olsa <[email protected]>
Cc Stephane Eranian <[email protected]>
Signed-off-by Namhyung Kim <[email protected]>
---
 tools/perf/util/hist.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
tools/perf/util/hist.c +3 -3
index 82df1b26f0d4..d4471c21ed17 100644
@@ -285,7 +285,7 @@ static struct hist_entry *add_hist_entry(struct hists *hists,
 		parent = *p;
 		he = rb_entry(parent, struct hist_entry, rb_node_in);
 
-		cmp = hist_entry__cmp(entry, he);
+		cmp = hist_entry__cmp(he, entry);
 
 		if (!cmp) {
 			he_stat__add_period(&he->stat, period);

@@ -729,7 +729,7 @@ static struct hist_entry *hists__add_dummy_entry(struct hists *hists,
 		parent = *p;
 		he = rb_entry(parent, struct hist_entry, rb_node);
 
-		cmp = hist_entry__cmp(pair, he);
+		cmp = hist_entry__cmp(he, pair);
 
 		if (!cmp)
 			goto out;

@@ -759,7 +759,7 @@ static struct hist_entry *hists__find_entry(struct hists *hists,
 
 	while (n) {
 		struct hist_entry *iter = rb_entry(n, struct hist_entry, rb_node);
-		int64_t cmp = hist_entry__cmp(he, iter);
+		int64_t cmp = hist_entry__cmp(iter, he);
 
 		if (cmp < 0)
 			n = n->rb_left;
Signature · 1 line
1.7.11.7
author maintainer
#2 lore raw

PATCH v2 2/3 perf hists: Link hist entries before inserting to an output tree

In reply to Namhyung Kim

From: Namhyung Kim <[email protected]>

For matching and/or linking hist entries, they need to be sorted by given sort keys. However current hists__match/link did this on the output trees, so that the entries in the output tree need to be resort before doing it.

This looks not so good since we have trees for collecting or collapsing entries before passing them to an output tree and they're already sorted by the given sort keys. Since we don't need to print anything at the time of matching/linking, we can use these internal trees directly instead of bothering with double resort on the output tree.

Its only user - at the time of this writing - perf diff can be easily converted to use the internal tree and can save some lines too by getting rid of unnecessary resorting codes.

It seems we need to do output resort on baseline hists for displacement prior to the match/link. But AFAIK it's okay since the resort doesn't make any change to the internal tree - i.e. every entries in the internal tree remain as is and just linked to both of internal and output tree using different keys. So linking entries in the internal tree after output resorting will not cause any harm IMHO.

Cc Jiri Olsa <[email protected]>
Cc Stephane Eranian <[email protected]>
Signed-off-by Namhyung Kim <[email protected]>
---
 tools/perf/builtin-diff.c | 73 ++++++++++++++++++-----------------------------
 tools/perf/util/hist.c    | 49 +++++++++++++++++++++++--------
 2 files changed, 65 insertions(+), 57 deletions(-)
tools/perf/builtin-diff.c +28 -45
index d869029fb75e..b52f5d8c4a6b 100644
@@ -276,46 +276,19 @@ static struct perf_tool tool = {
 	.ordering_requires_timestamps = true,
 };
 
-static void insert_hist_entry_by_name(struct rb_root *root,
-				      struct hist_entry *he)
-{
-	struct rb_node **p = &root->rb_node;
-	struct rb_node *parent = NULL;
-	struct hist_entry *iter;
-
-	while (*p != NULL) {
-		parent = *p;
-		iter = rb_entry(parent, struct hist_entry, rb_node);
-		if (hist_entry__cmp(he, iter) < 0)
-			p = &(*p)->rb_left;
-		else
-			p = &(*p)->rb_right;
-	}
-
-	rb_link_node(&he->rb_node, parent, p);
-	rb_insert_color(&he->rb_node, root);
-}
-
-static void hists__name_resort(struct hists *self, bool sort)
+static void hists__compute_position(struct hists *hists)
 {
 	unsigned long position = 1;
-	struct rb_root tmp = RB_ROOT;
-	struct rb_node *next = rb_first(&self->entries);
+	struct rb_node *next = rb_first(&hists->entries);
 
 	while (next != NULL) {
-		struct hist_entry *n = rb_entry(next, struct hist_entry, rb_node);
+		struct hist_entry *he;
 
-		next = rb_next(&n->rb_node);
-		n->position = position++;
+		he = rb_entry(next, struct hist_entry, rb_node);
+		he->position = position++;
 
-		if (sort) {
-			rb_erase(&n->rb_node, &self->entries);
-			insert_hist_entry_by_name(&tmp, n);
-		}
+		next = rb_next(next);
 	}
-
-	if (sort)
-		self->entries = tmp;
 }
 
 static struct perf_evsel *evsel_match(struct perf_evsel *evsel,

@@ -330,34 +303,39 @@ static struct perf_evsel *evsel_match(struct perf_evsel *evsel,
 	return NULL;
 }
 
-static void perf_evlist__resort_hists(struct perf_evlist *evlist, bool name)
+static void perf_evlist__resort_hists(struct perf_evlist *evlist, bool base)
 {
 	struct perf_evsel *evsel;
 
 	list_for_each_entry(evsel, &evlist->entries, node) {
 		struct hists *hists = &evsel->hists;
 
-		hists__output_resort(hists);
+		hists__collapse_resort(hists);
 
-		/*
-		 * The hists__name_resort only sets possition
-		 * if name is false.
-		 */
-		if (name || ((!name) && show_displacement))
-			hists__name_resort(hists, name);
+		if (base && show_displacement) {
+			hists__output_resort(hists);
+			hists__compute_position(hists);
+		}
 	}
 }
 
 static void hists__baseline_only(struct hists *hists)
 {
-	struct rb_node *next = rb_first(&hists->entries);
+	struct rb_root *root;
+	struct rb_node *next;
+
+	if (sort__need_collapse)
+		root = &hists->entries_collapsed;
+	else
+		root = hists->entries_in;
 
+	next = rb_first(root);
 	while (next != NULL) {
-		struct hist_entry *he = rb_entry(next, struct hist_entry, rb_node);
+		struct hist_entry *he = rb_entry(next, struct hist_entry, rb_node_in);
 
-		next = rb_next(&he->rb_node);
+		next = rb_next(&he->rb_node_in);
 		if (!hist_entry__next_pair(he)) {
-			rb_erase(&he->rb_node, &hists->entries);
+			rb_erase(&he->rb_node_in, root);
 			hist_entry__free(he);
 		}
 	}

@@ -481,6 +459,11 @@ static void hists__process(struct hists *old, struct hists *new)
 	else
 		hists__link(new, old);
 
+	hists__output_resort(new);
+
+	if (show_displacement)
+		hists__compute_position(new);
+
 	if (sort_compute) {
 		hists__precompute(new);
 		hists__compute_resort(new);
tools/perf/util/hist.c +37 -12

94 lines not on this page · raw message

Signature · 1 line
1.7.11.7
#3 lore raw

Re: [PATCH 2/3] perf hists: Link hist entries before inserting to an output tree

Quoted 2 lines · Namhyung Kim

From: Namhyung Kim <[email protected]>

SNIP

Quoted 23 lines
tools/perf/builtin-diff.c +11 -4
-	struct rb_node *next = rb_first(&hists->entries);
+	struct rb_root *root;
+	struct rb_node *next;
+
+	if (sort__need_collapse)
+		root = &hists->entries_collapsed;
+	else
+		root = hists->entries_in;
 
+	next = rb_first(root);
 	while (next != NULL) {
-		struct hist_entry *he = rb_entry(next, struct hist_entry, rb_node);
+		struct hist_entry *he = rb_entry(next, struct hist_entry, rb_node_in);
 
-		next = rb_next(&he->rb_node);
+		next = rb_next(&he->rb_node_in);
 		if (!hist_entry__next_pair(he)) {
-			rb_erase(&he->rb_node, &hists->entries);
+			rb_erase(&he->rb_node_in, root);
 			hist_entry__free(he);
 		}
 	}

tools/perf/builtin-diff.c static void hists__process(struct hists *old, struct hists *new) · line 466

Quoted 8 lines
tools/perf/builtin-diff.c +5 -0
@@ -481,6 +459,11 @@ static void hists__process(struct hists *old, struct hists *new)
 	else
 		hists__link(new, old);
 
+	hists__output_resort(new);
+
+	if (show_displacement)
+		hists__compute_position(new);
+

Computing the position after hists__link screws up the position data, because we likely have new entries in.

However, I wonder if anyone is actualy using displacement info..?

jirka

#4 lore raw

Re: [PATCH 2/3] perf hists: Link hist entries before inserting to an output tree

Em Wed, Dec 05, 2012 at 08:06:46PM +0100, Jiri Olsa escreveu:

Quoted 15 lines · Namhyung Kim

tools/perf/builtin-diff.c static void hists__process(struct hists *old, struct hists *new) · line 466

Quoted 10 lines

From: Namhyung Kim <[email protected]>

tools/perf/builtin-diff.c +5 -0
@@ -481,6 +459,11 @@ static void hists__process(struct hists *old, struct hists *new)
 	else
 		hists__link(new, old);
 
+	hists__output_resort(new);
+
+	if (show_displacement)
+		hists__compute_position(new);
+

Computing the position after hists__link screws up the position data, because we likely have new entries in.

Quoted 1 line

However, I wonder if anyone is actualy using displacement info..?

IIRC that was used long ago in the first version of 'perf diff', that is not the default, probably we can just ditch it to simplify things, can you check?

- Arnaldo

#5 lore raw

PATCH 1/1 perf diff: Removing displacement output option

Quoted 16 lines · Arnaldo Carvalho de Melo

Em Wed, Dec 05, 2012 at 08:06:46PM +0100, Jiri Olsa escreveu:

Quoted 15 lines · Namhyung Kim

Review static void hists__process(struct hists *old, struct hists *new) · line 466

Quoted 10 lines

From: Namhyung Kim <[email protected]>

(quoted diff) +5 -0
@@ -481,6 +459,11 @@ static void hists__process(struct hists *old, struct hists *new)
 	else
 		hists__link(new, old);
 
+	hists__output_resort(new);
+
+	if (show_displacement)
+		hists__compute_position(new);
+

Computing the position after hists__link screws up the position data, because we likely have new entries in.

Quoted 5 lines
Quoted 1 line

However, I wonder if anyone is actualy using displacement info..?

IIRC that was used long ago in the first version of 'perf diff', that is not the default, probably we can just ditch it to simplify things, can you check?

you're right, it's there since vert beggining.. and now it's gone ;) attached

jirka

---
Removing displacement output option. It seems not very useful, because it's possible and event more convenient to lookup related symbol by name. Also the output value for both 'baseline' and 'new' data is quite apparent from diff output.

And above all it complicates hist code factoring ;)

Ditching out PERF_HPP__DISPL column with related output functions.

Signed-off-by Jiri Olsa <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Corey Ashford <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Peter Zijlstra <[email protected]>
---
 tools/perf/Documentation/perf-diff.txt |  4 ----
 tools/perf/builtin-diff.c              | 29 +++++++----------------------
 tools/perf/ui/hist.c                   | 25 -------------------------
 tools/perf/util/hist.h                 |  1 -
 4 files changed, 7 insertions(+), 52 deletions(-)
tools/perf/Documentation/perf-diff.txt +0 -4
index 194f37d..5b3123d 100644
@@ -22,10 +22,6 @@ specified perf.data files.
 
 OPTIONS
 -------
--M::
---displacement::
-        Show position displacement relative to baseline.
-
 -D::
 --dump-raw-trace::
         Dump raw trace in ASCII.
tools/perf/builtin-diff.c +7 -22
index d869029..b2e7d39 100644
@@ -23,7 +23,6 @@ static char const *input_old = "perf.data.old",
 		  *input_new = "perf.data";
 static char	  diff__default_sort_order[] = "dso,symbol";
 static bool  force;
-static bool show_displacement;
 static bool show_period;
 static bool show_formula;
 static bool show_baseline_only;

@@ -296,9 +295,8 @@ static void insert_hist_entry_by_name(struct rb_root *root,
 	rb_insert_color(&he->rb_node, root);
 }
 
-static void hists__name_resort(struct hists *self, bool sort)
+static void hists__name_resort(struct hists *self)
 {
-	unsigned long position = 1;
 	struct rb_root tmp = RB_ROOT;
 	struct rb_node *next = rb_first(&self->entries);
 

@@ -306,16 +304,12 @@ static void hists__name_resort(struct hists *self, bool sort)
 		struct hist_entry *n = rb_entry(next, struct hist_entry, rb_node);
 
 		next = rb_next(&n->rb_node);
-		n->position = position++;
 
-		if (sort) {
-			rb_erase(&n->rb_node, &self->entries);
-			insert_hist_entry_by_name(&tmp, n);
-		}
+		rb_erase(&n->rb_node, &self->entries);
+		insert_hist_entry_by_name(&tmp, n);
 	}
 
-	if (sort)
-		self->entries = tmp;
+	self->entries = tmp;
 }
 
 static struct perf_evsel *evsel_match(struct perf_evsel *evsel,

@@ -339,12 +333,8 @@ static void perf_evlist__resort_hists(struct perf_evlist *evlist, bool name)
 
 		hists__output_resort(hists);
 
-		/*
-		 * The hists__name_resort only sets possition
-		 * if name is false.
-		 */
-		if (name || ((!name) && show_displacement))
-			hists__name_resort(hists, name);
+		if (name)
+			hists__name_resort(hists);
 	}
 }
 

@@ -549,8 +539,6 @@ static const char * const diff_usage[] = {
 static const struct option options[] = {
 	OPT_INCR('v', "verbose", &verbose,
 		    "be more verbose (show symbol address, etc)"),
-	OPT_BOOLEAN('M', "displacement", &show_displacement,
-		    "Show position displacement relative to baseline"),
 	OPT_BOOLEAN('b', "baseline-only", &show_baseline_only,
 		    "Show only items with match in baseline"),
 	OPT_CALLBACK('c', "compute", &compute,

@@ -585,7 +573,7 @@ static const struct option options[] = {
 static void ui_init(void)
 {
 	/*
-	 * Display baseline/delta/ratio/displacement/
+	 * Display baseline/delta/ratio
 	 * formula/periods columns.
 	 */
 	perf_hpp__column_enable(PERF_HPP__BASELINE);

@@ -604,9 +592,6 @@ static void ui_init(void)
 		BUG_ON(1);
 	};
 
-	if (show_displacement)
-		perf_hpp__column_enable(PERF_HPP__DISPL);
-
 	if (show_formula)
 		perf_hpp__column_enable(PERF_HPP__FORMULA);
tools/perf/ui/hist.c +0 -25
index 1785bab..1889c12 100644
@@ -351,30 +351,6 @@ static int hpp__entry_wdiff(struct perf_hpp *hpp, struct hist_entry *he)
 	return scnprintf(hpp->buf, hpp->size, fmt, buf);
 }
 
-static int hpp__header_displ(struct perf_hpp *hpp)
-{
-	return scnprintf(hpp->buf, hpp->size, "Displ.");
-}
-
-static int hpp__width_displ(struct perf_hpp *hpp __maybe_unused)
-{
-	return 6;
-}
-
-static int hpp__entry_displ(struct perf_hpp *hpp,
-			    struct hist_entry *he)
-{
-	struct hist_entry *pair = hist_entry__next_pair(he);
-	long displacement = pair ? pair->position - he->position : 0;
-	const char *fmt = symbol_conf.field_sep ? "%s" : "%6.6s";
-	char buf[32] = " ";
-
-	if (displacement)
-		scnprintf(buf, sizeof(buf), "%+4ld", displacement);
-
-	return scnprintf(hpp->buf, hpp->size, fmt, buf);
-}
-
 static int hpp__header_formula(struct perf_hpp *hpp)
 {
 	const char *fmt = symbol_conf.field_sep ? "%s" : "%70s";

@@ -427,7 +403,6 @@ struct perf_hpp_fmt perf_hpp__format[] = {
 	HPP__PRINT_FNS(delta),
 	HPP__PRINT_FNS(ratio),
 	HPP__PRINT_FNS(wdiff),
-	HPP__PRINT_FNS(displ),
 	HPP__PRINT_FNS(formula)
 };
tools/perf/util/hist.h +0 -1
index c1b2fad..5b3b007 100644
@@ -154,7 +154,6 @@ enum {
 	PERF_HPP__DELTA,
 	PERF_HPP__RATIO,
 	PERF_HPP__WEIGHTED_DIFF,
-	PERF_HPP__DISPL,
 	PERF_HPP__FORMULA,
 
 	PERF_HPP__MAX_INDEX
Signature · 1 line
1.7.11.7
author maintainer
#6 lore raw

Re: [PATCH] perf diff: Removing displacement output option

2012-12-06 (목), 14:22 +0100, Jiri Olsa:

Quoted 9 lines · Arnaldo Carvalho de Melo
Quoted 6 lines

Em Wed, Dec 05, 2012 at 08:06:46PM +0100, Jiri Olsa escreveu:

Quoted 1 line

However, I wonder if anyone is actualy using displacement info..?

IIRC that was used long ago in the first version of 'perf diff', that is not the default, probably we can just ditch it to simplify things, can you check?

you're right, it's there since vert beggining.. and now it's gone ;) attached

Good, I'll send v3 soon based on this.

Thanks,
Namhyung

#7 lore raw

[tip:perf/core] perf diff: Remove displacement output option

In reply to Jiri Olsa
Commit-ID:  db6d0bb86164497f6c9ef46020cf1881953f4b08
Gitweb:     http://git.kernel.org/tip/db6d0bb86164497f6c9ef46020cf1881953f4b08
Author:     Jiri Olsa <[email protected]>
AuthorDate: Thu, 6 Dec 2012 14:22:28 +0100
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Sun, 9 Dec 2012 08:46:07 -0300

perf diff: Remove displacement output option

It seems not very useful, because it's possible and event more convenient to lookup related symbol by name. Also the output value for both 'baseline' and 'new' data is quite apparent from diff output.

And above all it complicates hist code factoring ;)

Ditching out PERF_HPP__DISPL column with related output functions.

Suggested-by Arnaldo Carvalho de Melo <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
Cc Corey Ashford <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Stephane Eranian <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/Documentation/perf-diff.txt |  4 ----
 tools/perf/builtin-diff.c              | 29 +++++++----------------------
 tools/perf/ui/hist.c                   | 25 -------------------------
 tools/perf/util/hist.h                 |  1 -
 4 files changed, 7 insertions(+), 52 deletions(-)
tools/perf/Documentation/perf-diff.txt +0 -4
index 194f37d..5b3123d 100644
@@ -22,10 +22,6 @@ specified perf.data files.
 
 OPTIONS
 -------
--M::
---displacement::
-        Show position displacement relative to baseline.
-
 -D::
 --dump-raw-trace::
         Dump raw trace in ASCII.
tools/perf/builtin-diff.c +7 -22
index d869029..b2e7d39 100644
@@ -23,7 +23,6 @@ static char const *input_old = "perf.data.old",
 		  *input_new = "perf.data";
 static char	  diff__default_sort_order[] = "dso,symbol";
 static bool  force;
-static bool show_displacement;
 static bool show_period;
 static bool show_formula;
 static bool show_baseline_only;

@@ -296,9 +295,8 @@ static void insert_hist_entry_by_name(struct rb_root *root,
 	rb_insert_color(&he->rb_node, root);
 }
 
-static void hists__name_resort(struct hists *self, bool sort)
+static void hists__name_resort(struct hists *self)
 {
-	unsigned long position = 1;
 	struct rb_root tmp = RB_ROOT;
 	struct rb_node *next = rb_first(&self->entries);
 

@@ -306,16 +304,12 @@ static void hists__name_resort(struct hists *self, bool sort)
 		struct hist_entry *n = rb_entry(next, struct hist_entry, rb_node);
 
 		next = rb_next(&n->rb_node);
-		n->position = position++;
 
-		if (sort) {
-			rb_erase(&n->rb_node, &self->entries);
-			insert_hist_entry_by_name(&tmp, n);
-		}
+		rb_erase(&n->rb_node, &self->entries);
+		insert_hist_entry_by_name(&tmp, n);
 	}
 
-	if (sort)
-		self->entries = tmp;
+	self->entries = tmp;
 }
 
 static struct perf_evsel *evsel_match(struct perf_evsel *evsel,

@@ -339,12 +333,8 @@ static void perf_evlist__resort_hists(struct perf_evlist *evlist, bool name)
 
 		hists__output_resort(hists);
 
-		/*
-		 * The hists__name_resort only sets possition
-		 * if name is false.
-		 */
-		if (name || ((!name) && show_displacement))
-			hists__name_resort(hists, name);
+		if (name)
+			hists__name_resort(hists);
 	}
 }
 

@@ -549,8 +539,6 @@ static const char * const diff_usage[] = {
 static const struct option options[] = {
 	OPT_INCR('v', "verbose", &verbose,
 		    "be more verbose (show symbol address, etc)"),
-	OPT_BOOLEAN('M', "displacement", &show_displacement,
-		    "Show position displacement relative to baseline"),
 	OPT_BOOLEAN('b', "baseline-only", &show_baseline_only,
 		    "Show only items with match in baseline"),
 	OPT_CALLBACK('c', "compute", &compute,

@@ -585,7 +573,7 @@ static const struct option options[] = {
 static void ui_init(void)
 {
 	/*
-	 * Display baseline/delta/ratio/displacement/
+	 * Display baseline/delta/ratio
 	 * formula/periods columns.
 	 */
 	perf_hpp__column_enable(PERF_HPP__BASELINE);

@@ -604,9 +592,6 @@ static void ui_init(void)
 		BUG_ON(1);
 	};
 
-	if (show_displacement)
-		perf_hpp__column_enable(PERF_HPP__DISPL);
-
 	if (show_formula)
 		perf_hpp__column_enable(PERF_HPP__FORMULA);
tools/perf/ui/hist.c +0 -25
index 1785bab..1889c12 100644
@@ -351,30 +351,6 @@ static int hpp__entry_wdiff(struct perf_hpp *hpp, struct hist_entry *he)
 	return scnprintf(hpp->buf, hpp->size, fmt, buf);
 }
 
-static int hpp__header_displ(struct perf_hpp *hpp)
-{
-	return scnprintf(hpp->buf, hpp->size, "Displ.");
-}
-
-static int hpp__width_displ(struct perf_hpp *hpp __maybe_unused)
-{
-	return 6;
-}
-
-static int hpp__entry_displ(struct perf_hpp *hpp,
-			    struct hist_entry *he)
-{
-	struct hist_entry *pair = hist_entry__next_pair(he);
-	long displacement = pair ? pair->position - he->position : 0;
-	const char *fmt = symbol_conf.field_sep ? "%s" : "%6.6s";
-	char buf[32] = " ";
-
-	if (displacement)
-		scnprintf(buf, sizeof(buf), "%+4ld", displacement);
-
-	return scnprintf(hpp->buf, hpp->size, fmt, buf);
-}
-
 static int hpp__header_formula(struct perf_hpp *hpp)
 {
 	const char *fmt = symbol_conf.field_sep ? "%s" : "%70s";

@@ -427,7 +403,6 @@ struct perf_hpp_fmt perf_hpp__format[] = {
 	HPP__PRINT_FNS(delta),
 	HPP__PRINT_FNS(ratio),
 	HPP__PRINT_FNS(wdiff),
-	HPP__PRINT_FNS(displ),
 	HPP__PRINT_FNS(formula)
 };
tools/perf/util/hist.h +0 -1
index c1b2fad..5b3b007 100644
@@ -154,7 +154,6 @@ enum {
 	PERF_HPP__DELTA,
 	PERF_HPP__RATIO,
 	PERF_HPP__WEIGHTED_DIFF,
-	PERF_HPP__DISPL,
 	PERF_HPP__FORMULA,
 
 	PERF_HPP__MAX_INDEX
#8 lore raw

Re: [PATCH 2/3] perf hists: Link hist entries before inserting to an output tree

In reply to Jiri Olsa
Quoted 41 lines · Jiri Olsa
Quoted 2 lines · Namhyung Kim

From: Namhyung Kim <[email protected]>

SNIP

Quoted 23 lines
tools/perf/builtin-diff.c +11 -4
-	struct rb_node *next = rb_first(&hists->entries);
+	struct rb_root *root;
+	struct rb_node *next;
+
+	if (sort__need_collapse)
+		root = &hists->entries_collapsed;
+	else
+		root = hists->entries_in;
 
+	next = rb_first(root);
 	while (next != NULL) {
-		struct hist_entry *he = rb_entry(next, struct hist_entry, rb_node);
+		struct hist_entry *he = rb_entry(next, struct hist_entry, rb_node_in);
 
-		next = rb_next(&he->rb_node);
+		next = rb_next(&he->rb_node_in);
 		if (!hist_entry__next_pair(he)) {
-			rb_erase(&he->rb_node, &hists->entries);
+			rb_erase(&he->rb_node_in, root);
 			hist_entry__free(he);
 		}
 	}

tools/perf/builtin-diff.c static void hists__process(struct hists *old, struct hists *new) · line 466

Quoted 8 lines
tools/perf/builtin-diff.c +5 -0
@@ -481,6 +459,11 @@ static void hists__process(struct hists *old, struct hists *new)
 	else
 		hists__link(new, old);
 
+	hists__output_resort(new);
+
+	if (show_displacement)
+		hists__compute_position(new);
+

Computing the position after hists__link screws up the position data, because we likely have new entries in.

Quoted 1 line

However, I wonder if anyone is actualy using displacement info..?

hum,

the point of the displacement is to show how far is the matching entry in baseline wrt report output -> after hists__output_resort.. that goes in opposite way of what we try do to in here.

Anyone else in favour of removing 'Displ.' column? ;-)

jirka

author maintainer
#9 lore raw

Re: [PATCH 2/3] perf hists: Link hist entries before inserting to an output tree

Quoted 18 lines · Jiri Olsa
Quoted 15 lines · Jiri Olsa

tools/perf/builtin-diff.c static void hists__process(struct hists *old, struct hists *new) · line 466

Quoted 10 lines · Namhyung Kim

From: Namhyung Kim <[email protected]>

tools/perf/builtin-diff.c +5 -0
@@ -481,6 +459,11 @@ static void hists__process(struct hists *old, struct hists *new)
 	else
 		hists__link(new, old);
 
+	hists__output_resort(new);
+
+	if (show_displacement)
+		hists__compute_position(new);
+

Computing the position after hists__link screws up the position data, because we likely have new entries in.

Quoted 1 line

However, I wonder if anyone is actualy using displacement info..?

Quoted 7 lines

hum,

the point of the displacement is to show how far is the matching entry in baseline wrt report output -> after hists__output_resort.. that goes in opposite way of what we try do to in here.

Anyone else in favour of removing 'Displ.' column? ;-)

Oh, I somehow thought that the new dummy entries go into the baseline. It's your change in the multi-diff patchset, not current code, right?

So we can change either the baseline to have dummies or skipping dummies when computing their position. Of course I have no objection to get rid of the displacement logic completely too. :)

Thanks,
Namhyung

author maintainer
#10 lore raw

PATCH v2 3/3 perf diff: Use internal rb tree for compute resort

In reply to Namhyung Kim

From: Namhyung Kim <[email protected]>

There's no reason to run hists_compute_resort() using output tree. Convert it to use internal tree so that it can remove unnecessary _output_resort. Also move position computation below the resort since it changes the output ordering.

Cc Jiri Olsa <[email protected]>
Cc Stephane Eranian <[email protected]>
Signed-off-by Namhyung Kim <[email protected]>
---
 tools/perf/builtin-diff.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)
tools/perf/builtin-diff.c +19 -13
index b52f5d8c4a6b..4e18cea7c845 100644
@@ -435,19 +435,25 @@ static void insert_hist_entry_by_compute(struct rb_root *root,
 
 static void hists__compute_resort(struct hists *hists)
 {
-	struct rb_root tmp = RB_ROOT;
-	struct rb_node *next = rb_first(&hists->entries);
+	struct rb_root *root;
+	struct rb_node *next;
+
+	if (sort__need_collapse)
+		root = &hists->entries_collapsed;
+	else
+		root = hists->entries_in;
+
+	hists->entries = RB_ROOT;
+	next = rb_first(root);
 
 	while (next != NULL) {
-		struct hist_entry *he = rb_entry(next, struct hist_entry, rb_node);
+		struct hist_entry *he;
 
-		next = rb_next(&he->rb_node);
+		he = rb_entry(next, struct hist_entry, rb_node_in);
+		next = rb_next(&he->rb_node_in);
 
-		rb_erase(&he->rb_node, &hists->entries);
-		insert_hist_entry_by_compute(&tmp, he, compute);
+		insert_hist_entry_by_compute(&hists->entries, he, compute);
 	}
-
-	hists->entries = tmp;
 }
 
 static void hists__process(struct hists *old, struct hists *new)

@@ -459,16 +465,16 @@ static void hists__process(struct hists *old, struct hists *new)
 	else
 		hists__link(new, old);
 
-	hists__output_resort(new);
-
-	if (show_displacement)
-		hists__compute_position(new);
-
 	if (sort_compute) {
 		hists__precompute(new);
 		hists__compute_resort(new);
+	} else {
+		hists__output_resort(new);
 	}
 
+	if (show_displacement)
+		hists__compute_position(new);
+
 	hists__fprintf(new, true, 0, 0, stdout);
 }
Signature · 1 line
1.7.11.7
author maintainer
#11 lore raw

PATCH 1/1 perf test: Add a test case for hists__{match,link}

In reply to Namhyung Kim

As they are used from diff and event group report, add a test case to verify their behaviors.

In this test I made a fake machine and two evsel. Each evsel got 10 samples (so hist entries) - 5 are common and the rests are not. So after hists__match() both of them will have 5 entries with pair set.

And the second evsel has a collapsed entry so that the total number is 9 - I made it in order to simulate more realistic case. Thus after hists__link the first entry will have 14 entries - 5 are common (w/ pair), 5 are unmatch (w/o pair) and 4 are dummy (w/ pair). And the second entry will have 9 entries all have its pair.

Cc Jiri Olsa <[email protected]>
Cc Stephane Eranian <[email protected]>
Signed-off-by Namhyung Kim <[email protected]>
---
 tools/perf/Makefile             |   1 +
 tools/perf/tests/builtin-test.c |   4 +
 tools/perf/tests/hists_link.c   | 502 ++++++++++++++++++++++++++++++++++++++++
 tools/perf/tests/tests.h        |   1 +
 tools/perf/util/machine.h       |   1 +
 tools/perf/util/session.c       |   2 +-
 6 files changed, 510 insertions(+), 1 deletion(-)
 create mode 100644 tools/perf/tests/hists_link.c
tools/perf/Makefile +1 -0
index 75785bb98c30..3628065cda20 100644
@@ -460,6 +460,7 @@ LIB_OBJS += $(OUTPUT)tests/evsel-roundtrip-name.o
 LIB_OBJS += $(OUTPUT)tests/evsel-tp-sched.o
 LIB_OBJS += $(OUTPUT)tests/pmu.o
 LIB_OBJS += $(OUTPUT)tests/util.o
+LIB_OBJS += $(OUTPUT)tests/hists_link.o
 
 BUILTIN_OBJS += $(OUTPUT)builtin-annotate.o
 BUILTIN_OBJS += $(OUTPUT)builtin-bench.o
tools/perf/tests/builtin-test.c +4 -0
index 186f67535494..479d10484a74 100644
@@ -69,6 +69,10 @@ static struct test {
 		.func = test__attr,
 	},
 	{
+		.desc = "Test matching and linking mutliple hists",
+		.func = test__hists_link,
+	},
+	{
 		.func = NULL,
 	},
 };
tools/perf/tests/hists_link.c new file +502 -0

502 lines not on this page · raw message

tools/perf/tests/tests.h +1 -0
index fc121edab016..eddf1ca8cec9 100644
@@ -15,6 +15,7 @@ int test__pmu(void);
 int test__attr(void);
 int test__dso_data(void);
 int test__parse_events(void);
+int test__hists_link(void);
 
 /* Util */
 int trace_event__id(const char *evname);
tools/perf/util/machine.h +1 -0
index b7cde7467d55..166c93ccea22 100644
@@ -89,6 +89,7 @@ static inline bool machine__is_host(struct machine *machine)
 
 struct thread *machine__findnew_thread(struct machine *machine, pid_t pid);
 void machine__remove_thread(struct machine *machine, struct thread *th);
+void machine__delete_threads(struct machine *machine);
 
 size_t machine__fprintf(struct machine *machine, FILE *fp);
tools/perf/util/session.c +1 -1
index aa5e58255cba..3f25862e3ab9 100644
@@ -177,7 +177,7 @@ static void perf_session__delete_dead_threads(struct perf_session *session)
 	machine__delete_dead_threads(&session->host_machine);
 }
 
-static void machine__delete_threads(struct machine *self)
+void machine__delete_threads(struct machine *self)
 {
 	struct rb_node *nd = rb_first(&self->threads);
Signature · 1 line
1.7.11.7