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 serviceability-dev mailing list