RFR: 8337789: JEP 509: JFR CPU-Time Profiling (Experimental) [v47]

[Johannes Bechberger]

On Mon, 12 May 2025 18:05:03 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Simplify local trace stack
>
> src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 819:
> 
>> 817:   }
>> 818:   JfrThreadLocal* jtl = jt->jfr_thread_local();
>> 819:   if (jtl == nullptr) {
> 
> Shouldn’t this be an assert `jtl != nullptr`?

Good catch.

> src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 823:
> 
>> 821:   }
>> 822:   if (jt->thread_state() != _thread_in_native) {
>> 823:     jtl->set_wants_cpu_time_out_of_safepoint_recording(false);
> 
> Why do we clear the flag this early before we acquired the lock?

Because it should happen before we acquire the lock, else the sampler thread might be stuck in waiting to acquire the lock to sample out of safepoint unneccessarily.

> src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 836:
> 
>> 834:     int64_t period = get_sampling_period() * (info->si_overrun + 1);
>> 835:     trace->record_trace(jt, context, period);
>> 836:     if (jtl->cpu_time_jfr_stack().enqueue(trace)) {
> 
> If this fails shouldn’t we return the trace to the `_frame_store`?

Good catch

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r2091170772
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r2091165803
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r2091162399