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

[Jiri Olsa]

On Tue, Nov 05, 2019 at 11:36:11AM +0800, Jin Yao wrote:

SNIP

> 
> Signed-off-by: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
> ---
>  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();
		...
	}

plus below

SNIP

> +
> +static int block_hists_browser__title(struct hist_browser *browser, char *bf,
> +				      size_t size)
> +{
> +	struct hists *hists = evsel__hists(browser->block_evsel);
> +	const char *evname = perf_evsel__name(browser->block_evsel);
> +	unsigned long nr_samples = hists->stats.nr_events[PERF_RECORD_SAMPLE];
> +	int ret;
> +
> +	ret = scnprintf(bf, size, "# Samples: %lu", nr_samples);
> +	if (evname)
> +		scnprintf(bf + ret, size -  ret, " of event '%s'", evname);
> +
> +	return 0;
> +}
> +
> +int block_hists_tui_browse(struct block_hist *bh, struct evsel *evsel,
> +			   float min_percent)
> +{
> +	struct hists *hists = &bh->block_hists;
> +	struct hist_browser *browser;
> +	int key = -1;
> +	static const char help[] =
> +	" q             Quit \n";
> +
> +	browser = hist_browser__new(hists);
> +	if (!browser)
> +		return -1;
> +
> +	browser->block_evsel = evsel;
> +	browser->title = block_hists_browser__title;
> +	browser->min_pcnt = min_percent;
> +
> +	/* reset abort key so that it can get Ctrl-C as a key */
> +	SLang_reset_tty();
> +	SLang_init_tty(0, 0, 0);
> +
> +	while (1) {
> +		key = hist_browser__run(browser, "? - help", true);
> +
> +		switch (key) {
> +		case 'q':
> +			goto out;
> +		case '?':
> +			ui_browser__help_window(&browser->b, help);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +out:
> +	hist_browser__delete(browser);
> +	return 0;
> +}

also could this go to block-info.c as well?

thanks,
jirka