-
Notifications
You must be signed in to change notification settings - Fork 78
Rewrite BigDecimal#sqrt in ruby using Integer.sqrt #323
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
base: master
Are you sure you want to change the base?
Conversation
This approach’s performance advantage stems from the current, sub-optimal implementations of BigDecimal’s multiplication and division. However, once those operations are optimized, the advantage will likely disappear. Therefore, I’m willing to accept this change as a provisional improvement to the performance of sqrt. I don't want to let bigdecimal depend on GMP due to maintainability. By the way, I'm interested in why the slower cases are slower. Do you know the reasons? |
lib/bigdecimal.rb
Outdated
prec = [prec, n_significant_digits].max | ||
ex = prec + BigDecimal.double_fig - exponent / 2 | ||
base = BigDecimal(10) ** ex | ||
sqrt = Integer.sqrt(self * base * base) |
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.
How about adding the new methods for decimal shift operation to calculate the multiplication by
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.
Cost of multiplication/division with 10**n is not dominant in normal path, but in the fast path such as BigDecimal('16e1000000').sqrt(1000000)
, the improvement of using decimal shift operation is significant.
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.
How much does the calculation of 10**n
cost? I think we can further reduce the total cost by following two ways:
- Introduce the decimal shift operation I mentioned previously.
- Use
BigDecimal("10e#{ex}")
instead of usingBigDecimal#**
for calculating10**n
.
We reduce the cost of self * base * base
by the former, and the cost of calculating base
by the latter.
Instead of the latter way, implementing the fast pass for 10**n
in VpPowerByInt
is ok to me.
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.
scenario | decimal_shift | "1e{prec}" |
ten**prec |
---|---|---|---|
prec=10, 100000.times | 0.149624s | 0.244520s | 0.245961s |
prec=100, 100000.times | 0.442031s | 0.481656s | 0.605150s |
prec=1000, 100000.times | 2.846068s | 2.963888s | 3.544875s |
prec=10000, 10000.times | 3.441482s | 3.443580s | 3.844375s |
Comparision of:
BigDecimal(Integer.sqrt(BigDecimal(2)._decimal_shift(2*prec)))._decimal_shift(-prec)
Integer.sqrt(BigDecimal(2)*BigDecimal("1e#{2*prec}"))*BigDecimal("1e-#{prec}")
base = BigDecimal(10)**prec; Integer.sqrt(BigDecimal(2)*base*base)/base
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.
Updated the implementation c804b09 with * BigDecimal("1e#{2 * ex}")
and * BigDecimal("1e#{-ex}")
(this is 4 times faster than division)
irb(main):005> x=BigDecimal(2); 10000.times{x.sqrt 50}
processing time: 0.336679s
irb(main):006> x=BigDecimal(2); 10000.times{x.sqrt 100} # should be slower, but 10 times faster
processing time: 0.024903s |
This approach violates the policy in #319
But I think that both performance gain and maintainability gain might be worth enough.
Using
Integer.sqrt
Ruby's
Integer.sqrt
is pretty fast. Bignum can use GMP for division and base conversion if available.time_of{Integer.sqrt(n)} ≒ 2 * time_of{n/sqrt_n}
time_of{BigDecimal(Integer.sqrt(bigdecimal.to_i))} ≒ 4 * time_of{Integer.sqrt(n)}
Benchmark of
x.sqrt(prec)
(Updated with c804b09)