Optimize sun.invoke.util.BytecodeDescriptor.unparse

[Christoph Dreis]

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