Re: RFR: 8355369: Remove setAccessible usage for setting final fields in java.util.concurrent [v2]
On Wed, 23 Apr 2025 17:54:03 GMT, Chen Liang wrote: >> I wonder if we could just change it to return `new >> ConcurrentSkipListSet<>(m)`. COWAL could be changed to return a new object >> too. > > The set only has a map field, and AbstractSet does not define any additional > field. The map should be fine too - two fields in AbstractMap are cleared > when cloning happens, so recreating a map from a constructor should have the > same effect. (Note a significant field `head` is not cleared upon clone, but > seems immediately replaced later in `buildFromSorted`). Both should still be > fine with this new return values. As others have mentioned, clone() needs to be re-checked wrt issuing final field fences. But can be conservatively done so here anyway. - PR Review Comment: https://git.openjdk.org/jdk/pull/24821#discussion_r2058163496
Re: RFR: 8355369: Remove setAccessible usage for setting final fields in java.util.concurrent [v2]
On Wed, 23 Apr 2025 15:22:37 GMT, Viktor Klang wrote: >> This Pull Request replaces the uses of Field + setAccessible to modify final >> fields in java.util.concurrent with Unsafe. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Adding a releaseFence() to ConcurrentskipListSet.clone() Marked as reviewed by dl (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24821#pullrequestreview-2790744154
Re: RFR: 8353786: Migrate Vector API math library support to FFM API [v12]
On Wed, 23 Apr 2025 23:55:50 GMT, Vladimir Ivanov wrote:
>> Does the following check catch `UseRVV == false` case on RISC-V?
>>
>> public boolean isSupported(Operator op, VectorSpecies vspecies) {
>> ...
>> int maxLaneCount =
>> VectorSupport.getMaxLaneCount(vspecies.elementType());
>> if (vspecies.length() > maxLaneCount) {
>> return false; // lacking vector support
>> }
>> ...
>
> FTR both `VectorSupport.getMaxLaneCount()` and `CPUFeatures` don't rely on
> raw list of ISA extensions CPU supports, but only those reported by the JVM.
> So, if some feature support is disabled on JVM side, it won't be reported by
> `VM_Version` and, hence, `CPUFeatures`.
Thank you for updating! Looks good for riscv. I have ran some basic tests for
vector API, passed. I did not ran benchmark, as riscv & aarch64 share the same
way to bridge from java to sleef.
> Does the following check catch UseRVV == false case on RISC-V?
Yes. If you don't mind, an explicit comment might be helpful. As to me "lacking
vector support" here means the vector length is not large enough, but it's
quite subjective, so you are on the call.
> FTR both VectorSupport.getMaxLaneCount() and CPUFeatures don't rely on raw
> list of ISA extensions CPU supports, but only those reported by the JVM. So,
> if some feature support is disabled on JVM side, it won't be reported by
> VM_Version and, hence, CPUFeatures.
I'm fine with this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24462#discussion_r2058172975
Re: RFR: 8347408: Create an internal method handle adapter for system calls with errno [v5]
> As we advance, converting older JDK code to use the relatively new FFM API
> requires system calls that can provide `errno` and the likes to explicitly
> allocate a `MemorySegment` to capture potential error states. This can lead
> to negative performance implications if not designed carefully and also
> introduces unnecessary code complexity.
>
> Hence, this PR proposes adding a JDK internal method handle adapter that can
> be used to handle system calls with `errno`, `GetLastError`, and
> `WSAGetLastError`.
>
> It relies on an efficient carrier-thread-local cache of memory regions to
> allide allocations.
>
>
> Here are some benchmarks that ran on a platform thread and virtual threads
> respectively (M1 Mac):
>
>
> Benchmark Mode Cnt Score
> Error Units
> CaptureStateUtilBench.OfVirtual.adaptedSysCallFail avgt 30 24.330
> ? 0.820 ns/op
> CaptureStateUtilBench.OfVirtual.adaptedSysCallSuccess avgt 30 8.257
> ? 0.117 ns/op
> CaptureStateUtilBench.OfVirtual.explicitAllocationFail avgt 30 41.415
> ? 1.013 ns/op
> CaptureStateUtilBench.OfVirtual.explicitAllocationSuccess avgt 30 21.720
> ? 0.463 ns/op
> CaptureStateUtilBench.OfVirtual.tlAllocationFail avgt 30 23.636
> ? 0.182 ns/op
> CaptureStateUtilBench.OfVirtual.tlAllocationSuccessavgt 30 8.234
> ? 0.156 ns/op
> CaptureStateUtilBench.adaptedSysCallFail avgt 30 23.918
> ? 0.487 ns/op
> CaptureStateUtilBench.adaptedSysCallSuccessavgt 30 4.946
> ? 0.089 ns/op
> CaptureStateUtilBench.explicitAllocationFail avgt 30 42.280
> ? 1.128 ns/op
> CaptureStateUtilBench.explicitAllocationSuccessavgt 30 21.809
> ? 0.413 ns/op
> CaptureStateUtilBench.tlAllocationFail avgt 30 24.422
> ? 0.673 ns/op
> CaptureStateUtilBench.tlAllocationSuccess avgt 30 5.182
> ? 0.152 ns/op
>
>
> Adapted system call:
>
> return (int) ADAPTED_HANDLE.invoke(0, 0); // Uses a MH-internal pool
> ```
> Explicit allocation:
>
> try (var arena = Arena.ofConfined()) {
> return (int) HANDLE.invoke(arena.allocate(4), 0, 0);
> }
> ```
> Thread Local allocation:
>
> try (var arena = POOLS.take()) {
> return (int) HANDLE.invoke(arena.allocate(4), 0, 0); // Uses a
> manually specified pool
> }
> ```
> The adapted system call exhibits a ~4x performance improvement over the
> existing "explicit allocation" scheme for the happy path on platform threads.
> ...
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Revert change to ArenaImpl
-
Changes:
- all: https://git.openjdk.org/jdk/pull/23765/files
- new: https://git.openjdk.org/jdk/pull/23765/files/1a31ae04..c0e46d30
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=23765&range=04
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=23765&range=03-04
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/23765.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/23765/head:pull/23765
PR: https://git.openjdk.org/jdk/pull/23765
Re: RFR: 8355369: Remove setAccessible usage for setting final fields in java.util.concurrent [v2]
On Wed, 23 Apr 2025 16:23:00 GMT, Chen Liang wrote: >> Viktor Klang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Adding a releaseFence() to ConcurrentskipListSet.clone() > > Thanks. We can investigate how clone cleanups interact with truly final or > strict finals in another issue. > @liach @AlanBateman My proposal here would be to move forward with the > proposed changes for now, and then once 25 is branched off there's an > opportunity to take a broader look at `clone()`. Okay with me. We can also add a non-public constructor to some of these classes and use it from readResolve so that we don't need readObject methods that mutate finals. - PR Comment: https://git.openjdk.org/jdk/pull/24821#issuecomment-2827444652
Re: RFR: 8355369: Remove setAccessible usage for setting final fields in java.util.concurrent [v2]
On Wed, 23 Apr 2025 15:22:37 GMT, Viktor Klang wrote: >> This Pull Request replaces the uses of Field + setAccessible to modify final >> fields in java.util.concurrent with Unsafe. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Adding a releaseFence() to ConcurrentskipListSet.clone() Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24821#pullrequestreview-2790943049
Re: RFR: 8077587: BigInteger Roots [v30]
> This PR implements nth root computation for `BigInteger`s using Newton method > and optimizes `BigInteger.pow(int)` method. > [Here is a proof of convergence of the recurrence > used.](https://github.com/user-attachments/files/19785045/nth_root_newton_proof_integers.pdf) fabioromano1 has updated the pull request incrementally with one additional commit since the last revision: Some optimizations - Changes: - all: https://git.openjdk.org/jdk/pull/24690/files - new: https://git.openjdk.org/jdk/pull/24690/files/8676af7c..3cf820b2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24690&range=29 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24690&range=28-29 Stats: 9 lines in 1 file changed: 7 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/24690.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24690/head:pull/24690 PR: https://git.openjdk.org/jdk/pull/24690
Re: RFR: 8077587: BigInteger Roots [v31]
> This PR implements nth root computation for `BigInteger`s using Newton method > and optimizes `BigInteger.pow(int)` method. > [Here is a proof of convergence of the recurrence > used.](https://github.com/user-attachments/files/19785045/nth_root_newton_proof_integers.pdf) fabioromano1 has updated the pull request incrementally with one additional commit since the last revision: Systematization of special cases in BigInteger.pow(int) - Changes: - all: https://git.openjdk.org/jdk/pull/24690/files - new: https://git.openjdk.org/jdk/pull/24690/files/3cf820b2..23914e8a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24690&range=30 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24690&range=29-30 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/24690.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24690/head:pull/24690 PR: https://git.openjdk.org/jdk/pull/24690
Re: RFR: 8355080: java.base/jdk.internal.foreign.SystemLookup.find() doesn't work on static JDK [v3]
On Wed, 23 Apr 2025 00:56:18 GMT, Jiangli Zhou wrote:
>> Please review this PR that changes to use `NativeLibraries.loadLibrary()`
>> for loading the `libsyslookup` in `jdk.internal.foreign.SystemLookup` class.
>>
>> `NativeLibraries.loadLibrary()` handles both the shared library and (static)
>> built-in library loading properly. On `static-jdk`, calling
>> `NativeLibraries.loadLibrary()` for `systlookup` library can find the
>> built-in library by looking up using `JNI_OnLoad_syslookup`. The current
>> change adds `DEF_STATIC_JNI_OnLoad` in syslookup.c (in shared, windows and
>> aix versions) for that purpose.
>>
>> In addition to GHA testing, I tested the change on static-jdk with jdk tier1
>> tests on linux-x64 locally. All java/foreign/* jdk tier1 tests pass with the
>> change. Without the change, there are about 60 java/foreign/* jdk tier1
>> tests fail on static-jdk.
>
> Jiangli Zhou has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Merge branch 'JDK-8355080' of ssh://github.com/jianglizhou/jdk into
> JDK-8355080
> - Address henryjen@ comment:
>- Remove '#include '.
src/java.base/share/classes/jdk/internal/foreign/SystemLookup.java line 135:
> 133: @SuppressWarnings("restricted")
> 134: private static SymbolLookup sysLookup() {
> 135: NativeLibraries libs = NativeLibraries.newInstance(null);
I would have @mcimadamore or @JornVernee look at this for the native function
call permission requirements.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24801#discussion_r2058685087
Re: RFR: 8347408: Create an internal method handle adapter for system calls with errno [v4]
> As we advance, converting older JDK code to use the relatively new FFM API
> requires system calls that can provide `errno` and the likes to explicitly
> allocate a `MemorySegment` to capture potential error states. This can lead
> to negative performance implications if not designed carefully and also
> introduces unnecessary code complexity.
>
> Hence, this PR proposes adding a JDK internal method handle adapter that can
> be used to handle system calls with `errno`, `GetLastError`, and
> `WSAGetLastError`.
>
> It relies on an efficient carrier-thread-local cache of memory regions to
> allide allocations.
>
>
> Here are some benchmarks that ran on a platform thread and virtual threads
> respectively (M1 Mac):
>
>
> Benchmark Mode Cnt Score
> Error Units
> CaptureStateUtilBench.OfVirtual.adaptedSysCallFail avgt 30 24.330
> ? 0.820 ns/op
> CaptureStateUtilBench.OfVirtual.adaptedSysCallSuccess avgt 30 8.257
> ? 0.117 ns/op
> CaptureStateUtilBench.OfVirtual.explicitAllocationFail avgt 30 41.415
> ? 1.013 ns/op
> CaptureStateUtilBench.OfVirtual.explicitAllocationSuccess avgt 30 21.720
> ? 0.463 ns/op
> CaptureStateUtilBench.OfVirtual.tlAllocationFail avgt 30 23.636
> ? 0.182 ns/op
> CaptureStateUtilBench.OfVirtual.tlAllocationSuccessavgt 30 8.234
> ? 0.156 ns/op
> CaptureStateUtilBench.adaptedSysCallFail avgt 30 23.918
> ? 0.487 ns/op
> CaptureStateUtilBench.adaptedSysCallSuccessavgt 30 4.946
> ? 0.089 ns/op
> CaptureStateUtilBench.explicitAllocationFail avgt 30 42.280
> ? 1.128 ns/op
> CaptureStateUtilBench.explicitAllocationSuccessavgt 30 21.809
> ? 0.413 ns/op
> CaptureStateUtilBench.tlAllocationFail avgt 30 24.422
> ? 0.673 ns/op
> CaptureStateUtilBench.tlAllocationSuccess avgt 30 5.182
> ? 0.152 ns/op
>
>
> Adapted system call:
>
> return (int) ADAPTED_HANDLE.invoke(0, 0); // Uses a MH-internal pool
> ```
> Explicit allocation:
>
> try (var arena = Arena.ofConfined()) {
> return (int) HANDLE.invoke(arena.allocate(4), 0, 0);
> }
> ```
> Thread Local allocation:
>
> try (var arena = POOLS.take()) {
> return (int) HANDLE.invoke(arena.allocate(4), 0, 0); // Uses a
> manually specified pool
> }
> ```
> The adapted system call exhibits a ~4x performance improvement over the
> existing "explicit allocation" scheme for the happy path on platform threads.
> ...
Per Minborg has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains 35 commits:
- Merge master
- Merge master
- Add test for woven allocation
- Merge branch 'master' into errno-util3
- Use lazy initialization of method handles
- Clean up visibility
- Merge branch 'master' into errno-util3
- Add @ForceInline annotations
- Add out of order test for VTs
- Allow memory reuse for several arenas
- ... and 25 more: https://git.openjdk.org/jdk/compare/290d24d1...1a31ae04
-
Changes: https://git.openjdk.org/jdk/pull/23765/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23765&range=03
Stats: 1911 lines in 13 files changed: 1901 ins; 0 del; 10 mod
Patch: https://git.openjdk.org/jdk/pull/23765.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/23765/head:pull/23765
PR: https://git.openjdk.org/jdk/pull/23765
Re: RFR: 8355369: Remove setAccessible usage for setting final fields in java.util.concurrent [v2]
On Wed, 23 Apr 2025 15:22:37 GMT, Viktor Klang wrote: >> This Pull Request replaces the uses of Field + setAccessible to modify final >> fields in java.util.concurrent with Unsafe. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Adding a releaseFence() to ConcurrentskipListSet.clone() LGTM - Marked as reviewed by rgiulietti (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/24821#pullrequestreview-2790790850
Re: RFR: 8355369: Remove setAccessible usage for setting final fields in java.util.concurrent [v2]
On Thu, 24 Apr 2025 11:27:32 GMT, Doug Lea wrote: >> The set only has a map field, and AbstractSet does not define any additional >> field. The map should be fine too - two fields in AbstractMap are cleared >> when cloning happens, so recreating a map from a constructor should have the >> same effect. (Note a significant field `head` is not cleared upon clone, but >> seems immediately replaced later in `buildFromSorted`). Both should still be >> fine with this new return values. > > As others have mentioned, clone() needs to be re-checked wrt issuing final > field fences. But can be conservatively done so here anyway. @DougLea Yeah. I'd be surprised if `clone()`-invocations were specially tracked to get a trailing release fence inserted before handing out the instance. - PR Review Comment: https://git.openjdk.org/jdk/pull/24821#discussion_r2058547956
Integrated: 8355369: Remove setAccessible usage for setting final fields in java.util.concurrent
On Wed, 23 Apr 2025 10:10:54 GMT, Viktor Klang wrote: > This Pull Request replaces the uses of Field + setAccessible to modify final > fields in java.util.concurrent with Unsafe. This pull request has now been integrated. Changeset: 356c4d9c Author:Viktor Klang URL: https://git.openjdk.org/jdk/commit/356c4d9ca93c8a37231e86d583ce9628d693c733 Stats: 32 lines in 3 files changed: 7 ins; 6 del; 19 mod 8355369: Remove setAccessible usage for setting final fields in java.util.concurrent Reviewed-by: pminborg, dl, rgiulietti, alanb - PR: https://git.openjdk.org/jdk/pull/24821
Re: RFR: 8353786: Migrate Vector API math library support to FFM API [v13]
On Wed, 23 Apr 2025 23:54:01 GMT, Vladimir Ivanov wrote: >> Migrate Vector API math library (SVML and SLEEF) linkage from native code >> (in JVM) to Java FFM API. >> >> Since FFM API doesn't support vector calling conventions yet, migration >> affects only symbol lookup for now. But it still enables significant >> simplifications on JVM side. >> >> The patch consists of the following parts: >> * on-demand symbol lookup in Java code replaces eager lookup from native >> code during JVM startup; >> * 2 new VM intrinsics for vector calls (support unary and binary shapes) >> (code separated from unary/binary vector operations); >> * new internal interface to query supported CPU ISA extensions >> (`jdk.incubator.vector.CPUFeatures`) used for CPU dispatching. >> >> `java.lang.foreign` API is used to perform symbol lookup in vector math >> library, then the address is cached and fed into corresponding JVM >> intrinsic, so C2 can turn it into a direct vector call in generated code. >> >> Once `java.lang.foreign` supports vectors & vector calling conventions, VM >> intrinsics can go away. >> >> Performance is on par with original implementation (tested with >> microbenchmarks on linux-x64 and macosx-aarch64). >> >> Testing: hs-tier1 - hs-tier6, microbenchmarks (on linux-x64 and >> macosx-aarch64) >> >> Thanks! > > Vladimir Ivanov has updated the pull request incrementally with one > additional commit since the last revision: > > CPUFeatures: RISC-V support Marked as reviewed by mli (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24462#pullrequestreview-2790760838
Re: RFR: 8355369: Remove setAccessible usage for setting final fields in java.util.concurrent [v2]
On Wed, 23 Apr 2025 16:23:00 GMT, Chen Liang wrote: >> Viktor Klang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Adding a releaseFence() to ConcurrentskipListSet.clone() > > Thanks. We can investigate how clone cleanups interact with truly final or > strict finals in another issue. @liach @AlanBateman My proposal here would be to move forward with the proposed changes for now, and then once 25 is branched off there's an opportunity to take a broader look at `clone()`. - PR Comment: https://git.openjdk.org/jdk/pull/24821#issuecomment-2827154019
Re: RFR: 8355369: Remove setAccessible usage for setting final fields in java.util.concurrent [v2]
On Wed, 23 Apr 2025 15:22:37 GMT, Viktor Klang wrote: >> This Pull Request replaces the uses of Field + setAccessible to modify final >> fields in java.util.concurrent with Unsafe. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Adding a releaseFence() to ConcurrentskipListSet.clone() I think these changes look good. Before we integrate, could you please raise an issue with the future improvements of `clone()` and friends so that we do not forget that? - Marked as reviewed by pminborg (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/24821#pullrequestreview-2790715897
Re: RFR: 8355369: Remove setAccessible usage for setting final fields in java.util.concurrent [v2]
On Wed, 23 Apr 2025 12:38:55 GMT, Alan Bateman wrote: >> Yeah, I originally did that, but the following patch is the "smallest >> change". >> Given that the "original code" obtained the Field instance each call, this >> is still likely a performance improvement. > > This code has always used getDeclaredField each time so if anyone had run > into a performance issue then we should have heard by now. So I think keeping > the changes simple and just moving to Unsafe is okay for now. Fine. The main effect of this change is to nearly revert to the pre-VarHandle version of this code. - PR Review Comment: https://git.openjdk.org/jdk/pull/24821#discussion_r2058157158
Re: RFR: 8355443: [java.io] Use @requires tag instead of exiting based on File.separatorChar value
On Thu, 24 Apr 2025 00:33:20 GMT, Brian Burkhalter wrote:
> For tests of the `java.io` package, instead of doing this
>
> public static void main(String[] args) throws Exception {
> if (File.separatorChar != '\') {
> /* This test is only valid on win32 systems */
> return;
> }
>
> do this
>
>@requires (os.family == "windows")
Nice cleanup.
test/jdk/java/io/pathNames/win32/SJIS.java line 51:
> 49:that use the SJIS encoding */
> 50: String enc = System.getProperty("file.encoding");
> 51: if ((enc == null) || !enc.equals("SJIS")) return;
I just wonder this test has ever run since JDK18, as file.encoding is always
UTF-8 unless COMPAT is specified (and this test case does not specify it)
-
PR Review: https://git.openjdk.org/jdk/pull/24838#pullrequestreview-2791863041
PR Review Comment: https://git.openjdk.org/jdk/pull/24838#discussion_r2058850665
Re: RFR: 8348986: Improve coverage of enhanced exception messages [v7]
> Hi, > > Enhanced exception messages are designed to hide sensitive information such > as hostnames, IP > addresses from exception message strings, unless the enhanced mode for the > specific category > has been explicitly enabled. Enhanced exceptions were first introduced in > 8204233 in JDK 11 and > updated in 8207846. > > This PR aims to increase the coverage of enhanced exception messages in the > networking code. > A limited number of exceptions are already hidden (restricted) by default. > The new categories and > exceptions in this PR will be restricted on an opt-in basis, ie. the default > mode will be enhanced > (while preserving the existing behavior). > > The mechanism is controlled by the security/system property > "jdk.includeInExceptions" which takes as value > a comma separated list of category names, which identify groups of exceptions > where the exception > message may be enhanced. Any category not listed is "restricted" which means > that potentially > sensitive information (such as hostnames, IP addresses, user identities) are > excluded from the message text. > > The changes to the java.security conf file describe the exact changes in > terms of the categories now > supported and any changes in behavior. > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: Review update - Changes: - all: https://git.openjdk.org/jdk/pull/23929/files - new: https://git.openjdk.org/jdk/pull/23929/files/da33863c..efabc485 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23929&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23929&range=05-06 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/23929.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23929/head:pull/23929 PR: https://git.openjdk.org/jdk/pull/23929
Re: RFR: 8355443: [java.io] Use @requires tag instead of exiting based on File.separatorChar value
On Thu, 24 Apr 2025 00:33:20 GMT, Brian Burkhalter wrote:
> For tests of the `java.io` package, instead of doing this
>
> public static void main(String[] args) throws Exception {
> if (File.separatorChar != '\') {
> /* This test is only valid on win32 systems */
> return;
> }
>
> do this
>
>@requires (os.family == "windows")
Marked as reviewed by naoto (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/24838#pullrequestreview-2791975852
Re: RFR: 8355443: [java.io] Use @requires tag instead of exiting based on File.separatorChar value
On Thu, 24 Apr 2025 17:07:15 GMT, Naoto Sato wrote: >> So do you think that this test should be removed? If so, I think that would >> be a different PR. Thanks. > > IIUC, the test tries to create double byte path directory, so in recent jdk > it should check `sun.jnu.encoding==MS932` (it still only run on Japanese > Windows though) Sounds like a different issue needs to be filed. - PR Review Comment: https://git.openjdk.org/jdk/pull/24838#discussion_r2058912342
Re: RFR: 8353786: Migrate Vector API math library support to FFM API [v13]
On Wed, 23 Apr 2025 23:54:01 GMT, Vladimir Ivanov wrote: >> Migrate Vector API math library (SVML and SLEEF) linkage from native code >> (in JVM) to Java FFM API. >> >> Since FFM API doesn't support vector calling conventions yet, migration >> affects only symbol lookup for now. But it still enables significant >> simplifications on JVM side. >> >> The patch consists of the following parts: >> * on-demand symbol lookup in Java code replaces eager lookup from native >> code during JVM startup; >> * 2 new VM intrinsics for vector calls (support unary and binary shapes) >> (code separated from unary/binary vector operations); >> * new internal interface to query supported CPU ISA extensions >> (`jdk.incubator.vector.CPUFeatures`) used for CPU dispatching. >> >> `java.lang.foreign` API is used to perform symbol lookup in vector math >> library, then the address is cached and fed into corresponding JVM >> intrinsic, so C2 can turn it into a direct vector call in generated code. >> >> Once `java.lang.foreign` supports vectors & vector calling conventions, VM >> intrinsics can go away. >> >> Performance is on par with original implementation (tested with >> microbenchmarks on linux-x64 and macosx-aarch64). >> >> Testing: hs-tier1 - hs-tier6, microbenchmarks (on linux-x64 and >> macosx-aarch64) >> >> Thanks! > > Vladimir Ivanov has updated the pull request incrementally with one > additional commit since the last revision: > > CPUFeatures: RISC-V support src/hotspot/share/opto/vectorIntrinsics.cpp line 563: > 561: debug_name = > debug_name_oop->const_oop()->as_instance()->java_lang_String_str(buf, buflen); > 562: } > 563: Node* vcall = make_runtime_call(RC_VECTOR, By generating an upfront CallLeafVectorNode, we may miss out on performing any GVN-style optimization for trigonometric identities like the following. do you think creating a macro node which can lazily be expanded to call node during macro expansion will help. arcsin(sin(x)) => x arccos(cos(x)) => x sin(arcsin(x) => x cos(arccos(x) => x - PR Review Comment: https://git.openjdk.org/jdk/pull/24462#discussion_r2059057257
Re: RFR: 8355443: [java.io] Use @requires tag instead of exiting based on File.separatorChar value
On Thu, 24 Apr 2025 16:55:16 GMT, Brian Burkhalter wrote:
>> test/jdk/java/io/pathNames/win32/SJIS.java line 51:
>>
>>> 49:that use the SJIS encoding */
>>> 50: String enc = System.getProperty("file.encoding");
>>> 51: if ((enc == null) || !enc.equals("SJIS")) return;
>>
>> I just wonder this test has ever run since JDK18, as file.encoding is always
>> UTF-8 unless COMPAT is specified (and this test case does not specify it)
>
> So do you think that this test should be removed? If so, I think that would
> be a different PR. Thanks.
IIUC, the test tries to create double byte path directory, so in recent jdk it
should check `sun.jnu.encoding==MS932` (it still only run on Japanese Windows
though)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24838#discussion_r205444
Integrated: 8354424: java/util/logging/LoggingDeadlock5.java fails intermittently in tier6
On Wed, 16 Apr 2025 12:12:14 GMT, David Beaumont wrote: > Increasing timeout for deadlock detection and adjusting for the timeout > factor in higher tiers. This pull request has now been integrated. Changeset: e01e33d1 Author:David Beaumont Committer: Daniel Fuchs URL: https://git.openjdk.org/jdk/commit/e01e33d19b94ee85f7cb7cd6baec857a50086c76 Stats: 11 lines in 2 files changed: 9 ins; 1 del; 1 mod 8354424: java/util/logging/LoggingDeadlock5.java fails intermittently in tier6 Reviewed-by: dfuchs, smarks - PR: https://git.openjdk.org/jdk/pull/24687
Re: RFR: 8353786: Migrate Vector API math library support to FFM API [v13]
On Thu, 24 Apr 2025 18:57:11 GMT, Jatin Bhateja wrote: >> Vladimir Ivanov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> CPUFeatures: RISC-V support > > src/hotspot/share/opto/vectorIntrinsics.cpp line 563: > >> 561: debug_name = >> debug_name_oop->const_oop()->as_instance()->java_lang_String_str(buf, >> buflen); >> 562: } >> 563: Node* vcall = make_runtime_call(RC_VECTOR, > > By generating an upfront CallLeafVectorNode, we may miss out on performing > any GVN-style optimization for trigonometric identities like the following. > do you think creating a macro node which can lazily be expanded to call node > during macro expansion will help. > > arcsin(sin(x)) => x > arccos(cos(x)) => x > sin(arcsin(x) => x > cos(arccos(x) => x It does look attractive, but macro expansion-based solution requires JVM to internalize such operations and their properties. IMO a higher-level solution based on more generic JVM primitives would enable libraries to properly annotate their operations in Java bytecodes/class files, so C2 can perform such type of transformations without the need to intrinsify each individual operation first. (Think of [JDK-8218414](https://bugs.openjdk.org/browse/JDK-8218414) / [JDK-8347901](https://bugs.openjdk.org/browse/JDK-8347901) on steroids.) - PR Review Comment: https://git.openjdk.org/jdk/pull/24462#discussion_r2059356271
Re: RFR: 8353786: Migrate Vector API math library support to FFM API [v14]
> Migrate Vector API math library (SVML and SLEEF) linkage from native code (in > JVM) to Java FFM API. > > Since FFM API doesn't support vector calling conventions yet, migration > affects only symbol lookup for now. But it still enables significant > simplifications on JVM side. > > The patch consists of the following parts: > * on-demand symbol lookup in Java code replaces eager lookup from native > code during JVM startup; > * 2 new VM intrinsics for vector calls (support unary and binary shapes) > (code separated from unary/binary vector operations); > * new internal interface to query supported CPU ISA extensions > (`jdk.incubator.vector.CPUFeatures`) used for CPU dispatching. > > `java.lang.foreign` API is used to perform symbol lookup in vector math > library, then the address is cached and fed into corresponding JVM intrinsic, > so C2 can turn it into a direct vector call in generated code. > > Once `java.lang.foreign` supports vectors & vector calling conventions, VM > intrinsics can go away. > > Performance is on par with original implementation (tested with > microbenchmarks on linux-x64 and macosx-aarch64). > > Testing: hs-tier1 - hs-tier6, microbenchmarks (on linux-x64 and > macosx-aarch64) > > Thanks! Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision: Improve comments - Changes: - all: https://git.openjdk.org/jdk/pull/24462/files - new: https://git.openjdk.org/jdk/pull/24462/files/585312ae..541c4d7f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24462&range=13 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24462&range=12-13 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/24462.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24462/head:pull/24462 PR: https://git.openjdk.org/jdk/pull/24462
Re: RFR: 8353786: Migrate Vector API math library support to FFM API [v12]
On Thu, 24 Apr 2025 11:33:43 GMT, Hamlin Li wrote: >> FTR both `VectorSupport.getMaxLaneCount()` and `CPUFeatures` don't rely on >> raw list of ISA extensions CPU supports, but only those reported by the JVM. >> So, if some feature support is disabled on JVM side, it won't be reported by >> `VM_Version` and, hence, `CPUFeatures`. > > Thank you for updating! Looks good for riscv. I have ran some basic tests for > vector API, passed. I did not ran benchmark, as riscv & aarch64 share the > same way to bridge from java to sleef. > >> Does the following check catch UseRVV == false case on RISC-V? > > Yes. If you don't mind, an explicit comment might be helpful. As to me > "lacking vector support" here means the vector length is not large enough, > but it's quite subjective, so you are on the call. > >> FTR both VectorSupport.getMaxLaneCount() and CPUFeatures don't rely on raw >> list of ISA extensions CPU supports, but only those reported by the JVM. So, >> if some feature support is disabled on JVM side, it won't be reported by >> VM_Version and, hence, CPUFeatures. > > I'm fine with this. Thanks, I added some clarifications in the comments. - PR Review Comment: https://git.openjdk.org/jdk/pull/24462#discussion_r2059367624
Re: RFR: 8351565: Implement JEP 502: Stable Values (Preview) [v76]
On Thu, 24 Apr 2025 00:05:04 GMT, Chen Liang wrote: >> Per Minborg has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Replace 'contents' with 'result' in the docs > > src/java.base/share/classes/java/util/Collection.java line 61: > >> 59: * implementations in the Java platform libraries comply. >> 60: * >> 61: * Certain methods are specified to be > > Suggestion: > > * Certain methods are specified to be I want to keep the style as the existing one used in other places in the class. - PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2057933419
Re: RFR: 8351565: Implement JEP 502: Stable Values (Preview) [v77]
> Implement JEP 502. > > The PR passes tier1-tier3 tests. Per Minborg has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 370 commits: - Revert unwanted changes - Merge branch 'master' into implement-jep502 - Address comments - Replace 'contents' with 'result' in the docs - Rephrase happens-before clause - Replace 'content' with 'contents' and doc updates - Merge branch 'master' into implement-jep502 - Remove section on fun/coll sync - Reformat docs - Fix failing test (exception message) - ... and 360 more: https://git.openjdk.org/jdk/compare/290d24d1...bcc022fe - Changes: https://git.openjdk.org/jdk/pull/23972/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23972&range=76 Stats: 4807 lines in 30 files changed: 4795 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/23972.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23972/head:pull/23972 PR: https://git.openjdk.org/jdk/pull/23972
Re: RFR: 8077587: BigInteger Roots [v28]
> This PR implements nth root computation for `BigInteger`s using Newton method > and optimizes `BigInteger.pow(int)` method. > [Here is a proof of convergence of the recurrence > used.](https://github.com/user-attachments/files/19785045/nth_root_newton_proof_integers.pdf) fabioromano1 has updated the pull request incrementally with one additional commit since the last revision: Optimized BigInteger.pow(int) for single-word values - Changes: - all: https://git.openjdk.org/jdk/pull/24690/files - new: https://git.openjdk.org/jdk/pull/24690/files/f0d06055..f20d19be Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24690&range=27 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24690&range=26-27 Stats: 64 lines in 1 file changed: 55 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/24690.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24690/head:pull/24690 PR: https://git.openjdk.org/jdk/pull/24690
Re: RFR: 8351565: Implement JEP 502: Stable Values (Preview) [v77]
On Thu, 24 Apr 2025 09:21:03 GMT, Per Minborg wrote:
>> Implement JEP 502.
>>
>> The PR passes tier1-tier3 tests.
>
> Per Minborg has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 370 commits:
>
> - Revert unwanted changes
> - Merge branch 'master' into implement-jep502
> - Address comments
> - Replace 'contents' with 'result' in the docs
> - Rephrase happens-before clause
> - Replace 'content' with 'contents' and doc updates
> - Merge branch 'master' into implement-jep502
> - Remove section on fun/coll sync
> - Reformat docs
> - Fix failing test (exception message)
> - ... and 360 more: https://git.openjdk.org/jdk/compare/290d24d1...bcc022fe
src/java.base/share/classes/java/util/ImmutableCollections.java line 888:
> 886: private static final class StableReverseOrderListView extends
> ReverseOrderListView.Rand {
> 887:
> 888: public StableReverseOrderListView(List base) {
public?
src/java.base/share/classes/java/util/ReverseOrderListView.java line 300:
> 298: public String toString() {
> 299: Iterator it = iterator();
> 300: if (! it.hasNext())
Suggestion:
if (!it.hasNext())
src/java.base/share/classes/java/util/ReverseOrderListView.java line 308:
> 306: E e = it.next();
> 307: sb.append(e == this ? "(this Collection)" : e);
> 308: if (! it.hasNext())
Suggestion:
if (!it.hasNext())
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2057967620
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2057958943
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2057960785
Re: RFR: 8354242: VectorAPI: combine vector not operation with compare [v2]
On Fri, 18 Apr 2025 01:36:10 GMT, erifan wrote: >> This patch optimizes the following patterns: >> For integer types: >> >> (XorV (VectorMaskCmp src1 src2 cond) (Replicate -1)) >> => (VectorMaskCmp src1 src2 ncond) >> (XorVMask (VectorMaskCmp src1 src2 cond) (MaskAll m1)) >> => (VectorMaskCmp src1 src2 ncond) >> >> cond can be eq, ne, le, ge, lt, gt, ule, uge, ult and ugt, ncond is the >> negative comparison of cond. >> >> For float and double types: >> >> (XorV (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (Replicate -1)) >> => (VectorMaskCast (VectorMaskCmp src1 src2 ncond)) >> (XorVMask (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (MaskAll m1)) >> => (VectorMaskCast (VectorMaskCmp src1 src2 ncond)) >> >> cond can be eq or ne. >> >> Benchmarks on Nvidia Grace machine with 128-bit SVE2: With option >> `-XX:UseSVE=2`: >> >> BenchmarkUnitBefore Score Error After >> Score Error Uplift >> testCompareEQMaskNotByte ops/s 7912127.225 2677.289518 >> 10266136.26 8955.008548 1.29 >> testCompareEQMaskNotDouble ops/s 884737.6799 446.963779 >> 1179760.772 448.031844 1.33 >> testCompareEQMaskNotFloatops/s 1765045.787 682.332214 >> 2359520.803 896.305743 1.33 >> testCompareEQMaskNotInt ops/s 1787221.411 977.743935 >> 2353952.519 960.069976 1.31 >> testCompareEQMaskNotLong ops/s 895297.1974 673.44808 >> 1178449.02 323.804205 1.31 >> testCompareEQMaskNotShortops/s 3339987.002 3415.2226 >> 4712761.965 2110.862053 1.41 >> testCompareGEMaskNotByte ops/s 7907615.16 4094.243652 >> 10251646.9 9486.699831 1.29 >> testCompareGEMaskNotInt ops/s 1683738.958 4233.813092 >> 2352855.205 1251.952546 1.39 >> testCompareGEMaskNotLong ops/s 854496.1561 8594.598885 >> 1177811.493 521.12291.37 >> testCompareGEMaskNotShortops/s 3341860.309 1578.975338 >> 4714008.434 1681.10365 1.41 >> testCompareGTMaskNotByte ops/s 7910823.674 2993.367032 >> 10245063.58 9774.75138 1.29 >> testCompareGTMaskNotInt ops/s 1673393.928 3153.099431 >> 2353654.521 1190.848583 1.4 >> testCompareGTMaskNotLong ops/s 849405.9159 2432.858159 >> 1177952.041 359.96413 1.38 >> testCompareGTMaskNotShortops/s 3339509.141 3339.976585 >> 4711442.496 2673.364893 1.41 >> testCompareLEMaskNotByte ops/s 7911340.004 3114.69191 >> 10231626.5 27134.20035 1.29 >> testCompareLEMaskNotInt ops/s 1675812.113 1340.969885 >> 2353255.341 1452.4522 1.4 >> testCompareLEMaskNotLong ops/s 848862.8036 6564.841731 >> 1177763.623 539.290106 1.38 >> testCompareLEMaskNotShortops/s 3324951.54 2380.29473 >> 4712116.251 1544.559684 1.41 >> testCompareLTMaskNotByte ops/s 7910390.844 2630.861436 >> 10239567.69 6487.441672 1.29 >> testCompareLTMaskNotInt ops/s 16721... > > erifan 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 two additional commits since the > last revision: > > - Merge branch 'master' into JDK-8354242 > - 8354242: VectorAPI: combine vector not operation with compare > >This patch optimizes the following patterns: >For integer types: >``` >(XorV (VectorMaskCmp src1 src2 cond) (Replicate -1)) >=> (VectorMaskCmp src1 src2 ncond) >(XorVMask (VectorMaskCmp src1 src2 cond) (MaskAll m1)) >=> (VectorMaskCmp src1 src2 ncond) >``` >cond can be eq, ne, le, ge, lt, gt, ule, uge, ult and ugt, ncond is the >negative comparison of cond. > >For float and double types: >``` >(XorV (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (Replicate -1)) >=> (VectorMaskCast (VectorMaskCmp src1 src2 ncond)) >(XorVMask (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (MaskAll m1)) >=> (VectorMaskCast (VectorMaskCmp src1 src2 ncond)) >``` >cond can be eq or ne. > >Benchmarks on Nvidia Grace machine with 128-bit SVE2: >With option `-XX:UseSVE=2`: >``` >Benchmark UnitBefore Score Error After > Score Error Uplift >testCompareEQMaskNotByte ops/s 7912127.225 2677.289518 > 10266136.26 8955.008548 1.29 >testCompareEQMaskNotDouble ops/s 884737.6799 446.963779 > 1179760.772 448.031844 1.33 >testCompareEQMaskNotFloat ops/s 1765045.787 682.332214 > 2359520.803 896.305743 1.33 >testCompareEQMaskNotIntops/s 1787221.411 977.743935
Re: RFR: 8077587: BigInteger Roots [v29]
> This PR implements nth root computation for `BigInteger`s using Newton method > and optimizes `BigInteger.pow(int)` method. > [Here is a proof of convergence of the recurrence > used.](https://github.com/user-attachments/files/19785045/nth_root_newton_proof_integers.pdf) fabioromano1 has updated the pull request incrementally with one additional commit since the last revision: Optimized repeated squaring trick using cache for powers - Changes: - all: https://git.openjdk.org/jdk/pull/24690/files - new: https://git.openjdk.org/jdk/pull/24690/files/f20d19be..8676af7c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24690&range=28 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24690&range=27-28 Stats: 45 lines in 1 file changed: 18 ins; 0 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/24690.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24690/head:pull/24690 PR: https://git.openjdk.org/jdk/pull/24690
RFR: 8355475: UNCTest should use an existing UNC path
Please review this test-only PR which updates `UNCTest.java` to use a UNC path which is known to exist. The test currently uses the file URL `file://jdk/LOCAL-JAVA/jdk1.4/win/README.txt`, but since this is unlikely to resolve to an existing UNC path on any CI server, the test doesn't really verify what it's supposed to. This PR changes the test to instead use the path `file://computername/C$/Windows`, which should always be available unless Administrative Shares has been disabled. We detect this case by using File::exists for the path and simply skip the test if it does not exist. I forced this test to run in tier1 on GHA and verified that it ran successfully, without being skipped. Meaning Administrative Shares is enabled in the Windows GHA instance. A test run in Oracle's CI verifying a successful run (without skipping) would be welcome. This is a test only PR, `noreg-self` has been added in the JBS. - Commit messages: - UNCTest should use an existing UNC path Changes: https://git.openjdk.org/jdk/pull/24842/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24842&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8355475 Stats: 33 lines in 1 file changed: 30 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/24842.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24842/head:pull/24842 PR: https://git.openjdk.org/jdk/pull/24842
Re: RFR: 8354242: VectorAPI: combine vector not operation with compare [v2]
On Wed, 23 Apr 2025 12:09:51 GMT, Jatin Bhateja wrote:
>> erifan 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 two additional commits since
>> the last revision:
>>
>> - Merge branch 'master' into JDK-8354242
>> - 8354242: VectorAPI: combine vector not operation with compare
>>
>>This patch optimizes the following patterns:
>>For integer types:
>>```
>>(XorV (VectorMaskCmp src1 src2 cond) (Replicate -1))
>>=> (VectorMaskCmp src1 src2 ncond)
>>(XorVMask (VectorMaskCmp src1 src2 cond) (MaskAll m1))
>>=> (VectorMaskCmp src1 src2 ncond)
>>```
>>cond can be eq, ne, le, ge, lt, gt, ule, uge, ult and ugt, ncond is the
>>negative comparison of cond.
>>
>>For float and double types:
>>```
>>(XorV (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (Replicate -1))
>>=> (VectorMaskCast (VectorMaskCmp src1 src2 ncond))
>>(XorVMask (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (MaskAll m1))
>>=> (VectorMaskCast (VectorMaskCmp src1 src2 ncond))
>>```
>>cond can be eq or ne.
>>
>>Benchmarks on Nvidia Grace machine with 128-bit SVE2:
>>With option `-XX:UseSVE=2`:
>>```
>>Benchmark UnitBefore Score Error After
>> Score Error Uplift
>>testCompareEQMaskNotByte ops/s 7912127.225 2677.289518
>> 10266136.26 8955.008548 1.29
>>testCompareEQMaskNotDoubleops/s 884737.6799 446.963779
>> 1179760.772 448.031844 1.33
>>testCompareEQMaskNotFloat ops/s 1765045.787 682.332214
>> 2359520.803 896.305743 1.33
>>testCompareEQMaskNotInt ops/s 1787221.411 977.743935
>> 2353952.519 960.069976 1.31
>>testCompareEQMaskNotLong ops/s 895297.1974 673.44808
>> 1178449.02 323.804205 1.31
>>testCompareEQMaskNotShort ops/s 3339987.002 3415.2226
>> 4712761.965 2110.862053 1.41
>>testCompareGEMaskNotByte ops/s 7907615.16 4094.243652
>> 10251646.9 9486.699831 1.29
>>testCompareGEMaskNotInt ops/s 1683738.958 4233.813092
>> 2352855.205 1251.952546 1.39
>>testCompareGEMaskNotLong ops/s 854496.1561 8594.598885
>> 1177811.493 521.12291.37
>>testCompareGEMaskNotShort ops/s 3341860.309 1578.975338
>> 4714008.434 1681.10365 1.41
>>testCompareGTMaskNotByte ops/s 7910823.674 2993.367032
>> 10245063.58 9774.75138 1.29
>>testCompareGTMaskNotInt ops/s 1673393.928 3153.099431
>> 2353654.521 1190.848583 1.4
>>testCompareGTMaskNotLong ops/s 849405.9159 2432.858159
>> 1177952.041 359.96413 1.38
>>testCompareGTMaskNotShort ops/s 3339509.141 ...
>
> src/hotspot/share/opto/vectornode.cpp line 2234:
>
>> 2232: // XorV/XorVMask is commutative, swap
>> VectorMaskCmp/Op_VectorMaskCast to in1.
>> 2233: if (in1->Opcode() != Op_VectorMaskCmp && in1->Opcode() !=
>> Op_VectorMaskCast) {
>> 2234: swap(in1, in2);
>
> Swapping inputs like this without refreshing GVN bookkeeping is not safe. I
> guess you wanted to use Node::swap_edges.
The edges are not swapped, but two variables in1 and in2
> src/hotspot/share/opto/vectornode.cpp line 2243:
>
>> 2241: in1 = in1->in(1);
>> 2242: }
>> 2243: if (in1->Opcode() != Op_VectorMaskCmp || in1->outcnt() > 1 ||
>
> Checks on outcnt on line 2243 and 2238 can be removed. Idealization looks for
> a specific graph palette and replaces it with a new node whose inputs are the
> same as the inputs of the palette. GVN will do the retention job if any
> intermediate node has users beyond the pattern being replaced.
Thanks for telling me this information. Another more important reason to check
outcnt here is to prevent this optimization when the uses of VectorMaskCmp is
greater than 1, because this optimization may not be worthwhile. For example:
public static void testVectorMaskCmp() {
IntVector bv = IntVector.fromArray(I_SPECIES, ib, 0);
IntVector av = IntVector.fromArray(I_SPECIES, ia, 0);
VectorMask m1 = av.compare(VectorOperators.NE, bv); // two uses
VectorMask m2 =m1.not();
m1.intoArray(m, 0);
av.lanewise(VectorOperators.ABS, m2).intoArray(ia, 0);
}
If we do not check outcnt and still do this optimization, two VectorMaskCmp
nodes will be generated, and finally two VectorMaskCmp instructions will be
generated. This is unreasonable because VectorMaskCmp has much higher latency
than xor instruction on aarch64.
> src/hotspot/share/opto/vectornode.cpp line 2265:
>
>> 2263: vmcmp = new VectorMaskCastNode(phase->transform(vmcmp), vmcast_vt);
>> 2264: }
>> 2265: return vmcmp;
>
> It would be preferab
Re: RFR: 8354242: VectorAPI: combine vector not operation with compare [v2]
On Fri, 18 Apr 2025 01:36:10 GMT, erifan wrote: >> This patch optimizes the following patterns: >> For integer types: >> >> (XorV (VectorMaskCmp src1 src2 cond) (Replicate -1)) >> => (VectorMaskCmp src1 src2 ncond) >> (XorVMask (VectorMaskCmp src1 src2 cond) (MaskAll m1)) >> => (VectorMaskCmp src1 src2 ncond) >> >> cond can be eq, ne, le, ge, lt, gt, ule, uge, ult and ugt, ncond is the >> negative comparison of cond. >> >> For float and double types: >> >> (XorV (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (Replicate -1)) >> => (VectorMaskCast (VectorMaskCmp src1 src2 ncond)) >> (XorVMask (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (MaskAll m1)) >> => (VectorMaskCast (VectorMaskCmp src1 src2 ncond)) >> >> cond can be eq or ne. >> >> Benchmarks on Nvidia Grace machine with 128-bit SVE2: With option >> `-XX:UseSVE=2`: >> >> BenchmarkUnitBefore Score Error After >> Score Error Uplift >> testCompareEQMaskNotByte ops/s 7912127.225 2677.289518 >> 10266136.26 8955.008548 1.29 >> testCompareEQMaskNotDouble ops/s 884737.6799 446.963779 >> 1179760.772 448.031844 1.33 >> testCompareEQMaskNotFloatops/s 1765045.787 682.332214 >> 2359520.803 896.305743 1.33 >> testCompareEQMaskNotInt ops/s 1787221.411 977.743935 >> 2353952.519 960.069976 1.31 >> testCompareEQMaskNotLong ops/s 895297.1974 673.44808 >> 1178449.02 323.804205 1.31 >> testCompareEQMaskNotShortops/s 3339987.002 3415.2226 >> 4712761.965 2110.862053 1.41 >> testCompareGEMaskNotByte ops/s 7907615.16 4094.243652 >> 10251646.9 9486.699831 1.29 >> testCompareGEMaskNotInt ops/s 1683738.958 4233.813092 >> 2352855.205 1251.952546 1.39 >> testCompareGEMaskNotLong ops/s 854496.1561 8594.598885 >> 1177811.493 521.12291.37 >> testCompareGEMaskNotShortops/s 3341860.309 1578.975338 >> 4714008.434 1681.10365 1.41 >> testCompareGTMaskNotByte ops/s 7910823.674 2993.367032 >> 10245063.58 9774.75138 1.29 >> testCompareGTMaskNotInt ops/s 1673393.928 3153.099431 >> 2353654.521 1190.848583 1.4 >> testCompareGTMaskNotLong ops/s 849405.9159 2432.858159 >> 1177952.041 359.96413 1.38 >> testCompareGTMaskNotShortops/s 3339509.141 3339.976585 >> 4711442.496 2673.364893 1.41 >> testCompareLEMaskNotByte ops/s 7911340.004 3114.69191 >> 10231626.5 27134.20035 1.29 >> testCompareLEMaskNotInt ops/s 1675812.113 1340.969885 >> 2353255.341 1452.4522 1.4 >> testCompareLEMaskNotLong ops/s 848862.8036 6564.841731 >> 1177763.623 539.290106 1.38 >> testCompareLEMaskNotShortops/s 3324951.54 2380.29473 >> 4712116.251 1544.559684 1.41 >> testCompareLTMaskNotByte ops/s 7910390.844 2630.861436 >> 10239567.69 6487.441672 1.29 >> testCompareLTMaskNotInt ops/s 16721... > > erifan 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 two additional commits since the > last revision: > > - Merge branch 'master' into JDK-8354242 > - 8354242: VectorAPI: combine vector not operation with compare > >This patch optimizes the following patterns: >For integer types: >``` >(XorV (VectorMaskCmp src1 src2 cond) (Replicate -1)) >=> (VectorMaskCmp src1 src2 ncond) >(XorVMask (VectorMaskCmp src1 src2 cond) (MaskAll m1)) >=> (VectorMaskCmp src1 src2 ncond) >``` >cond can be eq, ne, le, ge, lt, gt, ule, uge, ult and ugt, ncond is the >negative comparison of cond. > >For float and double types: >``` >(XorV (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (Replicate -1)) >=> (VectorMaskCast (VectorMaskCmp src1 src2 ncond)) >(XorVMask (VectorMaskCast (VectorMaskCmp src1 src2 cond)) (MaskAll m1)) >=> (VectorMaskCast (VectorMaskCmp src1 src2 ncond)) >``` >cond can be eq or ne. > >Benchmarks on Nvidia Grace machine with 128-bit SVE2: >With option `-XX:UseSVE=2`: >``` >Benchmark UnitBefore Score Error After > Score Error Uplift >testCompareEQMaskNotByte ops/s 7912127.225 2677.289518 > 10266136.26 8955.008548 1.29 >testCompareEQMaskNotDouble ops/s 884737.6799 446.963779 > 1179760.772 448.031844 1.33 >testCompareEQMaskNotFloat ops/s 1765045.787 682.332214 > 2359520.803 896.305743 1.33 >testCompareEQMaskNotIntops/s 1787221.411 977.743935
Re: RFR: 8355443: [java.io] Use @requires tag instead of exiting based on File.separatorChar value
On Thu, 24 Apr 2025 00:33:20 GMT, Brian Burkhalter wrote:
> For tests of the `java.io` package, instead of doing this
>
> public static void main(String[] args) throws Exception {
> if (File.separatorChar != '\') {
> /* This test is only valid on win32 systems */
> return;
> }
>
> do this
>
>@requires (os.family == "windows")
Marked as reviewed by vyazici (Author).
-
PR Review: https://git.openjdk.org/jdk/pull/24838#pullrequestreview-2790211663
Re: RFR: 8351565: Implement JEP 502: Stable Values (Preview) [v78]
> Implement JEP 502. > > The PR passes tier1-tier3 tests. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Make public constuctor private - Changes: - all: https://git.openjdk.org/jdk/pull/23972/files - new: https://git.openjdk.org/jdk/pull/23972/files/bcc022fe..4839d186 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23972&range=77 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23972&range=76-77 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/23972.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23972/head:pull/23972 PR: https://git.openjdk.org/jdk/pull/23972
Re: RFR: 8351565: Implement JEP 502: Stable Values (Preview) [v77]
On Thu, 24 Apr 2025 09:29:14 GMT, Viktor Klang wrote: >> Per Minborg has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 370 commits: >> >> - Revert unwanted changes >> - Merge branch 'master' into implement-jep502 >> - Address comments >> - Replace 'contents' with 'result' in the docs >> - Rephrase happens-before clause >> - Replace 'content' with 'contents' and doc updates >> - Merge branch 'master' into implement-jep502 >> - Remove section on fun/coll sync >> - Reformat docs >> - Fix failing test (exception message) >> - ... and 360 more: https://git.openjdk.org/jdk/compare/290d24d1...bcc022fe > > src/java.base/share/classes/java/util/ReverseOrderListView.java line 308: > >> 306: E e = it.next(); >> 307: sb.append(e == this ? "(this Collection)" : e); >> 308: if (! it.hasNext()) > > Suggestion: > > if (!it.hasNext()) Reverted. - PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2058076032
Re: RFR: 8351565: Implement JEP 502: Stable Values (Preview) [v78]
On Thu, 24 Apr 2025 10:37:59 GMT, Per Minborg wrote: >> Implement JEP 502. >> >> The PR passes tier1-tier3 tests. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Make public constuctor private 👍 - Marked as reviewed by vklang (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23972#pullrequestreview-2790601691
Re: RFR: 8355080: java.base/jdk.internal.foreign.SystemLookup.find() doesn't work on static JDK [v3]
On Tue, 22 Apr 2025 21:32:48 GMT, Chen Liang wrote: >> Jiangli Zhou has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Merge branch 'JDK-8355080' of ssh://github.com/jianglizhou/jdk into >> JDK-8355080 >> - Address henryjen@ comment: >>- Remove '#include '. > > `sysLookup` does look much cleaner compared to `jdkLibraryPath`. @liach @slowhog Can you approve the change? Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/24801#issuecomment-2827970628
Re: RFR: 8355443: [java.io] Use @requires tag instead of exiting based on File.separatorChar value
On Thu, 24 Apr 2025 17:20:01 GMT, Brian Burkhalter wrote: >> IIUC, the test tries to create double byte path directory, so in recent jdk >> it should check `sun.jnu.encoding==MS932` (it still only run on Japanese >> Windows though) > > Sounds like a different issue needs to be filed. Yeah, that sounds better. - PR Review Comment: https://git.openjdk.org/jdk/pull/24838#discussion_r2058915500
Re: RFR: 8355301: Simplify Throwable::printStackTrace by replacing inner class with static method [v5]
> The current Throwable::printStackTrace implementation uses three inner > classes PrintStreamOrWriter/WrappedPrintStream/WrappedPrintWriter. We can > introduce a static method println to replace these three embedded classes. Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision: use interface + record - Changes: - all: https://git.openjdk.org/jdk/pull/24795/files - new: https://git.openjdk.org/jdk/pull/24795/files/b9050803..fd70d1ad Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24795&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24795&range=03-04 Stats: 61 lines in 1 file changed: 32 ins; 13 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/24795.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24795/head:pull/24795 PR: https://git.openjdk.org/jdk/pull/24795
Re: RFR: 8355080: java.base/jdk.internal.foreign.SystemLookup.find() doesn't work on static JDK [v3]
On Thu, 24 Apr 2025 17:33:19 GMT, Jiangli Zhou wrote:
>> src/java.base/share/classes/jdk/internal/foreign/SystemLookup.java line 135:
>>
>>> 133: @SuppressWarnings("restricted")
>>> 134: private static SymbolLookup sysLookup() {
>>> 135: NativeLibraries libs = NativeLibraries.newInstance(null);
>>
>> I would consult @mcimadamore or @JornVernee look at this for the native
>> function call permission requirements.
>
> @mcimadamore or @JornVernee Can you help take a look of this? Thanks!
Unfortunately, I believe both of them are on vacation, yet they have the
required expertise to review this PR. You might need to wait a bit, sorry!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24801#discussion_r2058934598
Re: RFR: 8355080: java.base/jdk.internal.foreign.SystemLookup.find() doesn't work on static JDK [v3]
On Thu, 24 Apr 2025 15:14:27 GMT, Henry Jen wrote:
>> Jiangli Zhou has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Merge branch 'JDK-8355080' of ssh://github.com/jianglizhou/jdk into
>> JDK-8355080
>> - Address henryjen@ comment:
>>- Remove '#include '.
>
> src/java.base/share/classes/jdk/internal/foreign/SystemLookup.java line 135:
>
>> 133: @SuppressWarnings("restricted")
>> 134: private static SymbolLookup sysLookup() {
>> 135: NativeLibraries libs = NativeLibraries.newInstance(null);
>
> I would consult @mcimadamore or @JornVernee look at this for the native
> function call permission requirements.
@mcimadamore or @JornVernee Can you help take a look of this? Thanks!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24801#discussion_r2058930806
RFR: 8355524: Only every second line in upgradeable files is being used
Please review this fix to [JDK-8353185](https://bugs.openjdk.org/browse/JDK-8353185). The reading logic for the config file would erroneously use `scanner.nextLine()` when the current line to be added is in `line`. `line` is not being added, but `scanner.nextLine()` unintentionally skipping lines. The config file has a change too. It moves the `tzdb.dat` line last so that the existing test verifies that the `cacerts` one isn't being skipped. This slipped through because `tzdb.dat` updates aren't automatically tested. Testing: - [ ] GHA - [x] The test from [JDK-8353185](https://bugs.openjdk.org/browse/JDK-8353185), `UpgradeableFileCacertsTest.java` fails with the config file change only (without the product fix) and passes with the one-liner. Also some manual testing when both files have been upgraded. Thoughts? - Commit messages: - 8355524: Only every second line in upgradable files is being read Changes: https://git.openjdk.org/jdk/pull/24855/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24855&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8355524 Stats: 3 lines in 2 files changed: 1 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/24855.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24855/head:pull/24855 PR: https://git.openjdk.org/jdk/pull/24855
Re: RFR: 8355524: Only every second line in upgradeable files is being used
On Thu, 24 Apr 2025 17:31:50 GMT, Severin Gehwolf wrote: > Please review this fix to > [JDK-8353185](https://bugs.openjdk.org/browse/JDK-8353185). The reading logic > for the config file would erroneously use `scanner.nextLine()` when the > current line to be added is in `line`. `line` is not being added, but > `scanner.nextLine()` unintentionally skipping lines. The config file has a > change too. It moves the `tzdb.dat` line last so that the existing test > verifies that the `cacerts` one isn't being skipped. This slipped through > because `tzdb.dat` updates aren't automatically tested. > > Testing: > - [ ] GHA > - [x] The test from > [JDK-8353185](https://bugs.openjdk.org/browse/JDK-8353185), > `UpgradeableFileCacertsTest.java` fails with the config file change only > (without the product fix) and passes with the one-liner. Also some manual > testing when both files have been upgraded. > > Thoughts? Ha! The bugfixes like these should be reserved for Friday evenings :) - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/24855#pullrequestreview-2792170097
Re: RFR: 8355443: [java.io] Use @requires tag instead of exiting based on File.separatorChar value
On Thu, 24 Apr 2025 16:41:27 GMT, Naoto Sato wrote:
>> For tests of the `java.io` package, instead of doing this
>>
>> public static void main(String[] args) throws Exception {
>> if (File.separatorChar != '\') {
>> /* This test is only valid on win32 systems */
>> return;
>> }
>>
>> do this
>>
>>@requires (os.family == "windows")
>
> test/jdk/java/io/pathNames/win32/SJIS.java line 51:
>
>> 49:that use the SJIS encoding */
>> 50: String enc = System.getProperty("file.encoding");
>> 51: if ((enc == null) || !enc.equals("SJIS")) return;
>
> I just wonder this test has ever run since JDK18, as file.encoding is always
> UTF-8 unless COMPAT is specified (and this test case does not specify it)
So do you think that this test should be removed? If so, I think that would be
a different PR. Thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24838#discussion_r2058870526
Re: RFR: 8354424: java/util/logging/LoggingDeadlock5.java fails intermittently in tier6 [v2]
On Wed, 16 Apr 2025 17:37:07 GMT, David Beaumont wrote: >> Increasing timeout for deadlock detection and adjusting for the timeout >> factor in higher tiers. > > David Beaumont has updated the pull request incrementally with one additional > commit since the last revision: > > Removing test from the problem list. Marked as reviewed by dfuchs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24687#pullrequestreview-2791839194
Re: RFR: [java.io] Use @requires tag instead of exiting based on "os.name" property value
On Thu, 24 Apr 2025 20:46:42 GMT, Brian Burkhalter wrote: > Use the `@requires` tag instead of obtaining the operating system name from > the `os.name` property and then exiting if the test is not run on that > operating system. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24860#pullrequestreview-2792485973
Re: RFR: 8354897: Support Soft/Weak Reference in AOT cache
On Thu, 24 Apr 2025 21:09:10 GMT, Ioi Lam wrote: >> I'm not sure what you are proposing. But enqueue is used to ensure that >> inactive references are pruned from the data structure so that dumping only >> includes active references. This way, the JVM that loads these objects >> create these references as active and hence follow the usual life cycle that >> every other reference does. I want to avoid having a new "special" life >> cycles for dumped references. > > The map is still being used (and will be stored into the AOT cache). The key > no longer has a referent, so we need to remove the key from the map. Adding > the key to the stale queue and calling `removeStaleReferences()` will > accomplish this. `key.unused()` does not remove the key from the map. Thanks. I am fine with the rest of the core library changes. - PR Review Comment: https://git.openjdk.org/jdk/pull/24757#discussion_r2059237363
Re: RFR: 8349910: Implement HTTP/3 for the HTTP Client API [v2]
On Wed, 23 Apr 2025 07:54:39 GMT, Jaikiran Pai wrote:
>> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line
>> 366:
>>
>>> 364: }
>>> 365:
>>> 366: public String chooseServerAlias(String keyType,
>>
>> This method should have default (package-private) access modifier.
>
> Hello Artur, you are right. This is an overisght and we'll fix this as part
> of the next refresh of this PR.
This is now addressed in the latest update to this PR.
>> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line
>> 375:
>>
>>> 373: }
>>> 374:
>>> 375: public String chooseClientAlias(String[] keyTypes, Principal[]
>>> issuers,
>>
>> Same as above, the method shouldn't be public.
>
> Agreed. We will address this in the next refresh of the PR.
This is now addressed in the latest update to this PR.
>> test/jdk/java/net/httpclient/http3/H3QuicTLSConnection.java line 86:
>>
>>> 84: public static void main(String[] args) throws Exception {
>>> 85: // re-enable 3DES
>>> 86: Security.setProperty("jdk.tls.disabledAlgorithms", "");
>>
>> Use `SecurityUtils.removeFromDisabledAlgs` and only remove 3DES from this
>> property.
>
> Thank you for pointing to `SecurityUtils`. I updated this test to use that
> test library and the test continues to work as expected. We will include this
> change in the next refresh of the PR.
This too is now addressed in the latest update to this PR.
>> test/jdk/java/net/httpclient/http3/H3QuicTLSConnection.java line 95:
>>
>>> 93: //System.setProperty("javax.net.ssl.keyStorePassword",
>>> PASSWORD);
>>> 94: //System.setProperty("javax.net.ssl.trustStore", KEYSTORE);
>>> 95: //System.setProperty("javax.net.ssl.trustStorePassword",
>>> PASSWORD);
>>
>> Why we don't delete this?
>
> This looks like a leftover. I'll remove this as part of the next refresh.
Addressed in the latest update to this PR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24751#discussion_r2059464244
PR Review Comment: https://git.openjdk.org/jdk/pull/24751#discussion_r2059464328
PR Review Comment: https://git.openjdk.org/jdk/pull/24751#discussion_r2059464619
PR Review Comment: https://git.openjdk.org/jdk/pull/24751#discussion_r2059464758
Re: RFR: 8351623: VectorAPI: Refactor subword gather load and add SVE implementation
On Wed, 16 Apr 2025 08:58:34 GMT, Xiaohong Gong wrote: > ### Summary: > [JDK-8318650](http://java-service.client.nvidia.com/?q=8318650) added the > hotspot intrinsifying of subword gather load APIs for X86 platforms [1]. This > patch aims at implementing the equivalent functionality for AArch64 SVE > platform. In addition to the AArch64 backend support, this patch also > refactors the API implementation in Java side and the compiler mid-end part > to make the operations more efficient and maintainable across different > architectures. > > ### Background: > Vector gather load APIs load values from memory addresses calculated by > adding a base pointer to integer indices stored in an int array. SVE provides > native vector gather load instructions for byte/short types using an int > vector saving indices (see [2][3]). > > The number of loaded elements must match the index vector's element count. > Since int elements are 4/2 times larger than byte/short elements, and given > `MaxVectorSize` constraints, the operation may need to be splitted into > multiple parts. > > Using a 128-bit byte vector gather load as an example, there are four > scenarios with different `MaxVectorSize`: > > 1. `MaxVectorSize = 16, byte_vector_size = 16`: >- Can load 4 indices per vector register >- So can finish 4 bytes per gather-load operation >- Requires 4 times of gather-loads and final merge >Example: >``` >byte[] arr = [a, b, c, d, e, f, g, h, i, g, k, l, m, n, o, p, ...] >int[] idx = [3, 2, 4, 1, 5, 7, 5, 2, 0, 6, 7, 1, 15, 10, 11, 9] > >4 gather-load: >idx_v1 = [1 4 2 3]gather_v1 = [ becd] >idx_v2 = [2 5 7 5]gather_v2 = [ cfhf] >idx_v3 = [1 7 6 0]gather_v3 = [ bhga] >idx_v4 = [9 11 10 15] gather_v4 = [ jlkp] >merge: v = [jlkp bhga cfhf becd] >``` > > 2. `MaxVectorSize = 32, byte_vector_size = MaxVectorSize / 2`: >- Can load 8 indices per vector register >- So can finish 8 bytes per gather-load operation >- Requires 2 times of gather-loads and merge >Example: >``` >byte[] arr = [a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, ...] >int[] index = [3, 2, 4, 1, 5, 7, 5, 2, 0, 6, 7, 1, 15, 10, 11, 9] > >2 gather-load: >idx_v1 = [2 5 7 5 1 4 2 3] >idx_v2 = [9 11 10 15 1 7 6 0] >gather_v1 = [ cfhf becd] >gather_v2 = [ jlkp bhga] >merge: v = [ jlkp bhga cfhf becd] >``` > > 3. `MaxVectorSize = 64, byte_vector_size = MaxVectorSize / 4`: >- Can load 16 indices per vector register >- So can ... I‘d like to close this PR and split the change with two new PRs. Thanks for the review! - PR Comment: https://git.openjdk.org/jdk/pull/24679#issuecomment-2829245185
Withdrawn: 8351623: VectorAPI: Refactor subword gather load and add SVE implementation
On Wed, 16 Apr 2025 08:58:34 GMT, Xiaohong Gong wrote: > ### Summary: > [JDK-8318650](http://java-service.client.nvidia.com/?q=8318650) added the > hotspot intrinsifying of subword gather load APIs for X86 platforms [1]. This > patch aims at implementing the equivalent functionality for AArch64 SVE > platform. In addition to the AArch64 backend support, this patch also > refactors the API implementation in Java side and the compiler mid-end part > to make the operations more efficient and maintainable across different > architectures. > > ### Background: > Vector gather load APIs load values from memory addresses calculated by > adding a base pointer to integer indices stored in an int array. SVE provides > native vector gather load instructions for byte/short types using an int > vector saving indices (see [2][3]). > > The number of loaded elements must match the index vector's element count. > Since int elements are 4/2 times larger than byte/short elements, and given > `MaxVectorSize` constraints, the operation may need to be splitted into > multiple parts. > > Using a 128-bit byte vector gather load as an example, there are four > scenarios with different `MaxVectorSize`: > > 1. `MaxVectorSize = 16, byte_vector_size = 16`: >- Can load 4 indices per vector register >- So can finish 4 bytes per gather-load operation >- Requires 4 times of gather-loads and final merge >Example: >``` >byte[] arr = [a, b, c, d, e, f, g, h, i, g, k, l, m, n, o, p, ...] >int[] idx = [3, 2, 4, 1, 5, 7, 5, 2, 0, 6, 7, 1, 15, 10, 11, 9] > >4 gather-load: >idx_v1 = [1 4 2 3]gather_v1 = [ becd] >idx_v2 = [2 5 7 5]gather_v2 = [ cfhf] >idx_v3 = [1 7 6 0]gather_v3 = [ bhga] >idx_v4 = [9 11 10 15] gather_v4 = [ jlkp] >merge: v = [jlkp bhga cfhf becd] >``` > > 2. `MaxVectorSize = 32, byte_vector_size = MaxVectorSize / 2`: >- Can load 8 indices per vector register >- So can finish 8 bytes per gather-load operation >- Requires 2 times of gather-loads and merge >Example: >``` >byte[] arr = [a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, ...] >int[] index = [3, 2, 4, 1, 5, 7, 5, 2, 0, 6, 7, 1, 15, 10, 11, 9] > >2 gather-load: >idx_v1 = [2 5 7 5 1 4 2 3] >idx_v2 = [9 11 10 15 1 7 6 0] >gather_v1 = [ cfhf becd] >gather_v2 = [ jlkp bhga] >merge: v = [ jlkp bhga cfhf becd] >``` > > 3. `MaxVectorSize = 64, byte_vector_size = MaxVectorSize / 4`: >- Can load 16 indices per vector register >- So can ... This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/24679
Re: RFR: 8354897: Support Soft/Weak Reference in AOT cache [v3]
> This PR contains 2 parts > > - Upstream of Soft/Weak Reference support authored by @macarte from [the > Leyden > repo](https://github.com/openjdk/leyden/commit/4ca75d156519596e23abc8a312496b7c2f0e0ca5) > - New C++ class `AOTReferenceObjSupport` and new Java method > `ReferencedKeyMap::prepareForAOTCache()` developed by @iklam on the advice of > @fisk from the GC team. These control the lifecycles of reference objects > during the assembly phase to simplify the implementation. > > One problem we faced in this PR is the handling of Reference objects that are > waiting for clean up. Currently, the only cached Reference objects that > require clean up are the `WeakReferenceKey`s used by `ReferencedKeyMap` > (which is used by `MethodType::internTable`): > > - When the referent of a `WeakReferenceKey` K has been collected, the key > will be placed on `Universe::reference_pending_list()`. It's linked to other > pending references with the `Reference::discovered` field. At this point, K > is still stored in the `ReferencedKeyMap`. > - When heapShared.cpp discovered the `ReferencedKeyMap`, it will discover K, > and it may also discover other pending references that are not intended for > the AOT cache. As a result, we end up caching unnecessary objects. > > `ReferencedKeyMap::prepareForAOTCache()` avoids the above problem. It goes > over all entries in the table: > > - If an entry has not yet been collected, we make sure it will never be > collected. > - If an entry has been collected, we remove it from the table > > Therefore, by the time heapShared.cpp starts scanning the `ReferencedKeyMap`, > it will never see any keys that are on the pending list, so we will not see > unintended objects. > > This implementation is the very first step of Reference support in the AOT > cache, so we chose a simplified approach that makes no assumptions on when > the pending reference list is processed. This is sufficient for the current > set of references objects in the AOT cache. > > In the future, we may relax the implementation to allow for other use cases. Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: Fixed windows path issues - Changes: - all: https://git.openjdk.org/jdk/pull/24757/files - new: https://git.openjdk.org/jdk/pull/24757/files/2c31ca7c..643eec2a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24757&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24757&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/24757.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24757/head:pull/24757 PR: https://git.openjdk.org/jdk/pull/24757
Re: RFR: 8351565: Implement JEP 502: Stable Values (Preview) [v78]
On Thu, 24 Apr 2025 10:37:59 GMT, Per Minborg wrote: >> Implement JEP 502. >> >> The PR passes tier1-tier3 tests. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Make public constuctor private src/java.base/share/classes/java/lang/StableValue.java line 470: > 468: * @param other to return if the contents is not set > 469: */ > 470: T orElse(T other); Just to note that JEP 506 is proposing to change ScopedValue.orElse to disallow null. A ScopedValue can be bound to null (e.g. usage in Subject API) so using orElse(null) is problematic. A orNull may be added later if needed. As a StableValue can hold null then it's similar and might be surprising to developers to have the two APIs be different here. Something to look at again after JEP 502 is integrated, I'm not suggesting changing anything in this PR of course. - PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2059637511
Re: RFR: 8353786: Migrate Vector API math library support to FFM API [v14]
On Fri, 25 Apr 2025 00:06:28 GMT, Fei Yang wrote:
>> Vladimir Ivanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Improve comments
>
> src/hotspot/cpu/riscv/riscv.ad line 1947:
>
>> 1945: // Vector calling convention not yet implemented.
>> 1946: bool Matcher::supports_vector_calling_convention(void) {
>> 1947: return EnableVectorSupport;
>
> You might also want to remove the use of `UseVectorStubs` in
> `Matcher::vector_return_value` at L1951.
>
> assert(EnableVectorSupport && UseVectorStubs, "sanity");
Good catch. Fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24462#discussion_r2059577657
Re: RFR: 8354897: Support Soft/Weak Reference in AOT cache [v4]
> This PR contains 2 parts > > - Upstream of Soft/Weak Reference support authored by @macarte from [the > Leyden > repo](https://github.com/openjdk/leyden/commit/4ca75d156519596e23abc8a312496b7c2f0e0ca5) > - New C++ class `AOTReferenceObjSupport` and new Java method > `ReferencedKeyMap::prepareForAOTCache()` developed by @iklam on the advice of > @fisk from the GC team. These control the lifecycles of reference objects > during the assembly phase to simplify the implementation. > > One problem we faced in this PR is the handling of Reference objects that are > waiting for clean up. Currently, the only cached Reference objects that > require clean up are the `WeakReferenceKey`s used by `ReferencedKeyMap` > (which is used by `MethodType::internTable`): > > - When the referent of a `WeakReferenceKey` K has been collected, the key > will be placed on `Universe::reference_pending_list()`. It's linked to other > pending references with the `Reference::discovered` field. At this point, K > is still stored in the `ReferencedKeyMap`. > - When heapShared.cpp discovered the `ReferencedKeyMap`, it will discover K, > and it may also discover other pending references that are not intended for > the AOT cache. As a result, we end up caching unnecessary objects. > > `ReferencedKeyMap::prepareForAOTCache()` avoids the above problem. It goes > over all entries in the table: > > - If an entry has not yet been collected, we make sure it will never be > collected. > - If an entry has been collected, we remove it from the table > > Therefore, by the time heapShared.cpp starts scanning the `ReferencedKeyMap`, > it will never see any keys that are on the pending list, so we will not see > unintended objects. > > This implementation is the very first step of Reference support in the AOT > cache, so we chose a simplified approach that makes no assumptions on when > the pending reference list is processed. This is sufficient for the current > set of references objects in the AOT cache. > > In the future, we may relax the implementation to allow for other use cases. Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @fisk comment - Changes: - all: https://git.openjdk.org/jdk/pull/24757/files - new: https://git.openjdk.org/jdk/pull/24757/files/643eec2a..bba16eef Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24757&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24757&range=02-03 Stats: 4 lines in 1 file changed: 1 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/24757.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24757/head:pull/24757 PR: https://git.openjdk.org/jdk/pull/24757
Re: RFR: 8352003: Support --add-opens with -XX:+AOTClassLinking [v7]
On Fri, 25 Apr 2025 04:23:12 GMT, Calvin Cheung wrote: >> This RFE allows --add-opens to be specified for AOT cache creation. AOT >> cache can be used during production run with --add-opens option as long as >> the same set of options is used during assembly phase. >> >> Passed tiers 1 - 4 testing. > > Calvin Cheung has updated the pull request incrementally with one additional > commit since the last revision: > > remove call to addExtraExportsAndOpens() in ModuleBootstrap.java Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24695#pullrequestreview-2793081603
Re: RFR: 8354897: Support Soft/Weak Reference in AOT cache
On Thu, 24 Apr 2025 21:01:11 GMT, Erik Österlund wrote:
>> src/java.base/share/classes/jdk/internal/util/ReferencedKeyMap.java line 355:
>>
>>> 353: if (referent == null) {
>>> 354: // We don't need this key anymore. Add to stale queue
>>> 355: ((Reference)key).enqueue();
>>
>> Is enqueue necessary here? Afaik this map only uses the queue to be alert of
>> member reference being garbage collected and then remove stale elements.
>> Since at this stage this map is no longer used, maybe `key.unused()` is
>> sufficient?
>
> I'm not sure what you are proposing. But enqueue is used to ensure that
> inactive references are pruned from the data structure so that dumping only
> includes active references. This way, the JVM that loads these objects create
> these references as active and hence follow the usual life cycle that every
> other reference does. I want to avoid having a new "special" life cycles for
> dumped references.
The map is still being used (and will be stored into the AOT cache). The key no
longer has a referent, so we need to remove the key from the map. Adding the
key to the stale queue and calling `removeStaleReferences()` will accomplish
this. `key.unused()` does not remove the key from the map.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24757#discussion_r2059235560
Re: RFR: 8354897: Support Soft/Weak Reference in AOT cache
On Thu, 24 Apr 2025 19:52:25 GMT, Chen Liang wrote:
>> This PR contains 2 parts
>>
>> - Upstream of Soft/Weak Reference support authored by @macarte from [the
>> Leyden
>> repo](https://github.com/openjdk/leyden/commit/4ca75d156519596e23abc8a312496b7c2f0e0ca5)
>> - New C++ class `AOTReferenceObjSupport` and new Java method
>> `ReferencedKeyMap::prepareForAOTCache()` developed by @iklam on the advice
>> of @fisk from the GC team. These control the lifecycles of reference objects
>> during the assembly phase to simplify the implementation.
>>
>> One problem we faced in this PR is the handling of Reference objects that
>> are waiting for clean up. Currently, the only cached Reference objects that
>> require clean up are the `WeakReferenceKey`s used by `ReferencedKeyMap`
>> (which is used by `MethodType::internTable`):
>>
>> - When the referent of a `WeakReferenceKey` K has been collected, the key
>> will be placed on `Universe::reference_pending_list()`. It's linked to other
>> pending references with the `Reference::discovered` field. At this point, K
>> is still stored in the `ReferencedKeyMap`.
>> - When heapShared.cpp discovered the `ReferencedKeyMap`, it will discover K,
>> and it may also discover other pending references that are not intended for
>> the AOT cache. As a result, we end up caching unnecessary objects.
>>
>> `ReferencedKeyMap::prepareForAOTCache()` avoids the above problem. It goes
>> over all entries in the table:
>>
>> - If an entry has not yet been collected, we make sure it will never be
>> collected.
>> - If an entry has been collected, we remove it from the table
>>
>> Therefore, by the time heapShared.cpp starts scanning the
>> `ReferencedKeyMap`, it will never see any keys that are on the pending list,
>> so we will not see unintended objects.
>>
>> This implementation is the very first step of Reference support in the AOT
>> cache, so we chose a simplified approach that makes no assumptions on when
>> the pending reference list is processed. This is sufficient for the current
>> set of references objects in the AOT cache.
>>
>> In the future, we may relax the implementation to allow for other use cases.
>
> src/java.base/share/classes/jdk/internal/util/ReferencedKeyMap.java line 355:
>
>> 353: if (referent == null) {
>> 354: // We don't need this key anymore. Add to stale queue
>> 355: ((Reference)key).enqueue();
>
> Is enqueue necessary here? Afaik this map only uses the queue to be alert of
> member reference being garbage collected and then remove stale elements.
> Since at this stage this map is no longer used, maybe `key.unused()` is
> sufficient?
I'm not sure what you are proposing. But enqueue is used to ensure that
inactive references are pruned from the data structure so that dumping only
includes active references. This way, the JVM that loads these objects create
these references as active and hence follow the usual life cycle that every
other reference does. I want to avoid having a new "special" life cycles for
dumped references.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24757#discussion_r2059226257
Re: RFR: 8354897: Support Soft/Weak Reference in AOT cache
On Fri, 18 Apr 2025 18:31:05 GMT, Ioi Lam wrote:
> This PR contains 2 parts
>
> - Upstream of Soft/Weak Reference support authored by @macarte from [the
> Leyden
> repo](https://github.com/openjdk/leyden/commit/4ca75d156519596e23abc8a312496b7c2f0e0ca5)
> - New C++ class `AOTReferenceObjSupport` and new Java method
> `ReferencedKeyMap::prepareForAOTCache()` developed by @iklam on the advice of
> @fisk from the GC team. These control the lifecycles of reference objects
> during the assembly phase to simplify the implementation.
>
> One problem we faced in this PR is the handling of Reference objects that are
> waiting for clean up. Currently, the only cached Reference objects that
> require clean up are the `WeakReferenceKey`s used by `ReferencedKeyMap`
> (which is used by `MethodType::internTable`):
>
> - When the referent of a `WeakReferenceKey` K has been collected, the key
> will be placed on `Universe::reference_pending_list()`. It's linked to other
> pending references with the `Reference::discovered` field. At this point, K
> is still stored in the `ReferencedKeyMap`.
> - When heapShared.cpp discovered the `ReferencedKeyMap`, it will discover K,
> and it may also discover other pending references that are not intended for
> the AOT cache. As a result, we end up caching unnecessary objects.
>
> `ReferencedKeyMap::prepareForAOTCache()` avoids the above problem. It goes
> over all entries in the table:
>
> - If an entry has not yet been collected, we make sure it will never be
> collected.
> - If an entry has been collected, we remove it from the table
>
> Therefore, by the time heapShared.cpp starts scanning the `ReferencedKeyMap`,
> it will never see any keys that are on the pending list, so we will not see
> unintended objects.
>
> This implementation is the very first step of Reference support in the AOT
> cache, so we chose a simplified approach that makes no assumptions on when
> the pending reference list is processed. This is sufficient for the current
> set of references objects in the AOT cache.
>
> In the future, we may relax the implementation to allow for other use cases.
Changes requested by eosterlund (Reviewer).
src/hotspot/share/cds/heapShared.cpp line 1408:
> 1406: oop obj = RawAccess<>::oop_load(p);
> 1407: if (!CompressedOops::is_null(obj)) {
> 1408: int field_offset = pointer_delta_as_int((char*)p,
> cast_from_oop(_referencing_obj));
The RawAccess oop load above should probably be a
HeapAccess::oop_load_at. By using the unknown oop ref and
supplying the field offset, we allow GCs to infer the actual reference strength
in the backend when loading and apply appropriate barriers for non-strong
references, while also applying appropriate load barriers for conc GCs.
-
PR Review: https://git.openjdk.org/jdk/pull/24757#pullrequestreview-2792646427
PR Review Comment: https://git.openjdk.org/jdk/pull/24757#discussion_r2059322068
Re: RFR: 8351565: Implement JEP 502: Stable Values (Preview) [v78]
On Thu, 24 Apr 2025 10:37:59 GMT, Per Minborg wrote:
>> Implement JEP 502.
>>
>> The PR passes tier1-tier3 tests.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Make public constuctor private
src/java.base/share/classes/java/lang/StableValue.java line 389:
> 387: * Invocations of {@link #setOrThrow(Object)} form a total order of zero
> or more
> 388: * exceptional invocations followed by zero (if the contents were
> already set) or one
> 389: * successful invocation. Since stable functions and stable collections
> are built on top
How can an exceptional invocation of `setOrThrow` be followed by a successful
invocation?
src/java.base/share/classes/java/lang/StableValue.java line 447:
> 445: permits StableValueImpl {
> 446:
> 447: // Principal methods
Not important, not practical and hacky (exceptions as control flow), but
technically all methods could be implemented in terms of `orElseSet`, so they
could also be called "convenience methods":
trySet(c) {
success = false
orElseSet(() -> {success = true; return c})
return succes
}
orElse(o) {
try { return orElseSet(() -> throw) }
catch { return o }
}
orElseThrow() {
orElseSet(() -> throw)
}
isSet() {
try { orElseSet(() -> throw) }
catch { return false }
return true
}
setOrThrow(c) {
success = false
orElseSet(() -> {success = true; return c})
if (!success) throw
}
I guess these two comments (`// Principal methods` and `// Convenience
methods`) could also just be omitted.
src/java.base/share/classes/java/lang/StableValue.java line 523:
> 521: * @throws IllegalStateException if the contents was already set
> 522: */
> 523: void setOrThrow(T contents);
Should probably also mention that `IllegalStateException` can be caused by
recursive initialization (see `trySet`).
src/java.base/share/classes/java/lang/StableValue.java line 577:
> 575: * already under computation will block until a value is computed or
> an exception is
> 576: * thrown by the computing thread. The computing threads will then
> observe the newly
> 577: * computed value (if any) and will then never execute.
This sentence seems off. I think it should say "competing threads" instead of
"computing threads". And what will they never execute?
src/java.base/share/classes/java/lang/StableValue.java line 688:
> 686: *
> 687: * Any direct {@link List#subList(int, int) subList} or {@link
> List#reversed()} views
> 688: * of the returned list are also stable.
What about non-direct views? I think they could be made stable (i.e. have a
non-evaluating `toString`) with few changes:
Override `subList` in
`ImmutableCollections.StableList.StableReverseOrderListView` similar to
`ReverseOrderListView.subList`:
@Override
public List subList(int fromIndex, int toIndex) {
int size = base.size();
subListRangeCheck(fromIndex, toIndex, size);
return new StableReverseOrderListView<>(base.subList(size - toIndex, size -
fromIndex));
}
Override `reversed` in `ImmutableCollections.SubList`:
@Override
public List reversed() {
if (root instanceof StableList) {
return new StableList.StableReverseOrderListView<>(this);
} else {
return super.reversed();
}
}
Change `ImmutableCollections.StableList.StableReverseOrderListView.toString`
(instead of copying and reversing, `StableUtil.renderElements` could also be
modified to have a configurable iteration order):
@Override
public String toString() {
final StableList stable;
final int from, to;
if (base instanceof SubList sub) {
stable = (StableList)sub.root;
from = sub.offset;
to = sub.offset + sub.size();
} else {
stable = (StableList)base;
from = 0;
to = stable.delegates.length;
}
final StableValueImpl[] reversed = ArraysSupport.reverse(
Arrays.copyOfRange(stable.delegates, from, to));
return StableUtil.renderElements(this, "StableList", reversed);
}
That should make sure that however many times and in whatever combination
`reversed` and `subList` are called on a stable list, it will always be an
instance of `ImmutableCollections.StableList`,
`ImmutableCollections.StableList.StableReverseOrderListView` or
`ImmutableCollections.SubList`. They all properly override `toString`. The view
nesting depth is also bounded.
Visualized as a table (`SL` = `ImmutableCollections.StableList`, `Rev` =
`ImmutableCollections.StableList.StableReverseOrderListView`, `Sub` =
`ImmutableCollections.SubList`):
| view nesting | calling `reversed` on that view returns | calling `subList`
on that view returns |
| - | --- |
-- |
| `SL` | `Rev(SL)` | `Sub(SL)`
|
| `Rev(SL)` | `SL`
Re: RFR: 8351565: Implement JEP 502: Stable Values (Preview) [v56]
On Tue, 22 Apr 2025 11:22:52 GMT, Per Minborg wrote:
>> src/java.base/share/classes/java/util/ImmutableCollections.java line 1592:
>>
>>> 1590: final K k = inner.getKey();
>>> 1591: return new NullableKeyValueHolder<>(k,
>>> inner.getValue().orElseSet(new Supplier() {
>>> 1592: @Override public V get() { return
>>> mapper.apply(k); }}));
>>
>> I suppose you could potentially make this more lazy by returning an `Entry`
>> implementation that only evaluates the mapper when calling `Entry::getValue`.
>
> Yepp. Interesting idea.
This lazy `Entry` implementation should then also have a `toString` that
doesn't evaluate the contents. It should also be used in `forEachRemaining`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r2059377365
Re: RFR: 8353786: Migrate Vector API math library support to FFM API [v15]
> Migrate Vector API math library (SVML and SLEEF) linkage from native code (in > JVM) to Java FFM API. > > Since FFM API doesn't support vector calling conventions yet, migration > affects only symbol lookup for now. But it still enables significant > simplifications on JVM side. > > The patch consists of the following parts: > * on-demand symbol lookup in Java code replaces eager lookup from native > code during JVM startup; > * 2 new VM intrinsics for vector calls (support unary and binary shapes) > (code separated from unary/binary vector operations); > * new internal interface to query supported CPU ISA extensions > (`jdk.incubator.vector.CPUFeatures`) used for CPU dispatching. > > `java.lang.foreign` API is used to perform symbol lookup in vector math > library, then the address is cached and fed into corresponding JVM intrinsic, > so C2 can turn it into a direct vector call in generated code. > > Once `java.lang.foreign` supports vectors & vector calling conventions, VM > intrinsics can go away. > > Performance is on par with original implementation (tested with > microbenchmarks on linux-x64 and macosx-aarch64). > > Testing: hs-tier1 - hs-tier6, microbenchmarks (on linux-x64 and > macosx-aarch64) > > Thanks! Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision: Remove UseVectorStubs usage in riscv.ad - Changes: - all: https://git.openjdk.org/jdk/pull/24462/files - new: https://git.openjdk.org/jdk/pull/24462/files/541c4d7f..f4373e41 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24462&range=14 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24462&range=13-14 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/24462.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24462/head:pull/24462 PR: https://git.openjdk.org/jdk/pull/24462
Re: RFR: 8355524: Only every second line in upgradeable files is being used
On Thu, 24 Apr 2025 17:31:50 GMT, Severin Gehwolf wrote: > Please review this fix to > [JDK-8353185](https://bugs.openjdk.org/browse/JDK-8353185). The reading logic > for the config file would erroneously use `scanner.nextLine()` when the > current line to be added is in `line`. `line` is not being added, but > `scanner.nextLine()` unintentionally skipping lines. The config file has a > change too. It moves the `tzdb.dat` line last so that the existing test > verifies that the `cacerts` one isn't being skipped. This slipped through > because `tzdb.dat` updates aren't automatically tested. > > Testing: > - [ ] GHA > - [x] The test from > [JDK-8353185](https://bugs.openjdk.org/browse/JDK-8353185), > `UpgradeableFileCacertsTest.java` fails with the config file change only > (without the product fix) and passes with the one-liner. Also some manual > testing when both files have been upgraded. > > Thoughts? Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24855#pullrequestreview-2793024177
Re: RFR: 8352003: Support --add-opens with -XX:+AOTClassLinking [v7]
> This RFE allows --add-opens to be specified for AOT cache creation. AOT cache > can be used during production run with --add-opens option as long as the same > set of options is used during assembly phase. > > Passed tiers 1 - 4 testing. Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision: remove call to addExtraExportsAndOpens() in ModuleBootstrap.java - Changes: - all: https://git.openjdk.org/jdk/pull/24695/files - new: https://git.openjdk.org/jdk/pull/24695/files/0f28e07b..4b4d4f50 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24695&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24695&range=05-06 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/24695.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24695/head:pull/24695 PR: https://git.openjdk.org/jdk/pull/24695
Re: RFR: 8354897: Support Soft/Weak Reference in AOT cache [v2]
> This PR contains 2 parts > > - Upstream of Soft/Weak Reference support authored by @macarte from [the > Leyden > repo](https://github.com/openjdk/leyden/commit/4ca75d156519596e23abc8a312496b7c2f0e0ca5) > - New C++ class `AOTReferenceObjSupport` and new Java method > `ReferencedKeyMap::prepareForAOTCache()` developed by @iklam on the advice of > @fisk from the GC team. These control the lifecycles of reference objects > during the assembly phase to simplify the implementation. > > One problem we faced in this PR is the handling of Reference objects that are > waiting for clean up. Currently, the only cached Reference objects that > require clean up are the `WeakReferenceKey`s used by `ReferencedKeyMap` > (which is used by `MethodType::internTable`): > > - When the referent of a `WeakReferenceKey` K has been collected, the key > will be placed on `Universe::reference_pending_list()`. It's linked to other > pending references with the `Reference::discovered` field. At this point, K > is still stored in the `ReferencedKeyMap`. > - When heapShared.cpp discovered the `ReferencedKeyMap`, it will discover K, > and it may also discover other pending references that are not intended for > the AOT cache. As a result, we end up caching unnecessary objects. > > `ReferencedKeyMap::prepareForAOTCache()` avoids the above problem. It goes > over all entries in the table: > > - If an entry has not yet been collected, we make sure it will never be > collected. > - If an entry has been collected, we remove it from the table > > Therefore, by the time heapShared.cpp starts scanning the `ReferencedKeyMap`, > it will never see any keys that are on the pending list, so we will not see > unintended objects. > > This implementation is the very first step of Reference support in the AOT > cache, so we chose a simplified approach that makes no assumptions on when > the pending reference list is processed. This is sufficient for the current > set of references objects in the AOT cache. > > In the future, we may relax the implementation to allow for other use cases. Ioi Lam has updated the pull request incrementally with three additional commits since the last revision: - Refactored code - Fixed error when running "make test JTREG=AOT_JDK=true TEST=open/test/hotspot/jtreg/runtime/invokedynamic" - Work around hotspot/jtreg/sources/TestNoNULL.java - Changes: - all: https://git.openjdk.org/jdk/pull/24757/files - new: https://git.openjdk.org/jdk/pull/24757/files/6678482b..2c31ca7c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24757&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24757&range=00-01 Stats: 56 lines in 4 files changed: 35 ins; 12 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/24757.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24757/head:pull/24757 PR: https://git.openjdk.org/jdk/pull/24757
Re: RFR: 8352003: Support --add-opens with -XX:+AOTClassLinking [v7]
On Fri, 25 Apr 2025 04:23:12 GMT, Calvin Cheung wrote: >> This RFE allows --add-opens to be specified for AOT cache creation. AOT >> cache can be used during production run with --add-opens option as long as >> the same set of options is used during assembly phase. >> >> Passed tiers 1 - 4 testing. > > Calvin Cheung has updated the pull request incrementally with one additional > commit since the last revision: > > remove call to addExtraExportsAndOpens() in ModuleBootstrap.java Marked as reviewed by iklam (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24695#pullrequestreview-2792985774
Re: RFR: 8077587: BigInteger Roots [v19]
On Mon, 21 Apr 2025 11:47:30 GMT, fabioromano1 wrote: >>> [Here is a proof of convergence of the recurrence >>> used.](https://github.com/user-attachments/files/19785045/nth_root_newton_proof_integers.pdf) >> >> That's very nice. It would be even nicer if this was a permalink into the >> JDK repo, and a reference in the source code. > >> That's very nice. It would be even nicer if this was a permalink into the >> JDK repo, and a reference in the source code. > > @theRealAph Ok. It would be useful to have a link to an explanation on how > this can be done, if there is one. Thanks. @fabioromano1 I just had a cursory glance at this PR. AFAIU, there are two main contributions here: - Performance enhancements in `pow()`, which is of general interest and does not require submitting a [CSR](https://wiki.openjdk.org/display/csr/Main). - Introduction of a new public API point for the _n_-th root, which would require a CSR. It's important to understand that if we add public API points, there must be some evidence and consensus about their general usefulness and demand for them. Every addition is a commitment for this community in terms of code maintenance, reviews, testing, documentation, so they should be evaluated with this perspective in mind. In this case, I feel that the proposed _n_-th root might not be among the top priority API points to add to `BigInteger`. Perhaps the overall plan is to use this method to implement a _n_-th root in `BigDecimal` in some followup PR, but this is not stated here. Anyway, a [preliminary discussion](https://openjdk.org/guide/#socialize-your-change) about the proposal should take place on the mailing list, _before_ submitting the PR and invest too much work on the code. To make progress here, I suggest to split this PR in two, if technically possible: - One for the enhancements in `pow`, with JMH results before/after. - Another PR for the proposed _n_-th root. Thanks - PR Comment: https://git.openjdk.org/jdk/pull/24690#issuecomment-2828501417
Re: RFR: 8347471: Provide valid flags and mask in AccessFlag.Location [v6]
> Some AccessFlag parsing methods throw IAE because a flag mask is not valid in > a location. However, there is no easy way to check what flag mask bits or > what flags are valid for a location. We need such APIs to check, specific to > each class file format version. > > Also in the investigation, it's noted that `ACC_SYNTHETIC` is incorrectly > represented - it is available since release 5.0 instead of release 7. This > bug is fixed together for implementation simplicity. > > The new methods are all in `AccessFlag.Location`: > - `Set flags()` > - `int flagsMask()` > - `Set flags(ClassFileFormatVersion)` > - `int flagsMask(ClassFileFormatVersion)` > > Also there is some simplification to `AccessFlag` itself to remove the > anonymous classes, which should be more startup-friendly. > > Testing: Tier 1-3 Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into feature/af-location-accessors - Preview notes - Further enhance the impl of access flags, make cffv aware to flags easier - Merge branch 'master' of https://github.com/openjdk/jdk into feature/af-location-accessors - Merge branch 'master' of https://github.com/openjdk/jdk into feature/af-location-accessors - More links - ClassLoading order, Typos in NPE test - 8347471: Provide valid flags and mask in AccessFlag.Location - Changes: - all: https://git.openjdk.org/jdk/pull/23095/files - new: https://git.openjdk.org/jdk/pull/23095/files/7e6d58a1..1019812f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23095&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23095&range=04-05 Stats: 35133 lines in 1000 files changed: 27235 ins; 5788 del; 2110 mod Patch: https://git.openjdk.org/jdk/pull/23095.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23095/head:pull/23095 PR: https://git.openjdk.org/jdk/pull/23095
Re: RFR: 8349910: Implement HTTP/3 for the HTTP Client API [v2]
> Hi, > > Please find here a PR for the implementation of JEP [JDK-8291976: HTTP/3 for > the HTTP Client API](https://bugs.openjdk.org/browse/JDK-8291976). > > The CSR can be viewed at [JDK-8350588: Implement HTTP/3 for the HTTP Client > API](https://bugs.openjdk.org/browse/JDK-8350588) > > This JEP proposes to enhance the HttpClient implementation to support HTTP/3. > It adds a non-exposed / non-exported internal implementation of the QUIC > protocol based on DatagramChannel and the SunJSSE SSLContext provider. Daniel Fuchs has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 409 commits: - merge latest changes from master branch - http3: add missing separator to Http3DiscoveryMode.ALT_SVC API documentation - http3: improve documentation for Http3DiscoveryMode.ALT_SVC - http3: Use AlgorithmConstraints and OCSP responses when validating server certificate during QUIC TLS handshake - http3: Artur's review - use SecurityUtils.removeFromDisabledTlsAlgs() in test - http3: minor improvement to log message - http3: Artur's review - remove commented out code from test - http3: Artur's review - make methods package private - http3: qpack - allow 0 capacity when max capacity is 0 - Remove flow control from stream limit comments - ... and 399 more: https://git.openjdk.org/jdk/compare/1ec64811...4da61bbe - Changes: https://git.openjdk.org/jdk/pull/24751/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24751&range=01 Stats: 102765 lines in 470 files changed: 100142 ins; 1119 del; 1504 mod Patch: https://git.openjdk.org/jdk/pull/24751.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24751/head:pull/24751 PR: https://git.openjdk.org/jdk/pull/24751
Re: RFR: 8354424: java/util/logging/LoggingDeadlock5.java fails intermittently in tier6 [v2]
On Tue, 22 Apr 2025 13:53:14 GMT, Daniel Fuchs wrote: >> test/jdk/java/util/logging/LoggingDeadlock5.java line 127: >> >>> 125: // in higher tiers, so it's necessary to be a bit pessimistic >>> here. >>> 126: private final static Duration JOIN_WAIT = >>> 127: Duration.ofMillis(Utils.adjustTimeout(2000)); >> >> Just checking, adjustTimeout does scaling based on the timeout factor given >> to jtreg? >> >> What happens to the expected test running time to the test in lower tiers >> without as high loads? > > Hi Joe, yes `adjustTimeout` will scale based on the jtreg timeout factor. I > believe the behaviour is to multiply whatever hardcoded timeout is passed by > the timeout factor. > On lower tiers, in our CI, I beleive it means the test will have to wait for > 8s before it can assert that a deadlock is detected. That should be way off > the default jtreg timeout - which IIRC is 480s on lower tiers. Note, the timeout factor also adjusts the jtreg timeout for the entire test. The adjustTimeout() method allows internal test timeouts to scale along with the jtreg timeout. - PR Review Comment: https://git.openjdk.org/jdk/pull/24687#discussion_r2058825862
RFR: 8354897: Support Soft/Weak Reference in AOT cache
This PR contains 2 parts - Upstream of Soft/Weak Reference support authored by @macarte from [the Leyden repo](https://github.com/openjdk/leyden/commit/4ca75d156519596e23abc8a312496b7c2f0e0ca5) - New C++ class `AOTReferenceObjSupport` and new Java method `ReferencedKeyMap::prepareForAOTCache()` developed by @iklam on the advice of @fisk from the GC team. These control the lifecycles of reference objects during the assembly phase to simplify the implementation. One problem we faced in this PR is the handling of Reference objects that are waiting for clean up. Currently, the only cached Reference objects that require clean up are the `WeakReferenceKey`s used by `ReferencedKeyMap` (which is used by `MethodType::internTable`): - When the referent of a `WeakReferenceKey` K has been collected, the key will be placed on `Universe::reference_pending_list()`. It's linked to other pending references with the `Reference::discovered` field. At this point, K is still stored in the `ReferencedKeyMap`. - When heapShared.cpp discovered the `ReferencedKeyMap`, it will discover K, and it may also discover other pending references that are not intended for the AOT cache. As a result, we end up caching unnecessary objects. `ReferencedKeyMap::prepareForAOTCache()` avoids the above problem. It goes over all entries in the table: - If an entry has not yet been collected, we make sure it will never be collected. - If an entry has been collected, we remove it from the table Therefore, by the time heapShared.cpp starts scanning the `ReferencedKeyMap`, it will never see any keys that are on the pending list, so we will not see unintended objects. This implementation is the very first step of Reference support in the AOT cache, so we chose a simplified approach that makes no assumptions on when the pending reference list is processed. This is sufficient for the current set of references objects in the AOT cache. In the future, we may relax the implementation to allow for other use cases. - Commit messages: - Merge branch 'master' into 8354897-soft-weak-references-in-aot-cache - cleaned up test cases - Added missing files - Avoid the need to Universe::reference_pending_list() and remove assumptions on GC behavior. Suggested by @fisk - Added code to process Universe::reference_pending_list() - 8354897: Support Soft/Weak Reference in AOT cache (imported from Leyden repo) Changes: https://git.openjdk.org/jdk/pull/24757/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24757&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8354897 Stats: 784 lines in 21 files changed: 627 ins; 102 del; 55 mod Patch: https://git.openjdk.org/jdk/pull/24757.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24757/head:pull/24757 PR: https://git.openjdk.org/jdk/pull/24757
Re: RFR: 8354897: Support Soft/Weak Reference in AOT cache
On Fri, 18 Apr 2025 18:31:05 GMT, Ioi Lam wrote:
> This PR contains 2 parts
>
> - Upstream of Soft/Weak Reference support authored by @macarte from [the
> Leyden
> repo](https://github.com/openjdk/leyden/commit/4ca75d156519596e23abc8a312496b7c2f0e0ca5)
> - New C++ class `AOTReferenceObjSupport` and new Java method
> `ReferencedKeyMap::prepareForAOTCache()` developed by @iklam on the advice of
> @fisk from the GC team. These control the lifecycles of reference objects
> during the assembly phase to simplify the implementation.
>
> One problem we faced in this PR is the handling of Reference objects that are
> waiting for clean up. Currently, the only cached Reference objects that
> require clean up are the `WeakReferenceKey`s used by `ReferencedKeyMap`
> (which is used by `MethodType::internTable`):
>
> - When the referent of a `WeakReferenceKey` K has been collected, the key
> will be placed on `Universe::reference_pending_list()`. It's linked to other
> pending references with the `Reference::discovered` field. At this point, K
> is still stored in the `ReferencedKeyMap`.
> - When heapShared.cpp discovered the `ReferencedKeyMap`, it will discover K,
> and it may also discover other pending references that are not intended for
> the AOT cache. As a result, we end up caching unnecessary objects.
>
> `ReferencedKeyMap::prepareForAOTCache()` avoids the above problem. It goes
> over all entries in the table:
>
> - If an entry has not yet been collected, we make sure it will never be
> collected.
> - If an entry has been collected, we remove it from the table
>
> Therefore, by the time heapShared.cpp starts scanning the `ReferencedKeyMap`,
> it will never see any keys that are on the pending list, so we will not see
> unintended objects.
>
> This implementation is the very first step of Reference support in the AOT
> cache, so we chose a simplified approach that makes no assumptions on when
> the pending reference list is processed. This is sufficient for the current
> set of references objects in the AOT cache.
>
> In the future, we may relax the implementation to allow for other use cases.
src/java.base/share/classes/jdk/internal/util/ReferencedKeyMap.java line 355:
> 353: if (referent == null) {
> 354: // We don't need this key anymore. Add to stale queue
> 355: ((Reference)key).enqueue();
Is enqueue necessary here? Afaik this map only uses the queue to be alert of
member reference being garbage collected and then remove stale elements. Since
at this stage this map is no longer used, maybe `key.unused()` is sufficient?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24757#discussion_r2059128012
Re: RFR: 8355301: Simplify Throwable::printStackTrace by replacing inner class with static method [v4]
On Wed, 23 Apr 2025 09:05:07 GMT, Alan Bateman wrote: >>> Ugh, hopefully this will be replaced in the next iteration. >> >> What should we replace it with? Do you have any suggestions? > >> What should we replace it with? Do you have any suggestions? > > The wrapper classes were needed when there were was a mix of synchronized and > j.u.concurrent locks in use. With JEP 491 integrated it meant that the > java.io classes could be changed back to use synchronized. Yes, we could do > some cleanup in Throwable too. Changing PrintStreamOrWriter to be an > interface should be fine but the rest of these changes in this PR doesn't > seem necessary. As others have already asked, I think it would be useful to > understand what issue you are running into and why you want to change this > code. @AlanBateman I have modified it to use interface + record. Is this what you want? - PR Review Comment: https://git.openjdk.org/jdk/pull/24795#discussion_r2059382954
Re: RFR: 8355177: Speed up StringBuilder::append(char[]) via UTF16::compress & Unsafe::copyMemory [v3]
> In BufferedReader.readLine and other similar scenarios, we need to use > StringBuilder.append(char[]) to build the string. > > For these scenarios, we can use the intrinsic method StringUTF16.compress and > Unsafe.copyMemory instead of the character copy of the char-by-char loop to > improve the speed. Shaojin Wen has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits: - Merge remote-tracking branch 'upstream/master' into optim_sb_append_chars_202504 # Conflicts: #src/java.base/share/classes/java/lang/StringUTF16.java - putCharsUnchecked - copyright - Using StringUTF16.compress to speed up LATIN1 StringBuilder append(char[]) - Using Unsafe.copyMemory to speed up UTF16 StringBuilder append(char[]) - add append(char[]) benchmark - Changes: https://git.openjdk.org/jdk/pull/24773/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24773&range=02 Stats: 47 lines in 3 files changed: 40 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/24773.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24773/head:pull/24773 PR: https://git.openjdk.org/jdk/pull/24773
Re: RFR: 8355301: Simplify Throwable::printStackTrace by replacing inner class with static method [v4]
On Thu, 24 Apr 2025 23:52:09 GMT, Shaojin Wen wrote: >>> What should we replace it with? Do you have any suggestions? >> >> The wrapper classes were needed when there were was a mix of synchronized >> and j.u.concurrent locks in use. With JEP 491 integrated it meant that the >> java.io classes could be changed back to use synchronized. Yes, we could do >> some cleanup in Throwable too. Changing PrintStreamOrWriter to be an >> interface should be fine but the rest of these changes in this PR doesn't >> seem necessary. As others have already asked, I think it would be useful to >> understand what issue you are running into and why you want to change this >> code. > > @AlanBateman I have modified it to use interface + record. Is this what you > want? Again, what goal is this PR trying to accomplish? - PR Review Comment: https://git.openjdk.org/jdk/pull/24795#discussion_r2059386047
Re: RFR: 8355301: Simplify Throwable::printStackTrace by replacing inner class with static method [v4]
On Thu, 24 Apr 2025 23:57:56 GMT, Joe Darcy wrote: >> @AlanBateman I have modified it to use interface + record. Is this what you >> want? > > Again, what goal is this PR trying to accomplish? The goal of this PR is to simplify the code by using new language features. - PR Review Comment: https://git.openjdk.org/jdk/pull/24795#discussion_r2059407222
Re: RFR: 8353786: Migrate Vector API math library support to FFM API [v14]
On Thu, 24 Apr 2025 23:29:28 GMT, Vladimir Ivanov wrote:
>> Migrate Vector API math library (SVML and SLEEF) linkage from native code
>> (in JVM) to Java FFM API.
>>
>> Since FFM API doesn't support vector calling conventions yet, migration
>> affects only symbol lookup for now. But it still enables significant
>> simplifications on JVM side.
>>
>> The patch consists of the following parts:
>> * on-demand symbol lookup in Java code replaces eager lookup from native
>> code during JVM startup;
>> * 2 new VM intrinsics for vector calls (support unary and binary shapes)
>> (code separated from unary/binary vector operations);
>> * new internal interface to query supported CPU ISA extensions
>> (`jdk.incubator.vector.CPUFeatures`) used for CPU dispatching.
>>
>> `java.lang.foreign` API is used to perform symbol lookup in vector math
>> library, then the address is cached and fed into corresponding JVM
>> intrinsic, so C2 can turn it into a direct vector call in generated code.
>>
>> Once `java.lang.foreign` supports vectors & vector calling conventions, VM
>> intrinsics can go away.
>>
>> Performance is on par with original implementation (tested with
>> microbenchmarks on linux-x64 and macosx-aarch64).
>>
>> Testing: hs-tier1 - hs-tier6, microbenchmarks (on linux-x64 and
>> macosx-aarch64)
>>
>> Thanks!
>
> Vladimir Ivanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Improve comments
src/hotspot/cpu/riscv/riscv.ad line 1947:
> 1945: // Vector calling convention not yet implemented.
> 1946: bool Matcher::supports_vector_calling_convention(void) {
> 1947: return EnableVectorSupport;
You might want to remove the use of `UseVectorStubs` in
`Matcher::vector_return_value` at L1951.
assert(EnableVectorSupport && UseVectorStubs, "sanity");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24462#discussion_r2059390324
Re: RFR: 8355301: Simplify Throwable::printStackTrace by replacing inner class with static method [v4]
On Fri, 25 Apr 2025 00:34:39 GMT, Shaojin Wen wrote: >> Again, what goal is this PR trying to accomplish? > > The goal of this PR is to simplify the code by using new language features. To what end? - "I was reading Throwable and noticed this possible refactoring." - "I've run an analysis of the JDK code base and this is the first of N patches to move to using private records." - "With this change, metric X is Y% better." Something else? - PR Review Comment: https://git.openjdk.org/jdk/pull/24795#discussion_r2059415698
Re: RFR: 8355301: Simplify Throwable::printStackTrace by replacing inner class with static method [v4]
On Fri, 25 Apr 2025 00:52:01 GMT, Joe Darcy wrote: >> The goal of this PR is to simplify the code by using new language features. > > To what end? > > - "I was reading Throwable and noticed this possible refactoring." > - "I've run an analysis of the JDK code base and this is the first of N > patches to move to using private records." > - "With this change, metric X is Y% better." > > Something else? I think the initial version tried to use record and interfaces to simplify declarations; then I suggested that we have 3 classes, but we can just use an on-site instanceof check to reduce number of classes (which will be individual files in images, also class loading penalty for stack trace printing). However, Alan has this comment, which I don't understand quite well either - seems Alan was looking at an older revision as the interface was removed at the time of his comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/24795#discussion_r2059417029
Re: RFR: 8347408: Create an internal method handle adapter for system calls with errno [v3]
On Thu, 13 Mar 2025 15:21:33 GMT, Per Minborg wrote:
>> As we advance, converting older JDK code to use the relatively new FFM API
>> requires system calls that can provide `errno` and the likes to explicitly
>> allocate a `MemorySegment` to capture potential error states. This can lead
>> to negative performance implications if not designed carefully and also
>> introduces unnecessary code complexity.
>>
>> Hence, this PR proposes adding a JDK internal method handle adapter that can
>> be used to handle system calls with `errno`, `GetLastError`, and
>> `WSAGetLastError`.
>>
>> It relies on an efficient carrier-thread-local cache of memory regions to
>> allide allocations.
>>
>>
>> Here are some benchmarks that ran on a platform thread and virtual threads
>> respectively (M1 Mac):
>>
>>
>> Benchmark Mode Cnt Score
>> Error Units
>> CaptureStateUtilBench.OfVirtual.adaptedSysCallFail avgt 30 24.330
>> ? 0.820 ns/op
>> CaptureStateUtilBench.OfVirtual.adaptedSysCallSuccess avgt 30 8.257
>> ? 0.117 ns/op
>> CaptureStateUtilBench.OfVirtual.explicitAllocationFail avgt 30 41.415
>> ? 1.013 ns/op
>> CaptureStateUtilBench.OfVirtual.explicitAllocationSuccess avgt 30 21.720
>> ? 0.463 ns/op
>> CaptureStateUtilBench.OfVirtual.tlAllocationFail avgt 30 23.636
>> ? 0.182 ns/op
>> CaptureStateUtilBench.OfVirtual.tlAllocationSuccessavgt 30 8.234
>> ? 0.156 ns/op
>> CaptureStateUtilBench.adaptedSysCallFail avgt 30 23.918
>> ? 0.487 ns/op
>> CaptureStateUtilBench.adaptedSysCallSuccessavgt 30 4.946
>> ? 0.089 ns/op
>> CaptureStateUtilBench.explicitAllocationFail avgt 30 42.280
>> ? 1.128 ns/op
>> CaptureStateUtilBench.explicitAllocationSuccessavgt 30 21.809
>> ? 0.413 ns/op
>> CaptureStateUtilBench.tlAllocationFail avgt 30 24.422
>> ? 0.673 ns/op
>> CaptureStateUtilBench.tlAllocationSuccess avgt 30 5.182
>> ? 0.152 ns/op
>>
>>
>> Adapted system call:
>>
>> return (int) ADAPTED_HANDLE.invoke(0, 0); // Uses a MH-internal pool
>> ```
>> Explicit allocation:
>>
>> try (var arena = Arena.ofConfined()) {
>> return (int) HANDLE.invoke(arena.allocate(4), 0, 0);
>> }
>> ```
>> Thread Local allocation:
>>
>> try (var arena = POOLS.take()) {
>> return (int) HANDLE.invoke(arena.allocate(4), 0, 0); // Uses a
>> manually specified pool
>> }
>> ```
>> The adapted system call exhibits a ~4x performance improvement ove...
>
> Per Minborg has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 34 commits:
>
> - Merge master
> - Add test for woven allocation
> - Merge branch 'master' into errno-util3
> - Use lazy initialization of method handles
> - Clean up visibility
> - Merge branch 'master' into errno-util3
> - Add @ForceInline annotations
> - Add out of order test for VTs
> - Allow memory reuse for several arenas
> - Remove file
> - ... and 24 more: https://git.openjdk.org/jdk/compare/4e51a8c9...6d782a84
src/java.base/share/classes/jdk/internal/foreign/CaptureStateUtil.java line 65:
> 63: // returnType in {int.class | long.class}
> 64: // stateName can be anything non-null but should be in
> {"GetLastError" | "WSAGetLastError"} | "errno")}
> 65: private record BasicKey(Class returnType, String stateName) {
Please at least add an `hashCode` - this avoids a hefty overhead from code
generation in ObjectMethods.bootstrap, unavoidable whenever we put entries into
a hash map.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23765#discussion_r2056408940
RFR: 8355444: [java.io] Use @requires tag instead of exiting based on os.name property value
Use the `@requires` tag instead of obtaining the operating system name from the `os.name` property and then exiting if the test is not run on that operating system. - Commit messages: - 8355444: [java.io] Use @requires tag instead of exiting based on os.name property value Changes: https://git.openjdk.org/jdk/pull/24860/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24860&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8355444 Stats: 36 lines in 6 files changed: 11 ins; 19 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/24860.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24860/head:pull/24860 PR: https://git.openjdk.org/jdk/pull/24860
Re: RFR: 8077587: BigInteger Roots [v19]
On Thu, 24 Apr 2025 18:17:38 GMT, Raffaello Giulietti wrote: > * Performance enhancements in `pow()`, which is of general interest and does > not require submitting a [CSR](https://wiki.openjdk.org/display/csr/Main). @rgiulietti Yes, but here, the primary enhancement in `pow()` is not concerned most on execution time, but rather in memory optimization, because the PR implementation does the "shift of the exponent" squaring the result rather than the base, so the base is not squared like in the current implementation, and this permits to save about half of the memory. > To make progress here, I suggest to split this PR in two, if technically > possible: > > * One for the enhancements in `pow`, with JMH results before/after. > * Another PR for the proposed _n_-th root. I'm not very sure if it is technically possible, because both `pow()` and `nthRoot()` requires the computation of a power of a long, so that code has to be shared by both methods, and so a supposed PR for `nthRoot()` would require that method. - PR Comment: https://git.openjdk.org/jdk/pull/24690#issuecomment-2828760458
Re: RFR: 8350703: Add standard system property stdin.encoding [v3]
On Wed, 23 Apr 2025 18:02:56 GMT, Stuart Marks wrote: >> * Windows and Unix: set sprops.stdin_encoding if connected to a console >> * Add specs for stdin.encoding >> * Adjust specs to change "undefined" to "unspecified" >> * Rewrite System.in spec to refer to new property and to clarify usage with >> classes that perform encoding >> * Update property test > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Yet another minor bit of wordsmithing. LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/24738#pullrequestreview-2791835320
Re: RFR: 8354424: java/util/logging/LoggingDeadlock5.java fails intermittently in tier6 [v2]
On Wed, 16 Apr 2025 17:37:07 GMT, David Beaumont wrote: >> Increasing timeout for deadlock detection and adjusting for the timeout >> factor in higher tiers. > > David Beaumont has updated the pull request incrementally with one additional > commit since the last revision: > > Removing test from the problem list. Looks reasonable. This raises the base timeout by 4x. It also includes the timeout factor adjustment, which can be quite large (8x?) in higher tiers. I think at this point we just have to expose it to more runs to see whether it still fails too frequently. - Marked as reviewed by smarks (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/24687#pullrequestreview-2791829277
Re: RFR: 8344706: Compiler Implementation of Compact Source Files and Instance Main Methods [v4]
On Tue, 22 Apr 2025 18:56:40 GMT, Stuart Marks wrote:
>> src/java.base/share/classes/java/lang/IO.java line 41:
>>
>>> 39: * The {@link #readln()} and {@link #readln(String)} methods decode
>>> bytes read from
>>> 40: * {@code System.in} into characters. The charset used for decoding is
>>> specified by the
>>> 41: * {@link System#getProperties stdin.encoding} property. If this
>>> property is not present,
>>
>> @stuart-marks Are you planning to propose/integrate pull/24738 in advance of
>> the JEP update? Asking because this paragraph will need to be adjusted if
>> pull/24738 doesn't go in first.
>
> Depends on how close you think 24738 is to being ready. As it stands it seems
> ok; but I'm still looking at the potential impact on other part of the JDK.
> In particular are there other JDK APIs that should be adjusted to mention
> `stdin.encoding`?
>
> As for the impact here, if 24738 doesn't go in first, then I'm not quite sure
> what this should say. I guess it could say "default charset" or something and
> then it could be amended with a bugfix later.
(After some further discussion, it looks like 24738 is quite close to being
ready.)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24438#discussion_r2058832242
Re: RFR: 8355301: Simplify Throwable::printStackTrace by using record [v4]
On Fri, 25 Apr 2025 00:53:35 GMT, Chen Liang wrote: >> To what end? >> >> - "I was reading Throwable and noticed this possible refactoring." >> - "I've run an analysis of the JDK code base and this is the first of N >> patches to move to using private records." >> - "With this change, metric X is Y% better." >> >> Something else? > > I think the initial version tried to use record and interfaces to simplify > declarations; then I suggested that we have 3 classes, but we can just use an > on-site instanceof check to reduce number of classes (which will be > individual files in images, also class loading penalty for stack trace > printing). However, Alan has this comment, which I don't understand quite > well either - seems Alan was looking at an older revision as the interface > was removed at the time of his comment. I was reading `Throwable` source code and saw this and thought we could use the new language feature `record`. We should use more new language features in the core library, and this will be used as a reference. - PR Review Comment: https://git.openjdk.org/jdk/pull/24795#discussion_r2059430988
