Re: [PATCH 6/7] perf hists browser: Split popup menu actions
Hi Arnaldo,
On Mon, Apr 20, 2015 at 06:28:45PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Apr 21, 2015 at 12:22:06AM +0900, Namhyung Kim escreveu:
> > Hi Arnaldo,
> >
> > On Mon, Apr 20, 2015 at 11:00:20AM -0300, Arnaldo Carvalho de Melo wrote:
> > > 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.
> >
> > OK, will do.
> >
> > >
> > > > 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;
> >
> > OK.
> >
> > >
> > > > + 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.
> >
> > I don't get it. The hbt and pstack is needed to annotate and zoom.
> > Are you saying about step-by-step conversion for each action like
> > first patch for annotate, seconf for zoom, and so on..?
>
> I am saying that pstack was a local variable in that big function,
> instead, perhaps we can have it as a member of struct hists_browser, so
> that we don't have to pass (struct hist_browser *, struct pstack *),
> just (struct hist_browser *).
Ah, I got it. Will change that way.
Thanks,
Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/7] perf hists browser: Split popup menu actions
Hi Arnaldo,
On Mon, Apr 20, 2015 at 06:28:45PM -0300, Arnaldo Carvalho de Melo wrote:
Em Tue, Apr 21, 2015 at 12:22:06AM +0900, Namhyung Kim escreveu:
Hi Arnaldo,
On Mon, Apr 20, 2015 at 11:00:20AM -0300, Arnaldo Carvalho de Melo wrote:
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 namhy...@kernel.org
---
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.
OK, will do.
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;
OK.
+ 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.
I don't get it. The hbt and pstack is needed to annotate and zoom.
Are you saying about step-by-step conversion for each action like
first patch for annotate, seconf for zoom, and so on..?
I am saying that pstack was a local variable in that big function,
instead, perhaps we can have it as a member of struct hists_browser, so
that we don't have to pass (struct hist_browser *, struct pstack *),
just (struct hist_browser *).
Ah, I got it. Will change that way.
Thanks,
Namhyung
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/7] perf hists browser: Split popup menu actions
Em Tue, Apr 21, 2015 at 12:22:06AM +0900, Namhyung Kim escreveu:
> Hi Arnaldo,
>
> On Mon, Apr 20, 2015 at 11:00:20AM -0300, Arnaldo Carvalho de Melo wrote:
> > 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.
>
> OK, will do.
>
> >
> > > 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;
>
> OK.
>
> >
> > > + 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.
>
> I don't get it. The hbt and pstack is needed to annotate and zoom.
> Are you saying about step-by-step conversion for each action like
> first patch for annotate, seconf for zoom, and so on..?
I am saying that pstack was a local variable in that big function,
instead, perhaps we can have it as a member of struct hists_browser, so
that we don't have to pass (struct hist_browser *, struct pstack *),
just (struct hist_browser *).
- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/7] perf hists browser: Split popup menu actions
Hi Arnaldo,
On Mon, Apr 20, 2015 at 11:00:20AM -0300, Arnaldo Carvalho de Melo wrote:
> 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.
OK, will do.
>
> > 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;
OK.
>
> > + 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.
I don't get it. The hbt and pstack is needed to annotate and zoom.
Are you saying about step-by-step conversion for each action like
first patch for annotate, seconf for zoom, and so on..?
>
>
> 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.
OK, I'll split the patch into steps..
>
> > + 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(, 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?).
Yes, it was. I'll change it anyway..
Thanks,
Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/7] perf hists browser: Split popup menu actions
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(, 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(>b, browser->hists->nr_entries);
> + if (err)
> + ui_browser__handle_resize(>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, >hists->thread_filter);
> + perf_hpp__set_elide(HISTC_THREAD, false);
> + thread__zput(browser->hists->thread_filter);
> +
[PATCH 6/7] perf hists browser: Split popup menu actions
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 | 567 ++---
1 file changed, 365 insertions(+), 202 deletions(-)
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index cace2df7e561..75366f8df848 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;
-}
-
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;
+
+ int (*fn)(struct popup_option *opt, struct hist_browser *browser,
+ struct hist_browser_timer *hbt, struct pstack *pstack,
+ 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(, 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)
+ return 1;
+
+ ui_browser__update_nr_entries(>b, browser->hists->nr_entries);
+ if (err)
+ ui_browser__handle_resize(>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, >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, >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;
+}
+
Re: [PATCH 6/7] perf hists browser: Split popup menu actions
On Mon, Apr 20, 2015 at 11:46:07AM +0200, Jiri Olsa wrote:
> On Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim wrote:
>
> SNIP
>
> > 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 == >hists->dso_filter)
> > - goto zoom_out_dso;
> > - if (top == >hists->thread_filter)
> > - goto zoom_out_thread;
> > + if (top == >hists->dso_filter) {
> > + perf_hpp__set_elide(HISTC_DSO, false);
> > + browser->hists->dso_filter = NULL;
> > + ui_helpline__pop();
> > + }
> > + if (top == >hists->thread_filter) {
> > + perf_hpp__set_elide(HISTC_THREAD, false);
> > + thread__zput(browser->hists->thread_filter);
> > + ui_helpline__pop();
> > + }
>
> ui_helpline__pop could be called in heare and removed in above legs:
>
> ui_helpline__pop()
>
> also the original code calls following for zoom out:
>
> - hists__filter_by_dso(hists);
> - hist_browser__reset(browser);
>
Right. The intention was to call do_zoom_thread() or do_zoom_dso()
like in the next patch. Will fix this anyway.
Thanks for your careful review!
Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/7] perf hists browser: Split popup menu actions
On Mon, Apr 20, 2015 at 11:21:59AM +0200, Jiri Olsa wrote:
> On Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim wrote:
>
> SNIP
>
> > +
> > +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) {
>
> there's also check for sym->namelen in the original code
Ah, right. I'll change it.
But I wonder there's a case 'sym && !sym->namelen' though..
Thanks,
Namhyung
>
> > + 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;
> > +}
>
> SNIP
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/7] perf hists browser: Split popup menu actions
On Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim wrote:
SNIP
> 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 == >hists->dso_filter)
> - goto zoom_out_dso;
> - if (top == >hists->thread_filter)
> - goto zoom_out_thread;
> + if (top == >hists->dso_filter) {
> + perf_hpp__set_elide(HISTC_DSO, false);
> + browser->hists->dso_filter = NULL;
> + ui_helpline__pop();
> + }
> + if (top == >hists->thread_filter) {
> + perf_hpp__set_elide(HISTC_THREAD, false);
> + thread__zput(browser->hists->thread_filter);
> + ui_helpline__pop();
> + }
ui_helpline__pop could be called in heare and removed in above legs:
ui_helpline__pop()
also the original code calls following for zoom out:
- hists__filter_by_dso(hists);
- hist_browser__reset(browser);
jirka
> 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;
>
SNIP
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/7] perf hists browser: Split popup menu actions
On Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim wrote:
SNIP
> +
> +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) {
there's also check for sym->namelen in the original code
> + 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;
> +}
SNIP
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/7] perf hists browser: Split popup menu actions
Em Tue, Apr 21, 2015 at 12:22:06AM +0900, Namhyung Kim escreveu:
Hi Arnaldo,
On Mon, Apr 20, 2015 at 11:00:20AM -0300, Arnaldo Carvalho de Melo wrote:
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 namhy...@kernel.org
---
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.
OK, will do.
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;
OK.
+ 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.
I don't get it. The hbt and pstack is needed to annotate and zoom.
Are you saying about step-by-step conversion for each action like
first patch for annotate, seconf for zoom, and so on..?
I am saying that pstack was a local variable in that big function,
instead, perhaps we can have it as a member of struct hists_browser, so
that we don't have to pass (struct hist_browser *, struct pstack *),
just (struct hist_browser *).
- Arnaldo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 6/7] perf hists browser: Split popup menu actions
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 namhy...@kernel.org
---
tools/perf/ui/browsers/hists.c | 567 ++---
1 file changed, 365 insertions(+), 202 deletions(-)
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index cace2df7e561..75366f8df848 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;
-}
-
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;
+
+ int (*fn)(struct popup_option *opt, struct hist_browser *browser,
+ struct hist_browser_timer *hbt, struct pstack *pstack,
+ 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)
+ 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
Re: [PATCH 6/7] perf hists browser: Split popup menu actions
On Mon, Apr 20, 2015 at 11:46:07AM +0200, Jiri Olsa wrote:
On Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim wrote:
SNIP
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();
+ }
ui_helpline__pop could be called in heare and removed in above legs:
ui_helpline__pop()
also the original code calls following for zoom out:
- hists__filter_by_dso(hists);
- hist_browser__reset(browser);
Right. The intention was to call do_zoom_thread() or do_zoom_dso()
like in the next patch. Will fix this anyway.
Thanks for your careful review!
Namhyung
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/7] perf hists browser: Split popup menu actions
On Mon, Apr 20, 2015 at 11:21:59AM +0200, Jiri Olsa wrote:
On Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim wrote:
SNIP
+
+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) {
there's also check for sym-namelen in the original code
Ah, right. I'll change it.
But I wonder there's a case 'sym !sym-namelen' though..
Thanks,
Namhyung
+ 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;
+}
SNIP
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/7] perf hists browser: Split popup menu actions
On Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim wrote:
SNIP
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();
+ }
ui_helpline__pop could be called in heare and removed in above legs:
ui_helpline__pop()
also the original code calls following for zoom out:
- hists__filter_by_dso(hists);
- hist_browser__reset(browser);
jirka
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;
SNIP
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/7] perf hists browser: Split popup menu actions
On Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim wrote:
SNIP
+
+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) {
there's also check for sym-namelen in the original code
+ 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;
+}
SNIP
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/7] perf hists browser: Split popup menu actions
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 namhy...@kernel.org
---
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)
Re: [PATCH 6/7] perf hists browser: Split popup menu actions
Hi Arnaldo,
On Mon, Apr 20, 2015 at 11:00:20AM -0300, Arnaldo Carvalho de Melo wrote:
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 namhy...@kernel.org
---
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.
OK, will do.
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;
OK.
+ 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.
I don't get it. The hbt and pstack is needed to annotate and zoom.
Are you saying about step-by-step conversion for each action like
first patch for annotate, seconf for zoom, and so on..?
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.
OK, I'll split the patch into steps..
+ 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?).
Yes, it was. I'll change it anyway..
Thanks,
Namhyung
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 6/7] perf hists browser: Split popup menu actions
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;
-}
-
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;
+
+ int (*fn)(struct popup_option *opt, struct hist_browser *browser,
+ struct hist_browser_timer *hbt, struct pstack *pstack,
+ 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(, 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)
+ return 1;
+
+ ui_browser__update_nr_entries(>b, browser->hists->nr_entries);
+ if (err)
+ ui_browser__handle_resize(>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, >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, >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;
+}
+
[PATCH 6/7] perf hists browser: Split popup menu actions
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 namhy...@kernel.org
---
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;
-}
-
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;
+
+ int (*fn)(struct popup_option *opt, struct hist_browser *browser,
+ struct hist_browser_timer *hbt, struct pstack *pstack,
+ 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)
+ 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
