Description

buraksaler

opened on May 7, 2023

Issue body actions

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

• gh-104273: Remove redundant len() calls in argparse function #104274

Activity

1. buraksaler

   added

   type-featureA feature request or enhancementA feature request or enhancement

   on May 7, 2023
2. itamaro commented on May 7, 2023

   

   itamaro

   on May 7, 2023

   Contributor

   More actions

   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.

   Reacted by Alex Waygood and Burak Saler
3. terryjreedy

   changed the title [-]redundant len() calling[/-] [+]argparse: remove redundant len()[/+] on May 7, 2023
4. terryjreedy commented on May 7, 2023

   

   terryjreedy

   on May 7, 2023

   Member

   More actions

   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.

   Reacted by Itamar Oren and Burak Saler
5. terryjreedy

   added

   performancePerformance or resource usagePerformance or resource usage

   and removed

   type-featureA feature request or enhancementA feature request or enhancement

   on May 7, 2023
6. hauntsaninja commented on May 7, 2023

   

   hauntsaninja

   on May 7, 2023

   Contributor

   More actions

   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.

   Reacted by Alex Waygood, Jelle Zijlstra, Burak Saler and Erlend E. Aasland
7. hauntsaninja

   removed

   performancePerformance or resource usagePerformance or resource usage

   on May 7, 2023
8. terryjreedy

   closed this as completedon May 7, 2023
9. terryjreedy

   added a commit that references this issue on May 7, 2023

   gh-104273: Remove redundant len() calls in argparse function (#104274)

   01cc9c1
10. jbower-fb

    added a commit that references this issue on May 8, 2023

    pythongh-104273: Remove redundant len() calls in argparse function (p…

    52539f0
11. carljm

    added a commit that references this issue on May 9, 2023

    Merge branch 'main' into inlinecomp2

    76077cd

Issue actions