-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Pow function optimization using exponentiation by squaring #785
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
Conversation
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.
Please create a new function instead of replacing the old one.
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.
Isn't this slower? This is using a recursive approach - arguably slower than the iterative one. You should add a separate function instead of overwriting the already fast implementation. If you still think this is fast, could you write a benchmark to see the differences.
Also, note that I'm not against having your approach - in fact, your approach is more mathematical but I question that the title of this PR is suitable for what you're trying to do here.
I decided to benchmark the new implementations. I used the number 50 only so that it is large enough. I used perf.link : and the new implementation is 76% faster. I also used jsbench: and the previous implementation is 77% slower. So I would assume that my own benchmark wasn't precise enough to plot the difference, but the new implementation is faster.
I hope it helps.
booleans-oss
commented
Feb 22, 2022
I made a simple benchmarking workspace to see the execution time difference for the two implementations. For simplicity sake, I use any integers from 0 to 100 and exponents range from 0 to 10000.
The screenshots show the same results as the previous benchmarks: the previous implementation is slower than the new one.
The benchmarks are good, but I would still suggest to keep both implementations.
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.
It's good that you improved this, however my previous comment was just suggesting that you shouldn't remove the previous implementation. I still think you should NOT REMOVE the old implementation but rather create a separate function, calling powIterative and powRecursive or something along those lines.
@farha1 seems like you abandoned this pr. Please go through with the review comments if you still want to work on this.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Isn't this slower? This is using a recursive approach - arguably slower than the iterative one. You should add a separate function instead of overwriting the already fast implementation. If you still think this is fast, could you write a benchmark to see the differences.
No, this implementation is way faster in fact. The naive algorithm does O(n) multiplications where n is the exponent. This algorithm halves the exponent in each step. The recursive call tree thus has logarithmic depth. Yet the effort per call is constant. So this is O(log n), way faster.
In practice the iterative approach might be faster for reasonably small exponents.
I too suggest keeping both, but this is generally the preferable approach.
Suggestion to make it faster: You don't need to check for all edge cases (0, 1, negative exponents) if you check for a couple in the calling func already and then call a recursive helper which only receives the exponents you expect.
@farha1 You need to fix the branch conflicts.
No, this implementation is way faster in fact. The naive algorithm does O(n) multiplications where n is the exponent. This algorithm halves the exponent in each step.
Yeah, didn't notice the halving of the exponent😅
Though still think instead of overwriting the function, we should keep both.
@farha1 seems to have abandoned this pr.
Welcome to JavaScript community
Open in Gitpod know more
Describe your change:
Checklist:
Example:
UserProfile.jsis allowed butuserprofile.js,Userprofile.js,user-Profile.js,userProfile.jsare notFixes: #{$ISSUE_NO}.