Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v5]
On Fri, 15 Mar 2024 21:00:16 GMT, Serguei Spitsyn wrote: >> So the current changes do not limit this to just `RawMonitorWait`. I was >> expecting to only see additional code in `RawMonitorWait` that emulates what >> the Java code does to get the virtual and carrier thread interrupt states in >> sync, using the interruptLock. > > Okay, I can do this separation. > One minor concern is the `JavaThread::sleep_nanos(jlong nanos)` ? > It is used by the `JavaThread::sleep(jlong millis)` which in tern is not used > much. Fixed now. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1527150170
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v5]
On Fri, 15 Mar 2024 10:05:06 GMT, David Holmes wrote: >> I've restored the `InterruptedException` catch in the `Object.wait`. >> However, the fix in `JavaThread::is_interrupted()` also impacts the >> variations of `sleep_nanos()`. > > So the current changes do not limit this to just `RawMonitorWait`. I was > expecting to only see additional code in `RawMonitorWait` that emulates what > the Java code does to get the virtual and carrier thread interrupt states in > sync, using the interruptLock. Okay, I can do this separation. One minor concern is the `JavaThread::sleep_nanos(jlong nanos)` ? It is used by the `JavaThread::sleep(jlong millis)` which in tern is not used much. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1526831878
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v5]
On Fri, 15 Mar 2024 09:06:33 GMT, Serguei Spitsyn wrote: >> Personally I'd prefer to see changes limited to just JVMTI `RawMonitorWait`. >> That minimises the risk of any unintended consequences from making the >> change. > > I've restored the `InterruptedException` catch in the `Object.wait`. > However, the fix in `JavaThread::is_interrupted()` also impacts the > variations of `sleep_nanos()`. So the current changes do not limit this to just `RawMonitorWait`. I was expecting to only see additional code in `RawMonitorWait` that emulates what the Java code does to get the virtual and carrier thread interrupt states in sync, using the interruptLock. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1526041006
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v5]
On Fri, 8 Mar 2024 06:21:17 GMT, David Holmes wrote: >> It may be that once Object.wait is implemented that we can remove the need >> to propagate the interrupt status (there are some TBDs here) so things will >> be a lot simpler. >> >> I think the change here is okay for now but we still have the choice of >> limiting the change to just JVMTI RawMonitorWait. > > Personally I'd prefer to see changes limited to just JVMTI `RawMonitorWait`. > That minimises the risk of any unintended consequences from making the change. I've restored the `InterruptedException` catch in the `Object.wait`. However, the fix in `JavaThread::is_interrupted()` also impacts the variations of `sleep_nanos()`. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1525958712
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v5]
On Thu, 7 Mar 2024 08:42:11 GMT, Alan Bateman wrote: >> Use of `ObjectLocker` here will introduce a new pinning point for Loom. We >> have been removing as many uses of `ObjectLocker` as we can. I also think >> this will need to be moved back to Java code when the pinning currently >> inherent in calling `Object.wait` is addressed. > > Yes, and it may be that once Object.wait is implemented that we can remove > the need to propagate the interrupt status (there are some TBDs here). > > I think the change here is okay for now but we still have the choice of > limiting the change to just JVMTI RawMonitorWait. Personally I'd prefer to see changes limited to just JVMTI `RawMonitorWait`. That minimises the risk of any unintended consequences from making the change. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1517212478
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v5]
On Thu, 7 Mar 2024 06:45:01 GMT, David Holmes wrote: >> Fixed as suggested by Alan. >> >>> Also need to think through if Object.wait will need to be changed as part >>> of this. >> >> Still need to look at this. > > Use of `ObjectLocker` here will introduce a new pinning point for Loom. We > have been removing as many uses of `ObjectLocker` as we can. I also think > this will need to be moved back to Java code when the pinning currently > inherent in calling `Object.wait` is addressed. Yes, and it may be that once Object.wait is implemented that we can remove the need to propagate the interrupt status (there are some TBDs here). I think the change here is okay for now but we still have the choice of limiting the change to just JVMTI RawMonitorWait. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1515760599
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v5]
On Wed, 6 Mar 2024 10:41:21 GMT, Serguei Spitsyn wrote: >> Thanks, I'm already working on it. > > Fixed as suggested by Alan. > >> Also need to think through if Object.wait will need to be changed as part of >> this. > > Still need to look at this. Use of `ObjectLocker` here will introduce a new pinning point for Loom. We have been removing as many uses of `ObjectLocker` as we can. I also think this will need to be moved back to Java code when the pinning currently inherent in calling `Object.wait` is addressed. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1515622346
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v5]
On Wed, 6 Mar 2024 09:34:40 GMT, Serguei Spitsyn wrote: >> src/hotspot/share/runtime/javaThread.cpp line 573: >> >>> 571: >>> 572: Handle thread_h(current, vthread_or_thread()); >>> 573: ObjectLocker lock(Handle(current, >>> java_lang_Thread::interrupt_lock(thread_h())), current); >> >> For this version then I assume you should limit it when its a virtual thread >> and when clear_interrupted is true. >> >> Also need to think through if Object.wait will need to be changed as part of >> this. > > Thanks, I'm already working on it. Fixed as suggested by Alan. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1514240306
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v5]
On Wed, 6 Mar 2024 09:27:18 GMT, Alan Bateman wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> removed trailing spaces in javaClasses.cpp and libInterruptRawMonitor.cpp > > src/hotspot/share/runtime/javaThread.cpp line 573: > >> 571: >> 572: Handle thread_h(current, vthread_or_thread()); >> 573: ObjectLocker lock(Handle(current, >> java_lang_Thread::interrupt_lock(thread_h())), current); > > For this version then I assume you should limit it when its a virtual thread > and when clear_interrupted is true. > > Also need to think through if Object.wait will need to be changed as part of > this. Thanks, I'm already working on it. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1514137662
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v5]
On Wed, 6 Mar 2024 09:14:18 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: > > removed trailing spaces in javaClasses.cpp and libInterruptRawMonitor.cpp src/hotspot/share/runtime/javaThread.cpp line 573: > 571: > 572: Handle thread_h(current, vthread_or_thread()); > 573: ObjectLocker lock(Handle(current, > java_lang_Thread::interrupt_lock(thread_h())), current); For this version then I assume you should limit it when its a virtual thread and when clear_interrupted is true. Also need to think through if Object.wait will need to be changed as part of this. - PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1514126817
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v5]
> 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: removed trailing spaces in javaClasses.cpp and libInterruptRawMonitor.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/18093/files - new: https://git.openjdk.org/jdk/pull/18093/files/064d8225..b99fad54 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18093&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18093&range=03-04 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 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
