RFR: 8366240: Improve memory ordering in new CPU Time Profiler
Remove acq-rel semantics where it isn't needed. ------------- Commit messages: - Remove unnecessary line - Remove redundant synchronization Changes: https://git.openjdk.org/jdk/pull/26960/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=26960&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8366240 Stats: 34 lines in 2 files changed: 5 ins; 9 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/26960.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/26960/head:pull/26960 PR: https://git.openjdk.org/jdk/pull/26960
On Wed, 27 Aug 2025 12:50:13 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
Remove acq-rel semantics where it isn't needed.
src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 169:
167: } 168: if (_capacity < CPU_TIME_QUEUE_MAX_CAPACITY) { 169: float ratio = (float)lost_samples_due_to_queue_full / (float)_capacity;
Can `_capacity` be zero here now? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26960#discussion_r2352999805
On Tue, 16 Sep 2025 16:09:19 GMT, Kerem Kat <krk@openjdk.org> wrote:
Remove acq-rel semantics where it isn't needed.
src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 169:
167: } 168: if (_capacity < CPU_TIME_QUEUE_MAX_CAPACITY) { 169: float ratio = (float)lost_samples_due_to_queue_full / (float)_capacity;
Can `_capacity` be zero here now?
No, because _capacity is always increasing and starts with `20`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26960#discussion_r2353017471
On Wed, 27 Aug 2025 12:50:13 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
Remove acq-rel semantics where it isn't needed.
This looks good. Could you add some code comments, perhaps in the *.h file, explaining why we can afford to do this and don't have to enforce the memory ordering and atomics? ------------- PR Review: https://git.openjdk.org/jdk/pull/26960#pullrequestreview-3238197922
Remove acq-rel semantics where it isn't needed.
Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision: Extend comment ------------- Changes: - all: https://git.openjdk.org/jdk/pull/26960/files - new: https://git.openjdk.org/jdk/pull/26960/files/f8dfe276..fbb21255 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=26960&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=26960&range=00-01 Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/26960.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/26960/head:pull/26960 PR: https://git.openjdk.org/jdk/pull/26960
On Wed, 27 Aug 2025 12:50:13 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
Remove acq-rel semantics where it isn't needed.
Is this enough? ------------- PR Comment: https://git.openjdk.org/jdk/pull/26960#issuecomment-3307283924
Remove acq-rel semantics where it isn't needed.
Johannes Bechberger has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits: - Merge branch 'master' into parttimenerd_reduce_synchronization - Extend comment - Remove unnecessary line - Remove redundant synchronization ------------- Changes: https://git.openjdk.org/jdk/pull/26960/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=26960&range=02 Stats: 41 lines in 2 files changed: 12 ins; 12 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/26960.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/26960/head:pull/26960 PR: https://git.openjdk.org/jdk/pull/26960
Remove acq-rel semantics where it isn't needed.
Johannes Bechberger has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits: - Merge branch 'master' of https://git.openjdk.org/jdk into parttimenerd_reduce_synchronization - Merge branch 'master' into parttimenerd_reduce_synchronization - Extend comment - Remove unnecessary line - Remove redundant synchronization ------------- Changes: https://git.openjdk.org/jdk/pull/26960/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=26960&range=03 Stats: 41 lines in 2 files changed: 12 ins; 12 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/26960.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/26960/head:pull/26960 PR: https://git.openjdk.org/jdk/pull/26960
On Tue, 14 Oct 2025 07:29:41 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
Remove acq-rel semantics where it isn't needed.
Johannes Bechberger has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
- Merge branch 'master' of https://git.openjdk.org/jdk into parttimenerd_reduce_synchronization - Merge branch 'master' into parttimenerd_reduce_synchronization - Extend comment - Remove unnecessary line - Remove redundant synchronization
Marked as reviewed by krk (no project role). ------------- PR Review: https://git.openjdk.org/jdk/pull/26960#pullrequestreview-3334709951
On Tue, 14 Oct 2025 07:29:41 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
Remove acq-rel semantics where it isn't needed.
Johannes Bechberger has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
- Merge branch 'master' of https://git.openjdk.org/jdk into parttimenerd_reduce_synchronization - Merge branch 'master' into parttimenerd_reduce_synchronization - Extend comment - Remove unnecessary line - Remove redundant synchronization
The queue is per-thread private. LGTM ------------- Marked as reviewed by zgu (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/26960#pullrequestreview-3336045379
On Tue, 14 Oct 2025 07:29:41 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
Remove acq-rel semantics where it isn't needed.
Johannes Bechberger has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
- Merge branch 'master' of https://git.openjdk.org/jdk into parttimenerd_reduce_synchronization - Merge branch 'master' into parttimenerd_reduce_synchronization - Extend comment - Remove unnecessary line - Remove redundant synchronization
Marked as reviewed by jbachorik (Reviewer). ------------- PR Review: https://git.openjdk.org/jdk/pull/26960#pullrequestreview-3375130649
On Tue, 14 Oct 2025 07:29:41 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
Remove acq-rel semantics where it isn't needed.
Johannes Bechberger has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
- Merge branch 'master' of https://git.openjdk.org/jdk into parttimenerd_reduce_synchronization - Merge branch 'master' into parttimenerd_reduce_synchronization - Extend comment - Remove unnecessary line - Remove redundant synchronization
This PR seems cause a new test failure [JDK-8370647](https://bugs.openjdk.org/browse/JDK-8370647) ------------- PR Comment: https://git.openjdk.org/jdk/pull/26960#issuecomment-3445413790
On Wed, 27 Aug 2025 12:50:13 GMT, Johannes Bechberger <jbechberger@openjdk.org> wrote:
Remove acq-rel semantics where it isn't needed.
This pull request has now been integrated. Changeset: b7a4c9ce Author: Johannes Bechberger <jbechberger@openjdk.org> URL: https://git.openjdk.org/jdk/commit/b7a4c9ced82717434e43b3f3a0a57083f4005f32 Stats: 41 lines in 2 files changed: 12 ins; 12 del; 17 mod 8366240: Improve memory ordering in new CPU Time Profiler Reviewed-by: jbachorik, krk, zgu ------------- PR: https://git.openjdk.org/jdk/pull/26960
participants (5)
-
Jaroslav Bachorik
-
Johannes Bechberger
-
Kerem Kat
-
SendaoYan
-
Zhengyu Gu