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

YCM blocks vim UI thread during ycmd startup #2071

Closed
r4nt opened this issue Mar 23, 2016 · 14 comments · Fixed by #2514
Closed

YCM blocks vim UI thread during ycmd startup #2071

r4nt opened this issue Mar 23, 2016 · 14 comments · Fixed by #2514

Comments

@r4nt
Copy link

r4nt commented Mar 23, 2016

To repro, create ycm-extra-conf with:
import time
def YcmCorePreload():
time.sleep(10)

$ vim
<now anything you type will be delayed by multiple seconds, until the first connection times out>

@d0k

@Valloric
Copy link
Member

Huh, I'm surprised that's the case. We call python's subprocess.Popen function which I think doesn't wait for process startup to complete (I could be wrong).

@micbou @puremourning @vheon Thoughts? If we indeed do block on ycmd startup, that sounds like an area ripe for a perf improvement.

@micbou
Copy link
Collaborator

micbou commented Mar 26, 2016

The issue is that when loading the global .ycm_extra_conf.py file/calling the YcmCorePreload function, the ycmd server is still not up while YCM is sending requests. Eventually, it fails with a maximum number of retries reached. This blocks the UI and results in a traceback in Vim. I'll take a look on how we can prevent this (at least the traceback).

@puremourning
Copy link
Member

Any improvement to startup time in general wins cookies.

@Valloric
Copy link
Member

The issue is that when loading the global .ycm_extra_conf.py file/calling the YcmCorePreload function, the ycmd server is still not up while YCM is sending requests. Eventually, it fails with a maximum number of retries reached.

That sounds unfortunate but livable.

This blocks the UI and results in a traceback in Vim.

That's the part that shouldn't happen, especially the UI blocking. Ugh. :(

@puremourning
Copy link
Member

That's the part that shouldn't happen, especially the UI blocking. Ugh. :(

We definitely block the UI on buffer read. I just found this while I had ycmd paused in the debugger. Vim hangs until ycmd responds after doing something like :split file.cpp. I think it might be querying for the supported filetypes or something. Perhaps we should convert that to async and just defer all YCM in the buffer until it is complete. I didn't look into it in detail yet.

@puremourning
Copy link
Member

Also, further notes:

  • the YcmCorePreLoad requires there to be a global extra_conf file (for anyone else trying to repro
  • the thing that hangs is the 'completer_available' request - YCM is trying to determine if there is native completion support - i.e. whether or not to use the omni-completer
  • it only blocks the first time you enter insert mode, due to deferring setting the omnifunc thing that @vheon worked on

Detail is something like

  • s:SetOmnicompleteFunc is called in InsertEnter
  • this calls YouCompleteMe.NativeFiletypeCompletionUsable
  • which calls FiletypeCompleterExistsForFiletype
  • which, the first time for a filetype, calls SendCompleterAvailableRequest
  • which blocks until it gets a response.

@Valloric Valloric mentioned this issue Apr 2, 2016
11 tasks
@puremourning
Copy link
Member

I have a solution for this YcmCorePreload blocking Vim. I'll send a PR sometime soon.

For background, the solution is to make the CompleterAvailableRequest use PostDataToHandlerAsync and jiggle the FiletypeCompleterExistsForFiletype method to understand that. It can basically return False until we have a proper answer.

@dc-mak
Copy link

dc-mak commented Jul 18, 2016

@puremourning. If the PR still isn't ready, would you mind providing a bit more detail so I (and others who face the issue) can get around this? It's been a serious thorn in my use of YCM

@puremourning
Copy link
Member

It looks like I wrote a fair amount of info on this thread. Suffice to say that it is fiddly internals.

Anyway, the simplest thing would be to avoid doing slow stuff in the core preload call. I know that's not super hearty, but short of rejiggling the vim client internals around this, there aren't realm any workarounds.

@dc-mak
Copy link

dc-mak commented Jul 18, 2016

Fair enough, poked around a bit more and I can see what you mean now, agreed (and thanks).

@satyenr
Copy link

satyenr commented Jan 13, 2017

@puremourning Any updates on the PR? I am still facing massive UI freezes while starting neovim.

@puremourning
Copy link
Member

I guess you can infer from the open nature of the issue that the answer is the status is very much quo

@dc-mak
Copy link

dc-mak commented Jan 18, 2017

For anyone else plagued by the issue, last I checked, this fork simply deletes the offending code (clearly this changes the behaviour but I was happy with it)
https://github.com/oblitum/YouCompleteMe

homu added a commit that referenced this issue Jan 26, 2017
[READY] Rely on connect timeout instead of checking that the server is alive

Currently, we always check that the ycmd process is up (with the `IsServerAlive` method) before sending a request. Without this check, each request could block Vim until a `NewConnectionError` exception is raised if the server crashed. This is the case on Windows where it takes ~1s before the exception is raised which makes Vim unusable. However, even with this check, Vim may still be blocked in the following cases:
 - the server crashes just after the check but before sending the request;
 - the server is up but unresponsive (e.g. its port is closed).

To avoid both cases, we instead use [the connect timeout parameter from Requests](http://docs.python-requests.org/en/master/user/advanced/?highlight=connect%20timeout#timeouts) and set it to a duration sufficiently short (10 ms) that the blocking can't be noticed by the user. Since the server is supposed to run locally (this is what YCM is designed for), 10ms is largely enough to establish a connection.

The `IsServerAlive` check is removed almost everywhere except in `OnFileReadyToParse` because we still want to notify the user if the server crashed.

This change makes it possible to not have to [wait for the server to be healthy before sending asynchronous requests](https://github.com/Valloric/YouCompleteMe/blob/master/python/ycm/client/base_request.py#L137-L138). This will dramatically improve startup time (see issue #2085) and fixes #2071. Next PR once this one is merged.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2514)
<!-- Reviewable:end -->
@micbou
Copy link
Collaborator

micbou commented Jan 26, 2017

Still not fixed but soon.

@micbou micbou reopened this Jan 26, 2017
homu added a commit that referenced this issue Jan 28, 2017
[READY] Remove healthy check

This PR removes the code that, if the server is not yet healthy, tries in a separate thread to send the same request at different intervals until it gets a response or reaches 5 failed attempts.

Main issue with that code is that it blocks Vim if a synchronous request (`semantic_completion_available`, `run_completer_command`, etc.)  is sent while the server is not up. See issue #2071.

Other issues are that it's hard to understand how this code behaves (it combines threads and retries), it becomes useless once the server is healthy (because `SERVER_HEALTHY` is cached and never reset), it depends on arbitrary parameters (why 5 retries with a delay of 0.5s multiplied by 1.5 at each attempt?), and it has a package dependency `retries`.

Now that we catch all server exceptions (PR #2453) and define a very short connection timeout (PR #2514), we can just drop this code.

This change gives a huge improvement in startup time on Windows (don't worry, more improvements are coming for all platforms). The reason is that `requests` (probably `urllib3`) tries for ~1s to check if the server is healthy on Windows while it's almost instantaneous on other platforms.
<table>
  <tr>
    <th rowspan="2">Platform</th>
    <th colspan="2">First run (ms)</th>
    <th colspan="2">Subsequent runs (ms)</th>
  </tr>
  <tr>
    <td>Before</td>
    <td>After</td>
    <td>Before</td>
    <td>After</td>
  </tr>
  <tr>
    <td>Ubuntu 16.04 64-bit</td>
    <td>228</td>
    <td>248</td>
    <td>176</td>
    <td>151</td>
  </tr>
  <tr>
    <td>macOS 10.12</td>
    <td>433</td>
    <td>417</td>
    <td>263</td>
    <td>263</td>
  </tr>
  <tr>
    <td>Windows 10 64-bit</td>
    <td>1861</td>
    <td>844</td>
    <td>814</td>
    <td>292</td>
  </tr>
</table>

These results were obtained by running the `prof.py` script from [this branch](https://github.com/micbou/YouCompleteMe/tree/profiling-startup). The difference between first run and subsequent runs is Python bytecode generation (`*.pyc` files).

Fixes #2071.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2517)
<!-- Reviewable:end -->
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants