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

[Raffaello Giulietti]

On Fri, 15 Nov 2024 14:21:21 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:
> 
>   Optimize sqrt branch for exact results

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

> 2246:                             }
> 2247:                         }
> 2248:                     } else { // mc.roundingMode == RoundingMode.UP || mc.roundingMode == RoundingMode.CEILING

It would be more robust to make all enum constant explicit, and reserve the `else` to throw a `RuntimeException` of some sort, e.g., `MatchException`. Should an additional enum constant be added to `RoundingMode` in the future, this code will then throw instead of silently producing a wrong result.

If possible at all, it would be preferable to use a `switch` rather than `if`s.

src/java.base/share/classes/java/math/RoundingMode.java line 425:

> 423:             default -> false;
> 424:         };
> 425:     }

For this PR, it would be preferable to move this method as a private static in BigDecimal itself, even if its usefulness is more broad.

Also, I suggest making all the cases explicit, like so

Suggestion:

    private static boolean isHalfWay(RoundingMode m) {
        return switch (m) {
            case HALF_DOWN, HALF_UP, HALF_EVEN -> true;
            case FLOOR, CEILING, DOWN, UP, UNNECESSARY -> false;
        };
    }


In this way, should another enum constant be added in the future, the method will throw instead of silently returning a wrong value, indicating that a new case must be added to the switch for it to be exhaustive. (There's a hidden `default` that throws.)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21301#discussion_r1846658170
PR Review Comment: https://git.openjdk.org/jdk/pull/21301#discussion_r1846627164