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

[Aleksey Shipilev]

On Fri, 23 May 2025 13:59:51 GMT, Markus Grönlund <mgronlun at openjdk.org> wrote:

>> 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()

Just so we are on the same page, the stores should be at least `Atomic::release_store`, but better even `Atomic::release_store_fence` to match `Atomic::load_acquire`.

>> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdMacros.hpp line 54:
>> 
>>> 52: #define LEAKP_META_BIT                            (BIT << 2)
>>> 53: #define LEAKP_BIT                                 (LEAKP_META_BIT << META_SHIFT)
>>> 54: #define TIMING_BIT                                LEAKP_BIT // Alias
>> 
>> I guess there a reason to piggyback on leakp bit? Is this necessary? Sounds like there is still space for this bit? I guess it might be confusing when something like `SET_TIMING_BIT` sets `LEAKP_BIT`?
>
> The number of available bits is precious and must be allocated with care.
> 
> Here we have a mutually exclusive situation that allows us to save bits.

Got it, thanks.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25306#discussion_r2104755554
PR Review Comment: https://git.openjdk.org/jdk/pull/25306#discussion_r2104758214