On Tue, 6 Jun 2023 13:37:03 GMT, Serguei Spitsyn <[email protected]> wrote:
>> The `GetThreadListStackTraces` returns `JVMTI_THREAD_STATE_RUNNABLE` for a
>> VirtualThread blocked on a monitor when called for more than one thread.
>> When called for a single VirtualThread it correctly returns a state that
>> includes the `JVMTI_THREAD_STATE_BLOCKED_ON_MONITOR_ENTER` flag.
>> The `VM_GetThreadListStackTraces::doit` should call the
>> `get_threadOop_and_JavaThread` instead of
>> `cv_external_thread_to_JavaThread`. But the `get_threadOop_and_JavaThread`
>> has a check for the current thread by comparing with the
>> JavaThread::current() which does not work for a `VM_op`. Some refactoring of
>> the `get_threadOop_and_JavaThread` was made to make it working for a `VM_op`.
>> Also, a minor bug in the `GetSingleStackTraceClosure::do_thread()` was
>> discovered during testing.
>>
>> The list of changes is:
>> - minor refactoring of the function`get_threadOop_and_JavaThread`: added an
>> overloaded version of this function with the extra parameter `JavaThread*
>> cur_thread`. It is called instead of
>> `JvmtiExport::cv_external_thread_to_JavaThread` from the
>> `VM_GetThreadListStackTraces::doit`.
>> - `GetSingleStackTraceClosure::do_thread()`: The use of `jt->threadObj()` is
>> replaced with the `JNIHandles::resolve_external_guard(_jthread)`.
>> - added new test to provide needed coverage:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/ThreadListStackTracesTest`
>>
>> Testing:
>> - ran new test:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/ThreadListStackTracesTest`
>> - TBD: tiers 1-6 (all are good)
>
> Serguei Spitsyn has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - fixed typo in a comment in jvmtiEnvBase.cpp
> - nit: restored one comment as was before
Changes requested by cjplummer (Reviewer).
test/hotspot/jtreg/serviceability/jvmti/vthread/ThreadListStackTracesTest/ThreadListStackTracesTest.java
line 63:
> 61: public void run() {
> 62: log("TestTask.run()");
> 63: }
I think this should be an abstract method.
test/hotspot/jtreg/serviceability/jvmti/vthread/ThreadListStackTracesTest/ThreadListStackTracesTest.java
line 106:
> 104: final Thread.State expState = Thread.State.WAITING;
> 105: reentrantLock.lock();
> 106: String name = "ObjectMonitorTestTask";
Should be "ReentrantLockTestTask"
test/hotspot/jtreg/serviceability/jvmti/vthread/ThreadListStackTracesTest/libThreadListStackTracesTest.cpp
line 35:
> 33: extern "C" {
> 34:
> 35: JNIEXPORT jint JNICALL
> Java_ThreadListStackTracesTest_getStateSingle(JNIEnv* jni, jclass clazz,
> jthread vthread) {
I'd suggest splitting into 2 lines just like
Java_ThreadListStackTracesTest_getStateMultiple() for the sake of consistency
and being able to more easily compare the two.
-------------
PR Review: https://git.openjdk.org/jdk/pull/14326#pullrequestreview-1466112714
PR Review Comment: https://git.openjdk.org/jdk/pull/14326#discussion_r1220365810
PR Review Comment: https://git.openjdk.org/jdk/pull/14326#discussion_r1220367970
PR Review Comment: https://git.openjdk.org/jdk/pull/14326#discussion_r1220361274
