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

[Patricio Chilano Mateo]

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 pchilanomate (Reviewer).

src/hotspot/cpu/x86/interp_masm_x86.cpp line 782:

> 780: void InterpreterMacroAssembler::call_VM_with_sender_Java_fp(Register fp_reg, Register tmp, address entry_point, bool store_bcp) {
> 781:   if (store_bcp) {
> 782:     save_bcp(fp_reg); // We must save the bcp, but need not restore it. This is because we have already popped the fp.

Why do we need to save the bcp? We already removed the frame from the sampler perspective.

src/hotspot/cpu/x86/interp_masm_x86.cpp line 1059:

> 1057:   bind(L_continuation);
> 1058:   lea(sender_sp, Address(rbp, frame::sender_sp_offset * wordSize));
> 1059:   movptr(rbp, Address(sender_sp, (int)(ContinuationEntry::size()))); // Update the frame link.

Method enterSpecial already sets rbp so we can read from current rbp as with the normal case.

src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp line 1168:

> 1166: 
> 1167:   // Remove activation
> 1168: 

Shouldn’t all this be done before we execute the result handler above?

src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp line 1171:

> 1169:   // For asynchronous profiling to work correctly, we must remove the
> 1170:   // activation frame _before_ we test the method return safepoint poll.
> 1171:   // This is equivalent to how it is done for compiled frames.

We also have the compiled native frame case where there is no poll after removing the frame. Aren’t we missing handling that case?

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

> 89: static inline address continuation_frame_sender_pc(void* sp) {
> 90:   assert(sp != nullptr, "invariant");
> 91:   return static_cast<address>(sp) + (ContinuationEntry::size() + wordSize);

Seems we are missing a dereference.

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

> 185:     request._sample_sp = request._sample_bcp;
> 186:     // Get real bcp.
> 187:     void* const bcp = interpreter_frame_bcp(request);

Shouldn’t we get the bcp from the context instead? The one saved in the frame is not updated on each bytecode.

src/hotspot/share/runtime/javaThread.hpp line 232:

> 230:   inline void set_obj_deopt_flag();
> 231:   inline void clear_obj_deopt_flag();
> 232:   bool is_trace_suspend()      { return (_suspend_flags & _trace_flag) != 0; }

Can be removed. Same with the definition of `_trace_flag` in `SuspendFlags` and the usage in `has_special_runtime_exit_condition()`.

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

PR Review: https://git.openjdk.org/jdk/pull/24296#pullrequestreview-2844409284
PR Review Comment: https://git.openjdk.org/jdk/pull/24296#discussion_r2091616535
PR Review Comment: https://git.openjdk.org/jdk/pull/24296#discussion_r2091612262
PR Review Comment: https://git.openjdk.org/jdk/pull/24296#discussion_r2091622683
PR Review Comment: https://git.openjdk.org/jdk/pull/24296#discussion_r2091623424
PR Review Comment: https://git.openjdk.org/jdk/pull/24296#discussion_r2091607251
PR Review Comment: https://git.openjdk.org/jdk/pull/24296#discussion_r2091608265
PR Review Comment: https://git.openjdk.org/jdk/pull/24296#discussion_r2091609346