Received-SPF: pass (google.com: domain of linux-kernel+bounces-201626-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 4 Jun 2024 17:42:50 -0700
From: Namhyung Kim <namhyung@kernel.org>
To: Arnaldo Carvalho de Melo <acme@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: <Zl-0igwvs8EuzOMs@google.com>
References: <20240603224412.1910049-1-namhyung@kernel.org>
 <20240603224412.1910049-2-namhyung@kernel.org>
 <Zl8tvVPEfLM8_T_z@x1>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <Zl8tvVPEfLM8_T_z@x1>

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