Add CPUTimeSample event support to jfrconv #1475

krk · 2025-09-11T13:22:20Z

This PR adds support for the jdk.CPUTimeSample event, an experimental feature introduced in OpenJDK 25.

This new mode provides a more accurate CPU profiling alternative to the default ExecutionSample in the JDK. It uses thread CPU time, which is particularly useful for analyzing CPU-bound applications.

Motivation and context

CPUTimeSample event support was added to OpenJDK in openjdk/jdk#25654, we can extend the JFR converter to support this new event type.

How has this been tested?

By manually generating flamegraphs and collapsed output.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

docs/ConverterUsage.md

apangin · 2025-09-11T21:20:50Z

src/converter/one/convert/Arguments.java

+                    } catch (IllegalArgumentException e) {
+                        f.set(this, CpuSampleType.ExecutionSample);
+
+                        // Next arg was not a valid value for the enum
+                        if (hasNextArg) {
+                            i--;
+                        }
+                    }


It's a hack. Let's not change behavior of an existing option.
I think --cpu can be a "smart" option that automatically detects what event to use. To force using a specific event, there can be a separate option.

We won't need this with --cpu-time. As for making --cpu a smart option, leaving that out of the current PR.

src/converter/one/jfr/event/IThreadState.java

src/converter/one/jfr/event/CPUTimeSample.java

…er#1494)

…iler#1471)

docs/ConverterUsage.md

src/converter/one/convert/Arguments.java

Baraa-Hasheesh · 2025-09-22T15:54:01Z

src/converter/one/convert/ThreadStateFilter.java

+
+public enum ThreadStateFilter {
+    Wall, Cpu, CpuTime, Default
+}


Thanks, didn't we have a linter for this?

that's clang-format which is different from clang-tidy

Baraa-Hasheesh · 2025-09-22T16:01:42Z

src/converter/one/convert/JfrConverter.java

-            threadStates = getThreadStates(false);
+            threadStates = getThreadStates(ThreadStateFilter.Wall);
+        } else {
+            threadStates = getThreadStates(ThreadStateFilter.Default);


let's remove this branch & keep threadStates = null, I don't see why it would be needed
You can do

!(event instanceof ExecutionSample) || threadStates == null || threadStates.get(((ExecutionSample) event).threadState)

Right, threadStates == null was an intentional micro-optimization for the common case when no filtering is needed.
BTW, threadStates == null can also substitute instanceof check: just don't set threadStates for event classes other than ExecutionSample.

Currently if you do bad usages such as --alloc --cpu it would exit with a casting exception

The correct solution IMO would adding a validation to prevent such cases

The simplest would be throwing a clear exception when the threadStates aren't null & the sample class is anything other than ExecutionSample

Baraa-Hasheesh · 2025-09-22T16:03:42Z

src/converter/one/convert/JfrConverter.java

        for (Event event; (event = jfr.readEvent(eventClass)) != null; ) {
            if (event.time >= startTicks && event.time <= endTicks) {
-                if (threadStates == null || threadStates.get(((ExecutionSample) event).threadState)) {
+                if (!(event instanceof ExecutionSample) || threadStates.get(((ExecutionSample) event).threadState)) {


yeah this was a risky code :)
If you do --alloc --cpu it would have failed previously

src/converter/one/convert/Arguments.java

apangin · 2025-09-23T23:18:09Z

src/converter/one/convert/JfrConverter.java

-            threadStates = getThreadStates(false);
+            threadStates = getThreadStates(ThreadStateFilter.Wall);
+        } else {
+            threadStates = getThreadStates(ThreadStateFilter.Default);


Right, threadStates == null was an intentional micro-optimization for the common case when no filtering is needed.
BTW, threadStates == null can also substitute instanceof check: just don't set threadStates for event classes other than ExecutionSample.

apangin · 2025-09-23T23:24:06Z

src/converter/one/convert/JfrConverter.java

    }

-    protected BitSet getThreadStates(boolean cpu) {
+    protected BitSet getThreadStates(ThreadStateFilter filter) {


I'm confused about this extra enum and all the related complications.
Will it work if you just add if (args.cpuTime) branch above and leave this method intact?

We need to make the distinction between cpu and cpuTime and lack of both thereof, boolean wasn't cutting it.

Since now we are parsing CPUTimeSamples as ExecutionSamples, we need to exclude CPUTimeSamples with getThreadState if mode is cpuTime. Both sample types can be in the jfr at the same time.

No, you don't need to call this function for cpuTime - that's the point.
Your ThreadStateFilter serves as unnecessary bridge to connect two pieces of logic that should not be separate in the first place.

E.g., instead of

if (a) { filter = F1; } else if (b) { filter = F2; } else if (c) { filter = F3; } if (filter == F1) { // do_A } else if (filter == F2) { // do_B } else if (filter == F3) { // do_C }

you could just write

if (a) { // do_A } else if (b) { // do_B } else if (c) { // do_C }

Baraa-Hasheesh · 2025-09-30T10:44:58Z

src/converter/one/jfr/event/ExecutionSample.java

 package one.jfr.event;

 public class ExecutionSample extends Event {
+    public static final int CpuTimeSample = 254;


can you add some context behind why this value was selected,
It does look out of place when first checking the code

I added comment

krk force-pushed the jfrconv-cputime branch from 0d7636f to 673745e Compare September 11, 2025 13:24

krk marked this pull request as draft September 11, 2025 13:38

Add CPUTimeSample event support to jfrconv

39fa108

krk force-pushed the jfrconv-cputime branch from 673745e to 39fa108 Compare September 11, 2025 13:51

krk marked this pull request as ready for review September 11, 2025 14:07

Baraa-Hasheesh reviewed Sep 11, 2025

View reviewed changes

docs/ConverterUsage.md Outdated Show resolved Hide resolved

apangin reviewed Sep 11, 2025

View reviewed changes

Baraa-Hasheesh and others added 7 commits September 22, 2025 14:39

Fix comptask test failure (async-profiler#1474)

40067ad

Fix processSamplingWithMemoryThreshold possible failure (async-profil…

88e7e4b

…er#1494)

Optimize method tracing when the function is not profiled (async-prof…

28340ae

…iler#1471)

asprof --latency option

3c38929

Eliminate period bias in CPU smoke test (async-profiler#1465)

361af18

address comments

96d1c39

Merge branch 'master' into jfrconv-cputime again

eeff0ae

Baraa-Hasheesh reviewed Sep 22, 2025

View reviewed changes

apangin reviewed Sep 23, 2025

View reviewed changes

krk added 5 commits September 24, 2025 14:05

address comments

484f726

Merge remote-tracking branch 'upstream/master' into jfrconv-cputime

dbbc7c8

address comments

1d62de2

simplify collectEvents

e1ac0cb

Merge branch 'master' into jfrconv-cputime

8b3138a

Baraa-Hasheesh reviewed Sep 30, 2025

View reviewed changes

Added comment. Minor stylistic changes.

1dc73eb

apangin merged commit f9b7810 into async-profiler:master Sep 30, 2025
23 of 24 checks passed

Add CPUTimeSample event support to jfrconv #1475

Add CPUTimeSample event support to jfrconv #1475

Uh oh!

Conversation

krk commented Sep 11, 2025

Motivation and context

How has this been tested?

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants