On Sat, 17 Aug 2024 12:16:03 GMT, Chen Liang <li...@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.
>
> 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`.
I have tested that `()A` will report an error. If length == 1, rely on
Wrapper.forBasicType to detect illegal input.
> 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)`?
Allocating a buffer will slow down, so I used a long, with each 8 bits storing
a parameter length, which can improve performance when the number of parameters
is <= 8.
> 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);
Now this version merges the loops, your suggestion doesn't handle paramIndex >=
8, and it won't be faster, the current version of skipOverFieldSignature
performs well enough.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1720769514
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1720452515
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1720768233
