RFR: 8342818: Implement JEP 509: JFR CPU-Time Profiling [v26]
David Holmes
dholmes at openjdk.org
Mon Jun 2 04:35:02 UTC 2025
On Sun, 1 Jun 2025 07:26:19 GMT, Johannes Bechberger <jbechberger at openjdk.org> wrote:
>> This is the code for the [JEP 509: CPU Time based profiling for JFR](https://openjdk.org/jeps/509).
>>
>> Currently tested using [this test suite](https://github.com/parttimenerd/basic-profiler-tests). This runs profiles the [Renaissance](https://renaissance.dev/) benchmark with
>> - ... different heap sizes
>> - ... different GCs
>> - ... different samplers (the standard JFR and the new CPU Time Sampler and both)
>> - ... different JFR recording durations
>> - ... different chunk-sizes
>
> Johannes Bechberger has updated the pull request incrementally with two additional commits since the last revision:
>
> - Refactoring
> - Remove convoluted native trace logic
Just some drive-by comments mainly on your acquire/release usage. I'm not at all clear what memory accesses you are trying to coordinate with those.
src/hotspot/share/jfr/jni/jfrJniMethod.cpp line 176:
> 174: JfrEventSetting::set_enabled(JfrCPUTimeSampleEvent, rate > 0);
> 175: JfrCPUTimeThreadSampling::set_rate(rate, autoadapt == JNI_TRUE);
> 176: return JNI_TRUE;
What is the point of having a boolean return type if you always return true?
src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 59:
> 57: Thread* raw_thread = Thread::current_or_null_safe();
> 58: JavaThread* jt;
> 59: if (raw_thread == nullptr || !raw_thread->is_Java_thread()) { // this can happen due to the high level of parralelism
Suggestion:
if (raw_thread == nullptr || !raw_thread->is_Java_thread()) { // this can happen due to the high level of parallelism
src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 119:
> 117: _data = new_data;
> 118: _capacity = capacity;
> 119: }
I assume there is a lock protecting this so it happens atomically?
src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 122:
> 120:
> 121: bool JfrCPUTimeTraceQueue::is_full() const {
> 122: return Atomic::load_acquire(&_head) >= _capacity;
I don't see why acquire semantics would be needed here. Also how can it be > capacity?
src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 126:
> 124:
> 125: bool JfrCPUTimeTraceQueue::is_empty() const {
> 126: return Atomic::load_acquire(&_head) == 0;
Acquire semantics are definitely not needed here.
src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 130:
> 128:
> 129: s4 JfrCPUTimeTraceQueue::lost_samples() const {
> 130: return Atomic::load_acquire(&_lost_samples);
Again acquire semantics seem highly dubious here - what loads are you synchronizing with?
src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 139:
> 137:
> 138: u4 JfrCPUTimeTraceQueue::get_and_reset_lost_samples() {
> 139: s4 lost_samples = Atomic::load_acquire(&_lost_samples);
Again acquire semantics seem highly dubious here - what loads are you synchronizing with?
src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 151:
> 149: set_capacity(capacity);
> 150: }
> 151: }
Seems an odd definition - typically `ensure_capacity` will grow a data structure to ensure it has sufficient capacity, and if already larger than needed that is fine. Suggestion `change_capacity`, or more traditionally `resize`?
src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 237:
> 235:
> 236: void JfrCPUTimeThreadSampler::trigger_async_processing_of_cpu_time_jfr_requests() {
> 237: Atomic::release_store(&_is_async_processing_of_cpu_time_jfr_requests_triggered, true);
What prior stores are you ensuring should be visible by using release semantics here?
-------------
PR Review: https://git.openjdk.org/jdk/pull/25302#pullrequestreview-2886627655
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2119983062
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2119983911
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2120016607
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2120011705
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2120012200
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2120014449
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2120014541
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2120020174
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2120021034
More information about the hotspot-dev
mailing list