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