Re: [PATCH 4/4] perf tools: Compare hists comm by addresses

[Namhyung Kim <namhyung@kernel.org>]

Hi,

On Thu, 12 Sep 2013 22:29:43 +0200, Frederic Weisbecker wrote:
> Now that comm strings are allocated only once and refcounted to be shared
> among threads, these can now be safely compared by addresses. This
> should remove most hists collapses on post processing.

[SNIP]

> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 26d8f11..3b307e7 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -79,7 +79,8 @@ struct sort_entry sort_thread = {
>  static int64_t
>  sort__comm_cmp(struct hist_entry *left, struct hist_entry *right)
>  {
> -	return right->thread->tid - left->thread->tid;
> +	/* Compare the addr that should be unique among comm */
> +	return thread__comm_curr(right->thread) - thread__comm_curr(left->thread);

I don't think this is enough.  A hist entry should reference the comm
itself at that time.  Saving thread can lead to referencing different
comm between insert and collapse time if thread changed the comm in the
meanwhile, right?

What about something like below:


>From 431fd9bfa844ddfe28ab1390959bc7de28804a9c Mon Sep 17 00:00:00 2001
From: Namhyung Kim <namhyung.kim@lge.com>
Date: Fri, 13 Sep 2013 16:28:57 +0900
Subject: [PATCH] perf tools: Get current comm instead of last one

At insert time, a hist entry should reference comm at the time
otherwise it can get a different comm later.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c   |  3 +++
 tools/perf/util/sort.c   |  9 +++++----
 tools/perf/util/sort.h   |  1 +
 tools/perf/util/thread.c | 10 ++--------
 tools/perf/util/thread.h |  2 ++
 5 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 1f5767f97dce..1fe90bd9dcb7 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -412,6 +412,7 @@ struct hist_entry *__hists__add_mem_entry(struct hists *self,
 {
 	struct hist_entry entry = {
 		.thread	= al->thread,
+		.comm = curr_comm(al->thread),
 		.ms = {
 			.map	= al->map,
 			.sym	= al->sym,
@@ -442,6 +443,7 @@ struct hist_entry *__hists__add_branch_entry(struct hists *self,
 {
 	struct hist_entry entry = {
 		.thread	= al->thread,
+		.comm = curr_comm(al->thread),
 		.ms = {
 			.map	= bi->to.map,
 			.sym	= bi->to.sym,
@@ -471,6 +473,7 @@ struct hist_entry *__hists__add_entry(struct hists *self,
 {
 	struct hist_entry entry = {
 		.thread	= al->thread,
+		.comm = curr_comm(al->thread),
 		.ms = {
 			.map	= al->map,
 			.sym	= al->sym,
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 3b307e740d6e..840481859e4b 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1,5 +1,6 @@
 #include "sort.h"
 #include "hist.h"
+#include "comm.h"
 #include "symbol.h"
 
 regex_t		parent_regex;
@@ -80,14 +81,14 @@ static int64_t
 sort__comm_cmp(struct hist_entry *left, struct hist_entry *right)
 {
 	/* Compare the addr that should be unique among comm */
-	return thread__comm_curr(right->thread) - thread__comm_curr(left->thread);
+	return comm__str(right->comm) - comm__str(left->comm);
 }
 
 static int64_t
 sort__comm_collapse(struct hist_entry *left, struct hist_entry *right)
 {
-	const char *comm_l = thread__comm_curr(left->thread);
-	const char *comm_r = thread__comm_curr(right->thread);
+	const char *comm_l = comm__str(left->comm);
+	const char *comm_r = comm__str(right->comm);
 
 	if (!comm_l || !comm_r)
 		return cmp_null(comm_l, comm_r);
@@ -98,7 +99,7 @@ sort__comm_collapse(struct hist_entry *left, struct hist_entry *right)
 static int hist_entry__comm_snprintf(struct hist_entry *self, char *bf,
 				     size_t size, unsigned int width)
 {
-	return repsep_snprintf(bf, size, "%*s", width, thread__comm_curr(self->thread));
+	return repsep_snprintf(bf, size, "%*s", width, comm__str(self->comm));
 }
 
 struct sort_entry sort_comm = {
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 4e80dbd271e7..4d0153f83e3c 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -84,6 +84,7 @@ struct hist_entry {
 	struct he_stat		stat;
 	struct map_symbol	ms;
 	struct thread		*thread;
+	struct comm		*comm;
 	u64			ip;
 	s32			cpu;
 
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index d3ca133329b2..e7c7fe6694e8 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -54,7 +54,7 @@ void thread__delete(struct thread *self)
 	free(self);
 }
 
-static struct comm *curr_comm(const struct thread *self)
+struct comm *curr_comm(const struct thread *self)
 {
 	if (list_empty(&self->comm_list))
 		return NULL;
@@ -65,13 +65,7 @@ static struct comm *curr_comm(const struct thread *self)
 /* CHECKME: time should always be 0 if event aren't ordered */
 int thread__set_comm(struct thread *self, const char *str, u64 timestamp)
 {
-	struct comm *new, *curr = curr_comm(self);
-
-	/* Override latest entry if it had no specific time coverage */
-	if (!curr->start) {
-		list_del(&curr->list);
-		comm__free(curr);
-	}
+	struct comm *new;
 
 	new = comm__new(str, timestamp);
 	if (!new)
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
index 1d9378abb515..cf8e31d0028b 100644
--- a/tools/perf/util/thread.h
+++ b/tools/perf/util/thread.h
@@ -26,6 +26,7 @@ struct thread {
 };
 
 struct machine;
+struct comm;
 
 struct thread *thread__new(pid_t pid, pid_t tid);
 void thread__delete(struct thread *self);
@@ -36,6 +37,7 @@ static inline void thread__exited(struct thread *thread)
 
 int thread__set_comm(struct thread *self, const char *comm, u64 timestamp);
 int thread__comm_len(struct thread *self);
+struct comm *curr_comm(const struct thread *self);
 const char *thread__comm_curr(const struct thread *self);
 void thread__insert_map(struct thread *self, struct map *map);
 int thread__fork(struct thread *self, struct thread *parent, u64 timestamp);
-- 
1.7.11.7