Re: [PATCH v7 1/3] perf report: Change sort order by a specified event in group

From: "Jin, Yao" <yao.jin@linux.intel.com>
To: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: jolsa@kernel.org, peterz@infradead.org, mingo@redhat.com,
	alexander.shishkin@linux.intel.com, Linux-kernel@vger.kernel.org,
	ak@linux.intel.com, kan.liang@intel.com, yao.jin@intel.com
Subject: Re: [PATCH v7 1/3] perf report: Change sort order by a specified event in group
Date: Thu, 19 Mar 2020 09:14:26 +0800	[thread overview]
Message-ID: <5ac67a9a-49e8-ba0d-2adf-5b2fd939332f@linux.intel.com> (raw)
In-Reply-To: <20200318184535.GK11531@kernel.org>

On 3/19/2020 2:45 AM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Feb 20, 2020 at 09:36:14AM +0800, Jin Yao escreveu:
>> When performing "perf report --group", it shows the event group information
>> together. By default, the output is sorted by the first event in group.
>>
>> It would be nice for user to select any event for sorting. This patch
>> introduces a new option "--group-sort-idx" to sort the output by the
>> event at the index n in event group.
>>
>> For example,
>>
>> Before:
> 
> Tested and applied, I also did the following simplifications, to try and
> follow naming conventions and use less lines to do the same thing:
> 
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index 35224b366305..025f4c7f96bf 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -151,44 +151,35 @@ static int field_cmp(u64 field_a, u64 field_b)
>   	return 0;
>   }
>   
> -static int pair_fields_alloc(struct hist_entry *a, struct hist_entry *b,
> -			     hpp_field_fn get_field, int nr_members,
> -			     u64 **fields_a, u64 **fields_b)
> +static int hist_entry__new_pair(struct hist_entry *a, struct hist_entry *b,
> +				hpp_field_fn get_field, int nr_members,
> +				u64 **fields_a, u64 **fields_b)
>   {
> -	struct evsel *evsel;
> +	u64 *fa = calloc(nr_members, sizeof(*fa)),
> +	    *fb = calloc(nr_members, sizeof(*fb));
>   	struct hist_entry *pair;
> -	u64 *fa, *fb;
> -	int ret = -1;
> -
> -	fa = calloc(nr_members, sizeof(*fa));
> -	fb = calloc(nr_members, sizeof(*fb));
>   
>   	if (!fa || !fb)
> -		goto out;
> +		goto out_free;
>   
>   	list_for_each_entry(pair, &a->pairs.head, pairs.node) {
> -		evsel = hists_to_evsel(pair->hists);
> +		struct evsel *evsel = hists_to_evsel(pair->hists);
>   		fa[perf_evsel__group_idx(evsel)] = get_field(pair);
>   	}
>   
>   	list_for_each_entry(pair, &b->pairs.head, pairs.node) {
> -		evsel = hists_to_evsel(pair->hists);
> +		struct evsel *evsel = hists_to_evsel(pair->hists);
>   		fb[perf_evsel__group_idx(evsel)] = get_field(pair);
>   	}
>   
>   	*fields_a = fa;
>   	*fields_b = fb;
> -	ret = 0;
> -
> -out:
> -	if (ret != 0) {
> -		free(fa);
> -		free(fb);
> -		*fields_a = NULL;
> -		*fields_b = NULL;
> -	}
> -
> -	return ret;
> +	return 0;
> +out_free:
> +	free(fa);
> +	free(fb);
> +	*fields_a = *fields_b = NULL;
> +	return -1;
>   }
>   
>   static int __hpp__group_sort_idx(struct hist_entry *a, struct hist_entry *b,
> @@ -206,8 +197,7 @@ static int __hpp__group_sort_idx(struct hist_entry *a, struct hist_entry *b,
>   	if (idx < 1 || idx >= nr_members)
>   		return cmp;
>   
> -	ret = pair_fields_alloc(a, b, get_field, nr_members,
> -				&fields_a, &fields_b);
> +	ret = hist_entry__new_pair(a, b, get_field, nr_members, &fields_a, &fields_b);
>   	if (ret) {
>   		ret = cmp;
>   		goto out;
> @@ -254,8 +244,7 @@ static int __hpp__sort(struct hist_entry *a, struct hist_entry *b,
>   		return ret;
>   
>   	nr_members = evsel->core.nr_members;
> -	i = pair_fields_alloc(a, b, get_field, nr_members,
> -			      &fields_a, &fields_b);
> +	i = hist_entry__new_pair(a, b, get_field, nr_members, &fields_a, &fields_b);
>   	if (i)
>   		goto out;
>   
> 

Thanks for refining the patch!

Thanks
Jin Yao