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

gh-118761: improve optparse import time by delaying textwrap import #128899

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

eli-schwartz
Copy link
Contributor

@eli-schwartz eli-schwartz commented Jan 16, 2025

The same change was made, and for the same reason, by argparse back in
2017. The textwrap module is only used when printing help text, so most invocations will never need it imported.

See: #1269
See: 8110837

Lib/optparse.py Outdated
Comment on lines 254 to 256
text_width = max(self.width - self.current_indent, 11)
indent = " "*self.current_indent
import textwrap
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: move local imports to the top of the block

Suggested change
text_width = max(self.width - self.current_indent, 11)
indent = " "*self.current_indent
import textwrap
import textwrap
text_width = max(self.width - self.current_indent, 11)
indent = " "*self.current_indent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, makes sense!

For what it's worth the reason I did it this way was a direct consequence of having just been reading argparse.py, which breaks this style suggestion, while documenting the motivation of the change. :) I suppose it is not worth bothering with the churn there as it's just a style nit on code that isn't being modified already...

Lib/optparse.py Outdated
help_text = self.expand_default(option)
import textwrap
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: move local imports to the top of the block

Suggested change
help_text = self.expand_default(option)
import textwrap
import textwrap
help_text = self.expand_default(option)

…port

The same change was made, and for the same reason, by argparse back in
2017. The textwrap module is only used when printing help text, so most
invocations will never need it imported.

See: python#1269
See: 8110837
@picnixz
Copy link
Member

picnixz commented Jan 17, 2025

Out of curiosity, how are the hyperfine and -X importtime benchmarks?

@eli-schwartz
Copy link
Contributor Author

As mentioned in the other PR -- I'm not used to benchmarking and I may be doing something totally wrong because I can get results saying this PR is now slower to import, or results saying it's faster, and hyperfine warns that I have lots of "outliers". But here's a favorable result.

Produced by copying the old and new versions of the file so that I can import them both without switching branches:

$ LD_LIBRARY_PATH=$PWD hyperfine --warmup 8 "./python -c 'import optparse_old'" "./python -c 'import optparse_new'"
Benchmark 1: ./python -c 'import optparse_old'
  Time (mean ± σ):      23.6 ms ±   7.7 ms    [User: 18.4 ms, System: 5.1 ms]
  Range (min … max):    17.2 ms …  47.7 ms    92 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
Benchmark 2: ./python -c 'import optparse_new'
  Time (mean ± σ):      17.5 ms ±   2.8 ms    [User: 14.4 ms, System: 3.0 ms]
  Range (min … max):    16.3 ms …  31.1 ms    98 runs
 
  Warning: The first benchmarking run for this command was significantly slower than the rest (29.2 ms). This could be caused by (filesystem) caches that were not filled until after the first run. You are already using the '--warmup' option which helps to fill these caches before the actual benchmark. You can either try to increase the warmup count further or re-run this benchmark on a quiet system in case it was a random outlier. Alternatively, consider using the '--prepare' option to clear the caches before each timing run.
 
Summary
  ./python -c 'import optparse_new' ran
    1.35 ± 0.49 times faster than ./python -c 'import optparse_old'

@AA-Turner AA-Turner merged commit 3b6a27c into python:main Jan 20, 2025
42 checks passed
@eli-schwartz eli-schwartz deleted the optparse-importtime branch January 20, 2025 00:09
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 21, 2025
The same change was made, and for the same reason, by ``argparse`` back in
2017. The ``textwrap`` module is only used when printing help text, so most
invocations will never need it imported.

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
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