On Thu, 30 May 2024 01:03:01 GMT, Alex Menkov <amen...@openjdk.org> 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
