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