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

fix delay for fly grep in neovim #1802

Closed
wants to merge 3 commits into from
Closed

fix delay for fly grep in neovim #1802

wants to merge 3 commits into from

Conversation

ssfdust
Copy link
Contributor

@ssfdust ssfdust commented Jun 7, 2018

PR Prelude

Thank you for working on SpaceVim! :)

Please complete these steps and check these boxes (by putting an x inside
the brackets) before filing your PR:

  • I have read and understood SpaceVim's CONTRIBUTING document.
  • I have read and understood SpaceVim's CODE_OF_CONDUCT document.
  • I have included tests for the changes in my PR. If not, I have included a
    rationale for why I haven't.
  • I understand my PR may be closed if it becomes obvious I didn't
    actually perform all of these steps.

Why this change is necessary and useful

[Please explain in detail why the changes in this PR are needed.]

https://github.com/SpaceVim/SpaceVim/issues/1559
尝试着解决了一下这个问题,这个问题是neovim的问题,当输出行过多而grep的外部程序结束时,就会出现卡顿,过多的定义随机器的性能而定。
所以就尝试应用shell的head和powershell的select方法来限制了grep的输出,我觉得3000行的限制已经足够了,应该没什么人会在3000行的搜索结果中去选择东西,所以设置了大小为3000。我正常情况下是在至多100+行的情况下进行选择。

@ssfdust ssfdust requested a review from wsdjeg as a code owner June 7, 2018 03:20
endif
if has('win32')
set shell = powershell
Copy link
Member

Choose a reason for hiding this comment

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

as flygrep use job API, so no need to change the value of &shell.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

set shell = powershell
let ext_cmd = ['|', 'select', '-first', '3000']
let cmd += ext_cmd
let cmd = join(cmd, ' ')
Copy link
Member

Choose a reason for hiding this comment

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

by default we use list as the default argv for job API, if we use string ,wo need to escape the command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我一开始试了一下用list,结果'|'管道后面的部分没执行,后来就改成了string。如果用list的话,可以给个用管道的示例吗,我想尽可能地和默认的cmd格式一致。

Copy link
Member

Choose a reason for hiding this comment

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

@ssfdust 有两个问题

  1. 如果使用管道,会不会影响结果的输出速度

  2. 如果使用管道的话,是不可以用 list 作为 job 的参数的,据我了解,目前的 job api 还不支持

Copy link
Contributor Author

@ssfdust ssfdust Jun 7, 2018

Choose a reason for hiding this comment

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

  1. 用管道不会减少输出速度,加以head的限制反而会加速输出速度
    Linux测试用指令
rg --hidden --no-heading --vimgrep -e l | head -100000 | nl | tail
rg --hidden --no-heading --vimgrep -e l | head -3000 | nl | tail

Windows测试用指令(windows中没有安装grep类型的文件,所以直接用的打印,因为是单纯测试管道是否影响效率)

powershell -c "gc recover.csv | select -first 300"
powershell -c "gc recover.csv | select -first 30"

上面一句的执行速度慢于下一句的执行速度,可见瓶颈在于输出渲染到屏幕,而不是管道
2. 那我还是用string类型吧

@codecov
Copy link

codecov bot commented Jun 7, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@945fb7c). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master    #1802   +/-   ##
=========================================
  Coverage          ?   26.33%           
=========================================
  Files             ?      251           
  Lines             ?    12512           
  Branches          ?        0           
=========================================
  Hits              ?     3295           
  Misses            ?     9217           
  Partials          ?        0

@ssfdust
Copy link
Contributor Author

ssfdust commented Jun 9, 2018

今天又重新倒腾了一下,发现问题不在于输出,在于neovim对于字符串的速度有点慢。
主要就是这个去重函数LIST.uniq_by_func,当数量很大的时候就会很慢,第一个是因为里面取key的时候,用到了matchstr,然后就是uniq_by_func会遍历数组,而在输入第一个字母的时候,数组数量非常庞大所以效率就很低

function! s:grep_stdout(id, data, event) abort
  let datas =filter(a:data, '!empty(v:val)')
  let datas = s:LIST.uniq_by_func(datas, function('s:file_line'))
  if bufnr('%') == s:flygrep_buffer_id
    if getline(1) ==# ''
      call setline(1, datas)
    else
      call append('$', datas)
    endif
  endif
endfunction

应该还是neovim的bug,猜测是neovim在jobstop的时候,可能没有连同stdout的回调函数一起关掉
neovim可能要快点话要用python包一层,去重部分放到python里面去做,这样输出的结果不需要遍历,而且可以保证jobstop的时候就直接结束job,而不会影响用户体验。
我在想要不要继续折腾下去,如果现在写了脚本,将来neovim修复了bug,就没有这个问题了。而且在外面包一层python可能就真的有效率问题了。如果不需要的话,就关掉这个pr吧。

@wsdjeg
Copy link
Member

wsdjeg commented Jun 26, 2018

I just comment this line, after that everything works well.

let datas = s:LIST.uniq_by_func(datas, function('s:file_line'))

I will improve the flygrep.

@ssfdust ssfdust closed this Jun 28, 2018
@wsdjeg
Copy link
Member

wsdjeg commented Jul 2, 2018

@ssfdust Hi, I just continue this PR in #1898 , and thanks for your PR.

@wsdjeg wsdjeg reopened this Jul 2, 2018
@ghost ghost assigned wsdjeg Jul 2, 2018
@ghost ghost added the WIP Work In Progress label Jul 2, 2018
@wsdjeg
Copy link
Member

wsdjeg commented Jul 2, 2018

This PR will be merged automatically.

@wsdjeg wsdjeg closed this Jul 9, 2018
@ghost ghost removed the WIP Work In Progress label Jul 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants