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

Conversation

tarruda
Copy link

@tarruda tarruda commented Jul 15, 2014

This PR contains some enhancements that will make YCM run better on Neovim, which uses a separate process for running python code.

Here's a summary of the changes:

  • Calls to the magic vim module have been reduced in order to lower the communication overhead between the python process and Neovim(It won't impact vim performance)
  • A new class which is automatically loaded by the python process(NvimYcmAsyncHelper) was added. This class handles events from Neovim and callbacks from ycmd requests.
  • Neovim send_event function is used to notify the python process about events. This was done because the python command blocks Neovim until the python process responds.
  • Instead of polling for compilation/completion results, NvimYcmAsyncHelper directly calls Neovim once ycmd responds. For Neovim, the complete() vimscript is used to display completions asynchronously('omnifunc' is not touched)

I've posted this for review, but it's not ready for merging yet. I'm still stabilizing the Neovim branch which supports python plugins, so there may be some minor changes to this code. I'm also going to improve commit messages and descriptions.

tarruda added 6 commits July 14, 2014 19:41
This was required to reduce the vim <-> python serialization/deserialization
overhead in Neovim. Vim should not be affected.
- Instead of calling BuildRequestData,
  EventNotification/SendEventNotificationAsync now receive request data as
  argument.
- Methods of the YouCompleteMe can also optionally receive a request data
Use `push_message` method(Neovim-specific) to notify listeners of
`ycmd_response` events. This will be used to asychronously update Neovim
whenever a completion/compilation request is complete without relying on
client-side polling.
@@ -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.

👍

@Valloric
Copy link
Member

Gave this a light review. Thanks for the PR!

[Note to future self: review this more carefully before merging.]

@tarruda
Copy link
Author

tarruda commented Aug 9, 2014

I've fixed some issues with the python client and now YCM now has no noticeable performance impact when compared to upstream vim, so these changes are unnecessary.

Thanks for reviewing anyway!

@tarruda tarruda closed this Aug 9, 2014
@Hinidu
Copy link
Contributor

Hinidu commented Aug 10, 2014

@tarruda are you sure that this should be closed? Does not this PR provide better performance in neovim than upstream vim?

@tarruda
Copy link
Author

tarruda commented Aug 10, 2014

@tarruda are you sure that this should be closed? Does not this PR provide better performance in neovim than upstream vim?

yes but Neovim is not yet 100% stable for accepting asynchronous API calls since some modes cant handle K_EVENT correctly yet. This might break if the call is received when confirming if a swap file should be loaded, for example.

After neovim/neovim#974 is corrected and merged I might give async completions another try

@Valloric
Copy link
Member

@tarruda Thank you for improving Neovim to the point where extensive changes to YCM are not needed. This will not only make maintaining YCM easier but should improve the performance of other Vim plugins that are written in Python and run inside Neovim. So thanks for that!

I'm totally open to future Neovim-related PRs though.

@Freyskeyd
Copy link

Hey, i'm trying to install YCM with Vundle on neovim but i can't make it working. It ask every time for Vim compiled with Python 2.x.

Any idea?

@tarruda
Copy link
Author

tarruda commented Nov 18, 2014

Hey, i'm trying to install YCM with Vundle on neovim but i can't make it working. It ask every time for Vim compiled with Python 2.x.

Any idea?

:h nvim-python contains instructions on how to emulate +python in Nvim

@Freyskeyd
Copy link

Oh, thank's !

@xanderdunn
Copy link

@tarruda It looks like neovim/neovim#974 was solved by neovim/neovim#1316. Do you think NeoVim is ready for asynchronous completions in YouCompleteMe now? Thanks a ton!

@tarruda
Copy link
Author

tarruda commented Jan 21, 2015

@tarruda It looks like neovim/neovim#974 was solved by neovim/neovim#1316. Do you think NeoVim is ready for asynchronous completions in YouCompleteMe now? Thanks a ton!

It should be possible to write a fully asynchronous YCM client for Neovim as a remote plugin. Here's a remote plugin example.

@Valloric
Copy link
Member

I'd recommend focusing on having great support for vanilla YCM out-of-the box for neovim. YCM is probably the most complex Vim plugin out there so if that works well, other plugins should too.

When neovim hits 1.0 and becomes more widely accepted as a Vim alternative, then we can take a look at a custom YCM client.

In other words, I recommend taking it one step at a time. Small bites are better than bigger ones.

@jfelchner
Copy link

@Valloric I completely understand not wanting to go nuts on changing YCM to support Neovim, but a 1.0 is probably a long way away. Is there something that can be done to start transitioning to async sooner?

@Yamakaky
Copy link

Any update here ?

@vheon
Copy link
Contributor

vheon commented Jan 11, 2016

Any update here ?

No, and personally I wonder what it would bring to the table feature-wise.

@puremourning
Copy link
Member

to quote the reason this was closed by the author:

I've fixed some issues with the python client and now YCM now has no noticeable performance impact when compared to upstream vim, so these changes are unnecessary.

So what is the expectation of change? Are you seeing a problem with neovim?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants