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

improve show_call_signatures performance #282

Closed
wants to merge 1 commit into from
Closed

Conversation

hattya
Copy link
Contributor

@hattya hattya commented Jul 3, 2014

The current implementation of show_call_signatures is:

  1. fire CursorMovedI
  2. parse the current buffer
  3. show call signatures

These steps are processed on every cursor move.

This tries to reduce parsing the current buffer as much as possible.
I hope this fixes #217.

@davidhalter
Copy link
Owner

Hmm, two things first:

  • I think that this logic would be better of living inside Jedi. It's a general issue and having a separate implementation in every IDE doesn't seem to make sense, IMHO.
  • There is already such an implementation in the current Jedi branches. The problem is that the last release removed support for that cache or rather it did it twice: https://github.com/davidhalter/jedi/blob/v0.8.0/jedi/api/__init__.py#L573 This line has since been removed again. It was inserted for debugging purposes. The next release will therefore contain a fast show_call_signatures performance again!

What do you think, is your PR still needed?

@hattya
Copy link
Contributor Author

hattya commented Jul 3, 2014

I've tested without this changes and removed the line you mentioned, but it was slow as current.

@hattya
Copy link
Contributor Author

hattya commented Jul 3, 2014

@hattya
Copy link
Contributor Author

hattya commented Jul 3, 2014

As documented in http://jedi.jedidjah.ch/en/latest/docs/plugin-api.html#jedi.api.Script.call_signatures, this method does not make sense when you call it from the outside of function call.
But it is difficult to know that the cursor is at the inside of function call, so this checks that the cursor is at the inside of parentheses instead.

@hattya
Copy link
Contributor Author

hattya commented Jul 3, 2014

And it is the same function object if the cursor is at the inside of same parentheses before, so caches it with the position of (.
This is the way to reduce parsing the current buffer as much as possible.

@hattya
Copy link
Contributor Author

hattya commented Jul 3, 2014

I understand your concern, but it depends on the implementation detail, so I think it should implement in jedi-vim.

@davidhalter
Copy link
Owner

@hattya Parsing shouldn't be the problem, Jedi's parser is extremely fast and only parses the parts that have changed.

I'm happy to be proved otherwise, you can give me an example...

However I will be on vacation for the next week. Don't expect an answer :-)

@hattya
Copy link
Contributor Author

hattya commented Jul 4, 2014

signatures_cache should be invalidated when the cursor is moved to out of parentheses.
I've updated the PR.

@hattya
Copy link
Contributor Author

hattya commented Jul 4, 2014

This script is illustrated the issues of jedi.parser.fast.FastParser.
https://gist.github.com/hattya/47ffe143d83be6f8d3dc

on my low-spec PC:

$ python jedi-test.py
* source splits into many parts
>>> over 10 parts
- 0.05
- 0.01
- 0.02
>>> over 100 parts
- 0.26
- 0.04
- 0.04
>>> over 1000 parts
- 2.78
- 0.79
- 0.79
--
* each parts are contained many lines
>>> 10 lines
- 0.04
- 0.01
- 0.02
>>> 100 lines
- 0.07
- 0.09
- 0.07
>>> 1000 lines
- 0.72
- 0.72
- 0.72

Now you see the issues.

  1. It splits the source into parts, and do linear search to find the parts that have changed, so the parsing time is increased if the source is split into many parts. (ex: many class, function or method definitions)
  2. It parses the parts that have changed, so the parsing time is also increased if the parts contain many lines.

The examples:

@davidhalter
Copy link
Owner

I will check this ASAP. I was on vacation the last 9 days.

@davidhalter
Copy link
Owner

Do we still need this one after your improvements to Jedi?

@hattya
Copy link
Contributor Author

hattya commented Jul 18, 2014

I updated the script
https://gist.github.com/hattya/47ffe143d83be6f8d3dc

  • clear all cache before for each test
  • tests for datetimetester.py and re_tests.py

on my low-spec PC:

$ python jedi-test.py
* source splits into many parts
>>> over 10 parts
1st: 0.03
inc: 0.0
>>> over 100 parts
1st: 0.19
inc: 0.00667
>>> over 1000 parts
1st: 3.06
inc: 0.13
* each parts are contained many lines
>>> 10 lines
1st: 0.01
inc: 0.0
>>> 100 lines
1st: 0.07
inc: 0.00333
>>> 1000 lines
1st: 0.73
inc: 0.01333
* datetimetester.py
1st: 4.71
inc: 0.19
* re_tests.py
1st: 0.7
inc: 0.01

Jedi is linter (davidhalter/jedi@2616143)

We need this even if Jedi is made faster.

@wilywampa
Copy link
Contributor

Jedi underlines the current argument in the call signature as you type the new arguments, but that doesn't work with this patch. Maybe keep track of how many commas are in the current parentheses and update when that changes?

@hattya
Copy link
Contributor Author

hattya commented Nov 21, 2014

@wilywampa Thank you for testing!

signatures_cache should be invalidated when a , is typed.
I've updated the PR.

@hattya
Copy link
Contributor Author

hattya commented Nov 22, 2014

I've update the PR.
It counts the number of commas in front of the cursor inside the current parentheses to determine the index of current argument.

@davidhalter
Copy link
Owner

@hattya Sorry for not merging it, yet.

I'm just not sure if something like this should be part of Jedi? Generally I would prefer that. Isn't that possible as well?

I'm currently rewriting Jedi's parser, so I don't have a lot of time for it. I just wanted to explain why I haven't merged yet. I don't think your solution is bad. It's probably quite good, but I don't want to bloat jedi-vim, if we can improve Jedi.

@davidhalter
Copy link
Owner

david@enrico:~/source/python$ python hattya_script.py 
* source splits into many parts
>>> over 10 parts
1st: 0.0
inc: 0.0
>>> over 100 parts
1st: 0.0
inc: 0.0
>>> over 1000 parts
1st: 0.0
inc: 0.0
* each parts are contained many lines
>>> 10 lines
1st: 0.01
inc: 0.0
>>> 100 lines
1st: 0.02
inc: 0.00667
>>> 1000 lines
1st: 0.14
inc: 0.05
* datetimetester.py
1st: 0.01
inc: 0.00333
* re_tests.py
1st: 0.0
inc: 0.00333

The time for datetimetester is now down to from 4.71s to 0.01s. Which I think is pretty ok ;-)
That's what I meant with improving the speed in general and not with a hackish jedi-vim solution.

Your solution was very nice though, thanks again! Especially for the test script!

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.

3 participants