On Tue, 13 May 2025 13:03:42 GMT, Johannes Graham <d...@openjdk.org> wrote:
>> Optimize `BigDecimal.valueOf(double)` by using `FormattedFPDecimal` instead
>> of converting to decimal string and then parsing it. This results in an
>> approximate 6x improvement for me.
>
> Johannes Graham has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fix code tag in javadoc
src/java.base/share/classes/java/math/BigDecimal.java line 1408:
> 1406: }
> 1407:
> 1408: return valueOf(s, fmt.getScale(), fmt.getPrecision());
I'd prefer to have a `getExp()` rather than `getScale()` in
`FormattedFPDecimal`.
src/java.base/share/classes/jdk/internal/math/FormattedFPDecimal.java line 100:
> 98: // Keep digits to left of decimal, plus leave a
> 99: // trailing zero
> 100: (expR + 1) + 1;
I think that the comment is sufficiently clear to be able to replace `(expR +
1) + 1` with `expR + 2`
src/java.base/share/classes/jdk/internal/math/FormattedFPDecimal.java line 106:
> 104: // there is a single digit
> 105: 2;
> 106: };
Can we have a chain of `if`s or conditional expressions (`? :`)?
src/java.base/share/classes/jdk/internal/math/FormattedFPDecimal.java line 129:
> 127:
> 128: return fd;
> 129: }
Please be consistent with the usage of `final` for local vars in the context of
a single method.
src/java.base/share/classes/jdk/internal/math/FormattedFPDecimal.java line 147:
> 145: public int getScale() {
> 146: return -e;
> 147: }
I'd prefer to have a `getExp()` rather than `getScale()` here, as everything
here uses an exponent rather than a scale.
Then there's no need for the JavaDoc comment.
test/jdk/java/math/BigDecimal/ValueOfDouble.java line 41:
> 39:
> 40: public class ValueOfDouble {
> 41: private static final String DIGITS = "1234567899123456789"; // Enough
> digits to fill a long
Suggestion:
private static final String DIGITS = "1234567000003456789"; // Enough
digits to fill a long
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25173#discussion_r2088553632
PR Review Comment: https://git.openjdk.org/jdk/pull/25173#discussion_r2088558336
PR Review Comment: https://git.openjdk.org/jdk/pull/25173#discussion_r2088561193
PR Review Comment: https://git.openjdk.org/jdk/pull/25173#discussion_r2088593281
PR Review Comment: https://git.openjdk.org/jdk/pull/25173#discussion_r2088563587
PR Review Comment: https://git.openjdk.org/jdk/pull/25173#discussion_r2088576102
