Optimize sun.invoke.util.BytecodeDescriptor.unparse

[Roger Riggs]

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();
>> +        }
>>        }
>>
>>
>
>