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

Enhancements for Neovim #1079

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
186 changes: 170 additions & 16 deletions autoload/youcompleteme.vim
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@
" This is basic vim plugin boilerplate
let s:save_cpo = &cpo
set cpo&vim

let s:is_nvim = has( 'neovim' )
if s:is_nvim
let s:channel_id = pyeval( 'vim.channel_id' )
endif

" This needs to be called outside of a function
let s:script_folder_path = escape( expand( '<sfile>:p:h' ), '\' )
Expand Down Expand Up @@ -120,6 +125,11 @@ function! s:SetUpPython()

py from ycm.youcompleteme import YouCompleteMe
py ycm_state = YouCompleteMe( user_options_store.GetAll() )

if s:is_nvim
call send_event( s:channel_id, 'ycm_setup', 0 )
endif

return 1
endfunction

Expand Down Expand Up @@ -164,9 +174,15 @@ function! s:SetUpKeyMappings()
let invoke_key = '<Nul>'
endif

" <c-x><c-o> trigger omni completion, <c-p> deselects the first completion
" candidate that vim selects by default
silent! exe 'inoremap <unique> ' . invoke_key . ' <C-X><C-O><C-P>'
if s:is_nvim
silent! exe 'inoremap ' . invoke_key .
\ ' <C-R>=youcompleteme#BeginCompletion()<cr>'
else
" <c-x><c-o> trigger omni completion, <c-p> deselects the first
" completion candidate that vim selects by default
silent! exe 'inoremap <unique> ' . invoke_key . ' <C-X><C-O><C-P>'
endif

endif

if !empty( g:ycm_key_detailed_diagnostics )
Expand Down Expand Up @@ -285,7 +301,7 @@ function! s:SetUpCpoptions()

" This prevents the display of "Pattern not found" & similar messages during
" completion. This is only available since Vim 7.4.314
if pyeval( 'vimsupport.VimVersionAtLeast("7.4.314")' )
if !s:is_nvim && pyeval( 'vimsupport.VimVersionAtLeast("7.4.314")' )
set shortmess+=c
endif
endfunction
Expand Down Expand Up @@ -328,7 +344,9 @@ endfunction


function! s:OnVimLeave()
py ycm_state.OnVimLeave()
if !s:is_nvim
py ycm_state.OnVimLeave()
endif
endfunction


Expand All @@ -344,7 +362,14 @@ function! s:OnBufferVisit()

call s:SetUpCompleteopt()
call s:SetCompleteFunc()
py ycm_state.OnBufferVisit()

if s:is_nvim
Copy link
Member

Choose a reason for hiding this comment

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

You should abstract this behind a function like OnBufferVisitInternal or whatever. Then that would check for is_nvim and call send_event or ycm_state.OnBufferVisit.

Do something similar in other places where possible. I'd prefer to have the logic that picks send_event over python calls encapsulated.

Copy link
Author

Choose a reason for hiding this comment

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

👍

call send_event( s:channel_id,
\ 'buffer_visit',
\ youcompleteme#BuildRequestData( 0 ) )
else
py ycm_state.OnBufferVisit()
endif
call s:OnFileReadyToParse()
endfunction

Expand All @@ -354,7 +379,11 @@ function! s:OnBufferUnload( deleted_buffer_file )
return
endif

py ycm_state.OnBufferUnload( vim.eval( 'a:deleted_buffer_file' ) )
if s:is_nvim
call send_event( s:channel_id, 'buffer_unload', a:deleted_buffer_file )
else
py ycm_state.OnBufferUnload( vim.eval( 'a:deleted_buffer_file' ) )
endif
endfunction


Expand All @@ -373,15 +402,21 @@ function! s:OnFileReadyToParse()
" happen for special buffers.
call s:SetUpYcmChangedTick()

" Order is important here; we need to extract any done diagnostics before
" reparsing the file again. If we sent the new parse request first, then
" the response would always be pending when we called
" UpdateDiagnosticNotifications.
call s:UpdateDiagnosticNotifications()
if !s:is_nvim
" Order is important here; we need to extract any done diagnostics before
" reparsing the file again. If we sent the new parse request first, then
" the response would always be pending when we called
" UpdateDiagnosticNotifications.
call s:UpdateDiagnosticNotifications()
endif

let buffer_changed = b:changedtick != b:ycm_changedtick.file_ready_to_parse
if buffer_changed
py ycm_state.OnFileReadyToParse()
if s:is_nvim
call s:BeginCompilation()
Copy link
Member

Choose a reason for hiding this comment

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

At first glance, this strikes me as wrong. ycmd expects to see a OnFileReadyToParse event and you're not sending it when the user is using neovim.

I don't want to have two models of ycmd calling behavior: one when the user is using neovim and one for plain vim. That would be difficult to maintain and reason about.

Given a sequence of equivalent keyboard presses in vim and neovim, both vim and neovim should call ycmd in the same way. The sequence of HTTP calls should be the same. There really isn't any good reason for it to not be the same since neovim support is just a different client.

Copy link
Author

Choose a reason for hiding this comment

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

It does send the event, the only difference is that it will pack the request data before calling python. OnFileReadyToParse is called by the nvim_ycm_async_helper module

I agree that it's strange, so I will remove this as the performance impact is likely to be small

else
py ycm_state.OnFileReadyToParse()
endif
endif
let b:ycm_changedtick.file_ready_to_parse = b:changedtick
endfunction
Expand Down Expand Up @@ -410,7 +445,10 @@ function! s:OnCursorMovedInsertMode()
return
endif

py ycm_state.OnCursorMoved()
if !s:is_nvim
py ycm_state.OnCursorMoved()
Copy link
Member

Choose a reason for hiding this comment

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

Similar here; why isn't this event being sent to ycmd?

Copy link
Author

Choose a reason for hiding this comment

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

👍

I didn't see an event being sent to ycmd in this case, just an update of the line/column number in the python class. I will fix to keep the current behavior

endif

call s:UpdateCursorMoved()

" Basically, we need to only trigger the completion menu when the user has
Expand All @@ -430,7 +468,11 @@ function! s:OnCursorMovedInsertMode()
endif

if g:ycm_auto_trigger || s:omnifunc_mode
call s:InvokeCompletion()
if s:is_nvim
call youcompleteme#BeginCompletion()
else
call s:InvokeCompletion()
endif
endif

" We have to make sure we correctly leave omnifunc mode even when the user
Expand Down Expand Up @@ -459,7 +501,11 @@ function! s:OnInsertLeave()

let s:omnifunc_mode = 0
call s:OnFileReadyToParse()
py ycm_state.OnInsertLeave()

if !s:is_nvim
py ycm_state.OnCursorMoved()
endif

if g:ycm_autoclose_preview_window_after_completion ||
\ g:ycm_autoclose_preview_window_after_insertion
call s:ClosePreviewWindowIfNeeded()
Expand Down Expand Up @@ -812,6 +858,114 @@ endfunction
command! YcmDiags call s:ShowDiagnostics()


function! s:BeginCompilation()
Copy link
Member

Choose a reason for hiding this comment

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

You can't have both s:BeginCompilation and youcompleteme#BeginCompilation; that's just a bad idea. Rename one.

Copy link
Author

Choose a reason for hiding this comment

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

👍

if b:changedtick == get(b:, 'last_changedtick', -1)
return
endif

let b:last_changedtick = b:changedtick
call send_event( s:channel_id,
\ 'begin_compilation',
\ youcompleteme#BuildRequestData( 1 ) )
endfunction


function youcompleteme#GetCurrentBufferFilepath()
Copy link
Member

Choose a reason for hiding this comment

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

I'm strongly against adding more vimscript code to YCM. I've been moving logic out of vimscript and into Python because it's both easier to test and maintain that; pulling logic back into vimscript is a major regression.

Is there really no way to easily run Python code inside the neovim process? If true, IMO you're setting back Vim plugin development then. If the only way to talk to Python in neovim is with costly communication overhead, that's a major issue. I've talked to several developers of popular Vim plugins and everyone hates writing (and especially maintaining) VimScript.

Copy link
Author

Choose a reason for hiding this comment

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

Most of the Neovim-specific performance issues are fixed by the first commit of this PR, the same which moves request building code to vimscript. This didn't match with my initial assumptions that the cause was the amount of data being transfered for each event, so I did some profiling to get to the real problem.

It seems the slowdown is caused by the synchronization code I added to support multithreading in the python client, not by IPC overhead. Here's the profiler output for this PR

         209630 function calls (205655 primitive calls) in 1.343 seconds

   Ordered by: internal time
   List reduced from 1388 to 30 due to restriction <30>

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
      268    0.831    0.003    0.834    0.003 {method 'run' of 'pyuv.Loop' objects}
       26    0.057    0.002    0.092    0.004 {method 'sub' of '_sre.SRE_Pattern' objects}
      509    0.049    0.000    0.049    0.000 {method 'acquire' of 'thread.lock' objects}
    46272    0.028    0.000    0.034    0.000 encoder.py:37(replace)
      132    0.022    0.000    0.022    0.000 {method 'write' of 'pyuv.Stream' objects}
      228    0.018    0.000    0.051    0.000 posixpath.py:372(_joinrealpath)
     1856    0.016    0.000    0.016    0.000 {posix.lstat}
     2121    0.013    0.000    0.013    0.000 {method 'search' of '_sre.SRE_Pattern' objects}
        2    0.013    0.006    0.013    0.006 {method 'connect' of '_socket.socket' objects}
     2567    0.009    0.000    0.009    0.000 {method 'startswith' of 'unicode' objects}

and here for the master branch:

         10214073 function calls (10038486 primitive calls) in 16.478 seconds

   Ordered by: internal time
   List reduced from 1400 to 30 due to restriction <30>

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
   114443    2.587    0.000    3.680    0.000 {method 'run' of 'pyuv.Loop' objects}
   114404    1.243    0.000    5.840    0.000 client.py:124(unpack_message)
   917638    0.845    0.000    0.845    0.000 __init__.py:1329(getEffectiveLevel)
    57458    0.792    0.000   10.812    0.000 client.py:25(wait)
   917638    0.772    0.000    1.618    0.000 __init__.py:1343(isEnabledFor)
171862/41    0.733    0.000   16.247    0.396 client.py:207(invoke_message_cb)
   229320    0.658    0.000    2.204    0.000 client.py:90(release)
   917637    0.609    0.000    2.226    0.000 __init__.py:1128(debug)
    57458    0.578    0.000    4.215    0.000 client.py:174(msgpack_rpc_request)
   229320    0.544    0.000    0.963    0.000 client.py:83(acquire)
   171349    0.503    0.000    4.541    0.000 uv_stream.py:123(_run)
   229335    0.405    0.000    0.760    0.000 threading.py:372(notify)
    57498    0.397    0.000    0.397    0.000 {method 'write' of 'pyuv.Stream' objects}
    57458    0.385    0.000    4.787    0.000 client.py:324(async_func)
   114404    0.384    0.000    6.270    0.000 client.py:162(queue_message)
    57497    0.320    0.000    0.613    0.000 uv_stream.py:76(_on_read)
   458641    0.307    0.000    0.403    0.000 threading.py:287(__exit__)
   458641    0.285    0.000    0.498    0.000 threading.py:284(__enter__)
    57498    0.279    0.000    2.147    0.000 uv_stream.py:177(write)
   229320    0.247    0.000    1.063    0.000 threading.py:399(notifyAll)
   114404    0.229    0.000    3.487    0.000 uv_stream.py:157(read)
   458640    0.213    0.000    0.213    0.000 {method '__enter__' of 'thread.lock' objects}
   172273    0.207    0.000    0.207    0.000 {hasattr}
   229339    0.174    0.000    0.280    0.000 threading.py:299(_is_owned)
   229320    0.172    0.000    1.136    0.000 client.py:95(__enter__)
    57497    0.164    0.000    0.164    0.000 client.py:41(__init__)
   229320    0.152    0.000    2.356    0.000 client.py:98(__exit__)
    57458    0.147    0.000    0.147    0.000 client.py:12(__init__)
    57498    0.146    0.000    0.266    0.000 __init__.py:41(packb)
    57458    0.128    0.000   15.727    0.000 client.py:339(func)

All I did was open nvim with a 20k loc C file

I bet YCM will run much better once I reimplement the client it using greenlets + twisted, which I was already planning to do.

In case you are wondering, I've posted the profiler output here because it may be interesting to you, considering that ycmd is implemented as a multithreaded python web server(the performance may improve if requests are served with green threads)

let filepath = expand( '%:p' )

if empty( filepath )
" Buffers that have just been created by a command like :enew don't have
" any buffer name so we use the buffer number for that.
let filepath = getcwd() . '/' . bufnr( '%' )
endif

return filepath
endfunction


function youcompleteme#BuildRequestData( include_buffer_data )
let pos = getpos( '.' )
let filepath = youcompleteme#GetCurrentBufferFilepath()

let data = {
\ 'line_num': pos[ 1 ],
\ 'column_num': pos[ 2 ],
\ 'filepath': filepath
\ }

if a:include_buffer_data
let data.file_data = s:GetUnsavedAndCurrentBufferData( filepath )
endif

return data
endfunction


function s:GetUnsavedAndCurrentBufferData( filepath )
let file_data = {}
let file_data[ a:filepath ] = {
\ 'filetypes': split( &filetype, ',' ),
\ 'contents': join( getline( 1, '$' ), "\n" ),
\ }
return file_data
endfunction


let s:next_completion_id = 1
let s:current_completion_id = 0


function! youcompleteme#BeginCompletion()
let s:current_completion_id = s:next_completion_id
let s:next_completion_id += 1

let data = youcompleteme#BuildRequestData( 1 )
let data.id = s:current_completion_id
let data.position = s:GetCompletionPosition()

call send_event( s:channel_id, 'begin_completion', data )
return ''
endfunction


function! youcompleteme#EndCompletion( data, result )
if mode() != 'i'
" Not in insert mode, ignore
return
endif

let completion_id = a:data.id
if s:current_completion_id != completion_id
" Completion expired
return
endif

let completion_pos = a:data.position
let current_pos = s:GetCompletionPosition()
if current_pos[ 0 ] != completion_pos[ 0 ]
\ || current_pos[ 1 ] != completion_pos[ 1 ]
" Completion position changed
return
endif

if empty( a:result )
return
endif

call complete( completion_pos[ 1 ], a:result )
" Deselect the first match
call feedkeys( "\<C-P>", 'n' )
endfunction


function! s:GetCompletionPosition()
" The completion position is the start of the current identifier
let pos = searchpos( '\i\@!', 'bn', line( '.' ) )
let pos[ 1 ] += 1
return pos
endfunction


" This is basic vim plugin boilerplate
let &cpo = s:save_cpo
unlet s:save_cpo
43 changes: 24 additions & 19 deletions python/ycm/client/base_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,15 @@ def PostDataToHandler( data, handler, timeout = _DEFAULT_TIMEOUT_SEC ):
# |timeout| is num seconds to tolerate no response from server before giving
# up; see Requests docs for details (we just pass the param along).
@staticmethod
def PostDataToHandlerAsync( data, handler, timeout = _DEFAULT_TIMEOUT_SEC ):
return BaseRequest._TalkToHandlerAsync( data, handler, 'POST', timeout )
def PostDataToHandlerAsync( data,
handler,
timeout = _DEFAULT_TIMEOUT_SEC,
request = None ):
return BaseRequest._TalkToHandlerAsync( data,
handler,
'POST',
timeout,
request )


# This returns a future! Use JsonFromFuture to get the value.
Expand All @@ -87,31 +94,40 @@ def PostDataToHandlerAsync( data, handler, timeout = _DEFAULT_TIMEOUT_SEC ):
def _TalkToHandlerAsync( data,
handler,
method,
timeout = _DEFAULT_TIMEOUT_SEC ):
timeout = _DEFAULT_TIMEOUT_SEC,
request = None ):
def Callback( session, resp ):
if request:
vimsupport.PushMessage( 'ycmd_response_ready', request )

def SendRequest( data, handler, method, timeout ):
if method == 'POST':
sent_data = ToUtf8Json( data )
return BaseRequest.session.post(
_BuildUri( handler ),
data = sent_data,
headers = BaseRequest._ExtraHeaders( sent_data ),
timeout = timeout )
timeout = timeout,
background_callback = Callback )
if method == 'GET':
return BaseRequest.session.get(
_BuildUri( handler ),
headers = BaseRequest._ExtraHeaders(),
timeout = timeout )
timeout = timeout,
background_callback = Callback )

@retries( 5, delay = 0.5, backoff = 1.5 )
def DelayedSendRequest( data, handler, method ):
if method == 'POST':
sent_data = ToUtf8Json( data )
return requests.post( _BuildUri( handler ),
resp = requests.post( _BuildUri( handler ),
data = sent_data,
headers = BaseRequest._ExtraHeaders( sent_data ) )
if method == 'GET':
return requests.get( _BuildUri( handler ),
resp = requests.get( _BuildUri( handler ),
headers = BaseRequest._ExtraHeaders() )
Callback( None, resp )
return resp

if not _CheckServerIsHealthyWithCache():
return _EXECUTOR.submit( DelayedSendRequest, data, handler, method )
Expand All @@ -134,18 +150,7 @@ def _ExtraHeaders( request_body = None ):


def BuildRequestData( include_buffer_data = True ):
line, column = vimsupport.CurrentLineAndColumn()
filepath = vimsupport.GetCurrentBufferFilepath()
request_data = {
'line_num': line + 1,
'column_num': column + 1,
'filepath': filepath
}

if include_buffer_data:
request_data[ 'file_data' ] = vimsupport.GetUnsavedAndCurrentBufferData()

return request_data
return vimsupport.BuildRequestData( include_buffer_data )


def JsonFromFuture( future ):
Expand Down
3 changes: 2 additions & 1 deletion python/ycm/client/completion_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ def __init__( self, request_data ):
def Start( self ):
self._response_future = self.PostDataToHandlerAsync( self.request_data,
'completions',
TIMEOUT_SECONDS )
TIMEOUT_SECONDS,
request = self )


def Done( self ):
Expand Down
Loading