RFR: 8342818: Implement CPU Time Profiling for JFR

[Johannes Bechberger]

On Wed, 30 Oct 2024 04:19:01 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> This is the code for the [JEP draft: CPU Time based profiling for JFR].
>
> src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 41:
> 
>> 39: #include "runtime/osThread.hpp"
>> 40: 
>> 41: #if defined(LINUX)
> 
> If this is all Linux specific then can't we put it in a linux-specific sourcefile instead of masquerading as shared code?

The idea is to place it close to the other method sampler.

> src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 57:
> 
>> 55: static bool thread_state_in_java(JavaThread* thread) {
>> 56:   assert(thread != nullptr, "invariant");
>> 57:   switch(thread->thread_state()) {
> 
> Why a full switch when only two states are of interest?

Similarity to the other sampler.

> src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 78:
> 
>> 76: }
>> 77: 
>> 78: static bool thread_state_in_native(JavaThread* thread) {
> 
> Whatever this is checking it is not what we normally call "in native" so please come up with a different name.

This is the same name the other sampler uses.

> src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 238:
> 
>> 236:     // Encodes generation of the element (to solve ABA problem)
>> 237:     // along with full/empty flag in the highest bit
>> 238:     u4 _state;
> 
> Can you add more commentary explaining the operation of the queue please and how this "state" interacts with enqueue/dequeue.

@jbachorik and @apangin: can you take this?

> src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 505:
> 
>> 503:       autoadapt_period_if_needed();
>> 504:       last_autoadapt_check = os::javaTimeMillis();
>> 505:     }
> 
> Do you really want wall-clock time here ???

What else should I use? I want to check for the adaption of the sampling period every few seconds.

> src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 788:
> 
>> 786: 
>> 787:   // create timers for all existing threads
>> 788:   MutexLocker tlock(Threads_lock);
> 
> When exactly is this called? The direct use of the Threads_lock raises concerns. Should this not be done at a global safepoint?

This is called transitively from the Java code whenever the event is first enabled. So this should lead to a safepoint?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1822031772
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1822033609
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1822033128
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1822034297
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1822035486
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1822036741