Split the logic to print the histogram values according to the format
string.  This was used in 3 different places so it's better to move out
the logic into a function.

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

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 685ba2a54fd8..e30fcb1e87e7 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -23,35 +23,42 @@
 	__ret;							\
 })
 
-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)
+static int __hpp__fmt_print(struct perf_hpp *hpp, struct hists *hists, u64 val,
+			    int nr_samples, const char *fmt, int len,
+			    hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
 {
-	int ret;
-	struct hists *hists = he->hists;
-	struct evsel *evsel = hists_to_evsel(hists);
-	char *buf = hpp->buf;
-	size_t size = hpp->size;
-
 	if (fmtype == PERF_HPP_FMT_TYPE__PERCENT) {
 		double percent = 0.0;
 		u64 total = hists__total_period(hists);
 
 		if (total)
-			percent = 100.0 * get_field(he) / total;
+			percent = 100.0 * val / total;
 
-		ret = hpp__call_print_fn(hpp, print_fn, fmt, len, percent);
-	} else if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
-		double average = 0;
+		return hpp__call_print_fn(hpp, print_fn, fmt, len, percent);
+	}
 
-		if (he->stat.nr_events)
-			average = 1.0 * get_field(he) / he->stat.nr_events;
+	if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
+		double avg = nr_samples ? (1.0 * val / nr_samples) : 0;
 
-		ret = hpp__call_print_fn(hpp, print_fn, fmt, len, average);
-	} else {
-		ret = hpp__call_print_fn(hpp, print_fn, fmt, len, get_field(he));
+		return hpp__call_print_fn(hpp, print_fn, fmt, len, avg);
 	}
 
+	return hpp__call_print_fn(hpp, print_fn, fmt, len, val);
+}
+
+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;
+	struct hists *hists = he->hists;
+	struct evsel *evsel = hists_to_evsel(hists);
+	char *buf = hpp->buf;
+	size_t size = hpp->size;
+
+	ret = __hpp__fmt_print(hpp, hists, get_field(he), he->stat.nr_events,
+			       fmt, len, print_fn, fmtype);
+
 	if (evsel__is_group_event(evsel)) {
 		int prev_idx, idx_delta;
 		struct hist_entry *pair;
@@ -72,30 +79,16 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
 
 			while (idx_delta--) {
 				/*
-				 * zero-fill group members in the middle which
-				 * have no sample
+				 * 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.
 				 */
-				if (fmtype != PERF_HPP_FMT_TYPE__RAW) {
-					ret += hpp__call_print_fn(hpp, print_fn,
-								  fmt, len, 0.0);
-				} else {
-					ret += hpp__call_print_fn(hpp, print_fn,
-								  fmt, len, 0ULL);
-				}
+				ret += __hpp__fmt_print(hpp, pair->hists, 0, 0,
+							fmt, len, print_fn, fmtype);
 			}
 
-			if (fmtype == PERF_HPP_FMT_TYPE__PERCENT) {
-				ret += hpp__call_print_fn(hpp, print_fn, fmt, len,
-							  100.0 * period / total);
-			} else if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
-				double avg = nr_samples ? (period / nr_samples) : 0;
-
-				ret += hpp__call_print_fn(hpp, print_fn, fmt,
-							  len, avg);
-			} else {
-				ret += hpp__call_print_fn(hpp, print_fn, fmt,
-							  len, period);
-			}
+			ret += __hpp__fmt_print(hpp, pair->hists, period, nr_samples,
+						fmt, len, print_fn, fmtype);
 
 			prev_idx = evsel__group_idx(evsel);
 		}
@@ -104,15 +97,11 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
 
 		while (idx_delta--) {
 			/*
-			 * zero-fill group members at last which have no sample
+			 * zero-fill group members at last which have no sample.
+			 * the hists is not correct but it's fine like above.
 			 */
-			if (fmtype != PERF_HPP_FMT_TYPE__RAW) {
-				ret += hpp__call_print_fn(hpp, print_fn,
-							  fmt, len, 0.0);
-			} else {
-				ret += hpp__call_print_fn(hpp, print_fn,
-							  fmt, len, 0ULL);
-			}
+			ret += __hpp__fmt_print(hpp, evsel__hists(evsel), 0, 0,
+						fmt, len, print_fn, fmtype);
 		}
 	}
 
-- 
2.45.1.288.g0e0cd299f1-goog

On Mon, Jun 03, 2024 at 03:44:09PM -0700, Namhyung Kim wrote:
> Split the logic to print the histogram values according to the format
> string.  This was used in 3 different places so it's better to move out
> the logic into a function.

But are all really equivalent? I had difficulty following it, perhaps it
would be better to introduce the function and then go on making one by
one use it instead of doing it all at once?

So in the end we have:

static int __hpp__fmt_print(struct perf_hpp *hpp, struct hists *hists, u64 val,
			    int nr_samples, const char *fmt, int len,
			    hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
{
	if (fmtype == PERF_HPP_FMT_TYPE__PERCENT) {
		double percent = 0.0;
		u64 total = hists__total_period(hists);

		if (total)
			percent = 100.0 * val / total;

		return hpp__call_print_fn(hpp, print_fn, fmt, len, percent);
	}
	if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
		double avg = nr_samples ? (1.0 * val / nr_samples) : 0;

		return hpp__call_print_fn(hpp, print_fn, fmt, len, avg);
	}

	return hpp__call_print_fn(hpp, print_fn, fmt, len, val);
}

I.e. we do something with the last arg for hpp__call_print_fn() for the
PERF_HPP_FMT_TYPE__PERCENT and PERF_HPP_FMT_TYPE__AVERAGE fmtype's, but
we don't check for those fmtypes on each of the places that we now call
__hpp__fmt_print(), right?

That while (idx_delta--) cases is only interested in !=PERF_HPP_FMT_TYPE__RAW...

Nah, after trying to describe the confusion I think I clarified it,
probably leave it as you did :-\

Continuing...

- Arnaldo
 
> +++ b/tools/perf/ui/hist.c
> @@ -23,35 +23,42 @@
>  	__ret;							\
>  })
>  
> -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)
> +static int __hpp__fmt_print(struct perf_hpp *hpp, struct hists *hists, u64 val,
> +			    int nr_samples, const char *fmt, int len,
> +			    hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
>  {
> -	int ret;
> -	struct hists *hists = he->hists;
> -	struct evsel *evsel = hists_to_evsel(hists);
> -	char *buf = hpp->buf;
> -	size_t size = hpp->size;
> -
>  	if (fmtype == PERF_HPP_FMT_TYPE__PERCENT) {
>  		double percent = 0.0;
>  		u64 total = hists__total_period(hists);
>  
>  		if (total)
> -			percent = 100.0 * get_field(he) / total;
> +			percent = 100.0 * val / total;
>  
> -		ret = hpp__call_print_fn(hpp, print_fn, fmt, len, percent);
> -	} else if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
> -		double average = 0;
> +		return hpp__call_print_fn(hpp, print_fn, fmt, len, percent);
> +	}
>  
> -		if (he->stat.nr_events)
> -			average = 1.0 * get_field(he) / he->stat.nr_events;
> +	if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
> +		double avg = nr_samples ? (1.0 * val / nr_samples) : 0;
>  
> -		ret = hpp__call_print_fn(hpp, print_fn, fmt, len, average);
> -	} else {
> -		ret = hpp__call_print_fn(hpp, print_fn, fmt, len, get_field(he));
> +		return hpp__call_print_fn(hpp, print_fn, fmt, len, avg);
>  	}
>  
> +	return hpp__call_print_fn(hpp, print_fn, fmt, len, val);
> +}
> +
> +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;
> +	struct hists *hists = he->hists;
> +	struct evsel *evsel = hists_to_evsel(hists);
> +	char *buf = hpp->buf;
> +	size_t size = hpp->size;
> +
> +	ret = __hpp__fmt_print(hpp, hists, get_field(he), he->stat.nr_events,
> +			       fmt, len, print_fn, fmtype);
> +
>  	if (evsel__is_group_event(evsel)) {
>  		int prev_idx, idx_delta;
>  		struct hist_entry *pair;
> @@ -72,30 +79,16 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
>  
>  			while (idx_delta--) {
>  				/*
> -				 * zero-fill group members in the middle which
> -				 * have no sample
> +				 * 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.
>  				 */
> -				if (fmtype != PERF_HPP_FMT_TYPE__RAW) {
> -					ret += hpp__call_print_fn(hpp, print_fn,
> -								  fmt, len, 0.0);
> -				} else {
> -					ret += hpp__call_print_fn(hpp, print_fn,
> -								  fmt, len, 0ULL);
> -				}
> +				ret += __hpp__fmt_print(hpp, pair->hists, 0, 0,
> +							fmt, len, print_fn, fmtype);
>  			}
>  
> -			if (fmtype == PERF_HPP_FMT_TYPE__PERCENT) {
> -				ret += hpp__call_print_fn(hpp, print_fn, fmt, len,
> -							  100.0 * period / total);
> -			} else if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
> -				double avg = nr_samples ? (period / nr_samples) : 0;
> -
> -				ret += hpp__call_print_fn(hpp, print_fn, fmt,
> -							  len, avg);
> -			} else {
> -				ret += hpp__call_print_fn(hpp, print_fn, fmt,
> -							  len, period);
> -			}
> +			ret += __hpp__fmt_print(hpp, pair->hists, period, nr_samples,
> +						fmt, len, print_fn, fmtype);
>  
>  			prev_idx = evsel__group_idx(evsel);
>  		}
> @@ -104,15 +97,11 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
>  
>  		while (idx_delta--) {
>  			/*
> -			 * zero-fill group members at last which have no sample
> +			 * zero-fill group members at last which have no sample.
> +			 * the hists is not correct but it's fine like above.
>  			 */
> -			if (fmtype != PERF_HPP_FMT_TYPE__RAW) {
> -				ret += hpp__call_print_fn(hpp, print_fn,
> -							  fmt, len, 0.0);
> -			} else {
> -				ret += hpp__call_print_fn(hpp, print_fn,
> -							  fmt, len, 0ULL);
> -			}
> +			ret += __hpp__fmt_print(hpp, evsel__hists(evsel), 0, 0,
> +						fmt, len, print_fn, fmtype);
>  		}
>  	}
>  
> -- 
> 2.45.1.288.g0e0cd299f1-goog

On Tue, Jun 04, 2024 at 12:07:41PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Jun 03, 2024 at 03:44:09PM -0700, Namhyung Kim wrote:
> > Split the logic to print the histogram values according to the format
> > string.  This was used in 3 different places so it's better to move out
> > the logic into a function.
> 
> But are all really equivalent? I had difficulty following it, perhaps it
> would be better to introduce the function and then go on making one by
> one use it instead of doing it all at once?

Sorry, I agree the code looks confusing.  But they are equivalent and
just to print the value in a different way.

> 
> So in the end we have:
> 
> static int __hpp__fmt_print(struct perf_hpp *hpp, struct hists *hists, u64 val,
> 			    int nr_samples, const char *fmt, int len,
> 			    hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
> {
> 	if (fmtype == PERF_HPP_FMT_TYPE__PERCENT) {
> 		double percent = 0.0;
> 		u64 total = hists__total_period(hists);
> 
> 		if (total)
> 			percent = 100.0 * val / total;
> 
> 		return hpp__call_print_fn(hpp, print_fn, fmt, len, percent);
> 	}
> 	if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
> 		double avg = nr_samples ? (1.0 * val / nr_samples) : 0;
> 
> 		return hpp__call_print_fn(hpp, print_fn, fmt, len, avg);
> 	}
> 
> 	return hpp__call_print_fn(hpp, print_fn, fmt, len, val);
> }
> 
> I.e. we do something with the last arg for hpp__call_print_fn() for the
> PERF_HPP_FMT_TYPE__PERCENT and PERF_HPP_FMT_TYPE__AVERAGE fmtype's, but
> we don't check for those fmtypes on each of the places that we now call
> __hpp__fmt_print(), right?
> 
> That while (idx_delta--) cases is only interested in !=PERF_HPP_FMT_TYPE__RAW...
> 
> Nah, after trying to describe the confusion I think I clarified it,
> probably leave it as you did :-\

:)

The 'while (idx_delta--)' part is just to fill the gap for entries
without samples.  The value is always 0, so it only cares the data type
(double or u64) in the format string.  RAW needs u64 while PERCENT and
AVERAGE need double.

Thanks,
Namhyung

  
> > +++ b/tools/perf/ui/hist.c
> > @@ -23,35 +23,42 @@
> >  	__ret;							\
> >  })
> >  
> > -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)
> > +static int __hpp__fmt_print(struct perf_hpp *hpp, struct hists *hists, u64 val,
> > +			    int nr_samples, const char *fmt, int len,
> > +			    hpp_snprint_fn print_fn, enum perf_hpp_fmt_type fmtype)
> >  {
> > -	int ret;
> > -	struct hists *hists = he->hists;
> > -	struct evsel *evsel = hists_to_evsel(hists);
> > -	char *buf = hpp->buf;
> > -	size_t size = hpp->size;
> > -
> >  	if (fmtype == PERF_HPP_FMT_TYPE__PERCENT) {
> >  		double percent = 0.0;
> >  		u64 total = hists__total_period(hists);
> >  
> >  		if (total)
> > -			percent = 100.0 * get_field(he) / total;
> > +			percent = 100.0 * val / total;
> >  
> > -		ret = hpp__call_print_fn(hpp, print_fn, fmt, len, percent);
> > -	} else if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
> > -		double average = 0;
> > +		return hpp__call_print_fn(hpp, print_fn, fmt, len, percent);
> > +	}
> >  
> > -		if (he->stat.nr_events)
> > -			average = 1.0 * get_field(he) / he->stat.nr_events;
> > +	if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
> > +		double avg = nr_samples ? (1.0 * val / nr_samples) : 0;
> >  
> > -		ret = hpp__call_print_fn(hpp, print_fn, fmt, len, average);
> > -	} else {
> > -		ret = hpp__call_print_fn(hpp, print_fn, fmt, len, get_field(he));
> > +		return hpp__call_print_fn(hpp, print_fn, fmt, len, avg);
> >  	}
> >  
> > +	return hpp__call_print_fn(hpp, print_fn, fmt, len, val);
> > +}
> > +
> > +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;
> > +	struct hists *hists = he->hists;
> > +	struct evsel *evsel = hists_to_evsel(hists);
> > +	char *buf = hpp->buf;
> > +	size_t size = hpp->size;
> > +
> > +	ret = __hpp__fmt_print(hpp, hists, get_field(he), he->stat.nr_events,
> > +			       fmt, len, print_fn, fmtype);
> > +
> >  	if (evsel__is_group_event(evsel)) {
> >  		int prev_idx, idx_delta;
> >  		struct hist_entry *pair;
> > @@ -72,30 +79,16 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
> >  
> >  			while (idx_delta--) {
> >  				/*
> > -				 * zero-fill group members in the middle which
> > -				 * have no sample
> > +				 * 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.
> >  				 */
> > -				if (fmtype != PERF_HPP_FMT_TYPE__RAW) {
> > -					ret += hpp__call_print_fn(hpp, print_fn,
> > -								  fmt, len, 0.0);
> > -				} else {
> > -					ret += hpp__call_print_fn(hpp, print_fn,
> > -								  fmt, len, 0ULL);
> > -				}
> > +				ret += __hpp__fmt_print(hpp, pair->hists, 0, 0,
> > +							fmt, len, print_fn, fmtype);
> >  			}
> >  
> > -			if (fmtype == PERF_HPP_FMT_TYPE__PERCENT) {
> > -				ret += hpp__call_print_fn(hpp, print_fn, fmt, len,
> > -							  100.0 * period / total);
> > -			} else if (fmtype == PERF_HPP_FMT_TYPE__AVERAGE) {
> > -				double avg = nr_samples ? (period / nr_samples) : 0;
> > -
> > -				ret += hpp__call_print_fn(hpp, print_fn, fmt,
> > -							  len, avg);
> > -			} else {
> > -				ret += hpp__call_print_fn(hpp, print_fn, fmt,
> > -							  len, period);
> > -			}
> > +			ret += __hpp__fmt_print(hpp, pair->hists, period, nr_samples,
> > +						fmt, len, print_fn, fmtype);
> >  
> >  			prev_idx = evsel__group_idx(evsel);
> >  		}
> > @@ -104,15 +97,11 @@ static int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
> >  
> >  		while (idx_delta--) {
> >  			/*
> > -			 * zero-fill group members at last which have no sample
> > +			 * zero-fill group members at last which have no sample.
> > +			 * the hists is not correct but it's fine like above.
> >  			 */
> > -			if (fmtype != PERF_HPP_FMT_TYPE__RAW) {
> > -				ret += hpp__call_print_fn(hpp, print_fn,
> > -							  fmt, len, 0.0);
> > -			} else {
> > -				ret += hpp__call_print_fn(hpp, print_fn,
> > -							  fmt, len, 0ULL);
> > -			}
> > +			ret += __hpp__fmt_print(hpp, evsel__hists(evsel), 0, 0,
> > +						fmt, len, print_fn, fmtype);
> >  		}
> >  	}
> >  
> > -- 
> > 2.45.1.288.g0e0cd299f1-goog