Re: RFR: 8352251: Implement Cooperative JFR Sampling [v12]
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: Attempt to build Windows-AARCH64 ------------- Changes: - all: https://git.openjdk.org/jdk/pull/24296/files - new: https://git.openjdk.org/jdk/pull/24296/files/92bbfbac..771a397a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24296&range=11 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24296&range=10-11 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/24296.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24296/head:pull/24296 PR: https://git.openjdk.org/jdk/pull/24296
On Thu, 24 Apr 2025 09:53:38 GMT, Markus Grönlund <mgronlun@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:
Attempt to build Windows-AARCH64
Hi, I've taken a quick look and wonder what needs to be done for PPC64 which has some fundamental differences. We never need to pass an FP because it can always be retrieved like this via Back Chain (required by "Power Architecture 64-Bit ELF V2 ABI Specification"): FP = *SP So, I guess we could use something like diff --git a/src/hotspot/cpu/ppc/javaFrameAnchor_ppc.hpp b/src/hotspot/cpu/ppc/javaFrameAnchor_ppc.hpp index 8b539bc8101..7d07c302b36 100644 --- a/src/hotspot/cpu/ppc/javaFrameAnchor_ppc.hpp +++ b/src/hotspot/cpu/ppc/javaFrameAnchor_ppc.hpp @@ -73,6 +73,10 @@ address last_Java_pc(void) { return _last_Java_pc; } + intptr_t* last_Java_fp() const { return *(intptr_t**)_last_Java_sp; } + + intptr_t* last_sender_Java_fp() const { return **(intptr_t***)_last_Java_sp; } + void set_last_Java_sp(intptr_t* sp) { OrderAccess::release(); _last_Java_sp = sp; } #endif // CPU_PPC_JAVAFRAMEANCHOR_PPC_HPP This frame constructor does not exist on PPC64: https://github.com/openjdk/jdk/blob/771a397a12a3bcb25af1f508061fd928d8f13c2e... We have one with different argument order and could emulate it like this: ```C++ inline frame::frame(intptr_t* sp, intptr_t* unextended_sp, intptr_t* sender_sp, address pc, CodeBlob* cb) : frame(sp, pc, unextended_sp, sender_sp, cb) {} Strange if we really need another one which only turns the argument order. The following constants do not exist: ```C++ // Used by JFR. Unfortunately, we can't use offset_of because it's not constexpr. ijava_state_slots = ijava_state_size >> LogBytesPerWord, // Members of ijava_state: interpreter_frame_method_offset = -ijava_state_slots + 0, interpreter_frame_bcp_offset = -ijava_state_slots + 5, interpreter_frame_sender_sp_offset = -ijava_state_slots + 9, // Stuff which shouldn't be used on this platform: interpreter_frame_initial_sp_offset = -ijava_state_slots, link_offset = 0, sender_sp_offset = 0, // Member of common_abi: lr return_addr_offset = 2, I hate adding them. Could we move the usages into platform code? ------------- PR Comment: https://git.openjdk.org/jdk/pull/24296#issuecomment-2830943762
On Thu, 24 Apr 2025 09:53:38 GMT, Markus Grönlund <mgronlun@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:
Attempt to build Windows-AARCH64
Hi, I've taken a quick look and wonder what needs to be done for PPC64 which has some fundamental differences.
We never need to pass an FP because it can always be retrieved like this via Back Chain (required by "Power Architecture 64-Bit ELF V2 ABI Specification"): FP = *SP So, I guess we could use something like
```diff diff --git a/src/hotspot/cpu/ppc/javaFrameAnchor_ppc.hpp b/src/hotspot/cpu/ppc/javaFrameAnchor_ppc.hpp index 8b539bc8101..7d07c302b36 100644 --- a/src/hotspot/cpu/ppc/javaFrameAnchor_ppc.hpp +++ b/src/hotspot/cpu/ppc/javaFrameAnchor_ppc.hpp @@ -73,6 +73,10 @@
address last_Java_pc(void) { return _last_Java_pc; }
+ intptr_t* last_Java_fp() const { return *(intptr_t**)_last_Java_sp; } + + intptr_t* last_sender_Java_fp() const { return **(intptr_t***)_last_Java_sp; } + void set_last_Java_sp(intptr_t* sp) { OrderAccess::release(); _last_Java_sp = sp; }
#endif // CPU_PPC_JAVAFRAMEANCHOR_PPC_HPP ```
This frame constructor does not exist on PPC64:
https://github.com/openjdk/jdk/blob/771a397a12a3bcb25af1f508061fd928d8f13c2e...
We have one with different argument order and could emulate it like this:
```c++ inline frame::frame(intptr_t* sp, intptr_t* unextended_sp, intptr_t* sender_sp, address pc, CodeBlob* cb) : frame(sp, pc, unextended_sp, sender_sp, cb) {} ```
Strange if we really need another one which only turns the argument order.
The following constants do not exist:
```c++ // Used by JFR. Unfortunately, we can't use offset_of because it's not constexpr. ijava_state_slots = ijava_state_size >> LogBytesPerWord, // Members of ijava_state: interpreter_frame_method_offset = -ijava_state_slots + 0, interpreter_frame_bcp_offset = -ijava_state_slots + 5, interpreter_frame_sender_sp_offset = -ijava_state_slots + 9, // Stuff which shouldn't be used on this platform: interpreter_frame_initial_sp_offset = -ijava_state_slots, link_offset = 0, sender_sp_offset = 0, // Member of common_abi: lr return_addr_offset = 2, ```
I hate adding them. Could we move the usages into platform code?
Hi Martin, On the other platforms, the walker code only uses _last_sender_Java_fp() if it is set. And it is only set by the sensitive Interpreter safe point polls just before frame pop. We would like something that is a value to be tested, which can be set using the mechanism you suggest, but only for those sensitive poll sites. I am moving support routines that rely on platform-specific constants into platform code. Stay tuned. ------------- PR Comment: https://git.openjdk.org/jdk/pull/24296#issuecomment-2833538638
On Thu, 24 Apr 2025 09:53:38 GMT, Markus Grönlund <mgronlun@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:
Attempt to build Windows-AARCH64
Hope this will makes things easier for you, Martin. [push back pd constants into pd code](https://github.com/openjdk/jdk/pull/24296/commits/ed57ed9f327729939d6d57108b...) Thanks Markus ------------- PR Comment: https://git.openjdk.org/jdk/pull/24296#issuecomment-2833541351
On Sun, 27 Apr 2025 16:42:08 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:
Hope this will make things easier for you, Martin.
[push back pd constants into pd code](https://github.com/openjdk/jdk/pull/24296/commits/ed57ed9f327729939d6d57108b...)
Thanks Markus
Thanks a lot! This looks like a much nicer and more generic frame interface. I need to dig through the details, yet. Would `frame_link` be better name than just `link`? Also some comments would be nice because I find it hard to understand e.g. the difference between fp, sender_sp and link. ------------- PR Comment: https://git.openjdk.org/jdk/pull/24296#issuecomment-2835720584
participants (2)
-
Markus Grönlund
-
Martin Doerr