RFR: 8269665: Clean-up toString() methods of some primitive wrappers [v2]

[Claes Redestad]

On Sun, 4 Jul 2021 21:35:31 GMT, Сергей Цыпанов <github.com+10835776+stsypanov at openjdk.org> wrote:

>> As of JDK 17 some of primitive wrappers, e.g. `Long`, `Integer`, `Double` and `Float` in their implementations of `Object.toString()` delegate to own utility `toString(primitive)` methods.
>> 
>> Unlike those, `Boolean`, `Byte`, `Character` and `Short` just duplicate the contents of utility methods in implementations of `Object.toString()`.
>> 
>> Yet another issue is a tiny discrepancy in implementation related to `Byte` and `Short` (see the first):
>> 
>> public static String toString(byte b) {
>>     return Integer.toString((int)b, 10);
>> }
>> 
>> public String toString() {
>>     return Integer.toString((int)value);
>> }
>> 
>> Unlike in overriden method, In utility one they explicitly specify radix which can be skipped, as implementation of `Integer.toString(int,int)` has a fast-path for radix 10, ending in `Integer.toString(int)`. This simplification gives tiny improvement, see benchmark:
>> 
>> @BenchmarkMode(Mode.AverageTime)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>> public class ByteToStringBenchmark {
>>     @Benchmark
>>     public String byteToString() {
>>         return Byte.toString((byte) 1);
>>     }
>> }
>> 
>> Results:
>> 
>> before
>> 
>> Benchmark                                                            Mode  Cnt     Score     Error   Units
>> ByteToStringBenchmark.byteToString                                   avgt   30    11,648 ±   1,906   ns/op
>> ByteToStringBenchmark.byteToString:·gc.alloc.rate                    avgt   30  3288,576 ± 418,119  MB/sec
>> ByteToStringBenchmark.byteToString:·gc.alloc.rate.norm               avgt   30    48,001 ±   0,001    B/op
>> ByteToStringBenchmark.byteToString:·gc.churn.G1_Eden_Space           avgt   30  3301,804 ± 455,932  MB/sec
>> ByteToStringBenchmark.byteToString:·gc.churn.G1_Eden_Space.norm      avgt   30    48,158 ±   2,085    B/op
>> ByteToStringBenchmark.byteToString:·gc.churn.G1_Survivor_Space       avgt   30     0,004 ±   0,001  MB/sec
>> ByteToStringBenchmark.byteToString:·gc.churn.G1_Survivor_Space.norm  avgt   30    ≈ 10⁻⁴              B/op
>> ByteToStringBenchmark.byteToString:·gc.count                         avgt   30   202,000            counts
>> ByteToStringBenchmark.byteToString:·gc.time                          avgt   30   413,000                ms
>> 
>> after
>> 
>> Benchmark                                                            Mode  Cnt     Score     Error   Units
>> ByteToStringBenchmark.byteToString                                   avgt   30    10,016 ±   0,530   ns/op
>> ByteToStringBenchmark.byteToString:·gc.alloc.rate                    avgt   30  3673,700 ± 167,450  MB/sec
>> ByteToStringBenchmark.byteToString:·gc.alloc.rate.norm               avgt   30    48,001 ±   0,001    B/op
>> ByteToStringBenchmark.byteToString:·gc.churn.G1_Eden_Space           avgt   30  3662,406 ± 205,978  MB/sec
>> ByteToStringBenchmark.byteToString:·gc.churn.G1_Eden_Space.norm      avgt   30    47,870 ±   1,750    B/op
>> ByteToStringBenchmark.byteToString:·gc.churn.G1_Survivor_Space       avgt   30     0,004 ±   0,002  MB/sec
>> ByteToStringBenchmark.byteToString:·gc.churn.G1_Survivor_Space.norm  avgt   30    ≈ 10⁻⁴              B/op
>> ByteToStringBenchmark.byteToString:·gc.count                         avgt   30   224,000            counts
>> ByteToStringBenchmark.byteToString:·gc.time                          avgt   30   358,000                ms
>
> Сергей Цыпанов has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - 8269665: Update copy-right year
>  - 8269665: Reuse String.valueOf(boolean)

Changes requested by redestad (Reviewer).

src/java.base/share/classes/java/lang/Byte.java line 93:

> 91:      */
> 92:     public static String toString(byte b) {
> 93:         return Integer.toString(b);

This change makes sense given the evidence that the radix tests in `Integer.toString(int, int)` are not elided. Same for the equivalent change in `Short.java`.

src/java.base/share/classes/java/lang/Byte.java line 441:

> 439:     @Override
> 440:     public String toString() {
> 441:         return toString(value);

I'm a bit more skeptical about these changes that re-route from `Integer.toString(int)` to the local `Byte.toString(byte)` which will then call `Integer.toString(int)` anyhow. An extra indirection will likely not be seen having an effect in micros, but can mess things up due inlining heuristics and of course slightly hamper startup, so I would prefer leaving the code as is. Adding missing `@Override`s is fine, of course.

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

PR: https://git.openjdk.java.net/jdk/pull/4633