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