Re: RFR: 8333893: Optimization for StringBuilder append boolean & null
On Mon, 10 Jun 2024 12:12:58 GMT, Shaojin Wen wrote: > After PR https://github.com/openjdk/jdk/pull/16245, C2 optimizes stores into > primitive arrays by combining values into larger stores. > > This PR rewrites the code of appendNull and append(boolean) methods so that > these two methods can be optimized by C2. You are right, I was thinking about the case where you have 2 short variables, you should combine them into a long explicitly for C2 to generate a 4-byte write, otherwise it would be 2 2-bytes. Omitted the constant part which already eliminates this restriction. - PR Comment: https://git.openjdk.org/jdk/pull/19626#issuecomment-2158743190
Re: RFR: 8333893: Optimization for StringBuilder append boolean & null
On Mon, 10 Jun 2024 12:12:58 GMT, Shaojin Wen wrote: > After PR https://github.com/openjdk/jdk/pull/16245, C2 optimizes stores into > primitive arrays by combining values into larger stores. > > This PR rewrites the code of appendNull and append(boolean) methods so that > these two methods can be optimized by C2. I think for that C2 JIT to work, we need to merge the `'t' 'r' 'u' 'e'` ascii bytes into an int constant. Same for `false`. - PR Comment: https://git.openjdk.org/jdk/pull/19626#issuecomment-2158504397
Re: RFR: 8333854: IllegalAccessError with proxies after JDK-8332457 [v2]
On Mon, 10 Jun 2024 04:08:41 GMT, Chen Liang wrote: >> Please review this patch that fixes a critical issue that breaks some Proxy >> usages. >> >> CONSTANT_Class and CONSTANT_MethodType must fail resolution for inaccessible >> package-private types per JVMS 5.4.3.1 and 5.4.3.5, yet such types can >> appear in method descriptors in the same class. The proposed way to bypass >> is to store the MethodType as its descriptor string in the bootstrap method >> arguments, and use MethodType.fromMethodDescriptorString to restore the >> arguments, which does not have this restriction and does not eagerly load >> the parameter classes. This case isn't covered by existing tests, so a new >> test has been added. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Obtain classloader in security manager friendly code path The condy bootstrap is not trusted by SecurityManager, therefore it cannot use the `null` ClassLoader to parse method descriptors. https://github.com/openjdk/jdk/blob/7b43a8cd7c663facbe490f889838d7ead0eba0f9/src/java.base/share/classes/java/lang/invoke/MethodType.java#L1193 Meanwhile, condy bootstrap cannot take MethodType or Class, as those constants cannot resolve a package private type appearing in method descriptors. Thus, my approach is to obtain a valid non-null ClassLoader in ``, which is called from `Proxy` so there's no SecurityManager problems. Then I use that ClassLoader to perform actual resolution lazily in the condy. The call to `getPlatformClassLoader` emulates the same call in `MethodType.fromMethodDescriptorString`, which resolves classes in the bootstrap loader. - PR Comment: https://git.openjdk.org/jdk/pull/19615#issuecomment-2158217420
Re: RFR: 8333854: IllegalAccessError with proxies after JDK-8332457 [v2]
> Please review this patch that fixes a critical issue that breaks some Proxy > usages. > > CONSTANT_Class and CONSTANT_MethodType must fail resolution for inaccessible > package-private types per JVMS 5.4.3.1 and 5.4.3.5, yet such types can appear > in method descriptors in the same class. The proposed way to bypass is to > store the MethodType as its descriptor string in the bootstrap method > arguments, and use MethodType.fromMethodDescriptorString to restore the > arguments, which does not have this restriction and does not eagerly load the > parameter classes. This case isn't covered by existing tests, so a new test > has been added. Chen Liang has updated the pull request incrementally with one additional commit since the last revision: Obtain classloader in security manager friendly code path - Changes: - all: https://git.openjdk.org/jdk/pull/19615/files - new: https://git.openjdk.org/jdk/pull/19615/files/8e8dcdd0..8448db2a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19615=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19615=00-01 Stats: 38 lines in 1 file changed: 34 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19615/head:pull/19615 PR: https://git.openjdk.org/jdk/pull/19615
Re: RFR: 8310843: Reimplement ByteArray and ByteArrayLittleEndian with Unsafe
On Mon, 10 Jun 2024 02:12:11 GMT, Glavo wrote: > Things have changed since https://github.com/openjdk/jdk/pull/14636 was > closed, so let me reopen it. > > https://github.com/openjdk/jdk/pull/15386 confirmed that `VarHandle` in BALE > caused a startup regression. In order to not have any more revert patches, it > makes sense to optimize BALE. > > The optimization of https://github.com/openjdk/jdk/pull/16245 allows the > traditional expression to have good performance, but BA and BALE save us from > having to copy these lengthy expressions everywhere. So it makes sense to > keep them. > > Now here's the question, should I rewrite this PR without `Unsafe`? I'll do > some research (e.g. does `Unsafe` have better performance during warmup?), > but I'd also like to take some advice. No matter if we use unsafe or not, keeping multi-byte write and read operations all to one class so we can update them altogether sounds like a good idea. The only risk is that some operations will happen in early startup, such as ClassFile processing, which prohibits some implementations like VarHandle. Also feel free to choose another title for this patch, I will update the JBS issue for you. - PR Comment: https://git.openjdk.org/jdk/pull/19616#issuecomment-2157121539
RFR: 8333854: IllegalAccessError with proxies after JDK-8332457
Please review this patch that fixes a critical issue that breaks some Proxy usages. CONSTANT_Class and CONSTANT_MethodType must fail resolution for inaccessible package-private types per JVMS 5.4.3.1 and 5.4.3.5, yet such types can appear in method descriptors in the same class. The proposed way to bypass is to store the MethodType as its descriptor string in the bootstrap method arguments, and use MethodType.fromMethodDescriptorString to restore the arguments, which does not have this restriction and does not eagerly load the parameter classes. This case isn't covered by existing tests, so a new test has been added. - Commit messages: - Minor cleanup - 8333854: IllegalAccessError with proxies after JDK-8332457 Changes: https://git.openjdk.org/jdk/pull/19615/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19615=00 Issue: https://bugs.openjdk.org/browse/JDK-8333854 Stats: 79 lines in 2 files changed: 70 ins; 2 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/19615.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19615/head:pull/19615 PR: https://git.openjdk.org/jdk/pull/19615
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v6]
On Sun, 9 Jun 2024 22:45:26 GMT, Shaojin Wen wrote: >> After PR #16245, C2 optimizes stores into primitive arrays by combining >> values into larger stores. In the UUID.toString method, >> ByteArrayLittleEndian can be removed, making the code more elegant and >> faster. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > better parameter names Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/19610#pullrequestreview-2106382827
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v3]
On Sat, 8 Jun 2024 23:30:38 GMT, Shaojin Wen wrote:
>> After PR #16245, C2 optimizes stores into primitive arrays by combining
>> values into larger stores. In the UUID.toString method,
>> ByteArrayLittleEndian can be removed, making the code more elegant and
>> faster.
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> change method name, putHex -> putHex4, and fix comments
src/java.base/share/classes/java/util/UUID.java line 1:
> 1: /*
We can update copyright year to 2024
src/java.base/share/classes/jdk/internal/util/HexDigits.java line 1:
> 1: /*
We can update copyright header to `Copyright (c) 2023, 2024, Oracle ...`
src/java.base/share/classes/jdk/internal/util/HexDigits.java line 118:
> 116: /**
> 117: * Insert the unsigned 2-byte integer into the buffer as 4
> hexadecimal digit ASCII bytes,
> 118: * {@code i} only least significant 16 bits are used.
Suggestion:
* only least significant 16 bits of {@code i} are used.
src/java.base/share/classes/jdk/internal/util/HexDigits.java line 123:
> 121: * @param i to convert
> 122: */
> 123: public static void putHex4(byte[] buffer, int off, int i) {
I recommend the name `put4`. `Hex` is redundant within the context of
`HexDigits`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19610#discussion_r1632187884
PR Review Comment: https://git.openjdk.org/jdk/pull/19610#discussion_r1632187984
PR Review Comment: https://git.openjdk.org/jdk/pull/19610#discussion_r1632187743
PR Review Comment: https://git.openjdk.org/jdk/pull/19610#discussion_r1632187666
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v2]
On Sat, 8 Jun 2024 15:51:13 GMT, Brett Okken wrote:
>> Shaojin Wen has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> add comments
>
> src/java.base/share/classes/jdk/internal/util/HexDigits.java line 117:
>
>> 115:
>> 116: /**
>> 117: * Insert the int into the buffer as 4 hexadecimal digits
>
> 4 hex digits is 2 bytes, right?
> should this take a short instead?
> At a minimum, seems documentation should be clear as to which 2 bytes are
> used.
Yes, 4 hex digits requires 2 bytes of input integer; we always take the least
significant bytes. I think taking an int is better than taking a short, as
subint types are int in bytecode, and short's signedness may be bug-prone.
> src/java.base/share/classes/jdk/internal/util/HexDigits.java line 122:
>
>> 120: * @param value to convert
>> 121: */
>> 122: public static void putHex(byte[] buffer, int off, int i) {
>
> Should there be 2 methods - for 2 and 4 bytes respectively?
> Does c2 optimize 8 byte writes as well?
The 4-byte unsigned int input for 8-byte write sounds plausible, I personally
am fine either with or without it.
> Does c2 optimize 8 byte writes as well?
>From the first few lines of #16245:
> Merging multiple consecutive small stores (e.g. 8 byte stores) into larger
> stores (e.g. one long store) can lead to speedup.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19610#discussion_r1632081700
PR Review Comment: https://git.openjdk.org/jdk/pull/19610#discussion_r1632082369
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v2]
On Sat, 8 Jun 2024 06:40:39 GMT, Shaojin Wen wrote: >> After PR #16245, C2 optimizes stores into primitive arrays by combining >> values into larger stores. In the UUID.toString method, >> ByteArrayLittleEndian can be removed, making the code more elegant and >> faster. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > add comments Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/19610#pullrequestreview-2105860441
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v2]
On Sat, 8 Jun 2024 06:40:39 GMT, Shaojin Wen wrote: >> After PR #16245, C2 optimizes stores into primitive arrays by combining >> values into larger stores. In the UUID.toString method, >> ByteArrayLittleEndian can be removed, making the code more elegant and >> faster. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > add comments And in addition, VarHandle is not initialized unless it's necessary; thus, programs that use UUIDs but not VarHandle no longer need to initialize VarHandle. See #15386 where JDK startup has a performance degradation because it had to initialize VarHandle after using BALE. - PR Comment: https://git.openjdk.org/jdk/pull/19610#issuecomment-2156044329
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v2]
On Sat, 8 Jun 2024 12:25:54 GMT, Sunmisc Unsafe wrote: >> Shaojin Wen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> add comments > > As far as I know, ByteArrayLittleEndian uses the VarHandle mechanism, which > more efficiently writes different primitives into the array, unlike the basic @sunmisc BALE uses byte array view VH which still uses Unsafe: https://github.com/openjdk/jdk/blob/8d2f9e57c3797c01c84df007f4d2bfdcd645d0c0/src/java.base/share/classes/java/lang/invoke/X-VarHandleByteArrayView.java.template#L141 Please take a look at #16245; you will see that C2 now JIT compiles these compatible "different primitives" like Unsafe would do, yet there's a bit of requirement on code shape. Thus I recommended the comment for wenshao, so future changes won't accidentally destroy the code shape and the optimization. - PR Comment: https://git.openjdk.org/jdk/pull/19610#issuecomment-2156036174
Re: RFR: 8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator [v2]
On Fri, 7 Jun 2024 18:58:36 GMT, Claes Redestad wrote: >> This PR refactors type matching in BootstrapMethodInvoker and adds a few >> types, seeking to improve bootstrap overheads of some ConstantBootstraps and >> in particular the ProxyGenerator condys generated for e.g. annotation >> proxies since [JDK-8332457](https://bugs.openjdk.org/browse/JDK-8332457) >> >> I've adjusted the micro-benchmark added by JDK-8332457 to not only generate >> a proxy but also call into one of the proxied methodt (`Object::hashCode`). >> >> Running org.openjdk.bench.java.lang.reflect.ProxyGenBench as a one-off >> startup benchmark sees significant improvement (-9% instructions, -6% >> cycles): >> >> Name Cnt Base ErrorTest >> Error Unit Change >> Perfstartup-JMH 20154,000 ±8,165 148,000 ± >> 23,164ms/op 1,04x (p = 0,352 ) >> :.cycles925335973,200 ± 47147600,262 842221278,800 ± >> 46836254,964 cycles 0,91x (p = 0,000*) >> :.instructions 2101588857,600 ± 81105850,361 1966307798,400 ± >> 22011043,908 instructions 0,94x (p = 0,000*) >> :.taskclock 291,500 ± 16,494 262,000 ± >> 15,328 ms 0,90x (p = 0,000*) >> * = significant >> >> Number of classes loaded drops from 1096 to 1092 >> >> Running the micro regularly shows no significant difference: >> >> Name Cnt Base ErrorTest Error Unit >> Change >> ProxyGenBench.generateAndProxy100 10 26,827 ± 8,954 26,855 ± 7,531 ms/op >> 1,00x (p = 0,991 ) >> * = significant > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Adress review comments, add ConstantBootstraps#invoke to list of recognized > type signatures I think Jorn is recommending to use invokeBasic with erased types as how LambdaForm does it. We can try that approach, but we need new tests to verify that bootstrap method type mismatches throw the same exceptions, so that should be another RFE. - Marked as reviewed by liach (Author). PR Review: https://git.openjdk.org/jdk/pull/19598#pullrequestreview-2105846859
Integrated: 8333749: Consolidate ConstantDesc conversion in java.base
On Thu, 6 Jun 2024 18:35:01 GMT, Chen Liang wrote: > In java.base, especially in bytecode generators, we have many different > methods converting known valid Class and MethodType into ClassDesc and > MethodTypeDesc. These conversions should be consolidated into the same > utility method for the ease of future maintenance. This pull request has now been integrated. Changeset: 8d2f9e57 Author: Chen Liang Committer: Claes Redestad URL: https://git.openjdk.org/jdk/commit/8d2f9e57c3797c01c84df007f4d2bfdcd645d0c0 Stats: 255 lines in 13 files changed: 113 ins; 47 del; 95 mod 8333749: Consolidate ConstantDesc conversion in java.base Co-authored-by: Claes Redestad Reviewed-by: redestad, jvernee - PR: https://git.openjdk.org/jdk/pull/19585
Re: RFR: 8327791: UUID toString removes the use of ByteArrayLittleEndian
On Sat, 8 Jun 2024 00:19:55 GMT, Shaojin Wen wrote:
> After PR #16245, C2 optimizes stores into primitive arrays by combining
> values into larger stores. In the UUID.toString method,
> ByteArrayLittleEndian can be removed, making the code more elegant and faster.
I think you mean bug ID 8333833, right?
src/java.base/share/classes/jdk/internal/util/HexDigits.java line 123:
> 121: */
> 122: public static void putHex(byte[] buffer, int off, int i) {
> 123: int v = (DIGITS[i & 0xff] << 16) | DIGITS[(i >> 8) & 0xff];
Looking at the comments in #16245, it's said that the int must be computed
beforehand so it will be a write of a 4-byte instead of 2 write of 2-bytes. Is
that understanding correct?
If my understanding is correct, I recommend to leave a comment explaining why
we compact the value into an int, like
// Prepare an int value so C2 generates a 4-byte write instead of two 2-byte
writes
-
PR Review: https://git.openjdk.org/jdk/pull/19610#pullrequestreview-2105776831
PR Review Comment: https://git.openjdk.org/jdk/pull/19610#discussion_r1631885732
Re: RFR: 8333828: Use value javadoc tag in java.lang.{Float, Double} [v2]
On Fri, 7 Jun 2024 22:16:36 GMT, Joe Darcy wrote: >> Use the value tag to make the javadoc for various format-related constants >> more informative to readers. Currently the information is available by >> following the "Constant Field Values" link. >> >> I'll reflow the paragraphs before a push. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Implement review feedback. Thanks for this simplification! - Marked as reviewed by liach (Author). PR Review: https://git.openjdk.org/jdk/pull/19607#pullrequestreview-2105458016
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v6]
On Fri, 7 Jun 2024 13:56:24 GMT, Chen Liang wrote: >> In java.base, especially in bytecode generators, we have many different >> methods converting known valid Class and MethodType into ClassDesc and >> MethodTypeDesc. These conversions should be consolidated into the same >> utility method for the ease of future maintenance. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > ExE-Boss review, go through validated path in Class Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/19585#issuecomment-2155630141
Re: RFR: 8333828: Use value javadoc tag in java.lang.{Float, Double}
On Fri, 7 Jun 2024 21:05:33 GMT, Joe Darcy wrote:
> Use the value tag to make the javadoc for various format-related constants
> more informative to readers. Currently the information is available by
> following the "Constant Field Values" link.
>
> I'll reflow the paragraphs before a push.
Since you are using the value tag on the field themselves to display their own
value, I believe a `{@value}` suffices. See the example in
java.lang.constant.ConstantDescs:
https://github.com/openjdk/jdk/blob/cf677c901e70d98404ec9cc3d75a93926e02fcd2/src/java.base/share/classes/java/lang/constant/ConstantDescs.java#L307
https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/constant/ConstantDescs.html#INIT_NAME
-
PR Comment: https://git.openjdk.org/jdk/pull/19607#issuecomment-2155627548
Re: RFR: 8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator [v2]
On Fri, 7 Jun 2024 18:47:53 GMT, Claes Redestad wrote:
>> Note that [`ConstantBootstraps::primitiveClass(Lookup, String, Class)`]
>> has a static return type of `Class<?>`, so the following is also needed:
>>
>> /**
>> * Exact type used of the {@link ConstantBootstraps#primitiveClass(Lookup,
>> String, Class)}
>> * bootstrap method.
>> */
>> private static final MethodType CONDY_CLASS_NO_ARG_MT
>> = CONDY_GET_STATIC_FINAL_MT.changeReturnType(Class.class);
>>
>>
>> [`ConstantBootstraps::primitiveClass(Lookup, String, Class)`]:
>> https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/invoke/ConstantBootstraps.html#primitiveClass(java.lang.invoke.MethodHandles.Lookup,java.lang.String,java.lang.Class)
>
> It's not the intent of this PR to exhaustively enumerate all methods in
> `ConstantBootstraps`, primarily those shown to be bootstrap sensitive in some
> app. I've so far never seen a use of `primitiveClass` (and I admit being
> ignorant as to why this even exists as a BSM) so I don't have any reason to
> believe special-casing it will carry its own weight.
For its existence, I think it was the same as null, that they weren't
representable by cp entries for bootstrap method args. They are now backed by
PrimitiveClassDescImpl, a type of condy, and ClassFile API currently writes
loadConstant(primitiveCd) as a condy.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19598#discussion_r1631606873
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v6]
On Fri, 7 Jun 2024 13:56:24 GMT, Chen Liang wrote: >> In java.base, especially in bytecode generators, we have many different >> methods converting known valid Class and MethodType into ClassDesc and >> MethodTypeDesc. These conversions should be consolidated into the same >> utility method for the ease of future maintenance. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > ExE-Boss review, go through validated path in Class No: they are missing checks against initial, trailing, or consecutive . or / separators. We can do that in a future patch, with proper test additions. - PR Comment: https://git.openjdk.org/jdk/pull/19585#issuecomment-2155386076
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v6]
> In java.base, especially in bytecode generators, we have many different > methods converting known valid Class and MethodType into ClassDesc and > MethodTypeDesc. These conversions should be consolidated into the same > utility method for the ease of future maintenance. Chen Liang has updated the pull request incrementally with one additional commit since the last revision: ExE-Boss review, go through validated path in Class - Changes: - all: https://git.openjdk.org/jdk/pull/19585/files - new: https://git.openjdk.org/jdk/pull/19585/files/e9ddc398..d3938b4b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19585=05 - incr: https://webrevs.openjdk.org/?repo=jdk=19585=04-05 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19585.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19585/head:pull/19585 PR: https://git.openjdk.org/jdk/pull/19585
Re: RFR: 8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator
On Fri, 7 Jun 2024 12:12:44 GMT, Claes Redestad wrote: > This PR refactors type matching in BootstrapMethodInvoker and adds a few > types, seeking to improve bootstrap overheads of some ConstantBootstraps and > in particular the ProxyGenerator condys generated for e.g. annotation proxies > since [JDK-8332457](https://bugs.openjdk.org/browse/JDK-8332457) > > I've adjusted the micro-benchmark added by JDK-8332457 to not only generate a > proxy but also call into one of the proxied methodt (`Object::hashCode`). > > Running org.openjdk.bench.java.lang.reflect.ProxyGenBench as a one-off > startup benchmark sees significant improvement (-9% instructions, -6% cycles): > > Name Cnt Base ErrorTest > Error Unit Change > Perfstartup-JMH 20154,000 ±8,165 148,000 ± > 23,164ms/op 1,04x (p = 0,352 ) > :.cycles925335973,200 ± 47147600,262 842221278,800 ± > 46836254,964 cycles 0,91x (p = 0,000*) > :.instructions 2101588857,600 ± 81105850,361 1966307798,400 ± > 22011043,908 instructions 0,94x (p = 0,000*) > :.taskclock 291,500 ± 16,494 262,000 ± > 15,328 ms 0,90x (p = 0,000*) > * = significant > > Number of classes loaded drops from 1096 to 1092 > > Running the micro regularly shows no significant difference: > > Name Cnt Base ErrorTest Error Unit > Change > ProxyGenBench.generateAndProxy100 10 26,827 ± 8,954 26,855 ± 7,531 ms/op > 1,00x (p = 0,991 ) > * = significant I recommend considering the `MethodHandle, Object[]` signature used by `ConstantBootstrap.invoke`: There had been previous proposals to use any method as a bootstrap method, and that API in particular can bridge that gap. src/java.base/share/classes/java/lang/invoke/BootstrapMethodInvoker.java line 289: > 287: * bootstrap method. > 288: */ > 289: private static final MethodType CONDY_GET_STATIC_FINAL_MT = > MethodType.methodType(Object.class, This type should just be called `CONDY_NO_ARG` as it's the most common form of condy that doesn't take any arg (`nullConstant` `primitiveClass` etc.) src/java.base/share/classes/java/lang/invoke/BootstrapMethodInvoker.java line 296: > 294: * bootstrap method. > 295: */ > 296: private static final MethodType CONDY_GET_STATIC_FINAL_CLASS_MT = > MethodType.methodType(Object.class, I recommend calling this `CONDY_EXTRA_CLASS`, as this is the type of `arrayVarHandle`. - PR Review: https://git.openjdk.org/jdk/pull/19598#pullrequestreview-2104617832 PR Review Comment: https://git.openjdk.org/jdk/pull/19598#discussion_r1631241317 PR Review Comment: https://git.openjdk.org/jdk/pull/19598#discussion_r1631250689
Re: RFR: 8333774: Avoid eagerly loading various EmptySpliterator classes [v2]
On Fri, 7 Jun 2024 12:38:24 GMT, Claes Redestad wrote: >> Trivially move a few private static finals to their respective source type >> to avoid eagerly loading a few small classes. > > Claes Redestad has updated the pull request incrementally with two additional > commits since the last revision: > > - revert typo > - Copyright Marked as reviewed by liach (Author). src/java.base/share/classes/java/util/Spliterators.java line 89: > 87: } > 88: > 89: Suggestion: Bikeshedding to the extreme (yes, I looked for why the deletion had one less line) - PR Review: https://git.openjdk.org/jdk/pull/19591#pullrequestreview-2104476389 PR Review Comment: https://git.openjdk.org/jdk/pull/19591#discussion_r1631160491
Re: RFR: 8333477: Delete extra empty spaces in Makefiles [v2]
On Fri, 7 Jun 2024 12:47:39 GMT, Erik Joelsson wrote: >> test/jdk/java/rmi/reliability/benchmark/bench/Makefile line 50: >> >>> 48: clean: >>> 49: rm -f *.class .classes >>> 50: >> >> Hmm, shouldn't this newline at EOF be kept? Asking @ all the people who've >> reviewed this so far, no need to change it just yet > > No, it's an extra newline. A file should end with a newline but one is enough. As confusing as they are, unfortunately GitHub UI does not render extra trailing newlines. This is the only one I could find with grepWin. - PR Review Comment: https://git.openjdk.org/jdk/pull/19537#discussion_r1631159094
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v5]
> In java.base, especially in bytecode generators, we have many different > methods converting known valid Class and MethodType into ClassDesc and > MethodTypeDesc. These conversions should be consolidated into the same > utility method for the ease of future maintenance. Chen Liang has updated the pull request incrementally with one additional commit since the last revision: One other place can use wrapperClassDescriptor - Changes: - all: https://git.openjdk.org/jdk/pull/19585/files - new: https://git.openjdk.org/jdk/pull/19585/files/69af091a..e9ddc398 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19585=04 - incr: https://webrevs.openjdk.org/?repo=jdk=19585=03-04 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19585.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19585/head:pull/19585 PR: https://git.openjdk.org/jdk/pull/19585
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v4]
> In java.base, especially in bytecode generators, we have many different > methods converting known valid Class and MethodType into ClassDesc and > MethodTypeDesc. These conversions should be consolidated into the same > utility method for the ease of future maintenance. Chen Liang has updated the pull request incrementally with one additional commit since the last revision: Wrapper.basic/wrapperClassDescriptor (#1) - Changes: - all: https://git.openjdk.org/jdk/pull/19585/files - new: https://git.openjdk.org/jdk/pull/19585/files/89bc5d8d..69af091a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19585=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19585=02-03 Stats: 49 lines in 4 files changed: 17 ins; 2 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/19585.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19585/head:pull/19585 PR: https://git.openjdk.org/jdk/pull/19585
Re: RFR: 8333774: Avoid eagerly loading various EmptySpliterator classes
On Fri, 7 Jun 2024 08:21:58 GMT, Claes Redestad wrote: > Trivially move a few private static finals to their respective source type to > avoid eagerly loading a few small classes. Remember to update the copyright year. - Marked as reviewed by liach (Author). PR Review: https://git.openjdk.org/jdk/pull/19591#pullrequestreview-2104267610
Re: RFR: 8333477: Delete extra empty spaces in Makefiles [v2]
On Fri, 7 Jun 2024 07:29:39 GMT, SendaoYan wrote: >> Hi all, >> This PR several extra empty spaces and extra empty lines in several >> Makefiles. It's trivial fix, no risk. >> >> Thanks. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > delete extra empty trailing blank line in > test/jdk/java/rmi/reliability/benchmark/bench/Makefile Thank you for the fix. - Marked as reviewed by liach (Author). PR Review: https://git.openjdk.org/jdk/pull/19537#pullrequestreview-2104241367
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v2]
On Thu, 6 Jun 2024 19:30:21 GMT, Jorn Vernee wrote:
>> Chen Liang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> mt -> md (desc)
>
> src/java.base/share/classes/jdk/internal/constant/ConstantUtils.java line 76:
>
>> 74: * type and parameter types can be described nominally.
>> 75: */
>> 76: public static MethodTypeDesc methodDesc(MethodType type) {
>
> Please name these methods `methodTypeDesc`, since we also have the `Method`
> type.
Thanks for the suggestion. Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630306158
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v3]
> In java.base, especially in bytecode generators, we have many different > methods converting known valid Class and MethodType into ClassDesc and > MethodTypeDesc. These conversions should be consolidated into the same > utility method for the ease of future maintenance. Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into cleanup/consolidate-todesc - Add referenceClassDesc, move ReferenceClassDescImpl.ofValidatedBinaryName to ConstantUtils.binaryNameToDesc - mt -> md (desc) - Missed license header - Consolidate class/mt to desc operations - Changes: - all: https://git.openjdk.org/jdk/pull/19585/files - new: https://git.openjdk.org/jdk/pull/19585/files/3e28306f..89bc5d8d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19585=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19585=01-02 Stats: 2350 lines in 73 files changed: 2189 ins; 26 del; 135 mod Patch: https://git.openjdk.org/jdk/pull/19585.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19585/head:pull/19585 PR: https://git.openjdk.org/jdk/pull/19585
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v2]
On Thu, 6 Jun 2024 19:09:36 GMT, Claes Redestad wrote: > There are some pre-existing places where we call > `ReferenceClassDescImpl.ofValidated` directly that could probably be switched > over to `ConstantUtils.classDesc` for slightly nicer code. Or - if it matters > - add a `referenceClassDesc` which avoids the `type.isPrimitive` test. I think the main advantage of `ofValidated` is that it avoids descriptor string computation from class objects and should be kept in performance-sensitive code. `referenceClassDesc` would be feasible for less performance-demanding code's initialization and known safe conversions (a few APIs in CF API expects non-primitive CDs already). But should its implementation be `ofValidated` or `ofValidatedBinaryName` (same question for ClassDesc)? We used `ofValidatedBinaryName` here instead of a `toClassDesc` (which calls `ofValidated` via `descriptorString`) - PR Comment: https://git.openjdk.org/jdk/pull/19585#issuecomment-2153281690
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v2]
On Thu, 6 Jun 2024 19:01:02 GMT, Claes Redestad wrote: >> Chen Liang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> mt -> md (desc) > > src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java > line 471: > >> 469: case Allocate allocate -> >> emitAllocBuffer(allocate); >> 470: case BoxAddress boxAddress -> >> emitBoxAddress(boxAddress); >> 471: case SegmentBase _ -> emitSegmentBase(); > > Seem a bit too far detached from the changes at hand for a drive-by code > cleanup? My uncontrollable urges to fix these minor IDE warnings... Please allow these fixes, as I have similar changes in ProxyGenerator as well. - PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630100819
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v2]
> In java.base, especially in bytecode generators, we have many different > methods converting known valid Class and MethodType into ClassDesc and > MethodTypeDesc. These conversions should be consolidated into the same > utility method for the ease of future maintenance. Chen Liang has updated the pull request incrementally with one additional commit since the last revision: mt -> md (desc) - Changes: - all: https://git.openjdk.org/jdk/pull/19585/files - new: https://git.openjdk.org/jdk/pull/19585/files/cb4f6d13..3e28306f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19585=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19585=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19585.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19585/head:pull/19585 PR: https://git.openjdk.org/jdk/pull/19585
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base
On Thu, 6 Jun 2024 18:56:51 GMT, Claes Redestad wrote: >> In java.base, especially in bytecode generators, we have many different >> methods converting known valid Class and MethodType into ClassDesc and >> MethodTypeDesc. These conversions should be consolidated into the same >> utility method for the ease of future maintenance. > > src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 469: > >> 467: // o instanceof Wrapped(float) >> 468: cb.aload(SELECTOR_OBJ); >> 469: >> cb.instanceOf(classDesc(Wrapper.forBasicType(classLabel) > > I have a patch somewhere to cache the wrapper class desc in > `sun.invoke.util.Wrapper`, both as a micro-optimization and to help > disambigutate the unfortunately named (my bad) `Wrapper::classDescriptor` > method. Maybe we can roll that into this..? Feel free, or you can get your patch merged first and then I base off yours if yours is ready. - PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630091038
Re: RFR: 8333377: Migrate Generic Signature parsing to ClassFile API [v2]
> Core reflection's generic signature parsing uses an ancient library with > outdated visitor pattern on a tree model and contains unnecessary > boilerplates. This is a duplication of ClassFile API's signature model. We > should just move to ClassFile API, which is more throughoutly tested as well. > > To ensure compatibility, new tests are added to ensure consistent behavior > when encountering malformed signatures or signatures with missing types. The > reflective objects have been preserved and the only change is that lazy > expansion now happens from CF objects, to reduce compatibility risks. Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 16 commits: - Merge branch 'master' of https://github.com/openjdk/jdk into feature/new-generic-info - Remove redundant try-catch in getEnclosingMethod/Constructor - Merge branch 'test/signature-error' into feature/new-generic-info - Fix everything - Fixxes - Stage - Stage new tests - Merge branch 'master' of https://github.com/openjdk/jdk into feature/new-generic-info - Merge branch 'master' of https://github.com/openjdk/jdk into feature/new-generic-info - Merge branch 'master' of https://github.com/openjdk/jdk into feature/new-generic-info - ... and 6 more: https://git.openjdk.org/jdk/compare/2a37764e...19ee8797 - Changes: https://git.openjdk.org/jdk/pull/19281/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19281=01 Stats: 4605 lines in 66 files changed: 1027 ins; 3483 del; 95 mod Patch: https://git.openjdk.org/jdk/pull/19281.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19281/head:pull/19281 PR: https://git.openjdk.org/jdk/pull/19281
RFR: 8333749: Consolidate ConstantDesc conversion in java.base
In java.base, especially in bytecode generators, we have many different methods converting known valid Class and MethodType into ClassDesc and MethodTypeDesc. These conversions should be consolidated into the same utility method for the ease of future maintenance. - Commit messages: - Missed license header - Consolidate class/mt to desc operations Changes: https://git.openjdk.org/jdk/pull/19585/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19585=00 Issue: https://bugs.openjdk.org/browse/JDK-8333749 Stats: 160 lines in 8 files changed: 71 ins; 31 del; 58 mod Patch: https://git.openjdk.org/jdk/pull/19585.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19585/head:pull/19585 PR: https://git.openjdk.org/jdk/pull/19585
Re: RFR: 8333477: Delete extra empty spaces in Makefiles
On Tue, 4 Jun 2024 07:47:46 GMT, SendaoYan wrote: > Hi all, > This PR several extra empty spaces and extra empty lines in several > Makefiles. It's trivial fix, no risk. > > Thanks. Changes requested by liach (Author). test/jdk/java/rmi/reliability/benchmark/bench/rmi/Makefile line 1: > 1: # This file change is dubious: 1. It does not have any trailing whitespace that can fail the skara checks. 2. If the duplicate blank lines in the end of this Makefile is indeed problematic (as fixed here), please fix the only other occasion in the JDK, which is the Makefile in the parent directory. (Checked with `\n$^\n$\Z` pattern in all Makefiles) Recommended actions: Either 1. Revert changes in this file; 2. Also update `test/jdk/java/rmi/reliability/benchmark/bench/Makefile` to remove the trailing blank line. - PR Review: https://git.openjdk.org/jdk/pull/19537#pullrequestreview-2102735910 PR Review Comment: https://git.openjdk.org/jdk/pull/19537#discussion_r1629981196
Re: RFR: 8332249: Micro-optimize Method.hashCode [v2]
On Thu, 6 Jun 2024 06:41:48 GMT, ExE Boss wrote: >> Sean Gwizdak has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains six additional >> commits since the last revision: >> >> - Remove trailing whitespace. >> - Move hashCode benchmark into the newly created MethodBenchmark file >> - Merge branch 'master' into method-hashcode-JDK-8332249 >> - Remove changes to JavaDoc per guidance. >> - Fix whitespace issues pointed by the bot >> - Micro-optimize Method.hashCode > > src/java.base/share/classes/java/lang/reflect/Method.java line 392: > >> 390: .hashCode(); >> 391: } >> 392: return hc; > > The `hash` field should probably somehow be shared with the `Method.root` > instance, so that it doesn’t need to be recomputed when different code gets a > `Method` reference. Currently the hashCode computation is quite cheap. I think we can consider this delegation if it gets more complex, say if the hash code now considers parameters. - PR Review Comment: https://git.openjdk.org/jdk/pull/19433#discussion_r1629365372
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v20]
On Wed, 5 Jun 2024 12:50:25 GMT, Adam Sotona wrote: >> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use >> classfile API for reflection proxy-generation. Actual implementation of >> `ProxyGenerator` is focused on performance, however it causes JDK bootstrap >> regressions. `ProxyGenerator.TEMPLATE` class model is statically created and >> each proxy class is transformed from the template. >> >> This patch is intended to examine plain proxy generation impact on >> performance and JDK bootstrap (vs proxy transformation from template). >> >> The generated proxy is migrated from static initialization to CONDY >> bootstrap. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > Update test/micro/org/openjdk/bench/java/lang/reflect/ProxyGenBench.java > > Co-authored-by: Chen Liang Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/19410#pullrequestreview-2099091898
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v19]
On Wed, 5 Jun 2024 12:00:25 GMT, Adam Sotona wrote: >> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use >> classfile API for reflection proxy-generation. Actual implementation of >> `ProxyGenerator` is focused on performance, however it causes JDK bootstrap >> regressions. `ProxyGenerator.TEMPLATE` class model is statically created and >> each proxy class is transformed from the template. >> >> This patch is intended to examine plain proxy generation impact on >> performance and JDK bootstrap (vs proxy transformation from template). >> >> The generated proxy is migrated from static initialization to CONDY >> bootstrap. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > An assortment of potential improvements > > Co-authored-by: Claes Redestad src/java.base/share/classes/java/lang/constant/ConstantDescs.java line 356: > 354: ClassDesc[] fullParamTypes = new ClassDesc[paramTypes.length + > prefixLen]; > 355: System.arraycopy(INDY_BOOTSTRAP_ARGS, 0, fullParamTypes, 0, > prefixLen); > 356: System.arraycopy(paramTypes, 0, fullParamTypes, prefixLen, > paramTypes.length); I'm thinking about creating a basic MethodTypeDesc like `INDY_BOOTSTRAP_TYPE = MethodTypeDesc.ofValidated(CD_Object, INDY_BOOTSTRAP_ARGS)`, and we derive bsm with INDY_BOOTSTRAP_TYPE .insertParameterTypes(INDY_BOOTSTRAP_TYPE.parameterCount(), paramTypes) .changeReturnType(returnType) which creates two MTD wrappers but they share the same parameter array test/micro/org/openjdk/bench/java/lang/reflect/ProxyGenBench.java line 2: > 1: /* > 2: * Copyright (c) 2014, 2024, Oracle and/or its affiliates. All rights > reserved. Suggestion: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. - PR Review Comment: https://git.openjdk.org/jdk/pull/19410#discussion_r1627683294 PR Review Comment: https://git.openjdk.org/jdk/pull/19410#discussion_r1627685503
Re: RFR: 8333477: Delete extra empty spaces in Makefiles
On Tue, 4 Jun 2024 07:47:46 GMT, SendaoYan wrote: > Hi all, > This PR several extra empty spaces and extra empty lines in several > Makefiles. It's trivial fix, no risk. > > Thanks. @altrisi As trivial and low-effort as this seems, this is actually fixing some technical debt for legacy Makefiles last changed before 8e7a855ee8f085cee080395058f79c8a75bfef40 when trailing whitespace checks applied to Makefiles. - PR Comment: https://git.openjdk.org/jdk/pull/19537#issuecomment-2148641237
Re: RFR: 8333462: Performance regression of new DecimalFormat() when compare to jdk11
On Tue, 4 Jun 2024 02:32:28 GMT, lingjun-cg wrote:
> Run the below benchmark test ,it show the average time of new
> DecimalFormat() increase 18% when compare to jdk 11.
>
> the result with jdk 11:
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
>
>
> the result with current jdk:
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>
>
>
> @BenchmarkMode(Mode.AverageTime)
> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @State(Scope.Thread)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> public class JmhDecimalFormat {
>
> @Setup(Level.Trial)
> public void setup() {
> }
>
> @Benchmark
> public void testNewOnly() throws InterruptedException {
> new DecimalFormat("#0.0");
> }
> }
>
>
> Compare the flame graph it shows the
> java.text.DecimalFormatSymbols#findNonFormatChar takes a significant time.
> After replacing the lambda implementation with a simple loop , it shows
> nearly the same performance as jdk 11.
>
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>
>
> [flame-graph-jdk11-jdk21.zip](https://github.com/user-attachments/files/15541764/flame-graph-jdk11-jdk21.zip)
Marked as reviewed by liach (Author).
-
PR Review: https://git.openjdk.org/jdk/pull/19534#pullrequestreview-2096229334
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v4]
On Tue, 4 Jun 2024 09:07:44 GMT, lingjun-cg wrote:
>> ### Performance regression of DecimalFormat.format
>> From the output of perf, we can see the hottest regions contain atomic
>> instructions. But when run with JDK 11, there is no such problem. The
>> reason is the removed biased locking.
>> The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself
>> contains many synchronized methods.
>> So I added support for some new methods that accept StringBuilder which is
>> lock-free.
>>
>> ### Benchmark testcase
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @State(Scope.Thread)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> public class JmhDecimalFormat {
>>
>> private DecimalFormat format;
>>
>> @Setup(Level.Trial)
>> public void setup() {
>> format = new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testNewAndFormat() throws InterruptedException {
>> new DecimalFormat("#0.0").format(9524234.1236457);
>> }
>>
>> @Benchmark
>> public void testNewOnly() throws InterruptedException {
>> new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testFormatOnly() throws InterruptedException {
>> format.format(9524234.1236457);
>> }
>> }
>>
>>
>> ### Test result
>> Current JDK before optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnly avgt 50 642.099 ? 1.253 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 989.307 ? 3.676 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>>
>>
>>
>> Current JDK after optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 351.499 ? 0.761 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 615.145 ? 2.478 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>>
>>
>> ### JDK 11
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 364.214 ? 1.191 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 658.699 ? 2.311 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
>
> lingjun-cg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 896: Performance regression of DecimalFormat.format
src/java.base/share/classes/java/text/NumberFormat.java line 350:
> 348: return result;
> 349:
> 350: return format(number, new StringBuffer(),
Instead of just calling the package-private method that cannot be implemented
by user subclasses, we can probably add a package-private method like `boolean
isInternalSubclass` to decide if we should call the Buffer or Builder version.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1625900669
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v4]
On Tue, 4 Jun 2024 09:07:44 GMT, lingjun-cg wrote:
>> ### Performance regression of DecimalFormat.format
>> From the output of perf, we can see the hottest regions contain atomic
>> instructions. But when run with JDK 11, there is no such problem. The
>> reason is the removed biased locking.
>> The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself
>> contains many synchronized methods.
>> So I added support for some new methods that accept StringBuilder which is
>> lock-free.
>>
>> ### Benchmark testcase
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @State(Scope.Thread)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> public class JmhDecimalFormat {
>>
>> private DecimalFormat format;
>>
>> @Setup(Level.Trial)
>> public void setup() {
>> format = new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testNewAndFormat() throws InterruptedException {
>> new DecimalFormat("#0.0").format(9524234.1236457);
>> }
>>
>> @Benchmark
>> public void testNewOnly() throws InterruptedException {
>> new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testFormatOnly() throws InterruptedException {
>> format.format(9524234.1236457);
>> }
>> }
>>
>>
>> ### Test result
>> Current JDK before optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnly avgt 50 642.099 ? 1.253 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 989.307 ? 3.676 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>>
>>
>>
>> Current JDK after optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 351.499 ? 0.761 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 615.145 ? 2.478 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>>
>>
>> ### JDK 11
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 364.214 ? 1.191 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 658.699 ? 2.311 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
>
> lingjun-cg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 896: Performance regression of DecimalFormat.format
src/java.base/share/classes/java/text/NumberFormat.java line 405:
> 403: StringBuilder toAppendTo,
> 404: FieldPosition pos) {
> 405: throw new UnsupportedOperationException("Subclasses should
> override this " +
NumberFormat supports user-created classes, as it is abstract and has a
protected constructor. This will cause exceptions in formatting users' custom
NumberFormat subclasses.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19513#discussion_r1625890872
Re: RFR: 8332547: Unloaded signature classes in DirectMethodHandles [v2]
On Mon, 3 Jun 2024 19:36:58 GMT, Vladimir Ivanov wrote: >> JVM routinely installs loader constraints for unloaded signature classes >> when method resolution takes place. MethodHandle resolution took a different >> route and eagerly resolves signature classes instead (see >> `java.lang.invoke.MemberName$Factory::resolve` and >> `sun.invoke.util.VerifyAccess::isTypeVisible` for details). >> >> There's a micro-optimization which bypasses eager resolution for `java.*` >> classes. The downside is that `java.*` signature classes can show up as >> unloaded. It manifests as inlining failures during JIT-compilation and may >> cause severe performance issues. >> >> Proposed fix removes the aforementioned special case logic during >> `MethodHandle` resolution. >> >> In some cases it may slow down `MethodHandle` construction a bit (e.g., when >> repeatedly constructing `DirectMethodHandle`s with lots of arguments), but >> `MethodHandle` construction step is not performance critical. >> >> Testing: hs-tier1 - hs-tier4 > > Vladimir Ivanov has updated the pull request incrementally with one > additional commit since the last revision: > > Renaming: isTypeVisible -> ensureTypeVisible Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/19319#pullrequestreview-2094960904
Re: RFR: 8333377: Migrate Generic Signature parsing to ClassFile API
On Fri, 17 May 2024 12:01:23 GMT, Chen Liang wrote: > Core reflection's generic signature parsing uses an ancient library with > outdated visitor pattern on a tree model and contains unnecessary > boilerplates. This is a duplication of ClassFile API's signature model. We > should just move to ClassFile API, which is more throughoutly tested as well. > > To ensure compatibility, new tests are added to ensure consistent behavior > when encountering malformed signatures or signatures with missing types. The > reflective objects have been preserved and the only change is that lazy > expansion now happens from CF objects, to reduce compatibility risks. Indeed, I put the JBS issue to target release 24. I believe given the potential behavioral changes, rushing into release 23 is a risky choice. (the 4 new tests can target 23 in a separate patch, but they require removal of a few redundant assertions in old generic code) - PR Comment: https://git.openjdk.org/jdk/pull/19281#issuecomment-2143205523
Re: RFR: 8333377: Migrate Generic Signature parsing to ClassFile API
On Sat, 18 May 2024 05:24:16 GMT, ExE Boss wrote: >> Core reflection's generic signature parsing uses an ancient library with >> outdated visitor pattern on a tree model and contains unnecessary >> boilerplates. This is a duplication of ClassFile API's signature model. We >> should just move to ClassFile API, which is more throughoutly tested as well. >> >> To ensure compatibility, new tests are added to ensure consistent behavior >> when encountering malformed signatures or signatures with missing types. The >> reflective objects have been preserved and the only change is that lazy >> expansion now happens from CF objects, to reduce compatibility risks. > > src/java.base/share/classes/java/lang/Class.java line 3487: > >> 3485: >> 3486: // Generic info repository; lazily initialized >> 3487: private transient volatile @Stable ClassGenericInfo genericInfo; > > I don’t think this field can be `@Stable`, as generic signatures can change > when a class gets redefined by **JVM TI**, just like > [simple][Class::getSimpleName()] and [canonical > names][Class::getCanonicalName()]. > https://github.com/openjdk/jdk/blob/8acdd2d7c8de17515b87815d54ce556237039406/src/java.base/share/classes/java/lang/Class.java#L3452-L3454 > > [Class::getSimpleName()]: > https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Class.html#getSimpleName() > [Class::getCanonicalName()]: > https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Class.html#getCanonicalName() Existing genericInfo is not part of ReflectionData. The migration of genericInfo into reflectionData should be done in a separate task. - PR Review Comment: https://git.openjdk.org/jdk/pull/19281#discussion_r1605781541
RFR: 8333377: Migrate Generic Signature parsing to ClassFile API
Core reflection's generic signature parsing uses an ancient library with outdated visitor pattern on a tree model and contains unnecessary boilerplates. This is a duplication of ClassFile API's signature model. We should just move to ClassFile API, which is more throughoutly tested as well. To ensure compatibility, new tests are added to ensure consistent behavior when encountering malformed signatures or signatures with missing types. The reflective objects have been preserved and the only change is that lazy expansion now happens from CF objects, to reduce compatibility risks. - Commit messages: - Remove redundant try-catch in getEnclosingMethod/Constructor - Merge branch 'test/signature-error' into feature/new-generic-info - Fix everything - Fixxes - Stage - Stage new tests - Merge branch 'master' of https://github.com/openjdk/jdk into feature/new-generic-info - Merge branch 'master' of https://github.com/openjdk/jdk into feature/new-generic-info - Merge branch 'master' of https://github.com/openjdk/jdk into feature/new-generic-info - Merge branch 'master' of https://github.com/openjdk/jdk into feature/new-generic-info - ... and 5 more: https://git.openjdk.org/jdk/compare/8aeada10...4110c13f Changes: https://git.openjdk.org/jdk/pull/19281/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19281=00 Issue: https://bugs.openjdk.org/browse/JDK-877 Stats: 4605 lines in 66 files changed: 1027 ins; 3483 del; 95 mod Patch: https://git.openjdk.org/jdk/pull/19281.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19281/head:pull/19281 PR: https://git.openjdk.org/jdk/pull/19281
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor} [v6]
On Fri, 31 May 2024 18:39:33 GMT, jengebr wrote: >> Improve `java/lang/reflect/Method.java` by eliminating needless cloning of >> Class[0] instances. This cloning is intended to prevent callers from >> changing array contents, but many Methods have zero exceptions or zero >> parameters, and returning the original `Class[0]` is sufficient. >> >> Results from the included JMH benchmark: >> Before: >> >> BenchmarkMode Cnt Score Error Units >> ConstructorBenchmark.getExceptionTypes avgt5 6.526 ± 0.183 ns/op >> ConstructorBenchmark.getExceptionTypesEmpty avgt5 5.803 ± 0.073 ns/op >> ConstructorBenchmark.getParameterTypes avgt5 6.521 ± 0.188 ns/op >> ConstructorBenchmark.getParameterTypesEmpty avgt5 5.747 ± 0.087 ns/op >> MethodBenchmark.getExceptionTypesavgt5 6.525 ± 0.163 ns/op >> MethodBenchmark.getExceptionTypesEmpty avgt5 5.783 ± 0.130 ns/op >> MethodBenchmark.getParameterTypesavgt5 6.518 ± 0.195 ns/op >> MethodBenchmark.getParameterTypesEmpty avgt5 5.742 ± 0.028 ns/op >> >> >> After: >> >> BenchmarkMode Cnt Score Error Units >> ConstructorBenchmark.getExceptionTypes avgt5 6.590 ± 0.124 ns/op >> ConstructorBenchmark.getExceptionTypesEmpty avgt5 1.351 ± 0.061 ns/op >> ConstructorBenchmark.getParameterTypes avgt5 6.651 ± 0.132 ns/op >> ConstructorBenchmark.getParameterTypesEmpty avgt5 1.353 ± 0.150 ns/op >> MethodBenchmark.getExceptionTypesavgt5 6.701 ± 0.151 ns/op >> MethodBenchmark.getExceptionTypesEmpty avgt5 1.422 ± 0.025 ns/op >> MethodBenchmark.getParameterTypesavgt5 6.629 ± 0.142 ns/op >> MethodBenchmark.getParameterTypesEmpty avgt5 1.273 ± 0.169 ns/op > > jengebr has updated the pull request incrementally with one additional commit > since the last revision: > > Fixing nits in benchmark Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/19327#pullrequestreview-2091537949
Re: RFR: 8332249: Micro-optimize Method.hashCode
On Tue, 28 May 2024 17:25:34 GMT, Sean Gwizdak wrote:
> Improve the speed of Method.hashCode by caching the hashcode on first use.
> I've seen an application where Method.hashCode is a hot path, and this is a
> fairly simple speedup. The memory overhead is low.
>
> This addresses issue
> [JDK-8332249](https://bugs.openjdk.org/browse/JDK-8332249).
>
> Before:
>
> Benchmark Mode Cnt Score Error Units
> # Intel Skylake
> MethodHashCode.benchmarkHashCode avgt5 1.843 ± 0.149 ns/op
> # Arm Neoverse N1
> MethodHashCode.benchmarkHashCode avgt5 2.363 ± 0.091 ns/op
>
>
>
> After:
>
>
> Benchmark Mode Cnt Score Error Units
> # Intel Skylake
> MethodHashCode.benchmarkHashCode avgt5 1.121 ± 1.189 ns/op
> # Arm Neoverse N1
> MethodHashCode.benchmarkHashCode avgt5 1.001 ± 0.001 ns/op
Also, have you considered micro-optimize Constructor.hashCode too? Given its
similar status to Method.
src/java.base/share/classes/java/lang/reflect/Method.java line 99:
> 97: private Method root;
> 98: // Hash code of this object
> 99: private int hash;
We can declare this field `@Stable`, like the `methodAccessor`.
src/java.base/share/classes/java/lang/reflect/Method.java line 388:
> 386: int hc = hash;
> 387:
> 388: if (hc == 0) {
Should we add an extra field `hashIsZero` like in `String` to avoid repeated
computation if the hash is 0?
-
PR Comment: https://git.openjdk.org/jdk/pull/19433#issuecomment-2137679451
PR Review Comment: https://git.openjdk.org/jdk/pull/19433#discussion_r1618110657
PR Review Comment: https://git.openjdk.org/jdk/pull/19433#discussion_r1618110354
Re: RFR: 8332249: Micro-optimize Method.hashCode
On Wed, 29 May 2024 15:03:15 GMT, Sean Gwizdak wrote: >> src/java.base/share/classes/java/lang/reflect/Method.java line 99: >> >>> 97: private Method root; >>> 98: // Hash code of this object >>> 99: private int hash; >> >> We can declare this field `@Stable`, like the `methodAccessor`. > > This was something that I tried and I observed a performance regression on > the ARM platform that I was testing. From my understanding, `@Stable` tells > the compiler to trust the contents of this field which is only given when the > field holder is a known constant. This is generally true for cases like > `Enums` but not usually for `Method`. > > > Benchmark Mode Cnt Score Error Units > # Intel Skylake > MethodHashCode.benchmarkHashCode avgt5 1.113 ± 1.146 ns/op > # Arm Neoverse N1 > MethodHashCode.benchmarkHashCode avgt5 3.204 ± 0.001 ns/op Interesting, don't know about hotspot internals so I can't diagnose the exact cause of this regression. - PR Review Comment: https://git.openjdk.org/jdk/pull/19433#discussion_r1619080123
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor} [v2]
On Fri, 31 May 2024 16:18:01 GMT, jengebr wrote:
>> Improve `java/lang/reflect/Method.java` by eliminating needless cloning of
>> Class[0] instances. This cloning is intended to prevent callers from
>> changing array contents, but smany Methods have zero exceptions or zero
>> parameters, and returning the original `Class[0]` is sufficient.
>
> jengebr has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Adding JMH benchmark
test/micro/org/openjdk/bench/java/lang/reflect/ExecutableParameterAndExceptionTypesBenchmark.java
line 69:
> 67:
> 68: @Benchmark
> 69: public void constructorParametersWithNoExceptions(Blackhole bh)
> throws Exception {
Wrong name. Maybe rename this to `constructorParametersEmpty` and the exception
ones to `constructorExceptionsEmpty`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1622668928
Re: Question on Lambda function
Hi Zhengyu,
This implementation is actually quite good in terms of performance and
cost. One improvement you can have is to replace the `invoke`
with `invokeExact` (and remember to call MethodHandle.asType() before
passing into your anonymous class).
Unfortunately, it will be somewhat slow due to lack of constant folding, as
this handle is not stored in a static final but just an instance final
field (without -XX:+TrustFinalNonStaticFields VM flag;
https://shipilev.net/jvm/anatomy-quarks/17-trust-nonstatic-final-fields/).
Therefore, it will be slower than Java 22 MethodHandleProxies (MHP), which
uses hidden classes that are automatically trusted.
Your simple implementation is already better than the existing
MethodHandleProxies (at least before 22). MHP uses Proxy, which needs to go
through InvocationHandler so it had an extra layer of slowness.
Feel free to ask if you have any questions.
Regards,
Chen
On Fri, May 31, 2024 at 9:03 AM Zhengyu Gu
wrote:
> Hi Chen,
>
>
>
> Do you see any pros and cons of wrapping a MethodHandle, e.g.
>
>
>
> new Function() {
>
> @Override
>
> public C apply(D t) {
>
> try {
>
>return (C) handle.invoke(t);
>
> } catch (Throwable e) {
>
>
>
> }
>
> }
>
>
>
>
>
> vs using MethodHandleProxies.asInterfaceInstance() ?
>
>
>
> I would really appreciate your insights.
>
>
>
> Thanks,
>
>
>
> -Zhengyu
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
> *From: *Zhengyu Gu
> *Date: *Wednesday, May 29, 2024 at 8:28 PM
> *To: *Chen Liang
> *Cc: *core-libs-dev@openjdk.org
> *Subject: *Re: Question on Lambda function
>
> Hi Chen,
>
>
>
> What is your usage pattern of these
> single-abstract-method implementations? Since it sounds like you are
>
> creating a lot of them, are you storing them in collections?
>
>
>
> Yes, we do have such usage patterns, e.g. stores methods as Function in
> hash table as handlers, etc.
>
>
>
>
>
> If you are keeping a lot of them in collection (say, as event handlers),
> you may try to use `MethodHandleProxies.asInterfaceInstance` as a temporary
> workaround on JDK 22 and higher (older version uses Proxy, which has
> horrible invocation performance).
>
>
>
> Thanks for the suggestion. We are currently at 17, I will investigate the
> library.
>
>
>
>
>
> Best,
>
>
>
> -Zhengyu
>
>
>
>
>
> If you are on older versions from 15 to 21, unfortunately you might have
> to write a hidden class for the same purpose or use an existing library.
> One library that might be useful is
> https://github.com/LanternPowered/Lmbda that effectively generates
> unloadable hidden classes, but its 3.x builds are not maven central so you
> have to build yourself.
>
>
>
> - Chen
>
>
>
> On Wed, May 29, 2024 at 3:35 PM Zhengyu Gu
> wrote:
>
> Hi Chen,
>
>
>
> Thanks for the insights.
>
>
>
> We did refactor our code to avoid using LambdaMetaFactory,metafactory()
> directly.
>
>
>
> With increasing use of Lambdas, in our applications and libraries, the
> metaspace impact becomes a concern. If current implementation (not able to
> unload unused Lambda classes) here to stay, we must come up with a coding
> guideline to avoid excessive creation of Lambda classes, any pointers or
> suggestions would be greatly appreciated.
>
>
>
> Best,
>
>
>
> -Zhengyu
>
>
>
> *From: *Chen Liang
> *Date: *Wednesday, May 29, 2024 at 2:43 PM
> *To: *Zhengyu Gu
> *Cc: *core-libs-dev@openjdk.org
> *Subject: *Re: Question on Lambda function
> *[External Email]*
>
>
> --
>
> Hi Gu,
>
> CallSite is specific to each invokedynamic instruction instead of each
> InvokeDynamic constant pool entry:
> https://docs.oracle.com/javase/specs/jvms/se22/html/jvms-6.html#jvms-6.5.invokedynamic
>
> And the linking is done by MethodHandleNatives.linkCallSite if you want to
> follow the Java implementation code.
>
> For why the lambda in the loop is constant, it's a feature from
> InnerClassLambdaMetafactory:
> https://github.com/openjdk/jdk/blob/c8eea59f508158075382079316cf0990116ff98e/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java#L236
>
> When the lambda is non-capturing, the bootstrap method
> LambdaMetafactory.metafactory will eagerly create a singleton instance and
> return this singleton in the indy instruction.
>
>
>
> Also, your metaspace pressure might be caused by the fact that Lambda
> classes (not instances) are no longer eagerly unloaded; see
> https://github.com/openjdk/jdk/pul
Re: RFR: 8333312: Incorrect since tags on new ClassReader and ConstantPool methods
On Fri, 31 May 2024 13:04:03 GMT, David M. Lloyd wrote: > The new method additions ClassReader.readEntryOrNull(int, Class) and > ConstantPool.entryByIndex(int, Class) have incorrect since tags; they should > be `@since 23`. Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/19501#pullrequestreview-2090804861
Re: RFR: 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull [v6]
On Thu, 30 May 2024 19:44:29 GMT, David M. Lloyd wrote: >> Chen Liang has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 10 additional >> commits since the last revision: >> >> - Add test to validate ClassReader behavior >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> feature/entry-by-type >> - Null-check the entry class eagerly, avoid returning null or throwing IAE >> - Remove redundant import >> - Use switch >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> feature/entry-by-type >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> feature/entry-by-type >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> feature/entry-by-type >> - Use constants, beautify code >> - 8332614: Type-checked ConstantPool.entryByIndex and >> ClassReader.readEntryOrNull > > src/java.base/share/classes/java/lang/classfile/ClassReader.java line 142: > >> 140: * @throws ConstantPoolException if the index is out of range of the >> 141: * constant pool size, or zero, or the entry is not of the >> given type >> 142: * @since 24 > > I just noticed that these are marked `@since 24`. Am I correct that this > should be `@since 23`? Thanks for pointing out, I was under the assumption that this patch might not come into 23. I will create an issue, and you can make a PR if you feel like contributing (doc-only changes can integrate before RDPs so no rush) - PR Review Comment: https://git.openjdk.org/jdk/pull/19330#discussion_r1621460803
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Wed, 22 May 2024 14:33:31 GMT, Aleksey Shipilev wrote:
>> Why can't you do something like this:
>>
>> return cloneArray ? ReflectionFactory.copyClasses(interfaces) : interfaces;
>>
>>
>> Alternatively, your proposal is a good one too; we can rename this
>> `getInterfaces(boolean)` to `getInterfacesShared()` (like
>> `getSharedEnumConstants()`) and move this copy operation to
>> `getInterfaces()` as you suggest.
>
> I really see no reason to try and save on re-use of this one-line method for
> _everything_. In fact, I do not quite see a very compelling reason to even
> have the utility method. Sharing the code between `Method` and `Constructor`
> is clean enough and provides a good balance between reuse and cleanliness.
> Everything else can have the copy of the method definition, if needed.
Alternatively, if a utility method is overkill, we can inline these to
`Executable`:
public Class[] getParameterTypes() {
var shared = getSharedParameterTypes();
return shared.length == 0 ? shared : shared.clone();
}
And the overrides in `Method` and `Constructor` will simply call super; the
declarations are kept to preserve the API documentation.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1610561896
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Wed, 22 May 2024 09:39:50 GMT, Aleksey Shipilev wrote:
>> Improve `java/lang/reflect/Method.java` by eliminating needless cloning of
>> Class[0] instances. This cloning is intended to prevent callers from
>> changing array contents, but smany Methods have zero exceptions or zero
>> parameters, and returning the original `Class[0]` is sufficient.
>
> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
> 249:
>
>> 247: * the original can safely be reused.
>> 248: */
>> 249: public Class[] copyClasses(Class[] classes) {
>
> There is no need to make this utility method non-static. This would also
> obviate the need for having an instance of `ReflectionFactory` to access it.
>
> Additionally, at the risk of more bikeshedding, I think this utility method
> is better suited in `AccessibleObject` base class, with package-private
> visibility. Putting the util methods in `ReflectionFactory` erodes this
> comment a bit:
>
>
> The methods in this class are extremely unsafe and can cause
> subversion of both the language and the verifier. For this reason,
> they are all instance methods, and access to the constructor of
> this factory is guarded by a security check, in similar style to
> {@link jdk.internal.misc.Unsafe}.
Can't this be used by class cloning interfaces too?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1609710019
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Wed, 22 May 2024 12:58:40 GMT, jengebr wrote: >> Can't this be used by class cloning interfaces too? > > @liach I see such an opportunity in `Proxy.getProxyConstructor`, is that what > you have in mind? No, I mean here: https://github.com/openjdk/jdk/blob/4f1a10f84bcfadef263a0890b6834ccd3d5bb52f/src/java.base/share/classes/java/lang/Class.java#L1329 (That's also why I suggest putting the utiltiy method in ReflectionFactory instead of AccessibleObject or Executable) `Proxy` is meaningless with an empty list of interfaces, as it's solely for implementing interfaces and delegating method calls to the given InvocationHandler. So I would ignore that scenario. - PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1609992490
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Wed, 22 May 2024 14:24:40 GMT, jengebr wrote: >> No, I mean here: >> https://github.com/openjdk/jdk/blob/4f1a10f84bcfadef263a0890b6834ccd3d5bb52f/src/java.base/share/classes/java/lang/Class.java#L1329 >> >> (That's also why I suggest putting the utiltiy method in ReflectionFactory >> instead of AccessibleObject or Executable) >> >> `Proxy` is meaningless with an empty list of interfaces, as it's solely for >> implementing interfaces and delegating method calls to the given >> InvocationHandler. So I would ignore that scenario. > > Thanks. Unfortunately the variable `cloneArray` is actually a method > parameter, and most of the callers pass in `false` - so using this utility > method to control cloning would actually introduce cloning in several places > where it is explicitly avoided. We may get a performance gain by modifying > `Class.getInterfaces()` directly (the sole caller that passes `true`) then > eliminating the parameter, then modifying each caller, etc. but that feels > like a separate change inspired by this one. > > Thoughts? Why can't you do something like this: return cloneArray ? ReflectionFactory.copyClasses(interfaces) : interfaces; Alternatively, your proposal is a good one too; we can rename this `getInterfaces(boolean)` to `getInterfacesShared()` (like `getSharedEnumConstants()`) and move this copy operation to `getInterfaces()` as you suggest. - PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1610109601
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Tue, 21 May 2024 14:07:29 GMT, jengebr wrote:
> Any suggestions?
I would recommend a new method `copyClasses`/`copyTypes` in
`jdk.internal.reflect.ReflectionFactory`, as it already has related
`copyConstructor` and `getExecutableSharedParameterTypes` methods.
Also, can you rename your PR's title to `8332586: Avoid cloning empty arrays in
java.lang.reflect.{Method,Constructor}` for the bot to handle this?
-
PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2122798223
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Tue, 21 May 2024 13:49:18 GMT, jengebr wrote: > Improve `java/lang/reflect/Method.java` by eliminating needless cloning of > Class[0] instances. This cloning is intended to prevent callers from > changing array contents, but smany Methods have zero exceptions or zero > parameters, and returning the original `Class[0]` is sufficient. I think we should probably create a utility method dedicated to clone only mutable Class arrays. This utility can be used for the other methods in other reflection classes. Can you update the ending copyright years from 2023 to 2024 on the 2nd line of the 2 modified files? - PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2122710633 PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2130420945
Re: RFR: 8330182: Start of release updates for JDK 24 [v9]
On Thu, 30 May 2024 16:44:21 GMT, Joe Darcy wrote: >> Get JDK 24 underway. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Update symbol files for JDK 23 build 25. src/jdk.compiler/share/data/symbols/jdk.incubator.foreign-N.sym.txt line 1: > 1: # Just curious, does CreateSymbols not track module migrations, now that jdk.incubator.foreign is completely merged into java.base? - PR Review Comment: https://git.openjdk.org/jdk/pull/18787#discussion_r1621274290
Re: RFR: 8333265: De-duplicate method references in java.util.stream.FindOps
On Thu, 30 May 2024 12:50:36 GMT, Claes Redestad wrote: > Extracting duplicate method references to static field reduce proxy class > spinning and loading. In this case 2 less classes loaded when using > `findAny()` on each type of stream. Marked as reviewed by liach (Author). Indeed, CallSites are created per indy instruction instead of per CP indy entry (required by JVMS); thus sharing instruction either in initializers or static methods would have the same effect. javac only deduplicates the CP entry. - PR Review: https://git.openjdk.org/jdk/pull/19477#pullrequestreview-2088345919 PR Comment: https://git.openjdk.org/jdk/pull/19477#issuecomment-2139602569
Re: RFR: 8333265: De-duplicate method references in java.util.stream.FindOps
On Thu, 30 May 2024 12:50:36 GMT, Claes Redestad wrote: > Extracting duplicate method references to static field reduce proxy class > spinning and loading. In this case 2 less classes loaded when using > `findAny()` on each type of stream. Should we extract them to private static utility methods for potential laziness support in the future? - PR Comment: https://git.openjdk.org/jdk/pull/19477#issuecomment-2139514424
Integrated: 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull
On Tue, 21 May 2024 14:33:54 GMT, Chen Liang wrote: > I propose to add type-checked ConstantPool.entryByIndex and > ClassReader.readEntryOrNull taking an extra Class parameter, which throws > ConstantPoolException instead of ClassCastException on type mismatch, which > can happen to malformed ClassFiles. > > Requesting a review from @asotona. Thanks! > > Proposal on mailing list: > https://mail.openjdk.org/pipermail/classfile-api-dev/2024-May/000512.html This pull request has now been integrated. Changeset: f608918d Author:Chen Liang Committer: Adam Sotona URL: https://git.openjdk.org/jdk/commit/f608918df3f887277845db383cf07b0863bba615 Stats: 269 lines in 15 files changed: 179 ins; 39 del; 51 mod 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull Reviewed-by: asotona - PR: https://git.openjdk.org/jdk/pull/19330
Re: RFR: 8333103: Re-examine the console provider loading
On Wed, 29 May 2024 19:51:36 GMT, Naoto Sato wrote: > There is an initialization code in `Console` class that searches for the > Console implementations. Refactoring the init code not to use lambda/stream > would reduce the (initial) number of loaded classes by about 100 for > java.base implementations. This would become relevant when the java.io.IO > (JEP 477) uses Console as the underlying framework. src/java.base/share/classes/java/io/Console.java line 673: > 671: return new ProxyingConsole(jc); > 672: } > 673: break; Suggestion: The original `findAny` is only after `filter(Objects::nonNull)` meaning we don't return `null` on a null `jcp.console` result. - PR Review Comment: https://git.openjdk.org/jdk/pull/19467#discussion_r1619495420
Re: Question on Lambda function
Hi Zhengyu, What is your usage pattern of these single-abstract-method implementations? Since it sounds like you are creating a lot of them, are you storing them in collections? If you are keeping a lot of them in collection (say, as event handlers), you may try to use `MethodHandleProxies.asInterfaceInstance` as a temporary workaround on JDK 22 and higher (older version uses Proxy, which has horrible invocation performance). If you are on older versions from 15 to 21, unfortunately you might have to write a hidden class for the same purpose or use an existing library. One library that might be useful is https://github.com/LanternPowered/Lmbda that effectively generates unloadable hidden classes, but its 3.x builds are not maven central so you have to build yourself. - Chen On Wed, May 29, 2024 at 3:35 PM Zhengyu Gu wrote: > Hi Chen, > > > > Thanks for the insights. > > > > We did refactor our code to avoid using LambdaMetaFactory,metafactory() > directly. > > > > With increasing use of Lambdas, in our applications and libraries, the > metaspace impact becomes a concern. If current implementation (not able to > unload unused Lambda classes) here to stay, we must come up with a coding > guideline to avoid excessive creation of Lambda classes, any pointers or > suggestions would be greatly appreciated. > > > > Best, > > > > -Zhengyu > > > > *From: *Chen Liang > *Date: *Wednesday, May 29, 2024 at 2:43 PM > *To: *Zhengyu Gu > *Cc: *core-libs-dev@openjdk.org > *Subject: *Re: Question on Lambda function > *[External Email]* > > > -- > > Hi Gu, > > CallSite is specific to each invokedynamic instruction instead of each > InvokeDynamic constant pool entry: > https://docs.oracle.com/javase/specs/jvms/se22/html/jvms-6.html#jvms-6.5.invokedynamic > > And the linking is done by MethodHandleNatives.linkCallSite if you want to > follow the Java implementation code. > > For why the lambda in the loop is constant, it's a feature from > InnerClassLambdaMetafactory: > https://github.com/openjdk/jdk/blob/c8eea59f508158075382079316cf0990116ff98e/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java#L236 > > When the lambda is non-capturing, the bootstrap method > LambdaMetafactory.metafactory will eagerly create a singleton instance and > return this singleton in the indy instruction. > > > > Also, your metaspace pressure might be caused by the fact that Lambda > classes (not instances) are no longer eagerly unloaded; see > https://github.com/openjdk/jdk/pull/12493 and > https://bugs.openjdk.org/browse/JDK-8302154. You are recommended to > create your own facility to create hidden classes in Java 17 instead of > continue to use LambdaMetafactory explicitly in code. > > > > Regards, > > Chen Liang > > > > On Wed, May 29, 2024 at 12:53 PM Zhengyu Gu > wrote: > > Hello Lambda experts, > > > > Since we upgraded JDK from 11 to 17, we’re experiencing metaspace > pressure, largely due to Lambda class implementation changes. > > > > There’s a scenario (see attached test case), that is especially puzzled > me, hopefully, you can share some insights. > > > > In this test case, there is only one Lambda class is created inside the > loop, but each one for the same functions outside loop. > > > > Example output: > > > > 0: Func = LambdaFunc$$Lambda/0x1f8c4a20@4de8b406 > > testMethod() called > > 1: Func = LambdaFunc$$Lambda/0x1f8c4a20@4de8b406 > > testMethod() called > > 2: Func = LambdaFunc$$Lambda/0x1f8c4a20@4de8b406 > > testMethod() called > > 3: Func = LambdaFunc$$Lambda/0x1f8c4a20@4de8b406 > > testMethod() called > > 4: Func = LambdaFunc$$Lambda/0x1f8c4a20@4de8b406 > > testMethod() called > > > > …. > > > > Outside loop1, Func = LambdaFunc$$Lambda/0x1f8c4c58@402f32ff > > testMethod() called > > Outside loop2 Func = LambdaFunc$$Lambda/0x1f8d1000@5ae9a829 > > testMethod() called > > Outside loop3 Func = LambdaFunc$$Lambda/0x1f8d1238@548b7f67 > > testMethod() called > > > > And jcmd also confirmed there were 4 Lambda classes created: > > > > 49: CLD 0x6134cb50: "app" instance of > jdk.internal.loader.ClassLoaders$AppClassLoader > > Loaded classes: > > 1:LambdaFunc$$Lambda/0x1f8d1238 > > 2:LambdaFunc$$Lambda/0x1f8d1000 > > 3:LambdaFunc$$Lambda/0x1f8c4c58 > > 4:LambdaFunc$$Lambda/0x1f8c4a20 > > 5:LambdaFunc > > >
Re: RFR: 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull [v6]
> I propose to add type-checked ConstantPool.entryByIndex and > ClassReader.readEntryOrNull taking an extra Class parameter, which throws > ConstantPoolException instead of ClassCastException on type mismatch, which > can happen to malformed ClassFiles. > > Requesting a review from @asotona. Thanks! > > Proposal on mailing list: > https://mail.openjdk.org/pipermail/classfile-api-dev/2024-May/000512.html Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision: - Add test to validate ClassReader behavior - Merge branch 'master' of https://github.com/openjdk/jdk into feature/entry-by-type - Null-check the entry class eagerly, avoid returning null or throwing IAE - Remove redundant import - Use switch - Merge branch 'master' of https://github.com/openjdk/jdk into feature/entry-by-type - Merge branch 'master' of https://github.com/openjdk/jdk into feature/entry-by-type - Merge branch 'master' of https://github.com/openjdk/jdk into feature/entry-by-type - Use constants, beautify code - 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull - Changes: - all: https://git.openjdk.org/jdk/pull/19330/files - new: https://git.openjdk.org/jdk/pull/19330/files/ec56ce98..e9021f24 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19330=05 - incr: https://webrevs.openjdk.org/?repo=jdk=19330=04-05 Stats: 1876 lines in 41 files changed: 1109 ins; 498 del; 269 mod Patch: https://git.openjdk.org/jdk/pull/19330.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19330/head:pull/19330 PR: https://git.openjdk.org/jdk/pull/19330
Re: Question on Lambda function
Hi Gu, CallSite is specific to each invokedynamic instruction instead of each InvokeDynamic constant pool entry: https://docs.oracle.com/javase/specs/jvms/se22/html/jvms-6.html#jvms-6.5.invokedynamic And the linking is done by MethodHandleNatives.linkCallSite if you want to follow the Java implementation code. For why the lambda in the loop is constant, it's a feature from InnerClassLambdaMetafactory: https://github.com/openjdk/jdk/blob/c8eea59f508158075382079316cf0990116ff98e/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java#L236 When the lambda is non-capturing, the bootstrap method LambdaMetafactory.metafactory will eagerly create a singleton instance and return this singleton in the indy instruction. Also, your metaspace pressure might be caused by the fact that Lambda classes (not instances) are no longer eagerly unloaded; see https://github.com/openjdk/jdk/pull/12493 and https://bugs.openjdk.org/browse/JDK-8302154. You are recommended to create your own facility to create hidden classes in Java 17 instead of continue to use LambdaMetafactory explicitly in code. Regards, Chen Liang On Wed, May 29, 2024 at 12:53 PM Zhengyu Gu wrote: > Hello Lambda experts, > > > > Since we upgraded JDK from 11 to 17, we’re experiencing metaspace > pressure, largely due to Lambda class implementation changes. > > > > There’s a scenario (see attached test case), that is especially puzzled > me, hopefully, you can share some insights. > > > > In this test case, there is only one Lambda class is created inside the > loop, but each one for the same functions outside loop. > > > > Example output: > > > > 0: Func = LambdaFunc$$Lambda/0x1f8c4a20@4de8b406 > > testMethod() called > > 1: Func = LambdaFunc$$Lambda/0x1f8c4a20@4de8b406 > > testMethod() called > > 2: Func = LambdaFunc$$Lambda/0x1f8c4a20@4de8b406 > > testMethod() called > > 3: Func = LambdaFunc$$Lambda/0x1f8c4a20@4de8b406 > > testMethod() called > > 4: Func = LambdaFunc$$Lambda/0x1f8c4a20@4de8b406 > > testMethod() called > > > > …. > > > > Outside loop1, Func = LambdaFunc$$Lambda/0x1f8c4c58@402f32ff > > testMethod() called > > Outside loop2 Func = LambdaFunc$$Lambda/0x1f8d1000@5ae9a829 > > testMethod() called > > Outside loop3 Func = LambdaFunc$$Lambda/0x1f8d1238@548b7f67 > > testMethod() called > > > > And jcmd also confirmed there were 4 Lambda classes created: > > > > 49: CLD 0x6134cb50: "app" instance of > jdk.internal.loader.ClassLoaders$AppClassLoader > > Loaded classes: > > 1:LambdaFunc$$Lambda/0x1f8d1238 > > 2:LambdaFunc$$Lambda/0x1f8d1000 > > 3:LambdaFunc$$Lambda/0x1f8c4c58 > > 4:LambdaFunc$$Lambda/0x1f8c4a20 > > 5:LambdaFunc > > > > Looking into bytecode, all four call sites have the same invokedynamic > bytecode (invokedynamic #7, 0 // InvokeDynamic > #0:run:()Ljava/lang/Runnable; ) and the first invokedynamic bytecode is > inside the loop. > > > > But when I ran the program with -XX:+TraceBytecodes, it seems that the > first invokedynamic was hoisted and result was used in the subsequence loop. > > > > Can anyone explain where this magic happens? If the magic can apply to > the instances outside the loop, so that only one Lambda class is created? > > > > > > Thank you for your time and expertise, > > > > -Zhengyu > > > > > > >
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v12]
On Wed, 29 May 2024 07:17:38 GMT, Adam Sotona wrote: >> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use >> classfile API for reflection proxy-generation. Actual implementation of >> `ProxyGenerator` is focused on performance, however it causes JDK bootstrap >> regressions. `ProxyGenerator.TEMPLATE` class model is statically created and >> each proxy class is transformed from the template. >> >> This patch is intended to examine plain proxy generation impact on >> performance and JDK bootstrap (vs proxy transformation from template). >> >> The generated proxy is migrated from static initialization to CONDY >> bootstrap. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 13 additional > commits since the last revision: > > - obsolete import > - Merge branch 'master' into JDK-8332457-proxy-startup > - missing bracket > - Update src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java > >Co-authored-by: liach <7806504+li...@users.noreply.github.com> > - removed obsolete entry > - MTD_ cleanup > - fixed javadoc > - CONDY implementation of ProxyGenerator > - simplification of the proxy class loading > - more improvements > - ... and 3 more: https://git.openjdk.org/jdk/compare/7a251c2c...942342d5 Shouldn't condy mainly benefit the cases where only some of the methods are called so the proxy doesn't have to initialize all methods with reflection, or the case where a proxy is created but none of its methods is called? Does it show a regression in those cases as well? - PR Comment: https://git.openjdk.org/jdk/pull/19410#issuecomment-2137366357
Re: RFR: 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull [v5]
> I propose to add type-checked ConstantPool.entryByIndex and > ClassReader.readEntryOrNull taking an extra Class parameter, which throws > ConstantPoolException instead of ClassCastException on type mismatch, which > can happen to malformed ClassFiles. > > Requesting a review from @asotona. Thanks! > > Proposal on mailing list: > https://mail.openjdk.org/pipermail/classfile-api-dev/2024-May/000512.html Chen Liang has updated the pull request incrementally with one additional commit since the last revision: Remove redundant import - Changes: - all: https://git.openjdk.org/jdk/pull/19330/files - new: https://git.openjdk.org/jdk/pull/19330/files/b3b94639..ec56ce98 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19330=04 - incr: https://webrevs.openjdk.org/?repo=jdk=19330=03-04 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19330.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19330/head:pull/19330 PR: https://git.openjdk.org/jdk/pull/19330
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Mon, 27 May 2024 20:55:29 GMT, Pavel Rappo wrote: >> Please review this PR, which supersedes a now withdrawn >> https://github.com/openjdk/jdk/pull/14831. >> >> This PR replaces `ArraysSupport.vectorizedHashCode` with a set of more >> user-friendly methods. Here's a summary: >> >> - Made the operand constants (i.e. `T_BOOLEAN` and friends) and the >> `vectorizedHashCode` method private >> >> - Made the `vectorizedHashCode` method private, but didn't rename it. >> Renaming would dramatically increase this PR review cost, because that >> method's name is used by a lot of VM code. On a bright side, since the >> method is now private, it's no longer callable by clients of >> `ArraysSupport`, thus a problem of an inaccurate name is less severe. >> >> - Made the `ArraysSupport.utf16HashCode` method private >> >> - Moved tiny cases (i.e. 0, 1, 2) to `ArraysSupport` > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Fix incorrect utf16 hashCode adaptation Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/19414#pullrequestreview-2085162416
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v12]
On Wed, 29 May 2024 07:17:38 GMT, Adam Sotona wrote: >> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use >> classfile API for reflection proxy-generation. Actual implementation of >> `ProxyGenerator` is focused on performance, however it causes JDK bootstrap >> regressions. `ProxyGenerator.TEMPLATE` class model is statically created and >> each proxy class is transformed from the template. >> >> This patch is intended to examine plain proxy generation impact on >> performance and JDK bootstrap (vs proxy transformation from template). >> >> The generated proxy is migrated from static initialization to CONDY >> bootstrap. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 13 additional > commits since the last revision: > > - obsolete import > - Merge branch 'master' into JDK-8332457-proxy-startup > - missing bracket > - Update src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java > >Co-authored-by: liach <7806504+li...@users.noreply.github.com> > - removed obsolete entry > - MTD_ cleanup > - fixed javadoc > - CONDY implementation of ProxyGenerator > - simplification of the proxy class loading > - more improvements > - ... and 3 more: https://git.openjdk.org/jdk/compare/19d52556...942342d5 Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/19410#pullrequestreview-2085154590
Re: RFR: 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull [v4]
> I propose to add type-checked ConstantPool.entryByIndex and > ClassReader.readEntryOrNull taking an extra Class parameter, which throws > ConstantPoolException instead of ClassCastException on type mismatch, which > can happen to malformed ClassFiles. > > Requesting a review from @asotona. Thanks! > > Proposal on mailing list: > https://mail.openjdk.org/pipermail/classfile-api-dev/2024-May/000512.html Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Use switch - Merge branch 'master' of https://github.com/openjdk/jdk into feature/entry-by-type - Merge branch 'master' of https://github.com/openjdk/jdk into feature/entry-by-type - Merge branch 'master' of https://github.com/openjdk/jdk into feature/entry-by-type - Use constants, beautify code - 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull - Changes: - all: https://git.openjdk.org/jdk/pull/19330/files - new: https://git.openjdk.org/jdk/pull/19330/files/a68519d3..b3b94639 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19330=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19330=02-03 Stats: 6144 lines in 270 files changed: 3709 ins; 843 del; 1592 mod Patch: https://git.openjdk.org/jdk/pull/19330.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19330/head:pull/19330 PR: https://git.openjdk.org/jdk/pull/19330
Re: RFR: 8332597: Remove redundant methods from j.l.classfile.ClassReader API [v2]
On Fri, 24 May 2024 16:41:33 GMT, Adam Sotona wrote: >> j.l.classfile.ClassReader instance is exposed in the Class-File API through >> j.l.classfile.AttributeMapper::readAttribute method only. >> ClassReader only purpose is to serve as a tool for reading content of a >> custom attribute in a user-provided AttribtueMapper. >> It contains useful set of low-level class reading methods for user to >> implement a custom attribute content parser. >> >> However methods ClassReader::thisClassPos and >> ClassReader::skipAttributeHolder are not necessary for a custom attribute >> content parsing and so redundant in the API. >> Class-File API implementation internally use these methods, however they >> should not be exposed in the API. >> >> This patch removes the methods from the API. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Merge branch 'master' into JDK-8332597-classreader-redundancy > ># Conflicts: ># src/java.base/share/classes/jdk/internal/classfile/impl/ClassImpl.java > - 8332597: Remove redundant methods from j.l.classfile.ClassReader API @asotona Can you transition the CSR to proposed again for reviewing? - PR Comment: https://git.openjdk.org/jdk/pull/19323#issuecomment-2136470497
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Mon, 27 May 2024 20:55:29 GMT, Pavel Rappo wrote:
>> Please review this PR, which supersedes a now withdrawn
>> https://github.com/openjdk/jdk/pull/14831.
>>
>> This PR replaces `ArraysSupport.vectorizedHashCode` with a set of more
>> user-friendly methods. Here's a summary:
>>
>> - Made the operand constants (i.e. `T_BOOLEAN` and friends) and the
>> `vectorizedHashCode` method private
>>
>> - Made the `vectorizedHashCode` method private, but didn't rename it.
>> Renaming would dramatically increase this PR review cost, because that
>> method's name is used by a lot of VM code. On a bright side, since the
>> method is now private, it's no longer callable by clients of
>> `ArraysSupport`, thus a problem of an inaccurate name is less severe.
>>
>> - Made the `ArraysSupport.utf16HashCode` method private
>>
>> - Moved tiny cases (i.e. 0, 1, 2) to `ArraysSupport`
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix incorrect utf16 hashCode adaptation
src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 320:
> 318: * @return the calculated hash value
> 319: */
> 320: public static int hashCode(Object[] a, int fromIndex, int length,
> int initialValue) {
Is the object variant necessary here? The object version is hard for JIT to
profile as it's quite polymorphic compared to other arrays, and the initial
value is always 1.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1618129002
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Tue, 28 May 2024 22:20:39 GMT, Pavel Rappo wrote: >> I believe, it should be `1`. Hear me out. In this method, the `length` is >> scaled down, whereas in `StringUTF16` it is not. In this method, it's >> `length`, in `StringUTF16` it's `((byte[]) value).length`. > > In fact, if I change it to `2`, the following tests will fail: > > - `jdk/jdk/classfile/Utf8EntryTest.java` > - `jdk/java/util/zip/ZipCoding.java` > - `jdk/java/text/Format/MessageFormat/MessageRegression.java` Indeed, the actual length passed at call site is `value.length >> 1` instead of `value.length`; this adjusted char-length carries on to `vectorizedHashCode` call. - PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1618126401
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v9]
On Tue, 28 May 2024 14:56:35 GMT, Adam Sotona wrote: >> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use >> classfile API for reflection proxy-generation. Actual implementation of >> `ProxyGenerator` is focused on performance, however it causes JDK bootstrap >> regressions. `ProxyGenerator.TEMPLATE` class model is statically created and >> each proxy class is transformed from the template. >> >> This patch is intended to examine plain proxy generation impact on >> performance and JDK bootstrap (vs proxy transformation from template). >> >> The generated proxy is migrated from static initialization to CONDY >> bootstrap. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > removed obsolete entry src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 676: > 674: toClassDesc(fromClass), > 675: method.getName(), > 676: > MethodType.methodType(method.getReturnType(), > method.getParameterTypes()).describeConstable().get())); Suggestion: desc); - PR Review Comment: https://git.openjdk.org/jdk/pull/19410#discussion_r1617457062
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v8]
On Tue, 28 May 2024 11:55:30 GMT, Adam Sotona wrote: >> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use >> classfile API for reflection proxy-generation. Actual implementation of >> `ProxyGenerator` is focused on performance, however it causes JDK bootstrap >> regressions. `ProxyGenerator.TEMPLATE` class model is statically created and >> each proxy class is transformed from the template. >> >> This patch is intended to examine plain proxy generation impact on >> performance and JDK bootstrap (vs proxy transformation from template). >> >> The generated proxy is migrated from static initialization to CONDY >> bootstrap. >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > MTD_ cleanup Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/19410#pullrequestreview-2082875192
Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v9]
On Fri, 24 May 2024 16:17:41 GMT, Adam Sotona wrote: >> ClassFile API `jdk.internal.classfile.verifier.VerifierImpl` performed only >> bytecode-level class verification. >> This patch adds `jdk.internal.classfile.verifier.ParserVerifier` with >> additional class checks inspired by >> `hotspot/share/classfile/classFileParser.cpp`. >> >> Also new `VerifierSelfTest::testParserVerifier` has been added. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 37 commits: > > - fixed ParserVerifier and VerifierSelfTest > - Merge branch 'master' into JDK-8320396-verifier-extension > - added verification of TypeAnnotation attributes > - added verification of SMT attribute > - added verification of module-related attributes > - applied the suggested changes > - applied the suggested changes > - fixed error thrown by VerifierImpl > - applied suggested changes > - Merge branch 'master' into JDK-8320396-verifier-extension > - ... and 27 more: https://git.openjdk.org/jdk/compare/cfdc64fc...b352b794 Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/16809#pullrequestreview-2082798275
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v8]
On Tue, 28 May 2024 11:55:30 GMT, Adam Sotona wrote: >> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use >> classfile API for reflection proxy-generation. Actual implementation of >> `ProxyGenerator` is focused on performance, however it causes JDK bootstrap >> regressions. `ProxyGenerator.TEMPLATE` class model is statically created and >> each proxy class is transformed from the template. >> >> This patch is intended to examine plain proxy generation impact on >> performance and JDK bootstrap (vs proxy transformation from template). >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > MTD_ cleanup Nice work migrating method object initialization to condy. - PR Comment: https://git.openjdk.org/jdk/pull/19410#issuecomment-2135212751
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v3]
On Mon, 27 May 2024 16:08:04 GMT, Adam Sotona wrote: >> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 822: >> >>> 820:.iconst_0() // false >>> 821:.aload(0)// classLoader >>> 822:.invokestatic(CD_Class, "forName", >>> MTD_Class_String_boolean_ClassLoader); >> >> We can probably replace this `forName(name, false, thisClassLoader)` with >> loading a class constant to reduce load on symbols. > > I'm wondering why all the `Class.forName(... .getClassLoader())` > is necessary? > Simple `ldc ` seems to work well. Indeed, I initially added it because a JBS ticket recommended so. ldc instruction has the same behavior as such as forName call. - PR Review Comment: https://git.openjdk.org/jdk/pull/19410#discussion_r1616241702
Re: RFR: 8292955: Collections.checkedMap Map.merge does not properly check key and value [v3]
On Thu, 7 Mar 2024 05:33:16 GMT, Lei Zhu wrote: >> When the specified key did not associated with a value, should check the >> `key` and `value` type. > > Lei Zhu has updated the pull request incrementally with one additional commit > since the last revision: > > Use testNG builtin functionalities and modify the test function name @minborg Is it possible for you to review this collection bugfix? - PR Comment: https://git.openjdk.org/jdk/pull/18141#issuecomment-2133400541
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v3]
On Mon, 27 May 2024 11:47:15 GMT, Adam Sotona wrote: >> [JDK-8294961](https://bugs.openjdk.org/browse/JDK-8294961) changed to use >> classfile API for reflection proxy-generation. Actual implementation of >> `ProxyGenerator` is focused on performance, however it causes JDK bootstrap >> regressions. `ProxyGenerator.TEMPLATE` class model is statically created and >> each proxy class is transformed from the template. >> >> This patch is intended to examine plain proxy generation impact on >> performance and JDK bootstrap (vs proxy transformation from template). >> >> Please review. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > performance improvements src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 76: > 74: > 75: private static final MethodTypeDesc > 76: MTD_boolean = MethodTypeDescImpl.ofValidated(CD_boolean, > ConstantUtils.EMPTY_CLASSDESC), Maybe we can change the `MethodTypeDescImpl.ofValidated` to varargs so we don't need explicit array initializations. src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 822: > 820:.iconst_0() // false > 821:.aload(0)// classLoader > 822:.invokestatic(CD_Class, "forName", > MTD_Class_String_boolean_ClassLoader); We can probably replace this `forName(name, false, thisClassLoader)` with loading a class constant to reduce load on symbols. - PR Review Comment: https://git.openjdk.org/jdk/pull/19410#discussion_r1615978269 PR Review Comment: https://git.openjdk.org/jdk/pull/19410#discussion_r1615982718
Re: RFR: 8328821: Map.of().entrySet() mutators should throw UnsupportedOperationException
On Mon, 27 May 2024 07:24:47 GMT, Per Minborg wrote:
>> This change overrides mutator methods in the implementation returned by
>> `Map.of().entrySet()` to throw `UnsupportedOperationException`.
>
> src/java.base/share/classes/java/util/ImmutableCollections.java line 1323:
>
>> 1321: @Override
>> 1322: public int hashCode() {
>> 1323: return MapN.this.hashCode();
>
> The hash code for a `Set` is defined as the sum of the elements in the `Set`
> (hash(`null`) == 0). The `Map. Entry` hash code is defined the same way
> `MapN.this.hashCode` operates so this seems right. It would be nice with a
> couple of tests that assert this invariant.
>
> Perhaps the existing tests already cover this?
Good point, this is currently missing from MOAT in Collections.
Though it can be as simple as one line here:
https://github.com/openjdk/jdk/blob/891d5aedf12e837c9a9c7cb800fb3affa7430f00/test/jdk/java/util/Collection/MOAT.java#L1329
check(m.hashCode() == m.entrySet().hashCode());
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18522#discussion_r1615976036
Re: RFR: 8242888: Convert dynamic proxy to hidden classes
On Mon, 27 May 2024 00:03:41 GMT, ExE Boss wrote:
>> Please review this change that convert dynamic proxies implementations to
>> hidden classes, intended to target JDK 24.
>>
>> Summary:
>> 1. Adds new implementation while preserving the old implementation behind
>> `-Djdk.reflect.useLegacyProxyImpl=true` in case there are compatibility
>> issues.
>> 2. ClassLoader.defineClass0 takes a ClassLoader instance but discards it in
>> native code; I updated native code to reuse that ClassLoader for Proxy
>> support.
>> 3. ProxyGenerator changes mainly involve using Class data to pass Method
>> list (accessed in a single condy) and removal of obsolete setup code
>> generation.
>>
>> Testing: tier1 and tier2 have no related failures.
>>
>> Comment: Since #8278, Proxy has been converted to ClassFile API, and
>> infrastructure has changed; now, the migration to hidden classes is much
>> cleaner and has less impact, such as preserving ProtectionDomain and dynamic
>> module without "anchor classes", and avoiding java.lang.invoke package.
>
> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
> 557:
>
>> 555: public static boolean useLegacyProxyImpl() {
>> 556: var config = config();
>> 557: return config.useLegacyProxyImpl &&
>> !config.useOldSerializableConstructor;
>
> Suggestion:
>
> return config.useLegacyProxyImpl ||
> config.useOldSerializableConstructor;
This site can actually simply be `config.useLegacyProxyImpl` as it's
initialized in `loadConfig`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19356#discussion_r1615382817
Re: RFR: 8327858: Improve spliterator and forEach for single-element immutable collections [v3]
On Fri, 26 Apr 2024 22:27:21 GMT, Chen Liang wrote: >> Please review this patch that: >> 1. Implemented `forEach` to optimize for 1 or 2 element collections. >> 2. Implemented `spliterator` to optimize for a single element. >> >> The default implementations for multiple-element immutable collections are >> fine as-is, specializing implementation doesn't provide much benefit. > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 16 commits: > > - Add test to ensure reproducible iteration order > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Use the improved form in forEach > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Null checks should probably be in the beginning... > - mark implicit null checks > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Copyright year, revert changes for non-few element collections > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - ... and 6 more: https://git.openjdk.org/jdk/compare/a920af23...70583024 Keep-alive. - PR Comment: https://git.openjdk.org/jdk/pull/15834#issuecomment-2132078702
Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`
Did you reorder the field in picocli.CommandLine$Command?
In class 13 the usageHelpWidth was preceded by versionProvider, in class 9
it's preceded by requiredOptionMarker. It would also be helpful if you can
provide the bytecode for Command class, as it might be due to method
ordering in reflection from multiple inheritance (if Command extends
multiple interfaces), as you see some subsections of the methods (such as
from requiredOptionMarker to said footerHeading) are ordered.
Chen
On Sat, May 25, 2024 at 4:03 PM Aman Sharma wrote:
> Hi Chen,
>
> I am attaching proxy classes generated in the JVM of OpenJDK 22. I,
> instead of decompiling, I disassembled them and I do see a difference. For
> example, see method `footerHeading` in both classes. In $Proxy9, it is
> mapped to `m39` field and in $Proxy13, it is mapped to `m21` field. What is
> the reason for this ordering? Why is mapping of methods to fields depend
> upon the execution?
>
>
> Regards,
> Aman Sharma
>
> PhD Student
> KTH Royal Institute of Technology
> School of Electrical Engineering and Computer Science (EECS)
> Department of Theoretical Computer Science (TCS)
> <https://www.kth.se/profile/amansha>https://algomaster99.github.io/
> --
> *From:* Chen Liang
> *Sent:* Wednesday, May 22, 2024 9:37:16 PM
> *To:* Aman Sharma
> *Cc:* core-libs-dev@openjdk.org; leyden-...@openjdk.org
> *Subject:* Re: Deterministic naming of subclasses of
> `java/lang/reflect/Proxy`
>
> Hi Aman,
> Even though the specification says "not in any particular order," the
> getInterfaces and getMethods actually return an ordered array, in the order
> these methods/interfaces are declared in their class files.
>
> I believe you are decompiling the proxy classes generated by an older
> version of the JDK; for example, back in JDK 8, the proxy methods were not
> ordered because they were tracked in a HashMap:
> https://github.com/openjdk/jdk8u/blob/6b53212ef78ad50f9eede829c5ff87cadcdb434b/jdk/src/share/classes/sun/misc/ProxyGenerator.java#L405
> Which is no longer the case:
> https://github.com/openjdk/jdk/blob/d59c12fe1041a1f61f68408241a9aa4d96ac4fd2/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java#L241
>
> - Chen
>
> On Wed, May 22, 2024 at 1:19 PM Aman Sharma wrote:
>
>> Hi,
>>
>>
>> Another thing I wanted to look into in this thread was the order of
>> fields in the Proxy classes generated. They are also based on the a
>> number. The same proxy classes across different executions can have random
>> order of `Method` fields and the methods could be mapped to different field
>> names.
>>
>>
>> For example, consider the proxy class based on `picocli.CommandLine
>> <https://github.com/remkop/picocli/blob/da98db63d1b516141b7485881b0dcddfd082dbc8/src/main/java/picocli/CommandLine.java#L4541>`
>> in two different executions.
>>
>> // fields and method are truncated for brevity
>> public final class $Proxy9 extends Proxy implements CommandLine.Command {
>> private static Method m1;
>> private static Method m32;
>> private static Method m21;
>> private static Method m43;
>> private static Method m36;
>> private static Method m27;
>>
>> public final boolean helpCommand() throws {
>> try {
>> return (Boolean)super.h.invoke(this, m32, (Object[])null);
>> } catch (RuntimeException | Error var2) {
>> throw var2;
>> } catch (Throwable var3) {
>> throw new UndeclaredThrowableException(var3);
>> }
>> }
>>
>> // fields and method are truncated for brevity
>> public final class $Proxy13 extends Proxy implements CommandLine.Command {
>> private static Method m1;
>> private static Method m29;
>> private static Method m16;
>> private static Method m40;
>> private static Method m38;
>> private static Method m12;
>>
>> public final boolean helpCommand() throws {
>> try {
>> return (Boolean)super.h.invoke(this, m29, (Object[])null);
>> } catch (RuntimeException | Error var2) {
>> throw var2;
>> } catch (Throwable var3) {
>> throw new UndeclaredThrowableException(var3);
>> }
>> }
>>
>>
>> Notice the difference in the order of fields and `helpCommand` method is
>> mapped to a different field name in both classes. This happens because
>> the method array returned by `getMethods` is not sorted in any
>> particular order
>> <https://github.com/openjdk/jdk/blob/ma
Re: RFR: 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull [v3]
> I propose to add type-checked ConstantPool.entryByIndex and > ClassReader.readEntryOrNull taking an extra Class parameter, which throws > ConstantPoolException instead of ClassCastException on type mismatch, which > can happen to malformed ClassFiles. > > Requesting a review from @asotona. Thanks! > > Proposal on mailing list: > https://mail.openjdk.org/pipermail/classfile-api-dev/2024-May/000512.html Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into feature/entry-by-type - Merge branch 'master' of https://github.com/openjdk/jdk into feature/entry-by-type - Use constants, beautify code - 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull - Changes: - all: https://git.openjdk.org/jdk/pull/19330/files - new: https://git.openjdk.org/jdk/pull/19330/files/c9f6fc18..a68519d3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19330=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19330=01-02 Stats: 15592 lines in 362 files changed: 10436 ins; 3396 del; 1760 mod Patch: https://git.openjdk.org/jdk/pull/19330.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19330/head:pull/19330 PR: https://git.openjdk.org/jdk/pull/19330
Re: RFR: 8242888: Convert dynamic proxy to hidden classes
On Thu, 23 May 2024 03:28:30 GMT, Chen Liang wrote: > Please review this change that convert dynamic proxies implementations to > hidden classes, intended to target JDK 24. > > Summary: > 1. Adds new implementation while preserving the old implementation behind > `-Djdk.reflect.useLegacyProxyImpl=true` in case there are compatibility > issues. > 2. ClassLoader.defineClass0 takes a ClassLoader instance but discards it in > native code; I updated native code to reuse that ClassLoader for Proxy > support. > 3. ProxyGenerator changes mainly involve using Class data to pass Method list > (accessed in a single condy) and removal of obsolete setup code generation. > > Testing: tier1 and tier2 have no related failures. > > Comment: Since #8278, Proxy has been converted to ClassFile API, and > infrastructure has changed; now, the migration to hidden classes is much > cleaner and has less impact, such as preserving ProtectionDomain and dynamic > module without "anchor classes", and avoiding java.lang.invoke package. Hmm, actually, looking at the specs of the method again, does it imply that Proxy classes are never unloaded once defined in a ClassLoader, as seen in `Proxy::getProxyClass`: > If a proxy class for the same permutation of interfaces has already been > defined by the class loader, then the existing proxy class will be returned If that's the case, Remi's suggestion on passing classdata to a non-hidden class might be better, and it seems to accomplish that in hotspot isn't too hard too. - PR Comment: https://git.openjdk.org/jdk/pull/19356#issuecomment-2128186405
Re: RFR: 8242888: Convert dynamic proxy to hidden classes
On Thu, 23 May 2024 03:28:30 GMT, Chen Liang wrote: > Please review this change that convert dynamic proxies implementations to > hidden classes, intended to target JDK 24. > > Summary: > 1. Adds new implementation while preserving the old implementation behind > `-Djdk.reflect.useLegacyProxyImpl=true` in case there are compatibility > issues. > 2. ClassLoader.defineClass0 takes a ClassLoader instance but discards it in > native code; I updated native code to reuse that ClassLoader for Proxy > support. > 3. ProxyGenerator changes mainly involve using Class data to pass Method list > (accessed in a single condy) and removal of obsolete setup code generation. > > Testing: tier1 and tier2 have no related failures. > > Comment: Since #8278, Proxy has been converted to ClassFile API, and > infrastructure has changed; now, the migration to hidden classes is much > cleaner and has less impact, such as preserving ProtectionDomain and dynamic > module without "anchor classes", and avoiding java.lang.invoke package. I have updated the compatibility risk description of the CSR. My CSR proposes to allow dynamic unloading of the proxy implementation classes, but currently it's not implemented as they are strongly referenced in the ClassLoaderValue caches. Should I implement dynamic unloading suggested in the CSR in this patch, or should I do it later? - PR Comment: https://git.openjdk.org/jdk/pull/19356#issuecomment-2127111543
Re: RFR: 8242888: Convert dynamic proxy to hidden classes
Hmm, I think Proxy being hidden in stacktraces might be an advantage; the same happens for lambdas. The main advantage of hidden classes compared to an explicit class with classData is that it supports flexible unloading, which might be useful for Proxy. I still believe the flexible unloading advantage justifies the migration to hidden classes. Chen On Thu, May 23, 2024 at 6:43 AM Remi Forax wrote: > - Original Message - > > From: "Chen Liang" > > To: "core-libs-dev" , "hotspot-dev" < > hotspot-...@openjdk.org>, kulla-...@openjdk.org > > Sent: Thursday, May 23, 2024 1:28:01 PM > > Subject: Re: RFR: 8242888: Convert dynamic proxy to hidden classes > > > On Thu, 23 May 2024 03:28:30 GMT, Chen Liang wrote: > > > >> Please review this change that convert dynamic proxies implementations > to hidden > >> classes, intended to target JDK 24. > >> > >> Summary: > >> 1. Adds new implementation while preserving the old implementation > behind > >> `-Djdk.reflect.useLegacyProxyImpl=true` in case there are compatibility > issues. > >> 2. ClassLoader.defineClass0 takes a ClassLoader instance but discards > it in > >> native code; I updated native code to reuse that ClassLoader for Proxy > support. > >> 3. ProxyGenerator changes mainly involve using Class data to pass > Method list > >> (accessed in a single condy) and removal of obsolete setup code > generation. > >> > >> Testing: tier1 and tier2 have no related failures. > >> > >> Comment: Since #8278, Proxy has been converted to ClassFile API, and > >> infrastructure has changed; now, the migration to hidden classes is much > >> cleaner and has less impact, such as preserving ProtectionDomain and > dynamic > >> module without "anchor classes", and avoiding java.lang.invoke package. > > > > A CSR targeting 24 describing the compatibility concerns and behavioral > > differences is here, somehow not linked by skara: > > https://bugs.openjdk.org/browse/JDK-8332770 > > The incompatibilities were much greater in the previous iterations of > this > > issue, such as in dynamic modules, serialization, and in proxy class > protection > > domain. Now these aspects are addressed by this patch, the only real one > left > > is the change in stack trace. Feel free to raise other incompatibilities > you > > have discovered. > > I wonder if instead of using hidden classes, we should not use usual named > classes and add a new Lookup.defineClass() that takes a classData as > parameter. > This will solve the both the problem of the stacktrace and the problem of > the roundtrip proxyClass != Class.forName(proxyClass.getName()). > > Rémi > > > > > - > > > > PR Comment: > https://git.openjdk.org/jdk/pull/19356#issuecomment-2126869679 >
Re: RFR: 8242888: Convert dynamic proxy to hidden classes
On Thu, 23 May 2024 03:28:30 GMT, Chen Liang wrote: > Please review this change that convert dynamic proxies implementations to > hidden classes, intended to target JDK 24. > > Summary: > 1. Adds new implementation while preserving the old implementation behind > `-Djdk.reflect.useLegacyProxyImpl=true` in case there are compatibility > issues. > 2. ClassLoader.defineClass0 takes a ClassLoader instance but discards it in > native code; I updated native code to reuse that ClassLoader for Proxy > support. > 3. ProxyGenerator changes mainly involve using Class data to pass Method list > (accessed in a single condy) and removal of obsolete setup code generation. > > Testing: tier1 and tier2 have no related failures. > > Comment: Since #8278, Proxy has been converted to ClassFile API, and > infrastructure has changed; now, the migration to hidden classes is much > cleaner and has less impact, such as preserving ProtectionDomain and dynamic > module without "anchor classes", and avoiding java.lang.invoke package. A CSR targeting 24 describing the compatibility concerns and behavioral differences is here, somehow not linked by skara: https://bugs.openjdk.org/browse/JDK-8332770 The incompatibilities were much greater in the previous iterations of this issue, such as in dynamic modules, serialization, and in proxy class protection domain. Now these aspects are addressed by this patch, the only real one left is the change in stack trace. Feel free to raise other incompatibilities you have discovered. - PR Comment: https://git.openjdk.org/jdk/pull/19356#issuecomment-2126869679
RFR: 8242888: Convert dynamic proxy to hidden classes
Please review this change that convert dynamic proxies implementations to hidden classes, intended to target JDK 24. Summary: 1. Adds new implementation while preserving the old implementation behind `-Djdk.reflect.useLegacyProxyImpl=true` in case there are compatibility issues. 2. ClassLoader.defineClass0 takes a ClassLoader instance but discards it in native code; I updated native code to reuse that ClassLoader for Proxy support. 3. ProxyGenerator changes mainly involve using Class data to pass Method list (accessed in a single condy) and removal of obsolete setup code generation. Testing: tier1 and tier2 have no related failures. Comment: Since #8278, Proxy has been converted to ClassFile API, and infrastructure has changed; now, the migration to hidden classes is much cleaner and has less impact, such as preserving ProtectionDomain and dynamic module without "anchor classes", and avoiding java.lang.invoke package. - Commit messages: - Fixes - Merge branch 'master' of https://github.com/openjdk/jdk into feature/hidden-proxy - First draft Changes: https://git.openjdk.org/jdk/pull/19356/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19356=00 Issue: https://bugs.openjdk.org/browse/JDK-8242888 Stats: 303 lines in 8 files changed: 70 ins; 153 del; 80 mod Patch: https://git.openjdk.org/jdk/pull/19356.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19356/head:pull/19356 PR: https://git.openjdk.org/jdk/pull/19356
Re: RFR: 8320575: generic type information lost on mandated parameters of record's compact constructors [v7]
On Thu, 23 May 2024 00:58:14 GMT, Joe Darcy wrote: > As a general comment, please update all the links to "mandated" so that the > text "implicitly declared" get linked to the MANDATED enum constant. Should we update the API specification for `Parameter::isImplicit`, which checks the "mandated" status but does not refer to this concept at all? - PR Comment: https://git.openjdk.org/jdk/pull/17070#issuecomment-2126011251
Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`
Hi Aman,
Even though the specification says "not in any particular order," the
getInterfaces and getMethods actually return an ordered array, in the order
these methods/interfaces are declared in their class files.
I believe you are decompiling the proxy classes generated by an older
version of the JDK; for example, back in JDK 8, the proxy methods were not
ordered because they were tracked in a HashMap:
https://github.com/openjdk/jdk8u/blob/6b53212ef78ad50f9eede829c5ff87cadcdb434b/jdk/src/share/classes/sun/misc/ProxyGenerator.java#L405
Which is no longer the case:
https://github.com/openjdk/jdk/blob/d59c12fe1041a1f61f68408241a9aa4d96ac4fd2/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java#L241
- Chen
On Wed, May 22, 2024 at 1:19 PM Aman Sharma wrote:
> Hi,
>
>
> Another thing I wanted to look into in this thread was the order of fields
> in the Proxy classes generated. They are also based on the a number. The
> same proxy classes across different executions can have random order of
> `Method` fields and the methods could be mapped to different field names.
>
>
> For example, consider the proxy class based on `picocli.CommandLine
> <https://github.com/remkop/picocli/blob/da98db63d1b516141b7485881b0dcddfd082dbc8/src/main/java/picocli/CommandLine.java#L4541>`
> in two different executions.
>
> // fields and method are truncated for brevity
> public final class $Proxy9 extends Proxy implements CommandLine.Command {
> private static Method m1;
> private static Method m32;
> private static Method m21;
> private static Method m43;
> private static Method m36;
> private static Method m27;
>
> public final boolean helpCommand() throws {
> try {
> return (Boolean)super.h.invoke(this, m32, (Object[])null);
> } catch (RuntimeException | Error var2) {
> throw var2;
> } catch (Throwable var3) {
> throw new UndeclaredThrowableException(var3);
> }
> }
>
> // fields and method are truncated for brevity
> public final class $Proxy13 extends Proxy implements CommandLine.Command {
> private static Method m1;
> private static Method m29;
> private static Method m16;
> private static Method m40;
> private static Method m38;
> private static Method m12;
>
> public final boolean helpCommand() throws {
> try {
> return (Boolean)super.h.invoke(this, m29, (Object[])null);
> } catch (RuntimeException | Error var2) {
> throw var2;
> } catch (Throwable var3) {
> throw new UndeclaredThrowableException(var3);
> }
> }
>
>
> Notice the difference in the order of fields and `helpCommand` method is
> mapped to a different field name in both classes. This happens because
> the method array returned by `getMethods` is not sorted in any particular
> order
> <https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/Class.java#L2178>
> when generating a proxy class. What dictates this order? And why is it
> not deterministic?
>
>
> Regards,
> Aman Sharma
>
> PhD Student
> KTH Royal Institute of Technology
> School of Electrical Engineering and Computer Science (EECS)
> Department of Theoretical Computer Science (TCS)
> <http://www.kth.se> <https://www.kth.se/profile/amansha>
> <https://www.kth.se/profile/amansha>
> <https://www.kth.se/profile/amansha>https://algomaster99.github.io/
> --
> *From:* Aman Sharma
> *Sent:* Wednesday, May 22, 2024 4:12:19 PM
> *To:* Chen Liang
> *Cc:* David Holmes; core-libs-dev@openjdk.org; leyden-...@openjdk.org
> *Subject:* Re: Deterministic naming of subclasses of
> `java/lang/reflect/Proxy`
>
>
> Hi Chen,
>
>
> That's clear. Thanks for letting me know. I guess then Project Leyden is
> working on naming the hidden classes deterministically to achieve their
> goals <https://openjdk.org/projects/leyden/notes/01-beginnings>.
>
>
> Regards,
> Aman Sharma
>
> PhD Student
> KTH Royal Institute of Technology
> School of Electrical Engineering and Computer Science (EECS)
> Department of Theoretical Computer Science (TCS)
> <http://www.kth.se> <https://www.kth.se/profile/amansha>
> <https://www.kth.se/profile/amansha>
> <https://www.kth.se/profile/amansha>https://algomaster99.github.io/
> --
> *From:* Chen Liang
> *Sent:* Wednesday, May 22, 2024 1:35:46 PM
> *To:* Aman Sharma
> *Cc:* David Holmes; core-libs-dev@openjdk.org; leyden-...@openjdk.org
> *Subject:* Re: Deterministic naming of subclasses of
> `java/lang/reflect/Proxy`
>
>
Re: RFR: 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull [v2]
> I propose to add type-checked ConstantPool.entryByIndex and > ClassReader.readEntryOrNull taking an extra Class parameter, which throws > ConstantPoolException instead of ClassCastException on type mismatch, which > can happen to malformed ClassFiles. > > Requesting a review from @asotona. Thanks! > > Proposal on mailing list: > https://mail.openjdk.org/pipermail/classfile-api-dev/2024-May/000512.html Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into feature/entry-by-type - Use constants, beautify code - 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull - Changes: - all: https://git.openjdk.org/jdk/pull/19330/files - new: https://git.openjdk.org/jdk/pull/19330/files/5a45e511..c9f6fc18 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19330=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19330=00-01 Stats: 1517 lines in 58 files changed: 777 ins; 404 del; 336 mod Patch: https://git.openjdk.org/jdk/pull/19330.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19330/head:pull/19330 PR: https://git.openjdk.org/jdk/pull/19330
Re: GC triggered before VM initialization completed
Hi Lorris, This mailing list is for Java's core libraries' development. Your issue is a support request and is not related to Java's core libraries in any way, thus I recommend you look for tech support or create/find an issue at bugs.java.com. Regards, Chen On Wed, May 22, 2024 at 7:16 AM Lorris wrote: > Hello, > I get an error when I try to run the java or javac commands on any file. > The exact message is the following: > > *Error occurred during initialization of VM* > *GC triggered before VM initialization completed. Try increasing NewSize, > current value 1331K.* > > I tried to increase the size up to 6G but it did not solve the problem. I > am using a modified version of the opendjdk-22.0.1 (3 commits behind the > master branch as I write this email). This version generates extra fields > and methods to propagate parameterised types information. I am totally > aware that the problem comes deep down from the version that I am using, > but as the error does not provide any other information, I cannot figure > out the origin of the problem. I also added at the end of this email the > verbose trace of the java command (it always blocks at the same point, > regardless of the allocated size), if it can be useful in any way. > > Lorris Creantor > > *[0.013s][info][class,load] path: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.013s][info][class,load] path: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.014s][info][class,load] java.lang.Object source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.014s][info][class,load] java.io.Serializable source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.015s][info][class,load] java.lang.Comparable source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.015s][info][class,load] java.lang.CharSequence source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.015s][info][class,load] java.lang.constant.Constable source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.015s][info][class,load] java.lang.constant.ConstantDesc source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.015s][info][class,load] java.lang.String source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.016s][info][class,load] java.lang.reflect.AnnotatedElement source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.016s][info][class,load] java.lang.reflect.GenericDeclaration source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.016s][info][class,load] java.lang.reflect.Type source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.017s][info][class,load] java.lang.invoke.TypeDescriptor source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.017s][info][class,load] java.lang.invoke.TypeDescriptor$OfField > source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.017s][info][class,load] java.lang.Class source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.017s][info][class,load] java.lang.Cloneable source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.018s][info][class,load] java.lang.ClassLoader source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.018s][info][class,load] java.lang.System source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.018s][info][class,load] java.lang.Throwable source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.018s][info][class,load] java.lang.Error source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.019s][info][class,load] java.lang.Exception source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.019s][info][class,load] java.lang.RuntimeException source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.019s][info][class,load] java.lang.SecurityManager source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.019s][info][class,load] java.security.ProtectionDomain source: > /Users/lorris/IdeaProjects/jdk/build/macosx-aarch64-server-release/jdk/modules/java.base* > *[0.020s][info][class,load] java.security.AccessControlContext
Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`
bit confused since I saw somewhere that the names of > >> hidden classes are null. But thanks for clarifying here. > > > > The JEP clearly defines the name format for hidden classes - though the > > final component is VM specific (and typically a hashcode). > > > > https://openjdk.org/jeps/371 <https://openjdk.org/jeps/371> > > > > Cheers, > > David > > - > > > >> > avoid dynamic class loading > >> > >> I don't see dynamic class loading as a problem. I only mind some > >> unstable generation aspects of them which make it hard to verify them > >> based on an allowlist. > >> > >> For example, if this hidden class is generated with the exact same name > >> and the exact same bytecode during runtime as well, it would be easy to > >> verify it. However, I do see the names are based on some sort of memory > >> address so and I don't know what bytecode it has so I don't have > >> suggestions to make them stable as of now. For Proxy classes, I feel it > >> can be addressed unless you disagree or some involved in Project Leyden > >> does. :) Thank you for forwarding my mail there. > >> > >> Regards, > >> Aman Sharma > >> > >> PhD Student > >> KTH Royal Institute of Technology > >> https://algomaster99.github.io/ <https://algomaster99.github.io/> > > <https://algomaster99.github.io/ <https://algomaster99.github.io/>> > >> > >> > >> *From:* liangchenb...@gmail.com > >> *Sent:* Friday, May 17, 2024 1:23:58 pm > >> *To:* Aman Sharma > >> *Cc:* core-libs-dev@openjdk.org ; > >> leyden-...@openjdk.org > >> *Subject:* Re: Deterministic naming of subclasses of > >> `java/lang/reflect/Proxy` > >> > >> Hi Aman, > >> For `-verbose:class`, it's a JVM argument instead of a program > argument; > >> so when you run a java program like `java Main`, you should call it as > >> `java -verbose:class Main`. > >> When done correctly, you should see hidden class outputs like: > >> [0.032s][info][class,load] > >> java.lang.invoke.LambdaForm$MH/0x0200cc000400 source: > >> __JVM_LookupDefineClass__ > >> The loading of java.lang.invoke hidden classes requires your program to > >> use MethodHandle features, like a lambda. > >> > >> I think the problem you are exploring, that to avoid dynamic class > >> loading and effectively turn Java Platform closed for security, is also > >> being accomplished by project Leyden (as I've shared initially); Thus, > I > >> am forwarding this to leyden-dev instead, so you can see what approach > >> Leyden uses to accomplish the same goal as yours. > >> > >> Regards, Chen Liang > >> > >> On Fri, May 17, 2024 at 4:40 AM Aman Sharma >> <mailto:aman...@kth.se <mailto:aman...@kth.se >>> > wrote: > >> > >> __ > >> > >> Hi Roger, > >> > >> > >> Do you have ideas on how to intercept them? My javaagent is not able > >> to nor a JVMTI agent passed using `agentpath` option. It also does > >> not seem to show up in logs when I pass `-verbose:class`. > >> > >> > >> Also, what do you think of renaming the proxy classes as suggested > >> below? > >> > >> > >> Regards, > >> Aman Sharma > >> > >> PhD Student > >> KTH Royal Institute of Technology > >> School of Electrical Engineering and Computer Science (EECS) > >> Department of Theoretical Computer Science (TCS) > >> <http://www.kth.se><https://www.kth.se/profile/amansha>< > https://www.kth.se/profile/amansha < > http://www.kth.se><https://www.kth.se/profile/amansha><https://www.kth.se/profile/amansha > >> > >> <https://www.kth.se/profile/amansha > > <https://www.kth.se/profile/amansha>>https://algomaster99.github.io/ > >> <https://algomaster99.github.io/ <https://algomaster99.github.io/>> > >> > > >> *From:* core-libs-dev >> <mailto:core-libs-dev-r...@openjdk.org > > <mailto:core-libs-dev-r...@openjdk.org >>> > on behalf of Roger Riggs > >> mailto:roger.ri...@oracle.com < > mailto:roger.ri...@oracle.co
RFR: 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull
I propose to add type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull taking an extra Class parameter, which throws ConstantPoolException instead of ClassCastException on type mismatch, which can happen to malformed ClassFiles. Requesting a review from @asotona. Thanks! Proposal on mailing list: https://mail.openjdk.org/pipermail/classfile-api-dev/2024-May/000512.html - Commit messages: - 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull Changes: https://git.openjdk.org/jdk/pull/19330/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19330=00 Issue: https://bugs.openjdk.org/browse/JDK-8332614 Stats: 183 lines in 14 files changed: 103 ins; 30 del; 50 mod Patch: https://git.openjdk.org/jdk/pull/19330.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19330/head:pull/19330 PR: https://git.openjdk.org/jdk/pull/19330
