RFR: 8342818: Implement JEP 509: JFR CPU-Time Profiling [v5]

[Markus Grönlund]

On Fri, 23 May 2025 21:20:39 GMT, Johannes Bechberger <jbechberger at 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:
> 
>   Fix compilation

src/hotspot/share/jfr/jfr.inline.hpp line 35:

> 33: inline bool Jfr::has_sample_request(JavaThread* jt) {
> 34:   assert(jt != nullptr, "invariant");
> 35:   return jt->jfr_thread_local()->has_sample_request() || jt->jfr_thread_local()->has_cpu_time_jfr_requests();

Only need a single dereference of jfr_thread_local() here, not two.

src/hotspot/share/jfr/metadata/metadata.xml line 966:

> 964: 
> 965:   <Event name="CPUTimeSample" category="Java Virtual Machine, Profiling" label="CPU Time Method Sample"
> 966:     description="Snapshot of a threads state from the CPU time sampler. The throttle can be either an upper bound for the event emission rate, e.g. 100/s, or the cpu-time period, e.g. 10ms, with s, ms, us and ns supported as time units."

What is the minimal practical sampling period allowed?

src/hotspot/share/jfr/metadata/metadata.xml line 972:

> 970:     <Field type="boolean" name="failed" label="Failed" description="Failed to obtain the stack trace" />
> 971:     <Field type="Tickspan" name="samplingPeriod" label="CPU Time Sampling Period"/>
> 972:     <Field type="boolean" name="biased" label="Biased" description="The sample is biased towards the frame at safepoint" />

"The sampled is biased towards the frame at safepoint". What does that mean? Are there three things involved? A sample, a frame and a safepoint? How are they related? See if you can clarify what you intend to communicate here.

src/hotspot/share/jfr/metadata/metadata.xml line 975:

> 973:   </Event>
> 974: 
> 975:   <Event name="CPUTimeSampleLoss" category="Java Virtual Machine, Profiling" label="CPU Time Method Profiling Lost Samples" description="Records that the CPU time sampler lost samples because of internal throttling"

"Because of internal throttling" seems to blur into the concept of "throttling" unnecessarily. Is there an external and an internal throttling? How are they related? What is the difference?

It is probably better to avoid the term "throttling" altogether here

src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 25:

> 23:  */
> 24: 
> 25: #include "gc/shared/barrierSet.hpp"

Is this used / needed?

src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 200:

> 198:   void sample_thread(JfrSampleRequest& request, void* ucontext, JavaThread* jt, JfrThreadLocal* tl);
> 199: 
> 200:   // sample all marked threads out of safepoint

What is mean by "out of safepoint"?

src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 219:

> 217:   void stop_timer();
> 218: 
> 219:   void trigger_out_of_safepoint_sampling();

What is "out of safepoint sampling"?

src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 233:

> 231: }
> 232: 
> 233: JfrCPUTimeThreadSampler::~JfrCPUTimeThreadSampler() {

Can remove.

src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 561:

> 559:     case _thread_in_native:
> 560:       return true;
> 561:     case _thread_in_vm:

"_thread_in_vm"?

src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 694:

> 692:     while (iter.has_next()) {
> 693:       JavaThread *thread = iter.next();
> 694:       JfrThreadLocal* tl = thread->jfr_thread_local();

A JfrThreadLocal is never null.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2106152036
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2106152299
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2106152736
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2106153242
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2106153514
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2106153926
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2106153992
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2106154053
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2106154497
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2106154997