Skip to content

Conversation

thatbirdguythatuknownot
Copy link
Contributor

@thatbirdguythatuknownot thatbirdguythatuknownot commented Jan 17, 2022

@sweeneyde
Copy link
Member

Come to think of it, this is kind of two separate orthogonal changes, right? It may be easier to benchmark, review and get parts merged if this was two PRS, one to implement the 1 << x and 2**x changes, and the other to separate l_divmod from l_mod

…07.00aN9w.rst

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@thatbirdguythatuknownot
Copy link
Contributor Author

thatbirdguythatuknownot commented Jan 17, 2022

Come to think of it, this is kind of two separate orthogonal changes, right? It may be easier to benchmark, review and get parts merged if this was two PRS, one to implement the 1 << x and 2**x changes, and the other to separate l_divmod from l_mod

How should I separate it? Should I get rid of the l_divmod/l_mod changes in this PR, change the title, then open a new PR with the l_divmod/l_mod changes?

@sweeneyde
Copy link
Member

Come to think of it, this is kind of two separate orthogonal changes, right? It may be easier to benchmark, review and get parts merged if this was two PRS, one to implement the 1 << x and 2**x changes, and the other to separate l_divmod from l_mod

How should I separate it? Should I get rid of the l_divmod/l_mod changes in this PR, change the title, then open a new PR with the l_divmod/l_mod changes?

Sounds good to me

@thatbirdguythatuknownot thatbirdguythatuknownot changed the title bpo-46407: Optimizing power-of-2 and only-modulo operations bpo-46407: Optimizing power-of-2 operations Jan 17, 2022
@sweeneyde sweeneyde requested a review from mdickinson January 17, 2022 23:28
Use the DIGIT_ONE static variable whenever possible
…07.00aN9w.rst

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@thatbirdguythatuknownot
Copy link
Contributor Author

It seems like this PR has lost all activity.

@mdickinson
Copy link
Member

I'm unassigning myself from review. I think @tim-one would be better placed to decide whether this is an optimisation worth making, and if so review the code.

@mdickinson mdickinson removed their request for review January 22, 2022 12:49
@AlexWaygood AlexWaygood requested a review from tim-one January 22, 2022 12:50
@thatbirdguythatuknownot
Copy link
Contributor Author

PR bump.

@thatbirdguythatuknownot
Copy link
Contributor Author

thatbirdguythatuknownot commented Jan 25, 2022

PR bump 2.

@thatbirdguythatuknownot
Copy link
Contributor Author

PR bump 3.

@thatbirdguythatuknownot
Copy link
Contributor Author

PR has lost all activity.

Copy link
Member

@tim-one tim-one left a comment

Choose a reason for hiding this comment

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

I appreciate the effort, but I'm inclined to reject this change. The increase in code complexity isn't justified by the relatively small (~6%) speedup the bpo report showed for a best-case isolated microbenchmark (x**10000000 after x=2). There are few cases the patch can help with, but testing for those cases slightly slows down all other cases (tests-and-branches to skip the new code), and also blows space in I cache for code that won't be executed most times.

long_pow() in particular is already a long-winded function without this, where existing optimizations have major payoffs that apply in many cases. The generality and high bang-for-the-buck nature of those justify their code complexity.

The other change - introducing remainder-only functions - was much more attractive on these grounds (no runtime tests needed - we pick which we want in each context at compile-time, and they're always faster when they're picked).

@thatbirdguythatuknownot
Copy link
Contributor Author

I see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants