On Sat, 20 May 2023 00:21:04 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> This enhancement adds ForceEarlyReturnXXX support for virtual threads. The >> spec defines minimal support that the JVMTI ForceEarlyReturnXXX can be used >> for a virtual thread suspended an an event. >> Actually, the ForceEarlyReturnXXX can supports suspended and mounted virtual >> threads. >> >> CSR (approved): https://bugs.openjdk.org/browse/JDK-8308401 add >> ForceEarlyReturn support for virtual threads >> >> Testing: >> New test was developed: serviceability/vthread/ForceEarlyReturnTest. >> Submitted mach5 tiers 1-6 are good. >> TBD: rerun mach5 tiers 1-6 at the end of review again if necessary. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > minor tweak in libForceEarlyReturnTest.cpp The overall looks good. A couple of comments inline. Although, test is very similar to popframe tests, seems merging code doesn't give a lot of benefits, The overall looks good. A couple of comments inline. Although, test is very similar to popframe tests, seems merging code doesn't give a lot of benefits, src/hotspot/share/prims/jvmtiEnvBase.cpp line 2042: > 2040: return err; > 2041: } > 2042: bool is_virtual = thread_obj != nullptr && > thread_obj->is_a(vmClasses::BaseVirtualThread_klass()); Does it make sense to reduce code duplication by moving these checks from forceearlyreturn and popframe code into a separate method? src/hotspot/share/prims/jvmtiEnvBase.cpp line 2078: > 2076: return; /* JVMTI_ERROR_THREAD_NOT_ALIVE (default) */ > 2077: } > 2078: if (!self) { Can't we have any racing by removing this check? We are checking thread state before handshake operation, but it is changed before thread start execution of this handshake? ------------- Changes requested by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14067#pullrequestreview-1435420629 PR Review: https://git.openjdk.org/jdk/pull/14067#pullrequestreview-1435421068 PR Review Comment: https://git.openjdk.org/jdk/pull/14067#discussion_r1199629139 PR Review Comment: https://git.openjdk.org/jdk/pull/14067#discussion_r1199629397
