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

Accessing completion attributes is very slow (tensorflow) #1116

Closed
ppwwyyxx opened this issue May 2, 2018 · 17 comments
Closed

Accessing completion attributes is very slow (tensorflow) #1116

ppwwyyxx opened this issue May 2, 2018 · 17 comments
Labels
database-index Needs a database index/Rewrite in Rust (#1059) performance

Comments

@ppwwyyxx
Copy link

ppwwyyxx commented May 2, 2018

import os
import time
from jedi import Script
source = r"""
#!/usr/bin/env python
import tensorflow as tf
tf.t""".strip()
lines = source.strip().split('\n')
print(len(lines), len(lines[-1]))

start = time.time()
ret = Script(source, len(lines), len(lines[-1]), None).completions()
[x.type for x in ret]   # this line is slow!!
print(time.time() - start, len(ret))

This code takes 6.6s on my laptop.
Removing the line mentioned above, it then goes down to 0.5s.

Any idea on why this happens and how the completion can be made faster?

@davidhalter
Copy link
Owner

tensorflow. Here we meet again. It's probably going to be a hard one to fix, because this library is just huge. I have ideas, but it's going to take a while.

@zjjott
Copy link

zjjott commented May 31, 2018

+1
microsoft/vscode-python#1728

@zjjott
Copy link

zjjott commented Jun 8, 2018

@davidhalter does this issue can be resolved?

@davidhalter
Copy link
Owner

davidhalter commented Oct 1, 2018

Please recheck! It should be way faster for tensorflow now. See my comments in #1195.

@ppwwyyxx
Copy link
Author

ppwwyyxx commented Oct 2, 2018

Tested with jedi 0.12.1 and 0.13.0, the original code I posted above takes the same amount of time on two versions (2.35s on my laptop). (It is faster then 5 months ago when I posted but I don't know whether that's due to upgrades of tensorflow or some other factors)

@davidhalter
Copy link
Owner

Ok.

@davidhalter davidhalter reopened this Oct 2, 2018
@davidhalter
Copy link
Owner

davidhalter commented Oct 2, 2018

I didn't really test this and somehow fucked it up. Thanks for testing. I'll probably create a 0.13.1 this evening. cc @hanspinckaers

@davidhalter
Copy link
Owner

Should be working now. If you want you can re-test or wait for the release this evening.

@micbou
Copy link
Contributor

micbou commented Oct 2, 2018

While completing import tensorflow; tensorflow. is now really fast, almost all suggestions are returned as modules instead of functions and docstrings are not available anymore. Furthermore, completing a member of tensorflow is still slow e.g. import tensorflow; tensorflow.abs. I suppose this is because there are less than 10 completions in that case and so type inferring is not disabled. All in all, this doesn't feel like an improvement to me (i.e. correctness trumps performance) but others may disagree.

@davidhalter
Copy link
Owner

I half agree with you. I tend to agree when it comes to type inference, but completions are kind of a weird thing that should also be fast...

I guess I'll wait for feedback, but we might revisit this, if people complain. Also this is just a temporary change and we will hopefully get rid of it in the future.

@micbou
Copy link
Contributor

micbou commented Apr 20, 2019

There have been three reports on the YCM repository because of commit 23b3327: ycm-core/YouCompleteMe#3291, ycm-core/YouCompleteMe#3322, and ycm-core/YouCompleteMe#3372. I believe this is enough to reconsider the change.

@davidhalter
Copy link
Owner

So are you in favor of removing the hack and of reopening this issue?

@micbou
Copy link
Contributor

micbou commented May 2, 2019

Yes, I am. The other option is to patch Jedi which it's something I'd like to avoid.

@davidhalter
Copy link
Owner

davidhalter commented May 3, 2019

I guess I'm fine with reverting it. Never liked it anyway :)

Probably not going to do it in the next few weeks but then. There's quite a big list of things I should be doing (like merging your other PR).

@davidhalter
Copy link
Owner

I just undid the tensorflow speedups. Need different solutions for this, #1059 is probably the way to go.

@davidhalter
Copy link
Owner

BTW: This means that completions take 23s again for intitial ones (previously 1.84s). For repeated completion, it goes down to 0.36s (0.1s previously).

@davidhalter davidhalter changed the title Accessing completion attributes is very slow Accessing completion attributes is very slow (tensorflow) Jun 22, 2019
@davidhalter davidhalter added the database-index Needs a database index/Rewrite in Rust (#1059) label Dec 14, 2019
@davidhalter
Copy link
Owner

Finally I slayed the beast. I finally had an idea how to do this.

Essentially I just cache some things without properly invalidating them if they change, but since some things never change (especially for installed libraries like numpy/tensorflow), it's save to just cache it. The idea seems simple, but applying it at the right place to keep everything working is a bit harder. I guess therefore I just never had the idea that it was even possible.

Benchmarks

Tensorflow:

Before:
First: 6.66
Second: 1.40

After:
First: 7.14
Second: 0.023

Tensorflow completions are now really fast.

Numpy:

Before:
First: 1.848
Second: 0.418


After:
First: 2.9678865449968725
Second: 0.1024

Note that the initial loading is still horribly slow, but IMO that's not a problem that is extremely annoying, because you could just use something like jedi.preload_module('numpy') to improve that by a lot. It's just that every completion after the first is now fast.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database-index Needs a database index/Rewrite in Rust (#1059) performance
Projects
None yet
Development

No branches or pull requests

4 participants