Re: RFR: 8342818: Implement JEP 509: JFR CPU-Time Profiling [v17]
This is the code for the [JEP 509: CPU Time based profiling for JFR](https://openjdk.org/jeps/509).
Currently tested using [this test suite](https://github.com/parttimenerd/basic-profiler-tests). This runs profiles the [Renaissance](https://renaissance.dev/) benchmark with - ... different heap sizes - ... different GCs - ... different samplers (the standard JFR and the new CPU Time Sampler and both) - ... different JFR recording durations - ... different chunk-sizes
Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision: Remove assertion ------------- Changes: - all: https://git.openjdk.org/jdk/pull/25302/files - new: https://git.openjdk.org/jdk/pull/25302/files/d49b0084..d91f5672 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25302&range=16 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25302&range=15-16 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/25302.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25302/head:pull/25302 PR: https://git.openjdk.org/jdk/pull/25302
On Mon, 26 May 2025 20:57:15 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
This is the code for the [JEP 509: CPU Time based profiling for JFR](https://openjdk.org/jeps/509).
Currently tested using [this test suite](https://github.com/parttimenerd/basic-profiler-tests). This runs profiles the [Renaissance](https://renaissance.dev/) benchmark with - ... different heap sizes - ... different GCs - ... different samplers (the standard JFR and the new CPU Time Sampler and both) - ... different JFR recording durations - ... different chunk-sizes
Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
Remove assertion
src/hotspot/share/jfr/periodic/sampling/jfrSampleRequest.cpp line 174:
172: assert(request._sample_pc != nullptr, "invariant"); 173: assert(jt != nullptr, "invariant"); 174: assert(jt->thread_state() == _thread_in_Java, "invariant");
What? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2108902215
On Tue, 27 May 2025 11:11:13 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:
Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
Remove assertion
src/hotspot/share/jfr/periodic/sampling/jfrSampleRequest.cpp line 174:
172: assert(request._sample_pc != nullptr, "invariant"); 173: assert(jt != nullptr, "invariant"); 174: assert(jt->thread_state() == _thread_in_Java, "invariant");
What?
I also call this method for native stack traces in my implementation. As I have to walk native stack traces at stackwalks. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2108995311
On Tue, 27 May 2025 11:58:22 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
src/hotspot/share/jfr/periodic/sampling/jfrSampleRequest.cpp line 174:
172: assert(request._sample_pc != nullptr, "invariant"); 173: assert(jt != nullptr, "invariant"); 174: assert(jt->thread_state() == _thread_in_Java, "invariant");
What?
I also call this method for native stack traces in my implementation. As I have to walk native stack traces at stackwalks.
Assert for thread->state() == _thread_in_native then. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2117674370
On Mon, 26 May 2025 20:57:15 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
This is the code for the [JEP 509: CPU Time based profiling for JFR](https://openjdk.org/jeps/509).
Currently tested using [this test suite](https://github.com/parttimenerd/basic-profiler-tests). This runs profiles the [Renaissance](https://renaissance.dev/) benchmark with - ... different heap sizes - ... different GCs - ... different samplers (the standard JFR and the new CPU Time Sampler and both) - ... different JFR recording durations - ... different chunk-sizes
Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
Remove assertion
src/jdk.jfr/share/classes/jdk/jfr/internal/PlatformEventType.java line 195:
193: 194: public boolean hasThreshold() { 195: if (hasCutoff || isCPUTimeMethodSampling) {
This is not a duration event anymore, right? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2108950974
On Tue, 27 May 2025 11:36:41 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:
Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
Remove assertion
src/jdk.jfr/share/classes/jdk/jfr/internal/PlatformEventType.java line 195:
193: 194: public boolean hasThreshold() { 195: if (hasCutoff || isCPUTimeMethodSampling) {
This is not a duration event anymore, right?
Yes ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2111388369
On Mon, 26 May 2025 20:57:15 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
This is the code for the [JEP 509: CPU Time based profiling for JFR](https://openjdk.org/jeps/509).
Currently tested using [this test suite](https://github.com/parttimenerd/basic-profiler-tests). This runs profiles the [Renaissance](https://renaissance.dev/) benchmark with - ... different heap sizes - ... different GCs - ... different samplers (the standard JFR and the new CPU Time Sampler and both) - ... different JFR recording durations - ... different chunk-sizes
Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
Remove assertion
test/langtools/tools/javac/diags/examples/SubtypeDoesntImplementSealed.java line 54:
52: interface B3 {} 53: 54: >>>>>>>> bbceab072555d5e2f5d3e99ae07a5ca5e909d7dc:test/langtools/tools/javac/diags/examples/SubtypeDoesntImplementSealed.java
merge error? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2109322448
On Mon, 26 May 2025 20:57:15 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
This is the code for the [JEP 509: CPU Time based profiling for JFR](https://openjdk.org/jeps/509).
Currently tested using [this test suite](https://github.com/parttimenerd/basic-profiler-tests). This runs profiles the [Renaissance](https://renaissance.dev/) benchmark with - ... different heap sizes - ... different GCs - ... different samplers (the standard JFR and the new CPU Time Sampler and both) - ... different JFR recording durations - ... different chunk-sizes
Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
Remove assertion
src/hotspot/share/jfr/support/jfrThreadLocal.hpp line 36:
34: #include "runtime/mutexLocker.hpp" 35: 36: #ifdef LINUX
Can things now be fwd declared in the .hpp and includes move to the .cpp instead? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2109331738
On Tue, 27 May 2025 14:17:46 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:
Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
Remove assertion
src/hotspot/share/jfr/support/jfrThreadLocal.hpp line 36:
34: #include "runtime/mutexLocker.hpp" 35: 36: #ifdef LINUX
Can things now be fwd declared in the .hpp and includes move to the .cpp instead?
No, because I need the `timer_t` in the arguments of the methods. Or should I use a wrapper class and a pointer to it?
test/langtools/tools/javac/diags/examples/SubtypeDoesntImplementSealed.java line 54:
52: interface B3 {} 53: 54: >>>>>>>> bbceab072555d5e2f5d3e99ae07a5ca5e909d7dc:test/langtools/tools/javac/diags/examples/SubtypeDoesntImplementSealed.java
merge error?
good catch ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2109398525 PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2109395879
On Tue, 27 May 2025 14:44:19 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
src/hotspot/share/jfr/support/jfrThreadLocal.hpp line 36:
34: #include "runtime/mutexLocker.hpp" 35: 36: #ifdef LINUX
Can things now be fwd declared in the .hpp and includes move to the .cpp instead?
No, because I need the `timer_t` in the arguments of the methods. Or should I use a wrapper class and a pointer to it?
It can be a timer_t* Then it's fwd declarable. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2109643605
On Tue, 27 May 2025 16:20:58 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:
No, because I need the `timer_t` in the arguments of the methods. Or should I use a wrapper class and a pointer to it?
It can be a timer_t*
Then it's fwd declarable.
If it's typedeffed as an int or a pointer (in sys/types.h or signal.h), then it won't even need a forward declaration. Try without it and see what happens. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2109654903
On Tue, 27 May 2025 16:27:02 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:
It can be a timer_t*
Then it's fwd declarable.
If it's typedeffed as an int or a pointer (in sys/types.h or signal.h), then it won't even need a forward declaration. Try without it and see what happens.
It seems to normally be a pointer. I'll try the forward declaration later. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2109658273
On Tue, 27 May 2025 16:29:05 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
If it's typedeffed as an int or a pointer (in sys/types.h or signal.h), then it won't even need a forward declaration. Try without it and see what happens.
It seems to normally be a pointer. I'll try the forward declaration later.
Don't worry if it's too hard. It would just be a nice thing to eliminate these special includes from the header file. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2109666735
On Tue, 27 May 2025 16:34:14 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:
It seems to normally be a pointer. I'll try the forward declaration later.
Don't worry if it's too hard. It would just be a nice thing to eliminate these special includes from the header file.
The signal.h is apparently included in the `globaldefinitions_gcc`, so no need to include it. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2111351553
On Mon, 26 May 2025 20:57:15 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
This is the code for the [JEP 509: CPU Time based profiling for JFR](https://openjdk.org/jeps/509).
Currently tested using [this test suite](https://github.com/parttimenerd/basic-profiler-tests). This runs profiles the [Renaissance](https://renaissance.dev/) benchmark with - ... different heap sizes - ... different GCs - ... different samplers (the standard JFR and the new CPU Time Sampler and both) - ... different JFR recording durations - ... different chunk-sizes
Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
Remove assertion
src/hotspot/share/jfr/support/jfrThreadLocal.hpp line 89:
87: 88: #ifdef LINUX 89: timer_t* _cpu_timer = nullptr;
initialize in initializer-list, please. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2109668742
participants (2)
-
Johannes Bechberger
-
Markus Grönlund