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

Implement cached label resolution and label resolution limit #26

Merged

Conversation

krassowski
Copy link
Contributor

Implement labels cache for Jedi. When snippets are disabled the single most time-consuming thing for Jedi is completion.get_signatures() call which is used by to create a label of the CompletionItem. See profiling in palantir/python-language-server#823 (comment)

It is a known issue and is unlikely to be fixed upstream in Jedi (any time soon): davidhalter/jedi#1059 (comment).

The Jedi author recommended to:

  • only call get_signatures() on ~ 10 completions. I set the default to 25; it can be changed using resolve_at_most_labels setting
  • cache calls to "big annoying libraries"; for now I added a list of with: 'pandas', 'numpy', 'tensorflow', 'matplotlib' but we can expand this or add a setting to allow the user to customize it.

This change, together with #25 was tested on my fork for over a month and found stable and providing a noticeable performance improvement as testified by users.

The improvement will not be seen if the snippets are used; we would need to cache snippets in the same way (basically avoid any call to get_signatures(). Please note that the "obvious" way of computing get_signatures() asynchronously or caching the result is sadly not possible because those hold references to pickle files which get deleted in the meantime and everything would lead to bad crashes (we would need to acquire jedi lock but this proved to be very fragile in my experiments). Obviously, the string result of processing get_signatures() can be cached (as happens with labels here).

@andfoy
Copy link
Contributor

andfoy commented Jul 13, 2021

@krassowski, thanks a lot for your help in improving the completion response time, do you know what is missing in this PR?

@krassowski
Copy link
Contributor Author

I will rebase and add settings to schema.

@krassowski krassowski force-pushed the improve-jedi-completion-performance branch from 6dd31cc to e00fcf9 Compare July 13, 2021 18:03
@krassowski krassowski force-pushed the improve-jedi-completion-performance branch from e00fcf9 to 647bf4e Compare July 13, 2021 18:07
@krassowski
Copy link
Contributor Author

Ok, that should be ready now.

CONFIGURATION.md Outdated Show resolved Hide resolved
Copy link
Contributor

@andfoy andfoy left a comment

Choose a reason for hiding this comment

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

LGTM!

@andfoy andfoy added this to the v1.2.0 milestone Jul 13, 2021
@andfoy andfoy merged commit e3c5dfe into python-lsp:develop Jul 13, 2021
@krassowski krassowski deleted the improve-jedi-completion-performance branch July 13, 2021 19:41
@krassowski
Copy link
Contributor Author

Thanks for the merge! I will be back with more improvements next week.

krassowski added a commit that referenced this pull request Jul 14, 2021
This is a key change that should have been included in #26 but was left out by omission when porting the changes from my fork.
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.

2 participants