Re: [PATCHSET 0/6] perf tools: A couple of TUI improvements

[Namhyung Kim <namhyung@kernel.org>]

Hi Ingo,

On Thu, 19 Dec 2013 13:14:35 +0100, Ingo Molnar wrote:
> * Namhyung Kim <namhyung@kernel.org> wrote:
>
>> Hello,
>> 
>> I was playing with TUI code and added two new windows.  One for
>> showing log messages and another for showing header information.
>> (Maybe they can be implemented on the GTK code too someday.)
>> 
>> Please try to use it! :)
>> 
>> I put the patches on 'perf/tui-v1' branch in my tree:
>> 
>>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
>> 
>> Any feedbacks are more than welcome, thanks
>> Namhyung
>
> In the morning haze it took me some time to figure out that typing 'l' 
> gives leads to the log window, 'i' to the header information.
>
> 'l' seems to work (no log messages though :-), but in perf-top 'i' 
> segfaulted:
>
>   comet:~/tip/tools/perf> perf top
>   perf: Segmentation fault

Ah, forgot to test perf top - I just played with perf report at this
time. :-/

>
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7ffff0d39700 (LWP 14785)]
> perf_header__fprintf_info (session=session@entry=0x957bf0, fp=fp@entry=0x7fffec000a30, full=full@entry=true) at util/header.c:2195
> 2195            int fd = perf_data_file__fd(session->file);
> Missing separate debuginfos, use: debuginfo-install audit-libs-2.3.2-1.fc19.x86_64 bzip2-libs-1.0.6-8.fc19.x86_64 elfutils-libelf-0.156-5.fc19.x86_64 elfutils-libs-0.156-5.fc19.x86_64 glibc-2.17-19.fc19.x86_64 libgcc-4.8.2-1.fc19.x86_64 libunwind-1.1-2.fc19.x86_64 nss-softokn-freebl-3.15.2-2.fc19.x86_64 numactl-libs-2.0.8-4.fc19.x86_64 perl-libs-5.16.3-266.fc19.x86_64 python-libs-2.7.5-9.fc19.x86_64 slang-2.2.4-8.fc19.x86_64 xz-libs-5.1.2-4alpha.fc19.x86_64 zlib-1.2.7-10.fc19.x86_64
> (gdb) bt
> #0  perf_header__fprintf_info (session=session@entry=0x957bf0, fp=fp@entry=0x7fffec000a30, full=full@entry=true) at util/header.c:2195
> #1  0x00000000004ce3fe in tui__header_window (env=env@entry=0x957c30) at ui/browsers/header.c:92
> #2  0x00000000004cbdfd in perf_evsel__hists_browse (evsel=evsel@entry=0x957560, nr_events=nr_events@entry=1, 
>     helpline=helpline@entry=0x57db88 "For a higher level overview, try: perf top --sort comm,dso", ev_name=0x957870 "cycles", left_exits=left_exits@entry=false, 
>     hbt=hbt@entry=0x7ffff0d38ea0, min_pcnt=min_pcnt@entry=0, env=env@entry=0x957c30) at ui/browsers/hists.c:1491
> #3  0x00000000004cd5a5 in perf_evlist__tui_browse_hists (evlist=0x8e5930, help=help@entry=0x57db88 "For a higher level overview, try: perf top --sort comm,dso", 
>     hbt=hbt@entry=0x7ffff0d38ea0, min_pcnt=0, env=0x957c30) at ui/browsers/hists.c:1957
> #4  0x0000000000433b48 in display_thread_tui (arg=0x7fffffffb150) at builtin-top.c:584
> #5  0x0000003e59407c53 in start_thread () from /lib64/libpthread.so.0
> #6  0x0000003e590f5dbd in clone () from /lib64/libc.so.6
> (gdb) 
>
> I suspect it got surprised by perf top not having a header per se? 
> Still it might make sense to also robustify 
> perf_header__fprintf_info() against segfaulting and such.

Right.  To be precise, perf top do have header.env but not set the
actual info in it.  I had to check it, sorry.  The patch below will fix
the problem.


>From 451a267fb2243860f62390151cc156a9eab7d317 Mon Sep 17 00:00:00 2001
From: Namhyung Kim <namhyung.kim@lge.com>
Date: Fri, 20 Dec 2013 09:14:23 +0900
Subject: [PATCH 1/3] perf ui/tui: fixup for header window on perf-top

It should check whether header.env is valid.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index d710c6403a14..c25859cae32d 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1488,7 +1488,9 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			tui__log_window();
 			continue;
 		case 'i':
-			tui__header_window(env);
+			/* env->arch is NULL for live-mode (i.e. perf top) */
+			if (env->arch)
+				tui__header_window(env);
 			continue;
 		case K_F1:
 		case 'h':
-- 
1.7.11.7


I'll fold it to the original patch.

>
> But it's a nice feature nevertheless!

Thanks! :)

>
> Btw., it would be nice if 'P' worked on these screens, so any 
> interesting data can be extracted! Cut & paste is usually a PITA due 
> to the graphical TUI characters.

Ah, okay.  I think it should be easy, will cook a patch.

>
> Btw., in case you are taking TUI usability bugreports, here's a few I 
> noticed while playing with your changes:

Hmm.. I'll take a look at them next week too. :)

>
> 1)
>
> In histogram view it would be nice if 'P' gave some status bar 
> indication that it just wrote to perf.hist.0 or so - otherwise the 
> user is kept in the dark.
>
> 2)
>
> Likewise, in a TUI every keypress must produce some tangible feedback 
> to the user. Try hitting 'o' for example - it should probably output 
> into the status bar that 'o' is not a bound keypress or so.
>
> 3)
>
> Same goes for page up / page down in histogram view if we are at the 
> end of the list: some low-key, single-character feedback should be 
> given that the keypress was seen but we are at the end of the list. 
> For exampe the scrollbar 'diamond' character could briefly 
> inverse-flash or so.
>
> Thanks,
>
> 	Ingo