Re: RFR: 8304303: implement VirtualThread class notifyJvmti methods as C2 intrinsics [v4]
On Fri, 17 Mar 2023 10:31:46 GMT, Serguei Spitsyn wrote: >> This is needed for future performance/scalability improvements in JVMTI >> support of virtual threads. >> The update includes the following: >> >> 1. Refactored the `VirtualThread` native methods: >> `notifyJvmtiMountBegin` and `notifyJvmtiMountEnd` => >> `notifyJvmtiMount` >> `notifyJvmtiUnmountBegin` and `notifyJvmtiUnmountEnd` => >> `notifyJvmtiUnmount` >> 2. Still useful implementation of old native methods is moved from `jvm.cpp` >> to `jvmtiThreadState.cpp`: >> `JVM_VirtualThreadMountStart` => `VTMS_mount_begin` >> `JVM_VirtualThreadMountEnd` => `VTMS_mount_end` >> `JVM_VirtualThreadUnmountStart` = > `VTMS_unmount_begin` >> `JVM_VirtualThreadUnmountEnd`=> `VTMS_mount_end` >> 3. Intrinsified the `VirtualThread` native methods: `notifyJvmtiMount`, >> `notifyJvmtiUnmount`, `notifyJvmtiHideFrames`. >> 4. Removed the`VirtualThread` static boolean state variable >> `notifyJvmtiEvents` and its support in `javaClasses`. >> 5. Added static boolean state variable `_VTMS_notify_jvmti_events` to the >> jvmtiVTMSTransitionDisabler class as a replacement of the `VirtualThread` >> `notifyJvmtiEvents` variable. >> >> Implementing the same methods as C1 intrinsics can be needed in the future >> but is a low priority for now. >> >> Testing: >> - Ran mach5 tiers 1-6. No regressions were found. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > minor tweaks in intrisics implementation Thank you for review, Leonid! - PR: https://git.openjdk.org/jdk/pull/13054
Re: RFR: 8304303: implement VirtualThread class notifyJvmti methods as C2 intrinsics [v4]
On Sat, 18 Mar 2023 11:24:47 GMT, Alan Bateman wrote: > The most important case is when there is no JVMTI env. If I read the changes > correctly, the overhead for park/continue changes from one volatile-read > (notifyJvmtiEvents) to two plain-writes (JavaThread::_is_in_VTMS_transition). > > If a JVMTI env has been created then there is no benefit for the moment as > there is still a call into the runtime to interact with > JvmtiVTMSTransitionDisabler. So I think you are saying that is for follow-on > PRs. @AlanBateman Yes, your conclusion is correct. - PR: https://git.openjdk.org/jdk/pull/13054
Re: RFR: 8304303: implement VirtualThread class notifyJvmti methods as C2 intrinsics [v4]
On Fri, 17 Mar 2023 10:31:46 GMT, Serguei Spitsyn wrote: >> This is needed for future performance/scalability improvements in JVMTI >> support of virtual threads. >> The update includes the following: >> >> 1. Refactored the `VirtualThread` native methods: >> `notifyJvmtiMountBegin` and `notifyJvmtiMountEnd` => >> `notifyJvmtiMount` >> `notifyJvmtiUnmountBegin` and `notifyJvmtiUnmountEnd` => >> `notifyJvmtiUnmount` >> 2. Still useful implementation of old native methods is moved from `jvm.cpp` >> to `jvmtiThreadState.cpp`: >> `JVM_VirtualThreadMountStart` => `VTMS_mount_begin` >> `JVM_VirtualThreadMountEnd` => `VTMS_mount_end` >> `JVM_VirtualThreadUnmountStart` = > `VTMS_unmount_begin` >> `JVM_VirtualThreadUnmountEnd`=> `VTMS_mount_end` >> 3. Intrinsified the `VirtualThread` native methods: `notifyJvmtiMount`, >> `notifyJvmtiUnmount`, `notifyJvmtiHideFrames`. >> 4. Removed the`VirtualThread` static boolean state variable >> `notifyJvmtiEvents` and its support in `javaClasses`. >> 5. Added static boolean state variable `_VTMS_notify_jvmti_events` to the >> jvmtiVTMSTransitionDisabler class as a replacement of the `VirtualThread` >> `notifyJvmtiEvents` variable. >> >> Implementing the same methods as C1 intrinsics can be needed in the future >> but is a low priority for now. >> >> Testing: >> - Ran mach5 tiers 1-6. No regressions were found. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > minor tweaks in intrisics implementation The most important case is when there is no JVMTI env. If I read the changes correctly, the overhead for park/continue changes from one volatile-read (notifyJvmtiEvents) to two plain-writes (JavaThread::_is_in_VTMS_transition). If a JVMTI env has been created then there is no benefit for the moment as there is still a call into the runtime to interact with JvmtiVTMSTransitionDisabler. So I think you are saying that is for follow-on PRs. - PR: https://git.openjdk.org/jdk/pull/13054
Re: RFR: 8304303: implement VirtualThread class notifyJvmti methods as C2 intrinsics [v4]
On Fri, 17 Mar 2023 17:33:46 GMT, Vladimir Ivanov wrote: > Overall, compiler changes look good. > Any performance numbers to justify the intrinsification? Thank you for review and your guidance and help with C2 intrinsification! My goal was to move the notifyJvmtiEvents checks from Java to VM side without a performance penalty. I do not observe any performance degradation with customized Skynet benchmark executing 5 million virtual threads. Used `time` utility to measure total execution time (in milliseconds) of 10 runs on Oracle Linux server: - without intrinsics: 6083, 5405, 5270, 5700, 5004, 5402, 5536, 5031, 4902, 5124 - with intrinsics: 5904, 5287, 5470, 5672, 5298, 5053, 6154, 4992, 6237, 5155 - PR: https://git.openjdk.org/jdk/pull/13054
Re: RFR: 8304303: implement VirtualThread class notifyJvmti methods as C2 intrinsics [v4]
On Fri, 17 Mar 2023 10:31:46 GMT, Serguei Spitsyn wrote: >> This is needed for performance improvements in support of virtual threads. >> The update includes the following: >> >> 1. Refactored the `VirtualThread` native methods: >> `notifyJvmtiMountBegin` and `notifyJvmtiMountEnd` => >> `notifyJvmtiMount` >> `notifyJvmtiUnmountBegin` and `notifyJvmtiUnmountEnd` => >> `notifyJvmtiUnmount` >> 2. Still useful implementation of old native methods is moved from `jvm.cpp` >> to `jvmtiThreadState.cpp`: >> `JVM_VirtualThreadMountStart` => `VTMS_mount_begin` >> `JVM_VirtualThreadMountEnd` => `VTMS_mount_end` >> `JVM_VirtualThreadUnmountStart` = > `VTMS_unmount_begin` >> `JVM_VirtualThreadUnmountEnd`=> `VTMS_mount_end` >> 3. Intrinsified the `VirtualThread` native methods: `notifyJvmtiMount`, >> `notifyJvmtiUnmount`, `notifyJvmtiHideFrames`. >> 4. Removed the`VirtualThread` static boolean state variable >> `notifyJvmtiEvents` and its support in `javaClasses`. >> 5. Added static boolean state variable `_VTMS_notify_jvmti_events` to the >> jvmtiVTMSTransitionDisabler class as a replacement of the `VirtualThread` >> `notifyJvmtiEvents` variable. >> >> Implementing the same methods as C1 intrinsics can be needed in the future >> but is a low priority for now. >> >> Testing: >> - Ran mach5 tiers 1-6. No regressions were found. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > minor tweaks in intrisics implementation Overall, compiler changes look good. Any performance numbers to justify the intrinsification? - Marked as reviewed by vlivanov (Reviewer). PR: https://git.openjdk.org/jdk/pull/13054
Re: RFR: 8304303: implement VirtualThread class notifyJvmti methods as C2 intrinsics [v4]
> This is needed for performance improvements in support of virtual threads. > The update includes the following: > > 1. Refactored the `VirtualThread` native methods: > `notifyJvmtiMountBegin` and `notifyJvmtiMountEnd` => > `notifyJvmtiMount` > `notifyJvmtiUnmountBegin` and `notifyJvmtiUnmountEnd` => > `notifyJvmtiUnmount` > 2. Still useful implementation of old native methods is moved from `jvm.cpp` > to `jvmtiThreadState.cpp`: > `JVM_VirtualThreadMountStart` => `VTMS_mount_begin` > `JVM_VirtualThreadMountEnd` => `VTMS_mount_end` > `JVM_VirtualThreadUnmountStart` = > `VTMS_unmount_begin` > `JVM_VirtualThreadUnmountEnd`=> `VTMS_mount_end` > 3. Intrinsified the `VirtualThread` native methods: `notifyJvmtiMount`, > `notifyJvmtiUnmount`, `notifyJvmtiHideFrames`. > 4. Removed the`VirtualThread` static boolean state variable > `notifyJvmtiEvents` and its support in `javaClasses`. > 5. Added static boolean state variable `_VTMS_notify_jvmti_events` to the > jvmtiVTMSTransitionDisabler class as a replacement of the `VirtualThread` > `notifyJvmtiEvents` variable. > > Implementing the same methods as C1 intrinsics can be needed in the future > but is a low priority for now. > > Testing: > - Ran mach5 tiers 1-6. No regressions were found. Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: minor tweaks in intrisics implementation - Changes: - all: https://git.openjdk.org/jdk/pull/13054/files - new: https://git.openjdk.org/jdk/pull/13054/files/f3692263..8233f0ab Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13054=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13054=02-03 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13054.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13054/head:pull/13054 PR: https://git.openjdk.org/jdk/pull/13054
Re: RFR: 8304303: implement VirtualThread class notifyJvmti methods as C2 intrinsics [v3]
> This is needed for performance improvements in support of virtual threads. > The update includes the following: > > 1. Refactored the `VirtualThread` native methods: > `notifyJvmtiMountBegin` and `notifyJvmtiMountEnd` => > `notifyJvmtiMount` > `notifyJvmtiUnmountBegin` and `notifyJvmtiUnmountEnd` => > `notifyJvmtiUnmount` > 2. Still useful implementation of old native methods is moved from `jvm.cpp` > to `jvmtiThreadState.cpp`: > `JVM_VirtualThreadMountStart` => `VTMS_mount_begin` > `JVM_VirtualThreadMountEnd` => `VTMS_mount_end` > `JVM_VirtualThreadUnmountStart` = > `VTMS_unmount_begin` > `JVM_VirtualThreadUnmountEnd`=> `VTMS_mount_end` > 3. Intrinsified the `VirtualThread` native methods: `notifyJvmtiMount`, > `notifyJvmtiUnmount`, `notifyJvmtiHideFrames`. > 4. Removed the`VirtualThread` static boolean state variable > `notifyJvmtiEvents` and its support in `javaClasses`. > 5. Added static boolean state variable `_VTMS_notify_jvmti_events` to the > jvmtiVTMSTransitionDisabler class as a replacement of the `VirtualThread` > `notifyJvmtiEvents` variable. > > Implementing the same methods as C1 intrinsics can be needed in the future > but is a low priority for now. > > Testing: > - Ran mach5 tiers 1-6. No regressions were found. Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: address pre-review comments from Leonid - Changes: - all: https://git.openjdk.org/jdk/pull/13054/files - new: https://git.openjdk.org/jdk/pull/13054/files/397b6337..f3692263 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13054=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13054=01-02 Stats: 22 lines in 2 files changed: 14 ins; 4 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/13054.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13054/head:pull/13054 PR: https://git.openjdk.org/jdk/pull/13054
Re: RFR: 8304303: implement VirtualThread class notifyJvmti methods as C2 intrinsics [v2]
> This is needed for performance improvements in support of virtual threads. > The update includes the following: > > 1. Refactored the `VirtualThread` native methods: > `notifyJvmtiMountBegin` and `notifyJvmtiMountEnd` => > `notifyJvmtiMount` > `notifyJvmtiUnmountBegin` and `notifyJvmtiUnmountEnd` => > `notifyJvmtiUnmount` > 2. Still useful implementation of old native methods is moved from `jvm.cpp` > to `jvmtiThreadState.cpp`: > `JVM_VirtualThreadMountStart` => `VTMS_mount_begin` > `JVM_VirtualThreadMountEnd` => `VTMS_mount_end` > `JVM_VirtualThreadUnmountStart` = > `VTMS_unmount_begin` > `JVM_VirtualThreadUnmountEnd`=> `VTMS_mount_end` > 3. Intrinsified the `VirtualThread` native methods: `notifyJvmtiMount`, > `notifyJvmtiUnmount`, `notifyJvmtiHideFrames`. > 4. Removed the`VirtualThread` static boolean state variable > `notifyJvmtiEvents` and its support in `javaClasses`. > 5. Added static boolean state variable `_VTMS_notify_jvmti_events` to the > jvmtiVTMSTransitionDisabler class as a replacement of the `VirtualThread` > `notifyJvmtiEvents` variable. > > Implementing the same methods as C1 intrinsics can be needed in the future > but is a low priority for now. > > Testing: > - Ran mach5 tiers 1-6. No regressions were found. Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: include jniHandles.hpp into sharedRuntime.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/13054/files - new: https://git.openjdk.org/jdk/pull/13054/files/8a379320..397b6337 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13054=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13054=00-01 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13054.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13054/head:pull/13054 PR: https://git.openjdk.org/jdk/pull/13054
RFR: 8304303: implement VirtualThread class notifyJvmti methods as C2 intrinsics
This is needed for performance improvements in support of virtual threads. The update includes the following: 1. Refactored the `VirtualThread` native methods: `notifyJvmtiMountBegin` and `notifyJvmtiMountEnd` => `notifyJvmtiMount` `notifyJvmtiUnmountBegin` and `notifyJvmtiUnmountEnd` => `notifyJvmtiUnmount` 2. Still useful implementation of old native methods is moved from `jvm.cpp` to `jvmtiThreadState.cpp`: `JVM_VirtualThreadMountStart` => `VTMS_mount_begin` `JVM_VirtualThreadMountEnd` => `VTMS_mount_end` `JVM_VirtualThreadUnmountStart` = > `VTMS_unmount_begin` `JVM_VirtualThreadUnmountEnd`=> `VTMS_mount_end` 3. Intrinsified the `VirtualThread` native methods: `notifyJvmtiMount`, `notifyJvmtiUnmount`, `notifyJvmtiHideFrames`. 4. Removed the`VirtualThread` static boolean state variable `notifyJvmtiEvents` and its support in `javaClasses`. 5. Added static boolean state variable `_VTMS_notify_jvmti_events` to the jvmtiVTMSTransitionDisabler class as a replacement of the `VirtualThread` `notifyJvmtiEvents` variable. Implementing the same methods as C1 intrinsics can be needed in the future but is a low priority for now. Testing: - Ran mach5 tiers 1-6. No regressions were found. - Commit messages: - fix traling spaces in a couple of files - minor update for VTMS_notify_jvmti_events variable - 8304303: implement VirtualThread class notifyJvmti methods as C2 intrinsics Changes: https://git.openjdk.org/jdk/pull/13054/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13054=00 Issue: https://bugs.openjdk.org/browse/JDK-8304303 Stats: 438 lines in 20 files changed: 265 ins; 125 del; 48 mod Patch: https://git.openjdk.org/jdk/pull/13054.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13054/head:pull/13054 PR: https://git.openjdk.org/jdk/pull/13054
