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

argparse: remove redundant len() #104273

Closed
buraksaler opened this issue May 7, 2023 · 3 comments
Closed

argparse: remove redundant len() #104273

buraksaler opened this issue May 7, 2023 · 3 comments

Comments

@buraksaler
Copy link
Contributor

buraksaler commented May 7, 2023

Feature or enhancement

I decreased calling of redundant len() function.

Pitch

(Explain why this feature or enhancement should be implemented and how it would be used.
Add examples, if applicable.)

Previous discussion

Linked PRs

@itamaro
Copy link
Contributor

itamaro commented May 7, 2023

hello @buraksaler!

it is not clear from this issue what len() calls you are referring to and why they are redundant.

looking at the linked PR, I'm inferring you refer to the argparse module, where you suggest replacing 3 len() calls with one.
while the change looks functionally correct to me, any change has inherent risks, so making such a change should be justifiable.
can you explain the benefits of making this change? it looks like the intention could have been improving performance by caching the result of a function call, in which case, you should provide benchmarking results (micro or macro) or profiling data showing the improvement from making this change.

@terryjreedy terryjreedy changed the title redundant len() calling argparse: remove redundant len() May 7, 2023
@terryjreedy
Copy link
Member

Justification: indent is a parameter of the helper function get_lines. It is not rebound within the function. Its length is therefore a constant within the function. Hence len(indent) is only needed once.

Calculating common subexpressions just once is a common optimization. I don't think benchmarking is needed.

@terryjreedy terryjreedy added performance Performance or resource usage and removed type-feature A feature request or enhancement labels May 7, 2023
@hauntsaninja
Copy link
Contributor

hauntsaninja commented May 7, 2023

I guess I'm okay with the specific PR, but in general we discourage microoptimisations in cold code, especially if done without any kind of benchmarking. These cost reviewer time, can hurt readability and if poorly done run the risk of regressions. I certainly wouldn't welcome PRs doing CSE wherever possible for calls to len in the standard library.

You'd need several millions of lines of help for this to add up to anything meaningful.

@hauntsaninja hauntsaninja removed the performance Performance or resource usage label May 7, 2023
jbower-fb pushed a commit to jbower-fb/cpython-jbowerfb that referenced this issue May 8, 2023
carljm added a commit to carljm/cpython that referenced this issue May 9, 2023
* main: (47 commits)
  pythongh-97696 Remove unnecessary check for eager_start kwarg (python#104188)
  pythonGH-104308: socket.getnameinfo should release the GIL (python#104307)
  pythongh-104310: Add importlib.util.allowing_all_extensions() (pythongh-104311)
  pythongh-99113: A Per-Interpreter GIL! (pythongh-104210)
  pythonGH-104284: Fix documentation gettext build (python#104296)
  pythongh-89550: Buffer GzipFile.write to reduce execution time by ~15% (python#101251)
  pythongh-104223: Fix issues with inheriting from buffer classes (python#104227)
  pythongh-99108: fix typo in Modules/Setup (python#104293)
  pythonGH-104145: Use fully-qualified cross reference types for the bisect module (python#104172)
  pythongh-103193: Improve `getattr_static` test coverage (python#104286)
  Trim trailing whitespace and test on CI (python#104275)
  pythongh-102500: Remove mention of bytes shorthand (python#104281)
  pythongh-97696: Improve and fix documentation for asyncio eager tasks (python#104256)
  pythongh-99108: Replace SHA3 implementation HACL* version (python#103597)
  pythongh-104273: Remove redundant len() calls in argparse function (python#104274)
  pythongh-64660: Don't hardcode Argument Clinic return converter result variable name (python#104200)
  pythongh-104265 Disallow instantiation of `_csv.Reader` and `_csv.Writer` (python#104266)
  pythonGH-102613: Improve performance of `pathlib.Path.rglob()` (pythonGH-104244)
  pythongh-103650: Fix perf maps address format (python#103651)
  pythonGH-89812: Churn `pathlib.Path` methods (pythonGH-104243)
  ...
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

No branches or pull requests

4 participants