On Wed, 4 Jun 2025 00:13:07 GMT, Andrei Pangin <apan...@openjdk.org> wrote:
>> Johannes Bechberger has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Rename autoadapt
>
> 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?
> src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 281:
>
>> 279: stop_timer();
>> 280: Atomic::store(&_stop_signals, true);
>> 281: while (Atomic::load_acquire(&_active_signal_handlers) > 0) {
>
> There can be a race when `handle_timer_signal` has already passed
> `_stop_signals` check but has not yet incremented `_active_signal_handlers`.
Amy idea on how to fix it?
> src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 472:
>
>> 470:
>> 471: void handle_timer_signal(int signo, siginfo_t* info, void* context) {
>> 472: assert(_instance != nullptr, "invariant");
>
> There can be an arbitrary delay in async signal delivery.
> It's unlikely, but not impossible for `_instance` to be deleted by the time
> signal handler is called. There should be a better way to synchronize with
> JFR shutdown.
Any ideas? Or is it something for a later PR?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2125678084
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2125680345
PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2125681876
