On Wed, 29 Apr 2026 16:32:12 GMT, Patricio Chilano Mateo <[email protected]> wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: (1) remove unneeded assert, (2) simplify one statement in >> JvmtiEnvBase::set_frame_pop > > src/hotspot/cpu/x86/interp_masm_x86.cpp line 1628: > >> 1626: push(state); >> 1627: call_VM(noreg, >> 1628: CAST_FROM_FN_PTR(address, >> InterpreterRuntime::post_method_exit)); > > I see `JvmtiExport::can_post_interpreter_events()` will be set for agents > loaded at start-up. So even if we don’t enable notifications for method exit > or frame pop events we will still make this VM call on method exit. Maybe we > could keep the interpreter only mode check for method exit cases, and also > add a new check for frame pop events? We could have something similar to > `check_and_handle_earlyret`, where we check for the `JvmtiThreadState` first > and then another field which will be set when there is at least one frame pop > request. Given that there could be multiple environments, a simple solution > could be to set a boolean in the `JvmtiThreadState` when we set a frame pop > notification request, and then clearing it in `post_method_exit` if there are > no requests left. Thank you for this suggestion! Yes, this makes sense to do. However, it would be nice to separate it from this PR. I'll file a performance bug if you are okay with it. The performance impact should not be that big in general while it is still important. It is because: - it might impact the JDWP agent and a couple of JVMTI agents that use debugging features like `FramePop` events; the Java agents are not impacted (as Alan mentioned) - only the `notify_method_exit` interpreter chunk is impacted, so it is already the interpreted slow path ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28407#discussion_r3202566749
