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