[PATCH v1 0/4] perf annotate: Display multiple events on the left side of annotate view

[PATCH v1 0/4] perf annotate: Display multiple events on the left side of annotate view

[Jin Yao]

perf record -e cycles,branches ...
perf annotate main --stdio

The result only shows cycles. It should show both cycles and
branches on the left side of annotate view. It works with
"--group", but need this to work even without groups.

The patch series supports to display multiple events on the
left side of annotate view for stdio, tui and gtk modes.

Jin Yao (4):
  perf annotate: create a new hists to manage multiple events samples
  perf annotate: Display multiple events for stdio mode
  perf annotate: Display multiple events for tui mode
  perf annotate: Display multiple events for gtk mode

 tools/perf/builtin-annotate.c     |  62 +++++++++----
 tools/perf/builtin-top.c          |   3 +-
 tools/perf/ui/browsers/annotate.c |  49 +++++++---
 tools/perf/ui/browsers/hists.c    |   2 +-
 tools/perf/ui/gtk/annotate.c      |  35 ++++---
 tools/perf/util/annotate.c        | 187 +++++++++++++++++++++++++++++---------
 tools/perf/util/annotate.h        |  16 +++-
 tools/perf/util/hist.h            |   8 +-
 tools/perf/util/sort.c            |  21 +++++
 tools/perf/util/sort.h            |  13 +++
 10 files changed, 301 insertions(+), 95 deletions(-)

-- 
2.7.4

[PATCH v1 1/4] perf annotate: create a new hists to manage multiple events samples

[Jin Yao]

An issue is found during using perf annotate.

perf record -e cycles,branches ...
perf annotate main --stdio

The result only shows cycles. It should show both cycles and
branches on the left side. It works with "--group", but need
this to work even without groups.

In current design, the hists is per event. So we need a new
hists to manage the samples for multiple events and use a new
hist_event data structure to save the map/symbol information
for per event.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/builtin-annotate.c | 60 +++++++++++++++++++++++++++++++------------
 tools/perf/util/annotate.c    |  8 ++++++
 tools/perf/util/annotate.h    |  4 +++
 tools/perf/util/sort.c        | 21 +++++++++++++++
 tools/perf/util/sort.h        | 13 ++++++++++
 5 files changed, 89 insertions(+), 17 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 658c920..833866c 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -45,6 +45,7 @@ struct perf_annotate {
 	bool	   skip_missing;
 	const char *sym_hist_filter;
 	const char *cpu_list;
+	struct hists events_hists;
 	DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
 };
 
@@ -153,6 +154,7 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
 	struct hists *hists = evsel__hists(evsel);
 	struct hist_entry *he;
 	int ret;
+	struct hist_event *hevt;
 
 	if (ann->sym_hist_filter != NULL &&
 	    (al->sym == NULL ||
@@ -177,12 +179,30 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
 	 */
 	process_branch_stack(sample->branch_stack, al, sample);
 
-	he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true);
-	if (he == NULL)
-		return -ENOMEM;
+	if (symbol_conf.nr_events == 1) {
+		he = hists__add_entry(hists, al, NULL, NULL, NULL,
+				      sample, true);
+		if (he == NULL)
+			return -ENOMEM;
+
+		ret = hist_entry__inc_addr_samples(he, sample, evsel->idx,
+						   al->addr);
+		hists__inc_nr_samples(hists, true);
+	} else {
+		he = hists__add_entry(&ann->events_hists, al, NULL,
+				      NULL, NULL, sample, true);
+		if (he == NULL)
+			return -ENOMEM;
+
+		hevt = hist_entry__event_add(he, evsel);
+		if (hevt == NULL)
+			return -ENOMEM;
+
+		ret = hist_event__inc_addr_samples(hevt, sample, hevt->idx,
+						   al->addr);
+		hists__inc_nr_samples(&ann->events_hists, true);
+	}
 
-	ret = hist_entry__inc_addr_samples(he, sample, evsel->idx, al->addr);
-	hists__inc_nr_samples(hists, true);
 	return ret;
 }
 
@@ -304,7 +324,8 @@ static int __cmd_annotate(struct perf_annotate *ann)
 	int ret;
 	struct perf_session *session = ann->session;
 	struct perf_evsel *pos;
-	u64 total_nr_samples;
+	u64 total_nr_samples = 0;
+	struct hists *hists;
 
 	if (ann->cpu_list) {
 		ret = perf_session__cpu_bitmap(session, ann->cpu_list,
@@ -335,23 +356,26 @@ static int __cmd_annotate(struct perf_annotate *ann)
 	if (verbose > 2)
 		perf_session__fprintf_dsos(session, stdout);
 
-	total_nr_samples = 0;
-	evlist__for_each_entry(session->evlist, pos) {
-		struct hists *hists = evsel__hists(pos);
-		u32 nr_samples = hists->stats.nr_events[PERF_RECORD_SAMPLE];
+	if (symbol_conf.nr_events > 1) {
+		hists = &ann->events_hists;
+		total_nr_samples +=
+			hists->stats.nr_events[PERF_RECORD_SAMPLE];
+
+		hists__collapse_resort(hists, NULL);
+		hists__output_resort(hists, NULL);
+		hists__find_annotations(hists, NULL, ann);
+	} else {
+		evlist__for_each_entry(session->evlist, pos) {
+			hists = evsel__hists(pos);
+			total_nr_samples +=
+				hists->stats.nr_events[PERF_RECORD_SAMPLE];
 
-		if (nr_samples > 0) {
-			total_nr_samples += nr_samples;
 			hists__collapse_resort(hists, NULL);
 			/* Don't sort callchain */
 			perf_evsel__reset_sample_bit(pos, CALLCHAIN);
 			perf_evsel__output_resort(pos, NULL);
-
-			if (symbol_conf.event_group &&
-			    !perf_evsel__is_group_leader(pos))
-				continue;
-
 			hists__find_annotations(hists, pos, ann);
+			break;
 		}
 	}
 
@@ -486,6 +510,8 @@ int cmd_annotate(int argc, const char **argv)
 	if (ret < 0)
 		goto out_delete;
 
+	__hists__init(&annotate.events_hists, &perf_hpp_list);
+
 	if (setup_sorting(NULL) < 0)
 		usage_with_options(annotate_usage, options);
 
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 2dab0e5..16ec881 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -828,6 +828,14 @@ int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *samp
 	return symbol__inc_addr_samples(he->ms.sym, he->ms.map, evidx, ip, sample);
 }
 
+int hist_event__inc_addr_samples(struct hist_event *hevt,
+				 struct perf_sample *sample,
+				 int idx, u64 ip)
+{
+	return symbol__inc_addr_samples(hevt->ms.sym, hevt->ms.map,
+					idx, ip, sample);
+}
+
 static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, struct map *map)
 {
 	dl->ins.ops = ins__find(arch, dl->ins.name);
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 9ce575c..0d44cfe 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -165,6 +165,10 @@ int addr_map_symbol__account_cycles(struct addr_map_symbol *ams,
 int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *sample,
 				 int evidx, u64 addr);
 
+int hist_event__inc_addr_samples(struct hist_event *hevt,
+				 struct perf_sample *sample,
+				 int idx, u64 addr);
+
 int symbol__alloc_hist(struct symbol *sym);
 void symbol__annotate_zero_histograms(struct symbol *sym);
 
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 12359bd..1500a8e 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1490,6 +1490,27 @@ struct sort_entry sort_sym_size = {
 	.se_width_idx	= HISTC_SYM_SIZE,
 };
 
+struct hist_event *hist_entry__event_add(struct hist_entry *he,
+					 struct perf_evsel *evsel)
+{
+	int i;
+
+	for (i = 0; i < he->event_nr; i++) {
+		if (he->events[i].evsel == evsel)
+			return &he->events[i];
+	}
+
+	if (i < HIST_ENTRY_EVENTS) {
+		memset(&he->events[i], 0, sizeof(struct hist_event));
+		memcpy(&he->events[i].ms, &he->ms, sizeof(struct map_symbol));
+		he->events[i].evsel = evsel;
+		he->events[i].idx = i;
+		he->event_nr++;
+		return &he->events[i];
+	}
+
+	return NULL;
+}
 
 struct sort_dimension {
 	const char		*name;
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index b7c7559..446a2a3 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -79,6 +79,14 @@ struct hist_entry_ops {
 	void	(*free)(void *ptr);
 };
 
+#define HIST_ENTRY_EVENTS	8
+
+struct hist_event {
+	struct map_symbol	ms;
+	struct perf_evsel	*evsel;
+	int			idx;
+};
+
 /**
  * struct hist_entry - histogram entry
  *
@@ -95,6 +103,8 @@ struct hist_entry {
 	struct he_stat		stat;
 	struct he_stat		*stat_acc;
 	struct map_symbol	ms;
+	struct hist_event	events[HIST_ENTRY_EVENTS];
+	int			event_nr;
 	struct thread		*thread;
 	struct comm		*comm;
 	struct namespace_id	cgroup_id;
@@ -291,4 +301,7 @@ sort__daddr_cmp(struct hist_entry *left, struct hist_entry *right);
 int64_t
 sort__dcacheline_cmp(struct hist_entry *left, struct hist_entry *right);
 char *hist_entry__get_srcline(struct hist_entry *he);
+struct hist_event *hist_entry__event_add(struct hist_entry *he,
+					 struct perf_evsel *evsel);
+
 #endif	/* __PERF_SORT_H */
-- 
2.7.4

Re: [PATCH v1 1/4] perf annotate: create a new hists to manage multiple events samples

[Arnaldo Carvalho de Melo]

Em Wed, Aug 16, 2017 at 06:18:33PM +0800, Jin Yao escreveu:
> An issue is found during using perf annotate.
> 
> perf record -e cycles,branches ...
> perf annotate main --stdio
> 
> The result only shows cycles. It should show both cycles and
> branches on the left side. It works with "--group", but need
> this to work even without groups.
> 
> In current design, the hists is per event. So we need a new
> hists to manage the samples for multiple events and use a new
> hist_event data structure to save the map/symbol information
> for per event.

Humm, why do we need another hists? Don't we have one per evsel, don't
we have a evlist from where to get all of those evsels, can't we just
use that to add one column per evsel?

- Arnaldo
 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
>  tools/perf/builtin-annotate.c | 60 +++++++++++++++++++++++++++++++------------
>  tools/perf/util/annotate.c    |  8 ++++++
>  tools/perf/util/annotate.h    |  4 +++
>  tools/perf/util/sort.c        | 21 +++++++++++++++
>  tools/perf/util/sort.h        | 13 ++++++++++
>  5 files changed, 89 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 658c920..833866c 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -45,6 +45,7 @@ struct perf_annotate {
>  	bool	   skip_missing;
>  	const char *sym_hist_filter;
>  	const char *cpu_list;
> +	struct hists events_hists;
>  	DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
>  };
>  
> @@ -153,6 +154,7 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
>  	struct hists *hists = evsel__hists(evsel);
>  	struct hist_entry *he;
>  	int ret;
> +	struct hist_event *hevt;
>  
>  	if (ann->sym_hist_filter != NULL &&
>  	    (al->sym == NULL ||
> @@ -177,12 +179,30 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
>  	 */
>  	process_branch_stack(sample->branch_stack, al, sample);
>  
> -	he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true);
> -	if (he == NULL)
> -		return -ENOMEM;
> +	if (symbol_conf.nr_events == 1) {
> +		he = hists__add_entry(hists, al, NULL, NULL, NULL,
> +				      sample, true);
> +		if (he == NULL)
> +			return -ENOMEM;
> +
> +		ret = hist_entry__inc_addr_samples(he, sample, evsel->idx,
> +						   al->addr);
> +		hists__inc_nr_samples(hists, true);
> +	} else {
> +		he = hists__add_entry(&ann->events_hists, al, NULL,
> +				      NULL, NULL, sample, true);
> +		if (he == NULL)
> +			return -ENOMEM;
> +
> +		hevt = hist_entry__event_add(he, evsel);
> +		if (hevt == NULL)
> +			return -ENOMEM;
> +
> +		ret = hist_event__inc_addr_samples(hevt, sample, hevt->idx,
> +						   al->addr);
> +		hists__inc_nr_samples(&ann->events_hists, true);
> +	}
>  
> -	ret = hist_entry__inc_addr_samples(he, sample, evsel->idx, al->addr);
> -	hists__inc_nr_samples(hists, true);
>  	return ret;
>  }
>  
> @@ -304,7 +324,8 @@ static int __cmd_annotate(struct perf_annotate *ann)
>  	int ret;
>  	struct perf_session *session = ann->session;
>  	struct perf_evsel *pos;
> -	u64 total_nr_samples;
> +	u64 total_nr_samples = 0;
> +	struct hists *hists;
>  
>  	if (ann->cpu_list) {
>  		ret = perf_session__cpu_bitmap(session, ann->cpu_list,
> @@ -335,23 +356,26 @@ static int __cmd_annotate(struct perf_annotate *ann)
>  	if (verbose > 2)
>  		perf_session__fprintf_dsos(session, stdout);
>  
> -	total_nr_samples = 0;
> -	evlist__for_each_entry(session->evlist, pos) {
> -		struct hists *hists = evsel__hists(pos);
> -		u32 nr_samples = hists->stats.nr_events[PERF_RECORD_SAMPLE];
> +	if (symbol_conf.nr_events > 1) {
> +		hists = &ann->events_hists;
> +		total_nr_samples +=
> +			hists->stats.nr_events[PERF_RECORD_SAMPLE];
> +
> +		hists__collapse_resort(hists, NULL);
> +		hists__output_resort(hists, NULL);
> +		hists__find_annotations(hists, NULL, ann);
> +	} else {
> +		evlist__for_each_entry(session->evlist, pos) {
> +			hists = evsel__hists(pos);
> +			total_nr_samples +=
> +				hists->stats.nr_events[PERF_RECORD_SAMPLE];
>  
> -		if (nr_samples > 0) {
> -			total_nr_samples += nr_samples;
>  			hists__collapse_resort(hists, NULL);
>  			/* Don't sort callchain */
>  			perf_evsel__reset_sample_bit(pos, CALLCHAIN);
>  			perf_evsel__output_resort(pos, NULL);
> -
> -			if (symbol_conf.event_group &&
> -			    !perf_evsel__is_group_leader(pos))
> -				continue;
> -
>  			hists__find_annotations(hists, pos, ann);
> +			break;
>  		}
>  	}
>  
> @@ -486,6 +510,8 @@ int cmd_annotate(int argc, const char **argv)
>  	if (ret < 0)
>  		goto out_delete;
>  
> +	__hists__init(&annotate.events_hists, &perf_hpp_list);
> +
>  	if (setup_sorting(NULL) < 0)
>  		usage_with_options(annotate_usage, options);
>  
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 2dab0e5..16ec881 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -828,6 +828,14 @@ int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *samp
>  	return symbol__inc_addr_samples(he->ms.sym, he->ms.map, evidx, ip, sample);
>  }
>  
> +int hist_event__inc_addr_samples(struct hist_event *hevt,
> +				 struct perf_sample *sample,
> +				 int idx, u64 ip)
> +{
> +	return symbol__inc_addr_samples(hevt->ms.sym, hevt->ms.map,
> +					idx, ip, sample);
> +}
> +
>  static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, struct map *map)
>  {
>  	dl->ins.ops = ins__find(arch, dl->ins.name);
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 9ce575c..0d44cfe 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -165,6 +165,10 @@ int addr_map_symbol__account_cycles(struct addr_map_symbol *ams,
>  int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *sample,
>  				 int evidx, u64 addr);
>  
> +int hist_event__inc_addr_samples(struct hist_event *hevt,
> +				 struct perf_sample *sample,
> +				 int idx, u64 addr);
> +
>  int symbol__alloc_hist(struct symbol *sym);
>  void symbol__annotate_zero_histograms(struct symbol *sym);
>  
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 12359bd..1500a8e 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -1490,6 +1490,27 @@ struct sort_entry sort_sym_size = {
>  	.se_width_idx	= HISTC_SYM_SIZE,
>  };
>  
> +struct hist_event *hist_entry__event_add(struct hist_entry *he,
> +					 struct perf_evsel *evsel)
> +{
> +	int i;
> +
> +	for (i = 0; i < he->event_nr; i++) {
> +		if (he->events[i].evsel == evsel)
> +			return &he->events[i];
> +	}
> +
> +	if (i < HIST_ENTRY_EVENTS) {
> +		memset(&he->events[i], 0, sizeof(struct hist_event));
> +		memcpy(&he->events[i].ms, &he->ms, sizeof(struct map_symbol));
> +		he->events[i].evsel = evsel;
> +		he->events[i].idx = i;
> +		he->event_nr++;
> +		return &he->events[i];
> +	}
> +
> +	return NULL;
> +}
>  
>  struct sort_dimension {
>  	const char		*name;
> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index b7c7559..446a2a3 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -79,6 +79,14 @@ struct hist_entry_ops {
>  	void	(*free)(void *ptr);
>  };
>  
> +#define HIST_ENTRY_EVENTS	8
> +
> +struct hist_event {
> +	struct map_symbol	ms;
> +	struct perf_evsel	*evsel;
> +	int			idx;
> +};
> +
>  /**
>   * struct hist_entry - histogram entry
>   *
> @@ -95,6 +103,8 @@ struct hist_entry {
>  	struct he_stat		stat;
>  	struct he_stat		*stat_acc;
>  	struct map_symbol	ms;
> +	struct hist_event	events[HIST_ENTRY_EVENTS];
> +	int			event_nr;
>  	struct thread		*thread;
>  	struct comm		*comm;
>  	struct namespace_id	cgroup_id;
> @@ -291,4 +301,7 @@ sort__daddr_cmp(struct hist_entry *left, struct hist_entry *right);
>  int64_t
>  sort__dcacheline_cmp(struct hist_entry *left, struct hist_entry *right);
>  char *hist_entry__get_srcline(struct hist_entry *he);
> +struct hist_event *hist_entry__event_add(struct hist_entry *he,
> +					 struct perf_evsel *evsel);
> +
>  #endif	/* __PERF_SORT_H */
> -- 
> 2.7.4

Re: [PATCH v1 1/4] perf annotate: create a new hists to manage multiple events samples

[Jin, Yao]

On 9/8/2017 9:43 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Aug 16, 2017 at 06:18:33PM +0800, Jin Yao escreveu:
>> An issue is found during using perf annotate.
>>
>> perf record -e cycles,branches ...
>> perf annotate main --stdio
>>
>> The result only shows cycles. It should show both cycles and
>> branches on the left side. It works with "--group", but need
>> this to work even without groups.
>>
>> In current design, the hists is per event. So we need a new
>> hists to manage the samples for multiple events and use a new
>> hist_event data structure to save the map/symbol information
>> for per event.
> Humm, why do we need another hists? Don't we have one per evsel, don't
> we have a evlist from where to get all of those evsels, can't we just
> use that to add one column per evsel?
>
> - Arnaldo
>   
>

Hi Arnaldo,

I'm considering a case.

Suppose we sample 2 events ("branches" and "cache-misses"). The samples 
of "branches" are hit in function A and the samples of "cache-misses" 
are hit in function B.

The branches evsel has one hists and cache-misses evsel has another hists.

The hists of branches evsel has one hist-entry which stands for the 
function A symbol. The hists of cache-misses evsel has one hist-entry 
which stands for the function B symbol.

If we start to show the instructions in function B from cache-misses 
evsel, we will lose the function A.

Because even if we get the branches evsel from the link in cache-misses 
evsel, but the function A is before function B and function B has been 
displayed yet, so the function A is lost.

Considering the number of events can be greater than 2, the code will be 
much more complicated. So using a global hists should be an easy solution.

Thanks
Jin Yao

Re: [PATCH v1 1/4] perf annotate: create a new hists to manage multiple events samples

[Jin, Yao]

On 9/11/2017 9:33 AM, Jin, Yao wrote:
> 
> 
> On 9/8/2017 9:43 PM, Arnaldo Carvalho de Melo wrote:
>> Em Wed, Aug 16, 2017 at 06:18:33PM +0800, Jin Yao escreveu:
>>> An issue is found during using perf annotate.
>>>
>>> perf record -e cycles,branches ...
>>> perf annotate main --stdio
>>>
>>> The result only shows cycles. It should show both cycles and
>>> branches on the left side. It works with "--group", but need
>>> this to work even without groups.
>>>
>>> In current design, the hists is per event. So we need a new
>>> hists to manage the samples for multiple events and use a new
>>> hist_event data structure to save the map/symbol information
>>> for per event.
>> Humm, why do we need another hists? Don't we have one per evsel, don't
>> we have a evlist from where to get all of those evsels, can't we just
>> use that to add one column per evsel?
>>
>> - Arnaldo
>>  
> 
> Hi Arnaldo,
> 
> I'm considering a case.
> 
> Suppose we sample 2 events ("branches" and "cache-misses"). The samples of "branches" are hit in function A and the samples of "cache-misses" are hit in function B.
> 
> The branches evsel has one hists and cache-misses evsel has another hists.
> 
> The hists of branches evsel has one hist-entry which stands for the function A symbol. The hists of cache-misses evsel has one hist-entry which stands for the function B symbol.
> 
> If we start to show the instructions in function B from cache-misses evsel, we will lose the function A.
> 
> Because even if we get the branches evsel from the link in cache-misses evsel, but the function A is before function B and function B has been displayed yet, so the function A is lost.
> 
> Considering the number of events can be greater than 2, the code will be much more complicated. So using a global hists should be an easy solution.
> 
> Thanks
> Jin Yao
> 

Hi Arnaldo,

Could the solution of using a new hists for multiple events be accepted?

Or anything I should update in the patches?

Thanks
Jin Yao

Re: [PATCH v1 1/4] perf annotate: create a new hists to manage multiple events samples

[Arnaldo Carvalho de Melo]

Em Thu, Sep 14, 2017 at 09:03:55AM +0800, Jin, Yao escreveu:
> On 9/11/2017 9:33 AM, Jin, Yao wrote:
> > On 9/8/2017 9:43 PM, Arnaldo Carvalho de Melo wrote:
> >> Em Wed, Aug 16, 2017 at 06:18:33PM +0800, Jin Yao escreveu:
> >>> An issue is found during using perf annotate.
> >>>
> >>> perf record -e cycles,branches ...
> >>> perf annotate main --stdio
> >>>
> >>> The result only shows cycles. It should show both cycles and
> >>> branches on the left side. It works with "--group", but need
> >>> this to work even without groups.
> >>>
> >>> In current design, the hists is per event. So we need a new
> >>> hists to manage the samples for multiple events and use a new
> >>> hist_event data structure to save the map/symbol information
> >>> for per event.
> >> Humm, why do we need another hists? Don't we have one per evsel, don't
> >> we have a evlist from where to get all of those evsels, can't we just
> >> use that to add one column per evsel?

> > I'm considering a case.

> > Suppose we sample 2 events ("branches" and "cache-misses"). The samples of "branches" are hit in function A and the samples of "cache-misses" are hit in function B.
> > 
> > The branches evsel has one hists and cache-misses evsel has another hists.
> > 
> > The hists of branches evsel has one hist-entry which stands for the function A symbol. The hists of cache-misses evsel has one hist-entry which stands for the function B symbol.
> > 
> > If we start to show the instructions in function B from cache-misses evsel, we will lose the function A.
> > 
> > Because even if we get the branches evsel from the link in cache-misses evsel, but the function A is before function B and function B has been displayed yet, so the function A is lost.
> > 
> > Considering the number of events can be greater than 2, the code will be much more complicated. So using a global hists should be an easy solution.
> 
> Could the solution of using a new hists for multiple events be accepted?
> 
> Or anything I should update in the patches?

I'm not having time at this moment for doing a proper review, wait a bit
more please.

- Arnaldo

Re: [PATCH v1 1/4] perf annotate: create a new hists to manage multiple events samples

[Jin, Yao]

On 9/14/2017 10:05 PM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Sep 14, 2017 at 09:03:55AM +0800, Jin, Yao escreveu:
>> On 9/11/2017 9:33 AM, Jin, Yao wrote:
>>> On 9/8/2017 9:43 PM, Arnaldo Carvalho de Melo wrote:
>>>> Em Wed, Aug 16, 2017 at 06:18:33PM +0800, Jin Yao escreveu:
>>>>> An issue is found during using perf annotate.
>>>>>
>>>>> perf record -e cycles,branches ...
>>>>> perf annotate main --stdio
>>>>>
>>>>> The result only shows cycles. It should show both cycles and
>>>>> branches on the left side. It works with "--group", but need
>>>>> this to work even without groups.
>>>>>
>>>>> In current design, the hists is per event. So we need a new
>>>>> hists to manage the samples for multiple events and use a new
>>>>> hist_event data structure to save the map/symbol information
>>>>> for per event.
>>>> Humm, why do we need another hists? Don't we have one per evsel, don't
>>>> we have a evlist from where to get all of those evsels, can't we just
>>>> use that to add one column per evsel?
> 
>>> I'm considering a case.
> 
>>> Suppose we sample 2 events ("branches" and "cache-misses"). The samples of "branches" are hit in function A and the samples of "cache-misses" are hit in function B.
>>>
>>> The branches evsel has one hists and cache-misses evsel has another hists.
>>>
>>> The hists of branches evsel has one hist-entry which stands for the function A symbol. The hists of cache-misses evsel has one hist-entry which stands for the function B symbol.
>>>
>>> If we start to show the instructions in function B from cache-misses evsel, we will lose the function A.
>>>
>>> Because even if we get the branches evsel from the link in cache-misses evsel, but the function A is before function B and function B has been displayed yet, so the function A is lost.
>>>
>>> Considering the number of events can be greater than 2, the code will be much more complicated. So using a global hists should be an easy solution.
>>
>> Could the solution of using a new hists for multiple events be accepted?
>>
>> Or anything I should update in the patches?
> 
> I'm not having time at this moment for doing a proper review, wait a bit
> more please.
> 
> - Arnaldo
> 

No problem, that's fine. Thanks in advance. 

Re: [PATCH v1 1/4] perf annotate: create a new hists to manage multiple events samples

[Arnaldo Carvalho de Melo]

Em Wed, Aug 16, 2017 at 06:18:33PM +0800, Jin Yao escreveu:
> An issue is found during using perf annotate.
> 
> perf record -e cycles,branches ...
> perf annotate main --stdio
> 
> The result only shows cycles. It should show both cycles and
> branches on the left side. It works with "--group", but need
> this to work even without groups.

Right, for --group we know that we'll be reading all the counters at
each sample, so it all works and we can use the current design.

When we're not using groups tho, each sample has just one of the events
and we end up with separate views.

Note tho that since the annotation buckets are kept per 'struct symbol'
instance, this problem should be present only in the hist_entry based
views, i.e. 'perf report' and 'perf top', right?

I.e. all struct hist_entry->ms.sym instances point to the same stuct
symbol and thus will use the same annotation histogram buckets, i.e.
symbol__annotation(hist_entry->ms.sym) point to the same 'struct
annotation' instance, and then its a matter of passing this pointer to
annotation__histogram(notes, IDX) where this IDX is perf_evsel->idx.

I wonder if we can't just add a new rb_node in struct hist_entry and
have it in two rb_trees, i.e. in two 'struct hists' instances, one per
evsel and one per evlist.

To be continued...
 
> In current design, the hists is per event. So we need a new
> hists to manage the samples for multiple events and use a new
> hist_event data structure to save the map/symbol information
> for per event.
> 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
>  tools/perf/builtin-annotate.c | 60 +++++++++++++++++++++++++++++++------------
>  tools/perf/util/annotate.c    |  8 ++++++
>  tools/perf/util/annotate.h    |  4 +++
>  tools/perf/util/sort.c        | 21 +++++++++++++++
>  tools/perf/util/sort.h        | 13 ++++++++++
>  5 files changed, 89 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 658c920..833866c 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -45,6 +45,7 @@ struct perf_annotate {
>  	bool	   skip_missing;
>  	const char *sym_hist_filter;
>  	const char *cpu_list;
> +	struct hists events_hists;
>  	DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
>  };
>  
> @@ -153,6 +154,7 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
>  	struct hists *hists = evsel__hists(evsel);
>  	struct hist_entry *he;
>  	int ret;
> +	struct hist_event *hevt;
>  
>  	if (ann->sym_hist_filter != NULL &&
>  	    (al->sym == NULL ||
> @@ -177,12 +179,30 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
>  	 */
>  	process_branch_stack(sample->branch_stack, al, sample);
>  
> -	he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true);
> -	if (he == NULL)
> -		return -ENOMEM;
> +	if (symbol_conf.nr_events == 1) {
> +		he = hists__add_entry(hists, al, NULL, NULL, NULL,
> +				      sample, true);
> +		if (he == NULL)
> +			return -ENOMEM;
> +
> +		ret = hist_entry__inc_addr_samples(he, sample, evsel->idx,
> +						   al->addr);
> +		hists__inc_nr_samples(hists, true);
> +	} else {
> +		he = hists__add_entry(&ann->events_hists, al, NULL,
> +				      NULL, NULL, sample, true);
> +		if (he == NULL)
> +			return -ENOMEM;
> +
> +		hevt = hist_entry__event_add(he, evsel);
> +		if (hevt == NULL)
> +			return -ENOMEM;
> +
> +		ret = hist_event__inc_addr_samples(hevt, sample, hevt->idx,
> +						   al->addr);
> +		hists__inc_nr_samples(&ann->events_hists, true);
> +	}
>  
> -	ret = hist_entry__inc_addr_samples(he, sample, evsel->idx, al->addr);
> -	hists__inc_nr_samples(hists, true);
>  	return ret;
>  }
>  
> @@ -304,7 +324,8 @@ static int __cmd_annotate(struct perf_annotate *ann)
>  	int ret;
>  	struct perf_session *session = ann->session;
>  	struct perf_evsel *pos;
> -	u64 total_nr_samples;
> +	u64 total_nr_samples = 0;
> +	struct hists *hists;
>  
>  	if (ann->cpu_list) {
>  		ret = perf_session__cpu_bitmap(session, ann->cpu_list,
> @@ -335,23 +356,26 @@ static int __cmd_annotate(struct perf_annotate *ann)
>  	if (verbose > 2)
>  		perf_session__fprintf_dsos(session, stdout);
>  
> -	total_nr_samples = 0;
> -	evlist__for_each_entry(session->evlist, pos) {
> -		struct hists *hists = evsel__hists(pos);
> -		u32 nr_samples = hists->stats.nr_events[PERF_RECORD_SAMPLE];
> +	if (symbol_conf.nr_events > 1) {
> +		hists = &ann->events_hists;
> +		total_nr_samples +=
> +			hists->stats.nr_events[PERF_RECORD_SAMPLE];
> +
> +		hists__collapse_resort(hists, NULL);
> +		hists__output_resort(hists, NULL);
> +		hists__find_annotations(hists, NULL, ann);
> +	} else {
> +		evlist__for_each_entry(session->evlist, pos) {
> +			hists = evsel__hists(pos);
> +			total_nr_samples +=
> +				hists->stats.nr_events[PERF_RECORD_SAMPLE];
>  
> -		if (nr_samples > 0) {
> -			total_nr_samples += nr_samples;
>  			hists__collapse_resort(hists, NULL);
>  			/* Don't sort callchain */
>  			perf_evsel__reset_sample_bit(pos, CALLCHAIN);
>  			perf_evsel__output_resort(pos, NULL);
> -
> -			if (symbol_conf.event_group &&
> -			    !perf_evsel__is_group_leader(pos))
> -				continue;
> -
>  			hists__find_annotations(hists, pos, ann);
> +			break;
>  		}
>  	}
>  
> @@ -486,6 +510,8 @@ int cmd_annotate(int argc, const char **argv)
>  	if (ret < 0)
>  		goto out_delete;
>  
> +	__hists__init(&annotate.events_hists, &perf_hpp_list);
> +
>  	if (setup_sorting(NULL) < 0)
>  		usage_with_options(annotate_usage, options);
>  
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 2dab0e5..16ec881 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -828,6 +828,14 @@ int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *samp
>  	return symbol__inc_addr_samples(he->ms.sym, he->ms.map, evidx, ip, sample);
>  }
>  
> +int hist_event__inc_addr_samples(struct hist_event *hevt,
> +				 struct perf_sample *sample,
> +				 int idx, u64 ip)
> +{
> +	return symbol__inc_addr_samples(hevt->ms.sym, hevt->ms.map,
> +					idx, ip, sample);
> +}
> +
>  static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, struct map *map)
>  {
>  	dl->ins.ops = ins__find(arch, dl->ins.name);
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 9ce575c..0d44cfe 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -165,6 +165,10 @@ int addr_map_symbol__account_cycles(struct addr_map_symbol *ams,
>  int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *sample,
>  				 int evidx, u64 addr);
>  
> +int hist_event__inc_addr_samples(struct hist_event *hevt,
> +				 struct perf_sample *sample,
> +				 int idx, u64 addr);
> +
>  int symbol__alloc_hist(struct symbol *sym);
>  void symbol__annotate_zero_histograms(struct symbol *sym);
>  
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 12359bd..1500a8e 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -1490,6 +1490,27 @@ struct sort_entry sort_sym_size = {
>  	.se_width_idx	= HISTC_SYM_SIZE,
>  };
>  
> +struct hist_event *hist_entry__event_add(struct hist_entry *he,
> +					 struct perf_evsel *evsel)
> +{
> +	int i;
> +
> +	for (i = 0; i < he->event_nr; i++) {
> +		if (he->events[i].evsel == evsel)
> +			return &he->events[i];
> +	}
> +
> +	if (i < HIST_ENTRY_EVENTS) {
> +		memset(&he->events[i], 0, sizeof(struct hist_event));
> +		memcpy(&he->events[i].ms, &he->ms, sizeof(struct map_symbol));
> +		he->events[i].evsel = evsel;
> +		he->events[i].idx = i;
> +		he->event_nr++;
> +		return &he->events[i];
> +	}
> +
> +	return NULL;
> +}
>  
>  struct sort_dimension {
>  	const char		*name;
> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index b7c7559..446a2a3 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -79,6 +79,14 @@ struct hist_entry_ops {
>  	void	(*free)(void *ptr);
>  };
>  
> +#define HIST_ENTRY_EVENTS	8
> +
> +struct hist_event {
> +	struct map_symbol	ms;
> +	struct perf_evsel	*evsel;
> +	int			idx;
> +};
> +
>  /**
>   * struct hist_entry - histogram entry
>   *
> @@ -95,6 +103,8 @@ struct hist_entry {
>  	struct he_stat		stat;
>  	struct he_stat		*stat_acc;
>  	struct map_symbol	ms;
> +	struct hist_event	events[HIST_ENTRY_EVENTS];
> +	int			event_nr;
>  	struct thread		*thread;
>  	struct comm		*comm;
>  	struct namespace_id	cgroup_id;
> @@ -291,4 +301,7 @@ sort__daddr_cmp(struct hist_entry *left, struct hist_entry *right);
>  int64_t
>  sort__dcacheline_cmp(struct hist_entry *left, struct hist_entry *right);
>  char *hist_entry__get_srcline(struct hist_entry *he);
> +struct hist_event *hist_entry__event_add(struct hist_entry *he,
> +					 struct perf_evsel *evsel);
> +
>  #endif	/* __PERF_SORT_H */
> -- 
> 2.7.4

Re: [PATCH v1 1/4] perf annotate: create a new hists to manage multiple events samples

[Jin, Yao]

On 10/5/2017 9:21 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Aug 16, 2017 at 06:18:33PM +0800, Jin Yao escreveu:
>> An issue is found during using perf annotate.
>>
>> perf record -e cycles,branches ...
>> perf annotate main --stdio
>>
>> The result only shows cycles. It should show both cycles and
>> branches on the left side. It works with "--group", but need
>> this to work even without groups.
> 
> Right, for --group we know that we'll be reading all the counters at
> each sample, so it all works and we can use the current design.
> 
> When we're not using groups tho, each sample has just one of the events
> and we end up with separate views.
> 
> Note tho that since the annotation buckets are kept per 'struct symbol'
> instance, this problem should be present only in the hist_entry based
> views, i.e. 'perf report' and 'perf top', right?

Yes, it seems to be in hist_entry based view.

'perf annotate', 'perf report' maybe others.

> 
> I.e. all struct hist_entry->ms.sym instances point to the same stuct
> symbol and thus will use the same annotation histogram buckets, i.e.
> symbol__annotation(hist_entry->ms.sym) point to the same 'struct
> annotation' instance, and then its a matter of passing this pointer to
> annotation__histogram(notes, IDX) where this IDX is perf_evsel->idx.
> 
> I wonder if we can't just add a new rb_node in struct hist_entry and
> have it in two rb_trees, i.e. in two 'struct hists' instances, one per
> evsel and one per evlist.
>
Currently we just have per-evsel hists. This idea will create a new per-evlist hists. 

struct perf_evlist {
	......
	struct hists	hists;
};

And let the hist_entry be linked in both per-evsel hists and per-evlist hists. 

Please correct me if my understanding is wrong for this idea.

Thanks
Jin Yao

> To be continued...
>  
>> In current design, the hists is per event. So we need a new
>> hists to manage the samples for multiple events and use a new
>> hist_event data structure to save the map/symbol information
>> for per event.
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>>  tools/perf/builtin-annotate.c | 60 +++++++++++++++++++++++++++++++------------
>>  tools/perf/util/annotate.c    |  8 ++++++
>>  tools/perf/util/annotate.h    |  4 +++
>>  tools/perf/util/sort.c        | 21 +++++++++++++++
>>  tools/perf/util/sort.h        | 13 ++++++++++
>>  5 files changed, 89 insertions(+), 17 deletions(-)
>>
>> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
>> index 658c920..833866c 100644
>> --- a/tools/perf/builtin-annotate.c
>> +++ b/tools/perf/builtin-annotate.c
>> @@ -45,6 +45,7 @@ struct perf_annotate {
>>  	bool	   skip_missing;
>>  	const char *sym_hist_filter;
>>  	const char *cpu_list;
>> +	struct hists events_hists;
>>  	DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
>>  };
>>  
>> @@ -153,6 +154,7 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
>>  	struct hists *hists = evsel__hists(evsel);
>>  	struct hist_entry *he;
>>  	int ret;
>> +	struct hist_event *hevt;
>>  
>>  	if (ann->sym_hist_filter != NULL &&
>>  	    (al->sym == NULL ||
>> @@ -177,12 +179,30 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
>>  	 */
>>  	process_branch_stack(sample->branch_stack, al, sample);
>>  
>> -	he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true);
>> -	if (he == NULL)
>> -		return -ENOMEM;
>> +	if (symbol_conf.nr_events == 1) {
>> +		he = hists__add_entry(hists, al, NULL, NULL, NULL,
>> +				      sample, true);
>> +		if (he == NULL)
>> +			return -ENOMEM;
>> +
>> +		ret = hist_entry__inc_addr_samples(he, sample, evsel->idx,
>> +						   al->addr);
>> +		hists__inc_nr_samples(hists, true);
>> +	} else {
>> +		he = hists__add_entry(&ann->events_hists, al, NULL,
>> +				      NULL, NULL, sample, true);
>> +		if (he == NULL)
>> +			return -ENOMEM;
>> +
>> +		hevt = hist_entry__event_add(he, evsel);
>> +		if (hevt == NULL)
>> +			return -ENOMEM;
>> +
>> +		ret = hist_event__inc_addr_samples(hevt, sample, hevt->idx,
>> +						   al->addr);
>> +		hists__inc_nr_samples(&ann->events_hists, true);
>> +	}
>>  
>> -	ret = hist_entry__inc_addr_samples(he, sample, evsel->idx, al->addr);
>> -	hists__inc_nr_samples(hists, true);
>>  	return ret;
>>  }
>>  
>> @@ -304,7 +324,8 @@ static int __cmd_annotate(struct perf_annotate *ann)
>>  	int ret;
>>  	struct perf_session *session = ann->session;
>>  	struct perf_evsel *pos;
>> -	u64 total_nr_samples;
>> +	u64 total_nr_samples = 0;
>> +	struct hists *hists;
>>  
>>  	if (ann->cpu_list) {
>>  		ret = perf_session__cpu_bitmap(session, ann->cpu_list,
>> @@ -335,23 +356,26 @@ static int __cmd_annotate(struct perf_annotate *ann)
>>  	if (verbose > 2)
>>  		perf_session__fprintf_dsos(session, stdout);
>>  
>> -	total_nr_samples = 0;
>> -	evlist__for_each_entry(session->evlist, pos) {
>> -		struct hists *hists = evsel__hists(pos);
>> -		u32 nr_samples = hists->stats.nr_events[PERF_RECORD_SAMPLE];
>> +	if (symbol_conf.nr_events > 1) {
>> +		hists = &ann->events_hists;
>> +		total_nr_samples +=
>> +			hists->stats.nr_events[PERF_RECORD_SAMPLE];
>> +
>> +		hists__collapse_resort(hists, NULL);
>> +		hists__output_resort(hists, NULL);
>> +		hists__find_annotations(hists, NULL, ann);
>> +	} else {
>> +		evlist__for_each_entry(session->evlist, pos) {
>> +			hists = evsel__hists(pos);
>> +			total_nr_samples +=
>> +				hists->stats.nr_events[PERF_RECORD_SAMPLE];
>>  
>> -		if (nr_samples > 0) {
>> -			total_nr_samples += nr_samples;
>>  			hists__collapse_resort(hists, NULL);
>>  			/* Don't sort callchain */
>>  			perf_evsel__reset_sample_bit(pos, CALLCHAIN);
>>  			perf_evsel__output_resort(pos, NULL);
>> -
>> -			if (symbol_conf.event_group &&
>> -			    !perf_evsel__is_group_leader(pos))
>> -				continue;
>> -
>>  			hists__find_annotations(hists, pos, ann);
>> +			break;
>>  		}
>>  	}
>>  
>> @@ -486,6 +510,8 @@ int cmd_annotate(int argc, const char **argv)
>>  	if (ret < 0)
>>  		goto out_delete;
>>  
>> +	__hists__init(&annotate.events_hists, &perf_hpp_list);
>> +
>>  	if (setup_sorting(NULL) < 0)
>>  		usage_with_options(annotate_usage, options);
>>  
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index 2dab0e5..16ec881 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -828,6 +828,14 @@ int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *samp
>>  	return symbol__inc_addr_samples(he->ms.sym, he->ms.map, evidx, ip, sample);
>>  }
>>  
>> +int hist_event__inc_addr_samples(struct hist_event *hevt,
>> +				 struct perf_sample *sample,
>> +				 int idx, u64 ip)
>> +{
>> +	return symbol__inc_addr_samples(hevt->ms.sym, hevt->ms.map,
>> +					idx, ip, sample);
>> +}
>> +
>>  static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, struct map *map)
>>  {
>>  	dl->ins.ops = ins__find(arch, dl->ins.name);
>> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
>> index 9ce575c..0d44cfe 100644
>> --- a/tools/perf/util/annotate.h
>> +++ b/tools/perf/util/annotate.h
>> @@ -165,6 +165,10 @@ int addr_map_symbol__account_cycles(struct addr_map_symbol *ams,
>>  int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *sample,
>>  				 int evidx, u64 addr);
>>  
>> +int hist_event__inc_addr_samples(struct hist_event *hevt,
>> +				 struct perf_sample *sample,
>> +				 int idx, u64 addr);
>> +
>>  int symbol__alloc_hist(struct symbol *sym);
>>  void symbol__annotate_zero_histograms(struct symbol *sym);
>>  
>> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
>> index 12359bd..1500a8e 100644
>> --- a/tools/perf/util/sort.c
>> +++ b/tools/perf/util/sort.c
>> @@ -1490,6 +1490,27 @@ struct sort_entry sort_sym_size = {
>>  	.se_width_idx	= HISTC_SYM_SIZE,
>>  };
>>  
>> +struct hist_event *hist_entry__event_add(struct hist_entry *he,
>> +					 struct perf_evsel *evsel)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < he->event_nr; i++) {
>> +		if (he->events[i].evsel == evsel)
>> +			return &he->events[i];
>> +	}
>> +
>> +	if (i < HIST_ENTRY_EVENTS) {
>> +		memset(&he->events[i], 0, sizeof(struct hist_event));
>> +		memcpy(&he->events[i].ms, &he->ms, sizeof(struct map_symbol));
>> +		he->events[i].evsel = evsel;
>> +		he->events[i].idx = i;
>> +		he->event_nr++;
>> +		return &he->events[i];
>> +	}
>> +
>> +	return NULL;
>> +}
>>  
>>  struct sort_dimension {
>>  	const char		*name;
>> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
>> index b7c7559..446a2a3 100644
>> --- a/tools/perf/util/sort.h
>> +++ b/tools/perf/util/sort.h
>> @@ -79,6 +79,14 @@ struct hist_entry_ops {
>>  	void	(*free)(void *ptr);
>>  };
>>  
>> +#define HIST_ENTRY_EVENTS	8
>> +
>> +struct hist_event {
>> +	struct map_symbol	ms;
>> +	struct perf_evsel	*evsel;
>> +	int			idx;
>> +};
>> +
>>  /**
>>   * struct hist_entry - histogram entry
>>   *
>> @@ -95,6 +103,8 @@ struct hist_entry {
>>  	struct he_stat		stat;
>>  	struct he_stat		*stat_acc;
>>  	struct map_symbol	ms;
>> +	struct hist_event	events[HIST_ENTRY_EVENTS];
>> +	int			event_nr;
>>  	struct thread		*thread;
>>  	struct comm		*comm;
>>  	struct namespace_id	cgroup_id;
>> @@ -291,4 +301,7 @@ sort__daddr_cmp(struct hist_entry *left, struct hist_entry *right);
>>  int64_t
>>  sort__dcacheline_cmp(struct hist_entry *left, struct hist_entry *right);
>>  char *hist_entry__get_srcline(struct hist_entry *he);
>> +struct hist_event *hist_entry__event_add(struct hist_entry *he,
>> +					 struct perf_evsel *evsel);
>> +
>>  #endif	/* __PERF_SORT_H */
>> -- 
>> 2.7.4

Re: [PATCH v1 1/4] perf annotate: create a new hists to manage multiple events samples

[Arnaldo Carvalho de Melo]

Em Sat, Oct 07, 2017 at 12:31:37AM +0800, Jin, Yao escreveu:
> On 10/5/2017 9:21 PM, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Aug 16, 2017 at 06:18:33PM +0800, Jin Yao escreveu:
> >> An issue is found during using perf annotate.

> >> perf record -e cycles,branches ...
> >> perf annotate main --stdio

> >> The result only shows cycles. It should show both cycles and
> >> branches on the left side. It works with "--group", but need
> >> this to work even without groups.
> > 
> > Right, for --group we know that we'll be reading all the counters at
> > each sample, so it all works and we can use the current design.

> > When we're not using groups tho, each sample has just one of the events
> > and we end up with separate views.

> > Note tho that since the annotation buckets are kept per 'struct symbol'
> > instance, this problem should be present only in the hist_entry based
> > views, i.e. 'perf report' and 'perf top', right?
 
> Yes, it seems to be in hist_entry based view.

Ok.

But note that your initial statement of the problem:

<quote Ji, Yao>
An issue is found during using perf annotate.

perf record -e cycles,branches ...
perf annotate main --stdio

The result only shows cycles. It should show both cycles and
branches on the left side. It works with "--group", but need
this to work even without groups.
</>

Can be solved right now, its just a matter of accessing the other
buckets in a given symbol, just like we have for --group.

The only problem is in presenting a list of symbols which can be
annotated, we have them for each evsel, and you, rightly, want to show
the list of all symbols in all evsels.

Ok?

> > I.e. all struct hist_entry->ms.sym instances point to the same stuct
> > symbol and thus will use the same annotation histogram buckets, i.e.
> > symbol__annotation(hist_entry->ms.sym) point to the same 'struct
> > annotation' instance, and then its a matter of passing this pointer to
> > annotation__histogram(notes, IDX) where this IDX is perf_evsel->idx.

> > I wonder if we can't just add a new rb_node in struct hist_entry and
> > have it in two rb_trees, i.e. in two 'struct hists' instances, one per
> > evsel and one per evlist.

> Currently we just have per-evsel hists. This idea will create a new per-evlist hists. 
 
> struct perf_evlist {
> 	......
> 	struct hists	hists;
> };
 
> And let the hist_entry be linked in both per-evsel hists and per-evlist hists. 
 
> Please correct me if my understanding is wrong for this idea.

Yes, do you see problems with trying to do it this way? At a first sight
seems like it will reuse more code, no?

I.e. in hists__findnew_entry(), when not finding an existing hist_entry
in the per-evsel hists you end up calling hist_entry__new(), right here
you'll add it to the evsel->evlist->hists, and when we want to go from
a hist_entry to the evlist it is in we'll use:

  hists_to_evsel(he->hists)->evlist

Right?

- Arnaldo

Re: [PATCH v1 1/4] perf annotate: create a new hists to manage multiple events samples

[Jin, Yao]

On 10/7/2017 2:54 AM, Arnaldo Carvalho de Melo wrote:
> Em Sat, Oct 07, 2017 at 12:31:37AM +0800, Jin, Yao escreveu:
>> On 10/5/2017 9:21 PM, Arnaldo Carvalho de Melo wrote:
>>> Em Wed, Aug 16, 2017 at 06:18:33PM +0800, Jin Yao escreveu:
>>>> An issue is found during using perf annotate.
> 
>>>> perf record -e cycles,branches ...
>>>> perf annotate main --stdio
> 
>>>> The result only shows cycles. It should show both cycles and
>>>> branches on the left side. It works with "--group", but need
>>>> this to work even without groups.
>>>
>>> Right, for --group we know that we'll be reading all the counters at
>>> each sample, so it all works and we can use the current design.
> 
>>> When we're not using groups tho, each sample has just one of the events
>>> and we end up with separate views.
> 
>>> Note tho that since the annotation buckets are kept per 'struct symbol'
>>> instance, this problem should be present only in the hist_entry based
>>> views, i.e. 'perf report' and 'perf top', right?
>  
>> Yes, it seems to be in hist_entry based view.
> 
> Ok.
> 
> But note that your initial statement of the problem:
> 
> <quote Ji, Yao>
> An issue is found during using perf annotate.
> 
> perf record -e cycles,branches ...
> perf annotate main --stdio
> 
> The result only shows cycles. It should show both cycles and
> branches on the left side. It works with "--group", but need
> this to work even without groups.
> </>
> 
> Can be solved right now, its just a matter of accessing the other
> buckets in a given symbol, just like we have for --group.
> 
> The only problem is in presenting a list of symbols which can be
> annotated, we have them for each evsel, and you, rightly, want to show
> the list of all symbols in all evsels.
> 
> Ok?
> 

Hi Arnaldo, I just feel there might be another problem when displaying the result.

I will talk about this in below.

>>> I.e. all struct hist_entry->ms.sym instances point to the same stuct
>>> symbol and thus will use the same annotation histogram buckets, i.e.
>>> symbol__annotation(hist_entry->ms.sym) point to the same 'struct
>>> annotation' instance, and then its a matter of passing this pointer to
>>> annotation__histogram(notes, IDX) where this IDX is perf_evsel->idx.
> 
>>> I wonder if we can't just add a new rb_node in struct hist_entry and
>>> have it in two rb_trees, i.e. in two 'struct hists' instances, one per
>>> evsel and one per evlist.
> 
>> Currently we just have per-evsel hists. This idea will create a new per-evlist hists. 
>  
>> struct perf_evlist {
>> 	......
>> 	struct hists	hists;
>> };
>  
>> And let the hist_entry be linked in both per-evsel hists and per-evlist hists. 
>  
>> Please correct me if my understanding is wrong for this idea.
> 
> Yes, do you see problems with trying to do it this way? At a first sight
> seems like it will reuse more code, no?
> 
> I.e. in hists__findnew_entry(), when not finding an existing hist_entry
> in the per-evsel hists you end up calling hist_entry__new(), right here
> you'll add it to the evsel->evlist->hists, and when we want to go from
> a hist_entry to the evlist it is in we'll use:
> 
>   hists_to_evsel(he->hists)->evlist
> 
> Right?
> 
> - Arnaldo
> 

With this method, we can get the events from per-evlist hists. 

For example, when printing the symbol annotate of 'cycles', we can also get the 'branches' from per-evlist hists. So we can print the values of both 'cycles' and 'branches' in annotate view of 'cycles'. 

The problem might be, once the annotate view of 'cycles' being printed, the view of 'branches' will be printed in next. Maybe they are the same symbol (e.g. function A), so duplicated.

The problem is that we only sort the per-evsel hists, but we don't sort the per-evlist hists.

>From my personal opinion, we may need a new sorted hists for multiple events. That will avoid the symbol duplication.

Maybe I misunderstand something, please correct me if I'm wrong.

Thanks
Jin Yao

 

Re: [PATCH v1 1/4] perf annotate: create a new hists to manage multiple events samples

[Jin, Yao]

On 10/9/2017 9:40 AM, Jin, Yao wrote:
> 
> 
> On 10/7/2017 2:54 AM, Arnaldo Carvalho de Melo wrote:
>> Em Sat, Oct 07, 2017 at 12:31:37AM +0800, Jin, Yao escreveu:
>>> On 10/5/2017 9:21 PM, Arnaldo Carvalho de Melo wrote:
>>>> Em Wed, Aug 16, 2017 at 06:18:33PM +0800, Jin Yao escreveu:
>>>>> An issue is found during using perf annotate.
>>
>>>>> perf record -e cycles,branches ...
>>>>> perf annotate main --stdio
>>
>>>>> The result only shows cycles. It should show both cycles and
>>>>> branches on the left side. It works with "--group", but need
>>>>> this to work even without groups.
>>>>
>>>> Right, for --group we know that we'll be reading all the counters at
>>>> each sample, so it all works and we can use the current design.
>>
>>>> When we're not using groups tho, each sample has just one of the events
>>>> and we end up with separate views.
>>
>>>> Note tho that since the annotation buckets are kept per 'struct symbol'
>>>> instance, this problem should be present only in the hist_entry based
>>>> views, i.e. 'perf report' and 'perf top', right?
>>   
>>> Yes, it seems to be in hist_entry based view.
>>
>> Ok.
>>
>> But note that your initial statement of the problem:
>>
>> <quote Ji, Yao>
>> An issue is found during using perf annotate.
>>
>> perf record -e cycles,branches ...
>> perf annotate main --stdio
>>
>> The result only shows cycles. It should show both cycles and
>> branches on the left side. It works with "--group", but need
>> this to work even without groups.
>> </>
>>
>> Can be solved right now, its just a matter of accessing the other
>> buckets in a given symbol, just like we have for --group.
>>
>> The only problem is in presenting a list of symbols which can be
>> annotated, we have them for each evsel, and you, rightly, want to show
>> the list of all symbols in all evsels.
>>
>> Ok?
>>
> 
> Hi Arnaldo, I just feel there might be another problem when displaying the result.
> 
> I will talk about this in below.
> 
>>>> I.e. all struct hist_entry->ms.sym instances point to the same stuct
>>>> symbol and thus will use the same annotation histogram buckets, i.e.
>>>> symbol__annotation(hist_entry->ms.sym) point to the same 'struct
>>>> annotation' instance, and then its a matter of passing this pointer to
>>>> annotation__histogram(notes, IDX) where this IDX is perf_evsel->idx.
>>
>>>> I wonder if we can't just add a new rb_node in struct hist_entry and
>>>> have it in two rb_trees, i.e. in two 'struct hists' instances, one per
>>>> evsel and one per evlist.
>>
>>> Currently we just have per-evsel hists. This idea will create a new per-evlist hists.
>>   
>>> struct perf_evlist {
>>> 	......
>>> 	struct hists	hists;
>>> };
>>   
>>> And let the hist_entry be linked in both per-evsel hists and per-evlist hists.
>>   
>>> Please correct me if my understanding is wrong for this idea.
>>
>> Yes, do you see problems with trying to do it this way? At a first sight
>> seems like it will reuse more code, no?
>>
>> I.e. in hists__findnew_entry(), when not finding an existing hist_entry
>> in the per-evsel hists you end up calling hist_entry__new(), right here
>> you'll add it to the evsel->evlist->hists, and when we want to go from
>> a hist_entry to the evlist it is in we'll use:
>>
>>    hists_to_evsel(he->hists)->evlist
>>
>> Right?
>>
>> - Arnaldo
>>
> 
> With this method, we can get the events from per-evlist hists.
> 
> For example, when printing the symbol annotate of 'cycles', we can also get the 'branches' from per-evlist hists. So we can print the values of both 'cycles' and 'branches' in annotate view of 'cycles'.
> 
> The problem might be, once the annotate view of 'cycles' being printed, the view of 'branches' will be printed in next. Maybe they are the same symbol (e.g. function A), so duplicated.
> 
> The problem is that we only sort the per-evsel hists, but we don't sort the per-evlist hists.
> 
>  From my personal opinion, we may need a new sorted hists for multiple events. That will avoid the symbol duplication.
> 
> Maybe I misunderstand something, please correct me if I'm wrong.
> 
> Thanks
> Jin Yao
> 
>   
> 

Hi Arnaldo,

Is my above explanation OK or anything I should improve?

Thanks
Jin Yao

[PATCH v1 2/4] perf annotate: Display multiple events for stdio mode

[Jin Yao]

For example:
    perf record -e cycles,branches ./div
    perf annotate main --stdio

 Percent         |      Source code & Disassembly of div for branches,cycles (90966 samples)
--------------------------------------------------------------------------------------------
    ......
                 :              for (i = 0; i < 2000000000; i++) {
                 :                      flag = compute_flag();
    5.77    4.85 :        4004e8:       xor    %eax,%eax
    0.01    0.01 :        4004ea:       callq  400640 <compute_flag>
                 :
                 :                      count++;
    2.38    4.38 :        4004ef:       mov    0x200b57(%rip),%edx
    0.00    0.00 :        4004f5:       add    $0x1,%edx
                 :
                 :                      if (flag)
    0.00    0.00 :        4004f8:       test   %eax,%eax
                 :              srand(s_randseed);
                 :
                 :              for (i = 0; i < 2000000000; i++) {
                 :                      flag = compute_flag();
                 :
                 :                      count++;
    0.60    0.44 :        4004fa:       mov    %edx,0x200b4c(%rip)
                 :
                 :                      if (flag)
    3.99    4.32 :        400500:       je     400532 <main+0x82>

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/builtin-annotate.c |   2 +-
 tools/perf/builtin-top.c      |   3 +-
 tools/perf/util/annotate.c    | 179 ++++++++++++++++++++++++++++++++----------
 tools/perf/util/annotate.h    |   6 +-
 4 files changed, 145 insertions(+), 45 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 833866c..98663bd 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -240,7 +240,7 @@ static int hist_entry__tty_annotate(struct hist_entry *he,
 				    struct perf_annotate *ann)
 {
 	return symbol__tty_annotate(he->ms.sym, he->ms.map, evsel,
-				    ann->print_line, ann->full_paths, 0, 0);
+				    ann->print_line, ann->full_paths, 0, 0, he);
 }
 
 static void hists__find_annotations(struct hists *hists,
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index ee954bd..2287667 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -245,7 +245,8 @@ static void perf_top__show_details(struct perf_top *top)
 	printf("  Events  Pcnt (>=%d%%)\n", top->sym_pcnt_filter);
 
 	more = symbol__annotate_printf(symbol, he->ms.map, top->sym_evsel,
-				       0, top->sym_pcnt_filter, top->print_entries, 4);
+				       0, top->sym_pcnt_filter,
+				       top->print_entries, 4, NULL);
 
 	if (top->evlist->enabled) {
 		if (top->zero)
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 16ec881..8630108 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1071,7 +1071,8 @@ static void annotate__branch_printf(struct block_range *br, u64 addr)
 
 static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 start,
 		      struct perf_evsel *evsel, u64 len, int min_pcnt, int printed,
-		      int max_lines, struct disasm_line *queue)
+		      int max_lines, struct disasm_line *queue,
+		      struct hist_entry *he)
 {
 	static const char *prev_line;
 	static const char *prev_color;
@@ -1082,31 +1083,39 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
 		double *ppercents = &percent;
 		struct sym_hist_entry sample;
 		struct sym_hist_entry *psamples = &sample;
-		int i, nr_percent = 1;
+		int i, nr_percent;
 		const char *color;
 		struct annotation *notes = symbol__annotation(sym);
 		s64 offset = dl->offset;
 		const u64 addr = start + offset;
 		struct disasm_line *next;
 		struct block_range *br;
+		struct hist_event *hevt;
 
 		next = disasm__get_next_ip_line(&notes->src->source, dl);
-
-		if (perf_evsel__is_group_event(evsel)) {
-			nr_percent = evsel->nr_members;
-			ppercents = calloc(nr_percent, sizeof(double));
-			psamples = calloc(nr_percent, sizeof(struct sym_hist_entry));
-			if (ppercents == NULL || psamples == NULL) {
-				return -1;
-			}
+		nr_percent = (evsel) ? 1 : he->event_nr;
+		ppercents = calloc(nr_percent, sizeof(double));
+		psamples = calloc(nr_percent, sizeof(struct sym_hist_entry));
+		if (ppercents == NULL || psamples == NULL) {
+			return -1;
 		}
 
 		for (i = 0; i < nr_percent; i++) {
-			percent = disasm__calc_percent(notes,
+			if (evsel) {
+				percent = disasm__calc_percent(notes,
 					notes->src->lines ? i : evsel->idx + i,
 					offset,
 					next ? next->offset : (s64) len,
 					&path, &sample);
+			} else {
+				hevt = &he->events[i];
+				notes = symbol__annotation(hevt->ms.sym);
+				percent = disasm__calc_percent(notes,
+					hevt->idx,
+					offset,
+					next ? next->offset : (s64) len,
+					&path, &sample);
+			}
 
 			ppercents[i] = percent;
 			psamples[i] = sample;
@@ -1125,7 +1134,7 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
 				if (queue == dl)
 					break;
 				disasm_line__print(queue, sym, start, evsel, len,
-						    0, 0, 1, NULL);
+						    0, 0, 1, NULL, he);
 			}
 		}
 
@@ -1179,7 +1188,9 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
 		if (queue)
 			return -1;
 
-		if (perf_evsel__is_group_event(evsel))
+		if (!evsel)
+			width *= he->event_nr;
+		else if (perf_evsel__is_group_event(evsel))
 			width *= evsel->nr_members;
 
 		if (!*dl->line)
@@ -1682,25 +1693,32 @@ static void symbol__free_source_line(struct symbol *sym, int len)
 /* Get the filename:line for the colored entries */
 static int symbol__get_source_line(struct symbol *sym, struct map *map,
 				   struct perf_evsel *evsel,
-				   struct rb_root *root, int len)
+				   struct rb_root *root, int len,
+				   struct hist_entry *he)
 {
 	u64 start;
 	int i, k;
-	int evidx = evsel->idx;
 	struct source_line *src_line;
 	struct annotation *notes = symbol__annotation(sym);
-	struct sym_hist *h = annotation__histogram(notes, evidx);
+	struct sym_hist *h;
 	struct rb_root tmp_root = RB_ROOT;
 	int nr_pcnt = 1;
-	u64 nr_samples = h->nr_samples;
+	u64 nr_samples = 0;
 	size_t sizeof_src_line = sizeof(struct source_line);
+	struct hist_event *hevt;
 
-	if (perf_evsel__is_group_event(evsel)) {
-		for (i = 1; i < evsel->nr_members; i++) {
-			h = annotation__histogram(notes, evidx + i);
+	if (evsel) {
+		h = annotation__histogram(notes, evsel->idx);
+		nr_samples = h->nr_samples;
+	} else {
+		for (i = 0; i < he->event_nr; i++) {
+			hevt = &he->events[i];
+			notes = symbol__annotation(hevt->ms.sym);
+			h = annotation__histogram(notes, hevt->idx);
 			nr_samples += h->nr_samples;
 		}
-		nr_pcnt = evsel->nr_members;
+
+		nr_pcnt = he->event_nr;
 		sizeof_src_line += (nr_pcnt - 1) * sizeof(src_line->samples);
 	}
 
@@ -1722,7 +1740,15 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map,
 		for (k = 0; k < nr_pcnt; k++) {
 			double percent = 0.0;
 
-			h = annotation__histogram(notes, evidx + k);
+			if (evsel) {
+				h = annotation__histogram(notes,
+							  evsel->idx + k);
+			} else {
+				hevt = &he->events[k];
+				notes = symbol__annotation(hevt->ms.sym);
+				h = annotation__histogram(notes, hevt->idx);
+			}
+
 			nr_samples = h->addr[i].nr_samples;
 			if (h->nr_samples)
 				percent = 100.0 * nr_samples / h->nr_samples;
@@ -1787,29 +1813,67 @@ static void print_summary(struct rb_root *root, const char *filename)
 	}
 }
 
-static void symbol__annotate_hits(struct symbol *sym, struct perf_evsel *evsel)
+static void symbol__annotate_hits(struct symbol *sym, struct perf_evsel *evsel,
+				  struct hist_entry *he)
 {
 	struct annotation *notes = symbol__annotation(sym);
-	struct sym_hist *h = annotation__histogram(notes, evsel->idx);
+	struct sym_hist *h;
 	u64 len = symbol__size(sym), offset;
+	int i;
+	struct hist_event *hevt;
 
-	for (offset = 0; offset < len; ++offset)
-		if (h->addr[offset].nr_samples != 0)
-			printf("%*" PRIx64 ": %" PRIu64 "\n", BITS_PER_LONG / 2,
-			       sym->start + offset, h->addr[offset].nr_samples);
-	printf("%*s: %" PRIu64 "\n", BITS_PER_LONG / 2, "h->nr_samples", h->nr_samples);
+	for (offset = 0; offset < len; ++offset) {
+		if (evsel) {
+			h = annotation__histogram(notes, evsel->idx);
+
+			if (h->addr[offset].nr_samples != 0) {
+				printf("%*" PRIx64 ": %" PRIu64 "\n",
+					BITS_PER_LONG / 2,
+					sym->start + offset,
+					h->addr[offset].nr_samples);
+			}
+		} else {
+			for (i = 0; i < he->event_nr; i++) {
+				hevt = &he->events[i];
+				notes = symbol__annotation(hevt->ms.sym);
+				h = annotation__histogram(notes, hevt->idx);
+
+				if (h->addr[offset].nr_samples != 0) {
+					printf("%*" PRIx64 ": %" PRIu64 "\n",
+						BITS_PER_LONG / 2,
+						sym->start + offset,
+						h->addr[offset].nr_samples);
+				}
+			}
+		}
+	}
+
+	if (evsel) {
+		h = annotation__histogram(notes, evsel->idx);
+		printf("%*s: %" PRIu64 "\n", BITS_PER_LONG / 2,
+			"h->nr_samples", h->nr_samples);
+	} else {
+		for (i = 0; i < he->event_nr; i++) {
+			hevt = &he->events[i];
+			notes = symbol__annotation(hevt->ms.sym);
+			h = annotation__histogram(notes, hevt->idx);
+
+			printf("%*s: %" PRIu64 "\n", BITS_PER_LONG / 2,
+				"h->nr_samples", h->nr_samples);
+		}
+	}
 }
 
 int symbol__annotate_printf(struct symbol *sym, struct map *map,
 			    struct perf_evsel *evsel, bool full_paths,
-			    int min_pcnt, int max_lines, int context)
+			    int min_pcnt, int max_lines, int context,
+			    struct hist_entry *he)
 {
 	struct dso *dso = map->dso;
 	char *filename;
 	const char *d_filename;
-	const char *evsel_name = perf_evsel__name(evsel);
 	struct annotation *notes = symbol__annotation(sym);
-	struct sym_hist *h = annotation__histogram(notes, evsel->idx);
+	struct sym_hist *h;
 	struct disasm_line *pos, *queue = NULL;
 	u64 start = map__rip_2objdump(map, sym->start);
 	int printed = 2, queue_len = 0;
@@ -1817,6 +1881,30 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
 	u64 len;
 	int width = symbol_conf.show_total_period ? 12 : 8;
 	int graph_dotted_len;
+	char name_buf[128];
+	int i, name_printed = 0;
+	u64 nr_samples = 0;
+	struct hist_event *hevt;
+
+	if (evsel) {
+		strncpy(name_buf, perf_evsel__name(evsel), sizeof(name_buf));
+		h = annotation__histogram(notes, evsel->idx);
+		nr_samples = h->nr_samples;
+
+	} else {
+		for (i = 0; i < he->event_nr; i++) {
+			hevt = &he->events[i];
+			name_printed += scnprintf(name_buf + name_printed,
+				sizeof(name_buf) - name_printed,
+				"%s%s",
+				perf_evsel__name(hevt->evsel),
+				(i < he->event_nr - 1) ? "," : "");
+
+			notes = symbol__annotation(hevt->ms.sym);
+			h = annotation__histogram(notes, hevt->idx);
+			nr_samples += h->nr_samples;
+		}
+	}
 
 	filename = strdup(dso->long_name);
 	if (!filename)
@@ -1829,18 +1917,20 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
 
 	len = symbol__size(sym);
 
-	if (perf_evsel__is_group_event(evsel))
+	if (!evsel)
+		width *= he->event_nr;
+	else if (perf_evsel__is_group_event(evsel))
 		width *= evsel->nr_members;
 
 	graph_dotted_len = printf(" %-*.*s|	Source code & Disassembly of %s for %s (%" PRIu64 " samples)\n",
 				  width, width, symbol_conf.show_total_period ? "Event count" : "Percent",
-				  d_filename, evsel_name, h->nr_samples);
+				  d_filename, name_buf, nr_samples);
 
 	printf("%-*.*s----\n",
 	       graph_dotted_len, graph_dotted_len, graph_dotted_line);
 
 	if (verbose > 0)
-		symbol__annotate_hits(sym, evsel);
+		symbol__annotate_hits(sym, evsel, he);
 
 	list_for_each_entry(pos, &notes->src->source, node) {
 		if (context && queue == NULL) {
@@ -1850,7 +1940,7 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
 
 		switch (disasm_line__print(pos, sym, start, evsel, len,
 					    min_pcnt, printed, max_lines,
-					    queue)) {
+					    queue, he)) {
 		case 0:
 			++printed;
 			if (context) {
@@ -1945,26 +2035,33 @@ size_t disasm__fprintf(struct list_head *head, FILE *fp)
 
 int symbol__tty_annotate(struct symbol *sym, struct map *map,
 			 struct perf_evsel *evsel, bool print_lines,
-			 bool full_paths, int min_pcnt, int max_lines)
+			 bool full_paths, int min_pcnt, int max_lines,
+			 struct hist_entry *he)
 {
 	struct dso *dso = map->dso;
 	struct rb_root source_line = RB_ROOT;
 	u64 len;
+	char *arch;
 
-	if (symbol__disassemble(sym, map, perf_evsel__env_arch(evsel),
+	if (!evsel)
+		arch = perf_evsel__env_arch(he->events[0].evsel);
+	else
+		arch = perf_evsel__env_arch(evsel);
+
+	if (symbol__disassemble(sym, map, arch,
 				0, NULL, NULL) < 0)
 		return -1;
-
 	len = symbol__size(sym);
 
 	if (print_lines) {
 		srcline_full_filename = full_paths;
-		symbol__get_source_line(sym, map, evsel, &source_line, len);
+		symbol__get_source_line(sym, map, evsel, &source_line, len, he);
 		print_summary(&source_line, dso->long_name);
 	}
 
 	symbol__annotate_printf(sym, map, evsel, full_paths,
-				min_pcnt, max_lines, 0);
+				min_pcnt, max_lines, 0, he);
+
 	if (print_lines)
 		symbol__free_source_line(sym, len);
 
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 0d44cfe..83b78ff 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -198,7 +198,8 @@ int symbol__strerror_disassemble(struct symbol *sym, struct map *map,
 
 int symbol__annotate_printf(struct symbol *sym, struct map *map,
 			    struct perf_evsel *evsel, bool full_paths,
-			    int min_pcnt, int max_lines, int context);
+			    int min_pcnt, int max_lines, int context,
+			    struct hist_entry *he);
 void symbol__annotate_zero_histogram(struct symbol *sym, int evidx);
 void symbol__annotate_decay_histogram(struct symbol *sym, int evidx);
 void disasm__purge(struct list_head *head);
@@ -207,7 +208,8 @@ bool ui__has_annotation(void);
 
 int symbol__tty_annotate(struct symbol *sym, struct map *map,
 			 struct perf_evsel *evsel, bool print_lines,
-			 bool full_paths, int min_pcnt, int max_lines);
+			 bool full_paths, int min_pcnt, int max_lines,
+			 struct hist_entry *he);
 
 #ifdef HAVE_SLANG_SUPPORT
 int symbol__tui_annotate(struct symbol *sym, struct map *map,
-- 
2.7.4

[PATCH v1 3/4] perf annotate: Display multiple events for tui mode

[Jin Yao]

For example:
    perf record -e cycles,branches ./div
    perf annotate main

              │            for (i = 0; i < 2000000000; i++) {
              │                    flag = compute_flag();
  5.77   4.85 │38:   xor    %eax,%eax
  0.01   0.01 │    → callq  compute_flag
              │
              │                    count++;
  2.38   4.38 │      mov    count,%edx
  0.00   0.00 │      add    $0x1,%edx
              │
              │                    if (flag)
              │      test   %eax,%eax
              │            srand(s_randseed);
              │
              │            for (i = 0; i < 2000000000; i++) {
              │                    flag = compute_flag();
              │
              │                    count++;
  0.60   0.44 │      mov    %edx,count
              │
              │                    if (flag)
  3.99   4.32 │    ↓ je     82

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/ui/browsers/annotate.c | 49 ++++++++++++++++++++++++++++-----------
 tools/perf/ui/browsers/hists.c    |  2 +-
 tools/perf/util/annotate.h        |  6 +++--
 tools/perf/util/hist.h            |  8 ++++---
 4 files changed, 45 insertions(+), 20 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 80f38da..b76c238 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -428,13 +428,15 @@ static void annotate_browser__set_rb_top(struct annotate_browser *browser,
 }
 
 static void annotate_browser__calc_percent(struct annotate_browser *browser,
-					   struct perf_evsel *evsel)
+					   struct perf_evsel *evsel,
+					   struct hist_entry *he)
 {
 	struct map_symbol *ms = browser->b.priv;
 	struct symbol *sym = ms->sym;
 	struct annotation *notes = symbol__annotation(sym);
 	struct disasm_line *pos, *next;
 	s64 len = symbol__size(sym);
+	struct hist_event *hevt;
 
 	browser->entries = RB_ROOT;
 
@@ -455,13 +457,25 @@ static void annotate_browser__calc_percent(struct annotate_browser *browser,
 
 		for (i = 0; i < browser->nr_events; i++) {
 			struct sym_hist_entry sample;
-
-			bpos->samples[i].percent = disasm__calc_percent(notes,
+			if (evsel) {
+				bpos->samples[i].percent =
+					disasm__calc_percent(notes,
 						evsel->idx + i,
 						pos->offset,
 						next ? next->offset : len,
 						&path, &sample);
-			bpos->samples[i].he = sample;
+				bpos->samples[i].he = sample;
+			} else if (he) {
+				hevt = &he->events[i];
+				notes = symbol__annotation(hevt->ms.sym);
+				bpos->samples[i].percent =
+					disasm__calc_percent(notes,
+						hevt->idx,
+						pos->offset,
+						next ? next->offset : len,
+						&path, &sample);
+				bpos->samples[i].he = sample;
+			}
 
 			if (max_percent < bpos->samples[i].percent)
 				max_percent = bpos->samples[i].percent;
@@ -567,7 +581,7 @@ static bool annotate_browser__callq(struct annotate_browser *browser,
 	}
 
 	pthread_mutex_unlock(&notes->lock);
-	symbol__tui_annotate(target.sym, target.map, evsel, hbt);
+	symbol__tui_annotate(target.sym, target.map, evsel, hbt, NULL);
 	sym_title(ms->sym, ms->map, title, sizeof(title));
 	ui_browser__show_title(&browser->b, title);
 	return true;
@@ -755,7 +769,8 @@ static void annotate_browser__update_addr_width(struct annotate_browser *browser
 
 static int annotate_browser__run(struct annotate_browser *browser,
 				 struct perf_evsel *evsel,
-				 struct hist_browser_timer *hbt)
+				 struct hist_browser_timer *hbt,
+				 struct hist_entry *he)
 {
 	struct rb_node *nd = NULL;
 	struct map_symbol *ms = browser->b.priv;
@@ -769,7 +784,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
 	if (ui_browser__show(&browser->b, title, help) < 0)
 		return -1;
 
-	annotate_browser__calc_percent(browser, evsel);
+	annotate_browser__calc_percent(browser, evsel, he);
 
 	if (browser->curr_hot) {
 		annotate_browser__set_rb_top(browser, browser->curr_hot);
@@ -782,7 +797,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
 		key = ui_browser__run(&browser->b, delay_secs);
 
 		if (delay_secs != 0) {
-			annotate_browser__calc_percent(browser, evsel);
+			annotate_browser__calc_percent(browser, evsel, he);
 			/*
 			 * Current line focus got out of the list of most active
 			 * lines, NULL it so that if TAB|UNTAB is pressed, we
@@ -929,13 +944,14 @@ static int annotate_browser__run(struct annotate_browser *browser,
 }
 
 int map_symbol__tui_annotate(struct map_symbol *ms, struct perf_evsel *evsel,
-			     struct hist_browser_timer *hbt)
+			     struct hist_browser_timer *hbt,
+			     struct hist_entry *he)
 {
 	/* Set default value for show_total_period.  */
 	annotate_browser__opts.show_total_period =
 	  symbol_conf.show_total_period;
 
-	return symbol__tui_annotate(ms->sym, ms->map, evsel, hbt);
+	return symbol__tui_annotate(ms->sym, ms->map, evsel, hbt, he);
 }
 
 int hist_entry__tui_annotate(struct hist_entry *he, struct perf_evsel *evsel,
@@ -945,7 +961,7 @@ int hist_entry__tui_annotate(struct hist_entry *he, struct perf_evsel *evsel,
 	SLang_reset_tty();
 	SLang_init_tty(0, 0, 0);
 
-	return map_symbol__tui_annotate(&he->ms, evsel, hbt);
+	return map_symbol__tui_annotate(&he->ms, evsel, hbt, he);
 }
 
 
@@ -1062,7 +1078,8 @@ static inline int width_jumps(int n)
 
 int symbol__tui_annotate(struct symbol *sym, struct map *map,
 			 struct perf_evsel *evsel,
-			 struct hist_browser_timer *hbt)
+			 struct hist_browser_timer *hbt,
+			 struct hist_entry *he)
 {
 	struct disasm_line *pos, *n;
 	struct annotation *notes;
@@ -1099,10 +1116,14 @@ int symbol__tui_annotate(struct symbol *sym, struct map *map,
 		return -1;
 	}
 
-	if (perf_evsel__is_group_event(evsel)) {
+	if (evsel && perf_evsel__is_group_event(evsel)) {
 		nr_pcnt = evsel->nr_members;
 		sizeof_bdl += sizeof(struct disasm_line_samples) *
 		  (nr_pcnt - 1);
+	} else if (he) {
+		nr_pcnt = he->event_nr;
+		sizeof_bdl += sizeof(struct disasm_line_samples) *
+		  (nr_pcnt - 1);
 	}
 
 	err = symbol__disassemble(sym, map, perf_evsel__env_arch(evsel),
@@ -1159,7 +1180,7 @@ int symbol__tui_annotate(struct symbol *sym, struct map *map,
 
 	annotate_browser__update_addr_width(&browser);
 
-	ret = annotate_browser__run(&browser, evsel, hbt);
+	ret = annotate_browser__run(&browser, evsel, hbt, he);
 	list_for_each_entry_safe(pos, n, &notes->src->source, node) {
 		list_del(&pos->node);
 		disasm_line__free(pos);
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index f4bc246..b37458d 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2570,7 +2570,7 @@ do_annotate(struct hist_browser *browser, struct popup_action *act)
 		return 0;
 
 	evsel = hists_to_evsel(browser->hists);
-	err = map_symbol__tui_annotate(&act->ms, evsel, browser->hbt);
+	err = map_symbol__tui_annotate(&act->ms, evsel, browser->hbt, NULL);
 	he = hist_browser__selected_entry(browser);
 	/*
 	 * offer option to annotate the other branch source or target
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 83b78ff..1b9ef1d 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -214,13 +214,15 @@ int symbol__tty_annotate(struct symbol *sym, struct map *map,
 #ifdef HAVE_SLANG_SUPPORT
 int symbol__tui_annotate(struct symbol *sym, struct map *map,
 			 struct perf_evsel *evsel,
-			 struct hist_browser_timer *hbt);
+			 struct hist_browser_timer *hbt,
+			 struct hist_entry *he);
 #else
 static inline int symbol__tui_annotate(struct symbol *sym __maybe_unused,
 				struct map *map __maybe_unused,
 				struct perf_evsel *evsel  __maybe_unused,
 				struct hist_browser_timer *hbt
-				__maybe_unused)
+				__maybe_unused,
+				struct hist_entry *he __maybe_unused)
 {
 	return 0;
 }
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index ee3670a..ece79a1 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -420,7 +420,8 @@ struct hist_browser_timer {
 #ifdef HAVE_SLANG_SUPPORT
 #include "../ui/keysyms.h"
 int map_symbol__tui_annotate(struct map_symbol *ms, struct perf_evsel *evsel,
-			     struct hist_browser_timer *hbt);
+			     struct hist_browser_timer *hbt,
+			     struct hist_entry *he);
 
 int hist_entry__tui_annotate(struct hist_entry *he, struct perf_evsel *evsel,
 			     struct hist_browser_timer *hbt);
@@ -441,8 +442,9 @@ int perf_evlist__tui_browse_hists(struct perf_evlist *evlist __maybe_unused,
 	return 0;
 }
 static inline int map_symbol__tui_annotate(struct map_symbol *ms __maybe_unused,
-					   struct perf_evsel *evsel __maybe_unused,
-					   struct hist_browser_timer *hbt __maybe_unused)
+			   struct perf_evsel *evsel __maybe_unused,
+			   struct hist_browser_timer *hbt __maybe_unused,
+			   struct hist_entry *he __maybe_unused)
 {
 	return 0;
 }
-- 
2.7.4

[PATCH v1 4/4] perf annotate: Display multiple events for gtk mode

[Jin Yao]

For example:
    perf record -e cycles,branches ./div
    perf annotate main --gtk

Both the cycles and branches are displayed at the left column in
gtk window.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/ui/gtk/annotate.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/tools/perf/ui/gtk/annotate.c b/tools/perf/ui/gtk/annotate.c
index 0217619..f314654 100644
--- a/tools/perf/ui/gtk/annotate.c
+++ b/tools/perf/ui/gtk/annotate.c
@@ -88,7 +88,8 @@ static int perf_gtk__get_line(char *buf, size_t size, struct disasm_line *dl)
 
 static int perf_gtk__annotate_symbol(GtkWidget *window, struct symbol *sym,
 				struct map *map, struct perf_evsel *evsel,
-				struct hist_browser_timer *hbt __maybe_unused)
+				struct hist_browser_timer *hbt __maybe_unused,
+				struct hist_entry *he)
 {
 	struct disasm_line *pos, *n;
 	struct annotation *notes;
@@ -98,6 +99,7 @@ static int perf_gtk__annotate_symbol(GtkWidget *window, struct symbol *sym,
 	GtkWidget *view;
 	int i;
 	char s[512];
+	struct hist_event *hevt;
 
 	notes = symbol__annotation(sym);
 
@@ -124,17 +126,18 @@ static int perf_gtk__annotate_symbol(GtkWidget *window, struct symbol *sym,
 
 		gtk_list_store_append(store, &iter);
 
-		if (perf_evsel__is_group_event(evsel)) {
-			for (i = 0; i < evsel->nr_members; i++) {
+		if (evsel) {
+			ret = perf_gtk__get_percent(s, sizeof(s), sym, pos,
+						    evsel->idx);
+		} else if (he) {
+			for (i = 0; i < he->event_nr; i++) {
+				hevt = &he->events[i];
 				ret += perf_gtk__get_percent(s + ret,
 							     sizeof(s) - ret,
-							     sym, pos,
-							     evsel->idx + i);
+							     hevt->ms.sym, pos,
+							     hevt->idx);
 				ret += scnprintf(s + ret, sizeof(s) - ret, " ");
 			}
-		} else {
-			ret = perf_gtk__get_percent(s, sizeof(s), sym, pos,
-						    evsel->idx);
 		}
 
 		if (ret)
@@ -157,19 +160,25 @@ static int perf_gtk__annotate_symbol(GtkWidget *window, struct symbol *sym,
 
 static int symbol__gtk_annotate(struct symbol *sym, struct map *map,
 				struct perf_evsel *evsel,
-				struct hist_browser_timer *hbt)
+				struct hist_browser_timer *hbt,
+				struct hist_entry *he)
 {
 	GtkWidget *window;
 	GtkWidget *notebook;
 	GtkWidget *scrolled_window;
 	GtkWidget *tab_label;
 	int err;
+	char *arch;
+
+	if (!evsel)
+		arch = perf_evsel__env_arch(he->events[0].evsel);
+	else
+		arch = perf_evsel__env_arch(evsel);
 
 	if (map->dso->annotate_warned)
 		return -1;
 
-	err = symbol__disassemble(sym, map, perf_evsel__env_arch(evsel),
-				  0, NULL, NULL);
+	err = symbol__disassemble(sym, map, arch, 0, NULL, NULL);
 	if (err) {
 		char msg[BUFSIZ];
 		symbol__strerror_disassemble(sym, map, err, msg, sizeof(msg));
@@ -228,7 +237,7 @@ static int symbol__gtk_annotate(struct symbol *sym, struct map *map,
 	gtk_notebook_append_page(GTK_NOTEBOOK(notebook), scrolled_window,
 				 tab_label);
 
-	perf_gtk__annotate_symbol(scrolled_window, sym, map, evsel, hbt);
+	perf_gtk__annotate_symbol(scrolled_window, sym, map, evsel, hbt, he);
 	return 0;
 }
 
@@ -236,7 +245,7 @@ int hist_entry__gtk_annotate(struct hist_entry *he,
 			     struct perf_evsel *evsel,
 			     struct hist_browser_timer *hbt)
 {
-	return symbol__gtk_annotate(he->ms.sym, he->ms.map, evsel, hbt);
+	return symbol__gtk_annotate(he->ms.sym, he->ms.map, evsel, hbt, he);
 }
 
 void perf_gtk__show_annotations(void)
-- 
2.7.4

Re: [PATCH v1 0/4] perf annotate: Display multiple events on the left side of annotate view

[Jin, Yao]

Hi,

Any comments for this patch-set?

I just find a merge conflict with the latest perf/core.

Do I need to send the v2 to fix the merge issue right now?

Thanks

Jin Yao

On 8/16/2017 6:18 PM, Jin Yao wrote:
> perf record -e cycles,branches ...
> perf annotate main --stdio
>
> The result only shows cycles. It should show both cycles and
> branches on the left side of annotate view. It works with
> "--group", but need this to work even without groups.
>
> The patch series supports to display multiple events on the
> left side of annotate view for stdio, tui and gtk modes.
>
> Jin Yao (4):
>    perf annotate: create a new hists to manage multiple events samples
>    perf annotate: Display multiple events for stdio mode
>    perf annotate: Display multiple events for tui mode
>    perf annotate: Display multiple events for gtk mode
>
>   tools/perf/builtin-annotate.c     |  62 +++++++++----
>   tools/perf/builtin-top.c          |   3 +-
>   tools/perf/ui/browsers/annotate.c |  49 +++++++---
>   tools/perf/ui/browsers/hists.c    |   2 +-
>   tools/perf/ui/gtk/annotate.c      |  35 ++++---
>   tools/perf/util/annotate.c        | 187 +++++++++++++++++++++++++++++---------
>   tools/perf/util/annotate.h        |  16 +++-
>   tools/perf/util/hist.h            |   8 +-
>   tools/perf/util/sort.c            |  21 +++++
>   tools/perf/util/sort.h            |  13 +++
>   10 files changed, 301 insertions(+), 95 deletions(-)
>