Classfile API performance and caching in Constant API

[Brian Goetz]

I'm looking at the patch for 8294960, for example.  The patches there 
for juc.MethodTypeDesc (and Impl) are straightforward "make the 
implementation faster / less bootstrap sensitive" improvements.  Whereas 
the change in MethodHandleImpl is "replace ASM with Classfile API".

I would want for these kinds of changes to be separated, so each patch 
focuses either on internal optimizations (like the MTD/MDTI changes), or 
switching over some component to use the classfile API.  This makes 
things easier to review, but also, if we have to revert something, it 
doesn't revert a bunch of other stuff.

Anything that affects the *API* of the JUC should be in a small, focused 
patch, since it must go through API review and CSR.

> So, in general, what I want to accomplish in the Constant API is these:
> 1. Add ClassDesc.internalName() which returns a string suitable for
> CONSTANT_Class_info for better performance
> 2. Add caching to this internalName and MethodTypeDesc.descriptorString()
> 3. Add MethodTypeDesc.of(ClassDesc returnType) and
> MethodTypeDesc.of(ClassDesc returnType, Collection<ClassDesc>
> parameterTypes) for convenience
> 4. Fix the bug that MethodTypeDesc.of(ClassDesc, ClassDesc[]) can be
> mutated by input array
> 5. Move the storage of parameters in MethodTypeDesc from ClassDesc[]
> to List<ClassDesc> (using List.of immutable lists) to speed up
> parameterList()
> 6. Optimize MethodTypeDesc.resolveConstantDesc() to resolve from class
> descs (parsing raw descriptor strings take longer from my benchmarks)
> and reject more-than-255-slot descs more eagerly

1 and 3 are API changes.  These should go into an API-only patch (but 
its OK to combine these improvements in one patch.)

4 is a bugfix.

2, 5, 6 are optimizations.  Again, these can be grouped in one patch.

So the minimum number of patches here is probably 3, you could go more 
fine-grained if it is convenient but you don't have to. On item (2), I'd 
like to have a brief discussion on caching strategy, as there are often 
side concerns like cache growth, location, and configuration, etc.

Make sense?

On 4/21/2023 1:01 PM, - wrote:
> Sure. My patches for new Constant API and caching will not migrate
> existing parts using ASM or other libraries to Classfile API, if that
> is what you mean.
>
> In addition, I have another patch that provides general improvements
> to MethodTypeDeschttps://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/13186__;!!ACWV5N9M2RV99hQ!IOPurfW6F5BA49gUm6RzXP5IW5IaUVjRaiog0y4xoo9sfmVsDsR52GCaxvAm_ZN9ml4YTW0w_WNd4gz2tdLD6wsH$   while
> fixing its mutability bug,
> and wishes to add two new APIs MethodTypeDesc.of(ClassDesc returnType)
> and MethodTypeDesc.of(ClassDesc returnType, Collection<ClassDesc>
> parameterTypes) as well.
>
> So, in general, what I want to accomplish in the Constant API is these:
> 1. Add ClassDesc.internalName() which returns a string suitable for
> CONSTANT_Class_info for better performance
> 2. Add caching to this internalName and MethodTypeDesc.descriptorString()
> 3. Add MethodTypeDesc.of(ClassDesc returnType) and
> MethodTypeDesc.of(ClassDesc returnType, Collection<ClassDesc>
> parameterTypes) for convenience
> 4. Fix the bug that MethodTypeDesc.of(ClassDesc, ClassDesc[]) can be
> mutated by input array
> 5. Move the storage of parameters in MethodTypeDesc from ClassDesc[]
> to List<ClassDesc> (using List.of immutable lists) to speed up
> parameterList()
> 6. Optimize MethodTypeDesc.resolveConstantDesc() to resolve from class
> descs (parsing raw descriptor strings take longer from my benchmarks)
> and reject more-than-255-slot descs more eagerly
>
> Should I submit separate patches, one for each?
>
> Chen
>
> On Fri, Apr 21, 2023 at 11:29 AM Brian Goetz<brian.goetz at oracle.com>  wrote:
>> Can you separate the pure JDK implementation improvements (e.g., making j.u.c more efficient) from the "switch to using classfile API in X" into separate patches?
>>
>> On 4/21/2023 12:23 PM, - wrote:
>>
>> If there is no objection, I will submit a patch incorporating the
>> caching and internalName method to the Constant API, to make the
>> Classfile API more efficient.
>>
>> Chen
>>
>> On Wed, Apr 19, 2023 at 11:15 PM -<liangchenblue at gmail.com>  wrote:
>>
>> Hello,
>> Since Adam has revealed that Classfile API is somewhat hampered by
>> performance issues and added a few patches in
>> https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/12945/__;!!ACWV5N9M2RV99hQ!IOPurfW6F5BA49gUm6RzXP5IW5IaUVjRaiog0y4xoo9sfmVsDsR52GCaxvAm_ZN9ml4YTW0w_WNd4gz2tdlpj8Fs$   that performs caching,
>>
>> His patch focuses on two optimizations:
>> 1. Speed up construction of descriptor string for MethodTypeDesc
>> 2. Caching of ClassDesc symbol in ClassEntry (representing CONSTANT_Class_info)
>>
>> Inspired by and in addition to his patches, I decided to try and see
>> the extent of performance impact of repeated string computation and
>> allocations from:
>> 1. Creation of internal names from ClassDesc
>>      I added a temporary API, String internalName(), to ClassDesc. This
>> API is forward-compatible with Valhalla, that it returns a String
>> suitable for a CONSTANT_Class_info.
>> 2. Repeated descriptorString calls on MethodTypeDesc
>>      I added a simple caching logic, much like the existing one in
>> java.lang.invoke.MethodType.
>> In addition, if the internal name and descriptor string are already
>> available via factory methods, the cache fields are immediately
>> assigned as well.
>>
>> The results are promising: In jdk.classfile.Write benchmark
>> https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/master/test/micro/org/openjdk/bench/jdk/classfile/Write.java__;!!ACWV5N9M2RV99hQ!IOPurfW6F5BA49gUm6RzXP5IW5IaUVjRaiog0y4xoo9sfmVsDsR52GCaxvAm_ZN9ml4YTW0w_WNd4gz2tVrtRojt$  ,
>> The throughput with Classfile API increased by 1/3.
>>
>> Master (Patch ofhttps://bugs.openjdk.org/browse/JDK-8305669, since
>> otherwise cannot run JMH with make on Windows):
>> Benchmark                Mode  Cnt      Score     Error  Units
>> Write.asmStream         thrpt    5  44866.172 ± 144.049  ops/s
>> Write.jdkTree           thrpt    5  16282.041 ±  95.782  ops/s
>> Write.jdkTreePrimitive  thrpt    5  16352.972 ± 280.751  ops/s
>>
>> New (https://urldefense.com/v3/__https://github.com/liachmodded/jdk/commit/e33261ad197c33836d56b4b48acabc04880a780b__;!!ACWV5N9M2RV99hQ!IOPurfW6F5BA49gUm6RzXP5IW5IaUVjRaiog0y4xoo9sfmVsDsR52GCaxvAm_ZN9ml4YTW0w_WNd4gz2teqdvD5R$  )
>> Benchmark                Mode  Cnt      Score     Error  Units
>> Write.asmStream         thrpt    5  45695.225 ± 649.249  ops/s
>> Write.jdkTree           thrpt    5  22786.872 ± 256.329  ops/s
>> Write.jdkTreePrimitive  thrpt    5  22437.085 ± 397.090  ops/s
>>
>> As a result, I believe it may be worth to update the Constant API to
>> offer ClassDesc::internalName() and method type descriptor string
>> caching, to enable better performance in the Classfile API.
>>
>> Thanks for reading,
>> Chen Liang
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/classfile-api-dev/attachments/20230421/dcafd875/attachment.htm>