RFR 8057793 BigDecimal is no longer effectively immutable

[Robert Gibson]

Hi Brian, 
How are the performance characteristics after the patch? I see that a lot of effort went in to tuning last December under 8029501.
By the way, as part of my investigations into this bug I noticed that BigIntegerTest::divideLarge no longer tests the Burnikel-Ziegler part of the division code, since the heuristic to decide Knuth or BZ diverged from the algorithm to generate the test cases (as part of 8029501). 
Best,
Robert
  
On Saturday, 13 September 2014, 2:09, Brian Burkhalter <brian.burkhalter at oracle.com> wrote:
Hello,

I created a formal webrev:

Issue:    https://bugs.openjdk.java.net/browse/JDK-8057793
Webrev:    http://cr.openjdk.java.net/~bpb/8057793/webrev.00/

Based on manual inspection of the revised code the patch looks good to me. The test submitted with the issue now succeeds as do all regression tests in jdk/test/java/math to which I also added the code from the test case in the issue report.

Note that this webrev is with respect to JDK 9.

Thanks,

Brian




On Sep 11, 2014, at 6:35 PM, Joe Darcy <joe.darcy at oracle.com> wrote:

> Hello,
> 
> Hmmm. I haven't dived into the details of the code, but setScale calls out to divide functionality so it is plausible a bug in divide could cause a problem in setScale.
> 
> Thanks for the bug report,
> 
> -Joe
> 
> On 9/9/2014 1:30 AM, Robert Gibson wrote:
>> 
>> 
>> Hi there,
>> 
>> I came across a case in BigDecimal division where the dividend ends up getting mutated, which is rather strange for a seemingly immutable class! (It's a subset of the cases where the Burnikel-Ziegler algorithm is used, I haven't done the analysis to find out under which exact conditions it's triggered.)
>> 
>> The attached patch - against the JDK8 version - should fix the problem, at the cost of an extra array copy.  Could somebody review and/or comment please?
>> 
>> Thanks,
>> Robert
>> 
>> --- MutableBigInteger.java      2014-09-04 09:42:23.426815000 +0200
>> +++ MutableBigInteger.java.patched      2014-09-04 09:46:21.344132000 +0200
>> @@ -1261,19 +1261,20 @@
>>              int sigma = (int) Math.max(0, n32 - b.bitLength());   // step 3: sigma = max{T | (2^T)*B < beta^n}
>>              MutableBigInteger bShifted = new MutableBigInteger(b);
>>              bShifted.safeLeftShift(sigma);   // step 4a: shift b so its length is a multiple of n
>> -            safeLeftShift(sigma);     // step 4b: shift this by the same amount
>> +            MutableBigInteger aShifted = new MutableBigInteger (this);
>> +            aShifted.safeLeftShift(sigma);     // step 4b: shift a by the same amount
>> -            // step 5: t is the number of blocks needed to accommodate this plus one additional bit
>> -            int t = (int) ((bitLength()+n32) / n32);
>> +            // step 5: t is the number of blocks needed to accommodate a plus one additional bit
>> +            int t = (int) ((aShifted.bitLength()+n32) / n32);
>>              if (t < 2) {
>>                  t = 2;
>>              }
>> -            // step 6: conceptually split this into blocks a[t-1], ..., a[0]
>> -            MutableBigInteger a1 = getBlock(t-1, t, n);   // the most significant block of this
>> +            // step 6: conceptually split a into blocks a[t-1], ..., a[0]
>> +            MutableBigInteger a1 = aShifted.getBlock(t-1, t, n);   // the most significant block of a
>>              // step 7: z[t-2] = [a[t-1], a[t-2]]
>> -            MutableBigInteger z = getBlock(t-2, t, n);    // the second to most significant block
>> +            MutableBigInteger z = aShifted.getBlock(t-2, t, n);    // the second to most significant block
>>              z.addDisjoint(a1, n);   // z[t-2]
>>              // do schoolbook division on blocks, dividing 2-block numbers by 1-block numbers
>> @@ -1284,7 +1285,7 @@
>>                  ri = z.divide2n1n(bShifted, qi);
>>                  // step 8b: z = [ri, a[i-1]]
>> -                z = getBlock(i-1, t, n);   // a[i-1]
>> +                z = aShifted.getBlock(i-1, t, n);   // a[i-1]
>>                  z.addDisjoint(ri, n);
>>                  quotient.addShifted(qi, i*n);   // update q (part of step 9)
>>              }
>> @@ -1292,7 +1293,7 @@
>>              ri = z.divide2n1n(bShifted, qi);
>>              quotient.add(qi);
>> -           ri.rightShift(sigma);   // step 9: this and b were shifted, so shift back
>> +           ri.rightShift(sigma);   // step 9: a and b were shifted, so shift back
>>              return ri;
>>          }
>>      }
>