RFR: 8342818: Implement CPU Time Profiling for JFR

[Jaroslav Bachorik]

On Wed, 4 Sep 2024 20:19:44 GMT, Markus Grönlund <mgronlun at openjdk.org> wrote:

>> This is the code for the [JEP draft: CPU Time based profiling for JFR].
>
> src/hotspot/share/jfr/metadata/metadata.xml line 945:
> 
>> 943:   </Event>
>> 944: 
>> 945:   <Event name="CPUTimeExecutionSampleThrottles" category="Java Virtual Machine, Profiling" label="CPU Time Method Profiling Dropped Samples" description="Records that the CPU time dropped samples because of internal throttling"
> 
> A better name for this event is "CPUTimeExecutionSampleLoss".
> 
> This is because JFR already has a general concept of "DataLoss," so this name would better align with it. 
> 
> Please update references to "dropped" -> "lost"

Done

> src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.hpp line 29:
> 
>> 27: 
>> 28: #include "jfr/utilities/jfrAllocation.hpp"
>> 29: #include "runtime/javaThread.hpp"
> 
> You don't need to include the "runtime/javaThread.hpp" header.
> 
> Instead, please hoist the forward declaration of "class JavaThread" to outside the "#if defined(LINUX)"

Done

> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdLoadBarrier.cpp line 102:
> 
>> 100: }
>> 101: 
>> 102: JfrBuffer* JfrTraceIdLoadBarrier::renew_sampler_enqueue_buffer(Thread* thread, size_t size) {
> 
> JfrCPUTimeThreadSampler should not need to take this specialized path because it does not suspend threads. It can be left as is.

Just to clarify - you mean that when we do `JfrTraceIdLoadBarrier::get_sampler_enqueue_buffer(this)` in `jfrCPUTimeThreadSampler.cpp` it may not return `nullptr`?
Or the whole part of the new code using the `enqueue_buffer` is not needed?

> src/hotspot/share/jfr/recorder/stacktrace/jfrStackTrace.hpp line 41:
> 
>> 39:  private:
>> 40:   bool _vthread;
>> 41:   const ContinuationEntry* _cont_entry;
> 
> You need a fwd declaration for ContinuationEntry here?

I think it will be pulled in by `vframe.hpp` - at least JDK builds successfully even without adding the fwd declaration.

> src/hotspot/share/jfr/support/jfrThreadLocal.hpp line 76:
> 
>> 74:   bool _dead;
>> 75:   LINUX_ONLY(bool _has_cpu_timer = false);
>> 76:   LINUX_ONLY(timer_t _cpu_timer);
> 
> No include for timer_t?

Added

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1750269452
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1750270486
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1751832161
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1751867711
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1752127434