[jdk23] Integrated: 8333931: Problemlist serviceability/jvmti/vthread/CarrierThreadEventNotification
On Tue, 11 Jun 2024 11:34:34 GMT, Serguei Spitsyn wrote: > Please, review a jdk23 backport of the: > [8333931](https://bugs.openjdk.org/browse/JDK-8333931): Problemlist > serviceability/jvmti/vthread/CarrierThreadEventNotification > > Thanks This pull request has now been integrated. Changeset: b17a1c09 Author:Serguei Spitsyn URL: https://git.openjdk.org/jdk/commit/b17a1c092ff082b58d4e9ad64c516a49e4f3adb9 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8333931: Problemlist serviceability/jvmti/vthread/CarrierThreadEventNotification Reviewed-by: cjplummer Backport-of: fe9c63cf73db7833646345e362cbda020ac403d1 - PR: https://git.openjdk.org/jdk/pull/19651
Re: [jdk23] RFR: 8333931: Problemlist serviceability/jvmti/vthread/CarrierThreadEventNotification
On Tue, 11 Jun 2024 11:34:34 GMT, Serguei Spitsyn wrote: > Please, review a jdk23 backport of the: > [8333931](https://bugs.openjdk.org/browse/JDK-8333931): Problemlist > serviceability/jvmti/vthread/CarrierThreadEventNotification > > Thanks Thank you for review, Chris. - PR Comment: https://git.openjdk.org/jdk/pull/19651#issuecomment-2161256191
[jdk23] RFR: 8333931: Problemlist serviceability/jvmti/vthread/CarrierThreadEventNotification
Please, review a jdk23 backport of the: [8333931](https://bugs.openjdk.org/browse/JDK-8333931): Problemlist serviceability/jvmti/vthread/CarrierThreadEventNotification Thanks - Commit messages: - Backport fe9c63cf73db7833646345e362cbda020ac403d1 Changes: https://git.openjdk.org/jdk/pull/19651/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19651=00 Issue: https://bugs.openjdk.org/browse/JDK-8333931 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19651.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19651/head:pull/19651 PR: https://git.openjdk.org/jdk/pull/19651
Re: RFR: 8333841: Add more logging into setfldw001 tests
On Sat, 8 Jun 2024 17:00:04 GMT, Leonid Mesnik wrote: > Tests > SetFieldAccessWatch/setfldw001 > SetFieldModificationWatch/setfmodw001 > intermittently fail with Xcomp. I was unable to reproduce the problem. > The fix adds more checks and variants with jvmti logging. > > The goal is to understand why the test fails. > 1. Confirm that the event is not sent. Currently, the test doesn't differ > between sending "NULL" event and not sending an event at all. > 2. Check if the interpreter-only mode is switched too late in Thread-1. The > jvmti logging shows when field events are enabled and when each thread > actually switches to interpreter-only and enables event handling. > > The plan is to try to reproduce the failure and remove '@test id=logging' > after https://bugs.openjdk.org/browse/JDK-8205957 is fixed. Marked as reviewed by sspitsyn (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19612#pullrequestreview-2109005980
Re: RFR: 8333813: Seviceability tests fail due to stderr containing Temporarily processing option UseNotificationThread
On Fri, 7 Jun 2024 19:56:03 GMT, Chris Plummer wrote: > Allow warning messages such as the following to appear in stderr: > > Java HotSpot(TM) 64-Bit Server VM warning: Temporarily processing option > UseNotificationThread; support is scheduled for removal in 24.0 > > Tested by running CI tier5, which reproduced the issue. Looks good. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19606#pullrequestreview-2105518837
Integrated: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers
On Thu, 16 May 2024 02:37:40 GMT, Serguei Spitsyn wrote: > The following RFE was fixed recently: > [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with > nullptr in JVMTI generated code > > It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI > agents can be developed in C or C++. > This update is to make it clear that `nullptr` is C programming language > `null` pointer. > > I think we do not need a CSR for this fix. > > Testing: N/A (not needed) This pull request has now been integrated. Changeset: 30894126 Author:Serguei Spitsyn URL: https://git.openjdk.org/jdk/commit/30894126a4ba8bc41c333c923ff3007503257688 Stats: 87 lines in 4 files changed: 5 ins; 0 del; 82 mod 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers Reviewed-by: kbarrett, cjplummer - PR: https://git.openjdk.org/jdk/pull/19257
Re: RFR: 8333680: com/sun/tools/attach/BasicTests.java fails with "SocketException: Permission denied: connect"
On Thu, 6 Jun 2024 02:12:11 GMT, Alex Menkov wrote: > The fix updates com/sun/tools/attach/BasicTests.java to listen and connect > using loopback addresses > > Testing: run the test on all Oracle-supported platforms Looks good. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19571#pullrequestreview-2100712779
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v7]
On Wed, 5 Jun 2024 23:57:02 GMT, Serguei Spitsyn wrote: >> The following RFE was fixed recently: >> [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with >> nullptr in JVMTI generated code >> >> It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI >> agents can be developed in C or C++. >> This update is to make it clear that `nullptr` is C programming language >> `null` pointer. >> >> I think we do not need a CSR for this fix. >> >> Testing: N/A (not needed) > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: add a sub-section: Null Pointers Thanks you for review, Kim Chris! Alan, Kim, David and Chris et all, thanks for the discussion and suggestion! - PR Comment: https://git.openjdk.org/jdk/pull/19257#issuecomment-2151151302
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v7]
> The following RFE was fixed recently: > [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with > nullptr in JVMTI generated code > > It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI > agents can be developed in C or C++. > This update is to make it clear that `nullptr` is C programming language > `null` pointer. > > I think we do not need a CSR for this fix. > > Testing: N/A (not needed) Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: add a sub-section: Null Pointers - Changes: - all: https://git.openjdk.org/jdk/pull/19257/files - new: https://git.openjdk.org/jdk/pull/19257/files/6275df3a..16cea131 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19257=06 - incr: https://webrevs.openjdk.org/?repo=jdk=19257=05-06 Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19257.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19257/head:pull/19257 PR: https://git.openjdk.org/jdk/pull/19257
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v5]
On Wed, 5 Jun 2024 22:49:51 GMT, Chris Plummer wrote: >> I'm still not sure this works. It seems kind of muddled. Rather than trying >> to retrofit in the clarifying text, why not start from scratch. That should >> result in better organization and clearer descriptions. For example, I think >> first you should clarify what is meant by a "null pointer". Maybe even make >> that a separate section. I can take a stab at this later today if you want. > > How about undoing the changes in this subsection and then just add the > following as a preceding subsection: > > **Null Pointers** > > Parts of this specification refer to a "null pointer" as a possible function > parameter or return value. A "null pointer" is C `NULL` or C++ `nullptr`. Good suggestion, thanks! Will add it now. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1628565113
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v6]
> The following RFE was fixed recently: > [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with > nullptr in JVMTI generated code > > It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI > agents can be developed in C or C++. > This update is to make it clear that `nullptr` is C programming language > `null` pointer. > > I think we do not need a CSR for this fix. > > Testing: N/A (not needed) Serguei Spitsyn has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits: - Merge - review: consistency and stylistical corrections - review: more null pointer corrections - review: replace nullptr with null pointer in the docs - review: corrected the nullptr clarification - 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers - Changes: https://git.openjdk.org/jdk/pull/19257/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19257=05 Stats: 82 lines in 4 files changed: 0 ins; 0 del; 82 mod Patch: https://git.openjdk.org/jdk/pull/19257.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19257/head:pull/19257 PR: https://git.openjdk.org/jdk/pull/19257
Integrated: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes
On Tue, 28 May 2024 22:24:53 GMT, Serguei Spitsyn wrote: > Please, review the following `interp-only` issue related to carrier threads. > There are 3 problems fixed here: > - The `EnterInterpOnlyModeClosure::do_threads` is taking the > `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect > when we have a deal with a carrier thread. The target state is known at the > point when the `HandshakeClosure` is set, so the fix is to pass it as a > constructor parameter. > - The `state->is_pending_interp_only_mode())` was processed at mounts only > but it has to be processed for unmounts as well. > - The test > `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp` > has a wrong assumption that there can't be `MethodExit` event on the carrier > thread when the function `breakpoint_hit1` is being executed. However, it can > happen if the virtual thread gets unmounted. > > The fix also includes new test case `vthread/CarrierThreadEventNotification` > developed by Patricio. > > Testing: > - Ran new test case locally > - Ran mach5 tiers 1-6 This pull request has now been integrated. Changeset: 60ea17e8 Author:Serguei Spitsyn URL: https://git.openjdk.org/jdk/commit/60ea17e8482936a6acbc442bb1be199e01008072 Stats: 251 lines in 9 files changed: 229 ins; 12 del; 10 mod 8311177: Switching to interpreter only mode in carrier thread can lead to crashes Reviewed-by: pchilanomate, amenkov - PR: https://git.openjdk.org/jdk/pull/19438
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes [v4]
On Mon, 3 Jun 2024 22:55:24 GMT, Serguei Spitsyn wrote: >> Please, review the following `interp-only` issue related to carrier threads. >> There are 3 problems fixed here: >> - The `EnterInterpOnlyModeClosure::do_threads` is taking the >> `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect >> when we have a deal with a carrier thread. The target state is known at the >> point when the `HandshakeClosure` is set, so the fix is to pass it as a >> constructor parameter. >> - The `state->is_pending_interp_only_mode())` was processed at mounts only >> but it has to be processed for unmounts as well. >> - The test >> `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp` >> has a wrong assumption that there can't be `MethodExit` event on the >> carrier thread when the function `breakpoint_hit1` is being executed. >> However, it can happen if the virtual thread gets unmounted. >> >> The fix also includes new test case >> `vthread/CarrierThreadEventNotification` developed by Patricio. >> >> Testing: >> - Ran new test case locally >> - Ran mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: get rid of unneeded casts in new test Thank you for review, Patricio! - PR Comment: https://git.openjdk.org/jdk/pull/19438#issuecomment-2150584829
Re: RFR: 8333178: ubsan: jvmti_tools.cpp:149:16: runtime error: null pointer passed as argument 2, which is declared to never be null
On Wed, 5 Jun 2024 13:04:03 GMT, Matthias Baesken wrote: > With ubsan enabled binaries we run into the following issue in HS :tier4 > tests : > e.g. > vmTestbase/nsk/jvmti/unit/FollowReferences/followref006/TestDescription.jtr > > /jdk/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp:149:16: > runtime error: null pointer passed as argument 2, which is declared to never > be null > #0 0x7fa7a1049482 in add_option > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp:149 > #1 0x7fa7a1049482 in nsk_jvmti_parseOptions > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp:242 > #2 0x7fa7a10634cd in Agent_Initialize > test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/FollowReferences/followref006/followref006.cpp:216 > #3 0x7fa79a9dbb36 in invoke_Agent_OnLoad > src/hotspot/share/prims/jvmtiAgent.cpp:609 > #4 0x7fa79a9dbb36 in JvmtiAgent::load(outputStream*) > src/hotspot/share/prims/jvmtiAgent.cpp:623 > #5 0x7fa79a9defd4 in load_agents > src/hotspot/share/prims/jvmtiAgentList.cpp:179 > #6 0x7fa79a9defd4 in JvmtiAgentList::load_agents() > src/hotspot/share/prims/jvmtiAgentList.cpp:190 > #7 0x7fa79bdad503 in Threads::create_vm(JavaVMInitArgs*, bool*) > src/hotspot/share/runtime/threads.cpp:505 > #8 0x7fa79a6e531f in JNI_CreateJavaVM_inner > src/hotspot/share/prims/jni.cpp:3581 > #9 0x7fa79a6e531f in JNI_CreateJavaVM src/hotspot/share/prims/jni.cpp:3672 > #10 0x7fa7a11277d5 in InitializeJVM > src/java.base/share/native/libjli/java.c:1550 > #11 0x7fa7a11277d5 in JavaMain > src/java.base/share/native/libjli/java.c:491 > #12 0x7fa7a1130f68 in ThreadJavaMain > src/java.base/unix/native/libjli/java_md.c:653 > #13 0x7fa7a10df6e9 in start_thread (/lib64/libpthread.so.0+0xa6e9) > (BuildId: 2f8d3c2d0f4d7888c2598d2ff6356537f5708a73) > #14 0x7fa7a071550e in clone (/lib64/libc.so.6+0x11850e) (BuildId: > f732026552f6adff988b338e92d466bc81a01c37) Marked as reviewed by sspitsyn (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19557#pullrequestreview-2099819278
Re: RFR: 8332785: Replace naked uses of UseSharedSpaces with CDSConfig::is_using_archive [v2]
On Fri, 31 May 2024 15:12:39 GMT, Sonia Zaldana Calles wrote: >> Hi folks, >> >> This PR addresses [8332785](https://bugs.openjdk.org/browse/JDK-8332785) >> replacing all naked uses for ```UseSharedSpaces``` with >> ```CDSConfig::is_using_archive```. >> >> Testing: >> - [x] Tier 1 with GHA. >> >> Thanks, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with one > additional commit since the last revision: > > Updating copyright headers and unnecessary CDSConfig:: Looks good. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19463#pullrequestreview-2098250154
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v6]
On Wed, 5 Jun 2024 01:17:37 GMT, Jaikiran Pai wrote: >> I think `timeout`s are not needed for the refactored tests. >> Per JDK-6528548 the shell action has timed out using a network mounted JDK >> (MakeJAR2.sh run `javac` 3 times and `jar` 1 time). >> But I don't see big problem here - up to the author to keep or remove them. > > Hello Alex, I agree that the timeout may not be needed. I haven't run the > higher tiers in our CI where `-Xcomp` and other options might slow tests down > in general. So, given Serguei's input, I'll let the timeout stay for now and > once we see how this behaves in higher tiers, then we can remove the timeout > in a subsequent PR if necessary. Agreed. Keeping the timeout for now is more safe at this stage of the release cycle. - PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1626809583
Re: RFR: 8333235: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with C1
On Tue, 4 Jun 2024 17:25:04 GMT, Leonid Mesnik wrote: > The kill sends async exception that is thrown somewhere during > log.display(...) method. > The log shows that it is thrown while PrintStream locking moving it into an > inconsistent state. > So the fix is to use some method that could be safely interrupted by async > exception. Looks okay to me. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19547#pullrequestreview-2097695938
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v5]
On Tue, 4 Jun 2024 19:00:32 GMT, Chris Plummer wrote: >> I think this is the right place but it is only for return values. There are >> a few functions where a parameter value can be a null pointer, e.g. in >> GetThreadState, SuspendThread, GetOwnedMonitorInfo the thread parameter can >> be a null pointer to mean the current thread. I don't think the >> introduction section has anywhere right now to reference for parameters that >> can be NULL/nullptr. > > Yes, my point was that this section is only for return values. The section is > titled "Function Return Values". Maybe we should add another short section > just before this one to describe what is meant by "null pointer". Okay, thanks. What about the following: : diff --git a/src/hotspot/share/prims/jvmti.xml b/src/hotspot/share/prims/jvmti.xml index a6ebd0d42c5..a81014c70bb 100644 --- a/src/hotspot/share/prims/jvmti.xml +++ b/src/hotspot/share/prims/jvmti.xml @@ -995,7 +995,10 @@ jvmtiEnv *jvmti; across threads and are created dynamically. - + +There are a few functions where a parameter value can be a null pointer +(C NULL or C++ nullptr), e.g. the thread parameter +can be a null pointer to mean the current thread. functions always return an error code via the function return value. @@ -1004,7 +1007,7 @@ jvmtiEnv *jvmti; In some cases, functions allocate memory that your program must explicitly deallocate. This is indicated in the individual function descriptions. Empty lists, arrays, sequences, etc are -returned as a null pointer (C NULL or C++ nullptr). +returned as a null pointer. In the event that the function encounters an error (any return value other than JVMTI_ERROR_NONE) the values I can try to add a couple of more examples where a null pointer can be passed as a parameter value if it is desirable. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1626756885
Re: RFR: 8333235: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with C1
On Tue, 4 Jun 2024 21:05:40 GMT, Leonid Mesnik wrote:
>> test/hotspot/jtreg/vmTestbase/nsk/jdb/kill/kill001/kill001a.java line 153:
>>
>>> 151: }
>>> 152: }
>>> 153:
>>
>> Have you tried an empty method? I don't think it's a matter of how much time
>> you spend in the method, but just whether or not a JVM async exception
>> polling point is reached. I suspect the method call or return will be a
>> polling point, but I'm unsure what happens if the method call is elided by
>> the JIT because it does nothing.
>
> The empty method is removed. So test failing with '-Xcomp /C2' and exception
> happens after try block.
>
> The log shows:
> reply[2]: main[1]
> Sending command: cont
> reply[0]: >
> reply[1]: Exception occurred: java.lang.NullPointerException (to be caught
> at: nsk.jdb.kill.kill001.MyThread.run(), line=165
> bci=107)"thread=MyThread-1", nsk.jdb.kill.kill001.MyThread.run(), line=164
> bci=100
> reply[2]: 164methodForException();
> reply[3]:
> reply[4]: MyThread-1[1]
> Sending command: cont
> reply[0]: >
> reply[1]: Exception occurred: nsk.jdb.kill.kill001.MyException (uncaught)
> reply[2]: Exception occurred: nsk.jdb.kill.kill001.MyException
> (uncaught)"thread=MyThread-4", nsk.jdb.kill.kill001.MyThread.run(), line=178
> bci=187
> reply[3]: 178kill001a.log.display(ThreadFinished);
> reply[4]:
> reply[5]: MyThread-4[1]
> Sending command: cont
> reply[0]: >
> reply[1]: Exception occurred: com.sun.jdi.IncompatibleThreadStateException
> (uncaught)
> reply[2]: Exception occurred: com.sun.jdi.IncompatibleThreadStateException
> (uncaught)"thread=MyThread-3", nsk.share.Log.display(), line=327 bci=9
> reply[3]: 327doPrint(message.toString());
> reply[4]:
> reply[5]: MyThread-3[1]
> Sending command: cont
> reply[0]: > Thread MyThread-1 caught expected async exception:
> java.lang.NullPointerException: kill001a's Exception
> reply[1]: Thread finished: MyThread-1
> reply[2]:
> reply[3]: Exception occurred: java.lang.ThreadDeath (uncaught)
> reply[4]: Exception occurred: java.lang.ThreadDeath
> (uncaught)"thread=MyThread-0", nsk.share.Log.doPrint(), line=495 bci=1
> reply[5]: 495PrintStream stream = findOutStream();
> reply[6]:
> reply[7]: MyThread-0[1]
> Sending command: cont
> reply[0]: >
> reply[1]: Exception occurred: java.lang.SecurityException (uncaught)
> reply[2]: Exception occurred: java.lang.SecurityException
> (uncaught)"thread=MyThread-2", nsk.share.Log.doPrint(), line=495 bci=1
> reply[3]: 495PrintStream stream = findOutStream();
Empty method could work but the JIT compiler can optimize it out with inlining.
But the loop is not needed. Something like this would work:
static public int trash;
void methodForException() {
trash = 10;
}
But I'm not sure if the static variable value needs to be used from outside of
this method. I'm afraid, the Escape Analysis can spoil this. :)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19547#discussion_r1626612160
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v5]
On Tue, 4 Jun 2024 08:54:40 GMT, Jaikiran Pai wrote: >> test/jdk/java/lang/instrument/NativeMethodPrefixAgent.java line 34: >> >>> 32: * @modules java.management >>> 33: * java.instrument >>> 34: * @run shell/timeout=240 MakeJAR2.sh NativeMethodPrefixAgent >>> NativeMethodPrefixApp 'Can-Retransform-Classes: true' >>> 'Can-Set-Native-Method-Prefix: true' >> >> Just a question. The original test had a timeout:`timeout=240`. My guess is >> that the default timeout was not enough. Do you think, it is not needed >> anymore or you just missed to add it? >> The same question applies to the other test. > > Hello Serguei, that `timeout=240` appears to have been added as part of > https://bugs.openjdk.org/browse/JDK-6528548 to several of these tests back in > Java 7 days because the `shell` test which was creating the jar file was > timing out. The actual test itself, the one which uses the generated jar to > run the agent, was completing within a second or two. > > With the current state of the PR I have run tier3 in our CI and there too > both these tests complete within a second on all platforms. So I decided not > to copy over this timeout as part of this change. My guess is these tests were timed out when executed with some specific options like -Xint, -Xcomp or any other that can slow down the execution (e.g. DeoptimizeALot). - PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1625855573
Re: RFR: 8307824: Clean up Finalizable.java and finalize terminology in vmTestbase/nsk/share [v3]
On Mon, 3 Jun 2024 23:02:28 GMT, Leonid Mesnik wrote: >> The fix removes finalization cleanup from vmTestbase. >> The last to classes that use it are: DebugeeBinder and SocketIOPipe. >> The DebugeeBinder is used in jdi and jdwp tests and is always linked with >> debuggee process. So the DebugeeProcess.waitFor() is the good place to close >> binder and free all it's resources. >> The SocketIOPipe is used directly in AOD tests where it should be closed >> after test execution. >> >> The OPipe (child of SocketIOPipe) also used in jdi and jdwp tests where it >> is connected directly in tests. However is also connected with debuggee and >> could be closed in DebugeeProcess.waitFor(). >> >> The VMOutOfMemoryException001 test is fixed to release some memory after >> throwing OOME so Sytem.exit() could complete successfully. Previously some >> memory freed during VM shutdown hook. >> >> I verified that cleanup printed that corresponding 'close' method has been >> already called before VM shutdown phase for debugger process. >> Additionally, run all vmTestbase tests to verify there are no failures, > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > moved return out of try/catch Looks good. Thank you for polishing it with Chris. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19505#pullrequestreview-2095566908
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v4]
On Tue, 4 Jun 2024 01:34:39 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change which addresses >> https://bugs.openjdk.org/browse/JDK-8333130? >> >> There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` >> under `test/jdk/java/lang/instrument/` which launch the app/test with a >> `-javaagent:` pointing to a test specific jar. The test, launched with >> that `javaagent`, then verifies the test specific details. >> >> In their current form, in order to construct the agent `.jar` and make it >> available to the jtreg test that's launched, they `@run` a jtreg `shell` >> test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar >> file. The contents of the script is relatively straightforward - it compiles >> (using `javac`) some boot classpath classes, some agent specific classes and >> then generates a jar file with the agent classes and a `MANIFEST.MF` which >> points to the boot classpath classes along with test specific manifest >> attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass >> `--enable-preview --release 23` since one of the existing agent class was >> updated to use the ClassFile API PreviewFeature >> (https://github.com/openjdk/jdk/pull/13009). >> >> This generated agent jar then is passed as `-javaagent:` to the subsequent >> `@run main` test which does the actual testing. >> >> So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there >> to create a jar file that's needed by the actual test. Within the JDK we >> have several tests which compile other classes and generate jar files using >> in-process test libraries and the `java.util.spi.ToolProvider` >> implementations. These 2 tests too can be updated to use a similar >> technique, to completely get rid of the `MakeJAR2.sh`. Removing the >> `MakeJAR2.sh` will simplify the ability to pass around value for `--release` >> when using `--enable-preview`. >> >> The commit in this PR updates these 2 tests to use `@run driver` which then >> compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and >> generates the agent jar file and then finally launching the test process. As >> part of the update, I did not retain the `@author` tag from the 2 test >> definitions, since it's my understanding that we no longer use those. I will >> add them back if we should retain them. >> >> The tests continue to pass locally with this change. I've also triggered >> tier testing in our CI for this change. > > Jaikiran Pai has updated the pull request incrementally with three additional > commits since the last revision: > > - Alex suggestion - remove -XX:-CheckIntrinsics from NativeMethodPrefixApp > test too > - Alex's suggestion - no need to include only the bootreporter classes in > boot classpath > - RetransformApp test did not previously use -XX:-CheckIntrinsics I've posted just a question and a nit. Otherwise, the fix looks good. Thank you for taking care about this refactoring! test/jdk/java/lang/instrument/NativeMethodPrefixAgent.java line 34: > 32: * @modules java.management > 33: * java.instrument > 34: * @run shell/timeout=240 MakeJAR2.sh NativeMethodPrefixAgent > NativeMethodPrefixApp 'Can-Retransform-Classes: true' > 'Can-Set-Native-Method-Prefix: true' Just a question. The original test had a timeout:`timeout=240`. My guess is that the default timeout was not enough. Do you think, it is not needed anymore or you just missed to add it? The same question applies to the other test. test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 26: > 24: > 25: import java.io.File; > 26: import java.nio.file.Files; Nit: It seems the import at line 26 is not needed. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19495#pullrequestreview-2095508509 PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1625470816 PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1625472194
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes [v4]
On Mon, 3 Jun 2024 22:55:24 GMT, Serguei Spitsyn wrote: >> Please, review the following `interp-only` issue related to carrier threads. >> There are 3 problems fixed here: >> - The `EnterInterpOnlyModeClosure::do_threads` is taking the >> `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect >> when we have a deal with a carrier thread. The target state is known at the >> point when the `HandshakeClosure` is set, so the fix is to pass it as a >> constructor parameter. >> - The `state->is_pending_interp_only_mode())` was processed at mounts only >> but it has to be processed for unmounts as well. >> - The test >> `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp` >> has a wrong assumption that there can't be `MethodExit` event on the >> carrier thread when the function `breakpoint_hit1` is being executed. >> However, it can happen if the virtual thread gets unmounted. >> >> The fix also includes new test case >> `vthread/CarrierThreadEventNotification` developed by Patricio. >> >> Testing: >> - Ran new test case locally >> - Ran mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: get rid of unneeded casts in new test Thank you for review, Alex! - PR Comment: https://git.openjdk.org/jdk/pull/19438#issuecomment-2146732081
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v5]
On Tue, 4 Jun 2024 04:48:04 GMT, David Holmes wrote: >> src/hotspot/share/prims/jvmti.xml line 1007: >> >>> 1005: explicitly deallocate. This is indicated in the individual >>> >>> 1006: function descriptions. Empty lists, arrays, sequences, etc are >>> 1007: returned as a null pointer (C NULL or C++ >>> nullptr). >> >> Why describe what is meant by a "null pointer" here when it is not done >> elsewhere? > > The intent is to provide a definition of what a null pointer is, for both C > and C++ programs. Is there a better place to do that so that elsewhere the > spec can simply to refer to "a null pointer" or "null"? Thanks, David. I also feel this clarification is still useful. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1625432133
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
On Fri, 17 May 2024 03:49:21 GMT, Quan Anh Mai wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: corrected the nullptr clarification > > src/hotspot/share/prims/jvmti.xml line 1007: > >> 1005: explicitly deallocate. This is indicated in the individual >> 1006: function descriptions. Empty lists, arrays, sequences, etc are >> 1007: returned as a null pointer (C NULL or C++ >> nullptr). > > This may be a little unnecessary rigor, but I believe that `nullptr` is not a > null pointer. `nullptr` is the pointer literal that can be implicitly > converted to a null pointer value of any pointer type and any pointer to > member type. And I think the thing returned here is a null pointer, not > `nullptr`. I'm not sure I understand this comment. Sorry. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1625430986
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes [v3]
On Fri, 31 May 2024 23:55:20 GMT, Serguei Spitsyn wrote: >> Please, review the following `interp-only` issue related to carrier threads. >> There are 3 problems fixed here: >> - The `EnterInterpOnlyModeClosure::do_threads` is taking the >> `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect >> when we have a deal with a carrier thread. The target state is known at the >> point when the `HandshakeClosure` is set, so the fix is to pass it as a >> constructor parameter. >> - The `state->is_pending_interp_only_mode())` was processed at mounts only >> but it has to be processed for unmounts as well. >> - The test >> `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp` >> has a wrong assumption that there can't be `MethodExit` event on the >> carrier thread when the function `breakpoint_hit1` is being executed. >> However, it can happen if the virtual thread gets unmounted. >> >> The fix also includes new test case >> `vthread/CarrierThreadEventNotification` developed by Patricio. >> >> Testing: >> - Ran new test case locally >> - Ran mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: refactored def and use of process_pending_interp_only() > I'm not sure I follow the test logic. Its summary says "Verifies that > MethodExit events are delivered on both carrier and virtual threads", but now > it just ignores MethodExit requested for carrier thread in breakpoint_hit1. Then there is no sense to request the event on carrier thread. > Per the test summary I'd expect the test should test MethodExit for carrier > thread, but then java part needs to force unmount. As we already agreed I've filed the cleanup test bug: [8333459](https://bugs.openjdk.org/browse/JDK-8333459) cleanup and check MethodExit events are posted on carrier threads in MethodExitTest - PR Comment: https://git.openjdk.org/jdk/pull/19438#issuecomment-2146254345
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes [v4]
> Please, review the following `interp-only` issue related to carrier threads. > There are 3 problems fixed here: > - The `EnterInterpOnlyModeClosure::do_threads` is taking the > `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect > when we have a deal with a carrier thread. The target state is known at the > point when the `HandshakeClosure` is set, so the fix is to pass it as a > constructor parameter. > - The `state->is_pending_interp_only_mode())` was processed at mounts only > but it has to be processed for unmounts as well. > - The test > `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp` > has a wrong assumption that there can't be `MethodExit` event on the carrier > thread when the function `breakpoint_hit1` is being executed. However, it can > happen if the virtual thread gets unmounted. > > The fix also includes new test case `vthread/CarrierThreadEventNotification` > developed by Patricio. > > Testing: > - Ran new test case locally > - Ran mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: get rid of unneeded casts in new test - Changes: - all: https://git.openjdk.org/jdk/pull/19438/files - new: https://git.openjdk.org/jdk/pull/19438/files/19e4d8fa..01304354 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19438=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19438=02-03 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19438.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19438/head:pull/19438 PR: https://git.openjdk.org/jdk/pull/19438
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes [v3]
On Sat, 1 Jun 2024 00:22:45 GMT, Alex Menkov wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: refactored def and use of process_pending_interp_only()
>
> test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
> line 40:
>
>> 38:
>> 39: static const char* CTHREAD_NAME_START = "ForkJoinPool";
>> 40: static const size_t CTHREAD_NAME_START_LEN = (int)strlen("ForkJoinPool");
>
> `(int)` cast is not needed
Thanks, fixed now.
> test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
> line 58:
>
>> 56: cthreads[ct_cnt++] = jni->NewGlobalRef(thread);
>> 57: }
>> 58: deallocate(jvmti, jni, (void*)tname);
>
> cast to `void*` is not needed
Why do you think, the cast is not needed?
This is the `deallocate()` function in the `jvmti_common.hpp`:
static void
deallocate(jvmtiEnv *jvmti, JNIEnv* jni, void* ptr) {
jvmtiError err = jvmti->Deallocate((unsigned char*)ptr);
check_jvmti_status(jni, err, "deallocate: error in JVMTI Deallocate call");
}
> test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
> line 96:
>
>> 94: }
>> 95: jvmtiError err = jvmti->Deallocate((unsigned char*)carrier_threads);
>> 96: check_jvmti_status(jni, err, "deallocate: error in JVMTI Deallocate
>> call");
>
> replace with `deallocate(jvmti, jni, carrier_threads);` ?
Thanks, fixed now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1624909747
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1624911862
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1624914083
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v4]
On Mon, 3 Jun 2024 08:01:25 GMT, David Holmes wrote: > The general rules are to either say "a null pointer" (possibly with capital A > depending on context), or just "null". And in most cases you could choose > either. I made various suggestions but really it is up to you. It is hard to > get a sense of consistency from these small fragments. > > The word "null" should never be in code font as it is not a programming > language entity. Thank you, David. This is really useful. I've pushed an update with all changes you suggested. - PR Comment: https://git.openjdk.org/jdk/pull/19257#issuecomment-2144777338
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v5]
> The following RFE was fixed recently: > [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with > nullptr in JVMTI generated code > > It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI > agents can be developed in C or C++. > This update is to make it clear that `nullptr` is C programming language > `null` pointer. > > I think we do not need a CSR for this fix. > > Testing: N/A (not needed) Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: consistency and stylistical corrections - Changes: - all: https://git.openjdk.org/jdk/pull/19257/files - new: https://git.openjdk.org/jdk/pull/19257/files/48ba8f5d..ed2eff27 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19257=04 - incr: https://webrevs.openjdk.org/?repo=jdk=19257=03-04 Stats: 43 lines in 4 files changed: 0 ins; 0 del; 43 mod Patch: https://git.openjdk.org/jdk/pull/19257.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19257/head:pull/19257 PR: https://git.openjdk.org/jdk/pull/19257
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v2]
On Fri, 31 May 2024 21:04:41 GMT, Daniel D. Daugherty wrote: > Ping @sspitsyn - who handles JLI/JPLIS reviews for the Serviceability team? Sorry for the latency. Will start reviewing it today. Also, I see Alex is reviewing it. - PR Comment: https://git.openjdk.org/jdk/pull/19495#issuecomment-2143136005
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes [v2]
On Thu, 30 May 2024 18:59:10 GMT, Patricio Chilano Mateo wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: addressed nits in new test > > src/hotspot/share/prims/jvmtiThreadState.cpp line 674: > >> 672: } >> 673: // enable interp_only_mode for carrier thread if it has pending bit >> 674: process_pending_interp_only(thread); > > So for the last unmount case we will call this before doing the JVMTI state > rebinding, but shouldn't it be called after it in VTMS_vthread_end? Actually > why not moving this call inside rebind_to_jvmti_thread_state_of()? Thank you for the comment! I was also thinking about placing it to the `rebind_to_jvmti_thread_state_of()`. I've made and pushed this change now. - PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1623046562
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes [v3]
> Please, review the following `interp-only` issue related to carrier threads. > There are 3 problems fixed here: > - The `EnterInterpOnlyModeClosure::do_threads` is taking the > `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect > when we have a deal with a carrier thread. The target state is known at the > point when the `HandshakeClosure` is set, so the fix is to pass it as a > constructor parameter. > - The `state->is_pending_interp_only_mode())` was processed at mounts only > but it has to be processed for unmounts as well. > - The test > `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp` > has a wrong assumption that there can't be `MethodExit` event on the carrier > thread when the function `breakpoint_hit1` is being executed. However, it can > happen if the virtual thread gets unmounted. > > The fix also includes new test case `vthread/CarrierThreadEventNotification` > developed by Patricio. > > Testing: > - Ran new test case locally > - Ran mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: refactored def and use of process_pending_interp_only() - Changes: - all: https://git.openjdk.org/jdk/pull/19438/files - new: https://git.openjdk.org/jdk/pull/19438/files/2f75975f..19e4d8fa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19438=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19438=01-02 Stats: 36 lines in 4 files changed: 16 ins; 18 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19438.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19438/head:pull/19438 PR: https://git.openjdk.org/jdk/pull/19438
Re: RFR: 8332923: ObjectMonitorUsage.java failed with unexpected waiter_count [v3]
On Thu, 30 May 2024 06:36:09 GMT, SendaoYan wrote: >> SendaoYan has updated the pull request incrementally with one additional >> commit since the last revision: >> >> change from java_lang_VirtualThread::is_instance(thread_oop) to >> hread_oop->is_a(vmClasses::BaseVirtualThread_klass()) in >> Threads::get_pending_threads() > > The GHA test runner report a intermittent failure > `ToolTabSnippetTest.testCleaningCompletionTODO(): failure`, which has been > recorded in [JDK-8287078](https://bugs.openjdk.org/browse/JDK-8287078), I > think it's unrelated to this PR. @sendaoYan The serviceability fixes require two reviews. Please, wait for a second reviewer before integration. - PR Comment: https://git.openjdk.org/jdk/pull/19405#issuecomment-2142964196
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v3]
On Fri, 31 May 2024 01:41:17 GMT, Serguei Spitsyn wrote: >> The following RFE was fixed recently: >> [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with >> nullptr in JVMTI generated code >> >> It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI >> agents can be developed in C or C++. >> This update is to make it clear that `nullptr` is C programming language >> `null` pointer. >> >> I think we do not need a CSR for this fix. >> >> Testing: N/A (not needed) > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: replace nullptr with null pointer in the docs Thanks, David. I've done one more attempt to correct it. Please, let me know if it is wrong. - PR Comment: https://git.openjdk.org/jdk/pull/19257#issuecomment-2141435705
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v4]
> The following RFE was fixed recently: > [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with > nullptr in JVMTI generated code > > It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI > agents can be developed in C or C++. > This update is to make it clear that `nullptr` is C programming language > `null` pointer. > > I think we do not need a CSR for this fix. > > Testing: N/A (not needed) Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: more null pointer corrections - Changes: - all: https://git.openjdk.org/jdk/pull/19257/files - new: https://git.openjdk.org/jdk/pull/19257/files/4e1c48a1..48ba8f5d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19257=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19257=02-03 Stats: 50 lines in 1 file changed: 0 ins; 0 del; 50 mod Patch: https://git.openjdk.org/jdk/pull/19257.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19257/head:pull/19257 PR: https://git.openjdk.org/jdk/pull/19257
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v3]
On Fri, 17 May 2024 04:47:31 GMT, Alan Bateman wrote: >> Thank you, Kim. I like this suggestion. Updated now. > > That part looks okay but I think all the parameters and error descriptions > changed by JDK-8324680 will now need to change to use "null" instead of > "nullptr". Okay. I've made a fix to replace in the doc: `nullptr` => `null` pointer as David suggested below. I can reduce it and remove the word 'pointer'. Please, let me know what is better. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1621575474
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
On Fri, 17 May 2024 04:34:22 GMT, Kim Barrett wrote: > But this clarification doesn't actually clarify that the rest of the spec > uses nullptr. Based on the proposed wording I would expect things like: > > The function may return nullptr > > to say > > The function may return a null pointer Okay. I've made a fix to replace in the docs `nullptr` with `null pointer` as you suggested. - PR Comment: https://git.openjdk.org/jdk/pull/19257#issuecomment-2141095067
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v3]
> The following RFE was fixed recently: > [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with > nullptr in JVMTI generated code > > It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI > agents can be developed in C or C++. > This update is to make it clear that `nullptr` is C programming language > `null` pointer. > > I think we do not need a CSR for this fix. > > Testing: N/A (not needed) Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: replace nullptr with null pointer in the docs - Changes: - all: https://git.openjdk.org/jdk/pull/19257/files - new: https://git.openjdk.org/jdk/pull/19257/files/9fe639e1..4e1c48a1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19257=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19257=01-02 Stats: 81 lines in 4 files changed: 0 ins; 0 del; 81 mod Patch: https://git.openjdk.org/jdk/pull/19257.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19257/head:pull/19257 PR: https://git.openjdk.org/jdk/pull/19257
Re: RFR: 8333108: Update vmTestbase/nsk/share/DebugeeProcess.java to don't use finalization
On Tue, 28 May 2024 20:24:31 GMT, Leonid Mesnik wrote: > The > test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeProcess.java > uses cleanup() to kill debuggee process. > > However, most tests kill the debuggee process explicitly. I verified that > debuggee process is killed before test finishes. (Just by printing it's > status.) > > The fix adds a few checks debuggee.waitFor() int tests that didn't check > debuggee process code. Looks good. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19437#pullrequestreview-2088643418
Re: RFR: 8332923: ObjectMonitorUsage.java failed with unexpected waiter_count [v3]
On Thu, 30 May 2024 01:13:20 GMT, SendaoYan wrote: >> Hi all, >> ObjectMonitorUsage.java failed with `unexpected waiter_count` after >> [JDK-8328083](https://bugs.openjdk.org/browse/JDK-8328083) on linux x86_32. >> There are two changes in this PR: >> 1. In `JvmtiEnvBase::get_object_monitor_usage` function, change from >> `java_lang_VirtualThread::is_instance(thread_oop)` to >> `thread_oop->is_a(vmClasses::BaseVirtualThread_klass())`to support the >> alternative implementation. >> 2. In `Threads::get_pending_threads(ThreadsList *, int, address)` function >> of threads.cpp file, change from >> `java_lang_VirtualThread::is_instance(thread_oop)` to >> `thread_oop->is_a(vmClasses::BaseVirtualThread_klass())`to support the >> alternative implementation. This modified function only used by >> `JvmtiEnvBase::get_object_monitor_usage(JavaThread*, jobject, >> jvmtiMonitorUsage*)`, so the risk of the modified on threads.cpp file is low. >> >> >> >> Additional testing: >> - [x] linux x86_32 run all testcases in serviceability/jvmti, all testcases >> run successed expect >> `serviceability/jvmti/vthread/GetThreadState/GetThreadStateTest.java#default` >> run failed. This test also run failed before this PR, which has been >> recorded in [JDK-8333140](https://bugs.openjdk.org/browse/JDK-8333140) >> - [x] linux x86_64 run all testcases in serviceability/jvmti, all testcases >> run successed. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > change from java_lang_VirtualThread::is_instance(thread_oop) to > hread_oop->is_a(vmClasses::BaseVirtualThread_klass()) in > Threads::get_pending_threads() Thank you for update. Looks good. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19405#pullrequestreview-2087753874
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes [v2]
On Wed, 29 May 2024 19:06:57 GMT, Chris Plummer wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: addressed nits in new test > > test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp > line 201: > >> 199: >> 200: // need to reset this value after the breakpoint_hit1 >> 201: received_method_exit_event = JNI_FALSE; > > There was a loom-dev email thread regarding this last year. Seems related. I > had concluded that the way the test was written that no MethodExit event > should have been received. I'm not sure if I missed something in my analysis > or if this failure is a result of your changes: > > https://mail.openjdk.org/pipermail/loom-dev/2023-August/006059.html > https://mail.openjdk.org/pipermail/loom-dev/2023-September/006170.html Thank you for the comment and links to the discussion. In fact, I've observed the MethodExit events really posted between the breakpoint hits: `hit1` and `hit2`. The first one is at the return from the `unmount()` method. I was not able to prove why they should not be expected. - PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619756552
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes [v2]
> Please, review the following `interp-only` issue related to carrier threads. > There are 3 problems fixed here: > - The `EnterInterpOnlyModeClosure::do_threads` is taking the > `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect > when we have a deal with a carrier thread. The target state is known at the > point when the `HandshakeClosure` is set, so the fix is to pass it as a > constructor parameter. > - The `state->is_pending_interp_only_mode())` was processed at mounts only > but it has to be processed for unmounts as well. > - The test > `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp` > has a wrong assumption that there can't be `MethodExit` event on the carrier > thread when the function `breakpoint_hit1` is being executed. However, it can > happen if the virtual thread gets unmounted. > > The fix also includes new test case `vthread/CarrierThreadEventNotification` > developed by Patricio. > > Testing: > - Ran new test case locally > - Ran mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: addressed nits in new test - Changes: - all: https://git.openjdk.org/jdk/pull/19438/files - new: https://git.openjdk.org/jdk/pull/19438/files/a0f5d278..2f75975f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19438=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19438=00-01 Stats: 18 lines in 2 files changed: 0 ins; 2 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/19438.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19438/head:pull/19438 PR: https://git.openjdk.org/jdk/pull/19438
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes
On Thu, 30 May 2024 01:03:01 GMT, Alex Menkov wrote:
>> Please, review the following `interp-only` issue related to carrier threads.
>> There are 3 problems fixed here:
>> - The `EnterInterpOnlyModeClosure::do_threads` is taking the
>> `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect
>> when we have a deal with a carrier thread. The target state is known at the
>> point when the `HandshakeClosure` is set, so the fix is to pass it as a
>> constructor parameter.
>> - The `state->is_pending_interp_only_mode())` was processed at mounts only
>> but it has to be processed for unmounts as well.
>> - The test
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp`
>> has a wrong assumption that there can't be `MethodExit` event on the
>> carrier thread when the function `breakpoint_hit1` is being executed.
>> However, it can happen if the virtual thread gets unmounted.
>>
>> The fix also includes new test case
>> `vthread/CarrierThreadEventNotification` developed by Patricio.
>>
>> Testing:
>> - Ran new test case locally
>> - Ran mach5 tiers 1-6
>
> test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
> line 44:
>
>> 42: static jint
>> 43: get_cthreads(JNIEnv* jni, jthread** cthreads_p) {
>> 44: jthread* tested_cthreads = NULL;
>
> This local variable has the same name as global.
> I'd suggest to rename the local var or remove it (and the function should set
> both `tested_cthreads` and ` cthread_cnt`)
Thanks. Renamed the local to `cthreads` and the global to `carrier_threads`.
> test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
> line 91:
>
>> 89: for (int i = 0; i < cthread_cnt; i++) {
>> 90: jthread thread = tested_cthreads[i];
>> 91: char* tname = get_thread_name(jvmti, jni, thread);
>
> `tname` is not needed
Removed, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619726804
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619728368
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes
On Thu, 30 May 2024 00:41:28 GMT, Alex Menkov wrote:
>> Please, review the following `interp-only` issue related to carrier threads.
>> There are 3 problems fixed here:
>> - The `EnterInterpOnlyModeClosure::do_threads` is taking the
>> `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect
>> when we have a deal with a carrier thread. The target state is known at the
>> point when the `HandshakeClosure` is set, so the fix is to pass it as a
>> constructor parameter.
>> - The `state->is_pending_interp_only_mode())` was processed at mounts only
>> but it has to be processed for unmounts as well.
>> - The test
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp`
>> has a wrong assumption that there can't be `MethodExit` event on the
>> carrier thread when the function `breakpoint_hit1` is being executed.
>> However, it can happen if the virtual thread gets unmounted.
>>
>> The fix also includes new test case
>> `vthread/CarrierThreadEventNotification` developed by Patricio.
>>
>> Testing:
>> - Ran new test case locally
>> - Ran mach5 tiers 1-6
>
> test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/CarrierThreadEventNotification.java
> line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
>
> (c) 2024
Fixed, thanks.
> test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
> line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
>
> (c) 2024
Fixed, thanks.
> test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
> line 40:
>
>> 38:
>> 39: static const char* CTHREAD_NAME_START = "ForkJoinPool";
>> 40: static const int CTHREAD_NAME_START_LEN = (int)strlen("ForkJoinPool");
>
> should be `size_t` (the value is used for `strncmp` which expects `size_t`)
Fixed, thanks.
> test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
> line 44:
>
>> 42: static jint
>> 43: get_cthreads(JNIEnv* jni, jthread** cthreads_p) {
>> 44: jthread* tested_cthreads = NULL;
>
> Suggestion:
>
> jthread* tested_cthreads = nullptr;
Fixed, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619716426
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619716604
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619717881
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619718490
Re: RFR: 8332923: ObjectMonitorUsage.java failed with unexpected waiter_count [v2]
On Tue, 28 May 2024 06:52:13 GMT, SendaoYan wrote:
>> Hi all,
>> ObjectMonitorUsage.java failed with `unexpected waiter_count` after
>> [JDK-8328083](https://bugs.openjdk.org/browse/JDK-8328083) on linux x86_32.
>> There are two changes in this PR:
>> 1. In JvmtiEnvBase::get_object_monitor_usage function, change from
>> `java_lang_VirtualThread::is_instance(thread_oop)` to
>> `thread_oop->is_a(vmClasses::BaseVirtualThread_klass())`to support the
>> alternative implementation.
>> 2. The JvmtiEnvBase::get_object_monitor_usage does take the vthread into
>> consideration when calculating nWant(mon->contentions()).
>>
>>
>>
>> Additional testing:
>> - [x] linux x86_32 run all testcases in serviceability/jvmti, all testcases
>> run successed expect
>> `serviceability/jvmti/vthread/GetThreadState/GetThreadStateTest.java#default`
>> run failed. This test also run failed before this PR, which has been
>> recorded in [JDK-8333140](https://bugs.openjdk.org/browse/JDK-8333140)
>> - [x] linux x86_64 run all testcases in serviceability/jvmti, all testcases
>> run successed.
>>
>> [x64.log](https://github.com/openjdk/jdk/files/15480081/x64.log)
>> [x86.log](https://github.com/openjdk/jdk/files/15480083/x86.log)
>
> SendaoYan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 1. java_lang_VirtualThread::is_instance(thread_oop) ->
> thread_oop->is_a(vmClasses::BaseVirtualThread_klass()); 2. calculating
> nWant(mon->contentions())
The fix looks good in general but I've inlined one suggestion.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1524:
> 1522: nWant_Skip++;
> 1523: }
> 1524: }
Thank you for taking care about this issue.
The nWant_Skip and and the fragment with lines 1518-1524 would not be needed if
the function `Threads::get_pending_threads()` is fixed instead:
-if (java_lang_VirtualThread::is_instance(thread_oop)) {
+if (thread_oop->is_a(vmClasses::BaseVirtualThread_klass())) {
``
-
PR Review: https://git.openjdk.org/jdk/pull/19405#pullrequestreview-2086876665
PR Review Comment: https://git.openjdk.org/jdk/pull/19405#discussion_r1619620594
Re: RFR: 8333149: ubsan : memset on nullptr target detected in jvmtiEnvBase.cpp get_object_monitor_usage
On Wed, 29 May 2024 09:09:16 GMT, Matthias Baesken wrote: > When running with ubsan - enabled binaries (--enable-ubsan), > in the vmTestbase/nsk/jdi tests some cases of memset on nullptr destinations > are detected in get_object_monitor_usage . > > // null out memory for robustness > memset(ret.waiters, 0, ret.waiter_count * sizeof(jthread *)); > memset(ret.notify_waiters, 0, ret.notify_waiter_count * sizeof(jthread *)); > > probably we should add checks there. > Example : > vmTestbase/nsk/jdi/ObjectReference/entryCount/entrycount002/TestDescription.jtr > > debugee.stderr> /src/hotspot/share/prims/jvmtiEnvBase.cpp:1560:11: runtime > error: null pointer passed as argument 1, which is declared to never be null > debugee.stderr> #0 0x7ffb2568559c in > JvmtiEnvBase::get_object_monitor_usage(JavaThread*, _jobject*, > jvmtiMonitorUsage*) src/hotspot/share/prims/jvmtiEnvBase.cpp:1560 > debugee.stderr> #1 0x7ffb27987bd7 in VM_GetObjectMonitorUsage::doit() > src/hotspot/share/prims/jvmtiEnvBase.hpp:594 > debugee.stderr> #2 0x7ffb28ddc2dd in VM_Operation::evaluate() > src/hotspot/share/runtime/vmOperations.cpp:75 > debugee.stderr> #3 0x7ffb28deac41 in > VMThread::evaluate_operation(VM_Operation*) > src/hotspot/share/runtime/vmThread.cpp:283 > debugee.stderr> #4 0x7ffb28decc4f in VMThread::inner_execute(VM_Operation*) > src/hotspot/share/runtime/vmThread.cpp:427 > debugee.stderr> #5 0x7ffb28ded7b9 in VMThread::loop() > src/hotspot/share/runtime/vmThread.cpp:493 > debugee.stderr> #6 0x7ffb28ded8a7 in VMThread::run() > src/hotspot/share/runtime/vmThread.cpp:177 > debugee.stderr> #7 0x7ffb28b7e31a in Thread::call_run() > src/hotspot/share/runtime/thread.cpp:225 > debugee.stderr> #8 0x7ffb281c4971 in thread_native_entry > src/hotspot/os/linux/os_linux.cpp:846 > debugee.stderr> #9 0x7ffb2df416e9 in start_thread > (/lib64/libpthread.so.0+0xa6e9) (BuildId: > 2f8d3c2d0f4d7888c2598d2ff6356537f5708a73) > debugee.stderr> #10 0x7ffb2d51550e in clone (/lib64/libc.so.6+0x11850e) > (BuildId: f732026552f6adff988b338e92d466bc81a01c37) > > vmTestbase/nsk/jdi/ObjectReference/owningThread/owningthread002/TestDescription.jtr > > debugee.stderr> /src/hotspot/share/prims/jvmtiEnvBase.cpp:1561:11: runtime > error: null pointer passed as argument 1, which is declared to never be null > debugee.stderr> #0 0x7f1e070855bb in > JvmtiEnvBase::get_object_monitor_usage(JavaThread*, _jobject*, > jvmtiMonitorUsage*) src/hotspot/share/prims/jvmtiEnvBase.cpp:1561 > debugee.stderr> #1 0x7f1e09387bd7 in VM_GetObjectMonitorUsage::doit() > src/hotspot/share/prims/jvmtiEnvBase.hpp:594 > debugee.stderr> #2 0x7f1e0a7dc2dd in VM_Operation::evaluate() src/hotsp... Looks good. Thank you for taking care about it. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19450#pullrequestreview-2085104582
Re: RFR: 8333047: Remove arena-size-workaround in jvmtiUtils.cpp
On Tue, 28 May 2024 12:36:41 GMT, Thomas Stuefe wrote: > In `JvmtiUtil::single_threaded_resource_area()`, we create a resource area > that is supposed to work even if the current thread is not attached yet and > there is no associated Thread or the Thread has no valid ResourceArea. > > It contains a workaround: > > > // lazily create the single threaded resource area > // pick a size which is not a standard since the pools don't exist yet > _single_threaded_resource_area = new (mtInternal) > ResourceArea(Chunk::non_pool_size); > > > It specifies a non-standard chunk size to circumvent the chunk-pool-based > allocation in the RA constructor, ensuring that only malloc is used. This is > because in the old days the ChunkPools had been allocated from C-Heap and > there was a time window when no chunk pools were live yet. > > This is quirky and a bit ugly. It is also unnecessary since > [JDK-8272112](https://bugs.openjdk.org/browse/JDK-8272112) (since JDK 18). We > now create chunk pools as global objects, so they are live as soon as the > libjvm C++ initialization ran. We can remove this workaround and the comment. > > --- > > Tests: GHAs. > I also manually called this function, and allocated from the resulting > ResourceArea, at the very beginning of CreateJavaVM. I made sure that both > allocations and follow-up-chunk-allocation worked even this early in VM life. Marked as reviewed by sspitsyn (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19425#pullrequestreview-2085073625
Re: RFR: 8328611: Thread safety issue in com.sun.tools.jdi.ReferenceTypeImpl::classObject [v2]
On Wed, 29 May 2024 01:47:27 GMT, Chris Plummer wrote: >> Fixed "double-checked-locking" bug in `ReferenceImplType.classObject()`. >> Details in the first comment. Tested with the following: >> - com/sun/jdi >> - nsk/jdi >> - nsk/jdwp >> - nsk/jdb > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Add period at end of comments Marked as reviewed by sspitsyn (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19439#pullrequestreview-2085064312
Re: RFR: 8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state [v5]
On Tue, 28 May 2024 22:29:28 GMT, Leonid Mesnik wrote:
>> The JvmtiTrace::safe_get_thread_name sometimes crashes when called while
>> current thread is in native thread state.
>>
>> It happens when thread_name is set for tracing from jvmti functions.
>> See:
>> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/jvmtiEnter.xsl#L649
>>
>>
>> The setup is called and the thread name is used in tracing before the thread
>> transition. There is no good location where this method could be called from
>> vm thread_state only. Some functions like raw monitor enter/exit never
>> transition in vm state. So sometimes it is needed to call this function from
>> native thread state.
>>
>> The change should affect JVMTI trace mode only (-XX:TraceJVMTI).
>>
>> Verified by running jvmti/jdi/jdb tests with tracing enabled.
>
> Leonid Mesnik has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - fixed space.
> - The result is updated.
This looks good, Posted one nit though.
src/hotspot/share/prims/jvmtiTrace.cpp line 284:
> 282: JavaThreadState current_state =
> JavaThread::cast(Thread::current())->thread_state();
> 283: if (current_state == _thread_in_native || current_state ==
> _thread_blocked) {
> 284: return "not readable";
Nit: I'd suggest to make it more detailed, something like like this:
"" or ""
-
Marked as reviewed by sspitsyn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19275#pullrequestreview-2084079674
PR Review Comment: https://git.openjdk.org/jdk/pull/19275#discussion_r1618051643
RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes
Please, review the following `interp-only` issue related to carrier threads. There are 3 problems fixed here: - The `EnterInterpOnlyModeClosure::do_threads` is taking the `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect when we have a deal with a carrier thread. The target state is known at the point when the `HandshakeClosure` is set, so the fix is to pass it as a constructor parameter. - The `state->is_pending_interp_only_mode())` was processed at mounts only but it has to be processed for unmounts as well. - The test `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp` has a wrong assumption that there can't be `MethodExit` event on the carrier thread when the function `breakpoint_hit1` is being executed. However, it can happen if the virtual thread gets unmounted. The fix also includes new test case `vthread/CarrierThreadEventNotification` developed by Patricio. Testing: - Ran new test case locally - Ran mach5 tiers 1-6 - Commit messages: - fix trailing spaces in new test - 8311177: Switching to interpreter only mode in carrier thread can lead to crashes Changes: https://git.openjdk.org/jdk/pull/19438/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19438=00 Issue: https://bugs.openjdk.org/browse/JDK-8311177 Stats: 251 lines in 7 files changed: 231 ins; 9 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/19438.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19438/head:pull/19438 PR: https://git.openjdk.org/jdk/pull/19438
Re: RFR: 8332631: Update nsk.share.jpda.BindServer to don't use finalization
On Tue, 21 May 2024 19:55:01 GMT, Leonid Mesnik wrote:
> The BindServer starts several threads and opens streams.
>
> It registered them for cleanup using "Finalizer" from nsk.share.framework.
> Currently, it cleanup resources during shutdown hook.
>
> This fix changes BindServer to explicitly close streams and finish threads
> after test is completed. The exceptions are just printed like it was done
> previously. I haven't caught any exception during in close method during
> testing.
Looks good. Posted one nit though.
test/hotspot/jtreg/vmTestbase/nsk/share/jpda/BindServer.java line 388:
> 386: * @see ServingThread
> 387: */
> 388: private static class ListeningThread extends Thread implements
> AutoCloseable{
Nit: Space is missed before '{'.
-
Marked as reviewed by sspitsyn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19335#pullrequestreview-2073933557
PR Review Comment: https://git.openjdk.org/jdk/pull/19335#discussion_r1611671166
Integrated: 8328083: degrade virtual thread support for GetObjectMonitorUsage
On Wed, 1 May 2024 10:20:52 GMT, Serguei Spitsyn wrote: > The fix is to degrade virtual threads support in the JVM TI > `GetObjectMonitorUsage` function so that it is specified to only return an > owner when the owner is a platform thread. Also, virtual threads are not > listed in the both `waiters` and `notify_waiters` lists returned in the > `jvmtiMonitorUsage` structure. Java 19 re-specified a number of JVMTI > functions and events for virtual threads, we missed this one. > > The main motivation for degrading it now is that the object monitor > implementation is being updated to allow virtual threads unmount while owning > monitors. It would add overhead to record monitor usage when > freezing/unmount, overhead that couldn't be tied to a JVMTI capability as the > capability can be enabled at any time. > > `GetObjectMonitorUsage` was broken for 20+ years > ([8247972](https://bugs.openjdk.org/browse/JDK-8247972)) without bug reports > so it seems unlikely that the function is widely used. Degrading it to only > return an owner when the owner is a platform thread has no compatibility > impact for tooling that uses it in conjunction with `HotSpot` thread dumps or > `ThreadMXBean`. > > One other point about `GetObjectMonitorUsage` is that it pre-dates > j.u.concurrent in Java 5 so it can't be used to get a full picture of the > lock usage in a program. > > The specs of the impacted `JDWP ObjectReference.MonitorInfo` command and the > JDI `ObjectReference` `ownerThread()`, `waitingThreads()` and `entryCount()` > methods are updated to match the JVM TI spec. > > Also, please, review the related CSR and Release Note: > - CSR: [8331422](https://bugs.openjdk.org/browse/JDK-8331422): degrade > virtual thread support for GetObjectMonitorUsage > - RN: [8331465](https://bugs.openjdk.org/browse/JDK-8331465): Release Note: > degrade virtual thread support for GetObjectMonitorUsage > > Testing: > - tested impacted and updated tests locally > - tested with mach5 tiers 1-6 This pull request has now been integrated. Changeset: b890336e Author:Serguei Spitsyn URL: https://git.openjdk.org/jdk/commit/b890336e111ea8473ae49e9992bc2fd61e716792 Stats: 188 lines in 12 files changed: 131 ins; 2 del; 55 mod 8328083: degrade virtual thread support for GetObjectMonitorUsage Reviewed-by: cjplummer, alanb - PR: https://git.openjdk.org/jdk/pull/19030
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v7]
On Wed, 15 May 2024 20:29:17 GMT, Serguei Spitsyn wrote: >> The fix is to degrade virtual threads support in the JVM TI >> `GetObjectMonitorUsage` function so that it is specified to only return an >> owner when the owner is a platform thread. Also, virtual threads are not >> listed in the both `waiters` and `notify_waiters` lists returned in the >> `jvmtiMonitorUsage` structure. Java 19 re-specified a number of JVMTI >> functions and events for virtual threads, we missed this one. >> >> The main motivation for degrading it now is that the object monitor >> implementation is being updated to allow virtual threads unmount while >> owning monitors. It would add overhead to record monitor usage when >> freezing/unmount, overhead that couldn't be tied to a JVMTI capability as >> the capability can be enabled at any time. >> >> `GetObjectMonitorUsage` was broken for 20+ years >> ([8247972](https://bugs.openjdk.org/browse/JDK-8247972)) without bug reports >> so it seems unlikely that the function is widely used. Degrading it to only >> return an owner when the owner is a platform thread has no compatibility >> impact for tooling that uses it in conjunction with `HotSpot` thread dumps >> or `ThreadMXBean`. >> >> One other point about `GetObjectMonitorUsage` is that it pre-dates >> j.u.concurrent in Java 5 so it can't be used to get a full picture of the >> lock usage in a program. >> >> The specs of the impacted `JDWP ObjectReference.MonitorInfo` command and the >> JDI `ObjectReference` `ownerThread()`, `waitingThreads()` and `entryCount()` >> methods are updated to match the JVM TI spec. >> >> Also, please, review the related CSR and Release Note: >> - CSR: [8331422](https://bugs.openjdk.org/browse/JDK-8331422): degrade >> virtual thread support for GetObjectMonitorUsage >> - RN: [8331465](https://bugs.openjdk.org/browse/JDK-8331465): Release Note: >> degrade virtual thread support for GetObjectMonitorUsage >> >> Testing: >> - tested impacted and updated tests locally >> - tested with mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: UNDO: removed incorrect simplification that removed a tmp local > skipped Thank you for review, Alan! - PR Comment: https://git.openjdk.org/jdk/pull/19030#issuecomment-2126628322
Re: RFR: 8331683: Clean up GetCarrierThread
On Sat, 18 May 2024 00:47:59 GMT, Alex Menkov wrote: > JVMTI GetCarrierThread extension function was introduced by loom for testing. > It's used by several tests in hotspot/jtreg/serviceability. > > Testings: tier1..tier6 Looks good. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19289#pullrequestreview-2067222107
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
On Fri, 17 May 2024 04:34:22 GMT, Kim Barrett wrote: > So JDK-8324680 was somewhat mistaken about what needed to be done, and what was done. The `jvmti.xml` is used to generate several things with the XSL scripts: - JVMTI spec (`jvm.html`) - JVMTI api (`jvmti.h`) - `jvmtiEnter.cpp`, `jvmtiEnterTrace.cpp` In fact, it is pretty tricky to separate these usage aspects of `nullptr` or `NULL`. One of the approaches is to undo the [JDK-8324680](https://bugs.openjdk.org/browse/JDK-8324680). Please, let me know if you prefer this path. - PR Comment: https://git.openjdk.org/jdk/pull/19257#issuecomment-2118354928
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
> The following RFE was fixed recently: > [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with > nullptr in JVMTI generated code > > It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI > agents can be developed in C or C++. > This update is to make it clear that `nullptr` is C programming language > `null` pointer. > > I think we do not need a CSR for this fix. > > Testing: N/A (not needed) Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: corrected the nullptr clarification - Changes: - all: https://git.openjdk.org/jdk/pull/19257/files - new: https://git.openjdk.org/jdk/pull/19257/files/fd0e8d43..9fe639e1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19257=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19257=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19257.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19257/head:pull/19257 PR: https://git.openjdk.org/jdk/pull/19257
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
On Thu, 16 May 2024 07:59:51 GMT, Kim Barrett wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: corrected the nullptr clarification > > src/hotspot/share/prims/jvmti.xml line 1008: > >> 1006: function descriptions. Empty lists, arrays, sequences, etc are >> 1007: returned as nullptr which is C programming language >> 1008: null pointer. > > Perhaps instead something like > > "returned as a null pointer (C NULL or C++ > nullptr)." > > "null pointer" is the generic phrase used in both the C and C++ standards. Thank you, Kim. I like this suggestion. Updated now. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1604210615
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
On Thu, 16 May 2024 19:26:07 GMT, Chris Plummer wrote: >> src/hotspot/share/prims/jvmti.xml line 1008: >> >>> 1006: function descriptions. Empty lists, arrays, sequences, etc are >>> 1007: returned as nullptr which is C programming language >>> 1008: null pointer. >> >> Shouldn't this be "NULL"? In any case, I think it would be helpful to >> expand this a bit to make it clear that usages of "nullptr" in parameter and >> error descriptions should be read or treated as "NULL" when developing an >> agent in C rather than C++. > > Yes, I think it should by NULL. Thank you for suggestions. I've changed it as Kim suggested below. Please, let me know if it is not good enough, or some additions are needed. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1604211567
RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers
The following RFE was fixed recently: [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with nullptr in JVMTI generated code It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI agents can be developed in C or C++. This update is to make it clear that `nullptr` is C programming language `null` pointer. I think we do not need a CSR for this fix. Testing: N/A (not needed) - Commit messages: - 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers Changes: https://git.openjdk.org/jdk/pull/19257/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19257=00 Issue: https://bugs.openjdk.org/browse/JDK-8326716 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19257.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19257/head:pull/19257 PR: https://git.openjdk.org/jdk/pull/19257
Re: RFR: 8332327: Return _methods_jmethod_ids field back in VMStructs
On Wed, 15 May 2024 21:12:03 GMT, Andrei Pangin wrote: > The fix for [JDK-8313332](https://bugs.openjdk.org/browse/JDK-8313332) has > [removed](https://github.com/openjdk/jdk/commit/21867c929a2f2c961148f2cd1e79d672ac278d27#diff-7d448441e80a0b784429d5d8aee343fcb131c224b8ec7bc70ea636f78d561ecd > ) `InstanceKlass::_methods_jmethod_ids` field from VMStructs. > > This broke > [async-profiler](https://github.com/async-profiler/async-profiler/), since > the profiler needs this field to obtain jmethodID in some corner cases. > > There was no actual need for removal, as the field is still there in > InstanceKlass. So, I propose to return the field back to restore the broken > functionality of async-profiler. This is a no risk change, because it only > exports an offset of one field and does not affect the JVM otherwise. Looks good and low risk. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19254#pullrequestreview-2059177894
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
On Wed, 15 May 2024 16:59:59 GMT, Kevin Walls wrote:
> Running JConsole from a previous JDK, and attaching to jdk-23 (after
> [JDK-832](https://bugs.openjdk.org/browse/JDK-832): Remove the Java
> Management Extension (JMX) Subject Delegation feature), the MBean tab is
> blank.
>
> In javax/management/remote/rmi/RMIConnectionImpl.java:
> addNotificationListener rejects a non-null delegationSubjects array, but
> older JDKs will send such an array. It could accept the array, and only
> reject/throw if it contains a non-null Subject (i.e. if an attempt to use
> subject delegation is really happening).
>
> Manually testing JConsole, the MBean tab is fully populated and usable.
Looks good in general. Posted a couple of questions though.
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 979:
> 977: for (Subject s: delegationSubjects) {
> 978: if (s != null) {
> 979: throw new UnsupportedOperationException("Subject
> Delegation has been removed.");
Q1: Would it make sense to provide any details about the failing
`delegationSubject` element?
Q2: Does this fix needs a CSR?
-
PR Review: https://git.openjdk.org/jdk/pull/19253#pullrequestreview-2059157190
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1602337036
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v7]
On Wed, 15 May 2024 20:29:17 GMT, Serguei Spitsyn wrote: >> The fix is to degrade virtual threads support in the JVM TI >> `GetObjectMonitorUsage` function so that it is specified to only return an >> owner when the owner is a platform thread. Also, virtual threads are not >> listed in the both `waiters` and `notify_waiters` lists returned in the >> `jvmtiMonitorUsage` structure. Java 19 re-specified a number of JVMTI >> functions and events for virtual threads, we missed this one. >> >> The main motivation for degrading it now is that the object monitor >> implementation is being updated to allow virtual threads unmount while >> owning monitors. It would add overhead to record monitor usage when >> freezing/unmount, overhead that couldn't be tied to a JVMTI capability as >> the capability can be enabled at any time. >> >> `GetObjectMonitorUsage` was broken for 20+ years >> ([8247972](https://bugs.openjdk.org/browse/JDK-8247972)) without bug reports >> so it seems unlikely that the function is widely used. Degrading it to only >> return an owner when the owner is a platform thread has no compatibility >> impact for tooling that uses it in conjunction with `HotSpot` thread dumps >> or `ThreadMXBean`. >> >> One other point about `GetObjectMonitorUsage` is that it pre-dates >> j.u.concurrent in Java 5 so it can't be used to get a full picture of the >> lock usage in a program. >> >> The specs of the impacted `JDWP ObjectReference.MonitorInfo` command and the >> JDI `ObjectReference` `ownerThread()`, `waitingThreads()` and `entryCount()` >> methods are updated to match the JVM TI spec. >> >> Also, please, review the related CSR and Release Note: >> - CSR: [8331422](https://bugs.openjdk.org/browse/JDK-8331422): degrade >> virtual thread support for GetObjectMonitorUsage >> - RN: [8331465](https://bugs.openjdk.org/browse/JDK-8331465): Release Note: >> degrade virtual thread support for GetObjectMonitorUsage >> >> Testing: >> - tested impacted and updated tests locally >> - tested with mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: UNDO: removed incorrect simplification that removed a tmp local > skipped Thank you for review, Chris! - PR Comment: https://git.openjdk.org/jdk/pull/19030#issuecomment-2113523235
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v4]
On Wed, 15 May 2024 20:09:52 GMT, Serguei Spitsyn wrote:
>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1535:
>>
>>> 1533: bool is_virtual =
>>> java_lang_VirtualThread::is_instance(thread_oop);
>>> 1534: if (is_virtual) {
>>> 1535: skipped++;
>>
>> Do we really need to maintain `skipped`. Isn't not adding to `nWait` the
>> same as skipping?
>
> Good suggestion, thanks. Fixed now.
I've undone this suggested simplification as it has not worked out. Please, see
my answer on your next comment.
>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1583:
>>
>>> 1581: assert(w != nullptr, "sanity check");
>>> 1582: if (java_lang_VirtualThread::is_instance(thread_oop)) {
>>> 1583: skipped++;
>>
>> I don't think maintaining `skipped` does anything here.
>
> Thank you for the question. It is needed at the line 1586 below to discount
> the index:
>
> if (java_lang_VirtualThread::is_instance(thread_oop)) {
> skipped++;
> } else {
> // If the thread was found on the ObjectWaiter list, then
> // it has not been notified.
> Handle th(current_thread, get_vthread_or_thread_oop(w));
> 1586: ret.notify_waiters[i - skipped] =
> (jthread)jni_reference(calling_thread, th);
> }
BTW: The simplification (getting rid of local `skipped`) you requested in
previous comment damaged this fragment by making it incorrect. Here we need the
`nWait` to account for virtual threads as well. Otherwise, the loop is shorted.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1602212314
PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1602210128
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v7]
> The fix is to degrade virtual threads support in the JVM TI > `GetObjectMonitorUsage` function so that it is specified to only return an > owner when the owner is a platform thread. Also, virtual threads are not > listed in the both `waiters` and `notify_waiters` lists returned in the > `jvmtiMonitorUsage` structure. Java 19 re-specified a number of JVMTI > functions and events for virtual threads, we missed this one. > > The main motivation for degrading it now is that the object monitor > implementation is being updated to allow virtual threads unmount while owning > monitors. It would add overhead to record monitor usage when > freezing/unmount, overhead that couldn't be tied to a JVMTI capability as the > capability can be enabled at any time. > > `GetObjectMonitorUsage` was broken for 20+ years > ([8247972](https://bugs.openjdk.org/browse/JDK-8247972)) without bug reports > so it seems unlikely that the function is widely used. Degrading it to only > return an owner when the owner is a platform thread has no compatibility > impact for tooling that uses it in conjunction with `HotSpot` thread dumps or > `ThreadMXBean`. > > One other point about `GetObjectMonitorUsage` is that it pre-dates > j.u.concurrent in Java 5 so it can't be used to get a full picture of the > lock usage in a program. > > The specs of the impacted `JDWP ObjectReference.MonitorInfo` command and the > JDI `ObjectReference` `ownerThread()`, `waitingThreads()` and `entryCount()` > methods are updated to match the JVM TI spec. > > Also, please, review the related CSR and Release Note: > - CSR: [8331422](https://bugs.openjdk.org/browse/JDK-8331422): degrade > virtual thread support for GetObjectMonitorUsage > - RN: [8331465](https://bugs.openjdk.org/browse/JDK-8331465): Release Note: > degrade virtual thread support for GetObjectMonitorUsage > > Testing: > - tested impacted and updated tests locally > - tested with mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: UNDO: removed incorrect simplification that removed a tmp local skipped - Changes: - all: https://git.openjdk.org/jdk/pull/19030/files - new: https://git.openjdk.org/jdk/pull/19030/files/f083fd65..7091a3f6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19030=06 - incr: https://webrevs.openjdk.org/?repo=jdk=19030=05-06 Stats: 6 lines in 1 file changed: 2 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19030.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19030/head:pull/19030 PR: https://git.openjdk.org/jdk/pull/19030
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v6]
> The fix is to degrade virtual threads support in the JVM TI > `GetObjectMonitorUsage` function so that it is specified to only return an > owner when the owner is a platform thread. Also, virtual threads are not > listed in the both `waiters` and `notify_waiters` lists returned in the > `jvmtiMonitorUsage` structure. Java 19 re-specified a number of JVMTI > functions and events for virtual threads, we missed this one. > > The main motivation for degrading it now is that the object monitor > implementation is being updated to allow virtual threads unmount while owning > monitors. It would add overhead to record monitor usage when > freezing/unmount, overhead that couldn't be tied to a JVMTI capability as the > capability can be enabled at any time. > > `GetObjectMonitorUsage` was broken for 20+ years > ([8247972](https://bugs.openjdk.org/browse/JDK-8247972)) without bug reports > so it seems unlikely that the function is widely used. Degrading it to only > return an owner when the owner is a platform thread has no compatibility > impact for tooling that uses it in conjunction with `HotSpot` thread dumps or > `ThreadMXBean`. > > One other point about `GetObjectMonitorUsage` is that it pre-dates > j.u.concurrent in Java 5 so it can't be used to get a full picture of the > lock usage in a program. > > The specs of the impacted `JDWP ObjectReference.MonitorInfo` command and the > JDI `ObjectReference` `ownerThread()`, `waitingThreads()` and `entryCount()` > methods are updated to match the JVM TI spec. > > Also, please, review the related CSR and Release Note: > - CSR: [8331422](https://bugs.openjdk.org/browse/JDK-8331422): degrade > virtual thread support for GetObjectMonitorUsage > - RN: [8331465](https://bugs.openjdk.org/browse/JDK-8331465): Release Note: > degrade virtual thread support for GetObjectMonitorUsage > > Testing: > - tested impacted and updated tests locally > - tested with mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: simplified a fragment by removing tmp local variable - Changes: - all: https://git.openjdk.org/jdk/pull/19030/files - new: https://git.openjdk.org/jdk/pull/19030/files/95ea3621..f083fd65 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19030=05 - incr: https://webrevs.openjdk.org/?repo=jdk=19030=04-05 Stats: 5 lines in 1 file changed: 0 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19030.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19030/head:pull/19030 PR: https://git.openjdk.org/jdk/pull/19030
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v4]
On Wed, 15 May 2024 19:52:36 GMT, Chris Plummer wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: 1. clarifications in JDWP and JDI spec; 2. clarifications in test
>> comments.
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1583:
>
>> 1581: assert(w != nullptr, "sanity check");
>> 1582: if (java_lang_VirtualThread::is_instance(thread_oop)) {
>> 1583: skipped++;
>
> I don't think maintaining `skipped` does anything here.
Thank you for the question. It is needed at the line 1586 below to discount the
index:
if (java_lang_VirtualThread::is_instance(thread_oop)) {
skipped++;
} else {
// If the thread was found on the ObjectWaiter list, then
// it has not been notified.
Handle th(current_thread, get_vthread_or_thread_oop(w));
1586: ret.notify_waiters[i - skipped] =
(jthread)jni_reference(calling_thread, th);
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1602193077
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v4]
On Wed, 15 May 2024 19:51:51 GMT, Chris Plummer wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: 1. clarifications in JDWP and JDI spec; 2. clarifications in test
>> comments.
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1535:
>
>> 1533: bool is_virtual =
>> java_lang_VirtualThread::is_instance(thread_oop);
>> 1534: if (is_virtual) {
>> 1535: skipped++;
>
> Do we really need to maintain `skipped`. Isn't not adding to `nWait` the same
> as skipping?
Good suggestion, thanks. Fixed now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1602188546
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v5]
> The fix is to degrade virtual threads support in the JVM TI > `GetObjectMonitorUsage` function so that it is specified to only return an > owner when the owner is a platform thread. Also, virtual threads are not > listed in the both `waiters` and `notify_waiters` lists returned in the > `jvmtiMonitorUsage` structure. Java 19 re-specified a number of JVMTI > functions and events for virtual threads, we missed this one. > > The main motivation for degrading it now is that the object monitor > implementation is being updated to allow virtual threads unmount while owning > monitors. It would add overhead to record monitor usage when > freezing/unmount, overhead that couldn't be tied to a JVMTI capability as the > capability can be enabled at any time. > > `GetObjectMonitorUsage` was broken for 20+ years > ([8247972](https://bugs.openjdk.org/browse/JDK-8247972)) without bug reports > so it seems unlikely that the function is widely used. Degrading it to only > return an owner when the owner is a platform thread has no compatibility > impact for tooling that uses it in conjunction with `HotSpot` thread dumps or > `ThreadMXBean`. > > One other point about `GetObjectMonitorUsage` is that it pre-dates > j.u.concurrent in Java 5 so it can't be used to get a full picture of the > lock usage in a program. > > The specs of the impacted `JDWP ObjectReference.MonitorInfo` command and the > JDI `ObjectReference` `ownerThread()`, `waitingThreads()` and `entryCount()` > methods are updated to match the JVM TI spec. > > Also, please, review the related CSR and Release Note: > - CSR: [8331422](https://bugs.openjdk.org/browse/JDK-8331422): degrade > virtual thread support for GetObjectMonitorUsage > - RN: [8331465](https://bugs.openjdk.org/browse/JDK-8331465): Release Note: > degrade virtual thread support for GetObjectMonitorUsage > > Testing: > - tested impacted and updated tests locally > - tested with mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: fixed minor typos in JDI and JDWP specs - Changes: - all: https://git.openjdk.org/jdk/pull/19030/files - new: https://git.openjdk.org/jdk/pull/19030/files/8438cf4a..95ea3621 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19030=04 - incr: https://webrevs.openjdk.org/?repo=jdk=19030=03-04 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19030.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19030/head:pull/19030 PR: https://git.openjdk.org/jdk/pull/19030
Re: RFR: 8307778: com/sun/jdi/cds tests are not compatible with jtreg test factory
On Wed, 15 May 2024 05:59:56 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which removes the > `test/jdk/com/sun/jdi/cds/` tests from being problem listed when jtreg > launches these tests through a virtual thread? > > These tests aren't actually incompatible with virtual threads. The real issue > is that in https://bugs.openjdk.org/browse/JDK-8305608, a test infrastructure > class `test/jdk/com/sun/jdi/VMConnection.java` was changed to use the > `test.class.path` system property instead of the previously used > `test.classes`. > > That change missed out updating the `test/jdk/com/sun/jdi/cds/` tests to pass > along the classpath through the `test.class.path` system property. As a > result these tests still use the old `test.classes` system property to pass > around the test classpath and thus causes these tests to fail. The reason why > this only impacts virtual threads is noted by Chris in this JBS comment > https://bugs.openjdk.org/browse/JDK-8305608?focusedId=14572100=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14572100. > > The commit in this PR updates the `CDSJDITest` to pass along > `test.class.path` instead of `test.classes`. The `CDSJDITest` is only used in > the tests under `test/jdk/com/sun/jdi/cds/`, so nothing else should be > impacted by this change. > > I ran these changes against both launching with platform thread as well as > virtual thread and these tests now pass in both these cases. Looks good. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19244#pullrequestreview-2057187138
Re: RFR: 8330969: scalability issue with loaded JVMTI agent [v2]
On Fri, 10 May 2024 22:09:01 GMT, Daniel D. Daugherty wrote: > Perhaps this is not what Chris had in mind, but I'm wondering what happens in > some > Thread-A when it is checked and passed by but then Thread-A sets the flag in > itself > after the for-loop has passed it by. Does that Thread-A flag value get lost? Thank you for the question. The Thread-A sets the flag optimistically and then re-checks if `sync_protocol_enabled()` and any disabler exists. It can be global disbaler (`_VTMS_transition_disable_for_all_count > 0`) or disabler of `Thread-A` only (`java_lang_Thread::VTMS_transition_disable_count(vth()) > 0`). If any disabler exists then `Thread-A` clears the optimistic settings and goes with the pessimistic approach under protection of `JvmtiVTMSTransition_lock`. Please, let me know if you still have questions. - PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1600987604
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v4]
> The fix is to degrade virtual threads support in the JVM TI > `GetObjectMonitorUsage` function so that it is specified to only return an > owner when the owner is a platform thread. Also, virtual threads are not > listed in the both `waiters` and `notify_waiters` lists returned in the > `jvmtiMonitorUsage` structure. Java 19 re-specified a number of JVMTI > functions and events for virtual threads, we missed this one. > > The main motivation for degrading it now is that the object monitor > implementation is being updated to allow virtual threads unmount while owning > monitors. It would add overhead to record monitor usage when > freezing/unmount, overhead that couldn't be tied to a JVMTI capability as the > capability can be enabled at any time. > > `GetObjectMonitorUsage` was broken for 20+ years > ([8247972](https://bugs.openjdk.org/browse/JDK-8247972)) without bug reports > so it seems unlikely that the function is widely used. Degrading it to only > return an owner when the owner is a platform thread has no compatibility > impact for tooling that uses it in conjunction with `HotSpot` thread dumps or > `ThreadMXBean`. > > One other point about `GetObjectMonitorUsage` is that it pre-dates > j.u.concurrent in Java 5 so it can't be used to get a full picture of the > lock usage in a program. > > The specs of the impacted `JDWP ObjectReference.MonitorInfo` command and the > JDI `ObjectReference` `ownerThread()`, `waitingThreads()` and `entryCount()` > methods are updated to match the JVM TI spec. > > Also, please, review the related CSR and Release Note: > - CSR: [8331422](https://bugs.openjdk.org/browse/JDK-8331422): degrade > virtual thread support for GetObjectMonitorUsage > - RN: [8331465](https://bugs.openjdk.org/browse/JDK-8331465): Release Note: > degrade virtual thread support for GetObjectMonitorUsage > > Testing: > - tested impacted and updated tests locally > - tested with mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: 1. clarifications in JDWP and JDI spec; 2. clarifications in test comments. - Changes: - all: https://git.openjdk.org/jdk/pull/19030/files - new: https://git.openjdk.org/jdk/pull/19030/files/e7c2d652..8438cf4a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19030=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19030=02-03 Stats: 29 lines in 3 files changed: 25 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/19030.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19030/head:pull/19030 PR: https://git.openjdk.org/jdk/pull/19030
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v3]
On Wed, 1 May 2024 20:45:58 GMT, Chris Plummer wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: tweaks in JVMTI and JDWP changes
>
> src/jdk.jdi/share/classes/com/sun/jdi/ObjectReference.java line 348:
>
>> 346: /**
>> 347: * Returns a List containing a {@link ThreadReference} for
>> 348: * each platform thread currently waiting for this object's monitor.
>
> You need to add "platform" a little below in the `@return` section.
Okay, thanks! Fixed now. The update is:
diff --git a/src/jdk.jdi/share/classes/com/sun/jdi/ObjectReference.java
b/src/jdk.jdi/share/classes/com/sun/jdi/ObjectReference.java
index 3f3490e84cd..affbf9f6c4c 100644
--- a/src/jdk.jdi/share/classes/com/sun/jdi/ObjectReference.java
+++ b/src/jdk.jdi/share/classes/com/sun/jdi/ObjectReference.java
@@ -355,7 +355,8 @@ Value invokeMethod(ThreadReference thread, Method method,
* operation is supported.
*
* @return a List of {@link ThreadReference} objects. The list
- * has zero length if no threads are waiting for the monitor.
+ * has zero length if no threads are waiting for the monitor,
+ * or only virtual threads are waiting for the monitor.
* @throws java.lang.UnsupportedOperationException if the
* target VM does not support this operation.
* @throws IncompatibleThreadStateException if any
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1600786965
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v3]
On Thu, 2 May 2024 21:50:26 GMT, Chris Plummer wrote: >> src/java.se/share/data/jdwp/jdwp.spec line 1622: >> >>> 1620: (threadObject owner "The platform thread owning this >>> monitor, or nullptr " >>> 1621: "if owned` by a virtual thread or not >>> owned.") >>> 1622: (int entryCount "The number of times the owning platform >>> thread has entered the monitor.") >> >> See the comment I left for the JVMTI spec. We should be more complete in the >> explanation here, explaining how it is 0 for virtual threads. > > I don't think this has been resolved. Okay, thanks! Fixed now. The update is: --- a/src/java.se/share/data/jdwp/jdwp.spec +++ b/src/java.se/share/data/jdwp/jdwp.spec @@ -1619,9 +1619,11 @@ JDWP "Java(tm) Debug Wire Protocol" (Reply (threadObject owner "The platform thread owning this monitor, or null " "if owned by a virtual thread or not owned.") -(int entryCount "The number of times the owning platform thread has entered the monitor.") +(int entryCount "The number of times the owning platform thread has entered the monitor, " +"or 0 if owned by a virtual thread or not owned.") (Repeat waiters "The total number of platform threads that are waiting to enter or re-enter " -"the monitor, or waiting to be notified by the monitor." +"the monitor, or waiting to be notified by the monitor, or 0 if " +"only virtual threads are waiting or no threads are waiting." (threadObject thread "A platform thread waiting for this monitor.") ) ) - PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1600779564
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v3]
On Tue, 14 May 2024 23:13:28 GMT, Serguei Spitsyn wrote: >> I don't understand the issue with the updated commented. It is precisely >> telling you what the expected "count" values should be. If you leave the >> macros in the comment, then the comment is wrong for virtual threads. If you >> want to keep the macros in the comment, you need to add something like "... >> or 0 for virtual threads". >> >> BTW, the "re-enter" comment should continue to be "i + 1". I'm not sure why >> it was changed to "expEntryCount()". > > Okay, please, let me explain this one more time. > The original comments before method `check()` calls describe the testing > scenario but not the numbers expected to be returned by the JVMTI > `GetObjectMonitorUsage`. > For instance, if the testing scenario says: "count of threads waiting to > enter: NUMBER_OF_ENTERING_THREADS" then it means there is a real number of > these threads waiting to enter the monitor. And it does not matter if they > are platform or virtual threads. They are really waiting to enter the > monitor. However, the JVMTI `GetObjectMonitorUsage` won't include virtual > threads into the returned results. > > Now, I'm suggesting to add the following header for comments before each > `check()` method call: > > +// The numbers below describe the testing scenario, not the > expected results. > +// The expected numbers are different for virtual threads because > +// they are not supported by JVMTI GetObjectMonitorUsage. > > Would it work for you? > BTW, the "re-enter" comment should continue to be "i + 1". > I'm not sure why it was changed to "expEntryCount()". It depends on what are we trying to describe. We either describe the testing scenario (the number of threads doing something) or the expected results. I understood that you wanted to describe the results instead of the scenario. And then it becomes problematic to do so as you can see. - PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1600772051
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v3]
On Tue, 14 May 2024 17:51:03 GMT, Chris Plummer wrote:
>>> expEnteringCount/expWaitingCount contain the tested patterns.
>>
>> I kind of disagree.
>> Please, take look at the loop below:
>>
>> for (int i = 0; i < NUMBER_OF_WAITING_THREADS; i++) {
>> expEnteringCount = isVirtual ? 0 :
>> NUMBER_OF_ENTERING_THREADS + i + 1;
>> expWaitingCount = isVirtual ? 0 : NUMBER_OF_WAITING_THREADS
>> - i - 1;
>> lockCheck.notify(); // notify waiting threads one by one
>> // now the notified WaitingTask has to be blocked on the
>> lockCheck re-enter
>>
>> // entry count: 1
>> // count of threads waiting to enter:
>> NUMBER_OF_ENTERING_THREADS
>> // count of threads waiting to re-enter:i + 1
>> // count of threads waiting to be notified:
>> NUMBER_OF_WAITING_THREADS - i - 1
>> check(lockCheck, expOwnerThread(), expEntryCount(),
>> expEnteringCount,
>> expWaitingCount);
>> }
>>
>> The comment fixed as you suggest does not look useful anymore as the tested
>> pattern is lost:
>>
>> // entry count: expOwnerThread()
>> // count of threads waiting to enter: expEnteringCount
>> // count of threads waiting to re-enter: expEntryCount()
>> // count of threads waiting to be notified: expWaitingCount
>> check(lockCheck, expOwnerThread(), expEntryCount(),
>> expEnteringCount,
>> expWaitingCount);
>> }
>>
>>
>> I understand your concern but your suggestion is not that good.
>> We could remove these comments but the tested pattern will be thrown away
>> with the comments.
>> Would it help if we add clarifications that the comments are correct for
>> platform threads only?
>
> I don't understand the issue with the updated commented. It is precisely
> telling you what the expected "count" values should be. If you leave the
> macros in the comment, then the comment is wrong for virtual threads. If you
> want to keep the macros in the comment, you need to add something like "...
> or 0 for virtual threads".
>
> BTW, the "re-enter" comment should continue to be "i + 1". I'm not sure why
> it was changed to "expEntryCount()".
Okay, please, let me explain this one more time.
The original comments before method `check()` calls describe the testing
scenario but not the numbers expected to be returned by the JVMTI
`GetObjectMonitorUsage`.
For instance, if the testing scenario says: "count of threads waiting to enter:
NUMBER_OF_ENTERING_THREADS" then it means there is a real number of these
threads waiting to enter the monitor. And it does not matter if they are
platform or virtual threads. They are really waiting to enter the monitor.
However, the JVMTI `GetObjectMonitorUsage` won't include virtual threads into
the returned results.
Now, I'm suggesting to add the following header for comments before each
`check()` method call:
+// The numbers below describe the testing scenario, not the
expected results.
+// The expected numbers are different for virtual threads because
+// they are not supported by JVMTI GetObjectMonitorUsage.
Would it work for you?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1600769143
Re: RFR: 8331950: Remove MemoryPoolMBean/isCollectionUsageThresholdExceeded from ZGC ProblemLists
On Wed, 8 May 2024 17:05:30 GMT, Kevin Walls wrote: > Remove from zgc problemlists. > Trivial fix. > > This was omitted when https://bugs.openjdk.org/browse/JDK-8303136 was > integrated. > > I see the tests passing, including with ZGC. Just ran my own batch of tests > in addition, and it includes passes with e.g. -XX:+UseZGC -XX:+ZGenerational Looks good and trivial. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19143#pullrequestreview-2048206935
Integrated: 8330146: assert(!_thread->is_in_any_VTMS_transition()) failed
On Thu, 2 May 2024 10:07:35 GMT, Serguei Spitsyn wrote: > Any event posting code except CFLH, ClassPrepare and ClassLoad events has a > conditional return in case if the event is posted during a VTMS transition. > The CFLH, ClassPrepare and ClassLoad event posting code has just an assert > instead. The ClassPrepare and ClassLoad events also have a conditional return > in a case of temporary VTMS transition. > This update is to align the CFLH, ClassPrepare and ClassLoad events with all > other events in this area. > > Testing: > - TBD: submit mach5 tiers 1-6 This pull request has now been integrated. Changeset: c4ff58b9 Author:Serguei Spitsyn URL: https://git.openjdk.org/jdk/commit/c4ff58b9bcfd08eae0623a648a837e08f25b3f9b Stats: 9 lines in 1 file changed: 2 ins; 2 del; 5 mod 8330146: assert(!_thread->is_in_any_VTMS_transition()) failed Reviewed-by: cjplummer, kevinw - PR: https://git.openjdk.org/jdk/pull/19054
Re: RFR: 8330146: assert(!_thread->is_in_any_VTMS_transition()) failed
On Thu, 2 May 2024 10:07:35 GMT, Serguei Spitsyn wrote: > Any event posting code except CFLH, ClassPrepare and ClassLoad events has a > conditional return in case if the event is posted during a VTMS transition. > The CFLH, ClassPrepare and ClassLoad event posting code has just an assert > instead. The ClassPrepare and ClassLoad events also have a conditional return > in a case of temporary VTMS transition. > This update is to align the CFLH, ClassPrepare and ClassLoad events with all > other events in this area. > > Testing: > - TBD: submit mach5 tiers 1-6 Thank you for review, Kevin! The fix is needed anyway independently of the "jdk.tracePinnedThreads" option. - PR Comment: https://git.openjdk.org/jdk/pull/19054#issuecomment-2102778109
Re: RFR: 8330146: assert(!_thread->is_in_any_VTMS_transition()) failed
On Thu, 2 May 2024 10:07:35 GMT, Serguei Spitsyn wrote: > Any event posting code except CFLH, ClassPrepare and ClassLoad events has a > conditional return in case if the event is posted during a VTMS transition. > The CFLH, ClassPrepare and ClassLoad event posting code has just an assert > instead. The ClassPrepare and ClassLoad events also have a conditional return > in a case of temporary VTMS transition. > This update is to align the CFLH, ClassPrepare and ClassLoad events with all > other events in this area. > > Testing: > - TBD: submit mach5 tiers 1-6 PING! Need one more review, please. - PR Comment: https://git.openjdk.org/jdk/pull/19054#issuecomment-2102593940
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Thu, 9 May 2024 01:00:52 GMT, Chris Plummer wrote:
>> Okay, thanks. Please, let me list the variants with some analysis.
>> We have 3 variants:
>> 1. Both monitors are leaf: Entering any monitor while a leaf monitor has
>> been entered is a violation (`LEAF RULE` violation). The order does not
>> matter. Any order is unacceptable.
>>
>> 2. A non-leaf monitor is being entered while a leaf monitor has been
>> entered: This is also a `LEAF RULE` violation. This violation is at the same
>> time always a "RANK ORDER VIOLATION" for the non-leaf monitor. My view is
>> there is no need to report this "RANK ORDER VIOLATION" as it always coexists
>> with the `LEAF RULE` violation for non-leaf monitors. But if you insist on
>> reporting it additionally then it is not a problem to check and report it in
>> the `assertOrderFailure()` function. It has all needed info available for it.
>>
>> 3. A non-leaf monitor is being entered while a non-leaf monitor has been
>> entered: It is a case+report for "RANK ORDER VIOLATION".
>>
>> There is one more variant:
>> 4. A leaf monitor is being entered while a non-leaf monitor has been
>> entered: It is never a violation, so this variant is excluded from the list
>> above.
>
> But 2 and 1 are very different. You can call them both leaf violations, but
> they are leaf violations for very different reasons, and 2 is more akin to a
> rank violation than a leaf violation.
>
> I'm reversing the ranks and reworking the loop a bit (both the comments and
> how the errors are reported). I'll try to post later tonight after testing is
> done.
This is updated function `assertOrderFailure()` to print "RANK ORDER" failure
for the case #2 above:
static void
assertOrderFailure(jthread thread, DebugRawMonitorRank rank, DebugRawMonitor*
dbg_monitor, char* msg)
{
char* threadName = getThreadName(thread);
tty_message("DebugRawMonitor %s failure: (%s: %d > %d) for thread (%s)",
msg, dbg_monitor->name, dbg_monitor->rank, rank, threadName);
if (rank < FIRST_LEAF_DEBUG_RAW_MONITOR &&
dbg_monitor ->rank >= FIRST_LEAF_DEBUG_RAW_MONITOR) {
tty_message("DebugRawMonitor RANK ORDER failure: (%s: %d > %d) for
thread (%s)",
msg, dbg_monitor->name, dbg_monitor->rank, rank,
threadName);
}
jvmtiDeallocate(threadName);
JDI_ASSERT(JNI_FALSE);
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1595383612
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Wed, 8 May 2024 22:41:45 GMT, Chris Plummer wrote:
>> I just wanted to say that Alex's suggestion is in a similar direction.
>> I do not see a big advantage to save just on creation of a few monitors. It
>> is not a scalability problem as their number is constant. Creation of the
>> monitors at initialization time is a simplification as we do not care about
>> synchronization at this phase. Also, it is easier to understand because
>> there is no interaction with other threads.
>
> The init code would have to know the name of each monitor since it is no
> longer being passed in. Something like the following might work:
>
> static DebugRawMonitor dbg_monitors[] = {
> { NULL, "JDWP VM_DEATH Lock", vmDeathLockForDebugLoop_Rank, NULL, 0 },
> { NULL, "JDWP Callback Block", callbackBlock_Rank, NULL, 0 },
> { NULL, "JDWP Callback Lock", callbackLock_Rank, NULL, 0 },
> ...
> };
>
> And then the init() function can iterate over the array and allocate the
> monitor for each entry. Note that this array needs to be kept in sync with
> the DebugRawMonitorRank enum (same entries and in the same order). I can
> assert during the initialization that the rank field for each entry is equal
> to its array index and that the array size is NUM_DEBUG_RAW_MONITORS.
Can we do as below? :
static DebugRawMonitor dbg_monitors[]; // initialized with all zeros by default
. . .
static void
createDebugRawMonitor(DebugRawMonitorRank rank, const char* name) {
dbg_monitors[rank] = debugMonitorCreate(rank, name);
}
static void
monitors_initialize() {
createDebugRawMonitor( vmDeathLockForDebugLoop_Rank, "JDWP VM_DEATH Lock");
createDebugRawMonitor( callbackBlock_Rank, "JDWP Callback Block");
createDebugRawMonitor( callbackLock_Rank, "JDWP Callback Lock");
. . .
}
The pair of functions `createDebugRawMonitor()+debugMonitorCreate()` can be
refactored into just one function.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594823032
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Wed, 8 May 2024 22:53:06 GMT, Chris Plummer wrote: >> Okay, thanks. >>> Then you have both a rank order violation and a violation for entering a >>> leaf monitor when you have already entered a leaf monitor. >> >> I kind of disagree with the second part of this statement. >> My understanding is that the second loop does a little bit different check: >> "Violation for entering ANY monitor when you have already entered a leaf >> monitor." >> Please, note that `rank` can be any rank including those with conditions: >> >>rank < FIRST_LEAF_DEBUG_RAW_MONITOR >>rank >= FIRST_LEAF_DEBUG_RAW_MONITOR >> >> It seems to me that if the second rule (in my edition) is violated then the >> first rule does not matter. >> It means that the first rule is for both ranks from the range: >> >>0 <= rank < FIRST_LEAF_DEBUG_RAW_MONITOR >> ``` > > Yes, the 2nd loop is a different check. That's why I said it also checks all > the leaf monitors "but for a different reason". Your two loops do not flag a > rank violation if both monitors are leafs, even if grabbed in the wrong > order. It only flags the leaf violation. Your two checks will always catch > any violation, it just a matter of whether my example (which is both a leaf > and a rank violation) is flagged as a leaf violation or a rank violation (or > even both could be indicated if we choose). Yours flags it as a leaf > violation. My code flags it as a rank violation. Mine could flag both > violations without any additional iterations. Your would need to iterate over > the leaf monitors twice to detect if there is both a rank and a leaf > violation. Okay, thanks. Please, let me list the variants with some analysis. We have 3 variants: 1. Both monitors are leaf: Entering any monitor while a leaf monitor has been entered is a violation (`LEAF RULE` violation). The order does not matter. Any order is unacceptable. 2. A non-leaf monitor is being entered while a leaf monitor has been entered: This is also a `LEAF RULE` violation. This violation is at the same time always a "RANK ORDER VIOLATION" for the non-leaf monitor. My view is there is no need to report this "RANK ORDER VIOLATION" as it always coexists with the `LEAF RULE` violation for non-leaf monitors. But if you insist on reporting it additionally then it is not a problem to check and report it in the `assertOrderFailure()` function. It has all needed info available for it. 3. A non-leaf monitor is being entered while a non-leaf monitor has been entered: It is a case+report for "RANK ORDER VIOLATION". There is one more variant: 4. A leaf monitor is being entered while a non-leaf monitor has been entered: It is never a violation, so this variant is excluded from the list above. - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594814115
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Wed, 8 May 2024 22:10:29 GMT, Chris Plummer wrote: >> In fact, I do not understand why reporting the "rank order" violation is >> important what "leaf rank order" is violated. I feel that I'm missing >> something. Let me think on it a little bit. > > If the following are all true > - you are entering is a leaf monitor > - the current thread has already entered a leaf monitor > - the rank of the monitor you are entering is lower than the rank of the > monitor already held > > Then you have both a rank order violation and a violation for entering a leaf > monitor when you have already entered a leaf monitor. My current > implementation will complain about the rank violation (although can easily be > made to instead complain about the leaf violation or complain about both). > Yours will complain about the leaf violation. To make yours instead complain > about the rank violation, the first loop needs to also check all the leaf > monitors. Since the 2nd loop also checks all the leaf monitors (but for a > different reason), this means iterating over the leaf monitors twice. > Complaining about both violations would also require iterating over the leaf > monitors twice. Okay, thanks. > Then you have both a rank order violation and a violation for entering a leaf > monitor when you have already entered a leaf monitor. I kind of disagree with the second part of this statement. My understanding is that the second loop does a little bit different check: "Violation for entering ANY monitor when you have already entered a leaf monitor." Please, note that `rank` can be any rank including those with conditions: rank < FIRST_LEAF_DEBUG_RAW_MONITOR rank >= FIRST_LEAF_DEBUG_RAW_MONITOR - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594776344
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Wed, 8 May 2024 20:34:01 GMT, Chris Plummer wrote: >>> The special popFrame check needs to go in the first loop only, so it >>> shouldn't be a problem or add any complexity that we don't already have. >> >> Sounds good. >> >>> One things to resolve is if we enter a regular monitor while holding a leaf >>> monitor, is this a "rank" failure or "leaf" failure. Currently in the code >>> it is a "rank" failure. Your change would make it a "leaf" failure. >> >> A "leaf" failure is more specific than a "rank order" failure, so it is >> better to report it first. Each "leaf" failure is also a "rank order" >> failure (AFAICS). >> >>> I'm not sure why you added the "i != rank" check with the leaf check. We >>> should never be re-entering a leaf monitor. The same >>> "dbg_monitor->ownerThread != NULL" check as in the first loop should be >>> used. >> >> You are right. This check is not needed and has to be removed. I was >> thinking a leaf monitor can be grabbed recursively. > >> A "leaf" failure is more specific than a "rank order" failure, so it is >> better to report it first. Each "leaf" failure is also a "rank order" >> failure (AFAICS). > > It's not just a matter of which is reported first. Even if you swap the order > of your two loops you get the same result. The problem is the "rank" loop > does not check if any of the leaf monitors are already held. I think to fix > that it would have to iterate up to NUM_DEBUG_RAW_MONITORS, which means there > is overlap with the range that the "leaf" loop iterators over. In fact, I do not understand why reporting the "rank order" violation is important what "leaf rank order" is violated. I feel that I'm missing something. Let me think on it a little bit. - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594725455
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Wed, 8 May 2024 19:09:18 GMT, Chris Plummer wrote: >> src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1117: >> >>> 1115: jrawMonitorID monitor; >>> 1116: const char* name; >>> 1117: DebugRawMonitorRank rank; >> >> Nit: Please, consider to add same comment for lines #1116-1117 as for the >> line #1119 below. >> It might be useful to move the field `ownerThread` before the other fields >> (not sure about the `monitor` field), so the same comment can be shared by >> multiple fields. > > I kind of don't want to distract from the point that ownerThread is protected > by the dbgRawMonitor, and that is the main purpose of dbgRawMonitor. Once you > have verified that ownerThread is the current thread, you can release the > dbgRawMonitor and access the rank and name fields safely. > > There is a race issue with all the fields during debugMonitorCreate() and > verifyMonitorRank(), which is why the debugMonitorCreate() must hold the > dbgRawMonitor(). However, there is not a race with the fields (other than > ownerThread) for the DebugRawMonitor passed into the debugMonitorXXX() APIs > because they are not called until the DebugRawMonitor is done being > initialized. So this means that debugMonitorXXX() can access the monitor, > name, and rank fields without holding dbgRawMonitor, because they are > guaranteed to be fully initialized by that point, and they don't change. > ownerThread does change, so you must access it while holding the > dbgRawMonitor. Technically speaking entryCount could also be changed by other > threads, but we don't access it until we know the current thread owns the > DebugRawMonitor, so it is safe to access (no other thread would modify or > access it). > > To put this another way, the debugMonitorXXX() APIs can safely access most of > the DebugRawMonitor fields for the DebugRawMonitor passed in because we know > they are initialized by this point and don't change. ownerThread (and to some > extent entryCount) are the exception. verifyMonitorRank() introduces > additional synchronization issues because it iterates over all > DebugRawMonitors, and some might be in the middle of creation, so some > synchronization with the creation code is needed. > > However, I now realize that if we initialized all the monitors up front, then > there would be no need to hold dbgRawMonitor during debugMonitorCreate(), > although this does not allow for any improvements in verifyMonitorRank() > because it still needs to hold the dbgRawMonitor due to accessing the > ownerThread field. No pressure, it is up to you. I just wonder if there is any room to make it a little bit more clear and simple. - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594587018
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Wed, 8 May 2024 19:22:31 GMT, Chris Plummer wrote: > The special popFrame check needs to go in the first loop only, so it > shouldn't be a problem or add any complexity that we don't already have. Sounds good. > One things to resolve is if we enter a regular monitor while holding a leaf > monitor, is this a "rank" failure or "leaf" failure. Currently in the code it > is a "rank" failure. Your change would make it a "leaf" failure. A "leaf" failure is more specific than a "rank order" failure, so it is better to report it first. Each "leaf" failure is also a "rank order" failure (AFAICS). > I'm not sure why you added the "i != rank" check with the leaf check. We > should never be re-entering a leaf monitor. The same > "dbg_monitor->ownerThread != NULL" check as in the first loop should be used. You are right. This check is not needed and has to be removed. I was thinking a leaf monitor can be grabbed recursively. - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594583974
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Wed, 8 May 2024 18:27:15 GMT, Chris Plummer wrote:
>> src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c line 66:
>>
>>> 64: debugLoop_initialize(void)
>>> 65: {
>>> 66: vmDeathLock = debugMonitorCreate(vmDeathLockForDebugLoop_Rank,
>>> "JDWP VM_DEATH Lock");
>>
>> Nit: Just a suggestion to consider (there was a similar suggestion from by
>> Alex)...
>> Can all the `debugMonitor's` created/initialized by one function
>> `debugMonitor_initialize()` called by util_initialize()? Then each monitor
>> consumer can cache needed `debugMonitor's` like this:
>>
>> vmDeathLock = getDebugMonitor(vmDeathLockForDebugLoop_Rank);
>
> I think you are suggesting that all monitors be created up front during
> util_intialize(). The debug agent in the past has always created them lazily
> (and sometimes some of them end up never being created because they are not
> needed). I don't really see a big advantage in creating them all up front.
>
> Alex's suggestion was a very different one, and was just a suggestion to
> initialize all the DebugRawMonitor ranks up front rather than when the
> jMonitorID is created, and the reason for the suggestion was to avoid having
> to grab the dbgRawMonitor when setting the rank, but I wasn't convinced that
> it actually allowed you to not grab the dbgRawMonitor.
I just wanted to say that Alex's suggestion is in a similar direction.
I do not see a big advantage to save just on creation of a few monitors. It is
not a scalability problem as their number is constant. Creation of the monitors
at initialization time is a simplification as we do not care about
synchronization at this phase. Also, it is easier to understand because there
is no interaction with other threads.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594571559
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Tue, 7 May 2024 18:53:23 GMT, Chris Plummer wrote:
>> This PR adds ranked monitor support to the debug agent. The debug agent has
>> a large number of monitors, and it's really hard to know which order to grab
>> them in, and for that matter which monitors might already be held at any
>> given moment. By imposing a rank on each monitor, we can check to make sure
>> they are always grabbed in the order of their rank. Having this in place
>> when I was working on
>> [JDK-8324868](https://bugs.openjdk.org/browse/JDK-8324868) would have made
>> it much easier to detect a deadlock that was occuring, and the reason for
>> it. That's what motivated me to do this work
>>
>> There were 2 or 3 minor rank issues discovered as a result of these changes.
>> I also learned a lot about some of the more ugly details of the locking
>> support in the process.
>>
>> Tested with the following on all supported platforms and with virtual
>> threads:
>>
>> com/sun/jdi
>> vmTestbase/nsk/jdi
>> vmTestbase/nsk/jdb
>> vmTestbase/nsk/jdwp
>>
>> Still need to run tier2 and tier5.
>>
>> Details of the changes follow in the first comment.
>
> Chris Plummer has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Fix jcheck extra whitespace.
> - Fix comment typo.
src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c line 66:
> 64: debugLoop_initialize(void)
> 65: {
> 66: vmDeathLock = debugMonitorCreate(vmDeathLockForDebugLoop_Rank, "JDWP
> VM_DEATH Lock");
Nit: Just a suggestion to consider...
Can all the `debugMonitor's` created by one function
`debugMonitor_initialize()` called by initialize() at start . Then each
monitor consumer can cache needed `debugMonitor's` like this:
vmDeathLock = getDebugMonitor(vmDeathLockForDebugLoop_Rank);
src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1117:
> 1115: jrawMonitorID monitor;
> 1116: const char* name;
> 1117: DebugRawMonitorRank rank;
Nit: Please, consider to add same comment for lines #1116-1117 as for the line
#1119 below.
It might be useful to move the field `ownerThread` before the other fields (not
sure about the `monitor` field), so the same comment can be shared by multiple
fields.
src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1297:
> 1295: // We must hold the dbgRawMonitor when calling verifyMonitorRank()
> 1296:
> 1297: // Iterate over all the monitors and makes sure we don't already
> hold one that
Nit: Typo: "makes sure" => "make sure".
src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1343:
> 1341: }
> 1342: }
> 1343: }
Nit: I'd suggest to consider two separate loops instead of one, something like
below:
static void
assertOrderFailure(jthread thread, DebugRawMonitorRank rank, DebugRawMonitor*
dbg_monitor, char* msg)
{
char* threadName = getThreadName(thread);
tty_message("DebugRawMonitor %s failure: (%s: %d > %d) for thread (%s)",
msg, dbg_monitor->name, dbg_monitor->rank, rank, threadName);
jvmtiDeallocate(threadName);
JDI_ASSERT(JNI_FALSE);
}
static void
verifyMonitorRank(JNIEnv *env, DebugRawMonitorRank rank, jthread thread)
{
DebugRawMonitorRank i;
// 1. Verify the held monitor's rank is lower than the rank of the monitor
we are trying to enter.
for (i = rank + 1; i < FIRST_LEAF_DEBUG_RAW_MONITOR; i++) {
DebugRawMonitor* dbg_monitor = _monitors[i];
if (dbg_monitor->ownerThread != NULL &&
isSameObject(env, dbg_monitor->ownerThread, thread)) {
// the lower ranked monitor is already owned by this thread
assertOrderFailure(thread, rank, dbg_monitor, "RANK ORDER");
}
}
// 2. Verify there are no other leaf monitors owned but this thread.
for (i = FIRST_LEAF_DEBUG_RAW_MONITOR; i < NUM_DEBUG_RAW_MONITORS; i++) {
DebugRawMonitor* dbg_monitor = _monitors[i];
if (i != rank && isSameObject(env, dbg_monitor->ownerThread, thread)) {
// the leaf ranked monitor is already owned by this thread
assertOrderFailure(thread, rank, dbg_monitor, "LEAF RANK");
}
}
I hope, this can be tweaked to adopt the `popFrame` locks correction.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594254039
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594328246
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594257313
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594285396
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Tue, 7 May 2024 18:54:37 GMT, Chris Plummer wrote: > What do you think about the terminology usage of "rank". I slightly prefer to have the same rank order as Hotspot uses. At the same time, I'm not sure if it won't cause any issues/difficulties. But I'd give it a try. - PR Comment: https://git.openjdk.org/jdk/pull/19044#issuecomment-2100849353
Re: RFR: 8308033: The jcmd thread dump related tests should test virtual threads [v3]
On Thu, 2 May 2024 03:57:25 GMT, Jaikiran Pai wrote: >> Can I please get a review of these test-only changes which proposes to >> remove the `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` and >> `test/jdk/sun/tools/jstack/BasicJStackTest.java` tests from >> `ProblemList-Virtual.txt`? >> >> When jtreg was enhanced to allow running the tests from within a virtual >> (main) thread, some tests were problem listed since they either were failing >> at that time or the test code would require additional work to make it work >> when the current thread is a virtual thread. The >> `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` and >> `test/jdk/sun/tools/jstack/BasicJStackTest.java` are 2 such threads which >> require special handling when the current thread is a virtual thread. >> >> `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` has been updated to >> use a different command to dump stacktraces when the test runs in a virtual >> thread. I went back and looked at the original issue for which this test was >> introduced and based on that, using a different command to dump the >> stacktraces shouldn't impact what the test was originally intended to test. >> >> `test/jdk/sun/tools/jstack/BasicJStackTest.java` has been updated to be >> skipped when the current thread is the virtual thread. This is because the >> test exercises the `jstack` tool which doesn't print stacktraces of virtual >> threads and thus the test isn't applicable for virtual threads. >> >> I've run these tests with this change, both with platform threads and >> virtual threads in our CI and the tests complete without failures. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Chris' review - DaemonThreadTest.java, no need for checking if thread is > virtual This looks good. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19016#pullrequestreview-2044949004
Re: RFR: 8331466: Problemlist serviceability/dcmd/gc/RunFinalizationTest.java on macos-aarch64 [v2]
On Wed, 8 May 2024 01:19:05 GMT, SendaoYan wrote: >> Hi, >> GHA >> [runner](https://github.com/sendaoYan/jdk-ysd/actions/runs/8881868940/job/24386063136) >> shows that serviceability/dcmd/gc/RunFinalizationTest.java intermittent >> fail on macos-aarch64. The testcase has been problemlisted on >> linux-all,windows-x64,aix-ppc64. I think we should add the problemlist with >> macos-aarch64. >> >> Only change the ProblemList, no risk. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > problemlist serviceability/dcmd/gc/RunFinalizationTest.java on generic-all Marked as reviewed by sspitsyn (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19033#pullrequestreview-2044866363
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v2]
On Mon, 6 May 2024 21:22:05 GMT, Chris Plummer wrote: >> This PR adds ranked monitor support to the debug agent. The debug agent has >> a large number of monitors, and it's really hard to know which order to grab >> them in, and for that matter which monitors might already be held at any >> given moment. By imposing a rank on each monitor, we can check to make sure >> they are always grabbed in the order of their rank. Having this in place >> when I was working on >> [JDK-8324868](https://bugs.openjdk.org/browse/JDK-8324868) would have made >> it much easier to detect a deadlock that was occuring, and the reason for >> it. That's what motivated me to do this work >> >> There were 2 or 3 minor rank issues discovered as a result of these changes. >> I also learned a lot about some of the more ugly details of the locking >> support in the process. >> >> Tested with the following on all supported platforms and with virtual >> threads: >> >> com/sun/jdi >> vmTestbase/nsk/jdi >> vmTestbase/nsk/jdb >> vmTestbase/nsk/jdwp >> >> Still need to run tier2 and tier5. >> >> Details of the changes follow in the first comment. > > Chris Plummer has updated the pull request incrementally with eight > additional commits since the last revision: > > - Minor comment fix in debugMonitorCreate() > - Don't do anything in assertIsCurrentThread() if asserts are disabled > - Print threads names instead of addresses in assertIsCurrentThread() > - Renamed thread -> savedOwnerThread and entryCount -> savedEntryCount. > - pass already fetched current_thread to assertIsCurrentThread() > - dbgRawMonitor reference should be dbg_monitor->monitor in dumpRawMonitors() > - Free localrefs allocated by GetThreadInfo(). > - Optimize loop iterations during rank verification. src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1203: > 1201: > 1202: // Need to lock during initialization so verifyMonitorRank() can be > guaranteed that > 1203: // if the monitor field is not NULL, then the monitor if fully > initialized. Nit: It looks like a typo: "the monitor if fully" => "the monitor is fully" - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1591680028
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v2]
On Mon, 6 May 2024 21:22:05 GMT, Chris Plummer wrote: >> This PR adds ranked monitor support to the debug agent. The debug agent has >> a large number of monitors, and it's really hard to know which order to grab >> them in, and for that matter which monitors might already be held at any >> given moment. By imposing a rank on each monitor, we can check to make sure >> they are always grabbed in the order of their rank. Having this in place >> when I was working on >> [JDK-8324868](https://bugs.openjdk.org/browse/JDK-8324868) would have made >> it much easier to detect a deadlock that was occuring, and the reason for >> it. That's what motivated me to do this work >> >> There were 2 or 3 minor rank issues discovered as a result of these changes. >> I also learned a lot about some of the more ugly details of the locking >> support in the process. >> >> Tested with the following on all supported platforms and with virtual >> threads: >> >> com/sun/jdi >> vmTestbase/nsk/jdi >> vmTestbase/nsk/jdb >> vmTestbase/nsk/jdwp >> >> Still need to run tier2 and tier5. >> >> Details of the changes follow in the first comment. > > Chris Plummer has updated the pull request incrementally with eight > additional commits since the last revision: > > - Minor comment fix in debugMonitorCreate() > - Don't do anything in assertIsCurrentThread() if asserts are disabled > - Print threads names instead of addresses in assertIsCurrentThread() > - Renamed thread -> savedOwnerThread and entryCount -> savedEntryCount. > - pass already fetched current_thread to assertIsCurrentThread() > - dbgRawMonitor reference should be dbg_monitor->monitor in dumpRawMonitors() > - Free localrefs allocated by GetThreadInfo(). > - Optimize loop iterations during rank verification. Chris, I'm looking at this fix. - PR Comment: https://git.openjdk.org/jdk/pull/19044#issuecomment-2097141567
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v2]
On Mon, 6 May 2024 21:31:02 GMT, Chris Plummer wrote: >> I originally started at `rank`, but then when I added the 2nd of the two >> checks below I switched to 0, but your min suggestion should be optimal. > > Fixed. I was about to post a similar comment but found it has been already fixed in the update. :) - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1591618748
Re: RFR: 8331466: Problemlist serviceability/dcmd/gc/RunFinalizationTest.java on macos-aarch64
On Wed, 1 May 2024 14:14:22 GMT, SendaoYan wrote: > Hi, > GHA > [runner](https://github.com/sendaoYan/jdk-ysd/actions/runs/8881868940/job/24386063136) > shows that serviceability/dcmd/gc/RunFinalizationTest.java intermittent fail > on macos-aarch64. The testcase has been problemlisted on > linux-all,windows-x64,aix-ppc64. I think we should add the problemlist with > macos-aarch64. > > Only change the ProblemList, no risk. Looks okay to me. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19033#pullrequestreview-2039319669
Re: RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly [v4]
On Fri, 3 May 2024 01:54:24 GMT, Alex Menkov wrote: >> Some cleanup related to JvmtiEnvBase::get_threadOop_and_JavaThread method >> >> Testing: tier1-6 > > Alex Menkov has updated the pull request incrementally with three additional > commits since the last revision: > > - update > - Revert "renamed current_thread to current" > >This reverts commit d5d614bcf0861466acd695296e974d2253f84c9f. > - Revert "renamed current_thread tp current" > >This reverts commit 4602632221044aa754a1bc8d11e7a3e9a0092590. Looks good. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18986#pullrequestreview-2039319268
Re: RFR: 8330534: Update nsk/jdwp tests to use driver instead of othervm
On Wed, 17 Apr 2024 20:19:49 GMT, Leonid Mesnik wrote: > The jdwp tests use debugger and debugee. There is no goal to execute debugger > part with all VM flags, they are needed to be used with debugee VM only. > The change is all tests is to don't use System.exit() and use 'driver' > instead of othervm. > test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeBinder.java > is updated to correctly set classpath for debugee This looks okay in general. There is a missing space in half of files in method `run()` parameters noticed by Andrei. Also, Chris had some concerns about converting to the `driver` mode. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18826#pullrequestreview-2039316239
Re: RFR: 8330534: Update nsk/jdwp tests to use driver instead of othervm
On Mon, 22 Apr 2024 11:21:15 GMT, Andrey Turbanov wrote:
>> The jdwp tests use debugger and debugee. There is no goal to execute
>> debugger part with all VM flags, they are needed to be used with debugee VM
>> only.
>> The change is all tests is to don't use System.exit() and use 'driver'
>> instead of othervm.
>> test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeBinder.java
>> is updated to correctly set classpath for debugee
>
> test/hotspot/jtreg/vmTestbase/nsk/jdwp/ThreadReference/Interrupt/interrupt001.java
> line 93:
>
>> 91: */
>> 92: public static void main (String argv[]) {
>> 93: int result = run(argv,System.out);
>
> Suggestion:
>
> int result = run(argv, System.out);
Yes, this missing space is all over this fix. Need to fix this formatting.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18826#discussion_r1589950014
Re: RFR: 8330146: assert(!_thread->is_in_any_VTMS_transition()) failed
On Thu, 2 May 2024 10:07:35 GMT, Serguei Spitsyn wrote: > Any event posting code except CFLH, ClassPrepare and ClassLoad events has a > conditional return in case if the event is posted during a VTMS transition. > The CFLH, ClassPrepare and ClassLoad event posting code has just an assert > instead. The ClassPrepare and ClassLoad events also have a conditional return > in a case of temporary VTMS transition. > This update is to align the CFLH, ClassPrepare and ClassLoad events with all > other events in this area. > > Testing: > - TBD: submit mach5 tiers 1-6 Thank you for review, Chris! - PR Comment: https://git.openjdk.org/jdk/pull/19054#issuecomment-2093703364
