1. Patchew
  2. linux
  3. [PATCH 0/4] perf report: Omit dummy events in the output (v2)
  4. View patch

[PATCH 2/4] perf hist: Simplify __hpp_fmt() using hpp_fmt_data

Namhyung Kim posted 4 patches 1 year, 8 months ago
[PATCH 2/4] perf hist: Simplify __hpp_fmt() using hpp_fmt_data
Posted by Namhyung Kim 1 year, 8 months ago 
The struct hpp_fmt_data is to keep the values for each group members so
it doesn't need to check the event index in the group.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/hist.c | 75 +++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 39 deletions(-)

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index e30fcb1e87e7..539978c95cfd 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -46,65 +46,62 @@ static int __hpp__fmt_print(struct perf_hpp *hpp, struct hists *hists, u64 val,
 	return hpp__call_print_fn(hpp, print_fn, fmt, len, val);
 }
 
+struct hpp_fmt_data {
+	struct hists *hists;
+	u64 val;
+	int samples;
+};
+
 static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
 		      hpp_field_fn get_field, const char *fmt, int len,
 		      hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
 {
-	int ret;
+	int ret = 0;
 	struct hists *hists = he->hists;
 	struct evsel *evsel = hists_to_evsel(hists);
+	struct evsel *pos;
 	char *buf = hpp->buf;
 	size_t size = hpp->size;
+	int i, nr_members = 1;
+	struct hpp_fmt_data *data;
+
+	if (evsel__is_group_event(evsel))
+		nr_members = evsel->core.nr_members;
+
+	data = calloc(nr_members, sizeof(*data));
+	if (data == NULL)
+		return 0;
+
+	i = 0;
+	for_each_group_evsel(pos, evsel)
+		data[i++].hists = evsel__hists(pos);
 
-	ret = __hpp__fmt_print(hpp, hists, get_field(he), he->stat.nr_events,
-			       fmt, len, print_fn, fmtype);
+	data[0].val = get_field(he);
+	data[0].samples = he->stat.nr_events;
 
 	if (evsel__is_group_event(evsel)) {
-		int prev_idx, idx_delta;
 		struct hist_entry *pair;
-		int nr_members = evsel->core.nr_members;
-
-		prev_idx = evsel__group_idx(evsel);
 
 		list_for_each_entry(pair, &he->pairs.head, pairs.node) {
-			u64 period = get_field(pair);
-			u64 total = hists__total_period(pair->hists);
-			int nr_samples = pair->stat.nr_events;
-
-			if (!total)
-				continue;
+			for (i = 0; i < nr_members; i++) {
+				if (data[i].hists != pair->hists)
+					continue;
 
-			evsel = hists_to_evsel(pair->hists);
-			idx_delta = evsel__group_idx(evsel) - prev_idx - 1;
-
-			while (idx_delta--) {
-				/*
-				 * zero-fill group members in the middle which have
-				 * no samples, pair->hists is not correct but it's
-				 * fine since the value is 0.
-				 */
-				ret += __hpp__fmt_print(hpp, pair->hists, 0, 0,
-							fmt, len, print_fn, fmtype);
+				data[i].val = get_field(pair);
+				data[i].samples = pair->stat.nr_events;
+				break;
 			}
-
-			ret += __hpp__fmt_print(hpp, pair->hists, period, nr_samples,
-						fmt, len, print_fn, fmtype);
-
-			prev_idx = evsel__group_idx(evsel);
 		}
+	}
 
-		idx_delta = nr_members - prev_idx - 1;
-
-		while (idx_delta--) {
-			/*
-			 * zero-fill group members at last which have no sample.
-			 * the hists is not correct but it's fine like above.
-			 */
-			ret += __hpp__fmt_print(hpp, evsel__hists(evsel), 0, 0,
-						fmt, len, print_fn, fmtype);
-		}
+	for (i = 0; i < nr_members; i++) {
+		ret += __hpp__fmt_print(hpp, data[i].hists, data[i].val,
+					data[i].samples, fmt, len,
+					print_fn, fmtype);
 	}
 
+	free(data);
+
 	/*
 	 * Restore original buf and size as it's where caller expects
 	 * the result will be saved.
-- 
2.45.1.288.g0e0cd299f1-goog
Re: [PATCH 2/4] perf hist: Simplify __hpp_fmt() using hpp_fmt_data
Posted by Arnaldo Carvalho de Melo 1 year, 8 months ago 
On Mon, Jun 03, 2024 at 03:44:10PM -0700, Namhyung Kim wrote:
> The struct hpp_fmt_data is to keep the values for each group members so
> it doesn't need to check the event index in the group.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/hist.c | 75 +++++++++++++++++++++-----------------------
>  1 file changed, 36 insertions(+), 39 deletions(-)
> 
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index e30fcb1e87e7..539978c95cfd 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -46,65 +46,62 @@ static int __hpp__fmt_print(struct perf_hpp *hpp, struct hists *hists, u64 val,
>  	return hpp__call_print_fn(hpp, print_fn, fmt, len, val);
>  }
>  
> +struct hpp_fmt_data {
> +	struct hists *hists;
> +	u64 val;
> +	int samples;
> +};

Can we try to avoid vague terms like 'data' and use a hopefully more
clear 'hpp_fmt_value' instead?

>  static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
>  		      hpp_field_fn get_field, const char *fmt, int len,
>  		      hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
>  {
> -	int ret;
> +	int ret = 0;
>  	struct hists *hists = he->hists;
>  	struct evsel *evsel = hists_to_evsel(hists);
> +	struct evsel *pos;
>  	char *buf = hpp->buf;
>  	size_t size = hpp->size;
> +	int i, nr_members = 1;
> +	struct hpp_fmt_data *data;

Here we then use:

	struct hpp_fmt_value *values;

> +
> +	if (evsel__is_group_event(evsel))
> +		nr_members = evsel->core.nr_members;
> +
> +	data = calloc(nr_members, sizeof(*data));
> +	if (data == NULL)
> +		return 0;


> +
> +	i = 0;
> +	for_each_group_evsel(pos, evsel)
> +		data[i++].hists = evsel__hists(pos);
>  
> -	ret = __hpp__fmt_print(hpp, hists, get_field(he), he->stat.nr_events,
> -			       fmt, len, print_fn, fmtype);
> +	data[0].val = get_field(he);
> +	data[0].samples = he->stat.nr_events;
>  
>  	if (evsel__is_group_event(evsel)) {
> -		int prev_idx, idx_delta;
>  		struct hist_entry *pair;
> -		int nr_members = evsel->core.nr_members;
> -
> -		prev_idx = evsel__group_idx(evsel);
>  
>  		list_for_each_entry(pair, &he->pairs.head, pairs.node) {
> -			u64 period = get_field(pair);
> -			u64 total = hists__total_period(pair->hists);
> -			int nr_samples = pair->stat.nr_events;
> -
> -			if (!total)
> -				continue;
> +			for (i = 0; i < nr_members; i++) {
> +				if (data[i].hists != pair->hists)
> +					continue;
>  
> -			evsel = hists_to_evsel(pair->hists);
> -			idx_delta = evsel__group_idx(evsel) - prev_idx - 1;
> -
> -			while (idx_delta--) {
> -				/*
> -				 * zero-fill group members in the middle which have
> -				 * no samples, pair->hists is not correct but it's
> -				 * fine since the value is 0.
> -				 */
> -				ret += __hpp__fmt_print(hpp, pair->hists, 0, 0,
> -							fmt, len, print_fn, fmtype);
> +				data[i].val = get_field(pair);
> +				data[i].samples = pair->stat.nr_events;
> +				break;
>  			}
> -
> -			ret += __hpp__fmt_print(hpp, pair->hists, period, nr_samples,
> -						fmt, len, print_fn, fmtype);
> -
> -			prev_idx = evsel__group_idx(evsel);
>  		}
> +	}
>  
> -		idx_delta = nr_members - prev_idx - 1;
> -
> -		while (idx_delta--) {
> -			/*
> -			 * zero-fill group members at last which have no sample.
> -			 * the hists is not correct but it's fine like above.
> -			 */
> -			ret += __hpp__fmt_print(hpp, evsel__hists(evsel), 0, 0,
> -						fmt, len, print_fn, fmtype);
> -		}
> +	for (i = 0; i < nr_members; i++) {
> +		ret += __hpp__fmt_print(hpp, data[i].hists, data[i].val,
> +					data[i].samples, fmt, len,
> +					print_fn, fmtype);
>  	}
>  
> +	free(data);
> +
>  	/*
>  	 * Restore original buf and size as it's where caller expects
>  	 * the result will be saved.
> -- 
> 2.45.1.288.g0e0cd299f1-goog
Re: [PATCH 2/4] perf hist: Simplify __hpp_fmt() using hpp_fmt_data
Posted by Namhyung Kim 1 year, 8 months ago 
On Tue, Jun 04, 2024 at 12:18:47PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Jun 03, 2024 at 03:44:10PM -0700, Namhyung Kim wrote:
> > The struct hpp_fmt_data is to keep the values for each group members so
> > it doesn't need to check the event index in the group.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/ui/hist.c | 75 +++++++++++++++++++++-----------------------
> >  1 file changed, 36 insertions(+), 39 deletions(-)
> > 
> > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > index e30fcb1e87e7..539978c95cfd 100644
> > --- a/tools/perf/ui/hist.c
> > +++ b/tools/perf/ui/hist.c
> > @@ -46,65 +46,62 @@ static int __hpp__fmt_print(struct perf_hpp *hpp, struct hists *hists, u64 val,
> >  	return hpp__call_print_fn(hpp, print_fn, fmt, len, val);
> >  }
> >  
> > +struct hpp_fmt_data {
> > +	struct hists *hists;
> > +	u64 val;
> > +	int samples;
> > +};
> 
> Can we try to avoid vague terms like 'data' and use a hopefully more
> clear 'hpp_fmt_value' instead?

Sure, I can do that.  I thought 'data' was better since it contains more
than a value like a pointer to hist and number of samples.  But it's not
a big deal, and I can make the change.

> 
> >  static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
> >  		      hpp_field_fn get_field, const char *fmt, int len,
> >  		      hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
> >  {
> > -	int ret;
> > +	int ret = 0;
> >  	struct hists *hists = he->hists;
> >  	struct evsel *evsel = hists_to_evsel(hists);
> > +	struct evsel *pos;
> >  	char *buf = hpp->buf;
> >  	size_t size = hpp->size;
> > +	int i, nr_members = 1;
> > +	struct hpp_fmt_data *data;
> 
> Here we then use:
> 
> 	struct hpp_fmt_value *values;

Yep, will change in v3.

Thanks,
Namhyung

> 
> > +
> > +	if (evsel__is_group_event(evsel))
> > +		nr_members = evsel->core.nr_members;
> > +
> > +	data = calloc(nr_members, sizeof(*data));
> > +	if (data == NULL)
> > +		return 0;
> 
> 
> > +
> > +	i = 0;
> > +	for_each_group_evsel(pos, evsel)
> > +		data[i++].hists = evsel__hists(pos);
> >  
> > -	ret = __hpp__fmt_print(hpp, hists, get_field(he), he->stat.nr_events,
> > -			       fmt, len, print_fn, fmtype);
> > +	data[0].val = get_field(he);
> > +	data[0].samples = he->stat.nr_events;
> >  
> >  	if (evsel__is_group_event(evsel)) {
> > -		int prev_idx, idx_delta;
> >  		struct hist_entry *pair;
> > -		int nr_members = evsel->core.nr_members;
> > -
> > -		prev_idx = evsel__group_idx(evsel);
> >  
> >  		list_for_each_entry(pair, &he->pairs.head, pairs.node) {
> > -			u64 period = get_field(pair);
> > -			u64 total = hists__total_period(pair->hists);
> > -			int nr_samples = pair->stat.nr_events;
> > -
> > -			if (!total)
> > -				continue;
> > +			for (i = 0; i < nr_members; i++) {
> > +				if (data[i].hists != pair->hists)
> > +					continue;
> >  
> > -			evsel = hists_to_evsel(pair->hists);
> > -			idx_delta = evsel__group_idx(evsel) - prev_idx - 1;
> > -
> > -			while (idx_delta--) {
> > -				/*
> > -				 * zero-fill group members in the middle which have
> > -				 * no samples, pair->hists is not correct but it's
> > -				 * fine since the value is 0.
> > -				 */
> > -				ret += __hpp__fmt_print(hpp, pair->hists, 0, 0,
> > -							fmt, len, print_fn, fmtype);
> > +				data[i].val = get_field(pair);
> > +				data[i].samples = pair->stat.nr_events;
> > +				break;
> >  			}
> > -
> > -			ret += __hpp__fmt_print(hpp, pair->hists, period, nr_samples,
> > -						fmt, len, print_fn, fmtype);
> > -
> > -			prev_idx = evsel__group_idx(evsel);
> >  		}
> > +	}
> >  
> > -		idx_delta = nr_members - prev_idx - 1;
> > -
> > -		while (idx_delta--) {
> > -			/*
> > -			 * zero-fill group members at last which have no sample.
> > -			 * the hists is not correct but it's fine like above.
> > -			 */
> > -			ret += __hpp__fmt_print(hpp, evsel__hists(evsel), 0, 0,
> > -						fmt, len, print_fn, fmtype);
> > -		}
> > +	for (i = 0; i < nr_members; i++) {
> > +		ret += __hpp__fmt_print(hpp, data[i].hists, data[i].val,
> > +					data[i].samples, fmt, len,
> > +					print_fn, fmtype);
> >  	}
> >  
> > +	free(data);
> > +
> >  	/*
> >  	 * Restore original buf and size as it's where caller expects
> >  	 * the result will be saved.
> > -- 
> > 2.45.1.288.g0e0cd299f1-goog

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.