lkml.org 
[lkml]   [2010]   [Mar]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 5/5] perf report: Initial TUI using newt

* Arnaldo Carvalho de Melo <acme@infradead.org> wrote:

> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> Newt has widespread availability and provides a rather simple API as can be
> seen by the size of this patch.
>
> The work needed to support it will benefit other frontends too.
>
> In this initial patch it just checks if the output is a tty, if not it falls
> back to the previous behaviour, also if newt-devel/libnewt-dev is not
> installed the previous behaviour is maintaned.
>
> Pressing enter on a symbol will annotate it, ESC in the annotation window will
> return to the report symbol list.

Very nice!

a few observations. Firstly, could we perhaps make most of the interface
functions GUI/TUI invariant? I.e. things like:

> + if (use_browser)
> + r = vfprintf(fp, fmt, args);
> + else
> + r = color_vfprintf(fp, color, fmt, args);

should be abstracted away into a single method:

r = color_vprintf(fp, color, fmt, args);

where color_vprintf() knows about which current GUI front-end to use.

(The old color_printf() should be renamed to ascii_color_printf() or so, and
put into a front-end driver structure perhaps - instead of explicit flags.)

There's a similar situation here too:

> - ret = vfprintf(stderr, fmt, args);
> + if (use_browser)
> + ret = browser__show_help(fmt, args);
> + else
> + ret = vfprintf(stderr, fmt, args);

Plus a few other observations about the newt TUI itself:

- The most important first-impression thing in a TUI is to make it obvious to
exit it. I eventually found that Escape would exit - but it would be nice
to map 'Q' and 'Ctrl-C' to it as well. Nothing is more annoying than a TUI
you cannot exit from.

- There's still a 'perf annotate' bug that has been introduced recently, and
it shows up in the TUI too. The bug is due to us passing this to objdump
and grep:

18573 execve("/bin/sh", ["sh", "-c", "objdump --start-address=0xffffffff81387b36
--stop-address=0xffffffff81387b4f -dS [kernel.kallsyms]|grep -v [kernel.kallsyms]"]

Look at how [kernel.kallsyms] goes unquoted to the shell, so globbing will
match it on random file names in the current directory - which will then be
showed by objdump, much to the surprise of the user!

- I suspect we should finally make use of the .perfconfig parser and enable people
to use a different front-end from Newt? Just in case they prefer ASCII.

- When i hit enter on a symbol to annotate it, but the annotation fails, the TUI
just does nothing currently. Instead it should print something informative
(and eye-catching) into a status line at the top or the bottom of the
screen, possibly printed in red characters or so. Not a separate window as that
needs extra key-hits to get rid of - just a sufficiently visible status
line would be perfect. There can be a few reasons why some functions can be
annotated while others cannot be.

- [ call-graph data is not represented yet :-) ]

Anyway, very nice stuff!

Ingo


\
 
 \ /
  Last update: 2010-03-12 10:49    [W:0.090 / U:0.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site