On Sat, 15 Jun 2024 13:27: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:
>
> 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
