On Wed, 3 Jan 2024 12:25:29 GMT, Adam Sotona <[email protected]> wrote:
>> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 737:
>>
>>> 735: private void generateMethod(ClassBuilder clb, ClassEntry
>>> className) {
>>> 736: var cp = clb.constantPool();
>>> 737: var desc = new StringJoiner("", "(", ")" +
>>> returnType.descriptorString());
>>
>> We should just use an MTD here; the MTD will be passed to StackCounter so we
>> don't have to recompute a MTD.
>>
>> The MT to MTD conversion shouldn't be too costly; the overhead probably
>> comes from Optional, which we have to wait until Valhalla, as proxy
>> generation is unlikely to be hot and compiled by C2.
>
> Original code has been significant in profiler.
>
>
> MethodTypeDesc desc = MethodTypeDesc.of(toClassDesc(returnType),
>
> Arrays.stream(parameterTypes).map(ProxyGenerator::toClassDesc).toArray(ClassDesc[]::new));
>
>
> 1. each `toClassDesc` builds `descriptorString` and parses/validates it while
> constructing `ClassDesc`
> 2. `Arrays.stream(...).map(...).toArray(...)` allocates an array
> 3. `MethodTypeDesc.of(...)` clones the array and iterates params to check for
> void
> 4. `desc.descriptorString()` then finally use the `StringJoiner`
>
> Optimized code only joins `descriptorString`, no validations, no streaming,
> no arrays, no cloning.
>
>
> I suggest this patch as this code is considered as performance critical.
> However we can go through `ClassDesc` and `MethodTypeDesc` if not performance
> critical or if the conversions would be optimized.
>
> For example better (trusted) paths from `MethodType` to
> `MethodTypeDescriptor` and from `Class` to `ClassDesc`, avoiding at least
> validations.
Turns out your approach to avoid MTD here is apparently useless;
`MethodTypeDesc` is still created for initializing the local tracker `topLocal`
in `DirectCodeBuilder`. In addition, `StackMapDecoder` also uses
`methodTypeSymbol` to compute the initial frame.
IMO we should just stay with MTD; the descriptor breakdown happens too often
and, from previous benchmarks, descriptor breakdown is actually slow (which
gives CF API a small edge over ASM here). But we can still replace the
`parameterList()` iteration with index-based iteration to avoid array copies.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17121#discussion_r1444049504
