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

[Patricio Chilano Mateo]

On Wed, 14 May 2025 08:09:28 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:
> 
>   validate_bcp

Hi Markus, added some comments below. I haven’t look in detail at the platform dependent changes yet but will do that too. Thanks.

src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp line 292:

> 290: // Sampling a thread in state _thread_in_Java
> 291: // involves a platform-specific thread suspend and CPU context retrieval.
> 292: bool JfrSamplerThread::sample_java_thread(JavaThread* jt) {

Why not just send a signal and let the target do everything including creating the request? Is it because enqueueing the request might need to allocate memory? Given that the sampling frequency is in the order of milliseconds, and the TTS for the target is sub-milliseconds we would just need a pretty small buffer if anything. Or is there something else I’m missing?

src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp line 323:

> 321: // without thread suspension and CPU context retrieval,
> 322: // if we carefully order the loads of the thread state.
> 323: bool JfrSamplerThread::sample_native_thread(JavaThread* jt) {

This method now begs to be converted into a handshake, it’s pretty much the same logic. I wonder if you considered that already and found some issue. The handshake would also provide the necessary synchronization so there is no need for the extra sample_monitor and wait/notifies. I’m assuming you didn’t go for this because you would still need to add the extra separate flag check in `SafepointMechanism` for the sample in java case, and so you just kept both under it for consistency? But even the sample in java case could use handshakes, we would just need to install an async one (only executed by target) at the end after enqueueing the request. We would still need to keep the `Jfr::check_and_process_sample_request()` method separately but we can do that. I’ve been playing around with these changes: https://github.com/pchilano/jdk/commit/dacce47402c38a2b466c819d2cd6c643b0de09ba
I think it is simpler to read and removes some of the complexity of the current approach. What do you think?

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

PR Review: https://git.openjdk.org/jdk/pull/24296#pullrequestreview-2840430440
PR Review Comment: https://git.openjdk.org/jdk/pull/24296#discussion_r2089091415
PR Review Comment: https://git.openjdk.org/jdk/pull/24296#discussion_r2089084362