Re: [PATCH v6 7/7] perf report: Sort by sampled cycles percent per block for tui

[Jiri Olsa <jolsa@redhat.com>]

On Thu, Nov 07, 2019 at 02:19:10PM +0800, Jin, Yao wrote:
> 
> 
> On 11/7/2019 5:01 AM, Jiri Olsa wrote:
> > On Tue, Nov 05, 2019 at 11:36:11AM +0800, Jin Yao wrote:
> > 
> > SNIP
> > 
> > > 
> > > Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> > > ---
> > >   tools/perf/builtin-report.c    | 27 ++++++++++++---
> > >   tools/perf/ui/browsers/hists.c | 62 +++++++++++++++++++++++++++++++++-
> > >   tools/perf/ui/browsers/hists.h |  2 ++
> > >   tools/perf/util/block-info.c   | 12 +++++++
> > >   tools/perf/util/block-info.h   |  3 ++
> > >   tools/perf/util/hist.h         | 12 +++++++
> > >   6 files changed, 112 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > > index 7a8b0be8f09a..af5a57d06f12 100644
> > > --- a/tools/perf/builtin-report.c
> > > +++ b/tools/perf/builtin-report.c
> > > @@ -485,6 +485,22 @@ static size_t hists__fprintf_nr_sample_events(struct hists *hists, struct report
> > >   	return ret + fprintf(fp, "\n#\n");
> > >   }
> > > +static int perf_evlist__tui_block_hists_browse(struct evlist *evlist,
> > > +					       struct report *rep)
> > > +{
> > > +	struct evsel *pos;
> > > +	int i = 0, ret;
> > > +
> > > +	evlist__for_each_entry(evlist, pos) {
> > > +		ret = report__tui_browse_block_hists(&rep->block_reports[i++].hist,
> > > +						     rep->min_percent, pos);
> > > +		if (ret != 0)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   static int perf_evlist__tty_browse_hists(struct evlist *evlist,
> > >   					 struct report *rep,
> > >   					 const char *help)
> > > @@ -595,6 +611,11 @@ static int report__browse_hists(struct report *rep)
> > >   	switch (use_browser) {
> > >   	case 1:
> > > +		if (rep->total_cycles_mode) {
> > > +			ret = perf_evlist__tui_block_hists_browse(evlist, rep);
> > > +			break;
> > > +		}
> > 
> > it's good that most of it is in the block-info.c,
> > however what I mean was to have a single report
> > function for rep->total_cycles_mode, like:
> > 
> > 	report__browse_block_hists()
> > 	{
> > 		switch (use_browser) {
> > 		case 1:
> > 			ret = perf_evlist__tui_block_hists_browse(evlist, rep);
> > 			break;
> > 		case 0:
> > 			ret = perf_evlist__tty_block_hists_browse(evlist, rep);
> > 			break;
> > 		...
> > 	}
> > 
> > preferable in block-info.c as well
> > 
> > which would be hooked in report__browse_hists:
> > 
> > 	report__browse_hists()
> > 	{
> > 		if (rep->total_cycles_mode)
> > 			return report__browse_block_hists();
> > 		...
> > 	}
> > 
> 
> If we move all block implementations from builtin-report.c to block-info.c,
> one difficulty is that we can't reuse some codes in builtin-report.c. For
> example, reuse the function which prints the event stats
> (hists__fprintf_nr_sample_events)

aah so that's why it's deep in perf_evlist__tty_browse_hists, I see
ok it's close enough then ;-)

thanks,
jirka