lkml.org 
[lkml]   [2012]   [Feb]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] perf report: Add a simple GTK2-based 'perf report' browser
    Hi,

    2012-02-24 2:52 AM, Pekka Enberg wrote:
    > From 3dc75243a43212fe907fad4139087f4033c2bf53 Mon Sep 17 00:00:00 2001
    > From: Pekka Enberg<penberg@kernel.org>
    > Date: Thu, 23 Feb 2012 17:49:18 +0200
    > Subject: [PATCH] perf report: Add a simple GTK2-based 'perf report' browser
    > MIME-Version: 1.0
    > Content-Type: text/plain; charset=UTF-8
    > Content-Transfer-Encoding: 8bit
    >
    > This patch adds a simple GTK2-based browser to 'perf report' that's based on
    > the TTY-based browser in builtin-report.c.
    >
    > Please not that you need to use
    >
    > make WERROR=0
    >
    > to build perf on Fedora 15 (and possibly other distributions) because GTK
    > headers do not compile cleanly:
    >
    > CC util/gtk/browser.o
    > In file included from /usr/include/gtk-2.0/gtk/gtk.h:234:0,
    > from util/gtk/browser.c:7:
    > /usr/include/gtk-2.0/gtk/gtkitemfactory.h:47:1: warning: function declaration isn’t a prototype [-Wstrict-prototypes]
    >
    > To launch "perf report" using the new GTK interface just type:
    >
    > ./perf report --gtk
    >
    > The interface is somewhat limited in features at the moment:
    >
    > - No callgraph support
    >
    > - No KVM guest profiling support
    >
    > - No color coding for percentages
    >
    > - No sorting from the UI
    >
    > - ..and many, many more!
    >
    > That said, I think this patch a reasonable start to build future features on.
    >

    Looks like a nice patch! Few minor nits below.


    > Cc: Peter Zijlstra<a.p.zijlstra@chello.nl>
    > Cc: Paul Mackerras<paulus@samba.org>
    > Cc: Ingo Molnar<mingo@elte.hu>
    > Cc: Arnaldo Carvalho de Melo<acme@ghostprotocols.net>
    > Signed-off-by: Pekka Enberg<penberg@kernel.org>
    > ---
    > tools/perf/Documentation/perf-report.txt | 2 +
    > tools/perf/Makefile | 14 +++
    > tools/perf/builtin-report.c | 23 +++-
    > tools/perf/config/feature-tests.mak | 13 ++
    > tools/perf/util/cache.h | 12 ++
    > tools/perf/util/gtk/browser.c | 189 ++++++++++++++++++++++++++++++
    > tools/perf/util/hist.h | 17 +++
    > 7 files changed, 264 insertions(+), 6 deletions(-)
    > create mode 100644 tools/perf/util/gtk/browser.c
    >
    > diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
    > index 9b430e9..9654f27 100644
    > --- a/tools/perf/Documentation/perf-report.txt
    > +++ b/tools/perf/Documentation/perf-report.txt
    > @@ -110,6 +110,8 @@ OPTIONS
    > requires a tty, if one is not present, as when piping to other
    > commands, the stdio interface is used.
    >
    > +--gtk:: Use the GTK2 interface.
    > +
    > -k::
    > --vmlinux=<file>::
    > vmlinux pathname
    > diff --git a/tools/perf/Makefile b/tools/perf/Makefile
    > index e011b50..371f114 100644
    > --- a/tools/perf/Makefile
    > +++ b/tools/perf/Makefile
    > @@ -485,6 +485,20 @@ else
    > endif
    > endif
    >
    > +ifdef NO_GTK2
    > + BASIC_CFLAGS += -DNO_GTK2

    s/-DNO_GTK2/-DNO_GTK2_SUPPORT/ ?


    > +else
    > + FLAGS_GTK2=$(ALL_CFLAGS) $(ALL_LDFLAGS) $(EXTLIBS) $(shell pkg-config --libs --cflags gtk+-2.0)
    > + ifneq ($(call try-cc,$(SOURCE_GTK2),$(FLAGS_GTK2)),y)
    > + msg := $(warning GTK2 not found, disables GTK2 support. Please install gtk2-devel or libgtk2.0-dev);
    > + BASIC_CFLAGS += -DNO_GTK2_SUPPORT
    > + else
    > + BASIC_CFLAGS += $(shell pkg-config --cflags gtk+-2.0)
    > + EXTLIBS += $(shell pkg-config --libs gtk+-2.0)
    > + LIB_OBJS += $(OUTPUT)util/gtk/browser.o
    > + endif
    > +endif
    > +
    > ifdef NO_LIBPERL
    > BASIC_CFLAGS += -DNO_LIBPERL
    > else
    > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
    > index 25d34d4..d6bf6ec 100644
    > --- a/tools/perf/builtin-report.c
    > +++ b/tools/perf/builtin-report.c
    > @@ -40,7 +40,7 @@ struct perf_report {
    > struct perf_tool tool;
    > struct perf_session *session;
    > char const *input_name;
    > - bool force, use_tui, use_stdio;
    > + bool force, use_tui, use_gtk, use_stdio;
    > bool hide_unresolved;
    > bool dont_use_callchains;
    > bool show_full_info;
    > @@ -326,8 +326,13 @@ static int __cmd_report(struct perf_report *rep)
    > }
    >
    > if (use_browser> 0) {
    > - perf_evlist__tui_browse_hists(session->evlist, help,
    > - NULL, NULL, 0);
    > + if (use_browser == 1) {
    > + perf_evlist__tui_browse_hists(session->evlist, help,
    > + NULL, NULL, 0);
    > + } else if (use_browser == 2) {
    > + perf_evlist__gtk_browse_hists(session->evlist, help,
    > + NULL, NULL, 0);
    > + }
    > } else
    > perf_evlist__tty_browse_hists(session->evlist, rep, help);
    >
    > @@ -474,6 +479,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __used)
    > OPT_STRING(0, "pretty",&report.pretty_printing_style, "key",
    > "pretty printing style key: normal raw"),
    > OPT_BOOLEAN(0, "tui",&report.use_tui, "Use the TUI interface"),
    > + OPT_BOOLEAN(0, "gtk",&report.use_gtk, "Use the GTK2 interface"),
    > OPT_BOOLEAN(0, "stdio",&report.use_stdio,
    > "Use the stdio interface"),
    > OPT_STRING('s', "sort",&sort_order, "key[,key2...]",
    > @@ -526,6 +532,8 @@ int cmd_report(int argc, const char **argv, const char *prefix __used)
    > use_browser = 0;
    > else if (report.use_tui)
    > use_browser = 1;
    > + else if (report.use_gtk)
    > + use_browser = 2;
    >
    > if (report.inverted_callchain)
    > callchain_param.order = ORDER_CALLER;
    > @@ -537,9 +545,12 @@ int cmd_report(int argc, const char **argv, const char *prefix __used)
    > report.input_name = "perf.data";
    > }
    >
    > - if (strcmp(report.input_name, "-") != 0)
    > - setup_browser(true);
    > - else
    > + if (strcmp(report.input_name, "-") != 0) {
    > + if (report.use_gtk)
    > + perf_gtk_setup_browser(argc, argv, true);
    > + else
    > + setup_browser(true);
    > + } else

    Wouldn't it be better if setup_browser() handled this internally based on the
    value of use_browser?


    > use_browser = 0;
    >
    > /*
    > diff --git a/tools/perf/config/feature-tests.mak b/tools/perf/config/feature-tests.mak
    > index 6170fd2..f5b551c 100644
    > --- a/tools/perf/config/feature-tests.mak
    > +++ b/tools/perf/config/feature-tests.mak
    > @@ -65,6 +65,19 @@ int main(void)
    > endef
    > endif
    >
    > +ifndef NO_GTK2
    > +define SOURCE_GTK2
    > +#include<gtk/gtk.h>
    > +
    > +int main(int argc, char *argv[])
    > +{
    > + gtk_init(&argc,&argv);
    > +
    > + return 0;
    > +}
    > +endef
    > +endif
    > +
    > ifndef NO_LIBPERL
    > define SOURCE_PERL_EMBED
    > #include<EXTERN.h>
    > diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
    > index fc5e5a0..8dd224d 100644
    > --- a/tools/perf/util/cache.h
    > +++ b/tools/perf/util/cache.h
    > @@ -45,6 +45,18 @@ void setup_browser(bool fallback_to_pager);
    > void exit_browser(bool wait_for_ok);
    > #endif
    >
    > +#ifdef NO_GTK2_SUPPORT
    > +static inline void perf_gtk_setup_browser(int argc __used, const char *argv[] __used, bool fallback_to_pager)
    > +{
    > + if (fallback_to_pager)
    > + setup_pager();
    > +}
    > +static inline void perf_gtk_exit_browser(bool wait_for_ok __used) {}
    > +#else
    > +void perf_gtk_setup_browser(int argc, const char *argv[], bool fallback_to_pager);
    > +void perf_gtk_exit_browser(bool wait_for_ok);
    > +#endif
    > +
    > char *alias_lookup(const char *alias);
    > int split_cmdline(char *cmdline, const char ***argv);
    >
    > diff --git a/tools/perf/util/gtk/browser.c b/tools/perf/util/gtk/browser.c
    > new file mode 100644
    > index 0000000..4878279
    > --- /dev/null
    > +++ b/tools/perf/util/gtk/browser.c

    I think it needs to be moved under util/ui. There will be some share point
    between tui and gui IMHO. Otherwise, if it is too much depends on TUI, how
    about rename util/ui to util/tui?

    Thanks,
    Namhyung
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-02-24 02:01    [W:0.036 / U:58.760 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site