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

[David Holmes]

On Sun, 1 Jun 2025 07:26:19 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 two additional commits since the last revision:
> 
>  - Refactoring
>  - Remove convoluted native trace logic

Just some drive-by comments mainly on your acquire/release usage. I'm not at all clear what memory accesses you are trying to coordinate with those.

src/hotspot/share/jfr/jni/jfrJniMethod.cpp line 176:

> 174:   JfrEventSetting::set_enabled(JfrCPUTimeSampleEvent, rate > 0);
> 175:   JfrCPUTimeThreadSampling::set_rate(rate, autoadapt == JNI_TRUE);
> 176:   return JNI_TRUE;

What is the point of having a boolean return type if you always return true?

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

> 57:   Thread* raw_thread = Thread::current_or_null_safe();
> 58:   JavaThread* jt;
> 59:   if (raw_thread == nullptr || !raw_thread->is_Java_thread()) { // this can happen due to the high level of parralelism

Suggestion:

  if (raw_thread == nullptr || !raw_thread->is_Java_thread()) { // this can happen due to the high level of parallelism

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

> 117:   _data = new_data;
> 118:   _capacity = capacity;
> 119: }

I assume there is a lock protecting this so it happens atomically?

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

> 120: 
> 121: bool JfrCPUTimeTraceQueue::is_full() const {
> 122:   return Atomic::load_acquire(&_head) >= _capacity;

I don't see why acquire semantics would be needed here. Also how can it be > capacity?

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

> 124: 
> 125: bool JfrCPUTimeTraceQueue::is_empty() const {
> 126:   return Atomic::load_acquire(&_head) == 0;

Acquire semantics are definitely not needed here.

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

> 128: 
> 129: s4 JfrCPUTimeTraceQueue::lost_samples() const {
> 130:   return Atomic::load_acquire(&_lost_samples);

Again acquire semantics seem highly dubious here - what loads are you synchronizing with?

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

> 137: 
> 138: u4 JfrCPUTimeTraceQueue::get_and_reset_lost_samples() {
> 139:   s4 lost_samples = Atomic::load_acquire(&_lost_samples);

Again acquire semantics seem highly dubious here - what loads are you synchronizing with?

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

> 149:     set_capacity(capacity);
> 150:   }
> 151: }

Seems an odd definition - typically `ensure_capacity` will grow a data structure to ensure it has sufficient capacity, and if already larger than needed that is fine. Suggestion `change_capacity`, or more traditionally `resize`?

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

> 235: 
> 236: void JfrCPUTimeThreadSampler::trigger_async_processing_of_cpu_time_jfr_requests() {
> 237:   Atomic::release_store(&_is_async_processing_of_cpu_time_jfr_requests_triggered, true);

What prior stores are you ensuring should be visible by using release semantics here?

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

PR Review: https://git.openjdk.org/jdk/pull/25302#pullrequestreview-2886627655
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2119983062
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2119983911
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2120016607
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2120011705
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2120012200
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2120014449
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2120014541
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2120020174
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2120021034