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

Use highest possible pickle protocol #1152

Merged
merged 1 commit into from
Jun 23, 2018

Conversation

micbou
Copy link
Contributor

@micbou micbou commented Jun 20, 2018

Currently, Jedi uses version 2 of the pickle protocol to be compatible with all supported versions of Python. However, we can be smarter and use the highest version of the pickle protocol that is compatible with the version of Python running Jedi itself and the one running the environment. Examples:

  • if both Jedi and the environment are running under Python 3.6, use version 4 of the protocol;
  • if Jedi is running under Python 3.6 but the environment is Python 3.3, use version 3;
  • if Jedi is running under Python 3.6 but the environment is Python 2.7, use version 2;
  • etc.

The reason for using the highest possible version of the pickle protocol is that performance is better with newer versions of the protocol as shown by the following results:

Interpreter Python 2.7 Python 3.6
Environment Python 2.7 Python 3.6 Python 2.7 Python 3.6
Pickle protocol Before After Before After Before After Before After
2 2 2 2 2 2 2 4
os 0.153s 0.159s 0.185s 0.184s 0.0978s 0.105s 0.074s 0.0714s
numpy 0.271s 0.26s 0.262s 0.259s 0.206s 0.198s 0.188s 0.173s
cv2 0.964s 0.949s 1.08s 1.07s 0.513s 0.513s 0.465s 0.308s

Measurements obtained with this script when completing the os, numpy, and cv2 (opencv-python) modules on Ubuntu 18.04 64-bit.


This change is Reviewable

@micbou micbou force-pushed the pickle-protocol branch from 8de08f5 to d0cfe8d Compare June 20, 2018 16:18
@codecov-io
Copy link

codecov-io commented Jun 20, 2018

Codecov Report

Merging #1152 into master will decrease coverage by 0.01%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1152      +/-   ##
==========================================
- Coverage    92.7%   92.69%   -0.02%     
==========================================
  Files          57       57              
  Lines        6910     6912       +2     
==========================================
+ Hits         6406     6407       +1     
- Misses        504      505       +1
Impacted Files Coverage Δ
jedi/api/environment.py 78.53% <100%> (ø) ⬆️
jedi/evaluate/compiled/subprocess/__init__.py 74.75% <75%> (-0.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea71ded...d23b12f. Read the comment docs.

@davidhalter
Copy link
Owner

I'm not that happy with the added complexity, but I can understand the cause. Do you think it's actually worth it? I know it feels like quite a performance increase for cv2 on Python3.6, but it's also just that. I guess I'll leave the decision if I should merge to you.

@micbou micbou force-pushed the pickle-protocol branch from d0cfe8d to d365784 Compare June 21, 2018 22:19
@blueyed
Copy link
Contributor

blueyed commented Jun 23, 2018

Interesting!
highest_pickle_protocol should have some unit tests probably.

@micbou micbou force-pushed the pickle-protocol branch from d365784 to d23b12f Compare June 23, 2018 11:15
@micbou
Copy link
Contributor Author

micbou commented Jun 23, 2018

I'm not that happy with the added complexity, but I can understand the cause. Do you think it's actually worth it? I know it feels like quite a performance increase for cv2 on Python3.6, but it's also just that. I guess I'll leave the decision if I should merge to you.

I think it's worth. All modules with huge files are going to benefit from this and not only on Python 3.6 but all versions of Python starting from 3.3.

highest_pickle_protocol should have some unit tests probably.

Added a bunch of unit tests.

@davidhalter
Copy link
Owner

Fair enough.

@davidhalter davidhalter merged commit 282c6a2 into davidhalter:master Jun 23, 2018
zzbot added a commit to ycm-core/ycmd that referenced this pull request Jun 23, 2018
[READY] Include Jedi performance improvements

This includes PRs davidhalter/jedi#1149, davidhalter/jedi#1150, davidhalter/jedi#1151, and davidhalter/jedi#1152 which significantly improve performance especially on Windows and Python 2.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/1056)
<!-- Reviewable:end -->
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.

4 participants