On Wed, 4 Jun 2025 05:26:42 GMT, Johannes Bechberger <jbechber...@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
