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

Jiri Olsa
2019-11-07T09:26:44Z

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