-
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 5 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 |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.util.List; | ||
| import java.util.Locale; | ||
| import org.apache.lucene.gradle.plugins.LuceneGradlePlugin; | ||
| import org.gradle.api.DefaultTask; | ||
| import org.gradle.api.GradleException; | ||
|
|
@@ -91,6 +92,8 @@ public void run() { | |
| getCount().get(), | ||
| getLineNumbers().get(), | ||
| getFrametypes().get()); | ||
| } catch (IllegalStateException e) { | ||
| throw new GradleException(e.getMessage(), e); | ||
| } catch (IOException e) { | ||
| throw new GradleException("Error when generating jfr profile summaries.", e); | ||
| } | ||
|
|
@@ -189,7 +192,7 @@ public void apply(Project project) { | |
| task.jvmArgs( | ||
| List.of( | ||
| "-XX:StartFlightRecording=dumponexit=true,maxsize=250M,settings=" | ||
| + gradlePluginResource(project, "testing/profiling.jfc"), | ||
| + gradlePluginResource(project, profilingSettingsPath()), | ||
| "-XX:+UnlockDiagnosticVMOptions", | ||
| "-XX:+DebugNonSafepoints")); | ||
| task.dependsOn(cleanPreviousProfiles); | ||
|
|
@@ -254,4 +257,14 @@ private record ProfilingOptions( | |
| Provider<Integer> countOption, | ||
| Provider<Boolean> lineNumbersOption, | ||
| Provider<Boolean> frametypesOption) {} | ||
|
|
||
| /** | ||
| * JEP 509 CPU-time sampling ({@code jdk.CPUTimeSample}) is implemented on Linux. Use a Linux-only | ||
| * JFR template with CPU-time sampling only; other hosts keep wall-clock execution sampling for | ||
| * short tests. | ||
|
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. Wait, it's for all tests, not just short tests right? Maybe just remove
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. Updated. Thanks! |
||
| */ | ||
| private static String profilingSettingsPath() { | ||
| String os = System.getProperty("os.name", "").toLowerCase(Locale.ROOT); | ||
| return os.contains("linux") ? "testing/profiling.linux.jfc" : "testing/profiling.jfc"; | ||
|
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. Actually, instead of baking
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, makes sense. Thanks! |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,10 +101,17 @@ 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) { | ||
|
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 don't think we need this boolean parameter nor the if block. Just something like: return name.equals("jdk.CPUTimeSample") ||
name.equals("jdk.ExecutionSample") ||
name.equals("jdk.NativeMethodSample");
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, updated. thanks! |
||
| // 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 present, we ignore jdk.ExecutionSample and jdk.NativeMethodSample | ||
| // (wall-clock / JNI execution sampling) so the same hot code is not double-counted. | ||
| 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 +128,37 @@ static boolean isGradlePollThread(RecordedThread thread) { | |
| return (thread != null && thread.getJavaName().startsWith("/127.0.0.1")); | ||
| } | ||
|
|
||
| /** | ||
| * Pre-scan recording files for CPU sampling strategy. Returns whether any {@code | ||
| * jdk.CPUTimeSample} events appear in the inputs. Throws if recordings mix those with legacy | ||
| * wall-clock samples ({@code jdk.ExecutionSample} / {@code jdk.NativeMethodSample}); only one | ||
| * CPU sampler should be enabled in JFR settings. | ||
| */ | ||
| static boolean resolveCpuTimeSampling(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. Remove the whole function as the two files are mutually exclusive. This is very verbose and not needed.
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! |
||
| boolean hasCpuTime = false; | ||
| boolean hasLegacy = false; | ||
| for (String file : files) { | ||
| try (RecordingFile recording = new RecordingFile(Paths.get(file))) { | ||
| while (recording.hasMoreEvents()) { | ||
| RecordedEvent event = recording.readEvent(); | ||
| String n = event.getEventType().getName(); | ||
| if (n.equals("jdk.CPUTimeSample")) { | ||
| hasCpuTime = true; | ||
| } else if (n.equals("jdk.ExecutionSample") || n.equals("jdk.NativeMethodSample")) { | ||
| hasLegacy = true; | ||
| } | ||
| if (hasCpuTime && hasLegacy) { | ||
| throw new IllegalStateException( | ||
| "Mixed CPU profiling events: found both jdk.CPUTimeSample and legacy wall-clock " | ||
| + "samples (jdk.ExecutionSample / jdk.NativeMethodSample). Use a single sampler " | ||
| + "in JFR settings (see gradle/testing/profiling*.jfc)."); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return hasCpuTime; | ||
| } | ||
|
|
||
| /** value we accumulate for this event */ | ||
| static long getValue(RecordedEvent event) { | ||
| switch (event.getEventType().getName()) { | ||
|
|
@@ -132,6 +170,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 +213,10 @@ public static void printReport( | |
| if (count < 1) { | ||
| throw new IllegalArgumentException("tests.profile.count must be positive"); | ||
| } | ||
|
|
||
| // Pre-scan recordings: decide CPU-time vs legacy sampling and reject mixed configurations. | ||
| boolean hasCPUTimeSamples = "cpu".equals(mode) && resolveCpuTimeSampling(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. nuke this one too
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, removed this too. thanks! |
||
|
|
||
| Map<String, SimpleEntry<String, Long>> histogram = new HashMap<>(); | ||
| int totalEvents = 0; | ||
| long sumValues = 0; | ||
|
|
@@ -181,7 +225,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 |
|---|---|---|
|
|
@@ -16,7 +16,9 @@ | |
| limitations under the License. | ||
| --> | ||
| <!-- | ||
| Collects only execution and method samples at a low interval | ||
| Wall-clock execution samples for CPU profiling on platforms where JEP 509 CPU-time sampling | ||
| is unavailable (non-Linux). On Linux, CodeProfilingPlugin uses profiling.linux.jfc instead. | ||
|
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 don't see a
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. my bad, missed it in adding in previous commit. Thanks! |
||
| Heap allocation events are enabled on all platforms. | ||
| --> | ||
| <configuration version="2.0" label="TestProfiling" description="Sampling for unit tests" provider="Apache"> | ||
| <event name="jdk.ExecutionSample"> | ||
|
|
||
| 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! |
||
| Linux test JVMs use gradle/testing/profiling.linux.jfc (CPU-time sampling only); other hosts | ||
| keep wall-clock jdk.ExecutionSample/jdk.NativeMethodSample. ProfileResults rejects recordings | ||
| that mix CPU-time and legacy execution samples. (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.
Maybe
is currently (Java 25) implemented only on Linux, so we use a Linux-only JFR template...?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.
yes, updated. Thanks!