<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<font size="4"><font face="monospace">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". <br>
<br>
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. <br>
<br>
Anything that affects the *API* of the JUC should be in a small,
focused patch, since it must go through API review and CSR. <br>
<br>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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</pre>
</blockquote>
<br>
1 and 3 are API changes. These should go into an API-only patch
(but its OK to combine these improvements in one patch.) <br>
<br>
4 is a bugfix.<br>
<br>
2, 5, 6 are optimizations. Again, these can be grouped in one
patch. <br>
<br>
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.<br>
<br>
Make sense?<br>
</font></font><br>
<div class="moz-cite-prefix">On 4/21/2023 1:01 PM, - wrote:<br>
</div>
<blockquote type="cite" cite="mid:CABe8uE09-iEmD7AfcRm-CBfo7h=t_NYas0zWfLj2twFr4vMbnw@mail.gmail.com">
<pre class="moz-quote-pre" wrap="">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 MethodTypeDesc <a class="moz-txt-link-freetext" href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/13186__;!!ACWV5N9M2RV99hQ!IOPurfW6F5BA49gUm6RzXP5IW5IaUVjRaiog0y4xoo9sfmVsDsR52GCaxvAm_ZN9ml4YTW0w_WNd4gz2tdLD6wsH$">https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/13186__;!!ACWV5N9M2RV99hQ!IOPurfW6F5BA49gUm6RzXP5IW5IaUVjRaiog0y4xoo9sfmVsDsR52GCaxvAm_ZN9ml4YTW0w_WNd4gz2tdLD6wsH$</a> 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 <a class="moz-txt-link-rfc2396E" href="mailto:brian.goetz@oracle.com"><brian.goetz@oracle.com></a> wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
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 - <a class="moz-txt-link-rfc2396E" href="mailto:liangchenblue@gmail.com"><liangchenblue@gmail.com></a> wrote:
Hello,
Since Adam has revealed that Classfile API is somewhat hampered by
performance issues and added a few patches in
<a class="moz-txt-link-freetext" href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/12945/__;!!ACWV5N9M2RV99hQ!IOPurfW6F5BA49gUm6RzXP5IW5IaUVjRaiog0y4xoo9sfmVsDsR52GCaxvAm_ZN9ml4YTW0w_WNd4gz2tdlpj8Fs$">https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/12945/__;!!ACWV5N9M2RV99hQ!IOPurfW6F5BA49gUm6RzXP5IW5IaUVjRaiog0y4xoo9sfmVsDsR52GCaxvAm_ZN9ml4YTW0w_WNd4gz2tdlpj8Fs$</a> 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
<a class="moz-txt-link-freetext" href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/master/test/micro/org/openjdk/bench/jdk/classfile/Write.java__;!!ACWV5N9M2RV99hQ!IOPurfW6F5BA49gUm6RzXP5IW5IaUVjRaiog0y4xoo9sfmVsDsR52GCaxvAm_ZN9ml4YTW0w_WNd4gz2tVrtRojt$">https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/master/test/micro/org/openjdk/bench/jdk/classfile/Write.java__;!!ACWV5N9M2RV99hQ!IOPurfW6F5BA49gUm6RzXP5IW5IaUVjRaiog0y4xoo9sfmVsDsR52GCaxvAm_ZN9ml4YTW0w_WNd4gz2tVrtRojt$</a> ,
The throughput with Classfile API increased by 1/3.
Master (Patch of <a class="moz-txt-link-freetext" href="https://bugs.openjdk.org/browse/JDK-8305669">https://bugs.openjdk.org/browse/JDK-8305669</a>, 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 (<a class="moz-txt-link-freetext" href="https://urldefense.com/v3/__https://github.com/liachmodded/jdk/commit/e33261ad197c33836d56b4b48acabc04880a780b__;!!ACWV5N9M2RV99hQ!IOPurfW6F5BA49gUm6RzXP5IW5IaUVjRaiog0y4xoo9sfmVsDsR52GCaxvAm_ZN9ml4YTW0w_WNd4gz2teqdvD5R$">https://urldefense.com/v3/__https://github.com/liachmodded/jdk/commit/e33261ad197c33836d56b4b48acabc04880a780b__;!!ACWV5N9M2RV99hQ!IOPurfW6F5BA49gUm6RzXP5IW5IaUVjRaiog0y4xoo9sfmVsDsR52GCaxvAm_ZN9ml4YTW0w_WNd4gz2teqdvD5R$</a> )
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
</pre>
</blockquote>
</blockquote>
<br>
</body>
</html>