search

echasnovski / mini.nvim

Library of 40+ independent Lua modules improving overall Neovim (version 0.8 and higher) experience with minimal effort
MIT License
4.45k stars 171 forks source link

Slow replace with clipboard unnamedplus #975

Closed Susensio closed 2 weeks ago

Susensio commented 2 weeks ago

Contributing guidelines

Module(s)

mini.operators

Description

When vim.opt.clipboard=unnamedplus, replace takes about 1 second to execute. I know this must be some problem with my system clipboard, but I cannot figure out what it is. substitute.nvim does not lag at all.

Neovim version

0.10

Steps to reproduce

nvim -nu minimal.lua

local path_package = '/tmp/nvim/'
local mini_path = path_package .. 'pack/deps/start/mini.nvim'
if not vim.loop.fs_stat(mini_path) then
  local clone_cmd = {
    'git', 'clone', '--filter=blob:none',
    'https://github.com/echasnovski/mini.nvim', mini_path
  }
  vim.fn.system(clone_cmd)
end
vim.opt.rtp:prepend(mini_path)

require('mini.deps').setup({ path = { package = path_package } })

vim.opt.clipboard='unnamedplus'
require('mini.operators').setup()
echasnovski commented 2 weeks ago

Thanks for the issue!

I can not reproduce. Would you mind listing exact steps that makes you reproduce the issue on your end?

When vim.opt.clipboard=unnamedplus, replace takes about 1 second to execute.

With this information (specifically 1 second), my guess is that there is some conflicting mapping starting with gr. This makes Neovim wait after gr a 'timeoutlen' milliseconds (1000 by default, i.e. 1 second) until executing the currently typed keys.

What does :map gr show? It should show only mappings from 'mini.operators' (and possibly 'mini.clue' if you use it with g as trigger). Note that it is not :nmap gr, but :map gr, which will show available mappings in several modes.

Susensio commented 2 weeks ago

I'm using the minimal.lua provided, so no additional mappings should be present. Anyway:

x  gr          * <Cmd>lua MiniOperators.replace('visual')<CR>
                 Replace selection
n  grr           gr_
                 Replace line
n  gr          * v:lua.MiniOperators.replace()
                 Replace operator

I've tested that changing vim.opt.clipboard=unnamedplus to vim.opt.clipboard='' fixes the delay. So it must be some problem with my system clipboard, but I cannot figure it out...

The steps for reproducing the issue:

  1. start neovim with nvim -nu minimal.lua
  2. yank some line with yy
  3. replace some other line with grr

Recording: https://asciinema.org/a/uOfLU2adUpLOTrG284DwjrN9e

Susensio commented 2 weeks ago

I've profiled the mini.operators code and most of the time is spent in https://github.com/echasnovski/mini.nvim/blob/ebc8cb0c1f73ee6c53e0560957a37a2307a70f98/lua/mini/operators.lua#L1027-L1031

echasnovski commented 2 weeks ago

I still can not reproduce this, but it indeed looks like an issue with caching and restoring " register (which maybe automatically touches system clipboard if 'clipboard' is not empty).

Would you mind confirming a couple for things for me, please?

Susensio commented 2 weeks ago
  • Does explicitly yanking and replacing from register produce the delay? So steps are now "ayy and "agrr.

It does not produce any delay. If using system clipboard with "+grr, there is a delay. (that delay does not exists with simple paste with "+p

  • Does the following patch fix the issue?

Nop, same delay

echasnovski commented 2 weeks ago

If using system clipboard with "+grr, there is a delay.

And I presume there is no delay if using regular register (like a)?

Some other questions that might help:

Unfortunately, that diff was the best attempt at fixing this. I'd need more profiling information. My suggestion would be to create a _G.log variable and iteratively add table.insert(_G.log, { place = '<descriptive place name>', time = vim.loop.hrtime() }) before and after suspected lines that might cause delay. Identifying the exact line (which is not a call to a function from this module) would massively help. Because there are a lot going on inside those 4 lines.

Susensio commented 2 weeks ago

If using system clipboard with "+grr, there is a delay.

And I presume there is no delay if using regular register (like a)?

That is correct. My previous answer was misquoted.

  • What clipboard/OS you're using here?

I'm on Debian Gnome, using wayland (nvim is using wl-copy and wl-paste)

  • Does delay happen on other text manipulation operators? Like exchange (gxx) and multiple (gmm)?

gmm and gxx work fine, with no visible delay

Unfortunately, that diff was the best attempt at fixing this. I'd need more profiling information.

I've managed to pinpoint the issue. vim.fn.getreginfo It takes ~0.5ms with regular registers and ~150ms with system registers. I think it gets called more than once..

vim.fn.getregtype has the same problem.

I think this is related to https://github.com/neovim/neovim/issues/12622 and https://github.com/neovim/neovim/issues/23748

This coment https://github.com/neovim/neovim/issues/23748#issuecomment-1980626397 explains that wl-copy has some hacks with undesirable effects.

The thing is that substitute.nvim has no noticeable delay. I think the reason is that vim.fn.getregtype and vim.fn.getreginfo are being called only once, and mini.operators calls then multiple times.

echasnovski commented 2 weeks ago

I've managed to pinpoint the issue. vim.fn.getreginfo It takes ~0.5ms with regular registers and ~150ms with system registers. I think it gets called more than once..

vim.fn.getregtype has the same problem.

I think this is related to neovim/neovim#12622 and neovim/neovim#23748

This coment neovim/neovim#23748 (comment) explains that wl-copy has some hacks with undesirable effects.

The thing is that substitute.nvim has no noticeable delay. I think the reason is that vim.fn.getregtype and vim.fn.getreginfo are being called only once, and mini.operators calls then multiple times.

I see. Thanks for the investigation!

If anything, this indeed sounds like an issue with the clipboard provider. I don't think it is a good idea to optimize the code specifically under the assumption that getreginfo() and getregtype() may take a long time.

I'll take a look, but no promises.

echasnovski commented 2 weeks ago

@Susensio, would you mind applying the following patch and see if the delay persists?

diff --git a/lua/mini/operators.lua b/lua/mini/operators.lua
index cddea215..249fd5d4 100644
--- a/lua/mini/operators.lua
+++ b/lua/mini/operators.lua
@@ -997,8 +997,8 @@ H.replace_do = function(data)
   local mark_from, mark_to = data.mark_from, data.mark_to

   -- Do nothing with empty/unknown register
-  local register_type = H.get_reg_type(register)
-  if register_type == '' then H.error('Register ' .. vim.inspect(register) .. ' is empty or unknown.') end
+  local reg_info = vim.fn.getreginfo(register)
+  if reg_info.regtype == nil then H.error('Register ' .. vim.inspect(register) .. ' is empty or unknown.') end

   -- Determine if region is at edge which is needed for the correct paste key
   local from_line, from_col = unpack(H.get_mark(mark_from))
@@ -1026,16 +1026,18 @@ H.replace_do = function(data)
     { mark_from = mark_from, mark_to = mark_to, submode = forced_motion, mode = data.mode, register = '_' }
   H.do_between_marks('d', delete_data)

-  -- Paste register (ensuring same submode type as region)
-  H.with_temp_context({ registers = { register } }, function()
-    H.set_reg_type(register, submode)
+  -- Modify register data to have proper submode and indent
+  local new_reg_info = vim.deepcopy(reg_info)
+  if new_reg_info.regtype:sub(1, 1) ~= submode then new_reg_info.regtype = submode end
+  if should_reindent then new_reg_info.regcontents = H.update_indent(new_reg_info.regcontents, init_indent) end
+  vim.fn.setreg(register, new_reg_info)

-    -- Possibly reindent
-    if should_reindent then H.set_reg_indent(register, init_indent) end
+  -- Paste
+  local paste_keys = string.format('%d"%s%s', data.count or 1, register, (is_edge and 'p' or 'P'))
+  H.cmd_normal(paste_keys)

-    local paste_keys = string.format('%d"%s%s', data.count or 1, register, (is_edge and 'p' or 'P'))
-    H.cmd_normal(paste_keys)
-  end)
+  -- Restore register data
+  vim.fn.setreg(register, reg_info)

   -- Adjust cursor to be at start mark
   vim.api.nvim_win_set_cursor(0, { from_line, from_col })
@@ -1152,26 +1154,6 @@ end

 H.is_content = function(x) return type(x) == 'table' and H.islist(x.lines) and type(x.submode) == 'string' end

--- Registers ------------------------------------------------------------------
-H.get_reg_type = function(regname) return vim.fn.getregtype(regname):sub(1, 1) end
-
-H.set_reg_type = function(regname, new_regtype)
-  local reg_info = vim.fn.getreginfo(regname)
-  local cur_regtype, n_lines = reg_info.regtype:sub(1, 1), #reg_info.regcontents
-
-  -- Do nothing if already the same type
-  if cur_regtype == new_regtype then return end
-
-  reg_info.regtype = new_regtype
-  vim.fn.setreg(regname, reg_info)
-end
-
-H.set_reg_indent = function(regname, new_indent)
-  local reg_info = vim.fn.getreginfo(regname)
-  reg_info.regcontents = H.update_indent(reg_info.regcontents, new_indent)
-  vim.fn.setreg(regname, reg_info)
-end
-
 -- Marks ----------------------------------------------------------------------
 H.get_region_data = function(mode)
   local submode = H.get_submode(mode)

As far as I can tell, now there should be a single call to either getregtype() and getreginfo() when doing replace.

Susensio commented 2 weeks ago

If anything, this indeed sounds like an issue with the clipboard provider. I don't think it is a good idea to optimize the code specifically under the assumption that getreginfo() and getregtype() may take a long time.

I was thinking the same and I totally agree with you. A hacky workaround in wl-clipboard should not make mini.nvim code messier.

@Susensio, would you mind applying the following patch and see if the delay persists?

As far as I can tell, now there should be a single call to either getregtype() and getreginfo() when doing replace.

It works! The delay is barely noticeable! Thank you so much for your time on this.

I don't know if this affects much more people. The lack of bug reports on this topic is strange. From what I've been reading, this hack depends on the wayland protocol implemented by each compositor. Mutter (gnome wayland compositor) forces wl-clipboard to use that focus-stealing hack, and thus the delay. Maybe the intersection of people using wayland, gnome and mini.operators is small.

Topics I found about this, for cross reference: https://github.com/bugaevc/wl-clipboard/issues/12#issuecomment-451977175 https://github.com/bugaevc/wl-clipboard/issues/163 https://gitlab.gnome.org/GNOME/mutter/-/issues/524 https://github.com/bugaevc/wl-clipboard/issues/227

It may be a niche problem but, from what I've read, it is not going to be fixed upstream anytime soon...

If you think this should not be part of mini.operators, I would keep this as a local branch in my fork. Anyhow, this is super good enough for me, thanks!

echasnovski commented 2 weeks ago

Thanks for the follow up!

That diff should now be part of the main branch. It reduces relatively big piece of code while making the rest not that bigger. So it's a win-win kind of situation :)