Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

Deferring OpenSSL import until usage. #159

Merged
merged 1 commit into from
Apr 15, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Apr 8, 2015

This is to speed up import times. In OpenSSL 0.14 the import takes
0.5 seconds due to cffi on-demand build of extensions in the
cryptography library.

For classes and functions which are conditionally defined based on
the existence of OpenSSL.crypto, we check that the module
exists (but don't import it) using imp.find_module.

See pyca/pyopenssl#137 for context (originally discovered by me because of google/apitools#14).

@dhermes
Copy link
Contributor Author

dhermes commented Apr 8, 2015

/cc @craigcitro

@coveralls
Copy link

Coverage Status

Coverage increased (+0.16%) to 64.96% when pulling 9dc6ba2 on dhermes:speedup-openssl-import into 0a6241c on google:master.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 10, 2015

Is there anyone to review this?

/cc @nathanielmanistaatgoogle @anthmgoogle

@anthmgoogle
Copy link

I took a look, but I don't think I know this part of the code or the language subtleties well enough to be the sole approver. I would suggest @nathanielmanistaatgoogle, but possibly also @craigcitro or @jcgregorio.

@@ -37,7 +39,10 @@ class AppIdentityError(Exception):


try:
from OpenSSL import crypto
_, package_dir, _ = imp.find_module('OpenSSL')

This comment was marked as spam.

This comment was marked as spam.

@craigcitro
Copy link
Contributor

one little thing, otherwise LGTM

This is to speed up import times. In OpenSSL 0.14 the import takes
0.5 seconds due to cffi on-demand build of extensions in the
cryptography library.

For classes and functions which are conditionally defined based on
the existence of OpenSSL.crypto, we check that the module
exists (but don't import it) using imp.find_module.
@dhermes
Copy link
Contributor Author

dhermes commented Apr 15, 2015

@craigcitro I git push -fed with suggested change.

Feel free to take a second look.

I don't have merge privileges so someone will need to use your LGTM for me.

@craigcitro
Copy link
Contributor

@dhermes yeah, i don't think we have an easy way to give non-googlers push rights on this repo.

merging now.

craigcitro added a commit that referenced this pull request Apr 15, 2015
Deferring OpenSSL import until usage.
@craigcitro craigcitro merged commit 22a532d into googleapis:master Apr 15, 2015
@dhermes dhermes deleted the speedup-openssl-import branch April 15, 2015 22:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants