Skip to content

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

Closed
wants to merge 6 commits into from

Conversation

tompng
Copy link
Member

@tompng tompng commented May 17, 2025

Related to #381 (doing the same rewrite without Integer.sqrt)

This approach violates the policy in #319

Avoid converting values to base-2 representations for computation whenever possible.

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)

x: [2, 121e100, 2e100, '2.'+'2'*100]
prec: [8, 12, 16, 24, 32, 50, 75, 100, 150, 200, 1000, 10000]

(Updated with c804b09)

Number Faster case Slower case
BigDecimal('121e100') prec = 10000: faster(x2.2) prec except 10000: slower(x2~x5.5)
BigDecimal(2) prec = 8, 24, 50, 75, 150, 200, 1000, 10000: faster(x2 ~ x1100) prec = 12, 16, 32, 100: slower(x2 ~ x4)
BigDecimal('2e100') prec except 150: faster(x3 ~ x4000) prec = 150: slower(x1.3)
BigDecimal('2.'+'2'*100) Always faster(x3 ~ x2000) -

@mrkn
Copy link
Member

mrkn commented May 18, 2025

But I think that both performance gain and maintainability gain might be worth enough.

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?

prec = [prec, n_significant_digits].max
ex = prec + BigDecimal.double_fig - exponent / 2
base = BigDecimal(10) ** ex
sqrt = Integer.sqrt(self * base * base)
Copy link
Member

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 $10^n$ faster?

Copy link
Member Author

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.

Copy link
Member

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:

  1. Introduce the decimal shift operation I mentioned previously.
  2. Use BigDecimal("10e#{ex}") instead of using BigDecimal#** for calculating 10**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.

Copy link
Member Author

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

Copy link
Member Author

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)

@tompng
Copy link
Member Author

tompng commented May 18, 2025

By the way, I'm interested in why the slower cases are slower

BigDecimal('121e100') case is slower because the original C implementation has a fast path of convergence.
After adding a fast path 73825c8, slowness improved from x12~x100 slower to x3~x10 slower.

BigDecimal(2) case, the original C implementation is randomly fast or slow depend on precision.

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

raise FloatDomainError, "sqrt of 'NaN'(Not a Number)" if nan?

n_digits = n_significant_digits
prec = [prec, n_digits].max
Copy link
Member Author

Choose a reason for hiding this comment

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

This part is re-implementing #354

def test_const
assert_in_delta(Math::PI, PI(N))
assert_in_delta(Math::E, E(N))
end

def test_sqrt
pend 'Integer.sqrt not correctly implemented' unless integer_sqrt_implemented?
Copy link
Member Author

Choose a reason for hiding this comment

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

Pend test_sqrt and test_atan that uses Integer.sqrt internally. Workaround for ci failure in truffleruby.
Integer.sqrt is fixed in truffleruby-head.

Another options are:

  • Override Integer.sqrt in test
  • Implement _integer_sqrt(n) = Integer.sqrt(n) or ruby_implementation in bigdecimal

@tompng tompng force-pushed the sqrt_in_ruby branch 2 times, most recently from 5d8febe to 1cf18e6 Compare July 18, 2025 15:47
@tompng
Copy link
Member Author

tompng commented Aug 14, 2025

We're doing it with #381

@tompng tompng closed this Aug 14, 2025
@tompng tompng deleted the sqrt_in_ruby branch August 14, 2025 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants