On Fri, 17 May 2024 09:26:03 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> My main concern here is the addition of clutter (checking two flags, and
>> creating two levels of nested "impl" methods) at every call site. We may
>> need to rearrange our internal API for JFR (jdk.internal.events) in order to
>> clean up the call sites without loading additional classes unnecessarily.
>>
>> I think the main performance comparison to make is the impact on startup
>> time of loading the additional FileReadEvent class. That is, to compare the
>> startup time of the baseline with code that loads the FileReadEvent class,
>> with JFR disabled in both cases. I don't know how to do this. Anyway, if
>> loading of additional classes imposes unacceptable overhead, then that
>> justifies doing more work to avoid it. That's separate from whether adding
>> the `jfrTracing` flag as done in this PR is an effective way to do it.
>
> I think `if (jfrTracing && FileReadEvent.enabled())` would be more readable
> as it would avoid calling going through the traceXXX methods when the flag is
> enabled but the specific event is not enabled.
Collapsing the extra layer of methods and combining the test into
if (jfrTracing && FileReadEvent.enabled())
would indeed keep things a little neater.
I'm still questioning the need for `jfrTracing` at all. There's the matter of
how this boolean gets set and unset, and whether this is done in a way that
comports with the memory model. Setting this aside, is the only benefit that it
avoids loading an extra class at JVM startup time (without JFR)? In this case
that would be the `FileReadEvent` class, which is the stub class in
`jdk.internal.event`. Wouldn't this class be in the CDS archive, making it not
worth the effort of bringing up this new `jfrTracing` mechanism simply to avoid
loading it unnecessarily?
I observe that in JDK 22 some (but not all) of the event classes in
`jdk.internal.event` seem to be present in the CDS archive. These include
various virtual thread events.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1609024695
