-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Support jdk.CPUTimeSample event in ProfileResults for Java 25+ (JEP 509) #15983
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
78af0c1
f952eb7
254196d
5125d0e
ed3c9bf
d076437
30d4510
0d78f10
6f2c12f
0e18580
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,10 +101,16 @@ public static void main(String[] args) throws IOException { | |
| } | ||
|
|
||
| /** true if we care about this event */ | ||
| static boolean isInteresting(String mode, RecordedEvent event) { | ||
| static boolean isInteresting(String mode, RecordedEvent event, boolean hasCPUTimeSamples) { | ||
| String name = event.getEventType().getName(); | ||
| switch (mode) { | ||
| case "cpu": | ||
| if (hasCPUTimeSamples) { | ||
| // Prefer jdk.CPUTimeSample (Java 25+, JEP 509): samples by CPU time, not wall-clock | ||
| // time, so idle threads (like gradle epoll) are inherently excluded — no filtering | ||
| // needed. When available, we skip legacy execution samples to avoid double-counting. | ||
| return name.equals("jdk.CPUTimeSample"); | ||
| } | ||
| return (name.equals("jdk.ExecutionSample") || name.equals("jdk.NativeMethodSample")) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I expanded the comment to say that explicitly so it matches the code. Thanks! |
||
| && !isGradlePollThread(event.getThread("sampledThread")); | ||
| case "heap": | ||
|
|
@@ -121,6 +127,24 @@ static boolean isGradlePollThread(RecordedThread thread) { | |
| return (thread != null && thread.getJavaName().startsWith("/127.0.0.1")); | ||
| } | ||
|
|
||
| /** | ||
| * Pre-scan recording files to detect if any jdk.CPUTimeSample events are present. When they are, | ||
| * we prefer them over legacy jdk.ExecutionSample/jdk.NativeMethodSample to avoid double-counting. | ||
| */ | ||
| static boolean detectCPUTimeSamples(List<String> files) throws IOException { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of this logic, should we just have profiling.linux.jfc that is loaded for linux, and profiling.jfc used for others?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, done. Linux uses gradle/testing/profiling.linux.jfc, everyone else gradle/testing/profiling.jfc, selected in CodeProfilingPlugin from the host OS. Thanks! |
||
| for (String file : files) { | ||
| try (RecordingFile recording = new RecordingFile(Paths.get(file))) { | ||
| while (recording.hasMoreEvents()) { | ||
| RecordedEvent event = recording.readEvent(); | ||
| if (event.getEventType().getName().equals("jdk.CPUTimeSample")) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** value we accumulate for this event */ | ||
| static long getValue(RecordedEvent event) { | ||
| switch (event.getEventType().getName()) { | ||
|
|
@@ -132,6 +156,8 @@ static long getValue(RecordedEvent event) { | |
| return 1L; | ||
| case "jdk.NativeMethodSample": | ||
| return 1L; | ||
| case "jdk.CPUTimeSample": | ||
| return 1L; | ||
| default: | ||
| throw new UnsupportedOperationException(event.toString()); | ||
| } | ||
|
|
@@ -173,6 +199,11 @@ public static void printReport( | |
| if (count < 1) { | ||
| throw new IllegalArgumentException("tests.profile.count must be positive"); | ||
| } | ||
|
|
||
| // Pre-scan to detect if CPU-time samples (Java 25+, JEP 509) are available. | ||
| // If so, prefer them over legacy execution samples to avoid double-counting. | ||
| boolean hasCPUTimeSamples = "cpu".equals(mode) && detectCPUTimeSamples(files); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we make this stricter? Throw an exception if we see a mix of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I initially added the strict mixed-sampler check here, but removed it after @rmuir follow up review since the two Lucene JFC files are now mutually exclusive. So the exclusivity now lives in the selected JFC config instead of a pre-scan in |
||
|
|
||
| Map<String, SimpleEntry<String, Long>> histogram = new HashMap<>(); | ||
| int totalEvents = 0; | ||
| long sumValues = 0; | ||
|
|
@@ -181,7 +212,7 @@ public static void printReport( | |
| try (RecordingFile recording = new RecordingFile(Paths.get(file))) { | ||
| while (recording.hasMoreEvents()) { | ||
| RecordedEvent event = recording.readEvent(); | ||
| if (!isInteresting(mode, event)) { | ||
| if (!isInteresting(mode, event, hasCPUTimeSamples)) { | ||
| continue; | ||
| } | ||
| RecordedStackTrace trace = event.getStackTrace(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,11 @@ Collects only execution and method samples at a low interval | |
| <setting name="period">1 ms</setting> | ||
| </event> | ||
|
|
||
| <event name="jdk.CPUTimeSample"> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we turn off the old (wall-clock) sampling (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The 1msec sampling was necessary for tests in order to find places wasting CPU. most tests just don't run that long.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done,
Thanks both! |
||
| <setting name="enabled">true</setting> | ||
| <setting name="throttle">10 ms</setting> | ||
| </event> | ||
|
|
||
| <event name="jdk.ObjectAllocationInNewTLAB"> | ||
| <setting name="enabled">true</setting> | ||
| <setting name="stackTrace">true</setting> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -185,6 +185,11 @@ Changes in Runtime Behavior | |
|
|
||
| Build | ||
| --------------------- | ||
| * GITHUB#15926: Support jdk.CPUTimeSample event (Java 25+, JEP 509) in ProfileResults. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we word a bit more user-friendly-y? Maybe
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, done. thanks! |
||
| CPU-time profiling produces more accurate profiles by sampling CPU instructions instead | ||
| of wall-clock time. When CPUTimeSample events are present, they are preferred over legacy | ||
| ExecutionSample events to avoid double-counting. (Prithvi S) | ||
|
|
||
| * GITHUB#15327: New low-level build options to detect abuse of LuceneTestCase.random(): | ||
| tests.random.maxacquires and tests.random.maxcalls (Robert Muir, Dawid Weiss) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this boolean parameter nor the if block.
Just something like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, updated. thanks!