On Sat, 11 May 2024 15:00:09 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> A compromise between performance and readability is:
>>
>> if (JFRTracing.isEnabled()) {
>> ...
>> }
>>
>> One additional class is loaded, but it's more clear where it comes from. I
>> didn't want to do that for the ThrowableTracer class since it had a clinit.
>>
>> This could potentially cause problems if JFRTracing is loaded early from
>> Throwable or other class in the future. The static boolean flag is more
>> safe, so probably better.
>
> One thing that isn't clear (to me anyway) is how it works with the memory
> model. It's plain read at the use sites, looks like the set when recording is
> turned on is also a plain write. Would it be possible to summarise what else
> happens when recording is turned on?
During JFR initialization, jfrTracing flag is set to true by the JFRTracing
class. Then, when the recording starts, a safepoint occurs, and all Java
threads are stopped at a poll site. When they wake up and eventually read the
memory of the jfrTracing field, it will be true because of memory fences
related to the safepoint.
I guess it's not 100% safe if the JIT decides to store the read value elsewhere
over several event checks, but it seems unlikely. Event settings checks (i.e.,
Event::isEnabled()) have always used plain reads, so it is not more unreliable
than any other event.
I'm fine with using a volatile too. I used it for the exception events, but I
now think a plain write/read of jfrTracing is safe for all practical purposes.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1597639174
