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

[Erik Österlund]

On Mon, 12 May 2025 18:49:34 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:
> 
>   tiny adjustments

Changes requested by eosterlund (Reviewer).

src/hotspot/share/jfr/periodic/sampling/jfrSampleRequest.cpp line 195:

> 193:     return false;
> 194:   }
> 195:   if (is_valid_interpreter_frame(request, jt)) {

I don't know how I feel about this kind of "quacks like an interpreter frame" check, if that's then used to 100% trust, as opposed to 99.999% guessed, that we have found a real parsable interpreter frame. If the quacking song sounds right, we can safely retrieve the Method* and fully trust it to have been an actual method that the interpreter is in the middle of executing, instead of just unexpected bit noise that was really really good at quacking like a valid interpreter frame. It might be more okay if the safe stack walk from the next safepoint poll validates that we found the same frame and it had the same method, and hence only choose to trust the bcp from the quacking sounds of the signal handler. If it's wrong, then we might get a confusing sample with an inaccurate bci. But that's better than a crash.

Regardless of the solution domain, I think this looks like a problem worth fixing.

src/hotspot/share/runtime/safepointMechanism.cpp line 135:

> 133: }
> 134: 
> 135: static inline bool has_handshake_operation(JavaThread* jt, bool allow_suspend, bool check_async) {

This function sounds like it checks if there is a pending handshake operation, but it also does the actual processing of discovered handshake operations, which is a bit misleading. This refactoring looks a bit orthogonal to what you are doing. Could we revert this refactoring and revisit after the integration?

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

PR Review: https://git.openjdk.org/jdk/pull/24296#pullrequestreview-2835512658
PR Review Comment: https://git.openjdk.org/jdk/pull/24296#discussion_r2086364885
PR Review Comment: https://git.openjdk.org/jdk/pull/24296#discussion_r2086075611