let's try 'better' approximating MRI's BigDecimal precision (/ and *)

esp. with division/multiplication - some parts now match C source closer however there's a fundamental difference in terms of MRI keeping the precision of a constructed number, while Java's BigDecimal doesn't ... thus we will tend to end up with different precisions based on input. this needs to be cared out carefully as going 'too far', as some of the ruby specs never finish computing (division atm) ... closes #3846, #4200 (we have tests guarding against regression)
jruby · Apr 15, 2018 · 4b004f1 · 4b004f1
1 parent c7658af
commit 4b004f1
Showing 3 changed files with 153 additions and 45 deletions.
diff --git a/core/src/main/java/org/jruby/ext/bigdecimal/RubyBigDecimal.java b/core/src/main/java/org/jruby/ext/bigdecimal/RubyBigDecimal.java
@@ -139,6 +139,8 @@ public RubyBigDecimal allocate(Ruby runtime, RubyClass klass) {
     // #elif SIZEOF_BDIGITS >= 4
     // # define RMPD_COMPONENT_FIGURES 9
     private static final short RMPD_COMPONENT_FIGURES = 9;
+    // rmpd_component_figures(void) { return RMPD_COMPONENT_FIGURES; }
+    // #define VpBaseFig() rmpd_component_figures()
     private static final short BASE_FIG = RMPD_COMPONENT_FIGURES;
 
     // Static constants
@@ -279,7 +281,7 @@ public IRubyObject _dump(ThreadContext context, IRubyObject unused) {
     @JRubyMethod(meta = true)
     public static RubyBigDecimal _load(ThreadContext context, IRubyObject recv, IRubyObject from) {
         String precisionAndValue = from.convertToString().asJavaString();
-        String value = precisionAndValue.substring(precisionAndValue.indexOf(":")+1);
+        String value = precisionAndValue.substring(precisionAndValue.indexOf(':') + 1);
         return newInstance(context, recv, RubyString.newString(context.runtime, value));
     }
 
@@ -452,7 +454,18 @@ private static BigDecimal toBigDecimal(final RubyInteger value) {
         return new BigDecimal(value.getBigIntegerValue());
     }
 
-    private static RubyBigDecimal getVpValue19(ThreadContext context, IRubyObject value, boolean must) {
+    private static RubyBigDecimal getVpRubyObjectWithPrecInner(ThreadContext context, RubyRational value, RoundingMode mode) {
+        BigDecimal numerator = toBigDecimal(value.getNumerator());
+        BigDecimal denominator = toBigDecimal(value.getDenominator());
+
+        int len = numerator.precision() + denominator.precision(); // 0
+        int pow = len / 4; // 0
+        MathContext mathContext = new MathContext((pow + 1) * 4, mode);
+
+        return new RubyBigDecimal(context.runtime, numerator.divide(denominator, mathContext));
+    }
+
+    private RubyBigDecimal getVpValueWithPrec(ThreadContext context, IRubyObject value, boolean must) {
         if (value instanceof RubyFloat) {
             double doubleValue = ((RubyFloat) value).getDoubleValue();
 
@@ -463,10 +476,11 @@ private static RubyBigDecimal getVpValue19(ThreadContext context, IRubyObject va
                 throw context.runtime.newFloatDomainError("NaN");
             }
 
-            return new RubyBigDecimal(context.runtime, BigDecimal.valueOf(doubleValue));
+            MathContext mathContext = new MathContext(RubyFloat.DIG + 1, getRoundingMode(context.runtime));
+            return new RubyBigDecimal(context.runtime, new BigDecimal(doubleValue, mathContext));
         }
         if (value instanceof RubyRational) {
-            return getVpRubyObjectWithPrecInner(context, (RubyRational) value, getRoundingMode(context.runtime));
+            return div2Impl(context, ((RubyRational) value).getNumerator(), ((RubyRational) value).getDenominator(), this.value.precision() * BASE_FIG);
         }
 
         return getVpValue(context, value, must);
@@ -488,17 +502,6 @@ private static RubyBigDecimal getVpValue(ThreadContext context, IRubyObject valu
         return cannotBeCoerced(context, value, must);
     }
 
-    private static RubyBigDecimal getVpRubyObjectWithPrecInner(ThreadContext context, RubyRational value, RoundingMode mode) {
-        BigDecimal numerator = toBigDecimal(value.getNumerator());
-        BigDecimal denominator = toBigDecimal(value.getDenominator());
-
-        int len = numerator.precision() + denominator.precision(); // 0
-        int pow = len / 4; // 0
-        MathContext mathContext = new MathContext((pow + 1) * 4, mode);
-
-        return new RubyBigDecimal(context.runtime, numerator.divide(denominator, mathContext));
-    }
-
     @JRubyMethod(meta = true)
     public static IRubyObject induced_from(ThreadContext context, IRubyObject recv, IRubyObject arg) {
         return getVpValue(context, arg, true);
@@ -816,20 +819,19 @@ public IRubyObject initialize_copy(IRubyObject original) {
             throw getRuntime().newTypeError("wrong argument class");
         }
 
-        RubyBigDecimal origRbd = (RubyBigDecimal)original;
+        RubyBigDecimal orig = (RubyBigDecimal) original;
 
-        this.isNaN = origRbd.isNaN;
-        this.infinitySign = origRbd.infinitySign;
-        this.zeroSign = origRbd.zeroSign;
-        this.value = origRbd.value;
+        this.isNaN = orig.isNaN;
+        this.infinitySign = orig.infinitySign;
+        this.zeroSign = orig.zeroSign;
+        this.value = orig.value;
 
         return this;
     }
 
     @JRubyMethod(name = {"%", "modulo"}, required = 1)
     public IRubyObject op_mod(ThreadContext context, IRubyObject other) {
-        // TODO: full-precision divmod is 1000x slower than MRI!
-        RubyBigDecimal val = getVpValue19(context, other, false);
+        RubyBigDecimal val = getVpValueWithPrec(context, other, false);
 
         if (val == null) return callCoerced(context, sites(context).op_mod, other, true);
         if (isNaN() || val.isNaN() || isInfinity() && val.isInfinity()) return newNaN(context.runtime);
@@ -853,7 +855,7 @@ public IRubyObject op_mod19(ThreadContext context, IRubyObject arg) {
     @Override
     @JRubyMethod(name = "remainder", required = 1)
     public IRubyObject remainder(ThreadContext context, IRubyObject arg) {
-        return remainderInternal(context, getVpValue19(context, arg, false), arg);
+        return remainderInternal(context, getVpValueWithPrec(context, arg, false), arg);
     }
 
     @Deprecated
@@ -862,7 +864,6 @@ public IRubyObject remainder19(ThreadContext context, IRubyObject arg) {
     }
 
     private IRubyObject remainderInternal(ThreadContext context, RubyBigDecimal val, IRubyObject arg) {
-        // TODO: full-precision remainder is 1000x slower than MRI!
         if (isInfinity() || isNaN()) return newNaN(context.runtime);
         if (val == null) return callCoerced(context, sites(context).remainder, arg, true);
         if (val.isInfinity() || val.isNaN() || val.isZero()) return newNaN(context.runtime);
@@ -873,7 +874,7 @@ private IRubyObject remainderInternal(ThreadContext context, RubyBigDecimal val,
 
     @JRubyMethod(name = "*", required = 1)
     public IRubyObject op_mul(ThreadContext context, IRubyObject arg) {
-        RubyBigDecimal val = getVpValue19(context, arg, false);
+        RubyBigDecimal val = getVpValueWithPrec(context, arg, false);
         if (val == null) return callCoerced(context, sites(context).op_times, arg, true);
         return multImpl(context.runtime, val);
     }
@@ -888,7 +889,7 @@ public IRubyObject mult2(ThreadContext context, IRubyObject b, IRubyObject n) {
         final int mx = getPrecisionInt(context.runtime, n);
         if (mx == 0) return op_mul(context, b);
 
-        RubyBigDecimal val = getVpValue19(context, b, false);
+        RubyBigDecimal val = getVpValueWithPrec(context, b, false);
         if (val == null) { // TODO: what about n arg?
             return callCoerced(context, sites(context).op_times, b, true);
         }
@@ -1036,7 +1037,7 @@ private BigDecimal powNegative(final int times) {
 
     @JRubyMethod(name = "+")
     public IRubyObject op_plus(ThreadContext context, IRubyObject b) {
-        return addInternal(context, getVpValue19(context, b, false), b, vpPrecLimit(context.runtime));
+        return addInternal(context, getVpValueWithPrec(context, b, false), b, vpPrecLimit(context.runtime));
     }
 
     @Deprecated
@@ -1046,7 +1047,7 @@ public IRubyObject op_plus19(ThreadContext context, IRubyObject b) {
 
     @JRubyMethod(name = "add")
     public IRubyObject add2(ThreadContext context, IRubyObject b, IRubyObject digits) {
-        return addInternal(context, getVpValue19(context, b, false), b, digits);
+        return addInternal(context, getVpValueWithPrec(context, b, false), b, digits);
     }
 
     @Deprecated
@@ -1131,7 +1132,7 @@ public IRubyObject op_uminus(ThreadContext context) {
 
     @JRubyMethod(name = "-", required = 1)
     public IRubyObject op_minus(ThreadContext context, IRubyObject b) {
-        return subInternal(context, getVpValue19(context, b, true), b, 0);
+        return subInternal(context, getVpValueWithPrec(context, b, true), b, 0);
     }
 
     @Deprecated
@@ -1141,7 +1142,7 @@ public IRubyObject op_minus19(ThreadContext context, IRubyObject b) {
 
     @JRubyMethod(name = "sub", required = 2)
     public IRubyObject sub2(ThreadContext context, IRubyObject b, IRubyObject n) {
-        return subInternal(context, getVpValue19(context, b, false), b, getPositiveInt(context, n));
+        return subInternal(context, getVpValueWithPrec(context, b, false), b, getPositiveInt(context, n));
     }
 
     @Deprecated
@@ -1186,7 +1187,7 @@ private RubyBigDecimal subSpecialCases(ThreadContext context, RubyBigDecimal val
 
     @JRubyMethod(name = {"/", "quo"})
     public IRubyObject op_quo(ThreadContext context, IRubyObject other) {
-        RubyBigDecimal val = getVpValue19(context, other, false);
+        RubyBigDecimal val = getVpValueWithPrec(context, other, false);
         if (val == null) return callCoerced(context, sites(context).op_quo, other, true);
 
         if (isNaN() || val.isNaN()) return newNaN(context.runtime);
@@ -1198,17 +1199,31 @@ public IRubyObject op_quo(ThreadContext context, IRubyObject other) {
     }
 
     private RubyBigDecimal quoImpl(ThreadContext context, RubyBigDecimal that) {
-        // regular division with some default precision
-        // proper algorithm to set the precision
-        // the precision is multiple of 4
-        // and the precision is larger than len * 2
-        int len = this.value.precision() + that.value.precision();
-        int scale = (len / 4 + 1) * 4 * 2;
+        int mx = this.value.precision();
+        int mxb = that.value.precision();
 
-        MathContext mathContext = new MathContext(scale, getRoundingMode(context.runtime));
+        if (mx < mxb) mx = mxb;
+        mx = (mx + 1) * BASE_FIG;
+
+        MathContext mathContext = new MathContext(mx, getRoundingMode(context.runtime));
         return new RubyBigDecimal(context.runtime, this.value.divide(that.value, mathContext)).setResult();
     }
 
+    private static RubyBigDecimal div2Impl(ThreadContext context, RubyNumeric a, RubyNumeric b, final int ix) {
+        RubyBigDecimal thiz = getVpValue(context, a, true);
+        RubyBigDecimal that = getVpValue(context, b, true);
+
+        if (thiz.isNaN() || that.isNaN()) return newNaN(context.runtime);
+
+        RubyBigDecimal div = thiz.divSpecialCases(context, that);
+        if (div != null) return div;
+
+        int mx = thiz.value.precision() + that.value.precision() + 2;
+
+        MathContext mathContext = new MathContext((mx * 2 + 2) * BASE_FIG, getRoundingMode(context.runtime));
+        return new RubyBigDecimal(context.runtime, thiz.value.divide(that.value, mathContext)).setResult(ix);
+    }
+
     @Deprecated
     public IRubyObject op_quo19(ThreadContext context, IRubyObject other) {
         return op_quo(context, other);
@@ -1450,7 +1465,7 @@ public RubyNumeric multiplyWith(ThreadContext context, RubyBignum value) {
     public IRubyObject divmod(ThreadContext context, IRubyObject other) {
         final Ruby runtime = context.runtime;
 
-        RubyBigDecimal val = getVpValue19(context, other, false);
+        RubyBigDecimal val = getVpValueWithPrec(context, other, false);
         if (val == null) return callCoerced(context, sites(context).divmod, other, true);
 
         if (isNaN() || val.isNaN() || isInfinity() && val.isInfinity()) return RubyArray.newArray(runtime, newNaN(runtime), newNaN(runtime));

diff --git a/test/jruby/test_big_decimal.rb b/test/jruby/test_big_decimal.rb
@@ -234,8 +234,8 @@ def teardown
   def test_big_decimal_mode
     # Accept valid arguments to #mode
     assert BigDecimal.mode(BigDecimal::EXCEPTION_OVERFLOW)
-    assert BigDecimal.mode(BigDecimal::EXCEPTION_OVERFLOW,true)
-    assert BigDecimal.mode(BigDecimal::EXCEPTION_OVERFLOW,false)
+    assert BigDecimal.mode(BigDecimal::EXCEPTION_OVERFLOW, true)
+    assert BigDecimal.mode(BigDecimal::EXCEPTION_OVERFLOW, false)
 
     # Reject invalid arguments to #mode
     assert_raises(TypeError) { BigDecimal.mode(true) } # first argument must be a Fixnum
@@ -388,6 +388,35 @@ def test_div_returns_integer_by_default
     assert_equal 999900009999000099990, res
   end
 
+  def test_div_mult_precision
+    small = BigDecimal("1E-99"); denom = BigDecimal('0.000000123')
+    res = small / denom
+    # MRI (2.5):
+    # 0.8130081300813008130081300813008130081300813008130081300813008130081300813008130081300813008130081e-92
+    puts "\n#{__method__} #{small} / #{denom} = #{res} (#{res.precs})" if $VERBOSE
+    assert res.to_s.length > 40, "not enough precision: #{res}" # in JRuby 9.1 only 20
+    assert_equal BigDecimal('0.81300813e-92'),  res.round(100)
+
+    val1 = BigDecimal('1'); val2 = BigDecimal('3')
+    res = val1 / val2
+    puts "\n#{__method__} #{val1} / #{val2} = #{res} (#{res.precs})" if $VERBOSE
+    assert 18 <= res.precs.first, "unexpected precision: #{res.precs.first}"
+    assert res.precs.first <= 20, "unexpected precision: #{res.precs.first}"
+    assert_equal '0.333333333333333333e0', res.to_s
+
+    val1 = BigDecimal("1.00000000000000000001"); val2 = Rational(1, 3)
+    res = val1 * val2
+    puts "\n#{__method__} #{val1} * #{val2} = #{res} (#{res.precs})" if $VERBOSE
+    assert res.to_s.start_with?('0.333333333333333333336666666666666666'), "invalid result: #{res.to_s}"
+    # MRI (2.5):
+    # 0.33333333333333333333666666666666666633333333333333333333e0
+
+    # test_div from MRI's suite :
+    assert_equal(BigDecimal('1486.868686869'), (BigDecimal('1472.0') / BigDecimal('0.99')).round(9))
+
+    assert_equal(4.124045235, (BigDecimal('0.9932') / (700 * BigDecimal('0.344045') / BigDecimal('1000.0'))).round(9))
+  end
+
   def test_zero_p # from MRI test_bigdecimal.rb
     # BigDecimal.mode(BigDecimal::EXCEPTION_INFINITY, false)
     # BigDecimal.mode(BigDecimal::EXCEPTION_NaN, false)
@@ -408,4 +437,69 @@ def test_power_of_three # from MRI test_bigdecimal.rb
     assert_in_delta(1.0/81, x ** -4)
   end
 
+  def test_div # from MRI test_bigdecimal.rb
+    x = BigDecimal((2**100).to_s)
+    assert_equal(BigDecimal((2**100 / 3).to_s), (x / 3).to_i)
+    assert_equal(BigDecimal::SIGN_POSITIVE_ZERO, (BigDecimal("0") / 1).sign)
+    assert_equal(BigDecimal::SIGN_NEGATIVE_ZERO, (BigDecimal("-0") / 1).sign)
+    assert_equal(2, BigDecimal("2") / 1)
+    assert_equal(-2, BigDecimal("2") / -1)
+
+    #assert_equal(BigDecimal('1486.868686869'), BigDecimal('1472.0') / BigDecimal('0.99'), '[ruby-core:59365] [#9316]')
+
+    #assert_equal(4.124045235, BigDecimal('0.9932') / (700 * BigDecimal('0.344045') / BigDecimal('1000.0')), '[#9305]')
+
+    BigDecimal.mode(BigDecimal::EXCEPTION_INFINITY, false)
+    assert_positive_zero(BigDecimal("1.0")  / BigDecimal("Infinity"))
+    assert_negative_zero(BigDecimal("-1.0") / BigDecimal("Infinity"))
+    assert_negative_zero(BigDecimal("1.0")  / BigDecimal("-Infinity"))
+    assert_positive_zero(BigDecimal("-1.0") / BigDecimal("-Infinity"))
+
+    # BigDecimal.mode(BigDecimal::EXCEPTION_INFINITY, true)
+    # BigDecimal.mode(BigDecimal::EXCEPTION_ZERODIVIDE, false)
+    # assert_raise_with_message(FloatDomainError, "Computation results to 'Infinity'") { BigDecimal("1") / 0 }
+    # assert_raise_with_message(FloatDomainError, "Computation results to '-Infinity'") { BigDecimal("-1") / 0 }
+  end
+
+  def test_new # from MRI test_bigdecimal.rb
+    assert_equal(1, BigDecimal("1"))
+    assert_equal(1, BigDecimal("1", 1))
+    assert_equal(1, BigDecimal(" 1 "))
+    assert_equal(111, BigDecimal("1_1_1_"))
+    assert_equal(10**(-1), BigDecimal("1E-1"), '#4825')
+
+    #assert_raise(ArgumentError, /"_1_1_1"/) { BigDecimal("_1_1_1") }
+
+    BigDecimal.mode(BigDecimal::EXCEPTION_OVERFLOW, false)
+    BigDecimal.mode(BigDecimal::EXCEPTION_NaN, false)
+    assert_positive_infinite(BigDecimal("Infinity"))
+    assert_negative_infinite(BigDecimal("-Infinity"))
+    assert_nan(BigDecimal("NaN"))
+    assert_positive_infinite(BigDecimal("1E1111111111111111111"))
+  end
+
+  private
+
+  def assert_nan(x)
+    assert(x.nan?, "Expected #{x.inspect} to be NaN")
+  end
+
+  def assert_positive_infinite(x)
+    assert(x.infinite?, "Expected #{x.inspect} to be positive infinite")
+    assert_operator(x, :>, 0)
+  end
+
+  def assert_negative_infinite(x)
+    assert(x.infinite?, "Expected #{x.inspect} to be negative infinite")
+    assert_operator(x, :<, 0)
+  end
+
+  def assert_positive_zero(x)
+    assert_equal(BigDecimal::SIGN_POSITIVE_ZERO, x.sign, "Expected #{x.inspect} to be positive zero")
+  end
+
+  def assert_negative_zero(x)
+    assert_equal(BigDecimal::SIGN_NEGATIVE_ZERO, x.sign, "Expected #{x.inspect} to be negative zero")
+  end
+
 end
diff --git a/test/mri/excludes/TestBigDecimal.rb b/test/mri/excludes/TestBigDecimal.rb
@@ -2,13 +2,12 @@
 
 exclude :test_BigMath_exp_under_gc_stress, "needs investigation"
 exclude :test_BigMath_log_under_gc_stress, "needs investigation"
-exclude :test_div, "needs investigation"
-exclude :test_exception_underflow, "needs investigation"
+exclude :test_div, "does not pass due precision differences (ported to test/jruby/test_big_decimal.rb)"
 
 exclude :test_global_new_with_invalid_string, "error change in 2.4?"
 exclude :test_limit, "needs investigation"
 exclude :test_marshal, "needs investigation"
-exclude :test_new, "stricter parsing in 2.4"
+exclude :test_new, "BigDecimal('_1_1_1') parses fine while in MRI raises"
 
 exclude :test_power_of_negative_infinity, "needs investigation"
 exclude :test_power_of_positive_infinity, "needs investigation"
@@ -22,6 +21,6 @@
 exclude :test_sqrt_bigdecimal, "needs investigation"
 exclude :test_thread_local_mode, "needs investigation"
 exclude :test_to_f, "needs investigation"
-exclude :test_to_special_string, "needs investigation"
+exclude :test_to_special_string, "these are really not that relevant - a bit weird behavior"
 exclude :test_zero_p, "200000000000000 is too big for Java int exponent"
 exclude :test_ctov, "needs investigation"