Skip to content
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

speedup BigDecimal division #5675

Merged
merged 6 commits into from Apr 6, 2019

Conversation

Projects
None yet
2 participants
@kares
Copy link
Member

kares commented Apr 2, 2019

in certain cases its really slow e.g. a x / 2 as discovered from #5650

TIMES = (ENV['TIMES'] || 10_000_000).to_i

Benchmark.bmbm do |bm|
  bm.report("/2 flt") {
    rad_per_deg = Math::PI/180
    loc1 = [46.3625, 15.114444]
    loc2 = [46.055556, 14.508333]
    val = ((loc2[1]-loc1[1]) * rad_per_deg)
    TIMES.times { val / 2 }
  }

  bm.report("/2 dec") {
    rad_per_deg = Math::PI/180
    loc1 = [BigDecimal('46.3625'), BigDecimal('15.114444')]
    loc2 = [BigDecimal('46.055556'), BigDecimal('14.508333')]
    val = ((loc2[1]-loc1[1]) * rad_per_deg)
    TIMES.times { val / 2 }
  }

  bm.report(" 1/1280") {
    one = BigDecimal(1)
    div = BigDecimal(1280)
    TIMES.times { one / div }
  }
end

here the "/2 dec" case is very slow - on JRuby 9.2.6 taking 400s, while MRI takes 10s :

                 user     system      total        real
/2 flt       0.670000   0.030000   0.700000 (  0.351421)
/2 dec     444.870000   0.930000 445.800000 (434.352299)
 1/1280     38.520000   0.100000  38.620000 ( 36.827996)

/2 flt       0.433256   0.000000   0.433256 (  0.433262)
/2 dec       9.910286   0.000000   9.910286 (  9.910299)
 1/1280      7.118558   0.000000   7.118558 (  7.118567)

... with this patch JRuby gets close to MRI's performance (over 30x improvement for the above / 2 case) :

/2 flt       0.740000   0.020000   0.760000 (  0.379111)
/2 dec      14.560000   0.240000  14.800000 ( 13.860334)
 1/1280      7.760000   0.030000   7.790000 (  6.865561)

Should be noted that only division (backed by java.math.BigDecimal#divide(another, context)) seemed slow, other decimal operations are either faster on on par with MRI. The 'new' division algorithm is based on Android's sources (ASF2 license) with a few tweaks and a major (in terms of performance) change of not down scaling exact results (remainder 0). Leaving that off to java.math.BigDecimal(BigInteger, int, MathContext) which will reduce the scale - n (doing BigInteger div 10^n) faster than we can using public API.

kares added some commits Mar 22, 2019

use BigDecimal.limit on div and do second guess
due performance issues - trying to end up with lower precision
tune BigDecimal division (to do less work)
as scale adjusting (10x) is performed by Java's constructor
... and since we're faster -> remove second precision guess

@kares kares added this to the JRuby 9.2.7.0 milestone Apr 2, 2019

@kares kares requested review from enebo and headius Apr 2, 2019

@enebo

enebo approved these changes Apr 5, 2019

Copy link
Member

enebo left a comment

I see some delta changes. I am ok with this but I was wondering if you envision any changes in various test suites from this?

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Apr 5, 2019

that the default delta is 0.01 or so - thus added an explicit delta to make those asserts meaningful

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Apr 5, 2019

@kares I just meant will we start seeing new errors from test suites like Rails which is assuming a particular rounding or something like that. I am ok with that but I just am wondering what potential fallout is possible if we are not rounding exactly the same as we were.

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Apr 5, 2019

we're not rounding exactly but weren't doing so before anyway. we do not match MRI with division, although have gotten better in 9.2 (compared to <= 9.1) and have some asserts guarding against regressions in terms of precision. so we should be doing the same (pbly even better regarding precision) as before.

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Apr 5, 2019

@kares thanks. lgtm!

@kares kares merged commit 1a1719a into jruby:master Apr 6, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.