RFR: 8352251: Implement JEP 518: JFR Cooperative Sampling [v23]

[Erik Österlund]

On Thu, 15 May 2025 16:45:16 GMT, Markus Grönlund <mgronlun at openjdk.org> wrote:

>> Greetings,
>> 
>> This is the implementation of JEP [JDK-8350338 Cooperative JFR Sampling](https://bugs.openjdk.org/browse/JDK-8350338).
>> 
>> Implementations in this change set are provided and have been tested on the following platforms:
>> 
>> - windows-x64
>> - windows-x64-debug
>> - linux-x64
>> - linux-x64-debug
>> - macosx-x64
>> - macosx-x64-debug
>> - linux-aarch64
>> - linux-aarch64-debug
>> - macosx-aarch64
>> - macosx-aarch64-debug
>> 
>> Testing: tier1-6, jdk_jfr, stress testing.
>> 
>> Platform porters note:
>> Some platform-specific code needs to be provided, mainly in the interpreter. Take a look at the following files for changes:
>> 
>> - src/hotspot/cpu/x86/frame_x86.cpp
>> - src/hotspot/cpu/x86/interp_masm_x86.cpp
>> - src/hotspot/cpu/x86/interp_masm_x86.hpp
>> - src/hotspot/cpu/x86/javaFrameAnchor_x86.hpp
>> - src/hotspot/cpu/x86/macroAssembler_x86.cpp
>> - src/hotspot/cpu/x86/macroAssembler_x86.hpp
>> - src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
>> - src/hotspot/cpu/x86/templateTable_x86.cpp
>> - src/hotspot/os_cpu/linux_x86/javaThread_linux_x86.hpp
>> 
>> Thanks
>> Markus
>
> Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision:
> 
>   growable array constructur without default initialization

Changes requested by eosterlund (Reviewer).

src/hotspot/cpu/aarch64/interp_masm_aarch64.cpp line 687:

> 685:   Label slow_path;
> 686:   Label fast_path;
> 687:   safepoint_poll(slow_path, this_fp, true /* at_return */, false /* acquire */, false /* in_nmethod */);

The way you moved the polling until after unwinding worries me a little bit.

First of all, you have to change the InterpreterRuntime::safepoint_poll and InterpreterRuntime::at_unwind functions that call StackWatermarkSet::before_unwind to call after_unwind instead, to better reflect what is happening now. That's doable and should be straight forward so that's good.

What I'm more worried about though, is what the JVMTI implications are. When single stepping in the debugger, it would seem that we now stop after unwinding instead of before. That seems like a change in debugger behaviour? I worry that previously you could see locals in the debugger before exiting while now you can't. I also have dark memories of seeing method exit JVMTI event assembly code that crops the expression stack assuming it belongs to the frame we are just returning from because that's how it worked. With fun code trying to deal with the oop maps being wrong across the event. Have you run any JVMTI method exit event tests?

-------------

PR Review: https://git.openjdk.org/jdk/pull/24296#pullrequestreview-2845067455
PR Review Comment: https://git.openjdk.org/jdk/pull/24296#discussion_r2092023295