On Thu, 21 May 2026 16:36:37 GMT, Serguei Spitsyn <[email protected]> wrote:
>> This change fixes a long standing performance issue related to the debugger >> single stepping that is using JVMTI `FramePop` events as a part of step over >> handling. The performance issue is that the target thread continues its >> execution in very slow `interp-only` mode in a context of frame marked for >> `FramePop` notification with the JVMTI `NotifyFramePop`. It includes other >> method calls recursively upon a return from the frame. >> >> This fix is to avoid enforcing the `interp-only` execution mode for threads >> when `FramePop` events are enabled with the JVMTI >> `SetEventNotificationMode()`. Instead, the target frame has been deoptimized >> and kept interpreted by disabling `OSR` optimization by the function >> `InterpreterRuntime::frequency_counter_overflow_inner()`. (Big thanks to >> @fisk for this suggestion!) Additionally, some tweaks are applied in several >> places where the `java_thread->is_interp_only_mode()` is checked. >> The other details will be provided in the first PR request comment. >> >> Testing: >> - test `serviceability/jvmti/vthread/ThreadStateTest` was updated to >> provide some extra test coverage >> - submitted mach5 tiers 1-6 >> >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > remove trailing spaces from one file Thanks for the updates Serguei! A few comments below. src/hotspot/cpu/x86/interp_masm_x86.cpp line 1621: > 1619: > 1620: movl(rdx, Address(rdx, JvmtiThreadState::frame_pop_cnt_offset())); > 1621: movl(rcx, Address(rthread, JavaThread::interp_only_mode_offset())); > // can we use rcx too? Comment can be removed. src/hotspot/share/prims/jvmtiEventController.cpp line 105: > 103: static const jlong EXCEPTION_BITS = EXCEPTION_THROW_BIT | > EXCEPTION_CATCH_BIT; > 104: static const jlong INTERP_EVENT_BITS = SINGLE_STEP_BIT | > METHOD_ENTRY_BIT | METHOD_EXIT_BIT | > 105: FRAME_POP_BIT | FIELD_ACCESS_BIT | > FIELD_MODIFICATION_BIT; I see `can_generate_frame_pop_events` is still used when setting `JvmtiExport::_can_post_interpreter_events` in `JvmtiManageCapabilities::update()`. Should we fix that? In that case the check in `InterpreterMacroAssembler::notify_method_exit` should be `JvmtiExport::can_post_interpreter_events()` || `JvmtiExport::can_post_frame_pop`. src/hotspot/share/prims/jvmtiThreadState.hpp line 252: > 250: void incr_frame_pop_cnt() { > 251: assert(Threads::number_of_threads() == 0 || > JvmtiThreadState_lock->is_locked(), "sanity check"); > 252: AtomicAccess::inc(&_frame_pop_cnt); How about adding: `assert(_frame_pop_cnt > 0, "Unexpected count: %d", _frame_pop_cnt);` src/hotspot/share/prims/jvmtiThreadState.hpp line 257: > 255: void decr_frame_pop_cnt() { > 256: assert(Threads::number_of_threads() == 0 || > JvmtiThreadState_lock->is_locked(), "sanity check"); > 257: AtomicAccess::dec(&_frame_pop_cnt); How about adding: `assert(_frame_pop_cnt >= 0, "Unexpected count: %d", _frame_pop_cnt);` Also for `decr_frame_pop_cnt` below. ------------- PR Review: https://git.openjdk.org/jdk/pull/28407#pullrequestreview-4338830588 PR Review Comment: https://git.openjdk.org/jdk/pull/28407#discussion_r3282810966 PR Review Comment: https://git.openjdk.org/jdk/pull/28407#discussion_r3282883203 PR Review Comment: https://git.openjdk.org/jdk/pull/28407#discussion_r3282872323 PR Review Comment: https://git.openjdk.org/jdk/pull/28407#discussion_r3282873988
