Re: RFR: 8287200: Test java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java timed out after JDK-8287103
On Wed, 25 May 2022 23:04:50 GMT, Leonid Mesnik wrote: >> test/jdk/java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java line >> 100: >> >>> 98: while (thread.getState() != Thread.State.BLOCKED) { >>> 99: Thread.sleep(10); >>> 100: if (thread.getState() == Thread.State.TERMINATED) { >> >> Thread.State.TERMINATED == thread.getState(). Does it make sense? > > Not sure. What is the goal? I assume this test is catch the catch where the thread terminates without blocking. Using isAlive might be cleaner for people to understand. - PR: https://git.openjdk.java.net/jdk/pull/8874
Re: RFR: 8287135: Calculation of jmm_GetMemoryUsage is wrong
On Mon, 23 May 2022 07:28:41 GMT, Yi Yang wrote: > It seems that calculation of > MemoryMXBean.getNonHeapMemoryUsage(jmm_GetMemoryUsage) is wrong. > > Currently, > `NonHeapUsage=CodeCache+Metaspace(ClassTypeSpace+NonClassTypeSpace)+CompressedClassSpace(ClassTypeSpace)` > > ==> CodeHeap 'non-nmethods' 1532544 (Used) > ==> CodeHeap 'profiled nmethods' 0 > ==> CodeHeap 'non-profiled nmethods' 13952 > ==> Metaspace 506696 > ==> Compressed Class Space 43312 > init = 7667712(7488K) used = 2096504(2047K) committed = 8454144(8256K) max = > -1(-1K) > > In this way, getNonHeapMemoryUsage is larger than it ought to be, it should > be `NonHeapUsage = CodeCache + Metaspace`. src/hotspot/share/services/management.cpp line 753: > 751: for (int i = 0; i < MemoryService::num_memory_pools(); i++) { > 752: MemoryPool* pool = MemoryService::get_memory_pool(i); > 753: if (pool->is_codeheap() || pool->is_metaspace()) { Our only special case is that all the memory reported by `CompressedKlassSpacePool` are already reported by `MetaspacePool`, so shouldn't we do this: if (pool->is_non_heap() && !pool->is_compressed_klass_space()) { // skip CompressedKlassSpacePool since its memory is already reported by // MetaspacePool - PR: https://git.openjdk.java.net/jdk/pull/8831
Re: RFR: 8286983: Add mention of "Preview Feature" for new loom related jdb and debug agent options [v2]
On Thu, 26 May 2022 02:35:08 GMT, David Holmes wrote: >>> If virtual threads become permanent then the usage message would minimally >>> be updated to drop the sentence that virtual threads are a preview feature. >> >> But that's precisely the commitment to this flag that I want to avoid. I >> want something that is the equivalent of `EXPERIMENTAL` for `-XX` flags. > > Again I have to say I agree with Chris. The flag itself is a "preview" flag > and relates to a "preview feature". If the feature goes obviously the flag > does too, but the flag could go even if the feature stays. Please see the discussion with Alex in [JDK-8286983](https://bugs.openjdk.java.net/browse/JDK-8286983) - PR: https://git.openjdk.java.net/jdk/pull/8780
Re: RFR: 8286983: Add mention of "Preview Feature" for new loom related jdb and debug agent options [v2]
On Wed, 25 May 2022 16:11:52 GMT, Chris Plummer wrote: >> If virtual threads become permanent then the usage message would minimally >> be updated to drop the sentence that virtual threads are a preview feature. >> At that point the debugger APIs may have been built out further and it might >> be that there is no need for the -trackvthreads option but that's too far >> out to know at this point. >> >> If virtual threads were to be removed then the -trackvthreads command line >> option would be removed too. > >> If virtual threads become permanent then the usage message would minimally >> be updated to drop the sentence that virtual threads are a preview feature. > > But that's precisely the commitment to this flag that I want to avoid. I > want something that is the equivalent of `EXPERIMENTAL` for `-XX` flags. Again I have to say I agree with Chris. The flag itself is a "preview" flag and relates to a "preview feature". If the feature goes obviously the flag does too, but the flag could go even if the feature stays. - PR: https://git.openjdk.java.net/jdk/pull/8780
Re: RFR: 8286960: Test serviceability/jvmti/vthread/SuspendResume2 crashed: missing ThreadsListHandle in calling context
On Wed, 25 May 2022 07:23:36 GMT, Serguei Spitsyn wrote: > A part of this issue was contributed with the following changeset: > > commit ea23e7333e03abb4aca3e9f3854bab418a4b70e2 > Author: Daniel D. Daugherty <[dcu...@openjdk.org](mailto:dcu...@openjdk.org)> > Date: Mon Nov 8 14:45:04 2021 + > > 8249004: Reduce ThreadsListHandle overhead in relation to direct > handshakes > Reviewed-by: coleenp, sspitsyn, dholmes, rehn > > The following change in `src/hotspot/share/runtime/thread.cpp` added new > assert: > > bool JavaThread::java_suspend() { > - ThreadsListHandle tlh; > - if (!tlh.includes(this)) { > - log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on > ThreadsList, no suspension", p2i(this)); > - return false; > - } > + guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */ true), > + "missing ThreadsListHandle in calling context."); > return this->handshake_state()->suspend(); > } > > This new assert misses a check for target thread as being current > `JavaThread`. > > Also, the JVMTI SuspendThread is protected with TLH: > > JvmtiEnv::SuspendThread(jthread thread) { > JavaThread* current = JavaThread::current(); > ThreadsListHandle tlh(current); <= TLS defined here!!! > >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > > > However, it is possible that a new carrier thread (and an associated > `JavaThread`) can be created after the `TLH` was set and the target virtual > thread can be mounted on new carrier thread. Then target virtual thread will > be associated with newly created `JavaThread` which is unprotected by the TLH. > The right way to be protected from this situation it is to prevent mount > state transitions with `JvmtiVTMSTransitionDisabler` before the TLH is set as > in the change below: > > > @@ -929,13 +929,13 @@ JvmtiEnv::GetAllThreads(jint* threads_count_ptr, > jthread** threads_ptr) { > jvmtiError > JvmtiEnv::SuspendThread(jthread thread) { >JavaThread* current = JavaThread::current(); > - ThreadsListHandle tlh(current); > >jvmtiError err; >JavaThread* java_thread = NULL; >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > +ThreadsListHandle tlh(current); > > err = get_threadOop_and_JavaThread(tlh.list(), thread, _thread, > _oop); > if (err != JVMTI_ERROR_NONE) { > > > > This problem exist in all JVMTI Suspend functions: > `SuspendThread`, `SuspendThreadList` and `SuspendAllVirtualThreads`. I was expecting there to be an existing non-vthread-related test for self-suspension. That seems to be a hole in our test coverage. - PR: https://git.openjdk.java.net/jdk/pull/8878
Re: How can Java developers best understand their impact on the start-up of the JVM?
cc'ing hotspot-jfr-dev On 26/05/2022 9:45 am, Mat Carter wrote: The Microsoft Java Engineering Group (JEG) have been looking at how Java developers can best understand the impact of their decisions on start-up time. Specifically, how developers can identify and mitigate the dominant activities that occur during start-up, through to their app / service reaching a steady state (warmed up). Upon investigation into the available tooling (i.e., jstat, jcmd, jmc, VisualVM, MXBeans), it seems that Performance Counters (PCs) and JDK Flight Recorder events (JFREs) are the main source of the available startup metrics that get surfaced. Whilst the tooling does provide insight to specific areas (GC, JIT Compilation etc.), there does not seem to be one view to help a developer identify the dominant activities that are impacting start-up. Q: Has the above been discussed before and are there any existing plans to address the outcomes that I might have missed? Looking at the specific PCs and JFREs there are a few “missing” metrics, for example a timestamp at the very beginning entry point of the JVM. Also, it appears that the JFREs represent a subset of the PCs - does this indicate that either some PCs should be removed (or only enabled for JVM development) or that there remain JFREs to be added? Q: Is there a current position on whether PCs and JFREs will continue to co-exist (the former supporting JMX MXBeans, et al.), or is there a plan to phase out PCs in favor of JFREs? Q: Assuming PCs should continue to exist, do folks see the value in documenting them and making them readily accessible to the running application? Especially in the deployment scenario where running a second application or tool (to sample the PCs remotely) is not feasible. FYI - during our initial investigations we wrote some scripts to find all possible PCs. We did this because jcmd only lists instantiated PCs and the PC names are procedurally generated, making them non-trivial to search for. Q: Do folks see value in documenting the JFREs, especially since the application can access the JFR Event stream? (There is non-official documentation available [1]) Q: While Project Leyden concerns itself primarily with improving start-up times, are there any other known activities targeting profiling of Java applications (esp. start-up)? Answers and discussion of the above is most welcome so that our group moves forward in helping our customers, whilst staying aligned with and contributing to the OpenJDK community! Thanks in advance Mat Carter [1] https://bestsolution-at.github.io/jfr-doc/index.html
Re: RFR: 8286960: Test serviceability/jvmti/vthread/SuspendResume2 crashed: missing ThreadsListHandle in calling context
On Wed, 25 May 2022 07:23:36 GMT, Serguei Spitsyn wrote: > A part of this issue was contributed with the following changeset: > > commit ea23e7333e03abb4aca3e9f3854bab418a4b70e2 > Author: Daniel D. Daugherty <[dcu...@openjdk.org](mailto:dcu...@openjdk.org)> > Date: Mon Nov 8 14:45:04 2021 + > > 8249004: Reduce ThreadsListHandle overhead in relation to direct > handshakes > Reviewed-by: coleenp, sspitsyn, dholmes, rehn > > The following change in `src/hotspot/share/runtime/thread.cpp` added new > assert: > > bool JavaThread::java_suspend() { > - ThreadsListHandle tlh; > - if (!tlh.includes(this)) { > - log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on > ThreadsList, no suspension", p2i(this)); > - return false; > - } > + guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */ true), > + "missing ThreadsListHandle in calling context."); > return this->handshake_state()->suspend(); > } > > This new assert misses a check for target thread as being current > `JavaThread`. > > Also, the JVMTI SuspendThread is protected with TLH: > > JvmtiEnv::SuspendThread(jthread thread) { > JavaThread* current = JavaThread::current(); > ThreadsListHandle tlh(current); <= TLS defined here!!! > >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > > > However, it is possible that a new carrier thread (and an associated > `JavaThread`) can be created after the `TLH` was set and the target virtual > thread can be mounted on new carrier thread. Then target virtual thread will > be associated with newly created `JavaThread` which is unprotected by the TLH. > The right way to be protected from this situation it is to prevent mount > state transitions with `JvmtiVTMSTransitionDisabler` before the TLH is set as > in the change below: > > > @@ -929,13 +929,13 @@ JvmtiEnv::GetAllThreads(jint* threads_count_ptr, > jthread** threads_ptr) { > jvmtiError > JvmtiEnv::SuspendThread(jthread thread) { >JavaThread* current = JavaThread::current(); > - ThreadsListHandle tlh(current); > >jvmtiError err; >JavaThread* java_thread = NULL; >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > +ThreadsListHandle tlh(current); > > err = get_threadOop_and_JavaThread(tlh.list(), thread, _thread, > _oop); > if (err != JVMTI_ERROR_NONE) { > > > > This problem exist in all JVMTI Suspend functions: > `SuspendThread`, `SuspendThreadList` and `SuspendAllVirtualThreads`. Thank you for review, Alex! - PR: https://git.openjdk.java.net/jdk/pull/8878
Integrated: 8286960: Test serviceability/jvmti/vthread/SuspendResume2 crashed: missing ThreadsListHandle in calling context
On Wed, 25 May 2022 07:23:36 GMT, Serguei Spitsyn wrote: > A part of this issue was contributed with the following changeset: > > commit ea23e7333e03abb4aca3e9f3854bab418a4b70e2 > Author: Daniel D. Daugherty <[dcu...@openjdk.org](mailto:dcu...@openjdk.org)> > Date: Mon Nov 8 14:45:04 2021 + > > 8249004: Reduce ThreadsListHandle overhead in relation to direct > handshakes > Reviewed-by: coleenp, sspitsyn, dholmes, rehn > > The following change in `src/hotspot/share/runtime/thread.cpp` added new > assert: > > bool JavaThread::java_suspend() { > - ThreadsListHandle tlh; > - if (!tlh.includes(this)) { > - log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on > ThreadsList, no suspension", p2i(this)); > - return false; > - } > + guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */ true), > + "missing ThreadsListHandle in calling context."); > return this->handshake_state()->suspend(); > } > > This new assert misses a check for target thread as being current > `JavaThread`. > > Also, the JVMTI SuspendThread is protected with TLH: > > JvmtiEnv::SuspendThread(jthread thread) { > JavaThread* current = JavaThread::current(); > ThreadsListHandle tlh(current); <= TLS defined here!!! > >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > > > However, it is possible that a new carrier thread (and an associated > `JavaThread`) can be created after the `TLH` was set and the target virtual > thread can be mounted on new carrier thread. Then target virtual thread will > be associated with newly created `JavaThread` which is unprotected by the TLH. > The right way to be protected from this situation it is to prevent mount > state transitions with `JvmtiVTMSTransitionDisabler` before the TLH is set as > in the change below: > > > @@ -929,13 +929,13 @@ JvmtiEnv::GetAllThreads(jint* threads_count_ptr, > jthread** threads_ptr) { > jvmtiError > JvmtiEnv::SuspendThread(jthread thread) { >JavaThread* current = JavaThread::current(); > - ThreadsListHandle tlh(current); > >jvmtiError err; >JavaThread* java_thread = NULL; >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > +ThreadsListHandle tlh(current); > > err = get_threadOop_and_JavaThread(tlh.list(), thread, _thread, > _oop); > if (err != JVMTI_ERROR_NONE) { > > > > This problem exist in all JVMTI Suspend functions: > `SuspendThread`, `SuspendThreadList` and `SuspendAllVirtualThreads`. This pull request has now been integrated. Changeset: 94811c0d Author:Serguei Spitsyn URL: https://git.openjdk.java.net/jdk/commit/94811c0dc7c20b0e7cb2649fe8da5061ce3d6246 Stats: 17 lines in 2 files changed: 8 ins; 7 del; 2 mod 8286960: Test serviceability/jvmti/vthread/SuspendResume2 crashed: missing ThreadsListHandle in calling context Reviewed-by: dholmes, pchilanomate, amenkov - PR: https://git.openjdk.java.net/jdk/pull/8878
How can Java developers best understand their impact on the start-up of the JVM?
The Microsoft Java Engineering Group (JEG) have been looking at how Java developers can best understand the impact of their decisions on start-up time. Specifically, how developers can identify and mitigate the dominant activities that occur during start-up, through to their app / service reaching a steady state (warmed up). Upon investigation into the available tooling (i.e., jstat, jcmd, jmc, VisualVM, MXBeans), it seems that Performance Counters (PCs) and JDK Flight Recorder events (JFREs) are the main source of the available startup metrics that get surfaced. Whilst the tooling does provide insight to specific areas (GC, JIT Compilation etc.), there does not seem to be one view to help a developer identify the dominant activities that are impacting start-up. Q: Has the above been discussed before and are there any existing plans to address the outcomes that I might have missed? Looking at the specific PCs and JFREs there are a few “missing” metrics, for example a timestamp at the very beginning entry point of the JVM. Also, it appears that the JFREs represent a subset of the PCs - does this indicate that either some PCs should be removed (or only enabled for JVM development) or that there remain JFREs to be added? Q: Is there a current position on whether PCs and JFREs will continue to co-exist (the former supporting JMX MXBeans, et al.), or is there a plan to phase out PCs in favor of JFREs? Q: Assuming PCs should continue to exist, do folks see the value in documenting them and making them readily accessible to the running application? Especially in the deployment scenario where running a second application or tool (to sample the PCs remotely) is not feasible. FYI - during our initial investigations we wrote some scripts to find all possible PCs. We did this because jcmd only lists instantiated PCs and the PC names are procedurally generated, making them non-trivial to search for. Q: Do folks see value in documenting the JFREs, especially since the application can access the JFR Event stream? (There is non-official documentation available [1]) Q: While Project Leyden concerns itself primarily with improving start-up times, are there any other known activities targeting profiling of Java applications (esp. start-up)? Answers and discussion of the above is most welcome so that our group moves forward in helping our customers, whilst staying aligned with and contributing to the OpenJDK community! Thanks in advance Mat Carter [1] https://bestsolution-at.github.io/jfr-doc/index.html
Re: RFR: 8286960: Test serviceability/jvmti/vthread/SuspendResume2 crashed: missing ThreadsListHandle in calling context
On Wed, 25 May 2022 07:23:36 GMT, Serguei Spitsyn wrote: > A part of this issue was contributed with the following changeset: > > commit ea23e7333e03abb4aca3e9f3854bab418a4b70e2 > Author: Daniel D. Daugherty <[dcu...@openjdk.org](mailto:dcu...@openjdk.org)> > Date: Mon Nov 8 14:45:04 2021 + > > 8249004: Reduce ThreadsListHandle overhead in relation to direct > handshakes > Reviewed-by: coleenp, sspitsyn, dholmes, rehn > > The following change in `src/hotspot/share/runtime/thread.cpp` added new > assert: > > bool JavaThread::java_suspend() { > - ThreadsListHandle tlh; > - if (!tlh.includes(this)) { > - log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on > ThreadsList, no suspension", p2i(this)); > - return false; > - } > + guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */ true), > + "missing ThreadsListHandle in calling context."); > return this->handshake_state()->suspend(); > } > > This new assert misses a check for target thread as being current > `JavaThread`. > > Also, the JVMTI SuspendThread is protected with TLH: > > JvmtiEnv::SuspendThread(jthread thread) { > JavaThread* current = JavaThread::current(); > ThreadsListHandle tlh(current); <= TLS defined here!!! > >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > > > However, it is possible that a new carrier thread (and an associated > `JavaThread`) can be created after the `TLH` was set and the target virtual > thread can be mounted on new carrier thread. Then target virtual thread will > be associated with newly created `JavaThread` which is unprotected by the TLH. > The right way to be protected from this situation it is to prevent mount > state transitions with `JvmtiVTMSTransitionDisabler` before the TLH is set as > in the change below: > > > @@ -929,13 +929,13 @@ JvmtiEnv::GetAllThreads(jint* threads_count_ptr, > jthread** threads_ptr) { > jvmtiError > JvmtiEnv::SuspendThread(jthread thread) { >JavaThread* current = JavaThread::current(); > - ThreadsListHandle tlh(current); > >jvmtiError err; >JavaThread* java_thread = NULL; >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > +ThreadsListHandle tlh(current); > > err = get_threadOop_and_JavaThread(tlh.list(), thread, _thread, > _oop); > if (err != JVMTI_ERROR_NONE) { > > > > This problem exist in all JVMTI Suspend functions: > `SuspendThread`, `SuspendThreadList` and `SuspendAllVirtualThreads`. Marked as reviewed by amenkov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8878
Re: RFR: 8287200: Test java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java timed out after JDK-8287103
On Wed, 25 May 2022 21:18:24 GMT, master-code-java wrote: >> Need to use proper synchronization. >> >> The CyclicBarriers might move the thread to WAITING state but not BLOCKED. >> So it should not confuse existing checks. > > test/jdk/java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java line > 98: > >> 96: >> 97: private static void awaitBlocked(Thread thread) throws >> InterruptedException { >> 98: while (thread.getState() != Thread.State.BLOCKED) { > > Thread.State.BLOCKED == thread.getState(). Does it make sense? Do you mean '==' or '!='? Both don't make sense to me, honestly. Could you please elaborate? > test/jdk/java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java line > 100: > >> 98: while (thread.getState() != Thread.State.BLOCKED) { >> 99: Thread.sleep(10); >> 100: if (thread.getState() == Thread.State.TERMINATED) { > > Thread.State.TERMINATED == thread.getState(). Does it make sense? Not sure. What is the goal? - PR: https://git.openjdk.java.net/jdk/pull/8874
Re: RFR: 8287200: Test java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java timed out after JDK-8287103
On Tue, 24 May 2022 19:52:57 GMT, Leonid Mesnik wrote: > Need to use proper synchronization. > > The CyclicBarriers might move the thread to WAITING state but not BLOCKED. So > it should not confuse existing checks. test/jdk/java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java line 98: > 96: > 97: private static void awaitBlocked(Thread thread) throws > InterruptedException { > 98: while (thread.getState() != Thread.State.BLOCKED) { Thread.State.BLOCKED == thread.getState(). Does it make sense? test/jdk/java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java line 100: > 98: while (thread.getState() != Thread.State.BLOCKED) { > 99: Thread.sleep(10); > 100: if (thread.getState() == Thread.State.TERMINATED) { Thread.State.TERMINATED == thread.getState(). Does it make sense? - PR: https://git.openjdk.java.net/jdk/pull/8874
Re: RFR: 8286960: Test serviceability/jvmti/vthread/SuspendResume2 crashed: missing ThreadsListHandle in calling context
On Wed, 25 May 2022 07:23:36 GMT, Serguei Spitsyn wrote: > A part of this issue was contributed with the following changeset: > > commit ea23e7333e03abb4aca3e9f3854bab418a4b70e2 > Author: Daniel D. Daugherty <[dcu...@openjdk.org](mailto:dcu...@openjdk.org)> > Date: Mon Nov 8 14:45:04 2021 + > > 8249004: Reduce ThreadsListHandle overhead in relation to direct > handshakes > Reviewed-by: coleenp, sspitsyn, dholmes, rehn > > The following change in `src/hotspot/share/runtime/thread.cpp` added new > assert: > > bool JavaThread::java_suspend() { > - ThreadsListHandle tlh; > - if (!tlh.includes(this)) { > - log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on > ThreadsList, no suspension", p2i(this)); > - return false; > - } > + guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */ true), > + "missing ThreadsListHandle in calling context."); > return this->handshake_state()->suspend(); > } > > This new assert misses a check for target thread as being current > `JavaThread`. > > Also, the JVMTI SuspendThread is protected with TLH: > > JvmtiEnv::SuspendThread(jthread thread) { > JavaThread* current = JavaThread::current(); > ThreadsListHandle tlh(current); <= TLS defined here!!! > >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > > > However, it is possible that a new carrier thread (and an associated > `JavaThread`) can be created after the `TLH` was set and the target virtual > thread can be mounted on new carrier thread. Then target virtual thread will > be associated with newly created `JavaThread` which is unprotected by the TLH. > The right way to be protected from this situation it is to prevent mount > state transitions with `JvmtiVTMSTransitionDisabler` before the TLH is set as > in the change below: > > > @@ -929,13 +929,13 @@ JvmtiEnv::GetAllThreads(jint* threads_count_ptr, > jthread** threads_ptr) { > jvmtiError > JvmtiEnv::SuspendThread(jthread thread) { >JavaThread* current = JavaThread::current(); > - ThreadsListHandle tlh(current); > >jvmtiError err; >JavaThread* java_thread = NULL; >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > +ThreadsListHandle tlh(current); > > err = get_threadOop_and_JavaThread(tlh.list(), thread, _thread, > _oop); > if (err != JVMTI_ERROR_NONE) { > > > > This problem exist in all JVMTI Suspend functions: > `SuspendThread`, `SuspendThreadList` and `SuspendAllVirtualThreads`. Thank you for quick review, Patricio! - PR: https://git.openjdk.java.net/jdk/pull/8878
Re: RFR: 8286960: Test serviceability/jvmti/vthread/SuspendResume2 crashed: missing ThreadsListHandle in calling context
On Wed, 25 May 2022 07:23:36 GMT, Serguei Spitsyn wrote: > A part of this issue was contributed with the following changeset: > > commit ea23e7333e03abb4aca3e9f3854bab418a4b70e2 > Author: Daniel D. Daugherty <[dcu...@openjdk.org](mailto:dcu...@openjdk.org)> > Date: Mon Nov 8 14:45:04 2021 + > > 8249004: Reduce ThreadsListHandle overhead in relation to direct > handshakes > Reviewed-by: coleenp, sspitsyn, dholmes, rehn > > The following change in `src/hotspot/share/runtime/thread.cpp` added new > assert: > > bool JavaThread::java_suspend() { > - ThreadsListHandle tlh; > - if (!tlh.includes(this)) { > - log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on > ThreadsList, no suspension", p2i(this)); > - return false; > - } > + guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */ true), > + "missing ThreadsListHandle in calling context."); > return this->handshake_state()->suspend(); > } > > This new assert misses a check for target thread as being current > `JavaThread`. > > Also, the JVMTI SuspendThread is protected with TLH: > > JvmtiEnv::SuspendThread(jthread thread) { > JavaThread* current = JavaThread::current(); > ThreadsListHandle tlh(current); <= TLS defined here!!! > >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > > > However, it is possible that a new carrier thread (and an associated > `JavaThread`) can be created after the `TLH` was set and the target virtual > thread can be mounted on new carrier thread. Then target virtual thread will > be associated with newly created `JavaThread` which is unprotected by the TLH. > The right way to be protected from this situation it is to prevent mount > state transitions with `JvmtiVTMSTransitionDisabler` before the TLH is set as > in the change below: > > > @@ -929,13 +929,13 @@ JvmtiEnv::GetAllThreads(jint* threads_count_ptr, > jthread** threads_ptr) { > jvmtiError > JvmtiEnv::SuspendThread(jthread thread) { >JavaThread* current = JavaThread::current(); > - ThreadsListHandle tlh(current); > >jvmtiError err; >JavaThread* java_thread = NULL; >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > +ThreadsListHandle tlh(current); > > err = get_threadOop_and_JavaThread(tlh.list(), thread, _thread, > _oop); > if (err != JVMTI_ERROR_NONE) { > > > > This problem exist in all JVMTI Suspend functions: > `SuspendThread`, `SuspendThreadList` and `SuspendAllVirtualThreads`. LGTM. - Marked as reviewed by pchilanomate (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8878
Re: RFR: 8286960: Test serviceability/jvmti/vthread/SuspendResume2 crashed: missing ThreadsListHandle in calling context
On Wed, 25 May 2022 07:23:36 GMT, Serguei Spitsyn wrote: > A part of this issue was contributed with the following changeset: > > commit ea23e7333e03abb4aca3e9f3854bab418a4b70e2 > Author: Daniel D. Daugherty <[dcu...@openjdk.org](mailto:dcu...@openjdk.org)> > Date: Mon Nov 8 14:45:04 2021 + > > 8249004: Reduce ThreadsListHandle overhead in relation to direct > handshakes > Reviewed-by: coleenp, sspitsyn, dholmes, rehn > > The following change in `src/hotspot/share/runtime/thread.cpp` added new > assert: > > bool JavaThread::java_suspend() { > - ThreadsListHandle tlh; > - if (!tlh.includes(this)) { > - log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on > ThreadsList, no suspension", p2i(this)); > - return false; > - } > + guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */ true), > + "missing ThreadsListHandle in calling context."); > return this->handshake_state()->suspend(); > } > > This new assert misses a check for target thread as being current > `JavaThread`. > > Also, the JVMTI SuspendThread is protected with TLH: > > JvmtiEnv::SuspendThread(jthread thread) { > JavaThread* current = JavaThread::current(); > ThreadsListHandle tlh(current); <= TLS defined here!!! > >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > > > However, it is possible that a new carrier thread (and an associated > `JavaThread`) can be created after the `TLH` was set and the target virtual > thread can be mounted on new carrier thread. Then target virtual thread will > be associated with newly created `JavaThread` which is unprotected by the TLH. > The right way to be protected from this situation it is to prevent mount > state transitions with `JvmtiVTMSTransitionDisabler` before the TLH is set as > in the change below: > > > @@ -929,13 +929,13 @@ JvmtiEnv::GetAllThreads(jint* threads_count_ptr, > jthread** threads_ptr) { > jvmtiError > JvmtiEnv::SuspendThread(jthread thread) { >JavaThread* current = JavaThread::current(); > - ThreadsListHandle tlh(current); > >jvmtiError err; >JavaThread* java_thread = NULL; >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > +ThreadsListHandle tlh(current); > > err = get_threadOop_and_JavaThread(tlh.list(), thread, _thread, > _oop); > if (err != JVMTI_ERROR_NONE) { > > > > This problem exist in all JVMTI Suspend functions: > `SuspendThread`, `SuspendThreadList` and `SuspendAllVirtualThreads`. The test `serviceability/jvmti/vthread/SelfSuspendDisablerTest` is triggering this guarantee. But it needs this update in the `SuspendThread` in order to do it. # Internal Error (/scratch/sspitsyn/loom5/open/src/hotspot/share/runtime/thread.cpp:1781), pid=31157, tid=31182 # guarantee(Thread::is_JavaThread_protected_by_TLH( this)) failed: missing ThreadsListHandle in calling context. # # JRE version: Java(TM) SE Runtime Environment (19.0) (fastdebug build 19-internal-2022-05-19-0744187.sspitsyn...) # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 19-internal-2022-05-19-0744187.sspitsyn..., mixed mode, sharing, tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64) # Problematic frame: # V [libjvm.so+0x1a41724] JavaThread::java_suspend()+0xa4 . . . . . --- S U M M A R Y Command Line: -Dtest.vm.opts=-XX:+CreateCoredumpOnCrash --enable-preview -Djava.util.concurrent.ForkJoinPool.common.parallelism=1 -XX:-VerifyContinuations -XX:+CheckUnhandledOops -Xcheck:jni -Dmain.wrapper=Virtual -Dtest.tool.vm.opts=-J-XX:+CreateCoredumpOnCrash -J--enable-preview -J-Djava.util.concurrent.ForkJoinPool.common.parallelism=1 -J-XX:-VerifyContinuations -J-XX:+CheckUnhandledOops -J-Xcheck:jni -J-Dmain.wrapper=Virtual -Dtest.compiler.opts= -Dtest.java.opts= -Dtest.jdk=/scratch/sspitsyn/loom5/build/linux-x64-debug/images/jdk -Dcompile.jdk=/scratch/sspitsyn/loom5/build/linux-x64-debug/images/jdk -Dtest.timeout.factor=1.0 -Dtest.nativepath=/scratch/sspitsyn/loom5/build/linux-x64-debug/images/test/hotspot/jtreg/native -Dtest.root=/scratch/sspitsyn/loom5/open/test/hotspot/jtreg -Dtest.name=serviceability/jvmti/vthread/SelfSuspendDisablerTest/SelfSuspendDisablerTest.java -Dtest.file=/scratch/sspitsyn/loom5/open/test/hotspot/jtreg/serviceability/jvmti/vthread/SelfSuspendDisablerT est/SelfSuspendDisablerTest.java -Dtest.src=/scratch/sspitsyn/loom5/open/test/hotspot/jtreg/serviceability/jvmti/vthread/SelfSuspendDisablerTest -Dtest.src.path=/scratch/sspitsyn/loom5/open/test/hotspot/jtreg/serviceability/jvmti/vthread/SelfSuspendDisablerTest:/scratch/sspitsyn/loom5/open/test/lib -Dtest.classes=/scratch/sspitsyn/tst/loom5/JTwork/classes/serviceability/jvmti/vthread/SelfSuspendDisablerTest/SelfSuspendDisablerTest.d
Re: RFR: 8286983: Add mention of "Preview Feature" for new loom related jdb and debug agent options [v2]
On Wed, 25 May 2022 06:23:48 GMT, Alan Bateman wrote: > If virtual threads become permanent then the usage message would minimally be > updated to drop the sentence that virtual threads are a preview feature. But that's precisely the commitment to this flag that I want to avoid. I want something that is the equivalent of `EXPERIMENTAL` for `-XX` flags. - PR: https://git.openjdk.java.net/jdk/pull/8780
Re: RFR: 8286960: Test serviceability/jvmti/vthread/SuspendResume2 crashed: missing ThreadsListHandle in calling context
On Wed, 25 May 2022 07:23:36 GMT, Serguei Spitsyn wrote: > A part of this issue was contributed with the following changeset: > > commit ea23e7333e03abb4aca3e9f3854bab418a4b70e2 > Author: Daniel D. Daugherty <[dcu...@openjdk.org](mailto:dcu...@openjdk.org)> > Date: Mon Nov 8 14:45:04 2021 + > > 8249004: Reduce ThreadsListHandle overhead in relation to direct > handshakes > Reviewed-by: coleenp, sspitsyn, dholmes, rehn > > The following change in `src/hotspot/share/runtime/thread.cpp` added new > assert: > > bool JavaThread::java_suspend() { > - ThreadsListHandle tlh; > - if (!tlh.includes(this)) { > - log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on > ThreadsList, no suspension", p2i(this)); > - return false; > - } > + guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */ true), > + "missing ThreadsListHandle in calling context."); > return this->handshake_state()->suspend(); > } > > This new assert misses a check for target thread as being current > `JavaThread`. > > Also, the JVMTI SuspendThread is protected with TLH: > > JvmtiEnv::SuspendThread(jthread thread) { > JavaThread* current = JavaThread::current(); > ThreadsListHandle tlh(current); <= TLS defined here!!! > >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > > > However, it is possible that a new carrier thread (and an associated > `JavaThread`) can be created after the `TLH` was set and the target virtual > thread can be mounted on new carrier thread. Then target virtual thread will > be associated with newly created `JavaThread` which is unprotected by the TLH. > The right way to be protected from this situation it is to prevent mount > state transitions with `JvmtiVTMSTransitionDisabler` before the TLH is set as > in the change below: > > > @@ -929,13 +929,13 @@ JvmtiEnv::GetAllThreads(jint* threads_count_ptr, > jthread** threads_ptr) { > jvmtiError > JvmtiEnv::SuspendThread(jthread thread) { >JavaThread* current = JavaThread::current(); > - ThreadsListHandle tlh(current); > >jvmtiError err; >JavaThread* java_thread = NULL; >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > +ThreadsListHandle tlh(current); > > err = get_threadOop_and_JavaThread(tlh.list(), thread, _thread, > _oop); > if (err != JVMTI_ERROR_NONE) { > > > > This problem exist in all JVMTI Suspend functions: > `SuspendThread`, `SuspendThreadList` and `SuspendAllVirtualThreads`. One query though - don't we have a self-suspension test that would have triggered the guarantee failure? If we don't then we should (obviously the resume will be a bit racy). - PR: https://git.openjdk.java.net/jdk/pull/8878
Re: RFR: 8286960: Test serviceability/jvmti/vthread/SuspendResume2 crashed: missing ThreadsListHandle in calling context
On Wed, 25 May 2022 07:23:36 GMT, Serguei Spitsyn wrote: > A part of this issue was contributed with the following changeset: > > commit ea23e7333e03abb4aca3e9f3854bab418a4b70e2 > Author: Daniel D. Daugherty <[dcu...@openjdk.org](mailto:dcu...@openjdk.org)> > Date: Mon Nov 8 14:45:04 2021 + > > 8249004: Reduce ThreadsListHandle overhead in relation to direct > handshakes > Reviewed-by: coleenp, sspitsyn, dholmes, rehn > > The following change in `src/hotspot/share/runtime/thread.cpp` added new > assert: > > bool JavaThread::java_suspend() { > - ThreadsListHandle tlh; > - if (!tlh.includes(this)) { > - log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on > ThreadsList, no suspension", p2i(this)); > - return false; > - } > + guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */ true), > + "missing ThreadsListHandle in calling context."); > return this->handshake_state()->suspend(); > } > > This new assert misses a check for target thread as being current > `JavaThread`. > > Also, the JVMTI SuspendThread is protected with TLH: > > JvmtiEnv::SuspendThread(jthread thread) { > JavaThread* current = JavaThread::current(); > ThreadsListHandle tlh(current); <= TLS defined here!!! > >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > > > However, it is possible that a new carrier thread (and an associated > `JavaThread`) can be created after the `TLH` was set and the target virtual > thread can be mounted on new carrier thread. Then target virtual thread will > be associated with newly created `JavaThread` which is unprotected by the TLH. > The right way to be protected from this situation it is to prevent mount > state transitions with `JvmtiVTMSTransitionDisabler` before the TLH is set as > in the change below: > > > @@ -929,13 +929,13 @@ JvmtiEnv::GetAllThreads(jint* threads_count_ptr, > jthread** threads_ptr) { > jvmtiError > JvmtiEnv::SuspendThread(jthread thread) { >JavaThread* current = JavaThread::current(); > - ThreadsListHandle tlh(current); > >jvmtiError err; >JavaThread* java_thread = NULL; >oop thread_oop = NULL; >{ > JvmtiVTMSTransitionDisabler disabler(true); > +ThreadsListHandle tlh(current); > > err = get_threadOop_and_JavaThread(tlh.list(), thread, _thread, > _oop); > if (err != JVMTI_ERROR_NONE) { > > > > This problem exist in all JVMTI Suspend functions: > `SuspendThread`, `SuspendThreadList` and `SuspendAllVirtualThreads`. These changes look good to me - as per our discussion in JBS issue. Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8878
Integrated: 8287254: Clean up Xcode sysroot logic
On Tue, 24 May 2022 17:09:10 GMT, Magnus Ihse Bursie wrote: > The logic in BASIC_SETUP_DEVKIT for setting a correct sysroot for Xcode is > hard to follow. This should be straightened out. We also expose variables > that are no longer used. So there's a bit of related cleanup. > > The new code is more or less functionally equivalent to the old. There were > some corner cases which the old code did not handle well; this has now been > improved. I've also added yet another method of trying to get the SDK root > before falling back to the system library, by using `xcrun --show-sdk-path`. > > In an ideal world, the sysroot flags to `mig` should be set in configure, > e.g. as `MIG_FLAGS` or `MIG_SYSROOT_FLAGS`. I can't be bothered to do that > for a single call to `mig`, though. > > As always, changes like this that depend on the environment is tricky to > test. I've tried running it on a couple of different macs, with and without a > devkit. This pull request has now been integrated. Changeset: d889319a Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/d889319a86101e944aefd4ad7f300505abbe5b30 Stats: 200 lines in 4 files changed: 91 ins; 98 del; 11 mod 8287254: Clean up Xcode sysroot logic Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/8872
RFR: 8286960: Test serviceability/jvmti/vthread/SuspendResume2 crashed: missing ThreadsListHandle in calling context
A part of this issue was contributed with the following changeset: commit ea23e7333e03abb4aca3e9f3854bab418a4b70e2 Author: Daniel D. Daugherty <[dcu...@openjdk.org](mailto:dcu...@openjdk.org)> Date: Mon Nov 8 14:45:04 2021 + 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes Reviewed-by: coleenp, sspitsyn, dholmes, rehn The following change in `src/hotspot/share/runtime/thread.cpp` added new assert: bool JavaThread::java_suspend() { - ThreadsListHandle tlh; - if (!tlh.includes(this)) { - log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on ThreadsList, no suspension", p2i(this)); - return false; - } + guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */ true), + "missing ThreadsListHandle in calling context."); return this->handshake_state()->suspend(); } This new assert misses a check for target thread as being current `JavaThread`. Also, the JVMTI SuspendThread is protected with TLH: JvmtiEnv::SuspendThread(jthread thread) { JavaThread* current = JavaThread::current(); ThreadsListHandle tlh(current); <= TLS defined here!!! oop thread_oop = NULL; { JvmtiVTMSTransitionDisabler disabler(true); However, it is possible that a new carrier thread (and an associated `JavaThread`) can be created after the `TLH` was set and the target virtual thread can be mounted on new carrier thread. Then target virtual thread will be associated with newly created `JavaThread` which is unprotected by the TLH. The right way to be protected from this situation it is to prevent mount state transitions with `JvmtiVTMSTransitionDisabler` before the TLH is set as in the change below: @@ -929,13 +929,13 @@ JvmtiEnv::GetAllThreads(jint* threads_count_ptr, jthread** threads_ptr) { jvmtiError JvmtiEnv::SuspendThread(jthread thread) { JavaThread* current = JavaThread::current(); - ThreadsListHandle tlh(current); jvmtiError err; JavaThread* java_thread = NULL; oop thread_oop = NULL; { JvmtiVTMSTransitionDisabler disabler(true); +ThreadsListHandle tlh(current); err = get_threadOop_and_JavaThread(tlh.list(), thread, _thread, _oop); if (err != JVMTI_ERROR_NONE) { This problem exist in all JVMTI Suspend functions: `SuspendThread`, `SuspendThreadList` and `SuspendAllVirtualThreads`. - Commit messages: - 8286960: Test serviceability/jvmti/vthread/SuspendResume2 crashed: missing ThreadsListHandle in calling context Changes: https://git.openjdk.java.net/jdk/pull/8878/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8878=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286960 Stats: 17 lines in 2 files changed: 8 ins; 7 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8878.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8878/head:pull/8878 PR: https://git.openjdk.java.net/jdk/pull/8878
Re: RFR: 8286983: Add mention of "Preview Feature" for new loom related jdb and debug agent options [v2]
On Tue, 24 May 2022 19:36:02 GMT, Chris Plummer wrote: >> I don't think it is correct to say that this command line option is a >> preview feature. Maybe it can be clarified on a second line: >> >> >> -trackvthreadstrack virtual threads as they are created >> Virtual threads are a preview feature of the >> Java platform. > > Does that imply that `-trackvthreads` can be changed or removed? If virtual threads become permanent then the usage message would minimally be updated to drop the sentence that virtual threads are a preview feature. At that point the debugger APIs may have been built out further and it might be that there is no need for the -trackvthreads option but that's too far out to know at this point. If virtual threads were to be removed then the -trackvthreads command line option would be removed too. - PR: https://git.openjdk.java.net/jdk/pull/8780
Integrated: 8286490: JvmtiEventControllerPrivate::set_event_callbacks CLEARING_MASK computation is incorrect
On Tue, 24 May 2022 05:45:17 GMT, Serguei Spitsyn wrote: > It was a typo when the original CLEARING_MASK was initially introduced: > > > // Mask to clear normal event bits. > const jlong CLEARING_MASK = (1L >> (TOTAL_MIN_EVENT_TYPE_VAL - > TOTAL_MIN_EVENT_TYPE_VAL)) - 1L; > // Avoid cleaning extension event bits. > jlong enabled_bits = CLEARING_MASK & > env->env_event_enable()->_event_callback_enabled.get_bits(); > > ``` > > The first TOTAL_MIN_EVENT_TYPE_VAL has to be JVMTI_MIN_EVENT_TYPE_VAL as > below: > ` const jlong CLEARING_MASK = (1L >> (JVMTI_MIN_EVENT_TYPE_VAL - > TOTAL_MIN_EVENT_TYPE_VAL)) - 1L;` > > Let me provide some background to understand how this mask is used. > > Normal JVMTI event types are numbered from 50 to 88. There are two constants: > > JVMTI_MIN_EVENT_TYPE_VAL = 50 > JVMTI_MAX_EVENT_TYPE_VAL = 88 > > The extension event types are numbered from 47 to 49. There are also two > constants: > > EXT_MIN_EVENT_TYPE_VAL = 47 > EXT_MAX_EVENT_TYPE_VAL = 49 > > > There are also two additional constants: > > TOTAL_MIN_EVENT_TYPE_VAL = 47 > TOTAL_MAX_EVENT_TYPE_VAL = 88 > > > The `enabled_bits` are shifted left by 47, so that the 0-bit in > `enabled_bits` is for first extension event type. And the first normal JVMTI > event type is numbered 3 in the `enabled_bits` bit mask. The `CLEARING_MASK` > is used to clear only normal JVMTI event types but keep the extension event > bits (0-2) unchanged. > > Testing: > I'll run all JVMTI tests including nsk.jvmti tests and serviceability/jvmti > tests including those added in Loom for virtual thread coverage. This pull request has now been integrated. Changeset: a0cccb54 Author:Serguei Spitsyn URL: https://git.openjdk.java.net/jdk/commit/a0cccb54791d954bf08da5aac9b9794e370617c8 Stats: 40 lines in 3 files changed: 35 ins; 4 del; 1 mod 8286490: JvmtiEventControllerPrivate::set_event_callbacks CLEARING_MASK computation is incorrect Reviewed-by: jbachorik, lmesnik, amenkov - PR: https://git.openjdk.java.net/jdk/pull/8860
Re: RFR: 8287200: Test java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java timed out after JDK-8287103
On Tue, 24 May 2022 19:52:57 GMT, Leonid Mesnik wrote: > Need to use proper synchronization. > > The CyclicBarriers might move the thread to WAITING state but not BLOCKED. So > it should not confuse existing checks. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8874