-
Notifications
You must be signed in to change notification settings - Fork 156
Improve performance of expo by multiplying previous result #103
base: master
Are you sure you want to change the base?
Conversation
|
This seems good to me. Just curious, did you run into this because you in your use case the number of retries was very high? |
|
No, I was just browsing the code and noticed that some work was unnecessarily re-done on each loop. I think it's pretty unlikely that anyone would reach enough retries for this to actually make much of a difference, but I figured I'd submit it anyway since it's such a simple change. |
e11e666 to
28552d5
Compare
|
Any chance this can get merged? PR has been open for over a year. |
|
Ping? |
| a = 1 | ||
| while True: | ||
| a = factor * base ** n | ||
| if max_value is None or a < max_value: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This max_value condition doesn't work properly with your changes, it needs to be updated a bit, like this, but still 24x faster than the original:
base_n = 1
while True:
a = factor * base_n
if max_value is None or a < max_value:
yield a
base_n *= baseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks.
28552d5 to
651a453
Compare
This computes the exponential by multiplying the previous result by
baseinstead of computing it from scratch with**each time, which seems to be about an order of magnitude faster. On my machine the following tests report around 2.63 seconds forexpo_oldand 0.10 forexpo_new.