RFR: 8342818: Implement CPU Time Profiling for JFR

[David Holmes]

On Wed, 28 Aug 2024 16:47:21 GMT, Johannes Bechberger <jbechberger at openjdk.org> wrote:

> This is the code for the [JEP draft: CPU Time based profiling for JFR].

Just a cursory look through with some drive-by comments.

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?

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?

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.

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.

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 ???

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?

src/hotspot/share/jfr/recorder/stacktrace/jfrAsyncStackTrace.cpp line 56:

> 54:   }
> 55:   assert(current_thread->in_asgct(), "invariant");
> 56:   assert(jt != current_thread || current_thread->in_asgct(), "invariant");

The second clause of the second assert is redundant given the first assert.

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

PR Review: https://git.openjdk.org/jdk/pull/20752#pullrequestreview-2403526518
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1821844294
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1821832345
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1821832710
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1821861186
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1821840144
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1821842412
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1821848326