Skip to content
New issue

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

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

Already on GitHub? # to your account

Testers needed for multiprocess = true #248

Closed
ibhagwan opened this issue Dec 12, 2021 · 23 comments
Closed

Testers needed for multiprocess = true #248

ibhagwan opened this issue Dec 12, 2021 · 23 comments
Labels
help wanted Extra attention is needed

Comments

@ibhagwan
Copy link
Owner

Background:

2d8a4e9 - This commit adds a new option named multiprocess, best described from the commit message:

major performance improvement: process entries externally, READ BELOW:
Since LUA is single threaded I reached a limit to performance
optimization, both 'git_icons' and 'file_icons' require string
matching and manipulations which eventually hurt performance
when running on large amount of files.
In order to solve that this commit introduces the option to spawn
commands and process the entries in a separate neovim process which
prints to stdio as if it was a regular shell command. This speeds up
things significantly and also makes the UI super responsive as if fzf
was run in the shell. This required a few lua hacks to be able to load
nvim-web-devicons in a '--headless --clean' instance and sharing the
user configuration through the RPC interface from the running instance.
This is enabled by default for 'files' and 'grep' providers and can also
be enabled for 'git.files' if required, control using the 'multiprocess'
option.

Turns out I was a bit ballsy and there are still issues with it (e.g. #247).

Before I re-enable it again as default I'd like to have some testers use this (perferably on large codebases) and let me know the results.

It's only possible to enable it for files, grep and git_files, either through setup:

require'fzf-lua'.setup {
  files = {
    multiprocess      = true,
    debug             = false,
  },
  grep = {
    multiprocess      = true,
    debug             = false,
  },
  git = {
    files             = {
      multiprocess    = true,
      debug           = false,
    },
  }
}

Or by specifying it directly:

:lua require'fzf-lua'.files({ multiprocess = true })
-- or
:FzfLua files multiprocess=true

If debug=true is specified the neovim shell command will be printed into the :messages stream so you can run the command directly in the shell and see if everything seems right, the command wraps a shell command and prints the output to the shell after additonal processing, for example:

~'nvim' -n --headless --clean --cmd 'lua loadfile([[/home/bhagwan/.local/share/nvim/site/pack/packer/opt/fzf-lua/lua/fzf-lua/libuv.lua]])().spawn_stdio({debug=true,cmd=[[fd --color=never --type f --hidden --follow --exclude .git]],cwd=[[/home/bhagwan/.config/nvim/colors]],git_icons=false,file_icons=true,color_icons=true},[[_G._devicons_path='\''/home/bhagwan/.local/share/nvim/site/pack/packer/opt/nvim-web-devicons/lua/nvim-web-devicons.lua'\''; return require("make_entry").file]],[[return require("make_entry").preprocess]])'
 embark.vim
 lua-embark.vim
 lua-onedark.vim

The command above is then piped to fzf (as a separate process) increasing performance and freeing the neovim UI to do it's thing.

Testers, comments, issues all welcome in this thread.

@ibhagwan ibhagwan added the help wanted Extra attention is needed label Dec 12, 2021
@nyngwang
Copy link
Contributor

The good news is: I just changed the previewer from bat to builtin. Now I can experience that the update of 2d8a4e9 makes everything insanely super fast!

@ibhagwan
Copy link
Owner Author

The good news is: I just changed the previewer from bat to builtin. Now I can experience that the update of 2d8a4e9 makes everything insanely super fast!

Fantastic! I guess the fixes to your issue #247 fixed this as well.

ab8b1a1 - Latest commit restores multiprocess = true for files|grep, wish us luck :-)

@jdrouhard
Copy link
Contributor

Tested this on the neovim repository on my MacBook, still looks broken (like #247). I enabled debug mode, here's the command it runs:

nvim -n --headless --clean --cmd 'lua loadfile([[/Users/jdrouhard/.local/share/nvim/site/pack/packer/opt/fzf-lua/lua/fzf-lua/libuv.lua]])().spawn_stdio({debug=true,cmd=[[rg --files --hidden -g '\''!{.git,node_modules}/*'\'']],git_icons=true,file_icons=true,color_icons=true},[[_G._devicons_path='\''/Users/jdrouhard/.local/share/nvim/site/pack/packer/start/nvim-web-devicons/lua/nvim-web-devicons.lua'\''; _G._fzf_lua_server='\''/var/folders/tf/xqtg2q817ls11g6xmkz3zw6h0000gn/T/nvimPUrfeT/1'\''; return require("make_entry").file]],[[return require("make_entry").preprocess]])'

Running this in a terminal and piping through less is also messed up. It seems like that command is printing raw terminal escape codes which messes up anything that expects printable characters only. As I try to scroll around with less, the screen gets cleared and changed all over the place, and even sometimes opens the print dialog.

The top of the output also has:

Error loading remote config section 'globals.git.icons': Vim:connection failed: connection refused
Error loading remote config section 'globals.file_icon_colors': Vim:connection failed: connection refused
Error loading remote config section 'globals.file_icon_padding': Vim:connection failed: connection refused
Error loading remote config section 'globals.files.git_status_cmd': Vim:connection failed: connection refused

If I pipe the output to a file, and then cat the file through less, it works as expected (other than the errors about the config section). It seems to be only when the headless nvim command is directly piped to another process that displays the output.

@jdrouhard
Copy link
Contributor

jdrouhard commented Dec 13, 2021

As a followup, repeatedly running the command and piping to cat always terminates early with about the same number of characters printed to the screen, and sometimes even says "cat: stdout: Resource temporarily unavailable".

Is there something about how the stdout pipe from the nvim process sends EOF when it flushes a buffer even if there is still more to come? I haven't looked at the new implementation of this headless multiprocess mode but I think the stdout handling needs to be looked at a bit. Pipe should stay open the whole time and only send EOF/close the pipe when it's completely done.

@ibhagwan
Copy link
Owner Author

ibhagwan commented Dec 13, 2021

Vim:connection failed: connection refused

This is abosultely fine, if you're running the command with _G._fzf_lua_server specified but your neovim instance was probably no longer running (or the server address changed) it will fail to retrieve user customizations from the primary instance, this is normal.

To run without retreieving the config need to drop the _G._fzf_lua_server part, i.e:

nvim -n --headless --clean --cmd 'lua loadfile([[/Users/jdrouhard/.local/share/nvim/site/pack/packer/opt/fzf-lua/lua/fzf-lua/libuv.lua]])().spawn_stdio({debug=true,cmd=[[rg --files --hidden -g '\''!{.git,node_modules}/*'\'']],git_icons=true,file_icons=true,color_icons=true}, [[_G._devicons_path='\''/Users/jdrouhard/.local/share/nvim/site/pack/packer/start/nvim-web-devicons/lua/nvim-web-devicons.lua'\''; return require("make_entry").file]],[[return require("make_entry").preprocess]])'

It seems like that command is printing raw terminal escape codes which messes up anything that expects printable characters only.

The escape codes might be the issue! I'll test against different terminals (been using alacritty), would be interesting to run with color_icons=false and see if this solves the issue (should print icons and git indicators but no color escape sequences)?

If I pipe the output to a file, and then cat the file through less, it works as expected (other than the errors about the config section). It seems to be only when the headless nvim command is directly piped to another process that displays the output.

I wonder what that may be, I'm going to install different terminals and do some testing (xterm, urxvt, kitty).

As a followup, repeatedly running the command and piping to cat always terminates early with about the same number of characters printed to the screen, and sometimes even says "cat: stdout: Resource temporarily unavailable".
Is there something about how the stdout pipe from the nvim process sends EOF when it flushes a buffer even if there is still more to come? I haven't looked at the new implementation of this headless multiprocess mode but I think the stdout handling needs to be looked at a bit. Pipe should stay open the whole time and only send EOF/close the pipe when it's completely done.

I'll look into this, ty so much for doing this and giving me great leads, I've disabled it as default for now as it seems that it needs some more polishing before being released in the wild :-)

I feel it's close though!

Let me know what you find out with color_icons=false?

@jdrouhard
Copy link
Contributor

Let me know what you find out with color_icons=false?

Didn't help. It's not the color escape codes, it's definitely the output pipe from the headless nvim process not behaving correctly. I don't think the terminal will affect this since pipes are an OS thing. It might be a macOS pipe implementation thing since #251 also seems to be someone using a Mac.

@ibhagwan
Copy link
Owner Author

Let me know what you find out with color_icons=false?

Didn't help. It's not the color escape codes, it's definitely the output pipe from the headless nvim process not behaving correctly. I don't think the terminal will affect this since pipes are an OS thing. It might be a macOS pipe implementation thing since #251 also seems to be someone using a Mac.

I wonder why, the pipe code is failry simple, roughly:

  -- I open two pipes to "/dev/stdout" and
  -- "/dev/stderr" for error messages
  local fd = uv.fs_open("/dev/stdout", "w", -1)
  local pipe = uv.new_pipe(false)
  pipe:open(fd)
  -- then writing to the pipe with:
  pipe:write(data, function(err) ... end)

I'll have to boot to my MacOS to test this I guess, haven't done so in a long time :-)

@ibhagwan
Copy link
Owner Author

@jdrouhard it's possible to specify the output pipe in the command, would you be able to run a quick test and see which of these devices work best for you?

  • /dev/tty
  • /dev/tty0
  • /dev/console

Below is how to modify the command to add the device (/dev/tty) in this example:

nvim -n --headless --clean --cmd 'lua loadfile([[/Users/jdrouhard/.local/share/nvim/site/pack/packer/opt/fzf-lua/lua/fzf-lua/libuv.lua]])().spawn_stdio({stdout=[[/dev/tty]],cmd=[[rg --files --hidden -g '\''!{.git,node_modules}/*'\'']],git_icons=true,file_icons=true,color_icons=true}, [[_G._devicons_path='\''/Users/jdrouhard/.local/share/nvim/site/pack/packer/start/nvim-web-devicons/lua/nvim-web-devicons.lua'\''; return require("make_entry").file]],[[return require("make_entry").preprocess]])'

@jdrouhard
Copy link
Contributor

/dev/tty

Doesn't exhibit the premature EOF problem, but also isn't pipeable (it doesn't print to stdout) so I don't think this will actually work.

/dev/tty0

Doesn't exist for me. My tty happens to be ttys008, which has the same result as using /dev/tty.

/dev/console

Prints nothing.

@ibhagwan
Copy link
Owner Author

/dev/tty

Doesn't exhibit the premature EOF problem, but also isn't pipeable (it doesn't print to stdout) so I don't think this will actually work.

/dev/tty0

Doesn't exist for me. My tty happens to be ttys008, which has the same result as using /dev/tty.

/dev/console

Prints nothing.

I have an idea, I'm going to use lua io.write instead of /dev/stdout, give me 2 minutes and you can test it :-)

@ibhagwan
Copy link
Owner Author

@jdrouhard can you try this 4a013fd?

@jdrouhard
Copy link
Contributor

jdrouhard commented Dec 13, 2021

Step ahead of you :) I tried that (missed cb(nil) which was the piece I was missing - needed for the headless nvim to finish/write EOF) but I did it for both stdout and stderr and this fully fixes the issue.

You can do

io.stdout:write(data)
io.stderr:write(data)

for stdout/stderr. I think this will fully fix it. Thanks again for the quick turnaround!

@jdrouhard
Copy link
Contributor

Correction, that commit doesn't fix it, but I have a diff that does. One sec....

@ibhagwan
Copy link
Owner Author

Step ahead of you :) I tried that (missed cb(nil) which was the piece I was missing - needed for the headless nvim to finish/write EOF) but I did it for both stdout and stderr and this fully fixes the issue.

This is amazing! so happy to have this properly fixed the performance enhancement is huge :-)

@ibhagwan
Copy link
Owner Author

Tysm @jdrouhard! just merged #252!

@ibhagwan
Copy link
Owner Author

@jdrouhard, just got confirmation it solved #251, with that in mind I feel this is ready to become the default once again :)

1a98c89

@ibhagwan
Copy link
Owner Author

ibhagwan commented Dec 13, 2021

@jdrouhard, there seems to be an issue with pressing <C-c> to cancel a running command (files or grep):

In a large codebse, if I just run the command in the shell (no need for neovim/fzf-lua) and press <C-c> the command exits immedieately, but if run it and pipe it to fzf (still from the shell:nvim --headless ... | fzf) the command seems to hang there for around 4 seconds extra before returning to the shell.

Not sure why yet, any ideas welcome.

Edit 1: it seems that if running the command piped data is still being received even after pressing <C-c>

@jdrouhard
Copy link
Contributor

My first guess is cb(nil) in on_write() might need to be smarter. I'll have to play with it a bit myself later tomorrow maybe since I don't currently have my large codebase up to try it out.

@ibhagwan
Copy link
Owner Author

My first guess is cb(nil) in on_write() might need to be smarter. I'll have to play with it a bit myself later tomorrow maybe since I don't currently have my large codebase up to try it out.

Further exloration: this issue does not happen when using /dev/stdout as the sink, the pipe:write fails with error EPIPE which then calls cb(err) (with an actual error) which then calls finish.

What I need to figure out is how to determine a write failure (if possible) with io.stdout or any other indicator that the process recieved SIGINT.

@jdrouhard
Copy link
Contributor

yeah, I was hacking something together, but all io functions return non-nil on success, and nil on failure, plus a second arg with an error message.

Try something like:

local success, errMsg = io.stdout:write(data)
if not success then
    io.stderr:write("pipe:write error: " .. errMsg)
    cb(true)
else
    cb(nil)
end

Or something like that.

@ibhagwan
Copy link
Owner Author

ibhagwan commented Dec 13, 2021

yeah, I was hacking something together, but all io functions return non-nil on success, and nil on failure, plus a second arg with an error message.

Try something like:

local success, errMsg = io.stdout:write(data)
if not success then
    io.stderr:write("pipe:write error: " .. errMsg)
    cb(true)
else
    cb(nil)
end

Or something like that.

I was actually just in the process of committing this :-)

ac2ae70

Edit: forced pushed 704a81f as safer

@ibhagwan
Copy link
Owner Author

FYI, live_grep, live_grep_resume and live_grep_glob were also added the multiprocess implementation including icon and search resume support.

Works pretty well on my machine, very fast and responsive, try it out and let me know if you feel the difference?

@ibhagwan
Copy link
Owner Author

Seeing there's no activity here and no outstanding issues with multi-process I'm closing this.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants