RFR: 8352738: Implement JEP 520: JFR Method Timing and Tracing

[Markus Grönlund]

On Thu, 22 May 2025 11:20:08 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Could I have a review of this enhancement that will add tracing capabilities to JFR? There are opportunities for performance improvements in the implementation, but I would rather add them later and separately.
>> 
>> Testing: tier 1-3, test/jdk/jdk/jfr
>> 
>> Thanks
>> Erik
>
> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.cpp line 67:
> 
>> 65: 
>> 66: bool JfrTraceIdEpoch::is_synchronizing() {
>> 67:   return Atomic::load_acquire(&_synchronizing);
> 
> This suggests the writes to `_synchronizing` should be releases. One might think `OrderAccess::fence`-s in  `{begin|end}_epoch_shift` and do it, but at least in `begin_epoch_shift`, the fence is misplaced, should be "ops; fence(); releasing-store". Maybe you want to just `Atomic::release_store_fence(&_synchronizing, ...)` them instead.

JfrTraceIdEpoch::is_synchronizing() is a remnant from the old days when we attempted to force CompilerThreads running in native into the correct epoch.

It is unused and can be deleted.

> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.cpp line 81:
> 
>> 79: 
>> 80: bool JfrTraceIdEpoch::has_method_tracer_changed_tag_state() {
>> 81:   return Atomic::load(&_method_tracer_state);
> 
> Checking: sure that no memory ordering is needed here? Sounds like this code consumes some state from another thread when checking this flag.
> 
> 
> +  if (JfrTraceIdEpoch::has_method_tracer_changed_tag_state()) {
> +    do_method_tracer_klasses();
> +  }

Correct, should be Atomic::load_acquire()

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25306#discussion_r2104662406
PR Review Comment: https://git.openjdk.org/jdk/pull/25306#discussion_r2104663266