On Fri, 16 Aug 2024 08:53:38 GMT, Shaojin Wen <d...@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
