Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]
On Tue, 5 Mar 2024 23:11:07 GMT, David Holmes wrote: > Okay so that is where the carrier and virtual thread states get back in sync, > and that is what is missing in the `RawMonitorWait` case. The proposed > fix/change to `is_interrupted` is what threw me as that is the wrong place to > make a change because it affects both cases. What is missing is an upcall > from `RawMonitorWait` to some Java code to do the same job as the > `Object.wait` Java code does. There was a duplication in the `Object.wait` which has been just removed. Please, see the incremental update 07: [Full](https://webrevs.openjdk.org/?repo=jdk&pr=18093&range=07) - [Incremental](https://webrevs.openjdk.org/?repo=jdk&pr=18093&range=06-07) ([923a3ff5](https://git.openjdk.org/jdk/pull/18093/files/923a3ff580cfc4d0a68775e5f849b63c691b8eb3)). Now, both `Object.wait` and `RawMonitorWait` clear the interrupt status with the `JavaThread::is_interrupted()` only. - PR Comment: https://git.openjdk.org/jdk/pull/18093#issuecomment-1982105087
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]
On Tue, 5 Mar 2024 19:57:39 GMT, Serguei Spitsyn wrote: > The behavior of ObjectMonitor::wait and RawMonitorWait is different. The Object.wait() throws the InterruptedException if it was interrupted. The RawMonitorWait clears the thread interrupt status and returns the error code JVMTI_ERROR_INTERRUPT. That is not a significant/relevant difference as far as I can see. They both call `thread->is_interrupted(true)`. > For Object.wait, the clearing of the interrupt status is done in the Java > code, Okay so that is where the carrier and virtual thread states get back in sync, and that is what is missing in the `RawMonitorWait` case. The proposed fix/change to `is_interrupted` is what threw me as that is the wrong place to make a change because it affects both cases. What is missing is an upcall from `RawMonitorWait` to some Java code to do the same job as the `Object.wait` Java code does. - PR Comment: https://git.openjdk.org/jdk/pull/18093#issuecomment-1979794228
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]
On Tue, 5 Mar 2024 19:57:39 GMT, Serguei Spitsyn wrote: > The Object.wait() throws the InterruptedException if it was interrupted. And clears the interrupted status, just like RawMonitorWait. - PR Comment: https://git.openjdk.org/jdk/pull/18093#issuecomment-1979670426
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]
On Tue, 5 Mar 2024 07:47:19 GMT, Alan Bateman wrote: >> Thank you for sharing this, Chris. It sounds like we need to introduce a >> mechanism to temporarily hide the JVMTI events. The question is if it is >> worth the complexity we add with it, especially if it is used just in a >> couple of cases. > >> If we do it with a Java upcall to the `VirtualThread.getAndClearInterrupt()` >> then we also have to skip the JVMTI events, AFAIK, as it is not a good idea >> to post the JVMTI events recursively. > > I meant it would need to use ObjectLocker to hold the lock in the VM when > changing both fields. It should only need to do this for RawMonitorWait at > this time. For Object.wait, the clearing of the interrupt status is done in > the Java code, something that will change once Object.wait changes to > freeze/unmount when not pinned. Got it. Thank you, Alan. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1513425638
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]
On Tue, 5 Mar 2024 07:07:27 GMT, David Holmes wrote: > I have to say that I don't understand how the behaviour of `RawMonitorWait` > is any different to `ObjectMonitor::wait` when it comes to the use of the > is_interrupted(true). ??? Is it simply that because we are in native code and > we can immediately query the actual thread state that we can observe when the > carrier and virtual thread states are transiently different? The behavior of `ObjectMonitor::wait` and `RawMonitorWait` is different. The `Object.wait()` throws the `InterruptedException` if it was interrupted. The `RawMonitorWait` clears the thread `interrupt status` and returns the error code `JVMTI_ERROR_INTERRUPT`. Also, I guess, Alan partially answered this question here: > For `Object.wait`, the clearing of the interrupt status is done in the Java > code, something that will change once `Object.wait` changes to > `freeze/unmount` when not pinned. - PR Comment: https://git.openjdk.org/jdk/pull/18093#issuecomment-1979528696
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]
On Tue, 5 Mar 2024 18:16:03 GMT, Leonid Mesnik wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: addressed a couple of comments on new test > > test/hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor/InterruptRawMonitor.java > line 51: > >> 49: System.out.println(thread); >> 50: thread.start(); >> 51: Thread.sleep(2000); > > I think it would be better to check 'thread' status or the same monitor to > sync instead of sleep. The sleep is always looks suspect, especially when > intermittent failure happens. > Also, it helps to save 2 seconds. Will implement this suggestion, thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1513397834
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]
On Tue, 5 Mar 2024 06:16:04 GMT, Serguei Spitsyn wrote: >> Please, review this fix correcting the JVMTI `RawMonitorWait()` >> implementation. >> The `RawMonitorWait()` is using the the `jt->is_interrupted(true)` to >> update the interrupt status of the interrupted waiting thread. The issue is >> that when it calls `jt->is_interrupted(true)` to fetch and clear the >> `interrupt status` of the virtual thread, this information is not propagated >> to the `java.lang.Thread` instance. >> In the `VirtualThread` class implementation the `interrupt status` for >> virtual thread is stored for both virtual and carrier threads. It is because >> the implementation of object monitors for virtual threads is based on >> pinning virtual threads, and so, always operates on carrier thread. The fix >> is to clear the interrupt status for both virtual and carrier >> `java.lang.Thread` instances. >> >> Testing: >> - tested with new test >> `hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor` which is >> passed with the fix and failed without it >> - ran mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: addressed a couple of comments on new test Changes requested by lmesnik (Reviewer). test/hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor/InterruptRawMonitor.java line 51: > 49: System.out.println(thread); > 50: thread.start(); > 51: Thread.sleep(2000); I think it would be better to check 'thread' status or the same monitor to sync instead of sleep. The sleep is always looks suspect, especially when intermittent failure happens. Also, it helps to save 2 seconds. - PR Review: https://git.openjdk.org/jdk/pull/18093#pullrequestreview-1917793041 PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1513292890
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]
On Tue, 5 Mar 2024 01:06:32 GMT, Serguei Spitsyn wrote: >>> AFAIK, as it is not a good idea to post the JVMTI events recursively >> >> We already do. When the debug agent is handling a JVMTI event and >> RawMonitorWait is interrupted, that results in the debug agent later on >> calling InterrtupThread before exiting the event handler. For virtual >> threads that results in an upcall to Thread.interrupt(), and therefore the >> potential for a JVMTI event to be triggered (and that is exactly what >> happens frequently in the com/sun/jdi/InterruptHang.java test). At least in >> that case the debug agent is somewhat in control of the situation because it >> is just doing "cleanup" before exiting the event handler. For example, no >> locks are being held. Having RawMonitorWait do a java upcall sounds a >> potential big can of worms. > > Thank you for sharing this, Chris. It sounds like we need to introduce a > mechanism to temporarily hide the JVMTI events. The question is if it is > worth the complexity we add with it, especially if it is used just in a > couple of cases. > If we do it with a Java upcall to the `VirtualThread.getAndClearInterrupt()` > then we also have to skip the JVMTI events, AFAIK, as it is not a good idea > to post the JVMTI events recursively. I meant it would need to use ObjectLocker to hold the lock in the VM when changing both fields. It should only need to do this for RawMonitorWait at this time. For Object.wait, the clearing of the interrupt status is done in the Java code, something that will change once Object.wait changes to freeze/unmount when not pinned. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1512274259
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]
On Tue, 5 Mar 2024 06:16:04 GMT, Serguei Spitsyn wrote: >> Please, review this fix correcting the JVMTI `RawMonitorWait()` >> implementation. >> The `RawMonitorWait()` is using the the `jt->is_interrupted(true)` to >> update the interrupt status of the interrupted waiting thread. The issue is >> that when it calls `jt->is_interrupted(true)` to fetch and clear the >> `interrupt status` of the virtual thread, this information is not propagated >> to the `java.lang.Thread` instance. >> In the `VirtualThread` class implementation the `interrupt status` for >> virtual thread is stored for both virtual and carrier threads. It is because >> the implementation of object monitors for virtual threads is based on >> pinning virtual threads, and so, always operates on carrier thread. The fix >> is to clear the interrupt status for both virtual and carrier >> `java.lang.Thread` instances. >> >> Testing: >> - tested with new test >> `hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor` which is >> passed with the fix and failed without it >> - ran mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: addressed a couple of comments on new test I have to say that I don't understand how the behaviour of `RawMonitorWait` is any different to `ObjectMonitor::wait` when it comes to the use of the `is_interrupted(true)`. ??? Is it simply that because we are in native code and we can immediately query the actual thread state that we can observe when the carrier and virtual thread states are transiently different? - PR Comment: https://git.openjdk.org/jdk/pull/18093#issuecomment-1978090675
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v2]
> Please, review this fix correcting the JVMTI `RawMonitorWait()` > implementation. > The `RawMonitorWait()` is using the the `jt->is_interrupted(true)` to update > the interrupt status of the interrupted waiting thread. The issue is that > when it calls `jt->is_interrupted(true)` to fetch and clear the `interrupt > status` of the virtual thread, this information is not propagated to the > `java.lang.Thread` instance. > In the `VirtualThread` class implementation the `interrupt status` for > virtual thread is stored for both virtual and carrier threads. It is because > the implementation of object monitors for virtual threads is based on pinning > virtual threads, and so, always operates on carrier thread. The fix is to > clear the interrupt status for both virtual and carrier `java.lang.Thread` > instances. > > Testing: > - tested with new test > `hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor` which is > passed with the fix and failed without it > - ran mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: addressed a couple of comments on new test - Changes: - all: https://git.openjdk.org/jdk/pull/18093/files - new: https://git.openjdk.org/jdk/pull/18093/files/bb920ccb..b365ede4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18093&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18093&range=00-01 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/18093.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18093/head:pull/18093 PR: https://git.openjdk.org/jdk/pull/18093
