On Sun, 4 Jun 2023 23:52:37 GMT, Chen Liang <li...@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
