RFR: Implement JEP 509: JFR CPU-Time Profiling [v45]

Zhengyu Gu zgu at openjdk.org
Fri May 2 16:07:00 UTC 2025


On Fri, 2 May 2025 12:37:08 GMT, Johannes Bechberger <jbechberger at openjdk.org> wrote:

>> This is the code for the [JEP draft: CPU Time based profiling for JFR].
>> 
>> Currently tested using [this test suite](https://github.com/parttimenerd/basic-profiler-tests).
>> 
>> A version based on the cooperative sampling JEP can be found [here](https://github.com/parttimenerd/jdk/tree/parttimenerd_cooperative_cpu_time_sampler).
>
> Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove assertions

Changes requested by zgu (Reviewer).

src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.hpp line 66:

> 64:       }
> 65:     } while (Atomic::cmpxchg(&_head, elementIndex, elementIndex + 1) != elementIndex);
> 66:     _data[elementIndex] = element;

I think you need `release_store` here at least. But I am questioning the correctness of the implementation. Consider following scenario:
T1: equeue(): after complete CAS successfully, e.g. `_head = 3, elementIndex = 2`
T2: dequeue(): after complete CAS successfully, `_head = 2, elementIndex = 3`
T2: read `_data[--elementIndex]` // _data[2] has yet set 
T1: write `_data[elementIndex] = element` // set _data[2] value

src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.hpp line 75:

> 73:       elementIndex = Atomic::load_acquire(&_head);
> 74:       if (elementIndex == 0) {
> 75:         return NULL;

Use `nullptr` instead.

src/hotspot/share/runtime/handshake.hpp line 133:

> 131: 
> 132:   bool can_run();
> 133:   bool can_run(bool allow_suspend, bool check_async_exception);

You may want to inline these three methods, looks like that they can be on hot pathes.

-------------

PR Review: https://git.openjdk.org/jdk/pull/20752#pullrequestreview-2812012324
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r2071810635
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r2071793767
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r2071612855


More information about the hotspot-dev mailing list