Re: [PATCH v7 2/3] perf report: Support a new key to reload the browser

[Jin, Yao]

On 3/19/2020 2:52 AM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Feb 20, 2020 at 09:36:15AM +0800, Jin Yao escreveu:
> > Sometimes we may need to reload the browser to update the output since
> > some options are changed.
> >
> > This patch creates a new key K_RELOAD. Once the __cmd_report() returns
> > K_RELOAD, it would repeat the whole process, such as, read samples from
> > data file, sort the data and display in the browser.
> >
> >   v7:
> >   ---
> >   Rebase to perf/core, no other change.
> >
> >   v6:
> >   ---
> >   No change.
> >
> >   v5:
> >   ---
> >   1. Fix the 'make NO_SLANG=1' error. Define K_RELOAD in util/hist.h.
> >   2. Skip setup_sorting() in repeat path if last key is K_RELOAD.
> >
> >   v4:
> >   ---
> >   Need to quit in perf_evsel_menu__run if key is K_RELOAD.
> >
> >   v3:
> >   ---
> >   No change.
> >
> >   v2:
> >   ---
> >   This is a new patch created in v2.
> >
> > Signed-off-by: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
> > ---
> >   tools/perf/builtin-report.c    | 6 +++---
> >   tools/perf/ui/browsers/hists.c | 1 +
> >   tools/perf/ui/keysyms.h        | 1 +
> >   tools/perf/util/hist.h         | 1 +
> >   4 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > index 862c7f8853dc..842ef92c3598 100644
> > --- a/tools/perf/builtin-report.c
> > +++ b/tools/perf/builtin-report.c
> > @@ -635,7 +635,7 @@ static int report__browse_hists(struct report *rep)
> >   		 * Usually "ret" is the last pressed key, and we only
> >   		 * care if the key notifies us to switch data file.
> >   		 */
> > -		if (ret != K_SWITCH_INPUT_DATA)
> > +		if (ret != K_SWITCH_INPUT_DATA && ret != K_RELOAD)
> >   			ret = 0;
> >   		break;
> >   	case 2:
> > @@ -1469,7 +1469,7 @@ int cmd_report(int argc, const char **argv)
> >   		sort_order = sort_tmp;
> >   	}
> >   
> > -	if ((last_key != K_SWITCH_INPUT_DATA) &&
> > +	if ((last_key != K_SWITCH_INPUT_DATA && last_key != K_RELOAD) &&
> >   	    (setup_sorting(session->evlist) < 0)) {
> >   		if (sort_order)
> >   			parse_options_usage(report_usage, options, "s", 1);
> > @@ -1548,7 +1548,7 @@ int cmd_report(int argc, const char **argv)
> >   	sort__setup_elide(stdout);
> >   
> >   	ret = __cmd_report(&report);
> > -	if (ret == K_SWITCH_INPUT_DATA) {
> > +	if (ret == K_SWITCH_INPUT_DATA || ret == K_RELOAD) {
> >   		perf_session__delete(session);
> >   		last_key = K_SWITCH_INPUT_DATA;
>
> Are you sure this shouldn't be:
> 	
> 		last_key = ret;
>
> ?

Yes, it should not be. We can reuse original flow for processing 
K_SWITCH_INPUT_DATA.

> I'm applying it to test now anyway,
>
> - Arnaldo

Thanks!

Thanks
Jin Yao

> >   		goto repeat;
> > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> > index f36dee499320..7c091fa51a5c 100644
> > --- a/tools/perf/ui/browsers/hists.c
> > +++ b/tools/perf/ui/browsers/hists.c
> > @@ -3440,6 +3440,7 @@ static int perf_evsel_menu__run(struct evsel_menu *menu,
> >   					pos = perf_evsel__prev(pos);
> >   				goto browse_hists;
> >   			case K_SWITCH_INPUT_DATA:
> > +			case K_RELOAD:
> >   			case 'q':
> >   			case CTRL('c'):
> >   				goto out;
> > diff --git a/tools/perf/ui/keysyms.h b/tools/perf/ui/keysyms.h
> > index fbfac29077f2..04cc4e5c031f 100644
> > --- a/tools/perf/ui/keysyms.h
> > +++ b/tools/perf/ui/keysyms.h
> > @@ -25,5 +25,6 @@
> >   #define K_ERROR	 -2
> >   #define K_RESIZE -3
> >   #define K_SWITCH_INPUT_DATA -4
> > +#define K_RELOAD -5
> >   
> >   #endif /* _PERF_KEYSYMS_H_ */
> > diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> > index 0aa63aeb58ec..bb994e030495 100644
> > --- a/tools/perf/util/hist.h
> > +++ b/tools/perf/util/hist.h
> > @@ -536,6 +536,7 @@ static inline int block_hists_tui_browse(struct block_hist *bh __maybe_unused,
> >   #define K_LEFT  -1000
> >   #define K_RIGHT -2000
> >   #define K_SWITCH_INPUT_DATA -3000
> > +#define K_RELOAD -4000
> >   #endif
> >   
> >   unsigned int hists__sort_list_width(struct hists *hists);
> > --
> > 2.17.1