From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755263AbbDTOA3 (ORCPT ); Mon, 20 Apr 2015 10:00:29 -0400 Received: from mail.kernel.org ([198.145.29.136]:46045 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751137AbbDTOA1 (ORCPT ); Mon, 20 Apr 2015 10:00:27 -0400 Date: Mon, 20 Apr 2015 11:00:20 -0300 From: Arnaldo Carvalho de Melo To: Namhyung Kim Cc: Ingo Molnar , Peter Zijlstra , Jiri Olsa , LKML , David Ahern Subject: Re: [PATCH 6/7] perf hists browser: Split popup menu actions Message-ID: <20150420140020.GE11111@kernel.org> References: <1429416255-12070-1-git-send-email-namhyung@kernel.org> <1429416255-12070-7-git-send-email-namhyung@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1429416255-12070-7-git-send-email-namhyung@kernel.org> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Em Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim escreveu: > Currently perf_evsel__hists_browse() function spins on a huge loop and > handles many key actions. Since it's hard to read and modify, let's > split it out into small helper functions. > > The add_XXX_opt() functions are to register popup menu item on the > selected entry. When it adds an item, it also saves related data into > struct popup_option and returns 1 so that it can increase the number of > items (nr_opts). A callback function named do_XXX is called with saved > data when the item is selected by user. > > No functional change intended. > > Signed-off-by: Namhyung Kim > --- > tools/perf/ui/browsers/hists.c | 565 ++++++++++++++++++++++++++--------------- > 1 file changed, 363 insertions(+), 202 deletions(-) > > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c > index cace2df7e561..315ebc493508 100644 > --- a/tools/perf/ui/browsers/hists.c > +++ b/tools/perf/ui/browsers/hists.c > @@ -1216,11 +1216,6 @@ static void hist_browser__delete(struct hist_browser *browser) > free(browser); > } > > -static struct hist_entry *hist_browser__selected_entry(struct hist_browser *browser) > -{ > - return browser->he_selection; > -} > - Why remove the above function? To reduce the patch size you could have left it and if the reason for removing it is that compelling, remove it in a later patch. > static struct thread *hist_browser__selected_thread(struct hist_browser *browser) > { > return browser->he_selection->thread; > @@ -1395,6 +1390,281 @@ close_file_and_continue: > return ret; > } > > +struct popup_option { > + struct thread *thread; > + struct map *map; > + struct dso *dso; > + struct symbol *sym; You could use struct map_symbol, that has the three above, right? In some cases you would have less lines by doing: ms = po->ms; > + int (*fn)(struct popup_option *opt, struct hist_browser *browser, > + struct hist_browser_timer *hbt, struct pstack *pstack, I wonder if, as a prep patch, you couldn't have browser->hbt, so that we would reduce the function signature above. Ditto for pstack. Also, you're adding the mechanism (popup_option) at the same time that you make those new functions use it. I think that it would be better if you first created the functions, call them directly, then introduce popup_option (probably better named as "popup_action"). Each patch would be smaller and more easily reviewable, as well bisect would work better when locating bugs. > + struct perf_session_env *env); > +}; > + > +static int > +do_annotate(struct popup_option *opt, struct hist_browser *browser, > + struct hist_browser_timer *hbt, struct pstack *pstack __maybe_unused, > + struct perf_session_env *env) > +{ > + struct perf_evsel *evsel; > + struct annotation *notes; > + struct map_symbol ms; > + int err; > + > + if (!objdump_path && perf_session_env__lookup_objdump(env)) > + return 0; > + > + ms.map = opt->map; > + ms.sym = opt->sym; > + > + notes = symbol__annotation(ms.sym); > + if (!notes->src) > + return 0; > + > + evsel = hists_to_evsel(browser->hists); > + err = map_symbol__tui_annotate(&ms, evsel, hbt); > + /* > + * offer option to annotate the other branch source or target > + * (if they exists) when returning from annotate > + */ > + if ((err == 'q' || err == CTRL('c')) > + && browser->he_selection->branch_info) Please put the && operator at the end of the first line, even if originally it was like that (was it?). > + return 1; > + > + ui_browser__update_nr_entries(&browser->b, browser->hists->nr_entries); > + if (err) > + ui_browser__handle_resize(&browser->b); > + return 0; > +} > + > +static int > +add_annotate_opt(struct popup_option *opt, char **optstr, > + struct hist_browser *browser __maybe_unused, > + struct map *map, struct symbol *sym) > +{ > + if (sym == NULL || map->dso->annotate_warned) > + return 0; > + > + if (asprintf(optstr, "Annotate %s", sym->name) < 0) > + return 0; > + > + opt->map = map; > + opt->sym = sym; > + opt->fn = do_annotate; > + return 1; > +} > + > +static int do_zoom_thread(struct popup_option *opt, > + struct hist_browser *browser, > + struct hist_browser_timer *hbt __maybe_unused, > + struct pstack *pstack, > + struct perf_session_env *env __maybe_unused) > +{ > + struct thread *thread = opt->thread; > + > + if (browser->hists->thread_filter) { > + pstack__remove(pstack, &browser->hists->thread_filter); > + perf_hpp__set_elide(HISTC_THREAD, false); > + thread__zput(browser->hists->thread_filter); > + ui_helpline__pop(); > + } else { > + ui_helpline__fpush("To zoom out press <- or -> + \"Zoom out of %s(%d) thread\"", > + thread->comm_set ? thread__comm_str(thread) : "", > + thread->tid); > + browser->hists->thread_filter = thread__get(thread); > + perf_hpp__set_elide(HISTC_THREAD, false); > + pstack__push(pstack, &browser->hists->thread_filter); > + } > + > + hists__filter_by_thread(browser->hists); > + hist_browser__reset(browser); > + return 0; > +} > + > +static int > +add_thread_opt(struct popup_option *opt, char **optstr, > + struct hist_browser *browser, struct thread *thread) > +{ > + if (thread == NULL) > + return 0; > + > + if (asprintf(optstr, "Zoom %s %s(%d) thread", > + browser->hists->thread_filter ? "out of" : "into", > + thread->comm_set ? thread__comm_str(thread) : "", > + thread->tid) < 0) > + return 0; > + > + opt->thread = thread; > + opt->fn = do_zoom_thread; > + return 1; > +} > + > +static int do_zoom_dso(struct popup_option *opt, > + struct hist_browser *browser, > + struct hist_browser_timer *hbt __maybe_unused, > + struct pstack *pstack, > + struct perf_session_env *env __maybe_unused) > +{ > + struct dso *dso = opt->dso; > + > + if (browser->hists->dso_filter) { > + pstack__remove(pstack, &browser->hists->dso_filter); > + perf_hpp__set_elide(HISTC_DSO, false); > + browser->hists->dso_filter = NULL; > + ui_helpline__pop(); > + } else { > + if (dso == NULL) > + return 0; > + ui_helpline__fpush("To zoom out press <- or -> + \"Zoom out of %s DSO\"", > + dso->kernel ? "the Kernel" : dso->short_name); > + browser->hists->dso_filter = dso; > + perf_hpp__set_elide(HISTC_DSO, true); > + pstack__push(pstack, &browser->hists->dso_filter); > + } > + > + hists__filter_by_dso(browser->hists); > + hist_browser__reset(browser); > + return 0; > +} > + > +static int > +add_dso_opt(struct popup_option *opt, char **optstr, > + struct hist_browser *browser, struct dso *dso) > +{ > + if (dso == NULL) > + return 0; > + > + if (asprintf(optstr, "Zoom %s %s DSO", > + browser->hists->dso_filter ? "out of" : "into", > + dso->kernel ? "the Kernel" : dso->short_name) < 0) > + return 0; > + > + opt->dso = dso; > + opt->fn = do_zoom_dso; > + return 1; > +} > + > +static int do_browse_map(struct popup_option *opt, > + struct hist_browser *browser __maybe_unused, > + struct hist_browser_timer *hbt __maybe_unused, > + struct pstack *pstack __maybe_unused, > + struct perf_session_env *env __maybe_unused) > +{ > + map__browse(opt->map); > + return 0; > +} > + > +static int > +add_map_opt(struct popup_option *opt, char **optstr, > + struct hist_browser *browser __maybe_unused, struct map *map) > +{ > + if (map == NULL) > + return 0; > + > + if (asprintf(optstr, "Browse map details") < 0) > + return 0; > + > + opt->map = map; > + opt->fn = do_browse_map; > + return 1; > +} > + > +static int do_run_script(struct popup_option *opt, > + struct hist_browser *browser __maybe_unused, > + struct hist_browser_timer *hbt __maybe_unused, > + struct pstack *pstack __maybe_unused, > + struct perf_session_env *env __maybe_unused) > +{ > + char script_opt[64]; > + memset(script_opt, 0, sizeof(script_opt)); > + > + if (opt->thread) { > + scnprintf(script_opt, sizeof(script_opt), " -c %s ", > + thread__comm_str(opt->thread)); > + } else if (opt->sym) { > + scnprintf(script_opt, sizeof(script_opt), " -S %s ", > + opt->sym->name); > + } > + > + script_browse(script_opt); > + return 0; > +} > + > +static int > +add_script_opt(struct popup_option *opt, char **optstr, > + struct hist_browser *browser __maybe_unused, > + struct thread *thread, struct symbol *sym) > +{ > + if (thread) { > + if (asprintf(optstr, "Run scripts for samples of thread [%s]", > + thread__comm_str(thread)) < 0) > + return 0; > + } else if (sym) { > + if (asprintf(optstr, "Run scripts for samples of symbol [%s]", > + sym->name) < 0) > + return 0; > + } else { > + if (asprintf(optstr, "Run scripts for all samples") < 0) > + return 0; > + } > + > + opt->thread = thread; > + opt->sym = sym; > + opt->fn = do_run_script; > + > + return 1; > +} > + > +static int do_switch_data(struct popup_option *opt __maybe_unused, > + struct hist_browser *browser __maybe_unused, > + struct hist_browser_timer *hbt __maybe_unused, > + struct pstack *pstack __maybe_unused, > + struct perf_session_env *env __maybe_unused) > +{ > + if (switch_data_file()) { > + ui__warning("Won't switch the data files due to\n" > + "no valid data file get selected!\n"); > + return 0; > + } > + > + return K_SWITCH_INPUT_DATA; > +} > + > +static int > +add_switch_opt(struct popup_option *opt, char **optstr, > + struct hist_browser *browser __maybe_unused, > + struct hist_browser_timer *hbt) > +{ > + if (!is_report_browser(hbt)) > + return 0; > + > + if (asprintf(optstr, "Switch to another data file in PWD") < 0) > + return 0; > + > + opt->fn = do_switch_data; > + return 1; > +} > + > +static int do_exit_browser(struct popup_option *opt __maybe_unused, > + struct hist_browser *browser __maybe_unused, > + struct hist_browser_timer *hbt __maybe_unused, > + struct pstack *pstack __maybe_unused, > + struct perf_session_env *env __maybe_unused) > +{ > + return 0; > +} > + > +static int > +add_exit_opt(struct popup_option *opt, char **optstr, > + struct hist_browser *browser __maybe_unused) > +{ > + if (asprintf(optstr, "Exit") < 0) > + return 0; > + > + opt->fn = do_exit_browser; > + return 1; > +} > + > static void hist_browser__update_nr_entries(struct hist_browser *hb) > { > u64 nr_entries = 0; > @@ -1424,11 +1694,11 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events, > struct hist_browser *browser = hist_browser__new(hists); > struct branch_info *bi; > struct pstack *fstack; > - char *options[16]; > - int nr_options = 0; > + char *optstr[16]; > + struct popup_option opts[16]; > + int nr_opts = 0; > int key = -1; > char buf[64]; > - char script_opt[64]; > int delay_secs = hbt ? hbt->refresh : 0; > struct perf_hpp_fmt *fmt; > > @@ -1479,7 +1749,8 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events, > > ui_helpline__push(helpline); > > - memset(options, 0, sizeof(options)); > + memset(optstr, 0, sizeof(optstr)); > + memset(opts, 0, sizeof(opts)); > > perf_hpp__for_each_format(fmt) > perf_hpp__reset_width(fmt, hists); > @@ -1489,14 +1760,10 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events, > > while (1) { > struct thread *thread = NULL; > - const struct dso *dso = NULL; > - int choice = 0, > - annotate = -2, zoom_dso = -2, zoom_thread = -2, > - annotate_f = -2, annotate_t = -2, browse_map = -2; > - int scripts_comm = -2, scripts_symbol = -2, > - scripts_all = -2, switch_data = -2; > + struct dso *dso = NULL; > + int choice = 0; > > - nr_options = 0; > + nr_opts = 0; > > key = hist_browser__run(browser, hbt); > > @@ -1526,17 +1793,28 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events, > browser->selection->sym == NULL || > browser->selection->map->dso->annotate_warned) > continue; > - goto do_annotate; > + > + opts->map = browser->selection->map; > + opts->sym = browser->selection->sym; > + > + do_annotate(opts, browser, hbt, fstack, env); > + continue; > case 'P': > hist_browser__dump(browser); > continue; > case 'd': > - goto zoom_dso; > + opts->dso = dso; > + > + do_zoom_dso(opts, browser, hbt, fstack, env); > + continue; > case 'V': > browser->show_dso = !browser->show_dso; > continue; > case 't': > - goto zoom_thread; > + opts->thread = thread; > + > + do_zoom_thread(opts, browser, hbt, fstack, env); > + continue; > case '/': > if (ui_browser__input_window("Symbol to show", > "Please enter the name of symbol you want to see", > @@ -1548,12 +1826,20 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events, > } > continue; > case 'r': > - if (is_report_browser(hbt)) > - goto do_scripts; > + if (is_report_browser(hbt)) { > + opts->thread = NULL; > + opts->sym = NULL; > + > + do_run_script(opts, browser, hbt, fstack, env); > + } > continue; > case 's': > - if (is_report_browser(hbt)) > - goto do_data_switch; > + if (is_report_browser(hbt)) { > + key = do_switch_data(opts, browser, hbt, > + fstack, env); > + if (key == K_SWITCH_INPUT_DATA) > + goto out_free_stack; > + } > continue; > case 'i': > /* env->arch is NULL for live-mode (i.e. perf top) */ > @@ -1592,10 +1878,16 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events, > continue; > } > top = pstack__pop(fstack); > - if (top == &browser->hists->dso_filter) > - goto zoom_out_dso; > - if (top == &browser->hists->thread_filter) > - goto zoom_out_thread; > + if (top == &browser->hists->dso_filter) { > + perf_hpp__set_elide(HISTC_DSO, false); > + browser->hists->dso_filter = NULL; > + ui_helpline__pop(); > + } > + if (top == &browser->hists->thread_filter) { > + perf_hpp__set_elide(HISTC_THREAD, false); > + thread__zput(browser->hists->thread_filter); > + ui_helpline__pop(); > + } > continue; > } > case K_ESC: > @@ -1620,200 +1912,69 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events, > if (sort__mode == SORT_MODE__BRANCH) { > bi = browser->he_selection->branch_info; > > - if (bi == NULL) > - goto skip_annotation; > - > - if (bi->from.sym != NULL && > - !bi->from.map->dso->annotate_warned && > - asprintf(&options[nr_options], "Annotate %s", bi->from.sym->name) > 0) { > - annotate_f = nr_options++; > - } > - > - if (bi->to.sym != NULL && > - !bi->to.map->dso->annotate_warned && > - (bi->to.sym != bi->from.sym || > - bi->to.map->dso != bi->from.map->dso) && > - asprintf(&options[nr_options], "Annotate %s", bi->to.sym->name) > 0) { > - annotate_t = nr_options++; > + if (bi != NULL) { > + nr_opts += add_annotate_opt(&opts[nr_opts], > + &optstr[nr_opts], > + browser, > + bi->from.map, > + bi->from.sym); > + if (bi->to.sym != bi->from.sym) > + nr_opts += add_annotate_opt(&opts[nr_opts], > + &optstr[nr_opts], > + browser, > + bi->to.map, > + bi->to.sym); > } > } else { > - if (browser->selection->sym != NULL && > - !browser->selection->map->dso->annotate_warned) { > - struct annotation *notes; > - > - notes = symbol__annotation(browser->selection->sym); > - > - if (notes->src && > - asprintf(&options[nr_options], "Annotate %s", > - browser->selection->sym->name) > 0) { > - annotate = nr_options++; > - } > - } > + nr_opts += add_annotate_opt(&opts[nr_opts], > + &optstr[nr_opts], browser, > + browser->selection->map, > + browser->selection->sym); > } > skip_annotation: > - if (thread != NULL && > - asprintf(&options[nr_options], "Zoom %s %s(%d) thread", > - (browser->hists->thread_filter ? "out of" : "into"), > - (thread->comm_set ? thread__comm_str(thread) : ""), > - thread->tid) > 0) > - zoom_thread = nr_options++; > - > - if (dso != NULL && > - asprintf(&options[nr_options], "Zoom %s %s DSO", > - (browser->hists->dso_filter ? "out of" : "into"), > - (dso->kernel ? "the Kernel" : dso->short_name)) > 0) > - zoom_dso = nr_options++; > - > - if (browser->selection != NULL && > - browser->selection->map != NULL && > - asprintf(&options[nr_options], "Browse map details") > 0) > - browse_map = nr_options++; > + nr_opts += add_thread_opt(&opts[nr_opts], &optstr[nr_opts], > + browser, thread); > + nr_opts += add_dso_opt(&opts[nr_opts], &optstr[nr_opts], > + browser, dso); > + nr_opts += add_map_opt(&opts[nr_opts], &optstr[nr_opts], > + browser, browser->selection->map); > > /* perf script support */ > if (browser->he_selection) { > - struct symbol *sym; > - > - if (asprintf(&options[nr_options], "Run scripts for samples of thread [%s]", > - thread__comm_str(browser->he_selection->thread)) > 0) > - scripts_comm = nr_options++; > - > - sym = browser->he_selection->ms.sym; > - if (sym && sym->namelen && > - asprintf(&options[nr_options], "Run scripts for samples of symbol [%s]", > - sym->name) > 0) > - scripts_symbol = nr_options++; > + nr_opts += add_script_opt(&opts[nr_opts], &optstr[nr_opts], > + browser, thread, NULL); > + nr_opts += add_script_opt(&opts[nr_opts], &optstr[nr_opts], > + browser, NULL, > + browser->selection->sym); > } > + nr_opts += add_script_opt(&opts[nr_opts], &optstr[nr_opts], > + browser, NULL, NULL); > > - if (asprintf(&options[nr_options], "Run scripts for all samples") > 0) > - scripts_all = nr_options++; > - > - if (is_report_browser(hbt) && asprintf(&options[nr_options], > - "Switch to another data file in PWD") > 0) > - switch_data = nr_options++; > + nr_opts += add_switch_opt(&opts[nr_opts], &optstr[nr_opts], > + browser, hbt); > add_exit_option: > - if (asprintf(&options[nr_options], "Exit") > 0) > - nr_options++; > -retry_popup_menu: > - choice = ui__popup_menu(nr_options, options); > - > - if (choice == nr_options - 1) > - break; > - > - if (choice == -1) { > - free_popup_options(options, nr_options - 1); > - continue; > - } > - > - if (choice == annotate || choice == annotate_t || choice == annotate_f) { > - struct hist_entry *he; > - struct annotation *notes; > - struct map_symbol ms; > - int err; > -do_annotate: > - if (!objdump_path && perf_session_env__lookup_objdump(env)) > - continue; > + nr_opts += add_exit_opt(&opts[nr_opts], &optstr[nr_opts], > + browser); > > - he = hist_browser__selected_entry(browser); > - if (he == NULL) > - continue; > - > - if (choice == annotate_f) { > - ms.map = he->branch_info->from.map; > - ms.sym = he->branch_info->from.sym; > - } else if (choice == annotate_t) { > - ms.map = he->branch_info->to.map; > - ms.sym = he->branch_info->to.sym; > - } else { > - ms = *browser->selection; > - } > - > - notes = symbol__annotation(ms.sym); > - if (!notes->src) > - continue; > - > - err = map_symbol__tui_annotate(&ms, evsel, hbt); > - /* > - * offer option to annotate the other branch source or target > - * (if they exists) when returning from annotate > - */ > - if ((err == 'q' || err == CTRL('c')) > - && annotate_t != -2 && annotate_f != -2) > - goto retry_popup_menu; > - > - ui_browser__update_nr_entries(&browser->b, browser->hists->nr_entries); > - if (err) > - ui_browser__handle_resize(&browser->b); > - > - } else if (choice == browse_map) > - map__browse(browser->selection->map); > - else if (choice == zoom_dso) { > -zoom_dso: > - if (browser->hists->dso_filter) { > - pstack__remove(fstack, &browser->hists->dso_filter); > -zoom_out_dso: > - ui_helpline__pop(); > - browser->hists->dso_filter = NULL; > - perf_hpp__set_elide(HISTC_DSO, false); > - } else { > - if (dso == NULL) > - continue; > - ui_helpline__fpush("To zoom out press <- or -> + \"Zoom out of %s DSO\"", > - dso->kernel ? "the Kernel" : dso->short_name); > - browser->hists->dso_filter = dso; > - perf_hpp__set_elide(HISTC_DSO, true); > - pstack__push(fstack, &browser->hists->dso_filter); > - } > - hists__filter_by_dso(hists); > - hist_browser__reset(browser); > - } else if (choice == zoom_thread) { > -zoom_thread: > - if (browser->hists->thread_filter) { > - pstack__remove(fstack, &browser->hists->thread_filter); > -zoom_out_thread: > - ui_helpline__pop(); > - thread__zput(browser->hists->thread_filter); > - perf_hpp__set_elide(HISTC_THREAD, false); > - } else { > - ui_helpline__fpush("To zoom out press <- or -> + \"Zoom out of %s(%d) thread\"", > - thread->comm_set ? thread__comm_str(thread) : "", > - thread->tid); > - browser->hists->thread_filter = thread__get(thread); > - perf_hpp__set_elide(HISTC_THREAD, false); > - pstack__push(fstack, &browser->hists->thread_filter); > - } > - hists__filter_by_thread(hists); > - hist_browser__reset(browser); > - } > - /* perf scripts support */ > - else if (choice == scripts_all || choice == scripts_comm || > - choice == scripts_symbol) { > -do_scripts: > - memset(script_opt, 0, 64); > + do { > + struct popup_option *opt; > > - if (choice == scripts_comm) > - sprintf(script_opt, " -c %s ", thread__comm_str(browser->he_selection->thread)); > + choice = ui__popup_menu(nr_opts, optstr); > + if (choice == -1 || choice >= nr_opts) > + break; > > - if (choice == scripts_symbol) > - sprintf(script_opt, " -S %s ", browser->he_selection->ms.sym->name); > + opt = &opts[choice]; > + key = opt->fn(opt, browser, hbt, fstack, env); > + } while (key == 1); > > - script_browse(script_opt); > - } > - /* Switch to another data file */ > - else if (choice == switch_data) { > -do_data_switch: > - if (!switch_data_file()) { > - key = K_SWITCH_INPUT_DATA; > - break; > - } else > - ui__warning("Won't switch the data files due to\n" > - "no valid data file get selected!\n"); > - } > + if (key == K_SWITCH_INPUT_DATA) > + break; > } > out_free_stack: > pstack__delete(fstack); > out: > hist_browser__delete(browser); > - free_popup_options(options, ARRAY_SIZE(options)); > + free_popup_options(optstr, ARRAY_SIZE(optstr)); > return key; > } > > -- > 2.3.5