On Sun, 1 Jun 2025 07:26:19 GMT, Johannes Bechberger <jbechber...@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
