RFR: 8358619: Fix interval recomputation in CPU Time Profiler
Fixes the recomputation issue by passing either the rate or the period directly to the sampler (instead of just the rate value with the period value converted into a rate). This prevents issues when the number of cores changes during the JFR recording. ------------- Commit messages: - 8358619: Fix interval recomputation in CPU Time Profiler Changes: https://git.openjdk.org/jdk/pull/25775/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25775&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8358619 Stats: 327 lines in 11 files changed: 180 ins; 78 del; 69 mod Patch: https://git.openjdk.org/jdk/pull/25775.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25775/head:pull/25775 PR: https://git.openjdk.org/jdk/pull/25775
On Thu, 12 Jun 2025 09:27:30 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
Fixes the recomputation issue by passing either the rate or the period directly to the sampler (instead of just the rate value with the period value converted into a rate). This prevents issues when the number of cores changes during the JFR recording.
src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 171:
169: double _rate; 170: u8 _period_nanos; 171: bool _is_rate;
Can this be turned into `union`? Only one of `_rate` or `_period_nanos` will ever be set at a time. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25775#discussion_r2154393526
On Wed, 18 Jun 2025 11:45:48 GMT, Jaroslav Bachorik <jbachorik@openjdk.org> wrote:
Fixes the recomputation issue by passing either the rate or the period directly to the sampler (instead of just the rate value with the period value converted into a rate). This prevents issues when the number of cores changes during the JFR recording.
src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 171:
169: double _rate; 170: u8 _period_nanos; 171: bool _is_rate;
Can this be turned into `union`? Only one of `_rate` or `_period_nanos` will ever be set at a time.
good idea ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25775#discussion_r2154397271
On Thu, 12 Jun 2025 09:27:30 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
Fixes the recomputation issue by passing either the rate or the period directly to the sampler (instead of just the rate value with the period value converted into a rate). This prevents issues when the number of cores changes during the JFR recording.
src/jdk.jfr/share/classes/jdk/jfr/internal/util/TimespanRateOrPeriod.java line 37:
35: public static final TimespanRateOrPeriod OFF = new TimespanRateOrPeriod(0, 0, false); 36: 37: public static TimespanRateOrPeriod of(String text) {
Please add the doc about the allowed format for the `text` input ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25775#discussion_r2154403402
On Thu, 12 Jun 2025 09:27:30 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
Fixes the recomputation issue by passing either the rate or the period directly to the sampler (instead of just the rate value with the period value converted into a rate). This prevents issues when the number of cores changes during the JFR recording.
src/jdk.jfr/share/classes/jdk/jfr/internal/PlatformEventType.java line 205:
203: public void setCPUThrottle(TimespanRateOrPeriod rate) { 204: if (isCPUTimeMethodSampling) { 205: System.out.println("Setting CPU throttle for " + getName() + " to " + rate);
Please remove debug printout here and in other places. src/jdk.jfr/share/classes/jdk/jfr/internal/util/TimespanRateOrPeriod.java line 59:
57: } 58: 59: public static TimespanRateOrPeriod max(TimespanRateOrPeriod a, TimespanRateOrPeriod b) {
`max` is not the best name for it, since it can be `min` for period or `max` for rate. The idea of the method is to select a value with the highest resolution, right? Let's reflect this in the name and/or code comment. src/jdk.jfr/share/classes/jdk/jfr/internal/util/TimespanRateOrPeriod.java line 67:
65: } 66: if (a.isRate) { 67: double bRate = Runtime.getRuntime().availableProcessors() / b.periodNanos() * 1_000_000_000.0;
Integer division will round it to zero. Correct expression would be `Runtime.getRuntime().availableProcessors() * (1_000_000_000.0 / b.periodNanos())` src/jdk.jfr/share/classes/jdk/jfr/internal/util/TimespanRateOrPeriod.java line 70:
68: return new TimespanRateOrPeriod(Math.max(a.rate(), bRate), 0, true); 69: } 70: return max(b, a); // swap to use the same logic
Please don't use recursion for trivial cases. src/jdk.jfr/share/classes/jdk/jfr/internal/util/TimespanRateOrPeriod.java line 95:
93: } 94: // fallback to days if no smaller unit is found 95: return String.format("%d/%s", (long)(rate / TimespanUnit.DAYS.nanos * 1_000_000_000.0), TimespanUnit.DAYS.text);
Do we really need this complication? Where is this `toString()` used? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25775#discussion_r2178359999 PR Review Comment: https://git.openjdk.org/jdk/pull/25775#discussion_r2178405607 PR Review Comment: https://git.openjdk.org/jdk/pull/25775#discussion_r2178374195 PR Review Comment: https://git.openjdk.org/jdk/pull/25775#discussion_r2178375571 PR Review Comment: https://git.openjdk.org/jdk/pull/25775#discussion_r2178399206
On Tue, 1 Jul 2025 19:41:43 GMT, Andrei Pangin <apangin@openjdk.org> wrote:
Fixes the recomputation issue by passing either the rate or the period directly to the sampler (instead of just the rate value with the period value converted into a rate). This prevents issues when the number of cores changes during the JFR recording.
src/jdk.jfr/share/classes/jdk/jfr/internal/util/TimespanRateOrPeriod.java line 95:
93: } 94: // fallback to days if no smaller unit is found 95: return String.format("%d/%s", (long)(rate / TimespanUnit.DAYS.nanos * 1_000_000_000.0), TimespanUnit.DAYS.text);
Do we really need this complication? Where is this `toString()` used?
In the CPUThrottleSetting: @Override public String combine(Set<String> values) { TimespanRateOrPeriod max = null; for (String value : values) { TimespanRateOrPeriod rate = TimespanRateOrPeriod.of(value); if (rate != null) { if (max == null) { max = rate; } else { max = TimespanRateOrPeriod.max(max, rate); } } } // "off" is not supported return Objects.requireNonNullElse(max.toString(), DEFAULT_VALUE); } When combining different settings. We have to return a string. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25775#discussion_r2197180848
On Tue, 1 Jul 2025 19:45:41 GMT, Andrei Pangin <apangin@openjdk.org> wrote:
Fixes the recomputation issue by passing either the rate or the period directly to the sampler (instead of just the rate value with the period value converted into a rate). This prevents issues when the number of cores changes during the JFR recording.
src/jdk.jfr/share/classes/jdk/jfr/internal/util/TimespanRateOrPeriod.java line 59:
57: } 58: 59: public static TimespanRateOrPeriod max(TimespanRateOrPeriod a, TimespanRateOrPeriod b) {
`max` is not the best name for it, since it can be `min` for period or `max` for rate. The idea of the method is to select a value with the highest resolution, right? Let's reflect this in the name and/or code comment.
I renamed it "selectHigherResolution" ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25775#discussion_r2197191717
On Thu, 12 Jun 2025 09:27:30 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
Fixes the recomputation issue by passing either the rate or the period directly to the sampler (instead of just the rate value with the period value converted into a rate). This prevents issues when the number of cores changes during the JFR recording.
Why was the class renamed to TimespanRateOrPeriod? I believe the name TimespanRate was chosen to indicate that it could represent both a timespan (e.g., 20 ms) and a rate (e.g., 1000/s). If you think period is more appropriate than timespan, how about RatePeriod? ------------- PR Comment: https://git.openjdk.org/jdk/pull/25775#issuecomment-3025671727
On Thu, 12 Jun 2025 09:27:30 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
Fixes the recomputation issue by passing either the rate or the period directly to the sampler (instead of just the rate value with the period value converted into a rate). This prevents issues when the number of cores changes during the JFR recording.
I totally forgot the old reasoning on the name. I dropped the renaming of the class. ------------- PR Comment: https://git.openjdk.org/jdk/pull/25775#issuecomment-3056693663
Fixes the recomputation issue by passing either the rate or the period directly to the sampler (instead of just the rate value with the period value converted into a rate). This prevents issues when the number of cores changes during the JFR recording.
Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision: Fix smaller issues ------------- Changes: - all: https://git.openjdk.org/jdk/pull/25775/files - new: https://git.openjdk.org/jdk/pull/25775/files/116f8326..781e3dd3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25775&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25775&range=00-01 Stats: 124 lines in 4 files changed: 2 ins; 108 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/25775.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25775/head:pull/25775 PR: https://git.openjdk.org/jdk/pull/25775
Fixes the recomputation issue by passing either the rate or the period directly to the sampler (instead of just the rate value with the period value converted into a rate). This prevents issues when the number of cores changes during the JFR recording.
Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision: Commit missing file ------------- Changes: - all: https://git.openjdk.org/jdk/pull/25775/files - new: https://git.openjdk.org/jdk/pull/25775/files/781e3dd3..ba40ca8f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25775&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25775&range=01-02 Stats: 115 lines in 1 file changed: 115 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/25775.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25775/head:pull/25775 PR: https://git.openjdk.org/jdk/pull/25775
On Thu, 10 Jul 2025 10:18:36 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
Fixes the recomputation issue by passing either the rate or the period directly to the sampler (instead of just the rate value with the period value converted into a rate). This prevents issues when the number of cores changes during the JFR recording.
Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
Commit missing file
Marked as reviewed by jbachorik (Reviewer). ------------- PR Review: https://git.openjdk.org/jdk/pull/25775#pullrequestreview-3016245614
On Thu, 10 Jul 2025 10:18:36 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
Fixes the recomputation issue by passing either the rate or the period directly to the sampler (instead of just the rate value with the period value converted into a rate). This prevents issues when the number of cores changes during the JFR recording.
Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
Commit missing file
Marked as reviewed by mgronlun (Reviewer). ------------- PR Review: https://git.openjdk.org/jdk/pull/25775#pullrequestreview-3019521278
On Thu, 12 Jun 2025 09:27:30 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
Fixes the recomputation issue by passing either the rate or the period directly to the sampler (instead of just the rate value with the period value converted into a rate). This prevents issues when the number of cores changes during the JFR recording.
This pull request has now been integrated. Changeset: c70258ca Author: Johannes Bechberger <jbechberger@openjdk.org> URL: https://git.openjdk.org/jdk/commit/c70258ca1cd074392b5bf844bf6f7b80601f45cc Stats: 208 lines in 10 files changed: 122 ins; 11 del; 75 mod 8358619: Fix interval recomputation in CPU Time Profiler Reviewed-by: jbachorik, mgronlun ------------- PR: https://git.openjdk.org/jdk/pull/25775
participants (5)
-
Andrei Pangin
-
Erik Gahlin
-
Jaroslav Bachorik
-
Johannes Bechberger
-
Markus Grönlund