-
Notifications
You must be signed in to change notification settings - Fork 18k
math/big: improve performance of nat.mulRange #65027
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
Comments
It should be possible to implement mulRange by allocating the close-to-correct amount of space up-front and then compute the product using |
Since
Benchmarks:
yields
|
Comments by Bakul Shah bakul@iitbombay.org led me to investigate very large inputs. By building up large values on each "side" of the recursion, Karatsuba gets used for the larger multiplies and the recursive version begins to win. On I will write up a hybrid that uses the iterative for shorter spans which ought to get us the best of both worlds. |
For reference: https://groups.google.com/g/golang-nuts/c/7kcFb41ARgM/m/5sSxZlKfAgAJ Some background and other interesting algorithms for factorials: http://www.luschny.de/math/factorial/FastFactorialFunctions.htm Richard Fateman's CL implementations: https://people.eecs.berkeley.edu/~fateman/papers/factorial.pdf |
I have a simple hybrid implementation that looks like this:
Unfortunately, the cutoff Would a "magic" cutoff like |
On Sun, Jan 7, 2024 at 5:46 PM John Jannotti <jannotti@gmail.com> wrote:
I enjoy bignum implementations, so I was looking through nat.go and saw that
mulRange
is implemented in a surprising, recursive way,. In the non-base case,mulRange(a, b)
returnsmulrange(a, (a+b)/2) * mulRange(1+(a+b)/2, b)
(lots of big.Int ceremony elided).That's fine, but I didn't see any advantage over the straightforward (and simpler?) for loop.
In fact, I suspected the existing code was slower, and allocated a lot more. That seems true. A quick benchmark, using the existing unit test as the benchmark, yields
BenchmarkRecusiveMulRangeN-10 169417 6856 ns/op 9452 B/op 338 allocs/op
BenchmarkIterativeMulRangeN-10 265354 4269 ns/op 2505 B/op 196 allocs/op
I doubt
mulRange
is a performance bottleneck in anyone's code! But it is exported asint.MulRange
so I guess it's viewed with some value. And seeing as how the for-loop seems even easier to understand that the recursive version, maybe it's worth submitting a PR? (If so, should I create an issue first?)The text was updated successfully, but these errors were encountered: