RFR: 8341402: BigDecimal's square root optimization [v20]

[Raffaello Giulietti]

On Tue, 19 Nov 2024 14:44:40 GMT, fabioromano1 <duke at openjdk.org> wrote:

>> After changing `BigInteger.sqrt()` algorithm, this can be also used to speed up `BigDecimal.sqrt()` implementation. Here is how I made it.
>> 
>> The main steps of the algorithm are as follows:
>> first argument reduce the value to an integer using the following relations:
>> 
>> x = y * 10 ^ exp
>> sqrt(x) = sqrt(y) * 10^(exp / 2) if exp is even
>> sqrt(x) = sqrt(y*10) * 10^((exp-1)/2) is exp is odd
>> 
>> Then use BigInteger.sqrt() on the reduced value to compute the numerical digits of the desired result.
>> 
>> Finally, scale back to the desired exponent range and perform any adjustment to get the preferred scale in the representation.
>
> fabioromano1 has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Correct remainder checking

src/java.base/share/classes/java/math/BigDecimal.java line 2200:

> 2198:                 // Adjust to requested precision and preferred
> 2199:                 // scale as appropriate.
> 2200:                 result = result.adjustToPreferredScale(preferredScale, mc.precision);

Suggestion:

                return result.adjustToPreferredScale(preferredScale, mc.precision);

The redundant `else` below can then be removed, resulting in less "indented" code.
But the current choice is OK as well if you like it better.

src/java.base/share/classes/java/math/BigDecimal.java line 2253:

> 2251: 
> 2252:                     case UP, CEILING -> {
> 2253:                         BigInteger[] sqrtRem = workingInt.sqrtAndRemainder();

Can't this follow the same logic as for the halfway cases, with just `sqrt()` and a `!workingInt.equals(sqrt.multiply(sqrt))` test? This would increase consistency.
Are there performance reasons to do it as in the current code?

src/java.base/share/classes/java/math/BigDecimal.java line 2260:

> 2258:                     }
> 2259: 
> 2260:                     default -> throw new AssertionError("Unexpected value for RoundingMode: " + mc.roundingMode);

I think `MatchException` might be better suited (with a `null` cause), but I'll crosscheck and let you know tomorrow.

src/java.base/share/classes/java/math/BigDecimal.java line 2274:

> 2272:             }
> 2273:             return result;
> 2274:         } else {

As above, this redundant `else` could be removed and the code below be un-indented a bit. Again, up to your preferences.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21301#discussion_r1848870665
PR Review Comment: https://git.openjdk.org/jdk/pull/21301#discussion_r1848871435
PR Review Comment: https://git.openjdk.org/jdk/pull/21301#discussion_r1848871866
PR Review Comment: https://git.openjdk.org/jdk/pull/21301#discussion_r1848872056