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

[Needs discussion] Improve startup time through lazy-loading #285

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jdemaeyer
Copy link
Contributor

@jdemaeyer jdemaeyer commented Jun 6, 2017

shub is excruciatingly slow for a command line tool. On my machine, shub version takes almost two seconds to print the version string.

I did some profiling: the time is mostly spent while importing pip and requests (and by extension while importing scrapinghub, because it imports requests). Both of these modules are always imported, because they are top-level imported in shub.utils, and almost all command modules import utility functions. Additionally, they are even imported if the command module does not import utility functions (e.g. shub.version), because all command modules are imported in shub.tool.

This PR does two things:

  • Lazy-import pip, requests, and scrapinghub in the utility functions. This reduces the import time for shub.utils from ~900 ms to ~120 ms.
  • Because we use requests all over the place and it is impractical to lazy-import it in every module, change shub.tool so that, if a command is given, it imports only that command's module, and not all command modules.

This is the time required to run shub version on my machine:

codebase time w/ global packages time in fresh virtualenv*
master 1.799 s 0.845 s
w/ only the first commit (lazy-importing pip in shub.utils) 1.043 s 0.550 s
all commits in this PR 0.130 s 0.146 s
*virtualenv has shub and pyopenssl installed. It's fair to assume that this is true for most real-world environments

There may be unforeseen side effects with the lazy imports, and the improvement from lazy-importing requests may be somewhat of a sham because almost all commands need it anyways (with the exception of some shub image commands). What do you think @chekunkov @dangra?

@codecov-io
Copy link

codecov-io commented Jun 6, 2017

Codecov Report

Merging #285 into master will decrease coverage by 0.11%.
The diff coverage is 68.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #285      +/-   ##
==========================================
- Coverage   91.36%   91.25%   -0.12%     
==========================================
  Files          30       30              
  Lines        1958     1966       +8     
==========================================
+ Hits         1789     1794       +5     
- Misses        169      172       +3
Impacted Files Coverage Δ
shub/utils.py 89.3% <66.66%> (-0.17%) ⬇️
shub/tool.py 91.3% <66.66%> (-3.7%) ⬇️
shub/image/__init__.py 83.33% <75%> (-5.56%) ⬇️

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 7d8136e...e4cc75c. Read the comment docs.

@jdemaeyer jdemaeyer force-pushed the improve-startup-time branch from 527d45f to e4cc75c Compare June 6, 2017 15:10
@vshlapakov
Copy link
Contributor

@jdemaeyer I'd say that it's a good idea: maybe most of the commands use the mentioned libraries, but from my feeling running a helper (say shub deploy --help) shouldn't take more than a second.

The idea is pretty popular nowadays, and there're a couple of projects implementing it, maybe we could reuse something (at least an approach concept to avoid additional dependencies) and not just hide imports to the helpers:

@rafaelcapucho rafaelcapucho force-pushed the master branch 2 times, most recently from a290a34 to b0e4614 Compare January 15, 2021 02:28
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