On Mon, 17 Jun 2024 04:58:41 GMT, Shaojin Wen <d...@openjdk.org> wrote:
>> The current versions of FloatToDecimal and DoubleToDecimal allocate
>> additional objects. Reducing these allocations can improve the performance
>> of Float/Double.toString and AbstractStringBuilder's append(float/double).
>>
>> This patch is just a code refactoring to reduce object allocation, but does
>> not change the Float/Double to decimal algorithm.
>>
>> The following code comments the allocated objects to be removed.
>>
>>
>> class FloatToDecimal {
>> public static String toString(float v) {
>> // allocate object FloatToDecimal
>> return new FloatToDecimal().toDecimalString(v);
>> }
>>
>> public static Appendable appendTo(float v, Appendable app)
>> throws IOException {
>> // allocate object FloatToDecimal
>> return new FloatToDecimal().appendDecimalTo(v, app);
>> }
>>
>> private Appendable appendDecimalTo(float v, Appendable app)
>> throws IOException {
>> switch (toDecimal(v)) {
>> case NON_SPECIAL:
>> // allocate object char[]
>> char[] chars = new char[index + 1];
>> for (int i = 0; i < chars.length; ++i) {
>> chars[i] = (char) bytes[i];
>> }
>> if (app instanceof StringBuilder builder) {
>> return builder.append(chars);
>> }
>> if (app instanceof StringBuffer buffer) {
>> return buffer.append(chars);
>> }
>> for (char c : chars) {
>> app.append(c);
>> }
>> return app;
>> // ...
>> }
>> }
>> }
>>
>> class DoubleToDecimal {
>> public static String toString(double v) {
>> // allocate object DoubleToDecimal
>> return new DoubleToDecimal(false).toDecimalString(v);
>> }
>>
>> public static Appendable appendTo(double v, Appendable app)
>> throws IOException {
>> // allocate object DoubleToDecimal
>> return new DoubleToDecimal(false).appendDecimalTo(v, app);
>> }
>>
>> private Appendable appendDecimalTo(double v, Appendable app)
>> throws IOException {
>> switch (toDecimal(v, null)) {
>> case NON_SPECIAL:
>> // allocate object char[]
>> char[] chars = new char[index + 1];
>> for (int i = 0; i < chars.length; ++i) {
>> chars[i] = (char) bytes[i];
>> }
>> ...
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Utf16 case remove `append first utf16 char`
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 43:
> 41: public final class DoubleToDecimal extends ToDecimal {
> 42: /**
> 43: * Use LATIN1 encoding to process the input byte[] str
Suggestion:
* Use LATIN1 encoding to process the in-out byte[] str
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 49:
> 47:
> 48: /**
> 49: * Use UTF16 encoding to process the input byte[] str
Suggestion:
* Use UTF16 encoding to process the in-out byte[] str
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 175:
> 173: */
> 174: public int putDecimal(byte[] str, int index, double v) {
> 175: assert index >= 0 && index + MAX_CHARS <= length(str) : "Trusted
> caller missed bounds check";
Suggestion:
assert 0 <= index && index <= length(str) - MAX_CHARS : "Trusted caller
missed bounds check";
This avoids a potential overflow.
But if you want to enforce the check, you should probably avoid using `assert`
since assertions might be disabled.
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 236:
> 234: dk = -1;
> 235: }
> 236: return toDecimal(str, index, Q_MIN, t, dk, fd) - start;
I suggest restoring the original logic like so:
/* subnormal value */
return (t < C_TINY
? toDecimal(str, index, Q_MIN, 10 * t, -1, fd)
: toDecimal(str, index, Q_MIN, t, 0, fd)) - start;
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 452:
> 450: if (l != 0) {
> 451: put8Digits(str, index, l);
> 452: index += 8;
As discussed in `ToDecimal`, all `put...` methods should return the updated
`index`. Then this code can be rewritten as
Suggestion:
index = put8Digits(str, index, l);
so that the caller doesn't need to know how many characters were added.
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 460:
> 458: putChar(str, index++, 'E');
> 459: if (e < 0) {
> 460: putChar(str, index++, '-');
By the same logic as suggested above for `put8Digits`
Suggestion:
index = putChar(str, index, '-');
and other places as well.
src/java.base/share/classes/jdk/internal/math/FloatToDecimal.java line 41:
> 39: * This class exposes a method to render a {@code float} as a string.
> 40: */
> 41: public final class FloatToDecimal extends ToDecimal {
The review of this class is the same as for `DoubleToDecimal`, so I won't
repeat it here.
src/java.base/share/classes/jdk/internal/math/ToDecimal.java line 34:
> 32: import static java.lang.Math.multiplyHigh;
> 33:
> 34: sealed class ToDecimal permits DoubleToDecimal, FloatToDecimal {
I think this should better be
Suggestion:
abstract sealed class ToDecimal permits DoubleToDecimal, FloatToDecimal {
since we don't want/need to instantiate this class.
src/java.base/share/classes/jdk/internal/math/ToDecimal.java line 45:
> 43: static final int PLUS_INF = 0x300;
> 44: static final int MINUS_INF = 0x400;
> 45: static final int NAN = 0x500;
These constants could be like the original ones (0, 1, 2, ...).
This would make the switch over them a `tableswitch` rather than a
`lookupswitch` in bytecode. I'm not sure whether this would improve machine
code, however.
Of course, logic would need to be changed in some places (`<< 8`, `>>> 8`, etc.)
src/java.base/share/classes/jdk/internal/math/ToDecimal.java line 63:
> 61: }
> 62:
> 63: final void putChar(byte[] str, int index, int c) {
I think it would be better for all the `put...` methods here to return the
updated `index`, hence replacing the return type from `void` to `int`.
This would make usages easier, as there's no need to remember at the call sites
by how much the index must be incremented after the usage.
src/java.base/share/classes/jdk/internal/math/ToDecimal.java line 71:
> 69: }
> 70:
> 71: final void putCharsAt(byte[] str, int index, int c1, int c2) {
This method is used to put at least one digit. This means that the caller has
to remember to add a '0' explicitly, which is questionable.
I suggest removing this method and invoke the correct combination of `putChar`
and `putDigit` for more readable code.
src/java.base/share/classes/jdk/internal/math/ToDecimal.java line 165:
> 163: }
> 164:
> 165: int length(byte[] str) {
Suggestion:
final int length(byte[] str) {
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1652975219
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1652975321
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1652972922
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1652978056
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1652984938
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1652987069
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1652991581
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1652933181
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1652993152
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1652993281
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1652993363
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1652993591
