[RFC] Reimplement input functions on top of libuv #395

tarruda · 2014-03-23T15:45:03Z

Functions that interact with read_cmd_fd were implemented on top libuv, using
uv_run() in conjunction with UV_RUN_ONCE.

This the input-related code extracted from #289 work, refactored to use a single thread. This is much simpler than listening for input from a background thread, while also letting us use libuv infrastructure for listening events from multiple sources without the added multithreading complexity. For example, this branch won't have the SIGTTIN problem detected by @oni-link, since there's no background thread reading input when Neovim starts a shell in the foreground.

RFC from @oni-link @mahkoh @philix @saghul @lslah @stefan991

mahkoh · 2014-03-23T16:05:16Z

src/os/io.c

+
+static void initialize_event_loop()
+{
+  in_buffer.wpos = in_buffer.rpos = in_buffer.apos = in_buffer.fpos = 0;


in_buffer is already 0.

Isn't it a good practice to be explicit?

tarruda · 2014-03-24T19:18:46Z

Did you use uv_guess_handle and use a tty handle when it was detected to be a tty? Do you have a (small) test case?

I dont have a small test case now, but here's a copy/paste way to reproduce:

git clone git@github.com:tarruda/neovim
cd neovim
git checkout saghul-tty-bug
make cmake && make
./build/bin/nvim -u NONE

When you enter the editor, run the :sh command, apparently it won't do anything but in fact it enters and exit the shell immediatelly with status 0. I stepped into the shell process to see what was going on but it seemed as if the shell exited due to EOF. This may be related to some lack of teardown code though

oni-link · 2014-03-24T19:55:48Z

src/os/io.c

+static uv_timer_t timer_req;
+static uv_handle_type read_stream_type;
+static InputBuffer in_buffer;
+static bool initialized = false, eof = false;


initialized is only used in io_poll. Can we make it local to io_poll or
do we need this for later?

Actually I was planning to call the initalization function directly from main. This is just temporary

tarruda · 2014-03-25T13:21:28Z

@stefan991 / @philix how is this branch "looking" on OS X?

stefan991 · 2014-03-25T14:16:24Z

@tarruda The UI looks good, I haven't got any crashes so far, resizing doesn't work (as described in my last comment: #395 (comment)).

But there is a problem running the unittests:

stefan at air in ~/Dev/neovim on git:tarruda-libuv-input-nothreads
% make unittest 
...
[100%] Built target nvim-test

●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●●○/Users/stefan/Dev/neovim/.deps/usr/bin/busted: line 41: 74308 Abort trap: 6           $COMMAND $BOOTSTRAP_PATH $*
[100%] Built target unit test

The tests abort, but cmake doesn't get that there is an error.
I tried to debug this, here is the stacktrace I got:

stefan at air in ~/Dev/neovim on git:tarruda-libuv-input-nothreads
% lldb .deps/usr/bin/luajit -- .deps/usr/bin/busted_bootstrap  -o TAP --pattern=.moon test
Current executable set to '.deps/usr/bin/luajit' (x86_64).
(lldb) run
Process 74352 launched: '/Users/stefan/Dev/neovim/.deps/usr/bin/luajit' (x86_64)
1..63
ok 1 - misc1 function / fullpathcmp / returns FPC_SAME when passed the same file
...
ok 53 - fs function / os_file_exists / returns FALSE when given a non-existing file
ok 54 - fs function / os_file_exists / returns TRUE when given an existing file
Process 74352 stopped
* thread #1: tid = 0x575234, 0x00007fff8cf69866 libsystem_kernel.dylib`__pthread_kill + 10, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
    frame #0: 0x00007fff8cf69866 libsystem_kernel.dylib`__pthread_kill + 10
libsystem_kernel.dylib`__pthread_kill + 10:
-> 0x7fff8cf69866:  jae    0x7fff8cf69870            ; __pthread_kill + 20
   0x7fff8cf69868:  movq   %rax, %rdi
   0x7fff8cf6986b:  jmpq   0x7fff8cf66175            ; cerror_nocancel
   0x7fff8cf69870:  ret    
(lldb) thread backtrace 
* thread #1: tid = 0x575234, 0x00007fff8cf69866 libsystem_kernel.dylib`__pthread_kill + 10, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
  * frame #0: 0x00007fff8cf69866 libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff8697d35c libsystem_pthread.dylib`pthread_kill + 92
    frame #2: 0x00007fff89957b1a libsystem_c.dylib`abort + 125
    frame #3: 0x0000000000701fb4 libnvim-test.so`uv_mutex_lock(mutex=<unavailable>) + 20 at thread.c:70
    frame #4: 0x00000000006f5237 libnvim-test.so`delay(ms=1000) + 39 at time.c:48
    frame #5: 0x00000000006f5203 libnvim-test.so`mch_delay(ms=1000, ignoreinput=false) + 115 at time.c:39
    frame #6: 0x000000010000641d luajit`___lldb_unnamed_function252$$luajit + 132
    frame #7: 0x0000000100050955 luajit`___lldb_unnamed_function828$$luajit + 2037
    frame #8: 0x0000000100065c7e luajit`___lldb_unnamed_function1085$$luajit + 78
    frame #9: 0x000000010000429b luajit`___lldb_unnamed_function104$$luajit + 52
    frame #10: 0x0000000100014524 luajit`lua_pcall + 100
    frame #11: 0x0000000100002519 luajit`___lldb_unnamed_function7$$luajit + 89
    frame #12: 0x0000000100001d9f luajit`___lldb_unnamed_function4$$luajit + 2015
    frame #13: 0x000000010000429b luajit`___lldb_unnamed_function104$$luajit + 52
    frame #14: 0x0000000100014569 luajit`lua_cpcall + 25
    frame #15: 0x00000001000014bb luajit`main + 59
    frame #16: 0x0000000100001455 luajit`___lldb_unnamed_function1$$luajit + 227
(lldb)

stefan991 · 2014-03-25T14:36:32Z

@tarruda: found the error, it was introduced with #393. time_init() was not called in the unittests, simple fix:

diff --git a/test/unit/os/time.moon b/test/unit/os/time.moon
index 76ad86b..e963f6e 100644
--- a/test/unit/os/time.moon
+++ b/test/unit/os/time.moon
@@ -4,6 +4,9 @@
 time = cimport './src/os/time.h'

 describe 'time function', ->
+  setup ->
+    time.time_init!
+
   describe 'mch_delay', ->
     mch_delay = (ms) ->
       time.mch_delay ms, false

tarruda · 2014-03-25T14:38:06Z

@stefan991 thanks for finding and debugging the issue, do you want to send a PR with the fix? If not I will merge it in this branch. There's also the issue of cmake not detecting the test failure, can you look into this @jszakmeister ?

stefan991 · 2014-03-25T14:39:15Z

@tarruda: I will do a quick pull request :)

jszakmeister · 2014-03-25T15:39:07Z

@tarruda Yes, I'll take a look tonight or early in the morning.

jszakmeister · 2014-03-25T17:12:42Z

@tarruda I had a few cycles over lunch. I pushed @stefan991's fix and an update to the cmake scripts to notice unit test failures. Sorry about that!

oni-link · 2014-03-25T19:58:13Z

Now that we can replace vim_mem* functionality with C99 functions, should we add explicitly the headers for memset and memmove?

felipecrv · 2014-03-25T20:58:14Z

@oni-link I'm working on it
On Mar 25, 2014 4:58 PM, "oni-link" notifications@github.com wrote:

Now that we can replace vim_mem* functionality with C99 functions, should
we add explicitly the headers for memset and memmove?

Reply to this email directly or view it on GitHubhttps://github.com//pull/395#issuecomment-38612813
.

oni-link · 2014-03-25T23:41:02Z

src/os/io.c

+}
+
+/* Poll the system for user input */
+PollResult io_poll(int32_t ms)


Missing static for consistency.

oni-link · 2014-03-26T11:14:48Z

In fill_input_buf https://github.com/tarruda/neovim/blob/libuv-input-nothreads/src/ui.c#L537 it is possible that read_cmd_fd is changed after pipe reads are set up in io_init. So uv_read_start
could read from the wrong descriptor.

tarruda · 2014-03-26T11:27:42Z

@oni-link I was aware of that. I think that should probably be handled in read_cb, by altering the file descriptor on error/EOF and make the appropriate changes to the libuv stream

tarruda · 2014-03-26T13:03:01Z

@oni-link inbuf_poll is for taking the input buffer into consideration when polling for characters. I'm trying to use the module name as the prefix for public functions, but in this case it doesn't apply.

oni-link · 2014-03-26T16:34:29Z

Don't know if this belongs here, but
echo "Makefile" | xargs nvim -u NONE -c ":q"
leaves my terminal in a bad state. Same happens with vim.

tarruda · 2014-03-26T17:44:04Z

@oni-link I have refactored to normalize(at least for a bit) the different semantics of reading from file descriptors pointing to files. The idea is to use an idle watcher(triggers an 'idle event' when there are no other pending events) to perform the actual reading synchronously.

If no one has objections I will squash the commits, and vim "event loop"(where events are only keypresses) is now officially integrated with libuv event loop in a efficient way without multithreading. I was blind by not seeing UV_RUN_ONCE/UV_RUN_NOWAIT as simple a way to integrate vim with libuv, in my mind multithreading was the only possible path.

After this we'll finally be able extend vim multitasking capabilities by simply adding libuv event listeners. Plugins, sockets, jobs, signals etc will all be polled by libuv and vim own event loop is only awaken when keys are placed in the read buffer.

oni-link · 2014-03-26T18:40:23Z

@tarruda With the idle_watcher the file read has the lowest priority. Would that be problematic in the future, when more and more events can be triggered? Or would that be in a different event loop?

tarruda · 2014-03-26T19:07:17Z

@oni-link I dont think that is a problem. When stdin is redirected from a file I can see two scenarios:

The file is empty, which will trigger the stderr_switch function
The file has some commands, and unless it has a quit command, vim will exit due to EOF read error.

Even if it wasn't the case(if reading from a terminal fed by the user for example), having a low priority would only be a problem if vim is flooded with signals or input from other sources such as plugins, and even like that I doubt the user would notice any delays.

The functions `mch_inchar`, `mch_breakcheck`, `mch_char_avail` were reimplemented on top of libuv. Here's how it works: - When Neovim needs to wait for characters, it will transfer control to libuv event loop. - When the libuv event loop gets user input, it will transfer control back to Neovim - Neovim uses the `input_read` function to get the actual data read by libuv. With this scheme its possible to keep Neovim single-threaded while enjoying the benefits provided by libuv. This commit leaves SIGWINCH broken for now

tarruda · 2014-03-26T21:57:08Z

Merged, now need to fix signals

felipecrv · 2014-03-26T22:31:22Z

@tarruda you have asked how stable this branch was on the Mac. I've been running nvim based on this branch and the only issues are related to redrawing. Some commands would make Airline disappear. I'm pretty sure this is related to the removal of resize handling in the input loop.

I'm glad you divided #289 into smaller, more manageable diffs.

Fixes neovim#395

mahkoh reviewed Mar 23, 2014
View reviewed changes

oni-link reviewed Mar 24, 2014
View reviewed changes

stefan991 mentioned this pull request Mar 25, 2014

[RDY] fix mch_delay() unittest #399

Merged

tarruda mentioned this pull request Mar 25, 2014

[RFC] add -Wvla #394

Closed

oni-link reviewed Mar 25, 2014
View reviewed changes

src/os/io.c

}

/* Poll the system for user input */

PollResult io_poll(int32_t ms)

Copy link

Contributor

oni-link Mar 25, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing static for consistency.

tarruda merged commit 4528046 into neovim:master Mar 26, 2014

tarruda mentioned this pull request Mar 29, 2014

[RFC] Migrate input reading to a libuv event loop #289

Closed

tarruda mentioned this pull request Apr 7, 2014

[RFC] Reimplement job control patch with libuv #475

Merged

Grimy pushed a commit to Grimy/neovim that referenced this pull request Jan 7, 2015

Adding DLL as a valid extension for ycm_core

2741520

Fixes neovim#395

Uh oh!

[RFC] Reimplement input functions on top of libuv #395

[RFC] Reimplement input functions on top of libuv #395

Uh oh!

Conversation

tarruda commented Mar 23, 2014

Uh oh!

mahkoh Mar 23, 2014

Choose a reason for hiding this comment

Uh oh!

tarruda Mar 24, 2014

Choose a reason for hiding this comment

Uh oh!

tarruda commented Mar 24, 2014

Uh oh!

oni-link Mar 24, 2014

Choose a reason for hiding this comment

Uh oh!

tarruda Mar 24, 2014

Choose a reason for hiding this comment

Uh oh!

tarruda commented Mar 25, 2014

Uh oh!

stefan991 commented Mar 25, 2014

Uh oh!

stefan991 commented Mar 25, 2014

Uh oh!

tarruda commented Mar 25, 2014

Uh oh!

stefan991 commented Mar 25, 2014

Uh oh!

jszakmeister commented Mar 25, 2014

Uh oh!

jszakmeister commented Mar 25, 2014

Uh oh!

oni-link commented Mar 25, 2014

Uh oh!

felipecrv commented Mar 25, 2014

Uh oh!

oni-link Mar 25, 2014

Choose a reason for hiding this comment

Uh oh!

oni-link commented Mar 26, 2014

Uh oh!

tarruda commented Mar 26, 2014

Uh oh!

tarruda commented Mar 26, 2014

Uh oh!

oni-link commented Mar 26, 2014

Uh oh!

tarruda commented Mar 26, 2014

Uh oh!

oni-link commented Mar 26, 2014

Uh oh!

tarruda commented Mar 26, 2014

Uh oh!

tarruda commented Mar 26, 2014

Uh oh!

felipecrv commented Mar 26, 2014

Uh oh!

Uh oh!