Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TUI (text user interface) support using ncurses library #326

Closed
namhyung opened this issue Mar 3, 2018 · 22 comments
Closed

TUI (text user interface) support using ncurses library #326

namhyung opened this issue Mar 3, 2018 · 22 comments

Comments

@namhyung
Copy link
Owner

namhyung commented Mar 3, 2018

Add a new command "tui" to implement user interface on a text console. This new command can do same thing with "graph" and "report" command. Also it can change the mode, fold/unfold graph nodes, and search functions dynamically. Following keys are used:

  • UP/DOWN, PAGE-UP/-DOWN, HOME/END - navigation
  • n/p - move to next or previous sibling (in graph mode)
  • u - move to parent node (in graph mode)
  • ENTER - fold/unfold current node (in graph mode), or show function call graph (in report mode)
  • c/e - recursively fold/unfold child node (in graph mode)
  • g/r - change to graph or report mode respectively
  • / - start search
  • </> - move next/previous node searched
  • ESC - end search
  • v - toggle debug mode
  • q - quit the tui

Here's a screenshot:
uftrace-tui

The code is available on the review/tui-support-v1 branch. Please give it a try! :)

@namhyung namhyung added the review label Mar 3, 2018
@honggyukim
Copy link
Collaborator

It looks great. Thanks for doing this work! But I wonder why my terminal shows the lines in a strange way.

  TOTAL TIME : FUNCTION
    2.048  m : (1) webkit_unit_tests
    2.048  m : (1) main
   16.011  s :  M-b~T~\M-b~T~@(1) base::TestSuite::TestSuite
  872.563 us :  M-b~T~B  M-b~T~\M-b~T~@(1) base::TestSuite::PreInitialize
  179.911 us :  M-b~T~B  M-b~T~B (1) malloc
  179.721 us :  M-b~T~B  M-b~T~B (1) ShimMalloc
  179.349 us :  M-b~T~B  M-b~T~B (1) _GLOBAL__N_1::TCMalloc
  179.148 us :  M-b~T~B  M-b~T~B (1) tc_malloc
  177.974 us :  M-b~T~B  M-b~T~B (1) do_debug_malloc_or_debug_cpp_alloc
  177.766 us :  M-b~T~B  M-b~T~B (1) DebugAllocate
  177.370 us :  M-b~T~B  M-b~T~B (1) MallocBlock::Allocate
  171.487 us :  M-b~T~B  M-b~T~B (1) _GLOBAL__N_1::do_malloc
  164.353 us :  M-b~T~B  M-b~T~B (1) tcmalloc::ThreadCache::Allocate
  163.540 us :  M-b~T~B  M-b~T~B (1) tcmalloc::ThreadCache::FetchFromCentralCache
  152.227 us :  M-b~T~B  M-b~T~B (1) tcmalloc::CentralFreeList::RemoveRange
             :  M-b~T~B  M-b~T~B
   16.009  s :  M-b~T~B  M-b~T~\M-b~T~@(1) base::TestSuite::InitializeFromCommandLine
    1.044 ms :  M-b~T~B  M-b~T~B  M-b~T~\M-b~T~@(1) base::CommandLine::Init
        ...

@honggyukim
Copy link
Collaborator

Could you please make vim users happy by adding j for down and k for up? :)

@honggyukim
Copy link
Collaborator

honggyukim commented Mar 4, 2018

I found the reason of broken output. I need to use libncursesw instead of libncurses so have to modify it as follows:

diff --git a/check-deps/Makefile b/check-deps/Makefile
index 59ce19c..44dba78 100644
--- a/check-deps/Makefile
+++ b/check-deps/Makefile
@@ -7,6 +7,7 @@ CHECK_LIST += have_libpython2.7
 CHECK_LIST += perf_clockid
 CHECK_LIST += perf_context_switch
 CHECK_LIST += arm_has_hardfp
+CHECK_LIST += have_libncursesw
 CHECK_LIST += have_libncurses

 #
@@ -21,6 +22,7 @@ LDFLAGS_cxa_demangle = -lstdc++
 LDFLAGS_have_libelf = -lelf
 CFLAGS_cc_has_mno_sse2 = -mno-sse2
 LDFLAGS_have_libpython2.7 = -lpython2.7
+LDFLAGS_have_libncursesw = -lncursesw
 LDFLAGS_have_libncurses = -lncurses

 check-build: check-tstamp $(CHECK_LIST)
diff --git a/check-deps/Makefile.check b/check-deps/Makefile.check
index 23241d6..20ca1b7 100644
--- a/check-deps/Makefile.check
+++ b/check-deps/Makefile.check
@@ -31,7 +31,12 @@ ifneq ($(wildcard $(srcdir)/check-deps/arm_has_hardfp),)
   COMMON_CFLAGS += -DHAVE_ARM_HARDFP
 endif

+ifneq ($(wildcard $(srcdir)/check-deps/have_libncursesw),)
+  COMMON_CFLAGS += -DHAVE_LIBNCURSESW
+  UFTRACE_LDFLAGS += -lncursesw -ltinfo
+else
 ifneq ($(wildcard $(srcdir)/check-deps/have_libncurses),)
   COMMON_CFLAGS += -DHAVE_LIBNCURSES
   UFTRACE_LDFLAGS += -lncurses -ltinfo
 endif
+endif
diff --git a/check-deps/__have_libncursesw.c b/check-deps/__have_libncursesw.c
new file mode 100644
index 0000000..4bbaedd
--- /dev/null
+++ b/check-deps/__have_libncursesw.c
@@ -0,0 +1,8 @@
+#include <ncursesw/curses.h>
+
+int main(void)
+{
+       initscr();
+       endwin();
+       return 0;
+}
diff --git a/cmd-tui.c b/cmd-tui.c
index 022b135..a99c78c 100644
--- a/cmd-tui.c
+++ b/cmd-tui.c
@@ -1,13 +1,18 @@
-#ifdef HAVE_LIBNCURSES
+#if defined(HAVE_LIBNCURSESW) || defined(HAVE_LIBNCURSES)

 #include <stdio.h>
 #include <stdlib.h>
 #include <stdint.h>
 #include <stdbool.h>
-#include <ncurses.h>
 #include <locale.h>
 #include <inttypes.h>

+#ifdef HAVE_LIBNCURSESW
+#include <ncursesw/curses.h>
+#elif HAVE_LIBNCURSES
+#include <ncurses.h>
+#endif
+
 #include "uftrace.h"
 #include "utils/utils.h"
 #include "utils/fstack.h"

Now it works fine.

@elfring
Copy link

elfring commented Mar 4, 2018

The screenshot looks mostly nice. I have spot areas where further fine-tuning is possible.

  • Would you like to offer any additional menu items in the header bar?
  • How do you think about to omit the column which contains the colon? (Can the field separation approach be reconsidered?)
  • How can the colour combination be adjusted for this text user interface?

I am also curious on further software evolution around implementation details.

@namhyung
Copy link
Owner Author

namhyung commented Mar 5, 2018

@honggyukim thanks, my system uses ncursesw by default. Maybe I need to change it link with ncursesw explicitly. And my system doesn't have the <ncursesw/curses.h> header - probably due to the version difference (I'm using 6.1)

@elfring I don't plan to add menu bar but popup menu can be added later. AFAIK there's no column contains colon. And I'm thinking of color customization as a low-priority work item.

@honggyukim
Copy link
Collaborator

@namhyung I also think that ncurses has some version difference in terms of header path.
I've got the below code snippet in tig project for handling such problem.

#if defined HAVE_NCURSESW_CURSES_H
#  include <ncursesw/curses.h>
#elif defined HAVE_NCURSESW_H
#  include <ncursesw.h>
#elif defined HAVE_NCURSES_CURSES_H
#  include <ncurses/curses.h>
#elif defined HAVE_NCURSES_H
#  include <ncurses.h>
#elif defined HAVE_CURSES_H
#  include <curses.h>
#else
#ifdef WARN_MISSING_CURSES_CONFIGURATION
#  warning SysV or X/Open-compatible Curses installation is required.
#  warning Will assume Curses is found in default include and library path.
#  warning To fix any build issues please use autotools to configure Curses.
#  warning See INSTALL.adoc file for instructions.
#endif
#  include <curses.h>
#endif

So we can handle it in a similar way.

@namhyung
Copy link
Owner Author

namhyung commented Mar 5, 2018

OMG what a mess..

@elfring
Copy link

elfring commented Mar 5, 2018

AFAIK there's no column contains colon.

I suggest to check the relevance of the identifier “FIELD_SEP” once more.


How do you think about to move the time unit display below the column header so that it would be printed only once?

@namhyung
Copy link
Owner Author

namhyung commented Mar 5, 2018

The FIELD_SEP is to separate fields from function graph so it's not a part of column header.

And time unit can be different for each line, why do you want to print it only once?

@elfring
Copy link

elfring commented Mar 5, 2018

… so it's not a part of column header.

Do you prefer any other description for the display “…TIME : FU…” in the screenshot?


And time unit can be different for each line,

This can be the default configuration.

why do you want to print it only once?

I propose to offer a slightly different report layout when the duration values can fit to a single time unit.

@namhyung
Copy link
Owner Author

namhyung commented Mar 5, 2018

@honggyukim maybe we can use pkg-config like below:

diff --git a/check-deps/Makefile b/check-deps/Makefile
index 59ce19c..9c57702 100644
--- a/check-deps/Makefile
+++ b/check-deps/Makefile
@@ -21,7 +21,8 @@ LDFLAGS_cxa_demangle = -lstdc++
 LDFLAGS_have_libelf = -lelf
 CFLAGS_cc_has_mno_sse2 = -mno-sse2
 LDFLAGS_have_libpython2.7 = -lpython2.7
-LDFLAGS_have_libncurses = -lncurses
+CFLAGS_have_libncurses = $(shell pkg-config --cflags ncursesw)
+LDFLAGS_have_libncurses = $(shell pkg-config --libs ncursesw)
 
 check-build: check-tstamp $(CHECK_LIST)
 
diff --git a/check-deps/Makefile.check b/check-deps/Makefile.check
index 23241d6..ee3b935 100644
--- a/check-deps/Makefile.check
+++ b/check-deps/Makefile.check
@@ -32,6 +32,6 @@ ifneq ($(wildcard $(srcdir)/check-deps/arm_has_hardfp),)
 endif
 
 ifneq ($(wildcard $(srcdir)/check-deps/have_libncurses),)
-  COMMON_CFLAGS += -DHAVE_LIBNCURSES
-  UFTRACE_LDFLAGS += -lncurses -ltinfo
+  COMMON_CFLAGS += -DHAVE_LIBNCURSES $(shell pkg-config --cflags ncursesw)
+  UFTRACE_LDFLAGS += $(shell pkg-config --libs ncursesw)
 endif
diff --git a/check-deps/__have_libncurses.c b/check-deps/__have_libncurses.c
index 0c1ef2d..499f79f 100644
--- a/check-deps/__have_libncurses.c
+++ b/check-deps/__have_libncurses.c
@@ -1,4 +1,4 @@
-#include <ncurses.h>
+#include <curses.h>
 
 int main(void)
 {
diff --git a/cmd-tui.c b/cmd-tui.c
index 022b135..236e100 100644
--- a/cmd-tui.c
+++ b/cmd-tui.c
@@ -4,7 +4,7 @@
 #include <stdlib.h>
 #include <stdint.h>
 #include <stdbool.h>
-#include <ncurses.h>
+#include <curses.h>
 #include <locale.h>
 #include <inttypes.h>
 

@honggyukim
Copy link
Collaborator

Thanks @namhyung! It has been a lot simpler now. :)

@namhyung
Copy link
Owner Author

namhyung commented Mar 6, 2018

Pushed review/tui-support-v2

Changelog:

  • use pkg-config to find ncursesw
  • add k/j key for up/down

@elfring
Copy link

elfring commented Mar 6, 2018

add k/j key for up/down

@namhyung
Copy link
Owner Author

namhyung commented Mar 6, 2018

Yep it's on the todo list.

@honggyukim
Copy link
Collaborator

honggyukim commented Mar 6, 2018

Thanks a lot for the update. I have a few more requests.

  • Support stack structure when changing the view across graph and report modes.
    • Someone may want to go back to the previous view by using ESC key, for example.
  • Support search and sort feature in report mode.
  • Support to show self time in graph mode.
  • Make g key to change the graph target even in graph mode as if a specific function is chosen in report mode.
  • When the target function is chosen, backtrace can show its direct caller function only and folds its parents. I found too many repeating and too long backtrace info, so better to show it simpler. The example is as follows:
             :=== Function Call Graph for 'blink::V8ScriptRunner::CompileScript' ===
             :========== Back-trace ==========
   15.021 ms :  ├─(1) blink::V8ScriptRunner::CompileScript
   15.021 ms :  │▶(1) blink::ScriptController::ExecuteScriptAndReturnValue
             :  │
    3.940 ms :  ├─(1) blink::V8ScriptRunner::CompileScript
    3.940 ms :  │▶(1) blink::ScriptController::ExecuteScriptAndReturnValue
             :  │
    7.762 ms :  ├─(2) blink::V8ScriptRunner::CompileScript
    7.762 ms :  │▶(2) blink::ScriptController::ExecuteScriptAndReturnValue
             :  │
    4.292 ms :  └─(1) blink::V8ScriptRunner::CompileScript
    4.292 ms :   ▶(1) blink::ScriptController::ExecuteScriptAndReturnValue
             :
             :========== Call Graph ==========
   31.017 ms : (5) blink::V8ScriptRunner::CompileScript
    9.381 ms :  ├─(4) blink::V8String
    9.370 ms :  │ (4) blink::StringCache::V8ExternalString
    9.367 ms :  │ (4) blink::StringCache::V8ExternalStringSlow
    9.311 ms :  │ (4) blink::StringCache::CreateStringAndInsertIntoCache
  148.117 us :  │  ├─(1) blink::MakeExternalString
        ...

@namhyung
Copy link
Owner Author

namhyung commented Apr 7, 2018

Pushed review/tui-support-v3

Changelog:

  • support (partial) graph display even in graph mode
  • fold backtrace at the first parent like above
  • change some key mappings
    • N/P - move to next/previous search result (as well as </> keys)
    • R - display report output (same as r)
    • G - display full graph output (previously g key did)
    • g - display partial graph output for the selected function

@honggyukim
Copy link
Collaborator

Thanks a lot for doing this. I have one more favor on it.

N/P - move to next/previous search result (as well as </> keys)

It's a matter of preference, but can it be n/N to search forward and backward instead of N/P? It will be same as search keys in vim and even in less as well.

Except for that, it's really great so I would like to use it very soon. So would it be possible to merge it soon then enhance it one by one later?

@namhyung
Copy link
Owner Author

namhyung commented May 1, 2018

It's a matter of preference, but can it be n/N to search forward and backward instead of N/P? It will be same as search keys in vim and even in less as well.

I know but I'd keep as is since I see a value in moving to next/prev siblings. I think it's good to have same keys ('n' and 'p') for same directions. And I'm an emacs user. :)

@honggyukim
Copy link
Collaborator

Ah.. There are p/n keys already for sibling search. I just wanted to make search related keys same as in replay command with pager but it's okay so. I didn't know that N/P are emacs keys. Thanks :)

@namhyung
Copy link
Owner Author

namhyung commented May 2, 2018

Just FYI, emacs search keys are different but it uses C-n/C-p for navigation. :)

@honggyukim
Copy link
Collaborator

Thanks a lot!

@honggyukim honggyukim added the tui label May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants