On Wed, 10 Apr 2024 04:21:23 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> The internal JVM TI JvmtiHandshake and JvmtiUnitedHandshakeClosure classes
>> were introduced in the JDK 22 to unify/simplify the JVM TI functions
>> supporting implementation of the virtual threads. This enhancement is to
>> refactor the JVM TI internal functions
>> JvmtiEnvThreadState::reset_current_location on the base of JvmtiHandshake
>> and JvmtiUnitedHandshakeClosure classes.
>>
>> Testing:
>> - Ran mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: refactored to get rid of overloaded doit functions
Looks good to me, just a few comments.
src/hotspot/share/prims/jvmtiEnvThreadState.cpp line 307:
> 305: if (!JvmtiEnvBase::is_vthread_alive(target_h())) {
> 306: return; // _completed remains false.
> 307: }
Do we need this? We already do this check in JvmtiHandshake::execute().
src/hotspot/share/prims/jvmtiEnvThreadState.cpp line 309:
> 307: }
> 308: ResourceMark rm;
> 309: javaVFrame *jvf = JvmtiEnvBase::get_vthread_jvf(target_h());
This method already handles both mounted and unmounted case, so do we need the
first conditional above?
src/hotspot/share/prims/jvmtiEnvThreadState.cpp line 367:
> 365: GetCurrentLocationClosure op;
> 366: JvmtiHandshake::execute(&op, &tlh, thread, thread_h);
> 367:
Seems we are missing a JvmtiVTMSTransitionDisabler.
-------------
PR Review: https://git.openjdk.org/jdk/pull/18630#pullrequestreview-1994658919
PR Review Comment: https://git.openjdk.org/jdk/pull/18630#discussion_r1561295746
PR Review Comment: https://git.openjdk.org/jdk/pull/18630#discussion_r1561298952
PR Review Comment: https://git.openjdk.org/jdk/pull/18630#discussion_r1561300541
