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

[Markus Grönlund]

On Wed, 14 May 2025 14:31:27 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> 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?
>
> Also, for the sampling in native case, we could make it more cooperative by adding a handshake operation like the one for all threads but for a list of threads. That way we can try to distribute the work instead of going one by one (this can actually be done with the current code too with some changes).

There might be opportunities for generalizations. The main reason was to keep much of the existing JFR sampler mechanism. It evolved more and more towards handshakes, I agree. Maybe we can generalize it later.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24296#discussion_r2089219304