RFR: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal [v8]

[Claes Redestad]

On Sat, 15 Jun 2024 13:27:41 GMT, Shaojin Wen <duke at 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:
> 
>   code format

Just suggesting some improvements

src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 185:

> 183:      * Returns
> 184:      *     Combine type and size, the first byte is size, the second byte is type
> 185:      *

Suggestion:

     *  Returns size in the lower byte, type in the high byte, where type is

src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 190:

> 188:      *     PLUS_INF        iff v is POSITIVE_INFINITY
> 189:      *     MINUS_INF       iff v is NEGATIVE_INFINITY
> 190:      *     NAN             iff v is NaN

Suggestion:

     *     NAN             iff v is NaN
     *     otherwise NON_SPECIAL

src/java.base/share/classes/jdk/internal/math/ToDecimal.java line 169:

> 167:     }
> 168: 
> 169:     /* Using the deprecated constructor enhances performance */

Enhances performance over what, `new String(str, 0, index, StandardCharsets.US_ASCII)`? Please specify.

src/java.base/share/classes/jdk/internal/math/ToDecimal.java line 171:

> 169:     /* Using the deprecated constructor enhances performance */
> 170:     @SuppressWarnings("deprecation")
> 171:     static String charsToString(byte[] str, int index) {

This should be named something like `asciiBytesToString` now. Perhaps `ToDecimal.LATIN1` can be renamed `ASCII`, too.

test/micro/org/openjdk/bench/java/lang/StringBuilders.java line 244:

> 242: 
> 243:     @Benchmark
> 244:     public int appendWithFloat8Latin1() {

It would be interesting with a mixed micro to explore effects of increased number of implementors (looping over an array with `sbLatin1` and `sbUtf16`, and appending both floats and doubles in the same loop) 

`buf.delete(0, buf.length())` could be replaced with `buf.setLength(0)` in these. Might reduce a bit of overhead and focus micros .

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

Changes requested by redestad (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19730#pullrequestreview-2121584681
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1641984296
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1641984363
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1641979604
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1641979428
PR Review Comment: https://git.openjdk.org/jdk/pull/19730#discussion_r1641980537