On Tue, 19 Nov 2024 14:44:40 GMT, fabioromano1 <d...@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