RFR: 8338532: Speed up the ClassFile API MethodTypeDesc#ofDescriptor

[Claes Redestad]

On Fri, 16 Aug 2024 08:53:38 GMT, Shaojin Wen <duke at openjdk.org> wrote:

> The current implementation of ofDescriptor puts return type and parameter types together in an ArrayList, and then splits them into return type and array of parameter types. This ArrayList creation is unnecessary, considering most descriptors only have few parameter types.
> 
> By splitting return type and parameter types separately and scanning the descriptor first to get the number of parameters, we can just allocate an exact, trusted array for the resulting MethodTypeDesc without copy.

Yes, this is a good one. I observed this oddity when I worked on #18971 but had a long list of other improvements to work on and didn't get around to filing an RFE. 

Can you check if the pre-existing MethodTypeDescFactories microbenchmark is sufficient to verify your optimization here?

src/java.base/share/classes/jdk/internal/constant/ConstantUtils.java line 309:

> 307:     private static ClassDesc resolveClassDesc(String descriptor, int start, int len) {
> 308:         if (len == 1) {
> 309:             return Wrapper.forPrimitiveType(descriptor.charAt(start)).basicClassDescriptor();

This was already piggy-backing on a quite fast routine. If it's a clean, significant win then I'm fine with this (untangling `Wrapper` from this is probably good, all things considered), but then we should also clean up `Wrapper` to not carry some descriptors around.

src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java line 109:

> 107:     public static MethodTypeDescImpl ofDescriptor(String descriptor) {
> 108:         // Implicit null-check of descriptor
> 109:         List<ClassDesc> ptypes = ConstantUtils.parseMethodDescriptor(descriptor);

This might have been the only use of parseMethodDescriptor - so you can probably remove that method, making this patch a net clean-up in terms of lines of code.

src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java line 137:

> 135:         var returnType = resolveClassDesc(descriptor, rightBracket + 1, retTypeLength);
> 136:         if (length == 3 && returnType == CD_void) {
> 137:             return Constants.MTD_void;

Feels a bit like overfitting with quite limited data.

Could this use `ConstantDescs.MTD_void` instead or does that cause a bootstrap cycle?

test/micro/org/openjdk/bench/java/lang/classfile/MethodTypeDescBench.java line 23:

> 21:  * questions.
> 22:  */
> 23: package org.openjdk.bench.java.lang.classfile;

Wrong package as the code being tested is in java.lang.constant. Also there is already a related microbenchmark in org.openjdk.bench.java.lang.constant.MethodTypeDescFactories - can you check if that covers your needs?

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

PR Review: https://git.openjdk.org/jdk/pull/20611#pullrequestreview-2242492738
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1721063121
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1719707693
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1721061415
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1719709479