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. Whether increments are handled or not, I think the `StringUTF16.putChar` intrinsic might be getting in the way here and prevent merging consecutive "char" stores into 4 or 8 byte stores. It would be interesting to get numbers for the `putChar` version with the intrinsic disabled (`-XX:+UnlockDiagnosticVMOptions -XX:DisableIntrinsic=_putCharStringU`). - PR Comment: https://git.openjdk.org/jdk/pull/19626#issuecomment-2159509806
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 I'm wary about the subtle semantic differences that might result from that and would be more comfortable reverting this behavior to what we did before JDK-8332457. Perhaps even backing that one out and re-examine the changes with less urgency. - PR Comment: https://git.openjdk.org/jdk/pull/19615#issuecomment-2158280708
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 Original code did all the classloading in the `` - should we do that here, too? Can you explain the need to `getPlatformClassLoader` when `classEntry.getClassLoader == null`? Pre-JDK-8332457 did no such thing. I'll park #19598 until this stabilizes. - PR Comment: https://git.openjdk.org/jdk/pull/19615#issuecomment-2158189721
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 Error Test 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 Thanks, I'll see if I can get that working and if it has any effect as a follow-up. Testing caught a bug introduced by a copy-paste error in 532966a, re-running a few more tests now. - PR Comment: https://git.openjdk.org/jdk/pull/19598#issuecomment-2157922124
Re: RFR: 8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator [v3]
> 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: Copy-paste error; CONDY_INVOKE returns Object. Fixes TestDynamicConstant.java - Changes: - all: https://git.openjdk.org/jdk/pull/19598/files - new: https://git.openjdk.org/jdk/pull/19598/files/532966a4..2d64ae1c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19598=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19598=01-02 Stats: 5 lines in 1 file changed: 0 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19598.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19598/head:pull/19598 PR: https://git.openjdk.org/jdk/pull/19598
Integrated: 8333824: Unused ClassValue in VarHandles
On Mon, 10 Jun 2024 08:30:59 GMT, Claes Redestad wrote: > Trivially removing the a leftover, unused `ClassValue` from > `java.lang.invoke.VarHandle` This pull request has now been integrated. Changeset: 7b43a8cd Author: Claes Redestad URL: https://git.openjdk.org/jdk/commit/7b43a8cd7c663facbe490f889838d7ead0eba0f9 Stats: 10 lines in 1 file changed: 0 ins; 10 del; 0 mod 8333824: Unused ClassValue in VarHandles Reviewed-by: mcimadamore - PR: https://git.openjdk.org/jdk/pull/19620
Re: RFR: 8333824: Unused ClassValue in VarHandles
On Mon, 10 Jun 2024 08:57:23 GMT, Maurizio Cimadamore wrote: > Good catch! Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/19620#issuecomment-2157910277
RFR: 8333824: Unused ClassValue in VarHandles
Trivially removing the a leftover, unused `ClassValue` from `java.lang.invoke.VarHandle` - Commit messages: - 8333824: Unused ClassValue in VarHandles Changes: https://git.openjdk.org/jdk/pull/19620/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19620=00 Issue: https://bugs.openjdk.org/browse/JDK-8333824 Stats: 10 lines in 1 file changed: 0 ins; 10 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19620.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19620/head:pull/19620 PR: https://git.openjdk.org/jdk/pull/19620
Re: RFR: 8333833: Remove the use of ByteArrayLittleEndian from UUID::toString [v7]
On Mon, 10 Jun 2024 07:32: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: > > Update src/java.base/share/classes/jdk/internal/util/HexDigits.java > > Co-authored-by: Claes Redestad Great, go ahead and /integrate again and I'll sponsor. - PR Comment: https://git.openjdk.org/jdk/pull/19610#issuecomment-2157627942
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 Error Test 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 Sure. If you can show how you'd do that I'd be happy to test it. Maybe there are excess native linkers I'm not seeing from using `invokeExact`, but AFAICT there are no linkers being spun when the type match is made exact via static casts (which is what these exact matches in BMI achieves). So I remain a bit confused about what we would be able to improve. Thanks for reviewing! - PR Comment: https://git.openjdk.org/jdk/pull/19598#issuecomment-2157595699
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v7]
On Mon, 10 Jun 2024 07:32: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: > > Update src/java.base/share/classes/jdk/internal/util/HexDigits.java > > Co-authored-by: Claes Redestad Thanks. I hate to nitpick, but is it OK if I rename the RFE as "Remove the use of ByteArrayLittleEndian from UUID::toString" (the PR need to follow suit). I think the current name might be read as doing something completely different. - PR Comment: https://git.openjdk.org/jdk/pull/19610#issuecomment-2157580938
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
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: * only least significant 16 bits of {@code i} are used.
Suggestion:
* only least significant 16 bits of {@code value} are used.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19610#discussion_r1632711983
Re: RFR: 8333833: UUID toString removes the use of ByteArrayLittleEndian [v5]
On Sun, 9 Jun 2024 07:22: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 two additional
> commits since the last revision:
>
> - rename putHex4 to put4
> - update copy right year
Glad to see #16245 in action, enabling simpler code with equal or better
performance.
src/java.base/share/classes/jdk/internal/util/HexDigits.java line 123:
> 121: * @param i to convert
> 122: */
> 123: public static void put4(byte[] buffer, int off, int i) {
Perhaps `index` and `value` are better names than `off` and `i`, respectively.
-
Marked as reviewed by redestad (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19610#pullrequestreview-2106355754
PR Review Comment: https://git.openjdk.org/jdk/pull/19610#discussion_r1632380045
Re: RFR: 8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator [v2]
On Fri, 7 Jun 2024 19:22:43 GMT, Chen Liang wrote: >> 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. I am sure there's a perfectly good explanation why we've done it like that rather than translating things like javac would (AFAICT a load from `Integer/Short/Byte/...TYPE`). - PR Review Comment: https://git.openjdk.org/jdk/pull/19598#discussion_r1632041009
Re: RFR: 8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator
On Fri, 7 Jun 2024 13:46:22 GMT, Jorn Vernee wrote: > As an aside: I suppose we pre-generate all the linkers that get spun up for > these `invokeExact`calls already. But, we might still get a little bit of > mileage out of switching to `invokeBasic`, which bypasses the linkers > altogether. With the caveat that we have to be very careful that the call > site type is correct. I'm not sure I see what linkers you're alluding to here in profiles (and I don't think we have code pre-generating all of them), and I don't see how to use `invokeBasic` here instead. Care to elaborate? - PR Comment: https://git.openjdk.org/jdk/pull/19598#issuecomment-2155357245
Re: RFR: 8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator [v2]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/19598/files - new: https://git.openjdk.org/jdk/pull/19598/files/21f945b5..532966a4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19598=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19598=00-01 Stats: 29 lines in 1 file changed: 17 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/19598.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19598/head:pull/19598 PR: https://git.openjdk.org/jdk/pull/19598
Re: RFR: 8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator
On Fri, 7 Jun 2024 18:38:52 GMT, ExE Boss wrote:
>> 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.)
>
> 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19598#discussion_r1631578674
Re: RFR: 8333774: Avoid eagerly loading various EmptySpliterator classes [v3]
On Fri, 7 Jun 2024 13:08: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 one additional > commit since the last revision: > > Update src/java.base/share/classes/java/util/Spliterators.java > > Co-authored-by: Chen Liang Thanks! Yes, the best holder for a singleton is typically the class itself - PR Comment: https://git.openjdk.org/jdk/pull/19591#issuecomment-2155003099
Integrated: 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. This pull request has now been integrated. Changeset: d744059b Author: Claes Redestad URL: https://git.openjdk.org/jdk/commit/d744059b5b3e944bee53536de6f404666e45e8e5 Stats: 33 lines in 1 file changed: 12 ins; 12 del; 9 mod 8333774: Avoid eagerly loading various EmptySpliterator classes Reviewed-by: liach, pminborg - PR: https://git.openjdk.org/jdk/pull/19591
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 Good incremental improvements. - Marked as reviewed by redestad (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19585#pullrequestreview-2104694382
Re: RFR: 8333774: Avoid eagerly loading various EmptySpliterator classes [v3]
> 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 one additional commit since the last revision: Update src/java.base/share/classes/java/util/Spliterators.java Co-authored-by: Chen Liang - Changes: - all: https://git.openjdk.org/jdk/pull/19591/files - new: https://git.openjdk.org/jdk/pull/19591/files/9d19bbe1..10611afd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19591=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19591=01-02 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19591.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19591/head:pull/19591 PR: https://git.openjdk.org/jdk/pull/19591
Re: RFR: 8333774: Avoid eagerly loading various EmptySpliterator classes [v2]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/19591/files - new: https://git.openjdk.org/jdk/pull/19591/files/5eb2ddd8..9d19bbe1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19591=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19591=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19591.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19591/head:pull/19591 PR: https://git.openjdk.org/jdk/pull/19591
RFR: 8333793: Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator
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 - Commit messages: - Rename microbenchmark - Ensure proxying for hashCode etc is 'implemented' - Ensure proxying for hashCode etc is 'implemented' - Change ProxyGenBench to provoke proxy condy - Improve BootstrapMethodInvoker for ConstantBootstraps and ProxyGenerator Changes: https://git.openjdk.org/jdk/pull/19598/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19598=00 Issue: https://bugs.openjdk.org/browse/JDK-8333793 Stats: 89 lines in 2 files changed: 35 ins; 24 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/19598.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19598/head:pull/19598 PR: https://git.openjdk.org/jdk/pull/19598
Re: RFR: 8332249: Micro-optimize Method.hashCode [v2]
On Wed, 5 Jun 2024 13:45:46 GMT, Per Minborg wrote: >> As a note, If we would have access to the contemplated `StableValue` and >> `hash` was declared even as an _instance variable_ `StableValue` in >> a regular class, then it would be trusted and would be eligible for constant >> folding due to the VM will have special rules for fields of StableValue >> type. > > Ahh. There was a benchmark in the initial message. Sorry. We think this is caused by the membars being conservatively emitted at method exit, and more precis membar control should help ARM. Filed: https://bugs.openjdk.org/browse/JDK-8333791 - PR Review Comment: https://git.openjdk.org/jdk/pull/19433#discussion_r1631071572
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v19]
On Wed, 5 Jun 2024 12:39:13 GMT, Chen Liang wrote: >> 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 That'd arguably be a bit cleaner. Might allocate an extra MTD (unless/until EA kicks in), but that might not matter really. - PR Review Comment: https://git.openjdk.org/jdk/pull/19410#discussion_r1630868455
RFR: 8333774: Avoid eagerly loading various EmptySpliterator classes
Trivially move a few private static finals to their respective source type to avoid eagerly loading a few small classes. - Commit messages: - 8333774: Avoid eagerly loading various EmptySpliterator classes Changes: https://git.openjdk.org/jdk/pull/19591/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19591=00 Issue: https://bugs.openjdk.org/browse/JDK-8333774 Stats: 31 lines in 1 file changed: 12 ins; 11 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/19591.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19591/head:pull/19591 PR: https://git.openjdk.org/jdk/pull/19591
Re: RFR: 8333749: Consolidate ConstantDesc conversion in java.base [v3]
On Thu, 6 Jun 2024 19:12:35 GMT, Chen Liang wrote: >> 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. Created a PR you could fold into this, if you'd like: https://github.com/liachmodded/jdk/pull/1 - PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630816783
Re: RFR: 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. Looks good to me. Probably should get someone to OK changes in foreign/abi (@JornVernee perhaps?). 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. src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 256: > 254: // the field name holding the method handle for this method > 255: String fieldName = "m" + count++; > 256: var mt = methodDesc(m.getReturnType(), > JLRA.getExecutableSharedParameterTypes(m)); Suggestion: var md = methodDesc(m.getReturnType(), JLRA.getExecutableSharedParameterTypes(m)); 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..? 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? - Marked as reviewed by redestad (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19585#pullrequestreview-2102953208 PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630049028 PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630061889 PR Review Comment: https://git.openjdk.org/jdk/pull/19585#discussion_r1630074187
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 Looks good. Generation of proxy classes gets a nice boost this way. The condy bsm calls that may happen later takes a small hit which we can improve in a follow-up. - Marked as reviewed by redestad (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19410#pullrequestreview-2098990024
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v14]
On Mon, 3 Jun 2024 11:09:31 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: > > ProxyGenBench simplification test/micro/org/openjdk/bench/java/lang/reflect/Proxy/ProxyGenBench.java line 23: > 21: * questions. > 22: */ > 23: package org.openjdk.bench.java.lang.reflect.Proxy; Package name needs to be lowercase. Not sure why the folder name is uppercase Proxy, but the two pre-existing benchmarks both have lower case package declarations. Uppercase letters in package names may subtly break a few tools - PR Review Comment: https://git.openjdk.org/jdk/pull/19410#discussion_r1624270828
Re: RFR: 8332457: Examine startup overheads from JDK-8294961 [v13]
On Mon, 3 Jun 2024 10:11:34 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: > > added ProxyGenBench JMH micro benchmark test/micro/org/openjdk/bench/java/lang/reflect/Proxy/ProxyGenBench.java line 66: > 64: ClassDesc tempDesc = > ClassDesc.ofDescriptor(Interfaze.class.descriptorString()); > 65: loader = new ClsLoader(); > 66: clsMap = new HashMap<>(100); Suggestion: clsMap = HashMap.newHashMap(100); - PR Review Comment: https://git.openjdk.org/jdk/pull/19410#discussion_r1624192732
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. Vicente filed a bug on javac to investigate this: https://bugs.openjdk.org/browse/JDK-8333278 I wouldn't mind using condy for constant aka non-capturing lambdas. I recall we had a prototype for that years ago, but switching over was shelved for some reason. - PR Comment: https://git.openjdk.org/jdk/pull/19477#issuecomment-2139703977
Re: RFR: 8333265: De-duplicate method references in java.util.stream.FindOps
On Thu, 30 May 2024 13:04:46 GMT, Chen Liang wrote: > Should we extract them to private static utility methods for potential > laziness support in the future? Ideally we shouldn't have to do any of this. I thought such method references were already de-duplicated in javac - at least within the same compilation units - but I saw this in a startup profile and was surprised myself that the demonstrated manual de-duplication has an observable effect. - PR Comment: https://git.openjdk.org/jdk/pull/19477#issuecomment-2139576911
RFR: 8333265: De-duplicate method references in java.util.stream.FindOps
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. - Commit messages: - De-duplicate identical lambdas in FindOps Changes: https://git.openjdk.org/jdk/pull/19477/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19477=00 Issue: https://bugs.openjdk.org/browse/JDK-8333265 Stats: 35 lines in 2 files changed: 17 ins; 7 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/19477.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19477/head:pull/19477 PR: https://git.openjdk.org/jdk/pull/19477
Re: RFR: 8333103: Re-examine the console provider loading
On Thu, 30 May 2024 10:02:25 GMT, Alan Bateman wrote:
> Would it be possible to provide more context/background here? This is not
> code that is used during startup. Is there benchmark data to share for first
> use of java.io.IO ?
I think this was brought to the fore by https://openjdk.org/jeps/477 where
running the example implicit main:
void main() {
println("Hello, World!");
}
.. brings in this code. This PR is one of several to try and smoothen a few
rough edges that makes the startup of that quite a bit heavier than the
baseline non-implicit Hello World.
-
PR Comment: https://git.openjdk.org/jdk/pull/19467#issuecomment-2139218517
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Wed, 29 May 2024 09:18:51 GMT, Pavel Rappo wrote: >> The non-constant test was added because that very bailout caused a crash. >> The other test is actually less interesting since it'll likely be covered >> indirectly by regular use. But as we are hiding these away this gets ever >> more obscure and perhaps the test could be dropped entirely. > > @cl4es, do you want me to delete that test file altogether? I thought you verified that the non-constant type test still provoke a crash (on x86) if you back out the code changes in https://github.com/openjdk/jdk/commit/969f6a37e4649079c7acea1952f5537fd9ba2f0a ? If so that test is still somewhat useful to guard against future coding mistakes by verifying that the bail out doesn't mess things up. The constant type tests have less utility, perhaps. I'd keep it as is unless there's a strong desire to reduce test runtime (these should be pretty quick). - PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1620259115
Re: RFR: 8333103: Re-examine the console provider loading
On Wed, 29 May 2024 22:25:09 GMT, Naoto Sato wrote: >> Yes, `break` guarantees that the search completes one way or another once >> the module name has been matched. This is not how it used to be done. > > Right. Since `findAny` is after the module name matching, there is at most 1 > match. In the case we didn't find any, the final `orElse(null)` eventually > returns null. So the refactored code is semantically the same. It's only semantically the same if we assume a module can only provide a single `JdkConsoleProvider`, no? The `break;` disallows multiple providers (for disjoint sets of charsets) in a single module. - PR Review Comment: https://git.openjdk.org/jdk/pull/19467#discussion_r1620164006
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. Marked as reviewed by redestad (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19467#pullrequestreview-2086537783
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/62348071...942342d5 The condy bootstrapping adds a little noise when running this through an affected startup benchmark. Overall the net impact is small or even negative. This is a nice cleanup, though, but perhaps we ought to keep JDK-8332457 open (or file a new bug to keep investigating overheads). - PR Comment: https://git.openjdk.org/jdk/pull/19410#issuecomment-2137310415
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 redestad (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19414#pullrequestreview-2084714609
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Wed, 29 May 2024 03:16:02 GMT, Chen Liang wrote: >> 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. Ah, sneaky. Might affect scores for the zero and one-char cases since the shift now happens unconditionally, but probably doesn't matter in practice. - PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1618456668
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Tue, 28 May 2024 19:13:30 GMT, Jorn Vernee wrote:
>> 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 275:
>
>> 273: return switch (length) {
>> 274: case 0 -> initialValue;
>> 275: case 1 -> 31 * initialValue + (a[fromIndex] & 0xff);
>
> For clarity, if you think it helps:
> Suggestion:
>
> case 1 -> 31 * initialValue + Byte.toUnsignedInt(a[fromIndex]);
I don't care as long as microbenchmarks don't get a hiccup.
> src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 301:
>
>> 299: return switch (length) {
>> 300: case 0 -> initialValue;
>> 301: case 1 -> 31 * initialValue + JLA.getUTF16Char(a,
>> fromIndex);
>
> There seems to be a mismatch here with the original code in StringUTF16,
> where the length that is tested for is `2` instead of `1`.
Yes, should be `2` (`a` is semantically a `char[]`). This typo likely pass
functional testing since `1` can never happen in practice, and the default case
should work for any value. There might be a String microbenchmark out there
that might be slightly unhappy, though.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617867797
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617865658
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Tue, 28 May 2024 19:19:51 GMT, Jorn Vernee wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix incorrect utf16 hashCode adaptation
>
> test/hotspot/jtreg/compiler/intrinsics/TestArraysHashCode.java line 88:
>
>> 86: private static int testIntrinsic(byte[] bytes, int type)
>> 87: throws InvocationTargetException, IllegalAccessException {
>> 88: return (int) vectorizedHashCode.invoke(null, bytes, 0, 256, 1,
>> type);
>
> Better to just call `hashCodeOfUnsigned` here I think.
>
> The test for the non-constant type could be dropped. That is no longer a part
> of the 'API' of `ArraySupport`. It looks like the intrinsic bails out when
> the basic type is not constant any ways:
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/library_call.cpp#L6401-L6404
The non-constant test was added because that very bailout caused a crash. The
other test is actually less interesting since it'll likely be covered
indirectly by regular use. But as we are hiding these away this gets ever more
obscure and perhaps the test could be dropped entirely.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617848032
Re: RFR: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations [v10]
On Fri, 24 May 2024 08:24:15 GMT, Adam Sotona wrote: >> Hi, >> During performance optimization work on Class-File API as JDK lambda >> generator we found some static initialization killers. >> One of them is `java.lang.classfile.Attributes` with tens of static fields >> initialized with individual attribute mappers, and common set of all >> mappers, and static map from attribute names to the mappers. >> >> I propose to turn all the static fields into lazy-initialized static methods >> and remove `PREDEFINED_ATTRIBUTES` and `standardAttribute(Utf8Entry name)` >> static mapping method from the `Attributes` API class. >> >> Please let me know your comments or objections and please review the >> [PR](https://github.com/openjdk/jdk/pull/19006) and >> [CSR](https://bugs.openjdk.org/browse/JDK-8331414), so we can make it into >> 23. >> >> Thank you, >> 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 16 commits: > > - fixed jdeps.Dependencies > - Merge branch 'master' into JDK-8331291-attributes > - addressed CSR review comments > - fixed CompilationIDMapper does not allow multiple instances > - fixed tests > - fixed tests > - fixed tests > - updated LimitsTest > - Merge branch 'master' into JDK-8331291-attributes > ># Conflicts: ># test/jdk/jdk/classfile/SignaturesTest.java > - Merge branch 'master' into JDK-8331291-attributes > - ... and 6 more: https://git.openjdk.org/jdk/compare/239c1b33...37f7f63f Looks good after revisions. - Marked as reviewed by redestad (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19006#pullrequestreview-2076508421
Integrated: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations
On Mon, 20 May 2024 10:52:27 GMT, Claes Redestad wrote: > We can fold the call to `Objects.checkIndex` into the code generated in > generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. > This loads 9 less classes (of which 8 generated LFs and Species classes) on a > minimal test, while being neutral on a throughput sanity test: > > > Name Cnt Base Error Test Error Unit Change > SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = > 0,800 ) > * = significant > ``` > > A few additional optimizations includes moving some seldom used `findStatic` > calls to a holder. All in all this means a reduction by 22M cycles to > bootstrap a trivial switch expression on my M1. This pull request has now been integrated. Changeset: 2581935b Author:Claes Redestad URL: https://git.openjdk.org/jdk/commit/2581935b47afaf661a94c8a8e50ce08065d632f6 Stats: 165 lines in 4 files changed: 125 ins; 14 del; 26 mod 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations Reviewed-by: liach, jlahoda - PR: https://git.openjdk.org/jdk/pull/19307
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations [v4]
On Thu, 23 May 2024 11:09:14 GMT, Claes Redestad wrote: >> We can fold the call to `Objects.checkIndex` into the code generated in >> generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. >> This loads 9 less classes (of which 8 generated LFs and Species classes) on >> a minimal test, while being neutral on a throughput sanity test: >> >> >> Name Cnt Base Error Test Error Unit Change >> SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = >> 0,800 ) >> * = significant >> ``` >> >> A few additional optimizations includes moving some seldom used `findStatic` >> calls to a holder. All in all this means a reduction by 22M cycles to >> bootstrap a trivial switch expression on my M1. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove unused import, revert left-overs in MethodTypeDescImpl Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/19307#issuecomment-2126979932
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations [v3]
On Thu, 23 May 2024 09:46:51 GMT, Jan Lahoda wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add type switch to HelloClasslist > > src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 53: > >> 51: import java.lang.classfile.instruction.SwitchCase; >> 52: >> 53: import jdk.internal.constant.MethodTypeDescImpl; > > Nit - this import seems to be unused, and neither seem to be the changes to > `MethodTypeDescImpl`. Is there some use missing? (OTOH, I like the > `StaticHolders` for the stuff that relates to the enum-switch special case, > for the time being.) Oops, some leftovers from the optimizations I reverted. Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/19307#discussion_r1611478359
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations [v4]
> We can fold the call to `Objects.checkIndex` into the code generated in > generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. > This loads 9 less classes (of which 8 generated LFs and Species classes) on a > minimal test, while being neutral on a throughput sanity test: > > > Name Cnt Base Error Test Error Unit Change > SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = > 0,800 ) > * = significant > ``` > > A few additional optimizations includes moving some seldom used `findStatic` > calls to a holder. All in all this means a reduction by 22M cycles to > bootstrap a trivial switch expression on my M1. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Remove unused import, revert left-overs in MethodTypeDescImpl - Changes: - all: https://git.openjdk.org/jdk/pull/19307/files - new: https://git.openjdk.org/jdk/pull/19307/files/f04d78ea..eb4babe5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19307=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19307=02-03 Stats: 3 lines in 2 files changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19307.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19307/head:pull/19307 PR: https://git.openjdk.org/jdk/pull/19307
Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM [v2]
On Wed, 22 May 2024 07:49:27 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change for addressing >> https://bugs.openjdk.org/browse/JDK-8332490? >> >> The jmh test opens a `InflaterInputStream`, reads the stream contents, but >> then doesn't close the stream. This can lead to resource leak which can then >> cause OOM as noted in that JBS issue. >> >> The commit in this PR closes the `InflaterInputStream` when the reads >> complete. >> >> >> make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams >> >> and >> >> >> ./build/macosx-aarch64/images/jdk/bin/java -jar >> ./build/macosx-aarch64/images/test/micro/benchmarks.jar >> org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead >> -t max -f 1 -wi 2 -foe true >> >> pass after this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > add a comment Sorry, thought I had already approved. Comment is fine. - Marked as reviewed by redestad (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19340#pullrequestreview-2072382952
Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM
On Wed, 22 May 2024 07:20:04 GMT, Alan Bateman wrote:
>> Can I please get a review of this test-only change for addressing
>> https://bugs.openjdk.org/browse/JDK-8332490?
>>
>> The jmh test opens a `InflaterInputStream`, reads the stream contents, but
>> then doesn't close the stream. This can lead to resource leak which can then
>> cause OOM as noted in that JBS issue.
>>
>> The commit in this PR closes the `InflaterInputStream` when the reads
>> complete.
>>
>>
>> make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams
>>
>> and
>>
>>
>> ./build/macosx-aarch64/images/jdk/bin/java -jar
>> ./build/macosx-aarch64/images/test/micro/benchmarks.jar
>> org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead
>> -t max -f 1 -wi 2 -foe true
>>
>> pass after this change.
>
> test/micro/org/openjdk/bench/java/util/zip/InflaterInputStreams.java line 113:
>
>> 111: try (InflaterInputStream iis = new
>> InflaterInputStream(deflated)) {
>> 112: while (iis.read(inflated, 0, inflated.length) != -1);
>> 113: }
>
> Presumably this only works because closing the underlying stream (a BAOS in
> this case) is a no-op.
Yes, the underlying BAIS can be repeatedly closed without affecting the
benchmark.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19340#discussion_r1609436837
Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM
On Wed, 22 May 2024 05:16:42 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change for addressing > https://bugs.openjdk.org/browse/JDK-8332490? > > The jmh test opens a `InflaterInputStream`, reads the stream contents, but > then doesn't close the stream. This can lead to resource leak which can then > cause OOM as noted in that JBS issue. > > The commit in this PR closes the `InflaterInputStream` when the reads > complete. > > > make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams > > and > > > ./build/macosx-aarch64/images/jdk/bin/java -jar > ./build/macosx-aarch64/images/test/micro/benchmarks.jar > org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead > -t max -f 1 -wi 2 -foe true > > pass after this change. LGTM - feel free to add a comment that closing the `InflaterInputStream` has no effect on the underlying stream `deflated`. - PR Review: https://git.openjdk.org/jdk/pull/19340#pullrequestreview-2070357036
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations [v3]
On Tue, 21 May 2024 09:01:32 GMT, Claes Redestad wrote: >> We can fold the call to `Objects.checkIndex` into the code generated in >> generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. >> This loads 9 less classes (of which 8 generated LFs and Species classes) on >> a minimal test, while being neutral on a throughput sanity test: >> >> >> Name Cnt Base Error Test Error Unit Change >> SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = >> 0,800 ) >> * = significant >> ``` >> >> A few additional optimizations includes moving some seldom used `findStatic` >> calls to a holder. All in all this means a reduction by 22M cycles to >> bootstrap a trivial switch expression on my M1. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add type switch to HelloClasslist [f04d78e](https://github.com/openjdk/jdk/pull/19307/commits/f04d78ea53ed0074026311f82eb0d4eafee3438d) passed tier1-3 testing - PR Comment: https://git.openjdk.org/jdk/pull/19307#issuecomment-2122519106
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations [v3]
On Tue, 21 May 2024 09:01:32 GMT, Claes Redestad wrote: >> We can fold the call to `Objects.checkIndex` into the code generated in >> generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. >> This loads 9 less classes (of which 8 generated LFs and Species classes) on >> a minimal test, while being neutral on a throughput sanity test: >> >> >> Name Cnt Base Error Test Error Unit Change >> SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = >> 0,800 ) >> * = significant >> ``` >> >> A few additional optimizations includes moving some seldom used `findStatic` >> calls to a holder. All in all this means a reduction by 22M cycles to >> bootstrap a trivial switch expression on my M1. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add type switch to HelloClasslist By adding a trivial type switch to `HelloClasslist` we archive a set of classes and avoid some runtime class generation. This grows the default CDS archive by approximately 1.2Mb by pulling in a large number of Classfile API classes, which is bound to happen soon anyway. On the SwitchSanity test, bootstrap cost drops by another 35M cycles: Wall clock:51.0 ms/op .taskclock:63.5 ms/op .cycles: 220064872 average cycles elapsed .instructions: 544981276 average instructions retired - PR Comment: https://git.openjdk.org/jdk/pull/19307#issuecomment-2122136668
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations [v3]
> We can fold the call to `Objects.checkIndex` into the code generated in > generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. > This loads 9 less classes (of which 8 generated LFs and Species classes) on a > minimal test, while being neutral on a throughput sanity test: > > > Name Cnt Base Error Test Error Unit Change > SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = > 0,800 ) > * = significant > ``` > > A few additional optimizations includes moving some seldom used `findStatic` > calls to a holder. All in all this means a reduction by 22M cycles to > bootstrap a trivial switch expression on my M1. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Add type switch to HelloClasslist - Changes: - all: https://git.openjdk.org/jdk/pull/19307/files - new: https://git.openjdk.org/jdk/pull/19307/files/c212a3d5..f04d78ea Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19307=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19307=01-02 Stats: 11 lines in 1 file changed: 11 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19307.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19307/head:pull/19307 PR: https://git.openjdk.org/jdk/pull/19307
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations [v2]
> We can fold the call to `Objects.checkIndex` into the code generated in > generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. > This loads 9 less classes (of which 8 generated LFs and Species classes) on a > minimal test, while being neutral on a throughput sanity test: > > > Name Cnt Base Error Test Error Unit Change > SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = > 0,800 ) > * = significant > ``` > > A few additional optimizations includes moving some seldom used `findStatic` > calls to a holder. All in all this means a reduction by 22M cycles to > bootstrap a trivial switch expression on my M1. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Revert bf68b1d, tidy up - Changes: - all: https://git.openjdk.org/jdk/pull/19307/files - new: https://git.openjdk.org/jdk/pull/19307/files/bf68b1d2..c212a3d5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19307=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19307=00-01 Stats: 27 lines in 1 file changed: 15 ins; 9 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19307.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19307/head:pull/19307 PR: https://git.openjdk.org/jdk/pull/19307
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations
On Tue, 21 May 2024 00:16:51 GMT, Chen Liang wrote:
>> We can fold the call to `Objects.checkIndex` into the code generated in
>> generateTypeSwitchSkeleton instead of doing so by filtering the MH argument.
>> This loads 9 less classes (of which 8 generated LFs and Species classes) on
>> a minimal test, while being neutral on a throughput sanity test:
>>
>>
>> Name Cnt Base Error Test Error Unit Change
>> SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p =
>> 0,800 )
>> * = significant
>> ```
>>
>> A few additional optimizations includes generating the switch method using
>> the precise type (to avoid the need for an explicitCast adaptation), and
>> moving some seldom used `findStatic` calls to a holder. All in all this
>> means a reduction by 33-34M cycles to bootstrap a trivial switch expression
>> on my M1.
>
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 630:
>
>> 628:.withMethodBody("typeSwitch",
>> 629:
>> MethodTypeDescImpl.ofValidated(ConstantDescs.CD_int,
>> 630:
>> ClassDesc.ofDescriptor(selectorType.descriptorString()),
>
> `selectorType` can be primitive, therefore verification errors arise at
> `aload` in the generated code. This is why tests fail on GitHub actions.
Dang. I had focused on making the `java/lang/runtime` tests pass locally,
thinking that'd be sufficient. Luckily bf68b1d had only a minor benefit.
Backing it out and tidying up a bit gives me these numbers:
Wall clock:65.0 ms/op
.taskclock:75.0 ms/op
.cycles: 255323158 average cycles elapsed
.instructions: 626086160 average instructions retired
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19307#discussion_r1607874407
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations
On Mon, 20 May 2024 18:06:32 GMT, Chen Liang wrote: >> We can fold the call to `Objects.checkIndex` into the code generated in >> generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. >> This loads 9 less classes (of which 8 generated LFs and Species classes) on >> a minimal test, while being neutral on a throughput sanity test: >> >> >> Name Cnt Base Error Test Error Unit Change >> SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = >> 0,800 ) >> * = significant >> ``` >> >> A few additional optimizations includes generating the switch method using >> the precise type (to avoid the need for an explicitCast adaptation), and >> moving some seldom used `findStatic` calls to a holder. All in all this >> means a reduction by 33-34M cycles to bootstrap a trivial switch expression >> on my M1. > > src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 406: > >> 404: cb.iload(RESTART_IDX); >> 405: cb.loadConstant(labelConstants.length + 1); >> 406: cb.invokestatic(CD_Objects, "checkIndex", >> MethodTypeDesc.of(ConstantDescs.CD_int, ConstantDescs.CD_int, >> ConstantDescs.CD_int)); > > We should cache this MethodTypeDesc too. `MethodTypeDesc.of` is actually quite cheap when inputs are `ClassDesc`s. Besides I think the main focus in these bootstraps should be improving the overhead of the first few calls - reduce dependencies, reduce runtime code generation - not treat this code as something that will be run many times over in a hot loop. - PR Review Comment: https://git.openjdk.org/jdk/pull/19307#discussion_r1607207823
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations
On Mon, 20 May 2024 10:52:27 GMT, Claes Redestad wrote: > We can fold the call to `Objects.checkIndex` into the code generated in > generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. > This loads 9 less classes (of which 8 generated LFs and Species classes) on a > minimal test, while being neutral on a throughput sanity test: > > > Name Cnt Base Error Test Error Unit Change > SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = > 0,800 ) > * = significant > ``` > > A few additional optimizations includes generating the switch method using > the precise type (to avoid the need for an explicitCast adaptation), and > moving some seldom used `findStatic` calls to a holder. All in all this means > a reduction by 33-34M cycles to bootstrap a trivial switch expression on my > M1. To help evaluate and diagnose the startup overheads I added a `main` method to the provided `SwitchSanity` micro. This only runs a single invocation and is easy to run using your startup benchmarking script/tool of choice: Name Cnt Base Error Test Error Unit Change Perfstartup-JMH 2075,000 ± 4,455 62,000 ± 6,042 ms/op 1,21x (p = 0,000*) :.cycles 277393501,850 ± 4422757,249 253819501,350 ± 5482340,325 cycles 0,92x (p = 0,000*) :.instructions 701618995,500 ± 3774721,992 622929549,250 ± 3634060,803 instructions 0,89x (p = 0,000*) :.taskclock 82,000 ± 3,564 74,000 ± 4,365 ms 0,90x (p = 0,000*) * = significant - PR Comment: https://git.openjdk.org/jdk/pull/19307#issuecomment-2121179185
RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations
We can fold the call to `Objects.checkIndex` into the code generated in generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. This loads 9 less classes (of which 8 generated LFs and Species classes) on a minimal test, while being neutral on a throughput sanity test: Name Cnt Base Error Test Error Unit Change SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = 0,800 ) * = significant ``` A few additional optimizations includes generating the switch method using the precise type (to avoid the need for an explicitCast adaptation), and moving some seldom used `findStatic` calls to a holder. All in all this means a reduction by 33-34M cycles to bootstrap a trivial switch expression on my M1. - Commit messages: - Remove explicitCastArguments and refactor - Move rarely used findStatics to Holder class - Drive-by desugaring - Move Objects.checkIndex call to code generated by generateTypeSwitch Changes: https://git.openjdk.org/jdk/pull/19307/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19307=00 Issue: https://bugs.openjdk.org/browse/JDK-8332528 Stats: 157 lines in 4 files changed: 113 ins; 18 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/19307.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19307/head:pull/19307 PR: https://git.openjdk.org/jdk/pull/19307
Integrated: 8331724: Refactor j.l.constant implementation to internal package
On Mon, 6 May 2024 14:39:08 GMT, Claes Redestad wrote: > This PR suggests refactoring the implementation classes of java.lang.constant > into a new package jdk.internal.constant to enable sharing some trusted > static factory methods with users elsewhere in java.base, such as > java.lang.invoke and java.lang.classfile. The refactoring also adds some > "trusted" methods for use when input is pre-validated, and makes use of them > where applicable in the current implementation. There are more changes in the > pipeline which will be folded into #17108 or PR'ed once that is integrated. > > There are two optimizations mixed up here. One in > `MethodTypeDesc.ofDescriptor`: > > Name (descString) > Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I > 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ()V > 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; > 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; > 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (..IIJ)V > 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (.). > 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*) > * = significant > > > The other in `ClassDesc::nested`, where to get rid of a simple static method > in `ConstantUtils` I ended up speeding up and simplifying the public factory > method: > > Name CntBase ErrorTest > Error Unit Change > ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± > 0,573 ns/op 9,53x (p = 0,000*) > * = significant > > > The optimizations could be split out and PRd independently, but I think they > are simple enough to include in this refactoring. This pull request has now been integrated. Changeset: 0b0445be Author:Claes Redestad URL: https://git.openjdk.org/jdk/commit/0b0445be2833286b4eace698b91a658de3e7608b Stats: 1969 lines in 26 files changed: 1028 ins; 856 del; 85 mod 8331724: Refactor j.l.constant implementation to internal package Reviewed-by: liach, asotona - PR: https://git.openjdk.org/jdk/pull/19105
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v13]
On Wed, 15 May 2024 11:17:42 GMT, Claes Redestad wrote: >> This PR suggests refactoring the implementation classes of >> java.lang.constant into a new package jdk.internal.constant to enable >> sharing some trusted static factory methods with users elsewhere in >> java.base, such as java.lang.invoke and java.lang.classfile. The refactoring >> also adds some "trusted" methods for use when input is pre-validated, and >> makes use of them where applicable in the current implementation. There are >> more changes in the pipeline which will be folded into #17108 or PR'ed once >> that is integrated. >> >> There are two optimizations mixed up here. One in >> `MethodTypeDesc.ofDescriptor`: >> >> Name >> (descString) Cnt Base Error Test Error Unit Change >> MethodTypeDescFactories.ofDescriptor >> (Ljava/lang/Object;Ljava/lang/String;)I 6 138,371 ± 0,767 136,939 ± >> 1,126 ns/op 1,01x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> ()V 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 209,390 ± 4,583 196,175 ± >> 3,211 ns/op 1,07x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> ()[Ljava/lang/String; 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x >> (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> (..IIJ)V 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> (.). 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op >> 1,89x (p = 0,000*) >> * = significant >> >> >> The other in `ClassDesc::nested`, where to get rid of a simple static method >> in `ConstantUtils` I ended up speeding up and simplifying the public factory >> method: >> >> Name CntBase ErrorTest >> Error Unit Change >> ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± >> 0,573 ns/op 9,53x (p = 0,000*) >> * = significant >> >> >> The optimizations could be split out and PRd independently, but I think they >> are simple enough to include in this refactoring. > > Claes Redestad has updated the pull request incrementally with two additional > commits since the last revision: > > - Update src/java.base/share/classes/java/lang/constant/ClassDesc.java > >Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> > - Update src/java.base/share/classes/java/lang/constant/ClassDesc.java > >Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/19105#issuecomment-2117171356
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v13]
On Wed, 15 May 2024 11:17:42 GMT, Claes Redestad wrote:
>> This PR suggests refactoring the implementation classes of
>> java.lang.constant into a new package jdk.internal.constant to enable
>> sharing some trusted static factory methods with users elsewhere in
>> java.base, such as java.lang.invoke and java.lang.classfile. The refactoring
>> also adds some "trusted" methods for use when input is pre-validated, and
>> makes use of them where applicable in the current implementation. There are
>> more changes in the pipeline which will be folded into #17108 or PR'ed once
>> that is integrated.
>>
>> There are two optimizations mixed up here. One in
>> `MethodTypeDesc.ofDescriptor`:
>>
>> Name
>> (descString) Cnt Base Error Test Error Unit Change
>> MethodTypeDescFactories.ofDescriptor
>> (Ljava/lang/Object;Ljava/lang/String;)I 6 138,371 ± 0,767 136,939 ±
>> 1,126 ns/op 1,01x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()V 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ([IJLjava/lang/String;Z)Ljava/util/List; 6 209,390 ± 4,583 196,175 ±
>> 3,211 ns/op 1,07x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()[Ljava/lang/String; 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x
>> (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> (..IIJ)V 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> (.). 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op
>> 1,89x (p = 0,000*)
>> * = significant
>>
>>
>> The other in `ClassDesc::nested`, where to get rid of a simple static method
>> in `ConstantUtils` I ended up speeding up and simplifying the public factory
>> method:
>>
>> Name CntBase ErrorTest
>> Error Unit Change
>> ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ±
>> 0,573 ns/op 9,53x (p = 0,000*)
>> * = significant
>>
>>
>> The optimizations could be split out and PRd independently, but I think they
>> are simple enough to include in this refactoring.
>
> Claes Redestad has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Update src/java.base/share/classes/java/lang/constant/ClassDesc.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> - Update src/java.base/share/classes/java/lang/constant/ClassDesc.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
FTR I had to back out a change that would delegate to the now simplified and
optimized `arrayType()` from `arrayType(int rank)` when `rank` equals `1` since
the methods actually throw different exceptions, `IllegalStateException` vs
`IllegalArgumentException`. This was caught by a later test.
Unfortunate that `java.lang.constant` has chosen to use these two in particular
since it's easy to miss due to the similarity of the exception names. There are
also some unspecified exceptional behavior. For example on `arrayType()` only
`IllegalStateException` is specified to be thrown, but calling
`ClassDesc.ofDescriptor("V").arrayType()` throws an `IllegalArgumentException`.
This behavior is not changed by this PR.
-
PR Comment: https://git.openjdk.org/jdk/pull/19105#issuecomment-2112663111
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v13]
> This PR suggests refactoring the implementation classes of java.lang.constant > into a new package jdk.internal.constant to enable sharing some trusted > static factory methods with users elsewhere in java.base, such as > java.lang.invoke and java.lang.classfile. The refactoring also adds some > "trusted" methods for use when input is pre-validated, and makes use of them > where applicable in the current implementation. There are more changes in the > pipeline which will be folded into #17108 or PR'ed once that is integrated. > > There are two optimizations mixed up here. One in > `MethodTypeDesc.ofDescriptor`: > > Name (descString) > Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I > 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ()V > 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; > 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; > 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (..IIJ)V > 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (.). > 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*) > * = significant > > > The other in `ClassDesc::nested`, where to get rid of a simple static method > in `ConstantUtils` I ended up speeding up and simplifying the public factory > method: > > Name CntBase ErrorTest > Error Unit Change > ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± > 0,573 ns/op 9,53x (p = 0,000*) > * = significant > > > The optimizations could be split out and PRd independently, but I think they > are simple enough to include in this refactoring. Claes Redestad has updated the pull request incrementally with two additional commits since the last revision: - Update src/java.base/share/classes/java/lang/constant/ClassDesc.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Update src/java.base/share/classes/java/lang/constant/ClassDesc.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/19105/files - new: https://git.openjdk.org/jdk/pull/19105/files/2a1b9ac9..273bcecc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19105=12 - incr: https://webrevs.openjdk.org/?repo=jdk=19105=11-12 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19105.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105 PR: https://git.openjdk.org/jdk/pull/19105
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v12]
> This PR suggests refactoring the implementation classes of java.lang.constant > into a new package jdk.internal.constant to enable sharing some trusted > static factory methods with users elsewhere in java.base, such as > java.lang.invoke and java.lang.classfile. The refactoring also adds some > "trusted" methods for use when input is pre-validated, and makes use of them > where applicable in the current implementation. There are more changes in the > pipeline which will be folded into #17108 or PR'ed once that is integrated. > > There are two optimizations mixed up here. One in > `MethodTypeDesc.ofDescriptor`: > > Name (descString) > Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I > 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ()V > 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; > 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; > 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (..IIJ)V > 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (.). > 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*) > * = significant > > > The other in `ClassDesc::nested`, where to get rid of a simple static method > in `ConstantUtils` I ended up speeding up and simplifying the public factory > method: > > Name CntBase ErrorTest > Error Unit Change > ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± > 0,573 ns/op 9,53x (p = 0,000*) > * = significant > > > The optimizations could be split out and PRd independently, but I think they > are simple enough to include in this refactoring. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Can't call arrayType() from arrayType(int) due different exception types - Changes: - all: https://git.openjdk.org/jdk/pull/19105/files - new: https://git.openjdk.org/jdk/pull/19105/files/4e78b1b1..2a1b9ac9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19105=11 - incr: https://webrevs.openjdk.org/?repo=jdk=19105=10-11 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19105.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105 PR: https://git.openjdk.org/jdk/pull/19105
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v10]
On Wed, 15 May 2024 09:51:13 GMT, Adam Sotona wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use sb.repeat, consolidate
>
> src/java.base/share/classes/java/lang/constant/ClassDesc.java line 341:
>
>> 339: String desc = descriptorString();
>> 340: int index = desc.lastIndexOf('/');
>> 341: return (index == -1) ? "" : internalToBinary(desc.substring(1,
>> index - 1));
>
> Here it cuts the package name, for example
> `ConstantDescs.CD_Integer.packageName()` returns `java.lan`
>
> Suggestion:
>
> return (index == -1) ? "" : internalToBinary(desc.substring(1,
> index));
Thanks! Verified there were tests covering this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1601383209
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v11]
> This PR suggests refactoring the implementation classes of java.lang.constant > into a new package jdk.internal.constant to enable sharing some trusted > static factory methods with users elsewhere in java.base, such as > java.lang.invoke and java.lang.classfile. The refactoring also adds some > "trusted" methods for use when input is pre-validated, and makes use of them > where applicable in the current implementation. There are more changes in the > pipeline which will be folded into #17108 or PR'ed once that is integrated. > > There are two optimizations mixed up here. One in > `MethodTypeDesc.ofDescriptor`: > > Name (descString) > Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I > 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ()V > 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; > 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; > 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (..IIJ)V > 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (.). > 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*) > * = significant > > > The other in `ClassDesc::nested`, where to get rid of a simple static method > in `ConstantUtils` I ended up speeding up and simplifying the public factory > method: > > Name CntBase ErrorTest > Error Unit Change > ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± > 0,573 ns/op 9,53x (p = 0,000*) > * = significant > > > The optimizations could be split out and PRd independently, but I think they > are simple enough to include in this refactoring. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/lang/constant/ClassDesc.java Co-authored-by: Adam Sotona <10807609+asot...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/19105/files - new: https://git.openjdk.org/jdk/pull/19105/files/b0aca160..4e78b1b1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19105=10 - incr: https://webrevs.openjdk.org/?repo=jdk=19105=09-10 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19105.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105 PR: https://git.openjdk.org/jdk/pull/19105
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v10]
> This PR suggests refactoring the implementation classes of java.lang.constant > into a new package jdk.internal.constant to enable sharing some trusted > static factory methods with users elsewhere in java.base, such as > java.lang.invoke and java.lang.classfile. The refactoring also adds some > "trusted" methods for use when input is pre-validated, and makes use of them > where applicable in the current implementation. There are more changes in the > pipeline which will be folded into #17108 or PR'ed once that is integrated. > > There are two optimizations mixed up here. One in > `MethodTypeDesc.ofDescriptor`: > > Name (descString) > Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I > 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ()V > 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; > 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; > 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (..IIJ)V > 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (.). > 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*) > * = significant > > > The other in `ClassDesc::nested`, where to get rid of a simple static method > in `ConstantUtils` I ended up speeding up and simplifying the public factory > method: > > Name CntBase ErrorTest > Error Unit Change > ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± > 0,573 ns/op 9,53x (p = 0,000*) > * = significant > > > The optimizations could be split out and PRd independently, but I think they > are simple enough to include in this refactoring. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Use sb.repeat, consolidate - Changes: - all: https://git.openjdk.org/jdk/pull/19105/files - new: https://git.openjdk.org/jdk/pull/19105/files/6d5df18e..b0aca160 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19105=09 - incr: https://webrevs.openjdk.org/?repo=jdk=19105=08-09 Stats: 8 lines in 1 file changed: 1 ins; 5 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19105.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105 PR: https://git.openjdk.org/jdk/pull/19105
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v9]
On Tue, 14 May 2024 20:20:59 GMT, Chen Liang wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add microbenchmark for ClassDesc methods + a few optimizations
>
> src/java.base/share/classes/java/lang/constant/ClassDesc.java line 226:
>
>> 224: for (int i = 0; i < rank; i++) {
>> 225: sb.append('[');
>> 226: }
>
> Would `sb.repeat("[", rank);` be better here?
>
> Also for parity with `arrayType()` I recommend moving this building code to
> after the void check.
Oh, that's new (in 21). I was comparing against `append("[".repeat(rank))`
which had a cost here. `sb.repeat('[', rank)` is in the same ballpark and
simpler. Fixing.
Benchmark Mode Cnt Score Error
Units
ClassDescMethods.arrayType2 avgt 1531,568 ± 2,108
ns/op
ClassDescMethods.arrayType2:gc.alloc.rate avgt 15 7758,056 ± 474,378
MB/sec
ClassDescMethods.arrayType2:gc.alloc.rate.norm avgt 15 256,000 ± 0,001
B/op
I'll align the code to construct `newDesc` before the void check in both
methods.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1601088246
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v9]
On Tue, 14 May 2024 17:25:37 GMT, Claes Redestad wrote: >> This PR suggests refactoring the implementation classes of >> java.lang.constant into a new package jdk.internal.constant to enable >> sharing some trusted static factory methods with users elsewhere in >> java.base, such as java.lang.invoke and java.lang.classfile. The refactoring >> also adds some "trusted" methods for use when input is pre-validated, and >> makes use of them where applicable in the current implementation. There are >> more changes in the pipeline which will be folded into #17108 or PR'ed once >> that is integrated. >> >> There are two optimizations mixed up here. One in >> `MethodTypeDesc.ofDescriptor`: >> >> Name >> (descString) Cnt Base Error Test Error Unit Change >> MethodTypeDescFactories.ofDescriptor >> (Ljava/lang/Object;Ljava/lang/String;)I 6 138,371 ± 0,767 136,939 ± >> 1,126 ns/op 1,01x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> ()V 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 209,390 ± 4,583 196,175 ± >> 3,211 ns/op 1,07x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> ()[Ljava/lang/String; 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x >> (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> (..IIJ)V 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> (.). 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op >> 1,89x (p = 0,000*) >> * = significant >> >> >> The other in `ClassDesc::nested`, where to get rid of a simple static method >> in `ConstantUtils` I ended up speeding up and simplifying the public factory >> method: >> >> Name CntBase ErrorTest >> Error Unit Change >> ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± >> 0,573 ns/op 9,53x (p = 0,000*) >> * = significant >> >> >> The optimizations could be split out and PRd independently, but I think they >> are simple enough to include in this refactoring. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add microbenchmark for ClassDesc methods + a few optimizations Added a few microbenchmarks, and a couple of optimizations: Name Cnt Base Error Test Error Unit Change ClassDescMethods.arrayType15 49,922 ± 0,61614,546 ±0,524 ns/op 3,43x (p = 0,000*) :gc.alloc.rate.norm312,000 ± 0,000 144,000 ±0,000 B/op 0,46x (p = 0,000*) ClassDescMethods.arrayType1 15 37,764 ± 0,62514,441 ±0,272 ns/op 2,62x (p = 0,000*) :gc.alloc.rate.norm312,000 ± 0,000 144,000 ±0,000 B/op 0,46x (p = 0,000*) ClassDescMethods.arrayType2 15 48,663 ± 0,16835,223 ±6,517 ns/op 1,38x (p = 0,000*) :gc.alloc.rate.norm360,000 ± 0,000 256,000 ±0,000 B/op 0,71x (p = 0,000*) ClassDescMethods.displayName 159,663 ± 0,326 9,285 ±0,334 ns/op 1,04x (p = 0,002*) :gc.alloc.rate.norm 48,000 ± 0,00048,000 ±0,000 B/op 1,00x (p = 0,002 ) ClassDescMethods.packageName 15 39,387 ± 1,13431,242 ±5,554 ns/op 1,26x (p = 0,000*) :gc.alloc.rate.norm168,000 ± 0,00096,000 ±0,000 B/op 0,57x (p = 0,000*) * = significant This can be broken out to a follow-up PR, naturally. - PR Comment: https://git.openjdk.org/jdk/pull/19105#issuecomment-2110768382
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v9]
> This PR suggests refactoring the implementation classes of java.lang.constant > into a new package jdk.internal.constant to enable sharing some trusted > static factory methods with users elsewhere in java.base, such as > java.lang.invoke and java.lang.classfile. The refactoring also adds some > "trusted" methods for use when input is pre-validated, and makes use of them > where applicable in the current implementation. There are more changes in the > pipeline which will be folded into #17108 or PR'ed once that is integrated. > > There are two optimizations mixed up here. One in > `MethodTypeDesc.ofDescriptor`: > > Name (descString) > Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I > 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ()V > 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; > 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; > 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (..IIJ)V > 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (.). > 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*) > * = significant > > > The other in `ClassDesc::nested`, where to get rid of a simple static method > in `ConstantUtils` I ended up speeding up and simplifying the public factory > method: > > Name CntBase ErrorTest > Error Unit Change > ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± > 0,573 ns/op 9,53x (p = 0,000*) > * = significant > > > The optimizations could be split out and PRd independently, but I think they > are simple enough to include in this refactoring. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Add microbenchmark for ClassDesc methods + a few optimizations - Changes: - all: https://git.openjdk.org/jdk/pull/19105/files - new: https://git.openjdk.org/jdk/pull/19105/files/e2d7b59d..6d5df18e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19105=08 - incr: https://webrevs.openjdk.org/?repo=jdk=19105=07-08 Stats: 108 lines in 2 files changed: 87 ins; 0 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/19105.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105 PR: https://git.openjdk.org/jdk/pull/19105
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v8]
> This PR suggests refactoring the implementation classes of java.lang.constant > into a new package jdk.internal.constant to enable sharing some trusted > static factory methods with users elsewhere in java.base, such as > java.lang.invoke and java.lang.classfile. The refactoring also adds some > "trusted" methods for use when input is pre-validated, and makes use of them > where applicable in the current implementation. There are more changes in the > pipeline which will be folded into #17108 or PR'ed once that is integrated. > > There are two optimizations mixed up here. One in > `MethodTypeDesc.ofDescriptor`: > > Name (descString) > Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I > 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ()V > 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; > 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; > 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (..IIJ)V > 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (.). > 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*) > * = significant > > > The other in `ClassDesc::nested`, where to get rid of a simple static method > in `ConstantUtils` I ended up speeding up and simplifying the public factory > method: > > Name CntBase ErrorTest > Error Unit Change > ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± > 0,573 ns/op 9,53x (p = 0,000*) > * = significant > > > The optimizations could be split out and PRd independently, but I think they > are simple enough to include in this refactoring. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Weed out redundant null-checks, clarify javadoc of internal static utilities - Changes: - all: https://git.openjdk.org/jdk/pull/19105/files - new: https://git.openjdk.org/jdk/pull/19105/files/5af9de09..e2d7b59d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19105=07 - incr: https://webrevs.openjdk.org/?repo=jdk=19105=06-07 Stats: 8 lines in 2 files changed: 3 ins; 1 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/19105.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105 PR: https://git.openjdk.org/jdk/pull/19105
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v7]
On Fri, 10 May 2024 08:50:07 GMT, Claes Redestad wrote: >> This PR suggests refactoring the implementation classes of >> java.lang.constant into a new package jdk.internal.constant to enable >> sharing some trusted static factory methods with users elsewhere in >> java.base, such as java.lang.invoke and java.lang.classfile. The refactoring >> also adds some "trusted" methods for use when input is pre-validated, and >> makes use of them where applicable in the current implementation. There are >> more changes in the pipeline which will be folded into #17108 or PR'ed once >> that is integrated. >> >> There are two optimizations mixed up here. One in >> `MethodTypeDesc.ofDescriptor`: >> >> Name >> (descString) Cnt Base Error Test Error Unit Change >> MethodTypeDescFactories.ofDescriptor >> (Ljava/lang/Object;Ljava/lang/String;)I 6 138,371 ± 0,767 136,939 ± >> 1,126 ns/op 1,01x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> ()V 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> ([IJLjava/lang/String;Z)Ljava/util/List; 6 209,390 ± 4,583 196,175 ± >> 3,211 ns/op 1,07x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> ()[Ljava/lang/String; 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x >> (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> (..IIJ)V 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) >> MethodTypeDescFactories.ofDescriptor >> (.). 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op >> 1,89x (p = 0,000*) >> * = significant >> >> >> The other in `ClassDesc::nested`, where to get rid of a simple static method >> in `ConstantUtils` I ended up speeding up and simplifying the public factory >> method: >> >> Name CntBase ErrorTest >> Error Unit Change >> ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± >> 0,573 ns/op 9,53x (p = 0,000*) >> * = significant >> >> >> The optimizations could be split out and PRd independently, but I think they >> are simple enough to include in this refactoring. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Update > src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java > > Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> Gentle reminder that this needs a Reviewer. - PR Comment: https://git.openjdk.org/jdk/pull/19105#issuecomment-2109626820
Re: RFR: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations [v8]
On Mon, 6 May 2024 18:24:25 GMT, Adam Sotona wrote: >> Hi, >> During performance optimization work on Class-File API as JDK lambda >> generator we found some static initialization killers. >> One of them is `java.lang.classfile.Attributes` with tens of static fields >> initialized with individual attribute mappers, and common set of all >> mappers, and static map from attribute names to the mappers. >> >> I propose to turn all the static fields into lazy-initialized static methods >> and remove `PREDEFINED_ATTRIBUTES` and `standardAttribute(Utf8Entry name)` >> static mapping method from the `Attributes` API class. >> >> Please let me know your comments or objections and please review the >> [PR](https://github.com/openjdk/jdk/pull/19006) and >> [CSR](https://bugs.openjdk.org/browse/JDK-8331414), so we can make it into >> 23. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > fixed tests Thank you for this! - Marked as reviewed by redestad (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19006#pullrequestreview-2054638558
Re: RFR: 8327499: MethodHandleStatics.traceLambdaForm includes methods that cannot be generated [v2]
On Fri, 10 May 2024 04:55:21 GMT, Chen Liang wrote: >> GenerateJLIClassesHelper has been making wrong assumptions about Invoker's >> LambdaForm method type parameters. Since they are distinct from those of >> Linkers, they are now tracked and generated separately. It seems that no >> proper invoker was ever generated before, except it happens that most >> invoker signatures can be taken as linker signature so we never detected it. >> >> Requesting @iklam for a review; since I don't know how to deal with CDS, I >> have to relay to someone else to ensure this fixes the problem from the CDS >> side as well. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Add CDS test case to ensure LF resolution success Code changes and jdk tests looks good to me. - Marked as reviewed by redestad (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19164#pullrequestreview-2050282503
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v7]
> This PR suggests refactoring the implementation classes of java.lang.constant > into a new package jdk.internal.constant to enable sharing some trusted > static factory methods with users elsewhere in java.base, such as > java.lang.invoke and java.lang.classfile. The refactoring also adds some > "trusted" methods for use when input is pre-validated, and makes use of them > where applicable in the current implementation. There are more changes in the > pipeline which will be folded into #17108 or PR'ed once that is integrated. > > There are two optimizations mixed up here. One in > `MethodTypeDesc.ofDescriptor`: > > Name (descString) > Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I > 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ()V > 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; > 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; > 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (..IIJ)V > 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (.). > 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*) > * = significant > > > The other in `ClassDesc::nested`, where to get rid of a simple static method > in `ConstantUtils` I ended up speeding up and simplifying the public factory > method: > > Name CntBase ErrorTest > Error Unit Change > ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± > 0,573 ns/op 9,53x (p = 0,000*) > * = significant > > > The optimizations could be split out and PRd independently, but I think they > are simple enough to include in this refactoring. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/19105/files - new: https://git.openjdk.org/jdk/pull/19105/files/b21d3e60..5af9de09 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19105=06 - incr: https://webrevs.openjdk.org/?repo=jdk=19105=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19105.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105 PR: https://git.openjdk.org/jdk/pull/19105
Re: RFR: 8331932: Startup regressions in 23-b13 [v5]
On Thu, 9 May 2024 13:34:16 GMT, Claes Redestad wrote: >> A rather large startup regression was introduced in 23-b13 from >> [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has >> been dealt with as enhancements such as >> [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), >> [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and >> [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide >> both point fixes and reduce initialization overhead of certain constructs >> more generally. The remaining issues stem from a set of lambdas added in >> code for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early >> bootstrapping of the lambda infrastructure and a bit of class generation. >> >> While the remaining overheads are relatively small and borderline acceptable >> (< 2-3ms), I think it's still worth acting on them in this particular case >> since the amount of added bootstrapping overhead is dependent on which >> locale the system runs under, which complicates testing and comparisons due >> to relatively large differences in paths taken on different systems. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Reuse ReferenceKeyMap.concurrentHashMapSupplier Thanks for reviewing! - PR Comment: https://git.openjdk.org/jdk/pull/19140#issuecomment-2104166596
Integrated: 8331932: Startup regressions in 23-b13
On Wed, 8 May 2024 14:53:05 GMT, Claes Redestad wrote: > A rather large startup regression was introduced in 23-b13 from > [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has > been dealt with as enhancements such as > [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), > [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and > [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide > both point fixes and reduce initialization overhead of certain constructs > more generally. The remaining issues stem from a set of lambdas added in code > for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early > bootstrapping of the lambda infrastructure and a bit of class generation. > > While the remaining overheads are relatively small and borderline acceptable > (< 2-3ms), I think it's still worth acting on them in this particular case > since the amount of added bootstrapping overhead is dependent on which locale > the system runs under, which complicates testing and comparisons due to > relatively large differences in paths taken on different systems. This pull request has now been integrated. Changeset: d6541245 Author:Claes Redestad URL: https://git.openjdk.org/jdk/commit/d65412450254992c05c851298323b6acd3b39bd3 Stats: 61 lines in 4 files changed: 45 ins; 5 del; 11 mod 8331932: Startup regressions in 23-b13 Reviewed-by: alanb, naoto, liach - PR: https://git.openjdk.org/jdk/pull/19140
Re: RFR: 8331744: java.lang.classfile.TypeKind improvements [v5]
On Thu, 9 May 2024 21:15:31 GMT, Chen Liang wrote: >> A peek into TypeKind during the research for #19105 reveals that TypeKind >> has a few issues: >> 1. Name mismatch for `newarraycode` and `fromNewArrayCode`: Renamed both to >> use "newarray code" >> 2. `fromDescriptor` can throw IOOBE if the input string is empty: changed to >> throw IAE and added tests. >> 3. `from(Class)` can be slow due to descriptor computation: added benchmark, >> will share result in next comment (as it may change with code changes). >> >> The first 2 changes involves API changes, and a CSR has been created. >> Requesting @asotona for a review. > > Chen Liang has updated the pull request incrementally with two additional > commits since the last revision: > > - Restore fixed seed > - Revert "Hash table, use fixed random seed" > >This reverts commit 9af30c65d2c7be3535e4483e278151bc4473d63c. Marked as reviewed by redestad (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19109#pullrequestreview-2049509533
Re: RFR: 8331744: java.lang.classfile.TypeKind improvements [v4]
On Thu, 9 May 2024 12:29:40 GMT, Adam Sotona wrote: >> I actually am not sure of the reason; it is indeed a tableswitch in >> bytecode. @cl4es might know it better? > > Generally I agree with the API changes and with the > `descriptor.isPrimitive()` test before requesting a descriptor string. > However I'm missing the point of the other changes, which only replace this > single `tableswitch` with something way more complicated, without clear > performance gain. The code I tested saw a decidedly larger speed-up, perhaps hinging on other optimizations such as being able to avoid the `s.isEmpty()` check (could maybe catch IIOBE) and not matching on `L` and `[` (the lookup table only needs to deal with primitives). As it looks here I agree the speed-up is negligible and not worth it, and since `TypeDescriptor.OfField` isn't a sealed hierarchy perhaps we need some redundant checks. - PR Review Comment: https://git.openjdk.org/jdk/pull/19109#discussion_r1595873311
Re: RFR: 8331932: Startup regressions in 23-b13 [v5]
> A rather large startup regression was introduced in 23-b13 from > [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has > been dealt with as enhancements such as > [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), > [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and > [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide > both point fixes and reduce initialization overhead of certain constructs > more generally. The remaining issues stem from a set of lambdas added in code > for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early > bootstrapping of the lambda infrastructure and a bit of class generation. > > While the remaining overheads are relatively small and borderline acceptable > (< 2-3ms), I think it's still worth acting on them in this particular case > since the amount of added bootstrapping overhead is dependent on which locale > the system runs under, which complicates testing and comparisons due to > relatively large differences in paths taken on different systems. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Reuse ReferenceKeyMap.concurrentHashMapSupplier - Changes: - all: https://git.openjdk.org/jdk/pull/19140/files - new: https://git.openjdk.org/jdk/pull/19140/files/819e3d47..37968988 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19140=04 - incr: https://webrevs.openjdk.org/?repo=jdk=19140=03-04 Stats: 6 lines in 1 file changed: 0 ins; 5 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19140.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19140/head:pull/19140 PR: https://git.openjdk.org/jdk/pull/19140
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
On Wed, 8 May 2024 18:32:42 GMT, Chen Liang wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove redundant constructor
>
> src/java.base/share/classes/jdk/internal/util/ReferencedKeySet.java line 84:
>
>> 82: */
>> 83: public static Supplier, ReferenceKey>>
>> concurrentHashMapSupplier() {
>> 84: return new Supplier<>() {
>
> Can this just `return ReferencedKeyMap.concurrentHashMapSupplier();`?
It compiles, so maybe. :-) Running some tests.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19140#discussion_r1594703469
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v6]
On Wed, 8 May 2024 20:24:52 GMT, Chen Liang wrote: > This patch allows us to merge parsing logic for invoke, constant, and > classfile in the future, all in jdk.internal. Thanks for reviewing! Yes, that's the idea. - PR Comment: https://git.openjdk.org/jdk/pull/19105#issuecomment-2101371721
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v5]
On Wed, 8 May 2024 19:06:35 GMT, ExE Boss wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Refactor to further avoid re-validating arguments
>
> src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java
> line 179:
>
>> 177:
>> 178: for (ClassDesc param : paramTypes)
>> 179: validateArgument(param);
>
> This check should be performed as part of copying `paramTypes` to `newArgs`
> to avoid TOC/TOU issues, e.g.:
>
> @Override
> public MethodTypeDesc insertParameterTypes(int pos, ClassDesc... paramTypes) {
> if (pos < 0 || pos > argTypes.length)
> throw new IndexOutOfBoundsException(pos);
>
> ClassDesc[] newArgs = new ClassDesc[argTypes.length +
> paramTypes.length];
> if (pos > 0) {
> System.arraycopy(argTypes, 0, newArgs, 0, pos);
> }
> for (int i = 0; i < paramTypes.length; i++) {
> newArgs[pos + i] = validateArgument(paramTypes[i]);
> }
> if (pos < argTypes.length) {
> System.arraycopy(argTypes, pos, newArgs, pos +
> paramTypes.length, argTypes.length - pos);
> }
> return ofValidated(returnType, newArgs);
> }
>
>
> See also:
> https://github.com/openjdk/jdk/blob/230fac80f25e9608006c8928a8a7708bf13a818c/src/java.base/share/classes/java/util/ImmutableCollections.java#L186-L195
Nice catch! I conservatively just move the validation loop after to keep the
arraycopying.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1594579344
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v6]
> This PR suggests refactoring the implementation classes of java.lang.constant > into a new package jdk.internal.constant to enable sharing some trusted > static factory methods with users elsewhere in java.base, such as > java.lang.invoke and java.lang.classfile. The refactoring also adds some > "trusted" methods for use when input is pre-validated, and makes use of them > where applicable in the current implementation. There are more changes in the > pipeline which will be folded into #17108 or PR'ed once that is integrated. > > There are two optimizations mixed up here. One in > `MethodTypeDesc.ofDescriptor`: > > Name (descString) > Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I > 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ()V > 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; > 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; > 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (..IIJ)V > 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (.). > 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*) > * = significant > > > The other in `ClassDesc::nested`, where to get rid of a simple static method > in `ConstantUtils` I ended up speeding up and simplifying the public factory > method: > > Name CntBase ErrorTest > Error Unit Change > ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± > 0,573 ns/op 9,53x (p = 0,000*) > * = significant > > > The optimizations could be split out and PRd independently, but I think they > are simple enough to include in this refactoring. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Validate after copying to avoid TOCTOU - Changes: - all: https://git.openjdk.org/jdk/pull/19105/files - new: https://git.openjdk.org/jdk/pull/19105/files/eb23cf51..b21d3e60 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19105=05 - incr: https://webrevs.openjdk.org/?repo=jdk=19105=04-05 Stats: 10 lines in 1 file changed: 6 ins; 3 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19105.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105 PR: https://git.openjdk.org/jdk/pull/19105
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
On Wed, 8 May 2024 17:57:22 GMT, Naoto Sato wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove redundant constructor > > src/java.base/share/classes/sun/util/locale/BaseLocale.java line 96: > >> 94: // Interned BaseLocale cache >> 95: private static final ReferencedKeySet CACHE = >> 96: ReferencedKeySet.create(true, >> ReferencedKeySet.concurrentHashMapSupplier()); > > Should this supplier be in `BaseLocale` class? Otherwise `ReferencedKeySet` > may end up with static suppliers for each map type? Maybe, though I think most potential uses of `ReferenceKeySet/-Map` will want to be using a plain `CHM` as the backing store, so keeping these next to their respective `create` methods will encourage sharing. - PR Review Comment: https://git.openjdk.org/jdk/pull/19140#discussion_r159407
Re: RFR: 8331932: Startup regressions in 23-b13 [v3]
On Wed, 8 May 2024 17:23:25 GMT, Alan Bateman wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Extract singleton for interning BaseLocale > > src/java.base/share/classes/java/util/Locale.java line 1022: > >> 1020: exts = null; >> 1021: hash = 0; >> 1022: } > > I don't think you need to add this constructor now. Was certain I had removed it. - PR Review Comment: https://git.openjdk.org/jdk/pull/19140#discussion_r1594382318
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
> A rather large startup regression was introduced in 23-b13 from > [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has > been dealt with as enhancements such as > [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), > [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and > [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide > both point fixes and reduce initialization overhead of certain constructs > more generally. The remaining issues stem from a set of lambdas added in code > for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early > bootstrapping of the lambda infrastructure and a bit of class generation. > > While the remaining overheads are relatively small and borderline acceptable > (< 2-3ms), I think it's still worth acting on them in this particular case > since the amount of added bootstrapping overhead is dependent on which locale > the system runs under, which complicates testing and comparisons due to > relatively large differences in paths taken on different systems. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Remove redundant constructor - Changes: - all: https://git.openjdk.org/jdk/pull/19140/files - new: https://git.openjdk.org/jdk/pull/19140/files/d8594b31..819e3d47 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19140=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19140=02-03 Stats: 6 lines in 1 file changed: 0 ins; 6 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19140.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19140/head:pull/19140 PR: https://git.openjdk.org/jdk/pull/19140
Re: RFR: 8331932: Startup regressions in 23-b13 [v3]
> A rather large startup regression was introduced in 23-b13 from > [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has > been dealt with as enhancements such as > [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), > [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and > [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide > both point fixes and reduce initialization overhead of certain constructs > more generally. The remaining issues stem from a set of lambdas added in code > for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early > bootstrapping of the lambda infrastructure and a bit of class generation. > > While the remaining overheads are relatively small and borderline acceptable > (< 2-3ms), I think it's still worth acting on them in this particular case > since the amount of added bootstrapping overhead is dependent on which locale > the system runs under, which complicates testing and comparisons due to > relatively large differences in paths taken on different systems. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Extract singleton for interning BaseLocale - Changes: - all: https://git.openjdk.org/jdk/pull/19140/files - new: https://git.openjdk.org/jdk/pull/19140/files/73097159..d8594b31 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19140=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19140=01-02 Stats: 21 lines in 1 file changed: 11 ins; 9 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19140.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19140/head:pull/19140 PR: https://git.openjdk.org/jdk/pull/19140
Re: RFR: 8331932: Startup regressions in 23-b13 [v2]
> A rather large startup regression was introduced in 23-b13 from > [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has > been dealt with as enhancements such as > [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), > [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and > [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide > both point fixes and reduce initialization overhead of certain constructs > more generally. The remaining issues stem from a set of lambdas added in code > for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early > bootstrapping of the lambda infrastructure and a bit of class generation. > > While the remaining overheads are relatively small and borderline acceptable > (< 2-3ms), I think it's still worth acting on them in this particular case > since the amount of added bootstrapping overhead is dependent on which locale > the system runs under, which complicates testing and comparisons due to > relatively large differences in paths taken on different systems. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Singleton Locale creator - Changes: - all: https://git.openjdk.org/jdk/pull/19140/files - new: https://git.openjdk.org/jdk/pull/19140/files/0520f953..73097159 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19140=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19140=00-01 Stats: 13 lines in 1 file changed: 3 ins; 7 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19140.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19140/head:pull/19140 PR: https://git.openjdk.org/jdk/pull/19140
RFR: 8331932: Startup regressions in 23-b13
A rather large startup regression was introduced in 23-b13 from [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has been dealt with as enhancements such as [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide both point fixes and reduce initialization overhead of certain constructs more generally. The remaining issues stem from a set of lambdas added in code for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early bootstrapping of the lambda infrastructure and a bit of class generation. While the remaining overheads are relatively small and borderline acceptable (< 2-3ms), I think it's still worth acting on them in this particular case since the amount of added bootstrapping overhead is dependent on which locale the system runs under, which complicates testing and comparisons due to relatively large differences in paths taken on different systems. - Commit messages: - 8331932: Startup regressions in 23-b13 Changes: https://git.openjdk.org/jdk/pull/19140/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19140=00 Issue: https://bugs.openjdk.org/browse/JDK-8331932 Stats: 74 lines in 4 files changed: 57 ins; 4 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/19140.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19140/head:pull/19140 PR: https://git.openjdk.org/jdk/pull/19140
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v5]
> This PR suggests refactoring the implementation classes of java.lang.constant > into a new package jdk.internal.constant to enable sharing some trusted > static factory methods with users elsewhere in java.base, such as > java.lang.invoke and java.lang.classfile. The refactoring also adds some > "trusted" methods for use when input is pre-validated, and makes use of them > where applicable in the current implementation. There are more changes in the > pipeline which will be folded into #17108 or PR'ed once that is integrated. > > There are two optimizations mixed up here. One in > `MethodTypeDesc.ofDescriptor`: > > Name (descString) > Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I > 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ()V > 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; > 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; > 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (..IIJ)V > 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (.). > 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*) > * = significant > > > The other in `ClassDesc::nested`, where to get rid of a simple static method > in `ConstantUtils` I ended up speeding up and simplifying the public factory > method: > > Name CntBase ErrorTest > Error Unit Change > ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± > 0,573 ns/op 9,53x (p = 0,000*) > * = significant > > > The optimizations could be split out and PRd independently, but I think they > are simple enough to include in this refactoring. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Refactor to further avoid re-validating arguments - Changes: - all: https://git.openjdk.org/jdk/pull/19105/files - new: https://git.openjdk.org/jdk/pull/19105/files/543d0709..eb23cf51 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19105=04 - incr: https://webrevs.openjdk.org/?repo=jdk=19105=03-04 Stats: 33 lines in 2 files changed: 21 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/19105.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105 PR: https://git.openjdk.org/jdk/pull/19105
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v4]
> This PR suggests refactoring the implementation classes of java.lang.constant > into a new package jdk.internal.constant to enable sharing some trusted > static factory methods with users elsewhere in java.base, such as > java.lang.invoke and java.lang.classfile. The refactoring also adds some > "trusted" methods for use when input is pre-validated, and makes use of them > where applicable in the current implementation. There are more changes in the > pipeline which will be folded into #17108 or PR'ed once that is integrated. > > There are two optimizations mixed up here. One in > `MethodTypeDesc.ofDescriptor`: > > Name (descString) > Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I > 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ()V > 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; > 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; > 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (..IIJ)V > 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (.). > 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*) > * = significant > > > The other in `ClassDesc::nested`, where to get rid of a simple static method > in `ConstantUtils` I ended up speeding up and simplifying the public factory > method: > > Name CntBase ErrorTest > Error Unit Change > ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± > 0,573 ns/op 9,53x (p = 0,000*) > * = significant > > > The optimizations could be split out and PRd independently, but I think they > are simple enough to include in this refactoring. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: pre-validated - Changes: - all: https://git.openjdk.org/jdk/pull/19105/files - new: https://git.openjdk.org/jdk/pull/19105/files/f2f90193..543d0709 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19105=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19105=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19105.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105 PR: https://git.openjdk.org/jdk/pull/19105
Re: RFR: 8305457: Implement java.io.IO
On Tue, 7 May 2024 12:18:52 GMT, Rémi Forax wrote: >> I assume it's about performance. If so, I would defer any >> performance-related tweaks until they are necessary. Interactive reading >> from console does not sound like something requiring that level of >> performance tweaking. > > yes, let see what @cl4es will say when he will test the time to print "hello > world" @forax I don't think anything touched on here is used during bootstrap; perhaps there are apps we could cover that uses these APIs, but I think for line-based console IO a few calls in the interpreter is not going to make a noticeable difference. - PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592438417
Re: RFR: 8331744: java.lang.classfile.TypeKind improvements [v3]
On Tue, 7 May 2024 01:49:27 GMT, Chen Liang wrote:
>> A peek into TypeKind during the research for #19105 reveals that TypeKind
>> has a few issues:
>> 1. Name mismatch for `newarraycode` and `fromNewArrayCode`: Renamed both to
>> use "newarray code"
>> 2. `fromDescriptor` can throw IOOBE if the input string is empty: changed to
>> throw IAE and added tests.
>> 3. `from(Class)` can be slow due to descriptor computation: added benchmark,
>> will share result in next comment (as it may change with code changes).
>>
>> The first 2 changes involves API changes, and a CSR has been created.
>> Requesting @asotona for a review.
>
> Chen Liang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add a mixed scenario
Nice improvement to the micro. It might be preferable to use random generation
with a hardcoded or parameterized seed to reduce run-to-run variation, eg. `new
Random(1)`.
Agree that call sites are unlikely to mix `CD` and `Class`, but we're likely to
see the bimorphicness from passing Primitive and ReferenceCDs. This is
typically a very small and constant call overhead.
The primitives path seem to be the weak link, and maybe we could take a cue
from `sun.util.invoke.Wrapper` and set up a similar perfect hash table instead
of a switch. This seem somewhat profitable:
Name(type) CntBaseError TestError
Unit Change
TypeKindBench.fromClassDescs PRIMITIVE 6 196,203 ± 2,469 147,898 ± 8,786
ns/op 1,33x (p = 0,000*)
TypeKindBench.fromClassesPRIMITIVE 6 325,336 ± 12,203 206,084 ± 2,180
ns/op 1,58x (p = 0,000*)
The `fromClasses PRIMITIVE` case is still a bit behind since getting the
descriptorString takes a few extra (non-allocating) steps.
Patch:
diff --git a/src/java.base/share/classes/java/lang/classfile/TypeKind.java
b/src/java.base/share/classes/java/lang/classfile/TypeKind.java
index 5ba566b3d06..dd0a06c63ea 100644
--- a/src/java.base/share/classes/java/lang/classfile/TypeKind.java
+++ b/src/java.base/share/classes/java/lang/classfile/TypeKind.java
@@ -58,6 +58,7 @@ public enum TypeKind {
private final String name;
private final String descriptor;
+private final char basicTypeChar;
private final int newarrayCode;
/** {@return the human-readable name corresponding to this type} */
@@ -100,6 +101,7 @@ public TypeKind asLoadable() {
TypeKind(String name, String descriptor, int newarrayCode) {
this.name = name;
this.descriptor = descriptor;
+this.basicTypeChar = descriptor.charAt(0);
this.newarrayCode = newarrayCode;
}
@@ -154,7 +156,29 @@ public static TypeKind fromDescriptor(CharSequence s) {
*/
public static TypeKind from(TypeDescriptor.OfField descriptor) {
return descriptor.isPrimitive() // implicit null check
-? fromDescriptor(descriptor.descriptorString())
+? ofPrimitive(descriptor.descriptorString())
: TypeKind.ReferenceType;
}
+
+// Perfect hashing for basic types, borrowed from sun.invoke.util.Wrapper
+private static final TypeKind[] FROM_CHAR = new TypeKind[16];
+
+static {
+for (TypeKind w : values()) {
+if (w == ReferenceType) {
+continue;
+}
+char c = w.descriptor.charAt(0);
+FROM_CHAR[(c + (c >> 1)) & 0xf] = w;
+}
+}
+
+private static TypeKind ofPrimitive(String descriptor) {
+char basicTypeChar = descriptor.charAt(0);
+TypeKind tk = FROM_CHAR[(basicTypeChar + (basicTypeChar >> 1)) & 0xf];
+if (tk == null || tk.basicTypeChar != basicTypeChar) {
+throw new IllegalArgumentException("bad descriptor: " +
descriptor);
+}
+return tk;
+}
}
```
-
PR Comment: https://git.openjdk.org/jdk/pull/19109#issuecomment-2098060815
Re: RFR: 8331744: java.lang.classfile.TypeKind improvements
On Mon, 6 May 2024 20:48:05 GMT, Chen Liang wrote: > A peek into TypeKind during the research for #19105 reveals that TypeKind has > a few issues: > 1. Name mismatch for `newarraycode` and `fromNewArrayCode`: Renamed both to > use "newarray code" > 2. `fromDescriptor` can throw IOOBE if the input string is empty: changed to > throw IAE and added tests. > 3. `from(Class)` can be slow due to descriptor computation: added benchmark, > will share result in next comment (as it may change with code changes). > > The first 2 changes involves API changes, and a CSR has been created. > Requesting @asotona for a review. Leaning on `TypeDescriptor.OfField::isPrimitive` presents an opportunity: override `isPrimitive` in `PrimitiveClassDescImpl` and `ReferenceClassDescImpl` to return `true` and `false` respectively: Name(type) CntBaseError TestError Unit Change TypeKindBench.fromClassDescs PRIMITIVE 6 199,765 ± 3,370 205,531 ± 2,632 ns/op 0,97x (p = 0,000*) TypeKindBench.fromClassDescs REFERENCE 6 75,018 ± 1,113 25,925 ± 1,145 ns/op 2,89x (p = 0,000*) TypeKindBench.fromClassesPRIMITIVE 6 344,477 ± 46,310 366,135 ± 54,955 ns/op 0,94x (p = 0,066 ) TypeKindBench.fromClassesREFERENCE 6 23,338 ± 0,467 23,183 ± 1,357 ns/op 1,01x (p = 0,484 ) * = significant Interestingly this has a tiny regression for the primitive case - in this micro. Probably an effect of the default `descriptor.length() == 1` implementation acting as a sort of prefetch the value we'll switch on (`descriptor.charAt(0)`) down this path. Only overriding for `ReferenceClassDescImpl` is neutral, but maybe that's overfitting: Name(type) CntBaseError TestError Unit Change TypeKindBench.fromClassDescs PRIMITIVE 6 199,765 ± 3,370 196,203 ± 2,469 ns/op 1,02x (p = 0,000*) TypeKindBench.fromClassDescs REFERENCE 6 75,018 ± 1,113 25,311 ± 0,138 ns/op 2,96x (p = 0,000*) TypeKindBench.fromClassesPRIMITIVE 6 344,477 ± 46,310 325,336 ± 12,203 ns/op 1,06x (p = 0,035 ) TypeKindBench.fromClassesREFERENCE 6 23,338 ± 0,467 23,462 ± 3,239 ns/op 0,99x (p = 0,805 ) * = significant - PR Comment: https://git.openjdk.org/jdk/pull/19109#issuecomment-2096996186
Re: RFR: 8331744: java.lang.classfile.TypeKind improvements
On Mon, 6 May 2024 20:48:05 GMT, Chen Liang wrote: > A peek into TypeKind during the research for #19105 reveals that TypeKind has > a few issues: > 1. Name mismatch for `newarraycode` and `fromNewArrayCode`: Renamed both to > use "newarray code" > 2. `fromDescriptor` can throw IOOBE if the input string is empty: changed to > throw IAE and added tests. > 3. `from(Class)` can be slow due to descriptor computation: added benchmark, > will share result in next comment (as it may change with code changes). > > The first 2 changes involves API changes, and a CSR has been created. > Requesting @asotona for a review. test/micro/org/openjdk/bench/java/lang/classfile/TypeKindBench.java line 52: > 50: @Warmup(iterations = 3, time = 2) > 51: @Measurement(iterations = 6, time = 1) > 52: @Fork(1) Suggestion: @Fork(jvmArgsAppend = "--enable-preview", value = 1) - PR Review Comment: https://git.openjdk.org/jdk/pull/19109#discussion_r1591587643
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v3]
On Mon, 6 May 2024 14:58:02 GMT, Chen Liang wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Rename ofTrusted to ofValidated, remove accidental module-info exports
>
> src/java.base/share/classes/java/lang/constant/ClassDesc.java line 222:
>
>> 220: }
>> 221: if (desc.length() == 1 && desc.charAt(0) == 'V') {
>> 222: throw new IllegalArgumentException(String.format("not a
>> valid reference type descriptor: %sV", "[".repeat(rank)));
>
> Suggestion:
>
> throw new IllegalArgumentException(String.format("not a valid
> reference type descriptor: %sV", "[".repeat(netRank)));
>
> Or should we override this in `PrimitiveClassDescImpl`, which can bypass the
> rank sum computation?
`currentDepth` must be 0 in this case, so `rank` or `netRank` doesn't matter.
Overriding in `PrimitiveClassDescImpl` sounds reasonable, but then perhaps
default method should be removed, too, since it would look strange to have the
default method be specialized for instance/array types. Sounds like a CSR might
be needed(?), so let's do that in a follow up.
> src/java.base/share/classes/jdk/internal/constant/ConstantUtils.java line 80:
>
>> 78: char ch = name.charAt(i);
>> 79: if (ch == ';' || ch == '[' || ch == '.')
>> 80: throw new IllegalArgumentException("Invalid class name:
>> " + name);
>
> We can check there's no consecutive `..` `//` or trailing `.` or `/` so we
> can just use the validated string to create a reference class desc.
Sounds reasonable, but I think I have already piled on too much into this PR.
Ok to defer to a follow-up?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1591165979
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1591167360
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v3]
> This PR suggests refactoring the implementation classes of java.lang.constant > into a new package jdk.internal.constant to enable sharing some trusted > static factory methods with users elsewhere in java.base, such as > java.lang.invoke and java.lang.classfile. The refactoring also adds some > "trusted" methods for use when input is pre-validated, and makes use of them > where applicable in the current implementation. There are more changes in the > pipeline which will be folded into #17108 or PR'ed once that is integrated. > > There are two optimizations mixed up here. One in > `MethodTypeDesc.ofDescriptor`: > > Name (descString) > Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I > 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ()V > 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; > 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; > 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (..IIJ)V > 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (.). > 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*) > * = significant > > > The other in `ClassDesc::nested`, where to get rid of a simple static method > in `ConstantUtils` I ended up speeding up and simplifying the public factory > method: > > Name CntBase ErrorTest > Error Unit Change > ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± > 0,573 ns/op 9,53x (p = 0,000*) > * = significant > > > The optimizations could be split out and PRd independently, but I think they > are simple enough to include in this refactoring. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Rename ofTrusted to ofValidated, remove accidental module-info exports - Changes: - all: https://git.openjdk.org/jdk/pull/19105/files - new: https://git.openjdk.org/jdk/pull/19105/files/f3112f8e..f2f90193 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19105=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19105=01-02 Stats: 44 lines in 5 files changed: 0 ins; 2 del; 42 mod Patch: https://git.openjdk.org/jdk/pull/19105.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105 PR: https://git.openjdk.org/jdk/pull/19105
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v2]
> This PR suggests refactoring the implementation classes of java.lang.constant > into a new package jdk.internal.constant to enable sharing some trusted > static factory methods with users elsewhere in java.base, such as > java.lang.invoke and java.lang.classfile. The refactoring also adds some > "trusted" methods for use when input is pre-validated, and makes use of them > where applicable in the current implementation. There are more changes in the > pipeline which will be folded into #17108 or PR'ed once that is integrated. > > There are two optimizations mixed up here. One in > `MethodTypeDesc.ofDescriptor`: > > Name (descString) > Cnt Base Error Test Error Unit Change > MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I > 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ()V > 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List; > 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor()[Ljava/lang/String; > 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (..IIJ)V > 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*) > MethodTypeDescFactories.ofDescriptor (.). > 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*) > * = significant > > > The other in `ClassDesc::nested`, where to get rid of a simple static method > in `ConstantUtils` I ended up speeding up and simplifying the public factory > method: > > Name CntBase ErrorTest > Error Unit Change > ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ± > 0,573 ns/op 9,53x (p = 0,000*) > * = significant > > > The optimizations could be split out and PRd independently, but I think they > are simple enough to include in this refactoring. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/lang/constant/ClassDesc.java Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/19105/files - new: https://git.openjdk.org/jdk/pull/19105/files/56631cac..f3112f8e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19105=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19105=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19105.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105 PR: https://git.openjdk.org/jdk/pull/19105
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package
On Mon, 6 May 2024 14:51:17 GMT, Chen Liang wrote:
>> This PR suggests refactoring the implementation classes of
>> java.lang.constant into a new package jdk.internal.constant to enable
>> sharing some trusted static factory methods with users elsewhere in
>> java.base, such as java.lang.invoke and java.lang.classfile. The refactoring
>> also adds some "trusted" methods for use when input is pre-validated, and
>> makes use of them where applicable in the current implementation. There are
>> more changes in the pipeline which will be folded into #17108 or PR'ed once
>> that is integrated.
>>
>> There are two optimizations mixed up here. One in
>> `MethodTypeDesc.ofDescriptor`:
>>
>> Name
>> (descString) Cnt Base Error Test Error Unit Change
>> MethodTypeDescFactories.ofDescriptor
>> (Ljava/lang/Object;Ljava/lang/String;)I 6 138,371 ± 0,767 136,939 ±
>> 1,126 ns/op 1,01x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()V 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ([IJLjava/lang/String;Z)Ljava/util/List; 6 209,390 ± 4,583 196,175 ±
>> 3,211 ns/op 1,07x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()[Ljava/lang/String; 6 36,039 ± 8,68420,794 ± 1,110 ns/op 1,73x
>> (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> (..IIJ)V 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> (.). 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op
>> 1,89x (p = 0,000*)
>> * = significant
>>
>>
>> The other in `ClassDesc::nested`, where to get rid of a simple static method
>> in `ConstantUtils` I ended up speeding up and simplifying the public factory
>> method:
>>
>> Name CntBase ErrorTest
>> Error Unit Change
>> ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ±
>> 0,573 ns/op 9,53x (p = 0,000*)
>> * = significant
>>
>>
>> The optimizations could be split out and PRd independently, but I think they
>> are simple enough to include in this refactoring.
>
> src/java.base/share/classes/jdk/internal/constant/ReferenceClassDescImpl.java
> line 68:
>
>> 66: * @jvms 4.3.2 Field Descriptors
>> 67: */
>> 68: public static ReferenceClassDescImpl ofTrusted(String descriptor) {
>
> If you named `ofTrusted` to be in parity with the MTDImpl factory, they are
> different as MTD one means array is trusted; this one means descriptor is
> already valid.
`ofValidated`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1591147979
