Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v3]
On Thu, 7 Mar 2024 07:20:15 GMT, David Holmes wrote: > Oracle requests/requires that the Oracle copyright always be updated when a > file is modified. Got it. Thanks for your explanation. - PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1515672685
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v8]
On Thu, 7 Mar 2024 00:13:56 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: dropped the catch of InterruptedException in Object.wait Can you check if vmTestbase/nsk/jvmti/scenarios/bcinstr/BI04/bi04t002/TestDescription.java is passing now? That test is a bit annoying in that it fails every time we touch j.l.Object so you might have to update BI04/bi04t002/newclass02/java.base/java/lang/Object.java again. - PR Comment: https://git.openjdk.org/jdk/pull/18093#issuecomment-1982739813
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v3]
On Thu, 7 Mar 2024 03:00:11 GMT, Guoxiong Li wrote: >> Matthias Baesken has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Introduce windows jni_util_md.h > > src/java.base/aix/native/libnio/ch/Pollset.c line 3: > >> 1: /* >> 2: * Copyright (c) 2008, 2024, Oracle and/or its affiliates. All rights >> reserved. >> 3: * Copyright (c) 2012, 2024 SAP SE. All rights reserved. > > It seems you only need to update the copyright of the company you work for. Oracle requests/requires that the Oracle copyright always be updated when a file is modified. - PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1515660944
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: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v3]
On Wed, 6 Mar 2024 16:30:23 GMT, Matthias Baesken wrote: >> We define the RESTARTABLE macro again and again at a lot of places in the >> JDK native codebase. This could be centralized to avoid repeating it again >> and again ! > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Introduce windows jni_util_md.h src/java.base/aix/native/libnio/ch/Pollset.c line 3: > 1: /* > 2: * Copyright (c) 2008, 2024, Oracle and/or its affiliates. All rights > reserved. > 3: * Copyright (c) 2012, 2024 SAP SE. All rights reserved. It seems you only need to update the copyright of the company you work for. - PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1515417217
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 01:38:56 GMT, Coleen Phillimore wrote:
>> This change creates a new sort of native recursive lock that can be held
>> during JNI and Java calls, which can be used for synchronization while
>> creating objArrayKlasses at runtime.
>>
>> Passes tier1-7.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Dean's comments.
This looks good to me. Thanks for working through the details with me.
src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp line 121:
> 119: {
> 120: // To get a consistent list of classes we need MultiArray_lock to
> ensure
> 121: // array classes aren't created during this walk. This walks through
> the
Just curious but how can walking the set of classes trigger creation of array
classes?
src/hotspot/share/runtime/mutexLocker.hpp line 335:
> 333:
> 334: // Instance of a RecursiveLock that may be held through Java heap
> allocation, which may include calls to Java,
> 335: // and JNI event notification for resource exhausted for metaspace or
> heap.
s/exhausted/exhaustion/
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17739#pullrequestreview-1921323899
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515402581
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515406082
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 01:38:56 GMT, Coleen Phillimore wrote: >> This change creates a new sort of native recursive lock that can be held >> during JNI and Java calls, which can be used for synchronization while >> creating objArrayKlasses at runtime. >> >> Passes tier1-7. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Dean's comments. > Does a PlatformMutex handle priority-inheritance? It would depend on the OS and the mutex impementation. You can ignore my comment. It was from long ago trying to put Java on top of a real-time OS. - PR Comment: https://git.openjdk.org/jdk/pull/17739#issuecomment-1982207873
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 01:38:56 GMT, Coleen Phillimore wrote: >> This change creates a new sort of native recursive lock that can be held >> during JNI and Java calls, which can be used for synchronization while >> creating objArrayKlasses at runtime. >> >> Passes tier1-7. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Dean's comments. src/hotspot/share/oops/instanceKlass.cpp line 1552: > 1550: RecursiveLocker rl(MultiArray_lock, THREAD); > 1551: > 1552: // This thread is the creator. This thread may not be the creator. The original comment was more apt - we typically say something like "Check if another thread beat us to it." - PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515371571
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 00:54:30 GMT, David Holmes wrote:
>> OK. It's a good thing HotSpot doesn't need to worry about
>> priority-inheritance for mutexes.
>
> Semaphore seems simpler/cleaner and ready to use.
It's such a rare race and unusual condition that we could execute more Java
code, that I started out with just a shared bit. Then David suggested a
semaphore that obeys the safepoint protocol, which seems a lot better. I've
literally skimmed over OSThreadWaitState. It looks like
Semaphore::wait_with_a_safepoint_check() uses it. I still don't know why it
exists:
// Note: the ThreadState is legacy code and is not correctly implemented.
// Uses of ThreadState need to be replaced by the state in the JavaThread.
enum ThreadState {
Does a PlatformMutex handle priority-inheritance? It still feels like it would
be overkill for this usage. I hope this RecursiveLocker does not become more
widely used, where we would have to replace the simple implementation with
something more difficult. We should discourage further use when possible.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515364466
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 00:18:53 GMT, Dean Long wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Dean's comments.
>
> src/hotspot/share/oops/arrayKlass.cpp line 135:
>
>> 133: ResourceMark rm(THREAD);
>> 134: {
>> 135: // Ensure atomic creation of higher dimensions
>
> Isn't this comment still useful?
Yes, it's a useful comment, readded.
> src/hotspot/share/oops/arrayKlass.cpp line 144:
>
>> 142: ObjArrayKlass* ak =
>> 143: ObjArrayKlass::allocate_objArray_klass(class_loader_data(),
>> dim + 1, this, CHECK_NULL);
>> 144: ak->set_lower_dimension(this);
>
> Would it be useful to assert that the lower dimension has been set?
Yes, added.
> src/hotspot/share/oops/instanceKlass.cpp line 2765:
>
>> 2763: // To get a consistent list of classes we need MultiArray_lock to
>> ensure
>> 2764: // array classes aren't observed while they are being restored.
>> 2765: MutexLocker ml(MultiArray_lock);
>
> Doesn't this revert JDK-8274338?
You're right. I didn't believe my own comment when I removed this. Thank you
for pointing out the bug.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515346198
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515346716
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515349878
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 01:35:45 GMT, Coleen Phillimore wrote: >> This change creates a new sort of native recursive lock that can be held >> during JNI and Java calls, which can be used for synchronization while >> creating objArrayKlasses at runtime. >> >> Passes tier1-7. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Dean's comments. Thanks for your review Dean. I've retested tier1 with the changes and will send it for more. Thank you for finding the CDS bug in my change. - PR Review: https://git.openjdk.org/jdk/pull/17739#pullrequestreview-1921122133
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
> This change creates a new sort of native recursive lock that can be held > during JNI and Java calls, which can be used for synchronization while > creating objArrayKlasses at runtime. > > Passes tier1-7. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Dean's comments. - Changes: - all: https://git.openjdk.org/jdk/pull/17739/files - new: https://git.openjdk.org/jdk/pull/17739/files/3625e6cc..d64aa560 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17739&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17739&range=00-01 Stats: 5 lines in 2 files changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17739.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17739/head:pull/17739 PR: https://git.openjdk.org/jdk/pull/17739
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Thu, 7 Mar 2024 00:16:39 GMT, Dean Long wrote: >> Semaphore seemed simpler (?) > > OK. It's a good thing HotSpot doesn't need to worry about > priority-inheritance for mutexes. Semaphore seems simpler/cleaner and ready to use. - PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515343108
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore wrote: > This change creates a new sort of native recursive lock that can be held > during JNI and Java calls, which can be used for synchronization while > creating objArrayKlasses at runtime. > > Passes tier1-7. Marked as reviewed by dlong (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17739#pullrequestreview-1921100707
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore wrote: > This change creates a new sort of native recursive lock that can be held > during JNI and Java calls, which can be used for synchronization while > creating objArrayKlasses at runtime. > > Passes tier1-7. src/hotspot/share/oops/instanceKlass.cpp line 2765: > 2763: // To get a consistent list of classes we need MultiArray_lock to > ensure > 2764: // array classes aren't observed while they are being restored. > 2765: MutexLocker ml(MultiArray_lock); Doesn't this revert JDK-8274338? - PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515330292
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: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore wrote:
> This change creates a new sort of native recursive lock that can be held
> during JNI and Java calls, which can be used for synchronization while
> creating objArrayKlasses at runtime.
>
> Passes tier1-7.
src/hotspot/share/oops/arrayKlass.cpp line 135:
> 133: ResourceMark rm(THREAD);
> 134: {
> 135: // Ensure atomic creation of higher dimensions
Isn't this comment still useful?
src/hotspot/share/oops/arrayKlass.cpp line 144:
> 142: ObjArrayKlass* ak =
> 143: ObjArrayKlass::allocate_objArray_klass(class_loader_data(),
> dim + 1, this, CHECK_NULL);
> 144: ak->set_lower_dimension(this);
Would it be useful to assert that the lower dimension has been set?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515322667
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515323404
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Wed, 6 Mar 2024 23:47:01 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/runtime/mutex.cpp line 537:
>>
>>> 535: // can be called by jvmti by VMThread.
>>> 536: if (current->is_Java_thread()) {
>>> 537: _sem.wait_with_safepoint_check(JavaThread::cast(current));
>>
>> Why not use PlatformMutex + OSThreadWaitState instead of a semaphore?
>
> Semaphore seemed simpler (?)
OK. It's a good thing HotSpot doesn't need to worry about priority-inheritance
for mutexes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515314037
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v7]
On Wed, 6 Mar 2024 18:56:54 GMT, Alan Bateman wrote: > I think you can drop the catch of InterruptedException in Object.wait now. > Easy to check this by running > test/jdk/java/lang/Thread/virtual/MonitorWaitNotify.java as it has tests for > interrupting Object.wait. Thanks. Fixed now. Tested with the `test/jdk/java/lang/Thread/virtual/MonitorWaitNotify.java`. - PR Comment: https://git.openjdk.org/jdk/pull/18093#issuecomment-1982080510
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v8]
> 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: dropped the catch of InterruptedException in Object.wait - Changes: - all: https://git.openjdk.org/jdk/pull/18093/files - new: https://git.openjdk.org/jdk/pull/18093/files/db3149dc..923a3ff5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18093&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18093&range=06-07 Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 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
Integrated: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer wrote: > PerfMapTest.java issues the Compiler.perfmap jcmd with a filename argument to > write the perfmap to. It does this in 3 different modes. 2 of the modes > result in a perfmap file being left in the tmp directory that is not removed > after the test executes (and should be removed). The 3rd mode creates the > perfmap file in the directory specified by the test.dir property, and is ok > to leave the file there. I've added code to delete the /tmp files that are > created. > > I did a bit of extra testing by hand. I created `/tmp/perf-.map` as > root. As expected the Files.deleteIfExists() call threw an exception due to > the lack of permissions to delete the file. However, I then realized the file > had a size of 0, which means the test was not even doing a proper job of > testing that the perfrmap jcmd was working. In other words, the test should > have failed long before getting to the Files.deleteIfExists(), so I added a > size check to make sure it fails when the file is not written to. This pull request has now been integrated. Changeset: ddcd6dec Author:Chris Plummer URL: https://git.openjdk.org/jdk/commit/ddcd6dece9ef9c8700bc3f8f5dc7a5405fe55a70 Stats: 11 lines in 1 file changed: 8 ins; 0 del; 3 mod 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir. Reviewed-by: amenkov, kevinw, lmesnik, sspitsyn - PR: https://git.openjdk.org/jdk/pull/17992
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Wed, 6 Mar 2024 23:09:34 GMT, Dean Long wrote:
>> This change creates a new sort of native recursive lock that can be held
>> during JNI and Java calls, which can be used for synchronization while
>> creating objArrayKlasses at runtime.
>>
>> Passes tier1-7.
>
> src/hotspot/share/runtime/mutex.cpp line 537:
>
>> 535: // can be called by jvmti by VMThread.
>> 536: if (current->is_Java_thread()) {
>> 537: _sem.wait_with_safepoint_check(JavaThread::cast(current));
>
> Why not use PlatformMutex + OSThreadWaitState instead of a semaphore?
Semaphore seemed simpler (?)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515294583
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v3]
On Wed, 6 Mar 2024 16:30:23 GMT, Matthias Baesken wrote: >> We define the RESTARTABLE macro again and again at a lot of places in the >> JDK native codebase. This could be centralized to avoid repeating it again >> and again ! > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Introduce windows jni_util_md.h src/java.base/unix/native/libnio/ch/Net.c line 2: > 1: /* > 2: * Copyright (c) 2001, 2024, Oracle and/or its affiliates. All rights > reserved. Is the year change needed as it looks like nothing was changed? - PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1515273416
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore wrote:
> This change creates a new sort of native recursive lock that can be held
> during JNI and Java calls, which can be used for synchronization while
> creating objArrayKlasses at runtime.
>
> Passes tier1-7.
OK, that makes sense about loom and JOM.
src/hotspot/share/runtime/mutex.cpp line 537:
> 535: // can be called by jvmti by VMThread.
> 536: if (current->is_Java_thread()) {
> 537: _sem.wait_with_safepoint_check(JavaThread::cast(current));
Why not use PlatformMutex + OSThreadWaitState instead of a semaphore?
-
PR Comment: https://git.openjdk.org/jdk/pull/17739#issuecomment-1982008253
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515269443
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v7]
On Wed, 6 Mar 2024 10:44:15 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: > > optimize holding the interruptLock in JavaThread::is_interrupted I think the updated implementation looks right. I think you can drop the catch of InterruptedException in Object.wait now. Easy to check this by running test/jdk/java/lang/Thread/virtual/MonitorWaitNotify.java as it has tests for interrupting Object.wait. - PR Comment: https://git.openjdk.org/jdk/pull/18093#issuecomment-1981574037
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v3]
On Wed, 6 Mar 2024 16:30:23 GMT, Matthias Baesken wrote:
>> We define the RESTARTABLE macro again and again at a lot of places in the
>> JDK native codebase. This could be centralized to avoid repeating it again
>> and again !
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Introduce windows jni_util_md.h
Looks good to me modulo a few license year things...
src/java.base/macosx/native/libnio/ch/KQueue.c line 2:
> 1: /*
> 2: * Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights
> reserved.
Here you only change the license year?
src/java.base/share/native/libjava/jni_util.h line 30:
> 28:
> 29: #include "jni.h"
> 30: #include "jni_util_md.h"
This file needs copyright year update
src/java.base/unix/native/libjava/childproc.h line 75:
> 73: #define FAIL_FILENO (STDERR_FILENO + 1)
> 74:
> 75: /* TODO: Refactor. */
Copyright year update missing.
src/java.base/unix/native/libnio/ch/nio_util.h line 34:
> 32: #include
> 33:
> 34: #define RESTARTABLE(_cmd, _result) do { \
Copyright year update
src/java.base/unix/native/libnio/fs/UnixFileSystem.c line 38:
> 36: #include "sun_nio_fs_UnixFileSystem.h"
> 37:
> 38: #define RESTARTABLE(_cmd, _result) do { \
Copyright year update
-
Changes requested by clanger (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18132#pullrequestreview-1920355323
PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514867262
PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514873505
PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514876266
PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514878580
PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514878995
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v3]
> We define the RESTARTABLE macro again and again at a lot of places in the JDK > native codebase. This could be centralized to avoid repeating it again and > again ! Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: Introduce windows jni_util_md.h - Changes: - all: https://git.openjdk.org/jdk/pull/18132/files - new: https://git.openjdk.org/jdk/pull/18132/files/f30a189d..2930075d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18132&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18132&range=01-02 Stats: 24 lines in 19 files changed: 1 ins; 22 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18132.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18132/head:pull/18132 PR: https://git.openjdk.org/jdk/pull/18132
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v2]
On Wed, 6 Mar 2024 15:49:05 GMT, Alan Bateman wrote: >> Matthias Baesken has updated the pull request incrementally with one >> additional commit since the last revision: >> >> introduce unix jni_util_md.h > > src/java.base/aix/native/libnio/ch/Pollset.c line 29: > >> 27: #include "jni.h" >> 28: #include "jni_util.h" >> 29: #include "jni_util_md.h" > > When I suggested jni_util_md.h then I meant create one for Unix and another > for Windows, then have jni_util.h include it. This will avoid needing to add > an include to all these files. I considered this too, but the one on Windows would be empty for now -> looks a bit strange . But I can do it why not . - PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514752727
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v2]
On Wed, 6 Mar 2024 15:45:16 GMT, Matthias Baesken wrote: >> We define the RESTARTABLE macro again and again at a lot of places in the >> JDK native codebase. This could be centralized to avoid repeating it again >> and again ! > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > introduce unix jni_util_md.h src/java.base/aix/native/libnio/ch/Pollset.c line 29: > 27: #include "jni.h" > 28: #include "jni_util.h" > 29: #include "jni_util_md.h" When I suggested jni_util_md.h then I meant create one for Unix and another for Windows, then have jni_util.h include it. This will avoid needing to add an include to all these files. - PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514724921
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v2]
> We define the RESTARTABLE macro again and again at a lot of places in the JDK > native codebase. This could be centralized to avoid repeating it again and > again ! Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: introduce unix jni_util_md.h - Changes: - all: https://git.openjdk.org/jdk/pull/18132/files - new: https://git.openjdk.org/jdk/pull/18132/files/014ab1fd..f30a189d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18132&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18132&range=00-01 Stats: 68 lines in 19 files changed: 52 ins; 7 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/18132.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18132/head:pull/18132 PR: https://git.openjdk.org/jdk/pull/18132
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase
On Wed, 6 Mar 2024 14:25:09 GMT, Matthias Baesken wrote: > We could maybe also use the existing > https://github.com/openjdk/jdk/blob/master/src/java.base/unix/native/include/jni_md.h > - what do you think ? jni_md.h goes into the JDK run-time image (for jni.h to include) so I don't think we should be changing that. jni_util.* is JDK internal so I think we're just missing a platform dependent include file. - PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514586726
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase
On Wed, 6 Mar 2024 14:13:26 GMT, Alan Bateman wrote: >> We define the RESTARTABLE macro again and again at a lot of places in the >> JDK native codebase. This could be centralized to avoid repeating it again >> and again ! > > src/java.base/share/native/libjava/jni_util.h line 243: > >> 241: } while((_result == -1) && (errno == EINTR)); \ >> 242: } while(0) >> 243: > > jni_util.h is for all platforms so not the right place for a Unix specific > macro. We need think where the right place for this, might have to introduce > a jni_util_md.h. We could maybe also use the existing https://github.com/openjdk/jdk/blob/master/src/java.base/unix/native/include/jni_md.h - what do you think ? - PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514574059
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase
On Wed, 6 Mar 2024 09:26:25 GMT, Matthias Baesken wrote: > We define the RESTARTABLE macro again and again at a lot of places in the JDK > native codebase. This could be centralized to avoid repeating it again and > again ! src/java.base/share/native/libjava/jni_util.h line 243: > 241: } while((_result == -1) && (errno == EINTR)); \ > 242: } while(0) > 243: jni_util.h is for all platforms so not the right place for a Unix specific macro. We need think where the right place for this, might have to introduce a jni_util_md.h. - PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514555111
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 5 Mar 2024 23:13:30 GMT, Dean Long wrote: >> This change creates a new sort of native recursive lock that can be held >> during JNI and Java calls, which can be used for synchronization while >> creating objArrayKlasses at runtime. >> >> Passes tier1-7. > > Is the caller always a JavaThread? I'm wondering if your new recursive lock > class could use the existing ObjectMonitor. I thought David asked the same > question, but I can't find it. @dean-long An ObjectLocker on the mirror oop for InstanceKlass might work for this but as @dholmes-ora said we've been trying to purge the ObjectLocker code from the C++ code because it's complicated by the Java Object monitor project. The JOM project may throw exceptions from the ObjectLocker constructor or destructor and the C++ code doesn't currently know what to do. We removed the ObjectLockers around class linking and some JVMTI cases. There are some in class loading but with a small amount of work, they can be removed also. The caller is always a JavaThread, some lockers are at a safepoint for traversal but the multi-arrays are only created by JavaThreads. - PR Comment: https://git.openjdk.org/jdk/pull/17739#issuecomment-1980812019
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v7]
On Tue, 5 Mar 2024 11:31:13 GMT, Kevin Walls wrote: >> Introduce the jcmd "VM.debug" to implement access to a useful set of the >> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". >> >> Not recommended for live production use. Calling these "debug" utilities, >> and not including them in the jcmd help output, is to remind us they are not >> general customer-facing tools. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Test update Hi Kevin, > > (If you find existing jcmds a mess, feel free to suggest changes as > appropriate.) you touch on a problem here, and why I think adding commands should be done more carefully (and I am guilty of adding commands too). Once these commands are rolled out, they find themselves in blogs, knowledge bases, scripts that are reused for different JDK releases, and so on. It is difficult to change them post-release. That is why good names are important. Cheers, Thomas - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1980807020
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v7]
On Tue, 5 Mar 2024 11:31:13 GMT, Kevin Walls wrote: >> Introduce the jcmd "VM.debug" to implement access to a useful set of the >> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". >> >> Not recommended for live production use. Calling these "debug" utilities, >> and not including them in the jcmd help output, is to remind us they are not >> general customer-facing tools. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Test update Hi, > once you start poking at the JVM innards at this level, I guess you will be > quickly at the limit of what this command can do for you and need to attach a > debugger anyway. I think we all expect a collection of features in the toolbox. This jcmd will not remove the need for a native debugger. I do think that the JVM should be in charge of decoding its own internals, and the built-in tools that do that should be more accessible. > Could this be an own command, e,g, VM.inspect, and possibly limited to debug > VMs? Yes, could be VM.find or VM.inspect. I liked the grouping into VM.debug, they have some common features: a conceptual group, tools that you might want to call alongside another debugger origins in debug.cpp utils (which are still there) "debug" label may discourage casual users from experimenting require UnlockDiagnosticVMOptions common ..but I'm not emotionally connected to the name or the group. I appreciate the feedback. As noted, there is no rule for jcmd grouping. VM.info yes is an example that doesn't fit one particular category, find/inspect is certainly another. (If you find existing jcmds a mess, feel free to suggest changes as appropriate.) > Do we really need this feature in production? Yes I am saying this should be in all builds. It's not only debug builds that people debug, or reproduce problems with. > Don't we have jhsdb for that? [core file/minidump handling] We have jhsdb and the SA tools in general. These have ongoing maintenance issues, the game of catch up as the VM changes is not solved. We need to have options. I will find a different place to propose that alternative in detail. I'll wait and see if there's any other input on the naming... 8-) - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1980761622
Re: RFR: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature [v5]
> The deprecated Subject Delegation feature in JMX will be removed. > > This was marked in JDK 21 as deprecated for removal (JDK-8298966). Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Test specifically that UOE is thrown. - Changes: - all: https://git.openjdk.org/jdk/pull/18025/files - new: https://git.openjdk.org/jdk/pull/18025/files/19ace691..c5256c3f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18025&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18025&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18025.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18025/head:pull/18025 PR: https://git.openjdk.org/jdk/pull/18025
Re: RFR: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature [v4]
On Wed, 6 Mar 2024 11:42:18 GMT, Kevin Walls wrote: >> The deprecated Subject Delegation feature in JMX will be removed. >> >> This was marked in JDK 21 as deprecated for removal (JDK-8298966). > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Test that SubjectDelegation is refused. Added a test to ensure jmxc.getMBeanServerConnection(delegationSubject) throws UnsupportedOperationException. - PR Comment: https://git.openjdk.org/jdk/pull/18025#issuecomment-1980688614
Re: RFR: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature [v4]
> The deprecated Subject Delegation feature in JMX will be removed. > > This was marked in JDK 21 as deprecated for removal (JDK-8298966). Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Test that SubjectDelegation is refused. - Changes: - all: https://git.openjdk.org/jdk/pull/18025/files - new: https://git.openjdk.org/jdk/pull/18025/files/c607b69a..19ace691 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18025&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18025&range=02-03 Stats: 115 lines in 1 file changed: 115 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18025.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18025/head:pull/18025 PR: https://git.openjdk.org/jdk/pull/18025
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase
On Wed, 6 Mar 2024 09:26:25 GMT, Matthias Baesken wrote: > We define the RESTARTABLE macro again and again at a lot of places in the JDK > native codebase. This could be centralized to avoid repeating it again and > again ! Looks good. Nice refactor. - Marked as reviewed by gli (Committer). PR Review: https://git.openjdk.org/jdk/pull/18132#pullrequestreview-1919422137
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 [v7]
> 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: optimize holding the interruptLock in JavaThread::is_interrupted - Changes: - all: https://git.openjdk.org/jdk/pull/18093/files - new: https://git.openjdk.org/jdk/pull/18093/files/2a483b6f..db3149dc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18093&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18093&range=05-06 Stats: 34 lines in 1 file changed: 26 ins; 5 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
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 [v6]
> 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: reordered JavaThread::is_interrupted code to use lock when needed only - Changes: - all: https://git.openjdk.org/jdk/pull/18093/files - new: https://git.openjdk.org/jdk/pull/18093/files/b99fad54..2a483b6f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18093&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18093&range=04-05 Stats: 12 lines in 1 file changed: 8 ins; 1 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
RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase
We define the RESTARTABLE macro again and again at a lot of places in the JDK native codebase. This could be centralized to avoid repeating it again and again ! - Commit messages: - JDK-8327444 Changes: https://git.openjdk.org/jdk/pull/18132/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18132&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8327444 Stats: 113 lines in 16 files changed: 8 ins; 105 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18132.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18132/head:pull/18132 PR: https://git.openjdk.org/jdk/pull/18132
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
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v3]
On Wed, 6 Mar 2024 06:28:01 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: improved sync in new test InterruptRawMonitor Added `ObjectLocker` of the `interruptLock` for sync to the `JavaThread::is_interrupted()`. - PR Comment: https://git.openjdk.org/jdk/pull/18093#issuecomment-1980391743
Re: RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is [v4]
> 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: added ObjectLocker of interruptLock for sync - Changes: - all: https://git.openjdk.org/jdk/pull/18093/files - new: https://git.openjdk.org/jdk/pull/18093/files/1e72452b..064d8225 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18093&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18093&range=02-03 Stats: 35 lines in 3 files changed: 15 ins; 18 del; 2 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
