On Wed, 24 May 2023 02:29:09 GMT, Chen Liang <li...@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
