Messages in this thread | | | Date | Fri, 12 Mar 2010 10:25:09 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH v2 5/5] perf report: Initial TUI using newt |
| |
Em Fri, Mar 12, 2010 at 10:45:33AM +0100, Ingo Molnar escreveu: > > * 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.
Yeah, I'll get to that, its just that I wanted to have something out as small as possible and that pointed to the places that need refactoring to properly support multiple UI frontends.
> (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.)
Right
> 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.
Newt's default is F12, I had to killall perf while developing and getting to know newtFormAddHotKey to exit, will add the other usual keys to exit.
> - 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!
Will fix that, if we don't find a vmlinux file, the kernel symbols will be marked non annotable in some way.
> - 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.
Right.
> - [ call-graph data is not represented yet :-) ]
It will
> Anyway, very nice stuff!
Thanks!
- Arnaldo
| |