RFR: 8306889: Convert MethodTypeDescImpl parameter storage from array to immutable list [v4]

[Mandy Chung]

On Wed, 24 May 2023 02:29:09 GMT, Chen Liang <liach at openjdk.org> wrote:

>> This patch moves the parameters to an immutable list, to avoid allocations on `parameterList` as well. In addition, it fixes 8304932, the bug where if a caller keeps a reference to the array passed into `MethodTypeDesc.of`, the caller may mutate the Desc via the array and can create invalid MethodTypeDesc.
>> 
>> The latest benchmark results are available here: https://github.com/openjdk/jdk/pull/13186#issuecomment-1560378822
>> 
>> This patch has minor performance gains on `ofDescriptor` factory, even compared to Adam's patch that optimized `ofDescriptor` in #12945.
>> 
>> Benchmark of Oracle JDK 20: https://gist.github.com/683c6219e183cbc2b336224fc2c0d50a
>> Benchmark of this patch: https://gist.github.com/22be9371a2370fb4a7b44f1684750ec4
>> Benchmark of [asotona's patch](https://github.com/openjdk/jdk/pull/12945/files#diff-ac8e413d3e13532a2b0d34a90253c6ddd7a4f04082f792b9d076e9b5a33f2078): https://gist.github.com/eb98579c3b51cafae481049a95a78f80
>> 
>> [sotona vs this](https://jmh.morethan.io/?gists=eb98579c3b51cafae481049a95a78f80,22be9371a2370fb4a7b44f1684750ec4); [20 vs this](https://jmh.morethan.io/?gists=683c6219e183cbc2b336224fc2c0d50a,22be9371a2370fb4a7b44f1684750ec4); [20 vs sotona](https://jmh.morethan.io/?gists=683c6219e183cbc2b336224fc2c0d50a,eb98579c3b51cafae481049a95a78f80), for reference
>
> Chen Liang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   reuse immutable list to avoid array allocation

The microbenchmark shows the performance of using the `MethodTypeDesc` factory methods.  With [#13671](https://github.com/openjdk/jdk/pull/13671), `MethodTypeDesc` is cached and I wonder if this is no longer the bottleneck of ClassFile API performance.   

[JDK-8304932](https://bugs.openjdk.org/browse/JDK-8304932) is a bug that can simply be fixed by 

diff --git a/src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java b/src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java
index 738c4d68a43..ed23887c9ef 100644
--- a/src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java
+++ b/src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java
@@ -95,7 +95,7 @@ public sealed interface MethodTypeDesc
      * {@link ClassDesc} for {@code void}
      */
     static MethodTypeDesc of(ClassDesc returnDesc, ClassDesc... paramDescs) {
-        return new MethodTypeDescImpl(returnDesc, paramDescs);
+        return new MethodTypeDescImpl(returnDesc, paramDescs.clone());
     }
 
     /**
diff --git a/src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java b/src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java
index 4341c3fb56f..8586bfb5926 100644
--- a/src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java
+++ b/src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java
@@ -41,7 +41,7 @@ import static java.util.Objects.requireNonNull;
  */
 final class MethodTypeDescImpl implements MethodTypeDesc {
     private final ClassDesc returnType;
-    private final ClassDesc[] argTypes;
+    private final ClassDesc[] argTypes;     // trusted array
 
     /**
      * Constructs a {@linkplain MethodTypeDesc} with the specified return type
@@ -102,14 +102,14 @@ final class MethodTypeDescImpl implements MethodTypeDesc {
 
     @Override
     public MethodTypeDesc changeReturnType(ClassDesc returnType) {
-        return MethodTypeDesc.of(returnType, argTypes);
+        return new MethodTypeDescImpl(returnType, argTypes);
     }
 
     @Override
     public MethodTypeDesc changeParameterType(int index, ClassDesc paramType) {
         ClassDesc[] newArgs = argTypes.clone();
         newArgs[index] = paramType;
-        return MethodTypeDesc.of(returnType, newArgs);
+        return new MethodTypeDescImpl(returnType, newArgs);
     }
 
     @Override
@@ -120,7 +120,7 @@ final class MethodTypeDescImpl implements MethodTypeDesc {
         ClassDesc[] newArgs = new ClassDesc[argTypes.length - (end - start)];
         System.arraycopy(argTypes, 0, newArgs, 0, start);
         System.arraycopy(argTypes, end, newArgs, start, argTypes.length - end);
-        return MethodTypeDesc.of(returnType, newArgs);
+        return new MethodTypeDescImpl(returnType, newArgs);
     }
 
     @Override
@@ -131,7 +131,7 @@ final class MethodTypeDescImpl implements MethodTypeDesc {
         System.arraycopy(argTypes, 0, newArgs, 0, pos);
         System.arraycopy(paramTypes, 0, newArgs, pos, paramTypes.length);
         System.arraycopy(argTypes, pos, newArgs, pos+paramTypes.length, argTypes.length - pos);
-        return MethodTypeDesc.of(returnType, newArgs);
+        return new MethodTypeDescImpl(returnType, newArgs);
     }
 
```      

I won't object to keep `argTypes` in an immutable list instead of an array.  However, `MethodTypeDescImpl::ofDescriptor` has to build from the list of string parameters to an array or mutable list of parameter descriptors first anyway.  Other APIs to add/drop the parameter types also construct a new array/list of parameter types.   Keeping argTypes in a trusted array seems to be the simplest.    Converting `argTypes` to an immutable list seems to be an extra step to make it immutable - we need frozen arrays!!   

So I think we should only fix JDK-8304932.

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

PR Comment: https://git.openjdk.org/jdk/pull/13186#issuecomment-1564925561