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

[Chen Liang]

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.

Also note on ofDescriptor's use at bootstrap: This can be completely avoided as there should be no need for this method.

I think I have diagnosed the cause of such calls: one is here https://github.com/liachmodded/jdk/commit/64fd147bddd085fa3caa437a2b25a3dda3bb517a, and there are a few others in `SwitchBootstraps`. I need to check if there are still any other bootstrap usages after these are eliminated.

https://github.com/wenshao/jdk/pull/10
Sent a patch to allow fast parsing of 1 more parameter

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

> 294:         // objectDesc appears a lot during the bootstrap process, so optimize it
> 295:         String objectDesc = "Ljava/lang/Object;";
> 296:         if (len == objectDesc.length() && descriptor.regionMatches(start, objectDesc, 0, len)) {

Note that from my bytestack investigations, `regionMatches` can be CPU-intensive like hashCode calculation. Running this trick against `Ljava/lang/String;` (same length) might cause a lot of misses and waste a lot of CPU time. Can you try on a case with many `Ljava/lang/String;` descriptors and see the results compared to the build without this special case?

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

> 302:             throw new IllegalArgumentException("Bad method descriptor: " + descriptor);
> 303:         ptypes.set(0, resolveClassDesc(descriptor, cur, rLen));
> 304:         return ptypes;

You can remove `parseMethodDescriptor` now

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

> 106:      */
> 107:     public static MethodTypeDescImpl ofDescriptor(String descriptor) {
> 108:         int rightBracket = descriptor.indexOf(')');

Would `lastIndexOf` be better for general purposes, since there's just one descriptor to the right?

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

> 109:         int len = descriptor.length() - rightBracket - 1;
> 110:         if (rightBracket <= 0 || descriptor.charAt(0) != '(' || len == 0
> 111:                 || len != ConstantUtils.skipOverFieldSignature(descriptor, rightBracket + 1, descriptor.length(), true)

I think this is the only place we still need this `true`; inlining the `V` return type check here (should be simple, we just need to check `len == 1 && descriptor.charAt(rightBracket + 1) == 'V'` and rejecting `V` in `skipOverFieldSignature`) might (or might not) help C2 inlining.

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

> 111:                 || (rightBracket = (descriptor.charAt(1) == ')' ? 1 : descriptor.lastIndexOf(')'))) <= 0
> 112:                 || (len = descriptor.length() - rightBracket - 1) == 0
> 113:                 || (len != 1 && len != ConstantUtils.skipOverFieldSignature(descriptor, rightBracket + 1, descriptor.length()))

Has this be tested against input values like `()A`? I would prefer to fail explicitly with `descriptor.charAt(rightBracket + 1) == 'V'` instead of using `len != 1` and relying on `Wrapper.forBasicType`.

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

> 123:         int paramCount = 0;
> 124:         for (int cur = start; cur < end; ) {
> 125:             int len = ConstantUtils.skipOverFieldSignature(descriptor, cur, end, false);

I recall skipping over signatures is the main reason descriptor parsing is slow. What is the benchmark result if you allocate a buffer array like `short[] offsets = new short[Math.min(end - start, 255)]` or a `BitSet offsets = new BitSet(end - start)`?

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

> 143:     }
> 144: 
> 145:     private static void badMethodDescriptor(String descriptor) {

We usually make such utilities return an exception that we can throw in control flow; this trick is that otherwise, compiler will still ask for a return value after this `badMethodDescriptor` call which can be annoying sometimes.

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

> 156:             }
> 157:             if (len == 0) {
> 158:                 len = ConstantUtils.skipOverFieldSignature(descriptor, cur, end);

This can use a simpler version of skip that doesn't validate, like:

int start = cur;
while (descriptor.charAt(cur) == '[') {
    cur++;
}
if (descriptor.charAt(cur++) == 'L') {
    cur = descriptor.indexOf(';', cur) + 1;
}
paramTypes[paramIndex++] = ConstantUtils.resolveClassDesc(descriptor, start, cur - start);

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

PR Comment: https://git.openjdk.org/jdk/pull/20611#issuecomment-2295266083
PR Comment: https://git.openjdk.org/jdk/pull/20611#issuecomment-2295529521
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1721097556
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1719841359
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1719838312
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1719842641
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1720768235
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1719836907
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1719849066
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1720767719