On Fri, 31 May 2024 23:55:20 GMT, Serguei Spitsyn <[email protected]> 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()
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
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
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);` ?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1623060427
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1623061692
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1623061890
