RFR: 8309413: General improvements to MethodTypeDesc implementation [v5]

[Mandy Chung]

On Sun, 4 Jun 2023 23:52:37 GMT, Chen Liang <liach at openjdk.org> wrote:

>> This patch aims to improve the performance of MethodTypeDesc in general, as it is currently a performance bottleneck in the Classfile API. A previous revision changed the parameter storage from an array to a list; this is dropped now. Sorry for the force push.
>> 
>> 
>> Benchmark                                                             (descString)  Mode  Cnt    Score   Error  Units
>> MethodTypeDescFactories.descriptorString   (Ljava/lang/Object;Ljava/lang/String;)I  avgt    6   27.778 ± 0.573  ns/op
>> MethodTypeDescFactories.descriptorString                                       ()V  avgt    6   13.343 ± 0.235  ns/op
>> MethodTypeDescFactories.descriptorString  ([IJLjava/lang/String;Z)Ljava/util/List;  avgt    6   40.828 ± 0.448  ns/op
>> MethodTypeDescFactories.descriptorString                     ()[Ljava/lang/String;  avgt    6   14.754 ± 0.162  ns/op
>> MethodTypeDescFactories.ofArray            (Ljava/lang/Object;Ljava/lang/String;)I  avgt    6    8.616 ± 0.132  ns/op
>> MethodTypeDescFactories.ofArray                                                ()V  avgt    6    2.146 ± 0.293  ns/op
>> MethodTypeDescFactories.ofArray           ([IJLjava/lang/String;Z)Ljava/util/List;  avgt    6   14.595 ± 0.235  ns/op
>> MethodTypeDescFactories.ofArray                              ()[Ljava/lang/String;  avgt    6    2.064 ± 0.085  ns/op
>> MethodTypeDescFactories.ofDescriptor       (Ljava/lang/Object;Ljava/lang/String;)I  avgt    6   97.077 ± 2.482  ns/op
>> MethodTypeDescFactories.ofDescriptor                                           ()V  avgt    6   13.563 ± 0.111  ns/op
>> MethodTypeDescFactories.ofDescriptor      ([IJLjava/lang/String;Z)Ljava/util/List;  avgt    6  130.543 ± 2.847  ns/op
>> MethodTypeDescFactories.ofDescriptor                         ()[Ljava/lang/String;  avgt    6   35.286 ± 0.260  ns/op
>> MethodTypeDescFactories.ofList             (Ljava/lang/Object;Ljava/lang/String;)I  avgt    6    4.156 ± 0.258  ns/op
>> MethodTypeDescFactories.ofList                                                 ()V  avgt    6    2.192 ± 0.063  ns/op
>> MethodTypeDescFactories.ofList            ([IJLjava/lang/String;Z)Ljava/util/List;  avgt    6   41.002 ± 0.235  ns/op
>> MethodTypeDescFactories.ofList                               ()[Ljava/lang/String;  avgt    6    2.200 ± 0.041  ns/op
>
> Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits:
> 
>  - Forgot to upload latest micro
>  - general optimizations to MTD, no longer change backing storage

Some comments in `MethodTypeDesc` and its implementation.  Will take a look at the test.

I'd like to see the ClassFile implementation changes in a separate PR that needs @asotona to give input and review.    The proposed change in the ClassFile implementation is trivial but I can't really tell what the performance issue is (of course, I know it avoids the array allocation).

For ClassFile performance issues, it would be useful to have the understanding of the major issues that contribute to the performance overhead.   That would help consider the fixes and alternatives.

src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java line 99:

> 97:      */
> 98:     static MethodTypeDesc of(ClassDesc returnDesc, ClassDesc... paramDescs) {
> 99:         if (paramDescs.length == 0)

`paramDescs` could be null.

src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java line 205:

> 203:      */
> 204:     default String descriptorString() {
> 205:         var sj = new StringJoiner("", "(", ")" + returnType().descriptorString());

`MethodTypeDescImpl` is the only class implementing `MethodTypeDesc` and implements `descriptorString()`.    This default implementation is not needed.   Should the implementation be moved to `MethodTypeDescImpl::descriptorString`?

src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java line 84:

> 82:             paramTypes = new ClassDesc[paramCount];
> 83:             for (int i = 0; i < paramCount; i++) {
> 84:                 paramTypes[i] = validateParameter(ClassDesc.ofDescriptor(types.get(i + 1)));

It seems useful to have a static factory method to take a trusted copy of parameter types and it will validate the parameters before constructing `MethodTypeDescImpl` instance.   Parameter validation in one single place.    Several methods doing the validation can simply call this factory method and the code would be cleaner.

`insertParameterTypes` can call it as well and overhead of re-validating existing parameter types isn't a big issue.

test/jdk/java/lang/constant/MethodTypeDescTest.java line 64:

> 62:         assertEquals(r, MethodTypeDesc.of(r.returnType(), r.parameterList().stream().toArray(ClassDesc[]::new)));
> 63:         assertEquals(r, MethodTypeDesc.of(r.returnType(), IntStream.range(0, r.parameterCount())
> 64:                 .mapToObj(r::parameterType)

this reformatting may be accidental.  Please keep the original format.   Same applies to other reformatted lines in this file.

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

PR Review: https://git.openjdk.org/jdk/pull/13186#pullrequestreview-1463008651
PR Review Comment: https://git.openjdk.org/jdk/pull/13186#discussion_r1218356523
PR Review Comment: https://git.openjdk.org/jdk/pull/13186#discussion_r1218369052
PR Review Comment: https://git.openjdk.org/jdk/pull/13186#discussion_r1218377366
PR Review Comment: https://git.openjdk.org/jdk/pull/13186#discussion_r1218380489