On Tue, 16 May 2023 08:12:21 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
> This enhancement adds `PopFrame` support for virtual threads. The spec > defines minimal support that the JVMTI `PopFrame` can be used for a virtual > thread suspended an an event. > Actually, the `PopFrame` can supports suspended and mounted virtual threads. > > CSR (approved): https://bugs.openjdk.org/browse/JDK-8308001: add PopFrame > support for virtual threads > > Testing: > New test was developed: `serviceability/vthread/PopFrameTest`. > Submitted mach5 tiers 1-6 are good. > TBD: rerun mach5 tiers 1-6 at the end of review again if necessary. It is also unclear how popFrame works if the the underlying frame is jdk.internal.vm.Continuation.enterSpecial(). Would it just return some error? src/hotspot/share/prims/jvmtiEnv.cpp line 1886: > 1884: jvmtiError err = get_threadOop_and_JavaThread(tlh.list(), thread, > &java_thread, &thread_obj); > 1885: > 1886: bool is_virtual = thread_obj != nullptr && > thread_obj->is_a(vmClasses::BaseVirtualThread_klass()); I think it would be better to check 'err' and try to handle the error before using java_thread and thread_obj. src/hotspot/share/prims/jvmtiEnv.cpp line 1896: > 1894: } > 1895: } else { > 1896: if (java_thread != current_thread && !java_thread->is_suspended() && This branch checks the state when the thread is platform OR current, that logic seems a little bit messy. Would not be better to clearly separate virtual and platform threads verification? (Also, it is unclear, we need to check platform threads here now). Might be some comments are needed? test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/PopFrameTest.java line 62: > 60: static final int FAILED = 2; > 61: > 62: static void log(String str) { System.out.println(str); } Better to flush system.out after each print. test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/PopFrameTest.java line 127: > 125: resumeThread(testTaskThread); > 126: testTask.clearDoLoop(); > 127: testTask.sleep(5); Why sleep is needed here? test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/PopFrameTest.java line 152: > 150: > 151: log("\nMain #B.3: unsuspended, call PopFrame on own thread"); > 152: ensureAtBreakpoint(); Am I understand correctly, that test expect here to pop frame and immediately get to the same breakpoint? test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/PopFrameTest.java line 154: > 152: ensureAtBreakpoint(); > 153: notifyAtBreakpoint(); > 154: testTask.sleep(5); Why sleep is needed here? test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/PopFrameTest.java line 222: > 220: } > 221: > 222: // Method is blocked on entering a synchronized statement. Not sure I see where this method is blocked. test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/PopFrameTest.java line 241: > 239: } > 240: > 241: // This method uses PoopFrame on its own thread. It is expected > to succeed. Isn't OPAQUE_FRAME expected? test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/libPopFrameTest.cpp line 49: > 47: LOG("Breakpoint: In method TestTask.B(): before sync section enter\n"); > 48: > 49: err = jvmti->RawMonitorEnter(monitor); You could use RawMonitorLocker instead: { RawMonitorLocker rml(jvmti, jni, monitor); bp_sync_reached = true; rml.wait(); } test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/libPopFrameTest.cpp line 62: > 60: > 61: err = jvmti->PopFrame(thread); > 62: LOG("Main: popFrame: PopFrame returned code: %s (%d)\n", > TranslateError(err), err); check_jvmti_status prints return code and translated error if fails. So this line is not needed, test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/libPopFrameTest.cpp line 181: > 179: LOG("Main: notifyAtBreakpoint\n"); > 180: > 181: err = jvmti->RawMonitorEnter(monitor); You could use RawMonitorLocker rml(jvmti, jni, monitor); rml.notify_all(); ------------- Changes requested by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14002#pullrequestreview-1429501761 PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195729952 PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195738621 PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195778145 PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195785775 PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195785443 PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195785817 PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195781893 PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195782744 PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195759443 PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195772530 PR Review Comment: https://git.openjdk.org/jdk/pull/14002#discussion_r1195757356
