Re: RFR: 8306889: Convert MethodTypeDescImpl parameter storage from array to immutable list [v4]
On Wed, 24 May 2023 02:29:09 GMT, Chen Liang 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 benefits of a list implementation over an array implementation are: 1. `parameterList()` is much faster, allowing more beautiful user code compared to manually unpacked iteration (immutable list factory doesn't allow using non-Object backing arrays) 2. `of(ClassDesc, List)` allows users to share immutable parameter lists than having to copy > With https://github.com/openjdk/jdk/pull/13671, MethodTypeDesc is cached and > I wonder if this is no longer the bottleneck of ClassFile API performance. The parsing is no more, but `descriptorString` is still a bottleneck. The master in the last benchmark already had 8306842 integrated. In fact, I don't think argType array sharing goes far enough: in these modification methods and ofDescriptor, we can skip the array/list iteration validation if we can ensure the change parts alone have not produced an invalid method type (insertion of void, slot changes). I would postpone such in-depth sharing into a later issue, where we track the slot count information as well (to preemptively reject invalid resolveConstantDesc calls) > Converting `argTypes` to an immutable list seems to be an extra step to make > it immutable - we need frozen arrays!! Unfortunately, this is currently the only way general users have access to frozen arrays, where index-based access may be constant-folded, as `@Stable` is not generally available. - PR Comment: https://git.openjdk.org/jdk/pull/13186#issuecomment-1565013833
Re: RFR: 8306889: Convert MethodTypeDescImpl parameter storage from array to immutable list [v4]
On Wed, 24 May 2023 02:29:09 GMT, Chen Liang 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 bu
Re: RFR: 8306889: Convert MethodTypeDescImpl parameter storage from array to immutable list [v4]
On Wed, 24 May 2023 02:29:09 GMT, Chen Liang 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 Now updated to the latest MethodTypeDesc.of factories, this patch is ready for review and integration. Below is a few performance information about this patch. This patch: Benchmark (descString) Mode Cnt Score Error Units MethodTypeDescDescriptor.computeDescriptorString (Ljava/lang/Object;Ljava/lang/String;)I thrpt6 39518.106 ± 525.077 ops/ms MethodTypeDescDescriptor.computeDescriptorString ()V thrpt6 62312.500 ± 579.585 ops/ms MethodTypeDescDescriptor.computeDescriptorString ([IJLjava/lang/String;Z)Ljava/util/List; thrpt6 21856.065 ± 678.237 ops/ms MethodTypeDescDescriptor.computeDescriptorString ()[Ljava/lang/String; thrpt6 57457.483 ± 1416.646 ops/ms Benchmark Mode Cnt Score Error Units RebuildMethodBodies.sharedthrpt4 21684.365 ± 941.586 ops/s RebuildMethodBodies.unshared thrpt4 15724.422 ± 153.420 ops/s master: Benchmark (descString) Mode Cnt Score Error Units MethodTypeDescDescriptorOld.computeDescriptorString (Ljava/lang/Object;Ljava/lang/String;)I thrpt6 6062.735 ± 61.852 ops/ms MethodTypeDescDescriptorOld.computeDescriptorString ()V thrpt6 8703.338 ± 205.429 ops/ms MethodTypeDescDescriptorOld.computeDescriptorString ([IJLjava/lang/String;Z)Ljava/util/List; thrpt6 5673.104 ± 62.810 ops/ms MethodTypeDescDescriptorOld.computeDescriptorString ()[Ljava/lang/String; thrpt6 8232.874 ± 161.060 ops/ms Benchmark Mode Cnt Score Error Units RebuildMethodBodies.sharedthrpt4 18317.288 ± 465.309 ops/s RebuildMethodBodies.unshared thrpt4 13744.541 ± 399.263 ops/s The Classfile API code generation and descriptor string computation speed up with this patch. Additional benchmarks for this patch: Benchmark (kind) Mode Cnt Score Error Units MethodTypeDescConstruct.ofArrayBench GENERIC thrpt6 203322.703 ± 5795.340 ops/ms MethodTypeDescConstruct.ofArrayBenchVOID thrpt6 397200.234 ± 7467.524 ops/ms MethodTypeDescConstruct.ofArrayBenchNO_PARAM thrpt6 398156.642 ± 7230.653 ops/ms MethodTypeDescConstruct.ofArrayBench ARBITRARY thrpt6 91751.039 ± 2451.052 ops/ms MethodTypeDescConstruct.ofDescriptorBenchGENERIC thrpt68184.264 ± 185.177 ops/ms MethodTypeDescConstruct.ofDescriptorBench VOID thrpt6 68775.908 ± 2471.949 ops/ms MethodTypeDescConstruct.ofDescriptorBench NO_PARAM thrpt6 28129.233 ± 454.477 ops/ms MethodTypeDescConstruct.ofDescriptorBench ARBITRARY thrpt67634.086 ± 333.202 ops/ms MethodTypeDescConstruct.ofListBench GENERIC thrpt6 311382.640 ± 15817.487 ops/ms MethodTypeDescConstruct.ofListBench VOID thrpt6 371300.447 ± 2310.545 ops/ms MethodTypeDescConstruct.ofListBench NO_PARAM thrpt6 367942.613 ± 10068.395 ops/ms MethodTypeDescConstruct.ofListBenchARBITRARY thrpt6 199825.985 ± 5963.658 ops/ms Shows that immutable para
Re: RFR: 8306889: Convert MethodTypeDescImpl parameter storage from array to immutable list [v4]
> 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. > > 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 - Changes: - all: https://git.openjdk.org/jdk/pull/13186/files - new: https://git.openjdk.org/jdk/pull/13186/files/6044b7ee..efc8018c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13186&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13186&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13186.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13186/head:pull/13186 PR: https://git.openjdk.org/jdk/pull/13186
