Received-SPF: pass (google.com: domain of linux-kernel+bounces-200924-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 4 Jun 2024 12:07:41 -0300
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Namhyung Kim <namhyung@kernel.org>
Cc: Ian Rogers <irogers@google.com>, Kan Liang <kan.liang@linux.intel.com>,
	Jiri Olsa <jolsa@kernel.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	linux-perf-users@vger.kernel.org
Subject: Re: [PATCH 1/4] perf hist: Factor out __hpp__fmt_print()
Message-ID: <Zl8tvVPEfLM8_T_z@x1>
References: <20240603224412.1910049-1-namhyung@kernel.org>
 <20240603224412.1910049-2-namhyung@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240603224412.1910049-2-namhyung@kernel.org>

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