RFR: 8342818: Implement JEP 509: JFR CPU-Time Profiling [v42]
Andrei Pangin
apangin at openjdk.org
Wed Jun 4 11:28:51 UTC 2025
On Wed, 4 Jun 2025 05:26:42 GMT, Johannes Bechberger <jbechberger at openjdk.org> wrote:
>> src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 161:
>>
>>> 159: return 0;
>>> 160: }
>>> 161: return os::active_processor_count() * 1000000000.0 / rate;
>>
>> If sampling period is configured as an absolute number in milliseconds, this value must be passed as is.
>> Double conversion via `Runtime.availableProcessors()` / `active_processor_count()` is unobvious and error-prone. First, because of asymmetry: e.g. `Runtime.availableProcessors()` may be redefined by an agent so that its value is not aligned with `active_processor_count()`. Second, because number of available processors may change at runtime, e.g., by adjusting cgroup quotas.
>
> Is this something for a later PR?
I'm OK with fixing this separately.
>> src/hotspot/share/jfr/support/jfrThreadLocal.cpp line 558:
>>
>>> 556: void JfrThreadLocal::set_cpu_timer(timer_t* timer) {
>>> 557: if (_cpu_timer == nullptr) {
>>> 558: _cpu_timer = JfrCHeapObj::new_array<timer_t>(1);
>>
>> `timer_t` is a primitive type, at most one machine word. Why extra indirection and allocation?
>
> @mgronlun wanted this indirection to move it abstract from implementation details
I don't see how it is an abstraction when the pointer still has concrete `timer_t` type.
All POSIX timer functions accept `timer_t` rather than `timer_t*`.
This is not a big issue, though, just a minor inefficiency.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2126360330
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2126304082
More information about the hotspot-dev
mailing list