RFR: CODETOOLS-7902929: JMH generators fail for benchmark in unnamed package

[Aleksey Shipilev]

On Mon, 10 May 2021 06:43:55 GMT, Jason Zaugg <jzaugg at openjdk.org> wrote:

> Prior to the patch, the enclosed test failed with:
> 
> 
> $ mvn -Preflection test
> ...
> Running UnnamedPackageTest
> Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.004 sec <<< FAILURE! - in UnnamedPackageTest
> compileTest(UnnamedPackageTest)  Time elapsed: 0.004 sec  <<< FAILURE!
> java.lang.AssertionError: Annotation generator had thrown the exception.:
> java.lang.NullPointerException
> 	at org.junit.Assert.fail(Assert.java:89)
> 	at org.junit.Assert.assertTrue(Assert.java:42)
> 	at UnnamedPackageTest.compileTest(UnnamedPackageTest.java:55)
> 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> 	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> 	at java.lang.reflect.Method.invoke(Method.java:498)
> 	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
> 	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)

In addition to patch comments, there are asm processor test failures with this new test, see GHA checks. It should be fixed e.g. like this:


diff --git a/jmh-generator-asm/src/main/java/org/openjdk/jmh/generators/asm/ASMClassInfo.java b/jmh-generator-asm/src/main/java/org/openjdk/jmh/generators/asm/ASMClassInfo.java
index f435d353..51b0442d 100644
--- a/jmh-generator-asm/src/main/java/org/openjdk/jmh/generators/asm/ASMClassInfo.java
+++ b/jmh-generator-asm/src/main/java/org/openjdk/jmh/generators/asm/ASMClassInfo.java
@@ -79,11 +79,24 @@ class ASMClassInfo extends ClassVisitor implements ClassInfo {
         this.superName = superName;
         this.idName = name;
         this.access = access;
-        qualifiedName = name.replace("/", ".");
-        packageName = qualifiedName.substring(0, qualifiedName.lastIndexOf("."));
-        origQualifiedName = qualifiedName;
-        qualifiedName = qualifiedName.replace('$', '.');
-        this.name = qualifiedName.substring(qualifiedName.lastIndexOf(".") + 1);
+        this.qualifiedName = name.replace("/", ".");
+
+        int dotIndex = qualifiedName.lastIndexOf(".");
+        if (dotIndex != -1) {
+            packageName = qualifiedName.substring(0, dotIndex);
+        } else {
+            packageName = "";
+        }
+
+        this.origQualifiedName = qualifiedName;
+        this.qualifiedName = qualifiedName.replace('$', '.');
+
+        dotIndex = qualifiedName.lastIndexOf(".");
+        if (dotIndex != -1) {
+            this.name = qualifiedName.substring(dotIndex + 1);
+        } else {
+            this.name = qualifiedName;
+        }
     }
 
     @Override


You can reproduce this locally by running `mvn ... -Pasm`.

Changes requested by shade (Committer).

Mondays, eh? Assuming tests are passing now, you are good to go!

jmh-core-ct/src/test/java/UnnamedPackageTest.java line 45:

> 43:     public void test(S s) {
> 44: 
> 45:     }

For the matter of this test, we can leave only:


@Benchmark
public void test() {}


...right?

jmh-core-ct/src/test/java/org/openjdk/jmh/ct/CompileTest.java line 91:

> 89:     }
> 90: 
> 91:     public static boolean doTest(Class<?> klass, InMemoryGeneratorDestination destination) {

This visibility change is not required anymore.

jmh-generator-asm/src/main/java/org/openjdk/jmh/generators/asm/ASMClassInfo.java line 89:

> 87:         } else {
> 88:             packageName = "";
> 89:         }

This is not the same as the previous code. Previous code handles "inner" classes with `$` additionally. See my previous comment how this should be resolved. This is why GHA have new failures.

-------------

Changes requested by shade (Committer).

PR: https://git.openjdk.java.net/jmh/pull/38Marked as reviewed by shade (Committer).