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