RFR: 8350903: Remove explicit libjvm.so dependency for libVThreadEventTest
Please review the test fix that removes `libVThreadEventTest` explicit dependency to `libjvm`, by removing the call to `JNI_GetCreatedJavaVMs` in `Agent_OnAttach`. There is a `vm` argument passed via `Agent_OnAttach`. With the change, `libVThreadEventTest` no longer needs to be linked with `libjvm.so` (or `jvm.dll`). `VThreadEventTest.java` now runs on both dynamic and static JDK. Tested on static JDK locally GHA: https://github.com/jianglizhou/jdk/actions/runs/13577993687/job/37960245161 - Commit messages: - Remove unused `nVMs` variable. - Remove JNI_GetCreatedJavaVMs call. Changes: https://git.openjdk.org/jdk/pull/23835/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23835&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8350903 Stats: 10 lines in 2 files changed: 0 ins; 9 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/23835.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23835/head:pull/23835 PR: https://git.openjdk.org/jdk/pull/23835
Re: RFR: 8348028: Unable to run gtests with CDS enabled [v4]
On Fri, 28 Feb 2025 00:37:24 GMT, Calvin Cheung wrote: >> A simple fix in `os::jvm_path()` so that gtest can be run with CDS >> (`-Xshare:on`). The fix is just to change the directory name from `hotspot` >> to `server`. >> Note that the bug doesn't exist on macOS and thus no change is required for >> `os_bsd.cpp`. >> >> Testing: >> >> - run gtest with -Xshare:on on linux-x64 >> - tier1 > > Calvin Cheung has updated the pull request incrementally with one additional > commit since the last revision: > > @magius and @iklam comments Marked as reviewed by iklam (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/23758#pullrequestreview-2649512032
Re: RFR: 8348028: Unable to run gtests with CDS enabled [v4]
On Fri, 28 Feb 2025 00:37:24 GMT, Calvin Cheung wrote: >> A simple fix in `os::jvm_path()` so that gtest can be run with CDS >> (`-Xshare:on`). The fix is just to change the directory name from `hotspot` >> to `server`. >> Note that the bug doesn't exist on macOS and thus no change is required for >> `os_bsd.cpp`. >> >> Testing: >> >> - run gtest with -Xshare:on on linux-x64 >> - tier1 > > Calvin Cheung has updated the pull request incrementally with one additional > commit since the last revision: > > @magius and @iklam comments I like this approach as it addresses the real issue - that we need to know the variant name to find the right directory. The jvm_path logic in os_bsd is completely broken so we should look at cleaning that up separately. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23758#pullrequestreview-2649521617
Re: RFR: 8350903: Remove explicit libjvm.so dependency for libVThreadEventTest
On Fri, 28 Feb 2025 01:40:34 GMT, Jiangli Zhou wrote: > Please review the test fix that removes `libVThreadEventTest` explicit > dependency to `libjvm`, by removing the call to `JNI_GetCreatedJavaVMs` in > `Agent_OnAttach`. There is a `vm` argument passed via `Agent_OnAttach`. With > the change, `libVThreadEventTest` no longer needs to be linked with > `libjvm.so` (or `jvm.dll`). `VThreadEventTest.java` now runs on both dynamic > and static JDK. > > Tested on static JDK locally > GHA: > https://github.com/jianglizhou/jdk/actions/runs/13577993687/job/37960245161 Marked as reviewed by mscott...@github.com (no known OpenJDK username). - PR Review: https://git.openjdk.org/jdk/pull/23835#pullrequestreview-2649962832
Re: RFR: 8343832: Enhance test summary with number of skipped tests [v8]
On Thu, 27 Feb 2025 16:10:17 GMT, Ivan Bereziuk wrote: >> The output for Jtreg v7.5 was >> [enhanced](https://github.com/openjdk/jtreg/pull/217/files#diff-b6ab77bf651f1fd9a83c3ca0aab9cd24ae5c08cef41e6e257f7eaccc07c7c366R947) >> with [information about skipped >> tests](https://github.com/openjdk/jtreg/blob/master/src/share/doc/javatest/regtest/faq.md#what-do-all-those-numbers-in-the-test-results-line-mean). >> >> Use the additional information in the "summary" block printed at the end of >> `make test` target. > > Ivan Bereziuk has updated the pull request incrementally with two additional > commits since the last revision: > > - Merge pull request #1 from magicus/fix-skipped-tests > >Fix so skipped tests are not considered failures > - Fix so skipped tests are not considered failures Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/22245#pullrequestreview-2648404873
Re: RFR: 8348028: Unable to run gtests with CDS enabled [v4]
> A simple fix in `os::jvm_path()` so that gtest can be run with CDS > (`-Xshare:on`). The fix is just to change the directory name from `hotspot` > to `server`. > Note that the bug doesn't exist on macOS and thus no change is required for > `os_bsd.cpp`. > > Testing: > > - run gtest with -Xshare:on on linux-x64 > - tier1 Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision: @magius and @iklam comments - Changes: - all: https://git.openjdk.org/jdk/pull/23758/files - new: https://git.openjdk.org/jdk/pull/23758/files/4703c943..2e6a55a6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23758&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23758&range=02-03 Stats: 15 lines in 6 files changed: 10 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/23758.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23758/head:pull/23758 PR: https://git.openjdk.org/jdk/pull/23758
Re: RFR: 8348028: Unable to run gtests with CDS enabled [v3]
On Thu, 27 Feb 2025 15:58:55 GMT, Ioi Lam wrote: >> This is completely wrong. >> >> The JVM_CFLAGS_FEATURES should be used to add feature-specific flags, not >> like this. If we really did want to add this to the CFLAGS for all Hotspot >> files, the correct thing to do would be to add it to JVM_CFLAGS in >> JvmFlags.gmk. >> >> However, that is not something we want to do. I fully agree with Julian. >> This is a specific patch needed for one file only, and it should be added >> just for this particular file. Compare how this is done for e.g. >> `abstract_vm_version.cpp` in CompileJvm.gmk. > > Maybe we can add a new API `const char* Abstract_VM_Version::vm_variant()` > that returns the value defined `-DJVM_VARIANT`? Files like os_*cpp and CDS > can call this function to get the string such as "server" or "client". I've pushed another commit based on the above comments. - PR Review Comment: https://git.openjdk.org/jdk/pull/23758#discussion_r1974546276
Re: RFR: 8350903: Remove explicit libjvm.so dependency for libVThreadEventTest
On Fri, 28 Feb 2025 01:40:34 GMT, Jiangli Zhou wrote: > Please review the test fix that removes `libVThreadEventTest` explicit > dependency to `libjvm`, by removing the call to `JNI_GetCreatedJavaVMs` in > `Agent_OnAttach`. There is a `vm` argument passed via `Agent_OnAttach`. With > the change, `libVThreadEventTest` no longer needs to be linked with > `libjvm.so` (or `jvm.dll`). `VThreadEventTest.java` now runs on both dynamic > and static JDK. > > Tested on static JDK locally > GHA: > https://github.com/jianglizhou/jdk/actions/runs/13577993687/job/37960245161 Marked as reviewed by mscott...@github.com (no known OpenJDK username). - PR Review: https://git.openjdk.org/jdk/pull/23835#pullrequestreview-2649953250
Re: RFR: 8350118: Simplify the layout access VarHandle [v2]
On Thu, 27 Feb 2025 13:24:16 GMT, Chen Liang wrote: > ... creation in clinit is super costly You mean because threads can not race to initialize? I'd think the extra walks to create 3 lookups might offset that though... - PR Review Comment: https://git.openjdk.org/jdk/pull/23720#discussion_r1973668377
Re: RFR: 8350118: Simplify the layout access VarHandle [v2]
On Wed, 26 Feb 2025 19:53:45 GMT, Chen Liang wrote: >> src/java.base/share/classes/java/lang/invoke/X-VarHandleSegmentView.java.template >> line 83: >> >>> 81: bb.unsafeGetBase(), >>> 82: offset(bb, base, offset), >>> 83: handle.be); >> >> Why do we not have a call to `convEndian` here? > > This is just how it was. Refer to line 141 in old diff. Ah, right, the unaligned Unsafe routines support specifying the endianess already, but for the aligned variants it has to be emulated. - PR Review Comment: https://git.openjdk.org/jdk/pull/23720#discussion_r1973675226
Re: RFR: 8350118: Simplify the layout access VarHandle [v3]
On Wed, 26 Feb 2025 19:54:59 GMT, Chen Liang wrote: >> I suggest maybe renaming `noStride` to something like >> `fixedOffsetInEnclosing` > > In last revision I called it `fixedOffset`, but it becomes confusing with the > actual fixed value of the offset. Maybe something like `isOffsetFixed` or `hasFixedOffset` would be better? - PR Review Comment: https://git.openjdk.org/jdk/pull/23720#discussion_r1973677125
Re: RFR: 8350118: Simplify the layout access VarHandle [v4]
> Simplify the layout access var handles to be direct in some common cases. > Also made `VarHandle::isAccessModeSupported` report if an access mode is > supported for a VH. > > Reduces the instructions to execute this code in a simple main by 47%: > > long[] arr = new long[8]; > var ms = MemorySegment.ofArray(arr); > ms.setAtIndex(ValueLayout.JAVA_BYTE, 12, (byte) 3); > > > Main overheads in FFM are identified to be: > 1. Eager initialization of direct MethodHandle; can be CDS archived > 2. MH combinator forms via LambdaFormEditor, not cached right now and always > have large overhead > > Still need other measures to deal with common user patterns of > `MethodHandles.insertCoordinates(vh, 1, 0L)` which currently is still very > slow. > > Tests: 2 unrelated failures on 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 14 additional commits since the last revision: - Rollback lazy Segment adapter cache per vernee, fix header indent - Merge branch 'master' of https://github.com/openjdk/jdk into feature/ffm-vh-inline - Jorn vernee review - Review remarks, dates, some more simplifications - More cleanup - Merge branch 'master' of https://github.com/openjdk/jdk into feature/ffm-vh-inline - Make sure the form impl class is initialized - Reduce MT initialization work - MH linkToStatic stall - Merge branch 'master' of https://github.com/openjdk/jdk into feature/ffm-vh-inline - ... and 4 more: https://git.openjdk.org/jdk/compare/23dee8fd...d59ff23a - Changes: - all: https://git.openjdk.org/jdk/pull/23720/files - new: https://git.openjdk.org/jdk/pull/23720/files/3d558a2d..d59ff23a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23720&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23720&range=02-03 Stats: 13038 lines in 332 files changed: 7301 ins; 4580 del; 1157 mod Patch: https://git.openjdk.org/jdk/pull/23720.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23720/head:pull/23720 PR: https://git.openjdk.org/jdk/pull/23720
Re: RFR: 8350118: Simplify the layout access VarHandle [v5]
On Thu, 27 Feb 2025 16:00:47 GMT, Chen Liang wrote: >> Simplify the layout access var handles to be direct in some common cases. >> Also made `VarHandle::isAccessModeSupported` report if an access mode is >> supported for a VH. >> >> Reduces the instructions to execute this code in a simple main by 47%: >> >> long[] arr = new long[8]; >> var ms = MemorySegment.ofArray(arr); >> ms.setAtIndex(ValueLayout.JAVA_BYTE, 12, (byte) 3); >> >> >> Main overheads in FFM are identified to be: >> 1. Eager initialization of direct MethodHandle; can be CDS archived >> 2. MH combinator forms via LambdaFormEditor, not cached right now and always >> have large overhead >> >> Still need other measures to deal with common user patterns of >> `MethodHandles.insertCoordinates(vh, 1, 0L)` which currently is still very >> slow. >> >> Tests: 2 unrelated failures on tier 1-3 > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > noStride -> constantOffset, optimize VH classes to have only 2 instead of 3 > classes for each type Latest version looks good. - Marked as reviewed by jvernee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23720#pullrequestreview-2648300971
Re: RFR: 8350118: Simplify the layout access VarHandle [v5]
> Simplify the layout access var handles to be direct in some common cases. > Also made `VarHandle::isAccessModeSupported` report if an access mode is > supported for a VH. > > Reduces the instructions to execute this code in a simple main by 47%: > > long[] arr = new long[8]; > var ms = MemorySegment.ofArray(arr); > ms.setAtIndex(ValueLayout.JAVA_BYTE, 12, (byte) 3); > > > Main overheads in FFM are identified to be: > 1. Eager initialization of direct MethodHandle; can be CDS archived > 2. MH combinator forms via LambdaFormEditor, not cached right now and always > have large overhead > > Still need other measures to deal with common user patterns of > `MethodHandles.insertCoordinates(vh, 1, 0L)` which currently is still very > slow. > > Tests: 2 unrelated failures on tier 1-3 Chen Liang has updated the pull request incrementally with one additional commit since the last revision: noStride -> constantOffset, optimize VH classes to have only 2 instead of 3 classes for each type - Changes: - all: https://git.openjdk.org/jdk/pull/23720/files - new: https://git.openjdk.org/jdk/pull/23720/files/d59ff23a..e5839fb8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23720&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23720&range=03-04 Stats: 606 lines in 6 files changed: 28 ins; 30 del; 548 mod Patch: https://git.openjdk.org/jdk/pull/23720.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23720/head:pull/23720 PR: https://git.openjdk.org/jdk/pull/23720
Re: RFR: 8343832: Enhance test summary with number of skipped tests [v8]
> The output for Jtreg v7.5 was > [enhanced](https://github.com/openjdk/jtreg/pull/217/files#diff-b6ab77bf651f1fd9a83c3ca0aab9cd24ae5c08cef41e6e257f7eaccc07c7c366R947) > with [information about skipped > tests](https://github.com/openjdk/jtreg/blob/master/src/share/doc/javatest/regtest/faq.md#what-do-all-those-numbers-in-the-test-results-line-mean). > > Use the additional information in the "summary" block printed at the end of > `make test` target. Ivan Bereziuk has updated the pull request incrementally with two additional commits since the last revision: - Merge pull request #1 from magicus/fix-skipped-tests Fix so skipped tests are not considered failures - Fix so skipped tests are not considered failures - Changes: - all: https://git.openjdk.org/jdk/pull/22245/files - new: https://git.openjdk.org/jdk/pull/22245/files/6af586a2..0912b019 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=22245&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22245&range=06-07 Stats: 9 lines in 1 file changed: 4 ins; 4 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/22245.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22245/head:pull/22245 PR: https://git.openjdk.org/jdk/pull/22245
Re: RFR: 8343832: Enhance test summary with number of skipped tests [v8]
On Thu, 27 Feb 2025 16:10:17 GMT, Ivan Bereziuk wrote: >> The output for Jtreg v7.5 was >> [enhanced](https://github.com/openjdk/jtreg/pull/217/files#diff-b6ab77bf651f1fd9a83c3ca0aab9cd24ae5c08cef41e6e257f7eaccc07c7c366R947) >> with [information about skipped >> tests](https://github.com/openjdk/jtreg/blob/master/src/share/doc/javatest/regtest/faq.md#what-do-all-those-numbers-in-the-test-results-line-mean). >> >> Use the additional information in the "summary" block printed at the end of >> `make test` target. > > Ivan Bereziuk has updated the pull request incrementally with two additional > commits since the last revision: > > - Merge pull request #1 from magicus/fix-skipped-tests > >Fix so skipped tests are not considered failures > - Fix so skipped tests are not considered failures Looks good now to me. Technically, I assume it would be good if you get a re-review of the latest commit, since I should not be reviewing my own code. - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22245#pullrequestreview-2648329385
Re: RFR: 8350118: Simplify the layout access VarHandle [v2]
On Wed, 26 Feb 2025 19:56:04 GMT, Chen Liang wrote: >> src/java.base/share/classes/jdk/internal/foreign/Utils.java line 74: >> >>> 72: return ret; >>> 73: return computeFilterHandle(index); >>> 74: } >> >> Why is this using an array, instead of just having 3 fields? > > This emulates how MethodHandleImpl does the cache. Ok. I think the benefit is that each element is individually lazy initialized. I'm not sure about the setup with the array though. It seems like having 3 stable fields would be simpler? Or maybe just initialize them up front in clinit, since they are mostly used together. That would also avoid the need to create a `Lookup` every time. - PR Review Comment: https://git.openjdk.org/jdk/pull/23720#discussion_r1973301941
Re: RFR: 8350118: Simplify the layout access VarHandle [v2]
On Thu, 27 Feb 2025 10:26:29 GMT, Jorn Vernee wrote: >> This emulates how MethodHandleImpl does the cache. > > Ok. I think the benefit is that each element is individually lazy > initialized. I'm not sure about the setup with the array though. It seems > like having 3 stable fields would be simpler? > > Or maybe just initialize them up front in clinit, since they are mostly used > together. That would also avoid the need to create a `Lookup` every time, > which might help performance since that method is CallerSenstive, so we need > to do a stack walk to get the caller when running in the interpreter. Well, the reason I removed the eager init is that their creation in clinit is super costly. Also I think one pair of getter + creator is better than 3 pairs. - PR Review Comment: https://git.openjdk.org/jdk/pull/23720#discussion_r1973576054
