Optimize sun.invoke.util.BytecodeDescriptor.unparse

[Roger Riggs]

Pushed as     8252127: Optimize sun.invoke.util.BytecodeDescriptor.unparse

https://bugs.openjdk.java.net/browse/JDK-8252127

Thanks Christoph

On 8/20/20 4:15 PM, Roger Riggs wrote:
> Hi Christoph,
>
> Looks good.
>
> Note that Claes added the cases for Object.class and int.class
> to maximize the performance of those common cases.
>
> I'll sponsor the change.
>
> Thanks, Roger
>
>
> On 8/20/20 2:01 PM, Christoph Dreis wrote:
>> Hi Roger,
>>
>> thanks for taking a look!
>>
>>> Though I wonder if performs differently than just calling  
>>> t.descriptorString()?
>> Seems pretty much to be the same.
>> The difference of 1 ns for the primitive case is a bit weird, but I 
>> guess at this levels it's also possible to have fluctuations here and 
>> there.
>>
>> Long.class
>> Benchmark                                          Mode  Cnt 
>> Score     Error   Units
>> MyBenchmark.unparseChristoph                       avgt   10 36,995 
>> ±   0,748   ns/op
>> MyBenchmark.unparseChristoph:·gc.alloc.rate.norm   avgt   10 168,009 
>> ±   0,001    B/op
>> MyBenchmark.unparseRoger                           avgt   10 36,857 
>> ±   1,472   ns/op
>> MyBenchmark.unparseRoger:·gc.alloc.rate.norm       avgt   10 168,009 
>> ±   0,001    B/op
>> MyBenchmark.unparseOld                             avgt   10 53,926 
>> ±   1,991   ns/op
>> MyBenchmark.unparseOld:·gc.alloc.rate.norm         avgt   10 256,014 
>> ±   0,001    B/op
>> long.class
>> Benchmark                                          Mode  Cnt 
>> Score     Error   Units
>> MyBenchmark.unparseChristoph                       avgt   10 5,184 
>> ±   0,168   ns/op
>> MyBenchmark.unparseChristoph:·gc.alloc.rate.norm   avgt   10 ≈ 
>> 10⁻⁶              B/op
>> MyBenchmark.unparseRoger                           avgt   10 6,149 
>> ±   0,238   ns/op
>> MyBenchmark.unparseRoger:·gc.alloc.rate.norm       avgt   10 ≈ 
>> 10⁻⁶              B/op
>> MyBenchmark.unparseOld                             avgt   10 11,236 
>> ±   0,464   ns/op
>> MyBenchmark.unparseOld:·gc.alloc.rate.norm         avgt   10 80,003 
>> ±   0,001    B/op
>>
>>> It should be equivalent, without extra checks.
>> It seems to be indeed equivalent, so I would change my proposal to 
>> the following.
>> I would keep the check for Object.class and int.class above as they 
>> seem to be the most common.
>> At least Object.class is good to have to avoid unnecessary String 
>> allocations in from descriptorString() imho.
>>
>> =========== PATCH ===============
>> --- 
>> a/src/java.base/share/classes/sun/invoke/util/BytecodeDescriptor.java 
>> Thu Aug 13 09:33:28 2020 -0700
>> +++ 
>> b/src/java.base/share/classes/sun/invoke/util/BytecodeDescriptor.java 
>> Thu Aug 20 19:44:57 2020 +0200
>> @@ -110,9 +110,7 @@
>>           } else if (type == int.class) {
>>               return "I";
>>           }
>> -        StringBuilder sb = new StringBuilder();
>> -        unparseSig(type, sb);
>> -        return sb.toString();
>> +        return type.descriptorString();
>>       }
>>
>> What do you think?
>>
>> Cheers,
>> Christoph
>>
>> On 8/13/20 1:31 PM, Christoph Dreis wrote:
>>> Hi,
>>>
>>> I just stumbled upon sun.invoke.util.BytecodeDescriptor.unparse that 
>>> seems to unnecessarily create a StringBuilder and checks for the 
>>> given type to be of Object.class twice in certain scenarios.
>>>
>>> When I apply the attached patch below with the following isolated 
>>> benchmark:
>>>
>>> @BenchmarkMode(Mode.AverageTime)
>>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>>> @State(Scope.Thread)
>>> public class MyBenchmark {
>>>
>>>     @State(Scope.Thread)
>>>     public static class BenchmarkState {
>>>         private Class<?> test = String.class; // long.class;
>>>     }
>>>
>>>     @Benchmark
>>>     public String unparseNew(BenchmarkState state) {
>>>         return BytecodeDescriptor.unparseNew(state.test);
>>>     }
>>>
>>>     @Benchmark
>>>     public String unparseOld(BenchmarkState state) {
>>>         return BytecodeDescriptor.unparseOld(state.test);
>>>     }
>>> }
>>>
>>> I get the following results:
>>> String.class
>>> Benchmark                                   Mode  Cnt Score     
>>> Error   Units
>>> MyBenchmark.unparseNew                      avgt   10 47,207 ±   
>>> 1,918   ns/op
>>> MyBenchmark.unparseNew:·gc.alloc.rate.norm  avgt   10 232,011 ±   
>>> 0,002    B/op
>>> MyBenchmark.unparseOld                      avgt   10 87,197 ±  
>>> 22,843   ns/op
>>> MyBenchmark.unparseOld:·gc.alloc.rate.norm  avgt   10 384,020 ±   
>>> 0,001    B/op
>>> long.class
>>> Benchmark                                   Mode  Cnt Score     
>>> Error   Units
>>> MyBenchmark.unparseNew                      avgt   10 4,996 ±    
>>> 0,022   ns/op
>>> MyBenchmark.unparseNew:·gc.alloc.rate.norm  avgt   10    ≈ 
>>> 10⁻⁶               B/op
>>> MyBenchmark.unparseOld                      avgt   10 13,303 ±    
>>> 6,305   ns/op
>>> MyBenchmark.unparseOld:·gc.alloc.rate.norm  avgt   10 80,003 ±    
>>> 0,001    B/op
>>>
>>> As you can see the new way makes things allocation free for 
>>> primitives and also improves normal classes.
>>>
>>> It seems like a relatively trivial improvement. In case you think 
>>> this is worthwhile, I would appreciate it if someone could sponsor 
>>> the change.
>>>
>>> Cheers,
>>> Christoph
>>>
>>> ======= PATCH =======
>>> --- 
>>> a/src/java.base/share/classes/sun/invoke/util/BytecodeDescriptor.java 
>>> Thu Aug 13 09:33:28 2020 -0700
>>> +++ 
>>> b/src/java.base/share/classes/sun/invoke/util/BytecodeDescriptor.java 
>>> Thu Aug 13 19:27:26 2020 +0200
>>> @@ -110,9 +110,13 @@
>>>            } else if (type == int.class) {
>>>                return "I";
>>>            }
>>> -        StringBuilder sb = new StringBuilder();
>>> -        unparseSig(type, sb);
>>> -        return sb.toString();
>>> +        Wrapper basicType = Wrapper.forBasicType(type);
>>> +        char c = basicType.basicTypeChar();
>>> +        if (c != 'L') {
>>> +            return basicType.basicTypeString();
>>> +        } else {
>>> +            return type.descriptorString();
>>> +        }
>>>        }
>>>
>>>
>>
>>
>